Hello Richard Spiegel, Martin Roth, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32648
to review the following change.
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
soc/amd/stoneyridge: Add IO access functions for PMx
Replace locations in the source that explicitely use the CD6/CD7 index/data pair with utility function calls.
Change-Id: I6e7ba472ef2551e363987d18a79408fcd2074de4 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/stoneyridge/enable_usbdebug.c M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/sb_util.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 57 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32648/1
diff --git a/src/soc/amd/stoneyridge/enable_usbdebug.c b/src/soc/amd/stoneyridge/enable_usbdebug.c index ce84a47..90bd6e8 100644 --- a/src/soc/amd/stoneyridge/enable_usbdebug.c +++ b/src/soc/amd/stoneyridge/enable_usbdebug.c @@ -26,10 +26,7 @@
pci_devfn_t pci_ehci_dbg_dev(unsigned int hcd_idx) { - /* Enable all of the USB controllers */ - outb(PM_USB_ENABLE, PM_INDEX); - outb(PM_USB_ALL_CONTROLLERS, PM_DATA); - + pm_io_write8(PM_USB_ENABLE, PM_USB_ALL_CONTROLLERS); return SOC_EHCI1_DEV; }
diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index f755c0a..618a5de 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -491,6 +491,12 @@ void sb_tpm_decode_spi(void); void lpc_wideio_512_window(uint16_t base); void lpc_wideio_16_window(uint16_t base); +uint8_t pm_io_read8(uint8_t reg); +uint16_t pm_io_read16(uint8_t reg); +uint32_t pm_io_read32(uint8_t reg); +void pm_io_write8(uint8_t reg, uint8_t value); +void pm_io_write16(uint8_t reg, uint16_t value); +void pm_io_write32(uint8_t reg, uint32_t value); u8 pm_read8(u8 reg); u16 pm_read16(u8 reg); u32 pm_read32(u8 reg); diff --git a/src/soc/amd/stoneyridge/sb_util.c b/src/soc/amd/stoneyridge/sb_util.c index 41ebb37..8781617 100644 --- a/src/soc/amd/stoneyridge/sb_util.c +++ b/src/soc/amd/stoneyridge/sb_util.c @@ -18,6 +18,52 @@ #include <arch/acpi.h> #include <soc/southbridge.h>
+/* PM registers are accessed a byte at a time via CD6/CD7 */ +uint8_t pm_io_read8(uint8_t reg) +{ + outb(reg, PM_INDEX); + return inb(PM_DATA); +} + +uint16_t pm_io_read16(uint8_t reg) +{ + int i; + uint16_t value = 0; + for (i = sizeof(value) - 1 ; i >= 0 ; i--) + value = (value << 8) | pm_io_read8(reg + i); + return value; +} + +uint32_t pm_io_read32(uint8_t reg) +{ + uint32_t value = pm_io_read16(reg + sizeof(uint16_t)) << 16; + return value | pm_io_read16(reg); +} + +void pm_io_write8(uint8_t reg, uint8_t value) +{ + outb(reg, PM_INDEX); + outb(value, PM_DATA); +} + +void pm_io_write16(uint8_t reg, uint16_t value) +{ + int i; + for (i = 0 ; i < sizeof(value) ; i++) { + pm_io_write8(reg + i, value & 0xff); + value >>= 8; + } +} + +void pm_io_write32(uint8_t reg, uint32_t value) +{ + int i; + for (i = 0 ; i < sizeof(value) ; i++) { + pm_io_write8(reg + i, value & 0xff); + value >>= 8; + } +} + /* smbus pci read/write - access registers at 0xfed80000 - currently unused */
/* smi read/write - access registers at 0xfed80200 */ diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index b590178..8dfef8b 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -329,11 +329,9 @@ u8 byte;
/* Enable LPC controller */ - outb(PM_LPC_GATING, PM_INDEX); - byte = inb(PM_DATA); + byte = pm_io_read8(PM_LPC_GATING); byte |= PM_LPC_ENABLE; - outb(PM_LPC_GATING, PM_INDEX); - outb(byte, PM_DATA); + pm_io_write8(PM_LPC_GATING, byte);
/* Enable port 80 LPC decode in pci function 3 configuration space. */ byte = pci_read_config8(SOC_LPC_DEV, LPC_IO_OR_MEM_DEC_EN_HIGH); @@ -367,11 +365,9 @@ uint8_t byte;
/* Enable ACPI MMIO range 0xfed80000 - 0xfed81fff */ - outb(PM_ISA_CONTROL, PM_INDEX); - byte = inb(PM_DATA); + byte = pm_io_read8(PM_ISA_CONTROL); byte |= MMIO_EN; - outb(PM_ISA_CONTROL, PM_INDEX); - outb(byte, PM_DATA); + pm_io_write8(PM_ISA_CONTROL, byte); }
static void sb_enable_cf9_io(void)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32648 )
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32648/1/src/soc/amd/stoneyridge/sb_util.c File src/soc/amd/stoneyridge/sb_util.c:
https://review.coreboot.org/#/c/32648/1/src/soc/amd/stoneyridge/sb_util.c@32 PS1, Line 32: for (i = sizeof(value) - 1 ; i >= 0 ; i--) Why are you using a loop here? Why not just something like:
return (pm_io_read8(reg) << 8) | pm_io_read8(reg + 1);
or even something like pm_io_read32()
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32648 )
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32648/1/src/soc/amd/stoneyridge/sb_util.c File src/soc/amd/stoneyridge/sb_util.c:
https://review.coreboot.org/#/c/32648/1/src/soc/amd/stoneyridge/sb_util.c@32 PS1, Line 32: for (i = sizeof(value) - 1 ; i >= 0 ; i--)
Why are you using a loop here? Why not just something like: […]
Only to match the style of biosram_*, although I can't seem to find the discussion history of how we'd settled on that before. I'll change it to what I prefer visually and see if it goes through without objection.
Hello Richard Spiegel, build bot (Jenkins), Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32648
to look at the new patch set (#2).
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
soc/amd/stoneyridge: Add IO access functions for PMx
Replace locations in the source that explicitely use the CD6/CD7 index/data pair with utility function calls.
Change-Id: I6e7ba472ef2551e363987d18a79408fcd2074de4 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/stoneyridge/enable_usbdebug.c M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/sb_util.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 48 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/32648/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32648 )
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32648/1/src/soc/amd/stoneyridge/sb_util.c File src/soc/amd/stoneyridge/sb_util.c:
https://review.coreboot.org/#/c/32648/1/src/soc/amd/stoneyridge/sb_util.c@32 PS1, Line 32: for (i = sizeof(value) - 1 ; i >= 0 ; i--)
Only to match the style of biosram_*, although I can't seem to find the discussion history of how we […]
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32648 )
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
Patch Set 2: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32648 )
Change subject: soc/amd/stoneyridge: Add IO access functions for PMx ......................................................................
soc/amd/stoneyridge: Add IO access functions for PMx
Replace locations in the source that explicitely use the CD6/CD7 index/data pair with utility function calls.
Change-Id: I6e7ba472ef2551e363987d18a79408fcd2074de4 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32648 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/stoneyridge/enable_usbdebug.c M src/soc/amd/stoneyridge/include/soc/southbridge.h M src/soc/amd/stoneyridge/sb_util.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 48 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/enable_usbdebug.c b/src/soc/amd/stoneyridge/enable_usbdebug.c index ce84a47..90bd6e8 100644 --- a/src/soc/amd/stoneyridge/enable_usbdebug.c +++ b/src/soc/amd/stoneyridge/enable_usbdebug.c @@ -26,10 +26,7 @@
pci_devfn_t pci_ehci_dbg_dev(unsigned int hcd_idx) { - /* Enable all of the USB controllers */ - outb(PM_USB_ENABLE, PM_INDEX); - outb(PM_USB_ALL_CONTROLLERS, PM_DATA); - + pm_io_write8(PM_USB_ENABLE, PM_USB_ALL_CONTROLLERS); return SOC_EHCI1_DEV; }
diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h index f755c0a..618a5de 100644 --- a/src/soc/amd/stoneyridge/include/soc/southbridge.h +++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h @@ -491,6 +491,12 @@ void sb_tpm_decode_spi(void); void lpc_wideio_512_window(uint16_t base); void lpc_wideio_16_window(uint16_t base); +uint8_t pm_io_read8(uint8_t reg); +uint16_t pm_io_read16(uint8_t reg); +uint32_t pm_io_read32(uint8_t reg); +void pm_io_write8(uint8_t reg, uint8_t value); +void pm_io_write16(uint8_t reg, uint16_t value); +void pm_io_write32(uint8_t reg, uint32_t value); u8 pm_read8(u8 reg); u16 pm_read16(u8 reg); u32 pm_read32(u8 reg); diff --git a/src/soc/amd/stoneyridge/sb_util.c b/src/soc/amd/stoneyridge/sb_util.c index 41ebb37..524efc4 100644 --- a/src/soc/amd/stoneyridge/sb_util.c +++ b/src/soc/amd/stoneyridge/sb_util.c @@ -18,6 +18,43 @@ #include <arch/acpi.h> #include <soc/southbridge.h>
+/* PM registers are accessed a byte at a time via CD6/CD7 */ +uint8_t pm_io_read8(uint8_t reg) +{ + outb(reg, PM_INDEX); + return inb(PM_DATA); +} + +uint16_t pm_io_read16(uint8_t reg) +{ + return (pm_io_read8(reg + sizeof(uint8_t)) << 8) | pm_io_read8(reg); +} + +uint32_t pm_io_read32(uint8_t reg) +{ + return (pm_io_read16(reg + sizeof(uint16_t)) << 16) | pm_io_read16(reg); +} + +void pm_io_write8(uint8_t reg, uint8_t value) +{ + outb(reg, PM_INDEX); + outb(value, PM_DATA); +} + +void pm_io_write16(uint8_t reg, uint16_t value) +{ + pm_io_write8(reg, value & 0xff); + value >>= 8; + pm_io_write8(reg + sizeof(uint8_t), value & 0xff); +} + +void pm_io_write32(uint8_t reg, uint32_t value) +{ + pm_io_write16(reg, value & 0xffff); + value >>= 16; + pm_io_write16(reg + sizeof(uint16_t), value & 0xffff); +} + /* smbus pci read/write - access registers at 0xfed80000 - currently unused */
/* smi read/write - access registers at 0xfed80200 */ diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index b590178..8dfef8b 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -329,11 +329,9 @@ u8 byte;
/* Enable LPC controller */ - outb(PM_LPC_GATING, PM_INDEX); - byte = inb(PM_DATA); + byte = pm_io_read8(PM_LPC_GATING); byte |= PM_LPC_ENABLE; - outb(PM_LPC_GATING, PM_INDEX); - outb(byte, PM_DATA); + pm_io_write8(PM_LPC_GATING, byte);
/* Enable port 80 LPC decode in pci function 3 configuration space. */ byte = pci_read_config8(SOC_LPC_DEV, LPC_IO_OR_MEM_DEC_EN_HIGH); @@ -367,11 +365,9 @@ uint8_t byte;
/* Enable ACPI MMIO range 0xfed80000 - 0xfed81fff */ - outb(PM_ISA_CONTROL, PM_INDEX); - byte = inb(PM_DATA); + byte = pm_io_read8(PM_ISA_CONTROL); byte |= MMIO_EN; - outb(PM_ISA_CONTROL, PM_INDEX); - outb(byte, PM_DATA); + pm_io_write8(PM_ISA_CONTROL, byte); }
static void sb_enable_cf9_io(void)
-
Marshall Dawson (Code Review) -
Martin Roth (Code Review)