Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
soc/intel/common/block/acpi: Allow additional soc overrides
Make MADT generation weak so that a soc can set a custom MADT. Remove static and add prototypes for functions needed by the soc override.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/1
diff --git a/src/soc/intel/common/block/acpi/acpi.c b/src/soc/intel/common/block/acpi/acpi.c index f0ff898..6c389fa 100644 --- a/src/soc/intel/common/block/acpi/acpi.c +++ b/src/soc/intel/common/block/acpi/acpi.c @@ -33,7 +33,7 @@ return current; }
-static int acpi_sci_irq(void) +int acpi_sci_irq(void) { int sci_irq = 9; uint32_t scis; @@ -82,7 +82,7 @@ return current; }
-unsigned long acpi_fill_madt(unsigned long current) +__weak unsigned long acpi_fill_madt(unsigned long current) { /* Local APICs */ current = acpi_create_madt_lapics(current); @@ -235,7 +235,7 @@ } }
-static int calculate_power(int tdp, int p1_ratio, int ratio) +int calculate_power(int tdp, int p1_ratio, int ratio) { u32 m; u32 power; diff --git a/src/soc/intel/common/block/include/intelblocks/acpi.h b/src/soc/intel/common/block/include/intelblocks/acpi.h index 21664c8..663214d 100644 --- a/src/soc/intel/common/block/include/intelblocks/acpi.h +++ b/src/soc/intel/common/block/include/intelblocks/acpi.h @@ -71,4 +71,8 @@ */ void soc_power_states_generation(int core_id, int cores_per_package);
+int acpi_sci_irq(void); + +int calculate_power(int tdp, int p1_ratio, int ratio); + #endif /* _SOC_INTEL_COMMON_BLOCK_ACPI_H_ */
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#2).
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
soc/intel/common/block/acpi: Allow additional soc overrides
Make MADT generation weak so that a soc can set a custom MADT. Remove static and add prototypes for functions needed by the soc override.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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/xeon_sp/include/soc/acpi.h 3 files changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/2
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 2:
src/soc/intel/xeon_sp/acpi.c:25:12: error: static declaration of 'acpi_sci_irq' follows non-static declaration static int acpi_sci_irq(void) ^~~~~~~~~~~~ In file included from src/soc/intel/xeon_sp/acpi.c:7: src/soc/intel/common/block/include/intelblocks/acpi.h:74:5: note: previous declaration of 'acpi_sci_irq' was here int acpi_sci_irq(void); ^~~~~~~~~~~~ cc1: error: unrecognized command line option '-Wno-address-of-packed-member' [-Werror] cc1: all warnings being treated as errors
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#3).
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
soc/intel/common/block/acpi: Allow additional soc overrides
Make MADT generation weak so that a soc can set a custom MADT. Remove static and add prototypes for functions needed by the soc override.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/3
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#4).
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
soc/intel/common/block/acpi: Allow additional soc overrides
Make MADT generation weak so that a soc can set a custom MADT. Remove static and add prototypes for functions needed by the soc override.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/4
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 4: Code-Review+1
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 4: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current) As usual, I would prefer not to have a weak function with actual content. In the past this has led to confusion and bugs.
What is it that the new implementation would make differently? Could it be handled by a Kconfig switch maybe?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 4: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
As usual, I would prefer not to have a weak function with actual content. […]
The MADT for server platforms is much more complex, and basically does everything differently.
I'm pretty sure I've ranted about it in the past: Xeon-SP is NOT a SoC!
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
The MADT for server platforms is much more complex, and basically does everything differently. […]
A CUSTOM MADT Kconfig would work as well. Maybe that makes more sense in this case. I agree the xeon_sp isn't an soc, but code duplication is also an issue. There is a LOT that is still common and this is the structure we have.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Allow additional soc overrides ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
A CUSTOM MADT Kconfig would work as well. Maybe that makes more sense in this case. […]
Xeon-SP is not a SoC in the sense that it needs a PCH. On the other hand, it is not just a cpu in coreboot terminology (so it does not fit into src/cpu directory).
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#6).
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
soc/intel/common/block/acpi: Add soc MADT override
Add a weak soc MADT function and remove static and add prototypes for functions needed by a soc MADT function.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/6
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
Xeon-SP is not a SoC in the sense that it needs a PCH. […]
I've added and weak soc madt function that should cover Nico's comment while also not adding another Kconfig and a #if.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 6: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
I've added and weak soc madt function that should cover Nico's comment while also not adding another […]
While that is true (the new function is literally empty), it still has the same semantics and effect: One can't easily decide what happens whilst reading the code. I don't understand the whole thing, so I won't block anything.
Questions open for me: * If you want some of the common code but not all of it, why not put it in separate compilation units? * If one acpi_fill_madt() implementation can live in `soc/intel/common` why shouldn't the other?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
While that is true (the new function is literally empty), it still has the […]
Since some code is only common for client platforms, a single Kconfig symbol to omit client-specific code for servers would be good enough IMHO. CB:48509 already does something like this.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 6:
It isn't unreasonable that a soc would want to override the default MADT generation. This make it an explicit call to the soc instead of a weak override. It seems like yet another kconfig isn't that desireable either.
Hello build bot (Jenkins), Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#7).
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
soc/intel/common/block/acpi: Add soc MADT override
Add a SOC_INTEL_COMMON_BLOCK_ACPI_OVERRIDE_MADT Kconfig option and add prototypes for functions needed by a soc MADT function.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/common/block/acpi/Kconfig M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/acpi.h 3 files changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/7
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/4/src/soc/intel/common/block/... PS4, Line 85: __weak unsigned long acpi_fill_madt(unsigned long current)
Since some code is only common for client platforms, a single Kconfig symbol to omit client-specific […]
I've added the Kconfig option.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 7: Code-Review+1
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 7:
Furquan added a comment in a dependency madt patch. Moving the discussion to here: https://review.coreboot.org/c/coreboot/+/48248/8/src/soc/intel/xeon_sp/acpi....
src/soc/intel/xeon_sp/acpi.c Line 99 IIUC, the major difference between the common ACPI implementation of this function and the SoC-specific implementation here is that this implementation adds multiple IOAPIC structures whereas the common implementation adds just one.
Would this work: Use the common ACPI implementation of acpi_fill_madt(), but make a callback into SoC to get IOAPIC info. Probably a structure like this:
struct ioapic_info {
u8 id; u32 addr; u32 gsi_base; }; /* Returns a table of ioapic_info entries and the count of entries in the table */ size_t soc_get_ioapic_info(struct ioapic_info **ioapic_arr);
xeon_sp can implement this function (using exactly the same logic here) to create this table instead of making calls to add_madt_ioapic() directly and that table can be used by the common acpi_fill_madt() to make calls into add_madt_ioapic() as required. Default implementation would be using id = 2, addr = IO_APIC_ADDR, gsi_base = 0 which works for all other SoCs.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT override ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48246/7/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/7/src/soc/intel/common/block/... PS7, Line 94: acpi_create_madt_lapics This is a callback to the soc.
https://review.coreboot.org/c/coreboot/+/48246/7/src/soc/intel/common/block/... PS7, Line 97: current += acpi_create_madt_ioapic((void *)current, 2, IO_APIC_ADDR, 0); Based on Furquan's comment. Make this a weak callback to the soc to return a pointer to a madt table to be filled by this function. The default would remain the current setting. See soc_get_tss_table below as an example.
Hello build bot (Jenkins), Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#8).
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
soc/intel/common/block/acpi: Add soc MADT IOAPIC callback
Add a callback for SOCs to provide an IOAPIC MADT table. If the SOC doesn't provide a table then a standard setting is used. Remove static and add declaration used by soc functions.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 34 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/8
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
Patch Set 8: Code-Review+2
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
Patch Set 8: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
Patch Set 8:
(4 comments)
I'm still not convinced that a weak implementation does anything better than a Kconfig symbol, but I don't mind too much.
https://review.coreboot.org/c/coreboot/+/48246/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48246/8//COMMIT_MSG@7 PS8, Line 7: callback Why call it `callback`, isn't it just a hook? The soc code didn't call, did it?
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 66: calculate_power Please find a more specific name, e.g. soc_calculate_package_power() (I'm guessing it's about the package), it's a global symbol after all.
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 72: __packed Why is this packed? Isn't it more efficient to let the compiler decide?
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 72: acpi_ioapic_t We usually don't do typedefs and the _t suffix is reserved for standard libraries, AFAIK.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
Patch Set 8:
Patch Set 8:
(4 comments)
I'm still not convinced that a weak implementation does anything better than a Kconfig symbol, but I don't mind too much.
I agree, but I didn't get an +2 on the Kconfig.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48246/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48246/8//COMMIT_MSG@7 PS8, Line 7: callback
Why call it `callback`, isn't it just a hook? The soc code didn't call, did it?
I'd been thinking of the soc calling the common apci code, but hook works, too.
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 66: calculate_power
Please find a more specific name, e.g. soc_calculate_package_power() (I'm guessing it's […]
soc_* has been used for functions that hook or call back to the soc specific code. This is a common function. I'll try changing the name to something more specific.
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 72: __packed
Why is this packed? Isn't it more efficient to let the compiler decide?
I was being consistent with acpi_cstate_t and acpi_tstate_t definitions.
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 72: acpi_ioapic_t
We usually don't do typedefs and the _t suffix is reserved for standard libraries, AFAIK.
I was being consistent with acpi_cstate_t and acpi_tstate_t, but they are defined by the acpi library. I thought that the _t was for typedef. I'll change this.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC callback ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 72: acpi_ioapic_t
I was being consistent with acpi_cstate_t and acpi_tstate_t, but they are defined by the acpi libra […]
I have to admit, I didn't know (at least not from the top of my head) that this is a common pattern in ACPI code. It seems to be another corner that doesn't quite look like common coreboot code.
https://review.coreboot.org/c/coreboot/+/48246/8/src/soc/intel/common/block/... PS8, Line 72: __packed
I was being consistent with acpi_cstate_t and acpi_tstate_t definitions.
Ah, interesting. It looks like somebody just copied the pattern by accident from structs that are part of actual ACPI tables and need a specific representation.
Hello build bot (Jenkins), Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#9).
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
soc/intel/common/block/acpi: Add soc MADT IOAPIC hook
Add a hook for SOCs to provide an IOAPIC MADT table. If the SOC doesn't provide a table then a standard setting is used.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 31 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/9
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 9: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 78: struct madt_ioapic_info * This could be `const` to allow implementations with static tables, AFAICS.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 9: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 9: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 91: nit: extra tab not required here and on next line.
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 77: */ nit: It might be helpful to add the information here that if SoC does not provide a custom implementation of this, then the default uses id=2, addr=IO_APIC_ADDR, gsi_base=0
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 78: struct madt_ioapic_info *
This could be `const` to allow implementations with static tables, AFAICS.
+1
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#10).
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
soc/intel/common/block/acpi: Add soc MADT IOAPIC hook
Add a hook for SOCs to provide an IOAPIC MADT table. If the SOC doesn't provide a table then a standard setting is used.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 31 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/10
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48246
to look at the new patch set (#11).
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
soc/intel/common/block/acpi: Add soc MADT IOAPIC hook
Add a hook for SOCs to provide an IOAPIC MADT table. If the SOC doesn't provide a table then a standard setting is used.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 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 2 files changed, 34 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/48246/11
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 91:
nit: extra tab not required here and on next line.
Ack
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 77: */
nit: It might be helpful to add the information here that if SoC does not provide a custom implement […]
I'll mention it uses the default, but not the code since that self documenting and could change and then the comment in the h file will be out of sync...
https://review.coreboot.org/c/coreboot/+/48246/9/src/soc/intel/common/block/... PS9, Line 78: struct madt_ioapic_info *
+1
Yes, good point.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 11: Code-Review+1
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 11: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
Patch Set 11: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48246 )
Change subject: soc/intel/common/block/acpi: Add soc MADT IOAPIC hook ......................................................................
soc/intel/common/block/acpi: Add soc MADT IOAPIC hook
Add a hook for SOCs to provide an IOAPIC MADT table. If the SOC doesn't provide a table then a standard setting is used.
Change-Id: Ic818a634e4912d88ef93971deb4da5ab708c9020 Signed-off-by: Marc Jones marcjones@sysproconsulting.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48246 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Jay Talbott JayTalbott@sysproconsulting.com Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/acpi.h 2 files changed, 34 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve 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 a6c626f..934a92f7 100644 --- a/src/soc/intel/common/block/acpi/acpi.c +++ b/src/soc/intel/common/block/acpi/acpi.c @@ -86,13 +86,34 @@ return current; }
+__weak const struct madt_ioapic_info *soc_get_ioapic_info(size_t *entries) +{ + *entries = 0; + return NULL; +} + unsigned long acpi_fill_madt(unsigned long current) { + const struct madt_ioapic_info *ioapic_table; + size_t ioapic_entries; + /* Local APICs */ current = acpi_create_madt_lapics(current);
/* IOAPIC */ - current += acpi_create_madt_ioapic((void *)current, 2, IO_APIC_ADDR, 0); + ioapic_table = soc_get_ioapic_info(&ioapic_entries); + if (ioapic_entries) { + for (int i = 0; i < ioapic_entries; i++) { + current += acpi_create_madt_ioapic( + (void *)current, + ioapic_table[i].id, + ioapic_table[i].addr, + ioapic_table[i].gsi_base); + } + } else { + /* Default SOC IOAPIC entry */ + current += acpi_create_madt_ioapic((void *)current, 2, IO_APIC_ADDR, 0); + }
return acpi_madt_irq_overrides(current); } diff --git a/src/soc/intel/common/block/include/intelblocks/acpi.h b/src/soc/intel/common/block/include/intelblocks/acpi.h index cd3a309..d76d95a 100644 --- a/src/soc/intel/common/block/include/intelblocks/acpi.h +++ b/src/soc/intel/common/block/include/intelblocks/acpi.h @@ -68,4 +68,16 @@ */ int common_calculate_power_ratio(int tdp, int p1_ratio, int ratio);
+struct madt_ioapic_info { + u8 id; + u32 addr; + u32 gsi_base; +}; + +/* + * Returns a table of MADT ioapic_info entries and the number of entries + * If the SOC doesn't implement this hook a default ioapic setting is used. + */ +const struct madt_ioapic_info *soc_get_ioapic_info(size_t *entries); + #endif /* _SOC_INTEL_COMMON_BLOCK_ACPI_H_ */
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Hung-Te Lin (Code Review)
-
Jay Talbott (Code Review)
-
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)
-
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)