Change in coreboot[master]: [WIP] soc/amd/common/blocks: Drop ACPIMMIO gpio1_ and gpio2_ banks
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: [WIP] soc/amd/common/blocks: Drop ACPIMMIO gpio1_ and gpio2_ banks ......................................................................
[WIP] soc/amd/common/blocks: Drop ACPIMMIO gpio1_ and gpio2_ banks
The banks are one after each other in the ACPIMMIO space. Also there is space for more banks and existing ASL takes advantage of this property.
Change-Id: Ib78559a60b5c20d53a60e1726ee2aad1f38f78ce Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- 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 3 files changed, 0 insertions(+), 66 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/42522/1
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index 60a9efd..c5f82f9 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -22,8 +22,6 @@ DECLARE_ACPIMMIO(acpimmio_misc, MISC); DECLARE_ACPIMMIO(acpimmio_dpvga, DPVGA); DECLARE_ACPIMMIO(acpimmio_gpio0, GPIO0); -DECLARE_ACPIMMIO(acpimmio_gpio1, GPIO1); -DECLARE_ACPIMMIO(acpimmio_gpio2, GPIO2); DECLARE_ACPIMMIO(acpimmio_xhci_pm, XHCIPM); DECLARE_ACPIMMIO(acpimmio_acdc_tmr, ACDCTMR); DECLARE_ACPIMMIO(acpimmio_aoac, AOAC); diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 3d0a17b..3caed11 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -344,68 +344,6 @@ write32(acpimmio_gpio0 + reg, value); }
-/* GPIO bank 1 */ -static inline uint8_t gpio1_read8(uint8_t reg) -{ - return read8(acpimmio_gpio1 + reg); -} - -static inline uint16_t gpio1_read16(uint8_t reg) -{ - return read16(acpimmio_gpio1 + reg); -} - -static inline uint32_t gpio1_read32(uint8_t reg) -{ - return read32(acpimmio_gpio1 + reg); -} - -static inline void gpio1_write8(uint8_t reg, uint8_t value) -{ - write8(acpimmio_gpio1 + reg, value); -} - -static inline void gpio1_write16(uint8_t reg, uint16_t value) -{ - write16(acpimmio_gpio1 + reg, value); -} - -static inline void gpio1_write32(uint8_t reg, uint32_t value) -{ - write32(acpimmio_gpio1 + reg, value); -} - -/* GPIO bank 2 */ -static inline uint8_t gpio2_read8(uint8_t reg) -{ - return read8(acpimmio_gpio2 + reg); -} - -static inline uint16_t gpio2_read16(uint8_t reg) -{ - return read16(acpimmio_gpio2 + reg); -} - -static inline uint32_t gpio2_read32(uint8_t reg) -{ - return read32(acpimmio_gpio2 + reg); -} - -static inline void gpio2_write8(uint8_t reg, uint8_t value) -{ - write8(acpimmio_gpio2 + reg, value); -} - -static inline void gpio2_write16(uint8_t reg, uint16_t value) -{ - write16(acpimmio_gpio2 + reg, value); -} - -static inline void gpio2_write32(uint8_t reg, uint32_t value) -{ - write32(acpimmio_gpio2 + reg, value); -} - static inline uint8_t xhci_pm_read8(uint8_t reg) { return read8(acpimmio_xhci_pm + 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 5d1e881..912e891 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -178,8 +178,6 @@ extern uint8_t *MAYBE_CONST acpimmio_misc; extern uint8_t *MAYBE_CONST acpimmio_dpvga; extern uint8_t *MAYBE_CONST acpimmio_gpio0; -extern uint8_t *MAYBE_CONST acpimmio_gpio1; -extern uint8_t *MAYBE_CONST acpimmio_gpio2; extern uint8_t *MAYBE_CONST acpimmio_xhci_pm; extern uint8_t *MAYBE_CONST acpimmio_acdc_tmr; extern uint8_t *MAYBE_CONST acpimmio_aoac;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Set Ready For Review
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 98: __gpio_and32 There's only one user of this. Just use __gpio_update32 in the gpio_input() func?
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 105: -1UL dirty ;)
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 139: gpio_set(gpio_num, value); I know this is the same ordering as previous code, but should we set the value prior to enabling output to reduce glitches? It can be in a follow up, but I absolutely think we shouldn't have this weird state where output value that is not expected is driven once output enable is set.
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 196: gpio_read32(0); /* Flush posted write */ Is it worth coalescing the posted writes? Or just use gpio_write32_rb() for both?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 98: __gpio_and32
There's only one user of this. […]
I have some other user in followup.
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 105: -1UL
dirty ;)
Do you want 0xfffffffff here? Or avoid using __gpio_update32()?
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 139: gpio_set(gpio_num, value);
I know this is the same ordering as previous code, but should we set the value prior to enabling out […]
Good question. I think pcengines/apu2 incorrectly sets up IOMUX function after setting the pin to output too. But it indeed attemps to avoid any glitches.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 196: gpio_read32(0); /* Flush posted write */
Is it worth coalescing the posted writes? Or just use gpio_write32_rb() for both?
I remember seeing some patch about disabling posted writes for some MMIO region, so need to check if this is needed anymore.
Also I don't like it that picasso forked copy of this function from stoneyridge only because i2c_2_gpi[] array is different, I have a [WIP] followup on this.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 6: Code-Review+1
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... PS6, Line 88: static void __gpio_update32(gpio_t gpio_num, uint32_t mask, uint32_t or) : { : uint32_t reg; : : reg = gpio_read32(gpio_num); : reg &= mask; : reg |= or; : gpio_write32(gpio_num, reg); : } In a different patch I had replaced all the invocations with mem_read_write32: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
Guessing this patch wasn't pushed.
Is your goal to delete mem_read_write32?
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... PS6, Line 98: __ why the __? See https://stackoverflow.com/questions/25090635/use-and-in-c-programs
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio.h:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/in... PS6, Line 372: static inline void gpio_write32_rb(uint8_t gpio_num, uint32_t value) : { : write32(gpio_ctrl_ptr(gpio_num), value); : read32(gpio_ctrl_ptr(gpio_num)); : } : I don't think it's worth while to pollute the public API with this. If you really feel strongly about keeping it, I would add some documentation as to when you would want to use this.
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42522
to look at the new patch set (#8).
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
soc/amd/common: Drop ACPIMMIO GPIO bank separation
The banks are one after each other in the ACPIMMIO space. Also there is space for more banks and existing ASL takes advantage of the property.
Change-Id: Ib78559a60b5c20d53a60e1726ee2aad1f38f78ce Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/include/amdblocks/gpio_banks.h M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h M src/soc/amd/stoneyridge/acpi.c M src/soc/amd/stoneyridge/i2c.c M src/soc/amd/stoneyridge/include/soc/i2c.h 9 files changed, 74 insertions(+), 101 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/42522/8
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... PS6, Line 88: static void __gpio_update32(gpio_t gpio_num, uint32_t mask, uint32_t or) : { : uint32_t reg; : : reg = gpio_read32(gpio_num); : reg &= mask; : reg |= or; : gpio_write32(gpio_num, reg); : }
In a different patch I had replaced all the invocations with mem_read_write32: https://chromium-revi […]
Ack
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... PS6, Line 98: __
why the __? See https://stackoverflow.com/questions/25090635/use-and-in-c-programs […]
Propose another prefix that gets us out of polluting global gpio_. Maybe I have understood incorrectly that __xxx would always be a safe name choice for user implementations, and never defined by compiler or standard includes.
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio.h:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/in... PS6, Line 372: static inline void gpio_write32_rb(uint8_t gpio_num, uint32_t value) : { : write32(gpio_ctrl_ptr(gpio_num), value); : read32(gpio_ctrl_ptr(gpio_num)); : } :
I don't think it's worth while to pollute the public API with this. […]
I agree, see CB:42825. There is also an open question in the follow-ups if this MMIO region now has posted writes disabled.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 8: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... PS6, Line 98: __
Propose another prefix that gets us out of polluting global gpio_. […]
You are not polluting the global namespace, just this compilation unit. We can clean it up in a followup.
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio.h:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/in... PS6, Line 372: static inline void gpio_write32_rb(uint8_t gpio_num, uint32_t value) : { : write32(gpio_ctrl_ptr(gpio_num), value); : read32(gpio_ctrl_ptr(gpio_num)); : } :
I agree, see CB:42825. […]
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 8: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 139: gpio_set(gpio_num, value);
Good question. […]
I left a reminder in CB:42521 (apu2: Switch to proper GPIO API) to address this.
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/6/src/soc/amd/common/block/gp... PS6, Line 98: __
You are not polluting the global namespace, just this compilation unit. […]
Let's call this avoiding the crowded global gpio_ namespace then.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 196: gpio_read32(0); /* Flush posted write */
I remember seeing some patch about disabling posted writes for some MMIO region, so need to check if […]
CB:42825 and CB:42689
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
Patch Set 8:
(2 comments)
I think this is ready to merge.
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/common/block/gp... PS5, Line 105: -1UL
Do you want 0xfffffffff here? Or avoid using __gpio_update32()?
We can fix it in a follow up.
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42522/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 196: gpio_read32(0); /* Flush posted write */
CB:42825 and CB:42689
Ack
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42522 )
Change subject: soc/amd/common: Drop ACPIMMIO GPIO bank separation ......................................................................
soc/amd/common: Drop ACPIMMIO GPIO bank separation
The banks are one after each other in the ACPIMMIO space. Also there is space for more banks and existing ASL takes advantage of the property.
Change-Id: Ib78559a60b5c20d53a60e1726ee2aad1f38f78ce Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42522 Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Martin Roth martinroth@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/include/amdblocks/gpio_banks.h M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/i2c.h M src/soc/amd/stoneyridge/acpi.c M src/soc/amd/stoneyridge/i2c.c M src/soc/amd/stoneyridge/include/soc/i2c.h 9 files changed, 74 insertions(+), 101 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, but someone else must approve Furquan Shaikh: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index d67e2d0..9125308 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -82,79 +82,60 @@
uintptr_t gpio_get_address(gpio_t gpio_num) { - uintptr_t gpio_address; + return (uintptr_t)gpio_ctrl_ptr(gpio_num); +}
- if (gpio_num < 64) - gpio_address = GPIO_BANK0_CONTROL(gpio_num); - else if (gpio_num < 128) - gpio_address = GPIO_BANK1_CONTROL(gpio_num); - else - gpio_address = GPIO_BANK2_CONTROL(gpio_num); +static void __gpio_update32(gpio_t gpio_num, uint32_t mask, uint32_t or) +{ + uint32_t reg;
- return gpio_address; + reg = gpio_read32(gpio_num); + reg &= mask; + reg |= or; + gpio_write32(gpio_num, reg); +} + +static void __gpio_and32(gpio_t gpio_num, uint32_t mask) +{ + __gpio_update32(gpio_num, mask, 0); +} + +static void __gpio_or32(gpio_t gpio_num, uint32_t or) +{ + __gpio_update32(gpio_num, -1UL, or); }
int gpio_get(gpio_t gpio_num) { uint32_t reg; - uintptr_t gpio_address = gpio_get_address(gpio_num);
- reg = read32((void *)gpio_address); - + reg = gpio_read32(gpio_num); return !!(reg & GPIO_PIN_STS); }
void gpio_set(gpio_t gpio_num, int value) { - uint32_t reg; - uintptr_t gpio_address = gpio_get_address(gpio_num); - - reg = read32((void *)gpio_address); - reg &= ~GPIO_OUTPUT_MASK; - reg |= !!value << GPIO_OUTPUT_SHIFT; - write32((void *)gpio_address, reg); + __gpio_update32(gpio_num, ~GPIO_OUTPUT_MASK, !!value << GPIO_OUTPUT_SHIFT); }
void gpio_input_pulldown(gpio_t gpio_num) { - uint32_t reg; - uintptr_t gpio_address = gpio_get_address(gpio_num); - - reg = read32((void *)gpio_address); - reg &= ~GPIO_PULLUP_ENABLE; - reg |= GPIO_PULLDOWN_ENABLE; - write32((void *)gpio_address, reg); + __gpio_update32(gpio_num, ~GPIO_PULLUP_ENABLE, GPIO_PULLDOWN_ENABLE); }
void gpio_input_pullup(gpio_t gpio_num) { - uint32_t reg; - uintptr_t gpio_address = gpio_get_address(gpio_num); - - reg = read32((void *)gpio_address); - reg &= ~GPIO_PULLDOWN_ENABLE; - reg |= GPIO_PULLUP_ENABLE; - write32((void *)gpio_address, reg); + __gpio_update32(gpio_num, ~GPIO_PULLDOWN_ENABLE, GPIO_PULLUP_ENABLE); }
void gpio_input(gpio_t gpio_num) { - uint32_t reg; - uintptr_t gpio_address = gpio_get_address(gpio_num); - - reg = read32((void *)gpio_address); - reg &= ~GPIO_OUTPUT_ENABLE; - write32((void *)gpio_address, reg); + __gpio_and32(gpio_num, ~GPIO_OUTPUT_ENABLE); }
void gpio_output(gpio_t gpio_num, int value) { - uint32_t reg; - uintptr_t gpio_address = gpio_get_address(gpio_num); - - reg = read32((void *)gpio_address); - reg |= GPIO_OUTPUT_ENABLE; - write32((void *)gpio_address, reg); + __gpio_or32(gpio_num, GPIO_OUTPUT_ENABLE); gpio_set(gpio_num, value); }
@@ -210,7 +191,7 @@
soc_gpio_hook(gpio, mux);
- gpio_ptr = (uint32_t *)gpio_get_address(gpio); + gpio_ptr = gpio_ctrl_ptr(gpio);
if (control_flags & GPIO_SPECIAL_FLAG) { gevent_num = get_gpio_gevent(gpio, gev_tbl, gev_items); @@ -276,13 +257,12 @@
int gpio_interrupt_status(gpio_t gpio) { - uintptr_t gpio_address = gpio_get_address(gpio); - uint32_t reg = read32((void *)gpio_address); + uint32_t reg = gpio_read32(gpio);
if (reg & GPIO_INT_STATUS) { /* Clear interrupt status, preserve wake status */ reg &= ~GPIO_WAKE_STATUS; - write32((void *)gpio_address, reg); + gpio_write32(gpio, reg); return 1; }
diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index d3deff1..2775b52 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -353,6 +353,28 @@ }
/* New GPIO banks configuration registers */ + +static inline void *gpio_ctrl_ptr(uint8_t gpio_num) +{ + return acpimmio_gpio0 + gpio_num * sizeof(uint32_t); +} + +static inline uint32_t gpio_read32(uint8_t gpio_num) +{ + return read32(gpio_ctrl_ptr(gpio_num)); +} + +static inline void gpio_write32(uint8_t gpio_num, uint32_t value) +{ + write32(gpio_ctrl_ptr(gpio_num), value); +} + +static inline void gpio_write32_rb(uint8_t gpio_num, uint32_t value) +{ + write32(gpio_ctrl_ptr(gpio_num), value); + read32(gpio_ctrl_ptr(gpio_num)); +} + /* GPIO bank 0 */ static inline uint8_t gpio0_read8(uint8_t reg) { diff --git a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h index f4288aa..da65e08 100644 --- a/src/soc/amd/common/block/include/amdblocks/gpio_banks.h +++ b/src/soc/amd/common/block/include/amdblocks/gpio_banks.h @@ -5,7 +5,6 @@
#include <stdint.h> #include <stddef.h> -#include <amdblocks/acpimmio.h>
struct soc_amd_gpio { uint8_t gpio; @@ -19,10 +18,6 @@ uint8_t event; };
-#define GPIO_BANK0_CONTROL(gpio) ((uintptr_t)acpimmio_gpio0 + ((gpio) * 4)) -#define GPIO_BANK1_CONTROL(gpio) ((uintptr_t)acpimmio_gpio1 + (((gpio) - 64) * 4)) -#define GPIO_BANK2_CONTROL(gpio) ((uintptr_t)acpimmio_gpio2 + (((gpio) - 128) * 4)) - #define GPIO_MASTER_SWITCH 0xFC #define GPIO_MASK_STS_EN BIT(28) #define GPIO_INTERRUPT_EN BIT(30) diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index c868c34..75509eb 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -302,7 +302,7 @@ " %d\n", gpio_num, SOC_GPIO_TOTAL_PINS); return -1; } - uintptr_t addr = (uintptr_t) gpio_get_address(gpio_num); + uintptr_t addr = gpio_get_address(gpio_num);
acpigen_soc_get_gpio_in_local5(addr);
@@ -332,7 +332,7 @@ " %d\n", gpio_num, SOC_GPIO_TOTAL_PINS); return -1; } - uintptr_t addr = (uintptr_t) gpio_get_address(gpio_num); + uintptr_t addr = gpio_get_address(gpio_num);
/* Store (0x40, Local0) */ acpigen_write_store(); diff --git a/src/soc/amd/picasso/i2c.c b/src/soc/amd/picasso/i2c.c index 6f34573..881278f 100644 --- a/src/soc/amd/picasso/i2c.c +++ b/src/soc/amd/picasso/i2c.c @@ -154,23 +154,16 @@ static void save_i2c_pin_registers(uint8_t gpio, struct soc_amd_i2c_save *save_table) { - uint32_t *gpio_ptr; - - gpio_ptr = (uint32_t *)gpio_get_address(gpio); save_table->mux_value = iomux_read8(gpio); - save_table->control_value = read32(gpio_ptr); + save_table->control_value = gpio_read32(gpio); }
static void restore_i2c_pin_registers(uint8_t gpio, struct soc_amd_i2c_save *save_table) { - uint32_t *gpio_ptr; - - gpio_ptr = (uint32_t *)gpio_get_address(gpio); iomux_write8(gpio, save_table->mux_value); iomux_read8(gpio); - write32(gpio_ptr, save_table->control_value); - read32(gpio_ptr); + gpio_write32_rb(gpio, save_table->control_value); }
/* Slaves to be reset are controlled by devicetree register i2c_scl_reset */ @@ -196,19 +189,19 @@ */ for (j = 0; j < 9; j++) { if (control & GPIO_I2C2_SCL) - write32((uint32_t *)GPIO_I2C2_ADDRESS, GPIO_SCL_LOW); + gpio_write32(I2C2_SCL_PIN, GPIO_OUTPUT_ENABLE); if (control & GPIO_I2C3_SCL) - write32((uint32_t *)GPIO_I2C3_ADDRESS, GPIO_SCL_LOW); + gpio_write32(I2C3_SCL_PIN, GPIO_OUTPUT_ENABLE);
- read32((uint32_t *)GPIO_I2C3_ADDRESS); /* Flush posted write */ + gpio_read32(0); /* Flush posted write */ udelay(4); /* 4usec gets 85KHz for 1 pin, 70KHz for 4 pins */
if (control & GPIO_I2C2_SCL) - write32((uint32_t *)GPIO_I2C2_ADDRESS, GPIO_SCL_HIGH); + gpio_write32(I2C2_SCL_PIN, 0); if (control & GPIO_I2C3_SCL) - write32((uint32_t *)GPIO_I2C3_ADDRESS, GPIO_SCL_HIGH); + gpio_write32(I2C3_SCL_PIN, 0);
- read32((uint32_t *)GPIO_I2C3_ADDRESS); /* Flush posted write */ + gpio_read32(0); /* Flush posted write */ udelay(4); }
diff --git a/src/soc/amd/picasso/include/soc/i2c.h b/src/soc/amd/picasso/include/soc/i2c.h index 58c27a4..34c19aa 100644 --- a/src/soc/amd/picasso/include/soc/i2c.h +++ b/src/soc/amd/picasso/include/soc/i2c.h @@ -17,11 +17,6 @@ #define I2C2_SCL_PIN GPIO_113 #define I2C3_SCL_PIN GPIO_19
-#define GPIO_I2C2_ADDRESS GPIO_BANK1_CONTROL(I2C2_SCL_PIN) -#define GPIO_I2C3_ADDRESS GPIO_BANK0_CONTROL(I2C3_SCL_PIN) -#define GPIO_SCL_HIGH 0 -#define GPIO_SCL_LOW GPIO_OUTPUT_ENABLE - #define I2C2_SCL_PIN_IOMUX_GPIOxx GPIO_113_IOMUX_GPIOxx #define I2C3_SCL_PIN_IOMUX_GPIOxx GPIO_19_IOMUX_GPIOxx
diff --git a/src/soc/amd/stoneyridge/acpi.c b/src/soc/amd/stoneyridge/acpi.c index ffead50..19dee79 100644 --- a/src/soc/amd/stoneyridge/acpi.c +++ b/src/soc/amd/stoneyridge/acpi.c @@ -277,7 +277,7 @@ " %d\n", gpio_num, SOC_GPIO_TOTAL_PINS); return -1; } - uintptr_t addr = (uintptr_t) gpio_get_address(gpio_num); + uintptr_t addr = gpio_get_address(gpio_num);
acpigen_soc_get_gpio_in_local5(addr);
@@ -307,7 +307,7 @@ " %d\n", gpio_num, SOC_GPIO_TOTAL_PINS); return -1; } - uintptr_t addr = (uintptr_t) gpio_get_address(gpio_num); + uintptr_t addr = gpio_get_address(gpio_num);
/* Store (0x40, Local0) */ acpigen_write_store(); diff --git a/src/soc/amd/stoneyridge/i2c.c b/src/soc/amd/stoneyridge/i2c.c index 8667d92..0327028 100644 --- a/src/soc/amd/stoneyridge/i2c.c +++ b/src/soc/amd/stoneyridge/i2c.c @@ -137,23 +137,16 @@ static void save_i2c_pin_registers(uint8_t gpio, struct soc_amd_i2c_save *save_table) { - uint32_t *gpio_ptr; - - gpio_ptr = (uint32_t *)gpio_get_address(gpio); save_table->mux_value = iomux_read8(gpio); - save_table->control_value = read32(gpio_ptr); + save_table->control_value = gpio_read32(gpio); }
static void restore_i2c_pin_registers(uint8_t gpio, struct soc_amd_i2c_save *save_table) { - uint32_t *gpio_ptr; - - gpio_ptr = (uint32_t *)gpio_get_address(gpio); iomux_write8(gpio, save_table->mux_value); iomux_read8(gpio); - write32(gpio_ptr, save_table->control_value); - read32(gpio_ptr); + gpio_write32_rb(gpio, save_table->control_value); }
/* Slaves to be reset are controlled by devicetree register i2c_scl_reset */ @@ -182,27 +175,27 @@ */ for (j = 0; j < 9; j++) { if (control & GPIO_I2C0_SCL) - write32((uint32_t *)GPIO_I2C0_ADDRESS, GPIO_SCL_LOW); + gpio_write32(I2C0_SCL_PIN, GPIO_OUTPUT_ENABLE); if (control & GPIO_I2C1_SCL) - write32((uint32_t *)GPIO_I2C1_ADDRESS, GPIO_SCL_LOW); + gpio_write32(I2C1_SCL_PIN, GPIO_OUTPUT_ENABLE); if (control & GPIO_I2C2_SCL) - write32((uint32_t *)GPIO_I2C2_ADDRESS, GPIO_SCL_LOW); + gpio_write32(I2C2_SCL_PIN, GPIO_OUTPUT_ENABLE); if (control & GPIO_I2C3_SCL) - write32((uint32_t *)GPIO_I2C3_ADDRESS, GPIO_SCL_LOW); + gpio_write32(I2C3_SCL_PIN, GPIO_OUTPUT_ENABLE);
- read32((uint32_t *)GPIO_I2C3_ADDRESS); /* Flush posted write */ + gpio_read32(0); /* Flush posted write */ udelay(4); /* 4usec gets 85KHz for 1 pin, 70KHz for 4 pins */
if (control & GPIO_I2C0_SCL) - write32((uint32_t *)GPIO_I2C0_ADDRESS, GPIO_SCL_HIGH); + gpio_write32(I2C0_SCL_PIN, 0); if (control & GPIO_I2C1_SCL) - write32((uint32_t *)GPIO_I2C1_ADDRESS, GPIO_SCL_HIGH); + gpio_write32(I2C1_SCL_PIN, 0); if (control & GPIO_I2C2_SCL) - write32((uint32_t *)GPIO_I2C2_ADDRESS, GPIO_SCL_HIGH); + gpio_write32(I2C2_SCL_PIN, 0); if (control & GPIO_I2C3_SCL) - write32((uint32_t *)GPIO_I2C3_ADDRESS, GPIO_SCL_HIGH); + gpio_write32(I2C3_SCL_PIN, 0);
- read32((uint32_t *)GPIO_I2C3_ADDRESS); /* Flush posted write */ + gpio_read32(0); /* Flush posted write */ udelay(4); }
diff --git a/src/soc/amd/stoneyridge/include/soc/i2c.h b/src/soc/amd/stoneyridge/include/soc/i2c.h index 874f7d1..844ff1b 100644 --- a/src/soc/amd/stoneyridge/include/soc/i2c.h +++ b/src/soc/amd/stoneyridge/include/soc/i2c.h @@ -21,11 +21,6 @@ #define I2C2_SCL_PIN GPIO_113 #define I2C3_SCL_PIN GPIO_19
-#define GPIO_I2C0_ADDRESS GPIO_BANK2_CONTROL(I2C0_SCL_PIN) -#define GPIO_I2C1_ADDRESS GPIO_BANK2_CONTROL(I2C1_SCL_PIN) -#define GPIO_I2C2_ADDRESS GPIO_BANK1_CONTROL(I2C2_SCL_PIN) -#define GPIO_I2C3_ADDRESS GPIO_BANK0_CONTROL(I2C3_SCL_PIN) - #define I2C0_SCL_PIN_IOMUX_GPIOxx GPIO_145_IOMUX_GPIOxx #define I2C1_SCL_PIN_IOMUX_GPIOxx GPIO_147_IOMUX_GPIOxx #define I2C2_SCL_PIN_IOMUX_GPIOxx GPIO_113_IOMUX_GPIOxx
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)