Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
soc/amd/common/block/lpc: Add lpc_initialize_spi_bar()
This change adds helper function lpc_initialize_spi_bar() which sets MMIO base for SPI controller and ROM enable bits. This is equivalent to renaming of lpc_set_spibase() to lpc_initialize_spi_bar(). Additionally, lpc_set_spibase() is updated to just set the MMIO base for SPI controller. This split is done to allow setting of MMIO base independent of ROM enable bits. On platforms like Picasso, eSPI base is determined by the same register and hence eSPI can set the BAR without having to touch the enable bits.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3f270ba1745b4bb8a403f00cd069a02e21d444be --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/common/block/lpc/lpc_util.c M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 36 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/41247/1
diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h index d7a455a..c915f30 100644 --- a/src/soc/amd/common/block/include/amdblocks/lpc.h +++ b/src/soc/amd/common/block/include/amdblocks/lpc.h @@ -179,6 +179,19 @@ int lpc_set_wideio_range(uint16_t start, uint16_t size);
uintptr_t lpc_get_spibase(void); -void lpc_set_spibase(uint32_t base, uint32_t enable); + +/* + * Sets MMIO base address for SPI controller and eSPI controller (if supported by platform). + * + * eSPI base = SPI base + 0x10000 + */ +void lpc_set_spibase(uint32_t base); + +/* + * Initializes SPI base addres register which includes the following two components: + * base = MMIO base for SPI controller and eSPI controller (if supported by platform). + * enable = ROM enable (SPI_ROM_ENABLE, SPI_ROM_ALT_ENABLE) + */ +void lpc_initialize_spi_bar(uint32_t base, uint32_t enable);
#endif /* __AMDBLOCKS_LPC_H__ */ diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c index 97ef17c..cc9ed48 100644 --- a/src/soc/amd/common/block/lpc/lpc_util.c +++ b/src/soc/amd/common/block/lpc/lpc_util.c @@ -322,19 +322,35 @@ return (uintptr_t)base; }
-void lpc_set_spibase(u32 base, u32 enable) +void lpc_set_spibase(uint32_t base) { - u32 reg32; + uint32_t reg32; + + reg32 = pci_read_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER); + + reg32 &= SPI_BASE_ALIGNMENT - 1; /* preserve only reserved, enables */ + reg32 |= ALIGN_DOWN(base, SPI_BASE_ALIGNMENT); + + pci_write_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER, reg32); +} + +static void lpc_set_rom_enable(uint32_t enable) +{ + uint32_t reg32;
/* only two types of CS# enables are allowed */ enable &= SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE;
reg32 = pci_read_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER);
- reg32 &= SPI_BASE_ALIGNMENT - 1; /* preserve only reserved, enables */ reg32 &= ~(SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE); reg32 |= enable; - reg32 |= ALIGN_DOWN(base, SPI_BASE_ALIGNMENT);
pci_write_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER, reg32); } + +void lpc_initialize_spi_bar(uint32_t base, uint32_t enable) +{ + lpc_set_spibase(base); + lpc_set_rom_enable(enable); +} diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 73ab03b..6d6a00d 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -207,7 +207,7 @@ if (base) return base;
- lpc_set_spibase(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); + lpc_initialize_spi_bar(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); return SPI_BASE_ADDRESS; }
diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index ccdedf4..acaeddb 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -265,7 +265,7 @@ if (base) return base;
- lpc_set_spibase(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); + lpc_initialize_spi_bar(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); return SPI_BASE_ADDRESS; }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/1/src/soc/amd/common/block/in... PS1, Line 191: * Initializes SPI base addres register which includes the following two components: 'addres' may be misspelled - perhaps 'address'?
Hello Raul Rangel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41247
to look at the new patch set (#2).
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
soc/amd/common/block/lpc: Add lpc_initialize_spi_bar()
This change adds helper function lpc_initialize_spi_bar() which sets MMIO base for SPI controller and ROM enable bits. This is equivalent to renaming of lpc_set_spibase() to lpc_initialize_spi_bar(). Additionally, lpc_set_spibase() is updated to just set the MMIO base for SPI controller. This split is done to allow setting of MMIO base independent of ROM enable bits. On platforms like Picasso, eSPI base is determined by the same register and hence eSPI can set the BAR without having to touch the enable bits.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3f270ba1745b4bb8a403f00cd069a02e21d444be --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/common/block/lpc/lpc_util.c M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 36 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/41247/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/1/src/soc/amd/common/block/in... PS1, Line 191: * Initializes SPI base addres register which includes the following two components:
'addres' may be misspelled - perhaps 'address'?
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... PS3, Line 195: lpc_initialize_spi_bar Why are there two ways of setting the spi base? How about dropping the base param and renaming this to lpc_enable_spi_rom(uint32_t enable)?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... PS3, Line 195: lpc_initialize_spi_bar
Why are there two ways of setting the spi base? How about dropping the base param and renaming this […]
I kept it this way so that the existing callers can just use lpc_initialize_spi_bar() to do the same action as being done right now. If you think having lpc_enable_spi_rom() instead of this function is cleaner, I can update this. I had started out with lpc_enable_spi_rom(), but then switched to lpc_initialize_spi_bar() when actually updating the callers.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... PS3, Line 195: lpc_initialize_spi_bar
I kept it this way so that the existing callers can just use lpc_initialize_spi_bar() to do the same […]
I think it would be cleaner. As someone who wants to use this API I need to stop and wonder why I would use lpc_set_spibase() over lpc_initialize_spi_bar(..., 0), or if there is something preventing me from calling lpc_initialize_spi_bar(..., 1) from the very beginning.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Add lpc_initialize_spi_bar() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... PS3, Line 195: lpc_initialize_spi_bar
I think it would be cleaner. […]
Sounds good. I will split this up.
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41247
to look at the new patch set (#4).
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions
This change splits lpc_set_spibase() into two separate functions: lpc_set_spibase() - Sets MMIO base address for SPI controller and eSPI controller (if supported by platforms) lpc_enable_spi_rom() - Enables SPI ROM
This split is done to allow setting of MMIO base independent of ROM enable bits. On platforms like Picasso, eSPI base is determined by the same register and hence eSPI can set the BAR without having to touch the enable bits.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3f270ba1745b4bb8a403f00cd069a02e21d444be --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/common/block/lpc/lpc_util.c M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 30 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/41247/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/3/src/soc/amd/common/block/in... PS3, Line 195: lpc_initialize_spi_bar
Sounds good. I will split this up.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4: Code-Review+2
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/4/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/4/src/soc/amd/common/block/in... PS4, Line 186: * eSPI base = SPI base + 0x10000 I'm not sure I follow why eSPI base is commented here?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/4/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/4/src/soc/amd/common/block/in... PS4, Line 186: * eSPI base = SPI base + 0x10000
I'm not sure I follow why eSPI base is commented here?
Just to show the relation between SPI and eSPI MMIO base.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41247/4/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/41247/4/src/soc/amd/common/block/in... PS4, Line 186: * eSPI base = SPI base + 0x10000
Just to show the relation between SPI and eSPI MMIO base.
Done
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41247 )
Change subject: soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions ......................................................................
soc/amd/common/block/lpc: Split lpc_set_spibase() into two functions
This change splits lpc_set_spibase() into two separate functions: lpc_set_spibase() - Sets MMIO base address for SPI controller and eSPI controller (if supported by platforms) lpc_enable_spi_rom() - Enables SPI ROM
This split is done to allow setting of MMIO base independent of ROM enable bits. On platforms like Picasso, eSPI base is determined by the same register and hence eSPI can set the BAR without having to touch the enable bits.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3f270ba1745b4bb8a403f00cd069a02e21d444be Reviewed-on: https://review.coreboot.org/c/coreboot/+/41247 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/common/block/lpc/lpc_util.c M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 30 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Felix Held: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h index d7a455a..00210a7 100644 --- a/src/soc/amd/common/block/include/amdblocks/lpc.h +++ b/src/soc/amd/common/block/include/amdblocks/lpc.h @@ -179,6 +179,15 @@ int lpc_set_wideio_range(uint16_t start, uint16_t size);
uintptr_t lpc_get_spibase(void); -void lpc_set_spibase(uint32_t base, uint32_t enable); + +/* + * Sets MMIO base address for SPI controller and eSPI controller (if supported by platform). + * + * eSPI base = SPI base + 0x10000 + */ +void lpc_set_spibase(uint32_t base); + +/* Enable SPI ROM (SPI_ROM_ENABLE, SPI_ROM_ALT_ENABLE) */ +void lpc_enable_spi_rom(uint32_t enable);
#endif /* __AMDBLOCKS_LPC_H__ */ diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c index 97ef17c..c9786e7 100644 --- a/src/soc/amd/common/block/lpc/lpc_util.c +++ b/src/soc/amd/common/block/lpc/lpc_util.c @@ -322,19 +322,29 @@ return (uintptr_t)base; }
-void lpc_set_spibase(u32 base, u32 enable) +void lpc_set_spibase(uint32_t base) { - u32 reg32; + uint32_t reg32; + + reg32 = pci_read_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER); + + reg32 &= SPI_BASE_ALIGNMENT - 1; /* preserve only reserved, enables */ + reg32 |= ALIGN_DOWN(base, SPI_BASE_ALIGNMENT); + + pci_write_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER, reg32); +} + +void lpc_enable_spi_rom(uint32_t enable) +{ + uint32_t reg32;
/* only two types of CS# enables are allowed */ enable &= SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE;
reg32 = pci_read_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER);
- reg32 &= SPI_BASE_ALIGNMENT - 1; /* preserve only reserved, enables */ reg32 &= ~(SPI_ROM_ENABLE | SPI_ROM_ALT_ENABLE); reg32 |= enable; - reg32 |= ALIGN_DOWN(base, SPI_BASE_ALIGNMENT);
pci_write_config32(_LPCB_DEV, SPIROM_BASE_ADDRESS_REGISTER, reg32); } diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 73ab03b..6308953 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -207,7 +207,9 @@ if (base) return base;
- lpc_set_spibase(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); + lpc_set_spibase(SPI_BASE_ADDRESS); + lpc_enable_spi_rom(SPI_ROM_ENABLE); + return SPI_BASE_ADDRESS; }
diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index ccdedf4..e90fe1b 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -265,7 +265,9 @@ if (base) return base;
- lpc_set_spibase(SPI_BASE_ADDRESS, SPI_ROM_ENABLE); + lpc_set_spibase(SPI_BASE_ADDRESS); + lpc_enable_spi_rom(SPI_ROM_ENABLE); + return SPI_BASE_ADDRESS; }
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Eric Peers (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Raul Rangel (Code Review)