Change in coreboot[master]: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function
According to BKDG's for families 15h and 16h the ACPI MMIO decode enable bit is the second LSB not the first LSB.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f --- M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37178/1
diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h index 755af52..d636432 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -28,7 +28,8 @@ * newer method. */ #define ACPIMMIO_DECODE_REGISTER 0x4 -#define ACPIMMIO_DECODE_EN BIT(0) +#define BIOSRAM_DECODE_EN BIT(0) +#define ACPIMMIO_DECODE_EN BIT(1)
/* MMIO register blocks are at fixed offsets from 0xfed80000 and are enabled * in PMx24[1] (older implementations) and PMx04[1] (newer implementations).
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG@7 PS1, Line 7: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function *the* can be dropped to make the git commit message summary shorter.
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG@11 PS1, Line 11: Any user visible bug that is fixed by this (TEST=)?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function ......................................................................
Patch Set 1:
(1 comment)
This is clearly fixing a bug (although it doesn't seem to affect us). The build is breaking on a couple of boards that don't use this code so it must be an earlier change upsetting Jenkins. If you rebase this one by itself onto origin/master, it's a very easy +2 & submit.
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG@9 PS1, Line 9: BKDG's Nit: BKDGs, however I realize an apostrophe when pluralizing abbreviations is more acceptable.
Hello Kyösti Mälkki, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37178
to look at the new patch set (#3).
Change subject: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function
According to BKDGs for families 15h and 16h the ACPI MMIO decode enable bit is the second LSB, not the first LSB.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f --- M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37178/3
Hello Kyösti Mälkki, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37178
to look at the new patch set (#4).
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function
According to BKDGs for families 15h and 16h the ACPI MMIO decode enable bit is the second LSB, not the first LSB.
It does not seem to impact any current board, but may be crucial for incoming C bootblock implementations when this bit will need to be set very early. Most likely this bit is set by AGESA right now.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f --- M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37178/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG@7 PS1, Line 7: soc/amd/common/block/acpimmio: fix the ACPIMMIO decode enable function
*the* can be dropped to make the git commit message summary shorter.
Done
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG@9 PS1, Line 9: BKDG's
Nit: BKDGs, however I realize an apostrophe when pluralizing abbreviations is more acceptable.
Done
https://review.coreboot.org/c/coreboot/+/37178/1//COMMIT_MSG@11 PS1, Line 11:
Any user visible bug that is fixed by this (TEST=)?
Binary blobs seem to enable this bit, but for early enablement, for example C bootblock implementations, it may be required. Added this info to commit message.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 5: Code-Review-2
(1 comment)
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... PS5, Line 26: * enable the decode in PMx24 instead. All discrete FCHs and the Kabini Doesn't the comment here specifically say fam14-15tn-16kn do not use ACPIMMIO_DECODE_REGISTER 0x4 and the bits specified below?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... PS5, Line 26: * enable the decode in PMx24 instead. All discrete FCHs and the Kabini
Doesn't the comment here specifically say fam14-15tn-16kn do not use ACPIMMIO_DECODE_REGISTER 0x4 an […]
Only SB800 uses PM register 0x24. However, that southbridge has its own early implementation of decode_enable thus not using the enable function provided by this common block. I wonder if this is the correct place to put CONFIG() macros and separate the implementation
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... PS5, Line 26: * enable the decode in PMx24 instead. All discrete FCHs and the Kabini
Only SB800 uses PM register 0x24. […]
See latest CB:37401 comments
Hello Kyösti Mälkki, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37178
to look at the new patch set (#6).
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function
According to BKDGs for families 15h and 16h the ACPI MMIO decode enable bit is the second LSB, not the first LSB.
It does not seem to impact any current board, but may be crucial for incoming C bootblock implementations when this bit will need to be set very early. Most likely this bit is set by AGESA right now.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f --- M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 1 file changed, 10 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37178/6
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/5/src/soc/amd/common/block/in... PS5, Line 26: * enable the decode in PMx24 instead. All discrete FCHs and the Kabini
See latest CB:37401 comments
Ohh right... Did not grep enough and have lacking knowledge in platforms codenames. Anyway, the change do not break anything at its current state. Will apply a conditional definitions for the discrete FCH's
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... PS8, Line 37: I think it's just better to create two functions; acpimmio_decode_enable_pm04 and _pm24.
I believe amd/lamar, from pi/hudson, requires PMx24 too. So you need to differentiate this inside southbridge_early_init().
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... PS8, Line 39: * in PMx24[1] (older implementations) and PMx04[1] (newer implementations). PMx24[0] ?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... PS8, Line 39: * in PMx24[1] (older implementations) and PMx04[1] (newer implementations).
PMx24[0] ?
The comment is wrong. According to RRGs bit 0 enables the blocks (default disable) and bit 1 configures the access mode (IO mapped - 1; or default memory mapped - 0). Will update it.
Hello Kyösti Mälkki, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37178
to look at the new patch set (#9).
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function
According to BKDGs for families 15h and 16h the ACPI MMIO decode enable bit is the second LSB, not the first LSB.
It does not seem to impact any current board, but may be crucial for incoming C bootblock implementations when this bit will need to be set very early. Most likely this bit is set by AGESA right now.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 5 files changed, 32 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37178/9
Hello Kyösti Mälkki, Marshall Dawson, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37178
to look at the new patch set (#10).
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function
According to BKDGs for families 15h 60-6fh or newer and families 16h the ACPI MMIO decode enable bit is the second LSB, not the first LSB.
Additionally create another enable function for older families where the register and bit is different.
It does not seem to impact any current board, but may be crucial for incoming C bootblock implementations when this bit will need to be set very early. Most likely this bit is set by AGESA right now.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 5 files changed, 33 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37178/10
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... PS8, Line 37:
I think it's just better to create two functions; acpimmio_decode_enable_pm04 and _pm24. […]
Done in subsequent patches. It seems that starting from family 15h processors 60-6fh the right register is PMx04. So bolton needs to use PMx24
https://review.coreboot.org/c/coreboot/+/37178/8/src/soc/amd/common/block/in... PS8, Line 39: * in PMx24[1] (older implementations) and PMx04[1] (newer implementations).
The comment is wrong. […]
Done
Kyösti Mälkki has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Removed Code-Review-2 by Kyösti Mälkki kyosti.malkki@gmail.com
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 10: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
Patch Set 11: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37178 )
Change subject: soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function ......................................................................
soc/amd/common/block/acpimmio: fix ACPIMMIO decode enable function
According to BKDGs for families 15h 60-6fh or newer and families 16h the ACPI MMIO decode enable bit is the second LSB, not the first LSB.
Additionally create another enable function for older families where the register and bit is different.
It does not seem to impact any current board, but may be crucial for incoming C bootblock implementations when this bit will need to be set very early. Most likely this bit is set by AGESA right now.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Iaa31abc3dbdf77d8513fa83c7415b9a1b7fd266f Reviewed-on: https://review.coreboot.org/c/coreboot/+/37178 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 5 files changed, 33 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index 04d5e4a..a589ef5 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -18,13 +18,22 @@ #include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
-void enable_acpimmio_decode(void) +void enable_acpimmio_decode_pm24(void) { uint32_t dw;
- dw = pm_io_read32(ACPIMMIO_DECODE_REGISTER); - dw |= ACPIMMIO_DECODE_EN; - pm_io_write32(ACPIMMIO_DECODE_REGISTER, dw); + dw = pm_io_read32(ACPIMMIO_DECODE_REGISTER_24); + dw |= PM_24_ACPIMMIO_DECODE_EN; + pm_io_write32(ACPIMMIO_DECODE_REGISTER_24, dw); +} + +void enable_acpimmio_decode_pm04(void) +{ + uint32_t dw; + + dw = pm_io_read32(ACPIMMIO_DECODE_REGISTER_04); + dw |= PM_04_ACPIMMIO_DECODE_EN; + pm_io_write32(ACPIMMIO_DECODE_REGISTER_04, dw); }
/* PM registers are accessed a byte at a time via CD6/CD7 */ diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 57d24db..c441ab8 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -101,7 +101,12 @@ */
/* Enable the AcpiMmio range at 0xfed80000 */ -void enable_acpimmio_decode(void); + +/* For older discrete FCHs */ +void enable_acpimmio_decode_pm24(void); + +/* For newer integrated FCHs */ +void enable_acpimmio_decode_pm04(void);
/* Access PM registers using IO cycles */ uint8_t pm_io_read8(uint8_t reg); diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h index 755af52..9a15840 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -22,16 +22,21 @@ #define PM_INDEX 0xcd6 #define PM_DATA 0xcd7
-/* TODO: In the event this is ported backward far enough, earlier devices - * enable the decode in PMx24 instead. All discrete FCHs and the Kabini - * SoC fall into this category. Kabini's successor, Mullins, uses this - * newer method. +/* Earlier devices enable the decode in PMx24 instead. All discrete FCHs and + * the Kabini SoC fall into this category. Kabini's successor, Mullins, uses + * this newer method. */ -#define ACPIMMIO_DECODE_REGISTER 0x4 -#define ACPIMMIO_DECODE_EN BIT(0) + +#define ACPIMMIO_DECODE_REGISTER_24 0x24 +#define PM_24_ACPIMMIO_DECODE_EN BIT(0) + +#define ACPIMMIO_DECODE_REGISTER_04 0x4 +#define PM_04_BIOSRAM_DECODE_EN BIT(0) +#define PM_04_ACPIMMIO_DECODE_EN BIT(1) +
/* MMIO register blocks are at fixed offsets from 0xfed80000 and are enabled - * in PMx24[1] (older implementations) and PMx04[1] (newer implementations). + * in PMx24[0] (older implementations) and PMx04[1] (newer implementations). * PM registers are also accessible via IO CD6/CD7. * * All products do not support all blocks below, however AMD has avoided diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index fe801d4..041d262 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -278,7 +278,7 @@ sb_disable_4dw_burst(); sb_set_spi100(SPI_SPEED_33M, SPI_SPEED_33M, SPI_SPEED_16M, SPI_SPEED_16M); - enable_acpimmio_decode(); + enable_acpimmio_decode_pm04(); fch_smbus_init(); sb_enable_cf9_io(); sb_enable_legacy_io(); diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index 8556790..85c7eaf 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -399,7 +399,7 @@ lpc_enable_spi_prefetch(); sb_init_spi_base(); sb_disable_4dw_burst(); /* Must be disabled on CZ(ST) */ - enable_acpimmio_decode(); + enable_acpimmio_decode_pm04(); fch_smbus_init(); sb_enable_cf9_io(); setup_spread_spectrum(&reboot);
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)