Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
soc/intel/common/block/lpc: Include correct acpi.h
If ACPI is configured, use the correct acpi.h include path.
Change-Id: I0590a028b11f34e423d8f0007e0653037b0849a0 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/common/block/lpc/lpc.c 1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/48251/1
diff --git a/src/soc/intel/common/block/lpc/lpc.c b/src/soc/intel/common/block/lpc/lpc.c index d11aa10..37b4e04 100644 --- a/src/soc/intel/common/block/lpc/lpc.c +++ b/src/soc/intel/common/block/lpc/lpc.c @@ -4,10 +4,17 @@ #include <device/device.h> #include <device/pci.h> #include <device/pci_ids.h> -#include <intelblocks/acpi.h> #include <intelblocks/lpc_lib.h> #include <soc/pm.h>
+#if CONFIG(HAVE_ACPI_TABLES) +#if CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI) +#include <intelblocks/acpi.h> +#else +#include <soc/acpi.h> +#endif /* HAVE_ACPI_TABLES */ +#endif /* SOC_INTEL_COMMON_ACPI */ + /* SoC overrides */
/* Common weak definition, needs to be implemented in each soc LPC driver. */
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/1/src/soc/intel/common/block/... PS1, Line 16: #endif /* SOC_INTEL_COMMON_ACPI */ These comments are reversed, aren't they?
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/1/src/soc/intel/common/block/... PS1, Line 16: #endif /* SOC_INTEL_COMMON_ACPI */
These comments are reversed, aren't they?
Ack
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48251
to look at the new patch set (#2).
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
soc/intel/common/block/lpc: Include correct acpi.h
If ACPI is configured, use the correct acpi.h include path.
Change-Id: I0590a028b11f34e423d8f0007e0653037b0849a0 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/common/block/lpc/lpc.c 1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/48251/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h> It is not clear to me why soc/acpi.h is required. I think intelblocks/acpi.h is included for the declaration of southbridge_write_acpi_tables. Is that creating a problem for some platform?
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3: Code-Review+1
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
It is not clear to me why soc/acpi.h is required. I think intelblocks/acpi. […]
Yes, that is the problem. It should pick up the soc/*/include/soc/acpi.h where the soc acpi functions are defined and it doesn't use the common block acpi. A later change to intelblocks/acpi.h has issues with duplicate defines . See soc/skylake as an example. https://qa.coreboot.org/job/coreboot-gerrit/154009/testReport/junit/board/ch...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
Yes, that is the problem. It should pick up the soc/*/include/soc/acpi. […]
I know it's not the best, but would it make sense to drop the redundant declarations in skylake SoC and make it use intelblocks/acpi.h? Not sure if that breaks more things. Ideally, skylake should be moved to enable SOC_INTEL_COMMON_BLOCK_ACPI. But, as a quick fix, I think pushing the change in a particular SoC might be better than adding #ifdefs in common block code.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
I know it's not the best, but would it make sense to drop the redundant declarations in skylake SoC […]
I don't think that using common block defines if the common block isn't use is correct. There are other dependencies across the common block that are similar to this and this change seems fairly minor.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
I don't think that using common block defines if the common block isn't use is correct. […]
Humm.. I think long term we should do one of the following: 1. Clean up SKL to enable SOC_INTEL_COMMON_BLOCK_ACPI 2. Move the declaration of functions that are required by this driver into its corresponding header file i.e. southbridge_write_acpi_tables()is expected to be implemented if using this driver. In that case, it should be declared by intel/common/block/lpc.h. What actually implements these functions doesn't matter then - it could be SoC specific implementation or common/block/acpi implementation.
I think #2 might not be too difficult to do right now too. Also, I think we need to be careful about using other common blocks in any given common block since we want to provide flexibility to each SoC to decide what common drivers it wants to use. Here the use of common/block/acpi breaks this flexibility for skl/xeon_sp as you pointed out. Given that #2 sounds like a better approach to me. Thoughts?
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
Humm.. I think long term we should do one of the following: […]
#2 doesn't seem correct either and could make the common block stuff very confusing if the header is not with the source. There is a lot of dependencies in the common code and they are all #if CONFIG and/or if CONFIG blocks. COMMON LPC expects southbridge_write_acpi_tables(), but it could be called by and soc that doesn't use common LPC (but there aren't any, currently).
I understand your point that it is not explicitly which header file is picked up with soc/acpi.h, but it is widely used in the soc/intel socs and you would expect it to pick up the soc it is building. It would be no more confusing than the southbridge_write_acpi_tables() being in lpc.h. *shrug*
It seems that this is the simplest solution, but will accept something that moves this forward.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
#2 doesn't seem correct either and could make the common block stuff very confusing if the header is […]
Sure. I don't want to block you on this. If there is something that needs to be used by more than this driver, then it should have a common place -- under arch, lib or drivers. I would say that it is inherently wrong that lpc is using southbridge_write_acpi_tables. There should be a specific device for that rather than tagging along with LPC. I know the scope grows much more than what this is intending.
Given that southbridge_write_acpi_tables() is used only by LPC, it seems fine to me that it is declared in lpc.h. When/if we ever get to cleaning it properly, we can move the declaration and the call to the appropriate device.
I don't really have any other better ideas which do not involve more work. If you plan to keep it this way, I think it would be good to add a comment here explaining why this is being done so that it provides some context on how it can be cleaned up as well.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
Sure. I don't want to block you on this. […]
I'll try to update the patch to move the define for southbridge_write_acpi_tables() to lpc.h. It looks like it will mess it up more.
Since there isn't a single southbridge device, LPC make sense as the default to use.
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48251
to look at the new patch set (#4).
Change subject: soc/intel/common/block/lpc: Move southbridge_write_acpi_tables declaration ......................................................................
soc/intel/common/block/lpc: Move southbridge_write_acpi_tables declaration
Move the southbridge_write_acpi_tables declaration from acpi.h to common lpc_lib.h, as common LPC is always the caller. This removes a duplicate declaration since all soc/intel devices use common LPC, but not all use common ACPI. The southbridge_write_acpi_tables function is defined in acpi.c with the other acpi functions.
Note that this would have the reverse problem if there is ever a non-common LPC device.
Change-Id: I0590a028b11f34e423d8f0007e0653037b0849a0 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/acpi.h M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc.c M src/soc/intel/skylake/include/soc/acpi.h M src/soc/intel/xeon_sp/acpi.c M src/soc/intel/xeon_sp/include/soc/acpi.h 7 files changed, 9 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/48251/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Move southbridge_write_acpi_tables declaration ......................................................................
Patch Set 4: Code-Review+2
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Move southbridge_write_acpi_tables declaration ......................................................................
Patch Set 4: Code-Review+1
Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Move southbridge_write_acpi_tables declaration ......................................................................
soc/intel/common/block/lpc: Move southbridge_write_acpi_tables declaration
Move the southbridge_write_acpi_tables declaration from acpi.h to common lpc_lib.h, as common LPC is always the caller. This removes a duplicate declaration since all soc/intel devices use common LPC, but not all use common ACPI. The southbridge_write_acpi_tables function is defined in acpi.c with the other acpi functions.
Note that this would have the reverse problem if there is ever a non-common LPC device.
Change-Id: I0590a028b11f34e423d8f0007e0653037b0849a0 Signed-off-by: Marc Jones marcjones@sysproconsulting.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48251 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Jay Talbott JayTalbott@sysproconsulting.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/acpi.h M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc.c M src/soc/intel/skylake/include/soc/acpi.h M src/soc/intel/xeon_sp/acpi.c M src/soc/intel/xeon_sp/include/soc/acpi.h 7 files changed, 9 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Jay Talbott: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/common/block/acpi/acpi.c b/src/soc/intel/common/block/acpi/acpi.c index 4efeb88..2948faf 100644 --- a/src/soc/intel/common/block/acpi/acpi.c +++ b/src/soc/intel/common/block/acpi/acpi.c @@ -13,6 +13,7 @@ #include <cpu/intel/common/common.h> #include <cpu/x86/smm.h> #include <intelblocks/acpi.h> +#include <intelblocks/lpc_lib.h> #include <intelblocks/msr.h> #include <intelblocks/pmclib.h> #include <intelblocks/uart.h> diff --git a/src/soc/intel/common/block/include/intelblocks/acpi.h b/src/soc/intel/common/block/include/intelblocks/acpi.h index 21664c8..d058a5d 100644 --- a/src/soc/intel/common/block/include/intelblocks/acpi.h +++ b/src/soc/intel/common/block/include/intelblocks/acpi.h @@ -24,14 +24,6 @@ void soc_write_sci_irq_select(uint32_t scis);
/* - * Calls acpi_write_hpet which creates and fills HPET table and - * adds it to the RSDT (and XSDT) structure. - */ -unsigned long southbridge_write_acpi_tables(const struct device *device, - unsigned long current, - struct acpi_rsdp *rsdp); - -/* * get_cstate_map returns a table of processor specific acpi_cstate_t entries * and number of entries in the table */ diff --git a/src/soc/intel/common/block/include/intelblocks/lpc_lib.h b/src/soc/intel/common/block/include/intelblocks/lpc_lib.h index b04df76..2fdcdef 100644 --- a/src/soc/intel/common/block/include/intelblocks/lpc_lib.h +++ b/src/soc/intel/common/block/include/intelblocks/lpc_lib.h @@ -115,5 +115,12 @@ * 2. Disable NMI sources */ void pch_misc_init(void); +/* + * Calls acpi_write_hpet which creates and fills HPET table and + * adds it to the RSDT (and XSDT) structure. + */ +unsigned long southbridge_write_acpi_tables(const struct device *device, + unsigned long current, + struct acpi_rsdp *rsdp);
#endif /* _SOC_COMMON_BLOCK_LPC_LIB_H_ */ diff --git a/src/soc/intel/common/block/lpc/lpc.c b/src/soc/intel/common/block/lpc/lpc.c index d11aa10..fc24b0a 100644 --- a/src/soc/intel/common/block/lpc/lpc.c +++ b/src/soc/intel/common/block/lpc/lpc.c @@ -4,7 +4,6 @@ #include <device/device.h> #include <device/pci.h> #include <device/pci_ids.h> -#include <intelblocks/acpi.h> #include <intelblocks/lpc_lib.h> #include <soc/pm.h>
diff --git a/src/soc/intel/skylake/include/soc/acpi.h b/src/soc/intel/skylake/include/soc/acpi.h index bf81afb..5f209a7 100644 --- a/src/soc/intel/skylake/include/soc/acpi.h +++ b/src/soc/intel/skylake/include/soc/acpi.h @@ -13,8 +13,6 @@ #define PSS_LATENCY_BUSMASTER 10
unsigned long acpi_madt_irq_overrides(unsigned long current); -unsigned long southbridge_write_acpi_tables(const struct device *device, - unsigned long current, struct acpi_rsdp *rsdp); unsigned long northbridge_write_acpi_tables(const struct device *, unsigned long current, struct acpi_rsdp *);
diff --git a/src/soc/intel/xeon_sp/acpi.c b/src/soc/intel/xeon_sp/acpi.c index 4fe8b20..4c17eea 100644 --- a/src/soc/intel/xeon_sp/acpi.c +++ b/src/soc/intel/xeon_sp/acpi.c @@ -7,6 +7,7 @@ #include <device/pci.h> #include <cbmem.h> #include <cpu/x86/smm.h> +#include <intelblocks/lpc_lib.h> #include <soc/acpi.h> #include <soc/cpu.h> #include <soc/intel/common/acpi.h> diff --git a/src/soc/intel/xeon_sp/include/soc/acpi.h b/src/soc/intel/xeon_sp/include/soc/acpi.h index 2f22c62..743251b 100644 --- a/src/soc/intel/xeon_sp/include/soc/acpi.h +++ b/src/soc/intel/xeon_sp/include/soc/acpi.h @@ -19,8 +19,6 @@
unsigned long northbridge_write_acpi_tables(const struct device *device, unsigned long current, struct acpi_rsdp *rsdp); -unsigned long southbridge_write_acpi_tables(const struct device *device, - unsigned long current, struct acpi_rsdp *rsdp); uint32_t soc_read_sci_irq_select(void); int soc_madt_sci_irq_polarity(int sci); void soc_power_states_generation(int core, int cores_per_package);
-
Furquan Shaikh (Code Review) -
Jay Talbott (Code Review)
-
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)