Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Remove gpio1_ and gpio2_ references ......................................................................
mb/pcengines/apu2: Remove gpio1_ and gpio2_ references
The banks are one after each other in the ACPIMMIO space. Also there is space for more banks and existing ASL also takes advantage of this property.
Change-Id: I348ba43a76287be5b24012ae3dfc28ed783da9c7 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/mainboard/pcengines/apu2/gpio_ftns.c 1 file changed, 8 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/42521/1
diff --git a/src/mainboard/pcengines/apu2/gpio_ftns.c b/src/mainboard/pcengines/apu2/gpio_ftns.c index c249c2d..03eddce 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.c +++ b/src/mainboard/pcengines/apu2/gpio_ftns.c @@ -10,27 +10,20 @@ static u32 gpio_read_wrapper(u32 iomux_gpio) { u32 gpio = iomux_gpio << 2; + if (gpio >= 0x300) + die("Invalid GPIO");
- if (gpio < 0x100) - return gpio0_read32(gpio & 0xff); - else if (gpio >= 0x100 && gpio < 0x200) - return gpio1_read32(gpio & 0xff); - else if (gpio >= 0x200 && gpio < 0x300) - return gpio2_read32(gpio & 0xff); + return gpio0_read32(gpio & 0x3ff);
- die("Invalid GPIO"); }
static void gpio_write_wrapper(u32 iomux_gpio, u32 setting) { u32 gpio = iomux_gpio << 2; + if (gpio >= 0x300) + die("Invalid GPIO");
- if (gpio < 0x100) - gpio0_write32(gpio & 0xff, setting); - else if (gpio >= 0x100 && gpio < 0x200) - gpio1_write32(gpio & 0xff, setting); - else if (gpio >= 0x200 && gpio < 0x300) - gpio2_write32(gpio & 0xff, setting); + gpio0_write32(gpio & 0x3ff, setting); }
void configure_gpio(u32 gpio, u8 iomux_ftn, u32 setting) @@ -70,9 +63,9 @@ * One SPD file contains all 4 options, determine which index to * read here, then call into the standard routines. */ - if (gpio1_read8(0x02) & BIT0) + if (read_gpio(GPIO_49)) index |= BIT0; - if (gpio1_read8(0x06) & BIT0) + if (read_gpio(GPIO_50)) index |= BIT1;
return index;
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Remove gpio1_ and gpio2_ references ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 13: if (gpio >= 0x300) What is w/ this 0x300 check and how does it correspond 0x300? Seems we shouldn't be open coding any of this as the soc code should be bounds checking. If anything the bounds check should be on iomux_gpio. And really this code shouldn't be in here at all.
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 16: return gpio0_read32(gpio & 0x3ff); if a single namespace for gpios is intended to be used then gpioX_read() should be dropped entirely instead of implicitly assuming the memory map is contiguous.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Remove gpio1_ and gpio2_ references ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 13: if (gpio >= 0x300)
What is w/ this 0x300 check and how does it correspond 0x300? Seems we shouldn't be open coding any […]
There are 3 GPIO banks, each occupies the 0x100 bytes in the APCI MMIO space. This bounds check was intended to not read higher memory than the GPIO2 bank.
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 16: return gpio0_read32(gpio & 0x3ff);
if a single namespace for gpios is intended to be used then gpioX_read() should be dropped entirely […]
It is all about assuming that the GPIO reading functions will be used sanely by the developer.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Remove gpio1_ and gpio2_ references ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 13: if (gpio >= 0x300)
What is w/ this 0x300 check and how does it correspond 0x300? Seems we shouldn't be open coding any […]
I agree, but boundary checking should be enforced by SOC code.
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 16: return gpio0_read32(gpio & 0x3ff);
if a single namespace for gpios is intended to be used then gpioX_read() should be dropped entirely […]
There are sane and insane developers, you know :)
But really, the idea of exposing the entire GPIO ACPIMMIO memory space without any abstraction is not an API to follow.
Hello build bot (Jenkins), Michał Żygowski, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42521
to look at the new patch set (#2).
Change subject: [WIP] mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
[WIP] mb/pcengines/apu2: Switch to proper GPIO API
This reverts commit 87f9fc8584c980dc4c73667f4c88d71d0e447a0c.
The banks are one after each other in the ACPIMMIO space. Also there is space for more banks and existing ASL also takes advantage of this property.
Change-Id: I348ba43a76287be5b24012ae3dfc28ed783da9c7 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/mainboard/pcengines/apu2/gpio_ftns.c M src/mainboard/pcengines/apu2/gpio_ftns.h M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/romstage.c 4 files changed, 23 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/42521/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: [WIP] mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42521/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42521/2//COMMIT_MSG@13 PS2, Line 13: takes advantage of this property. I forgot to change the text here, and also did not push the dependencies now. The point is we already have some existing GPIO API we should utilise instead of going directly into the ACPIMMIO GPIO memory space under mb/ code.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: [WIP] mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42521/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42521/2//COMMIT_MSG@13 PS2, Line 13: takes advantage of this property.
I forgot to change the text here, and also did not push the dependencies now. […]
Please let me know when you have the patch stack in order the way you like. I can then review further.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Set Ready For Review
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 16: return gpio0_read32(gpio & 0x3ff);
There are sane and insane developers, you know :) […]
Yes, sorry. As before in my other comments it was about maintaining proper API and abstractions. Anything in src/mainboard shouldn't be adding code to maintaining the boundary conditions of the chipset. We should provide the proper abstractions in chipset code to make mainboard ports easier. I'll try to be more clear in the future, but I'd really like to remove most of this chipset specific code from mainboard and provide the semantics desired from mainboard.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 13: if (gpio >= 0x300)
I agree, but boundary checking should be enforced by SOC code.
Ack
https://review.coreboot.org/c/coreboot/+/42521/1/src/mainboard/pcengines/apu... PS1, Line 16: return gpio0_read32(gpio & 0x3ff);
Yes, sorry. As before in my other comments it was about maintaining proper API and abstractions. […]
Ack
https://review.coreboot.org/c/coreboot/+/42521/7/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/7/src/mainboard/pcengines/apu... PS7, Line 36: gpio_output(gpio, setting & GPIO_OUTPUT_VALUE); To avoid glitches ?
gpio_output(gpio, gpio_input(gpio)); gpio_output(gpio, setting & GPIO_OUTPUT_VALUE);
Below, iomux_write() below might have to move too.
Or maybe remove this configure_gpio() altogether and call program_gpios().
Hello build bot (Jenkins), Michał Żygowski, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42521
to look at the new patch set (#8).
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
mb/pcengines/apu2: Switch to proper GPIO API
Use the abstractions <gpio.h> provides.
Change-Id: I348ba43a76287be5b24012ae3dfc28ed783da9c7 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/mainboard/pcengines/apu2/gpio_ftns.c M src/mainboard/pcengines/apu2/gpio_ftns.h M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/romstage.c 4 files changed, 49 insertions(+), 100 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/42521/8
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42521/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42521/2//COMMIT_MSG@13 PS2, Line 13: takes advantage of this property.
Please let me know when you have the patch stack in order the way you like. […]
Ack
https://review.coreboot.org/c/coreboot/+/42521/7/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/7/src/mainboard/pcengines/apu... PS7, Line 36: gpio_output(gpio, setting & GPIO_OUTPUT_VALUE);
To avoid glitches ? […]
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 43: gpio_output(gpio, 0); Address this sequence in gpio_banks/gpio.c:gpio_output() instead.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 11: Code-Review+2
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Restored
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 11:
haven't looked at the details, but i like where this is moving
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42521/13/src/mainboard/pcengines/ap... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/13/src/mainboard/pcengines/ap... PS13, Line 14: GPIO_49 did you verify that this is the correct gpio number? haven't looked in the datasheet, but from the datasheets of picasso, renoir and cezanne i expected this to be gpio 64 and the next one gpio 65
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42521/13/src/mainboard/pcengines/ap... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/13/src/mainboard/pcengines/ap... PS13, Line 14: GPIO_49
did you verify that this is the correct gpio number? haven't looked in the datasheet, but from the d […]
This SoC carried two enumerations for GPIOs in the docs, making it confusing indeed.
52740 Rev 3.06 - March 18, 2016
Table 303: Function name for GPIOx[1FC:100:step4] Register Reset Function GPIOx100 0000_0000h GPIO49_AGPIO64 GPIOx104 0014_0000h GPIO50_AGPIO65
Same bit, read8 GPIOx102 bit 0 vs read32 GPIOx100 bit 16.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42521/13/src/mainboard/pcengines/ap... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/13/src/mainboard/pcengines/ap... PS13, Line 14: GPIO_49
This SoC carried two enumerations for GPIOs in the docs, making it confusing indeed. […]
ah, with that info it does make sense to me
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 43: gpio_output(gpio, 0);
Address this sequence in gpio_banks/gpio.c:gpio_output() instead.
if i saw that correctly this is the only unresolved comment in this patch, but i'd say that this isn't something that should be addressed in this patch. not sure, but i might have a patch on my work machine that fixes the behavior this works around
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 14:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 41: /* out the data value to prevent glitches */ If the IOMUX was already programmed for GPIO, this would need to copy GPIO_PIN_STS to GPIO_OUTPUT_VALUE. I don't think this currently does what the comment says.
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 50: iomux_write8(gpio, iomux_ftn & 0x3); Does GPIO_PIN_STS follow the pad state/voltage even before IOMUX is set to GPIO function?
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 43: gpio_output(gpio, 0);
if i saw that correctly this is the only unresolved comment in this patch, but i'd say that this isn […]
Left more unresolved comments about this sequence.
https://review.coreboot.org/c/coreboot/+/42521/14/src/mainboard/pcengines/ap... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/14/src/mainboard/pcengines/ap... PS14, Line 19: early_lpc_init(); Not about LPC anymore.
https://review.coreboot.org/c/coreboot/+/42521/14/src/mainboard/pcengines/ap... PS14, Line 28: /* Release GPIO32/33 for other uses. */ Hmm.. early_lpc_init() already wanted 32 and 33.
From amd/thatcher/bootblock.c: /* Disable PCI-PCI bridge and release GPIO32/33 for other uses. */ pm_write8(0xea, 0x1);
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 41: /* out the data value to prevent glitches */
If the IOMUX was already programmed for GPIO, this would need to copy GPIO_PIN_STS to GPIO_OUTPUT_VA […]
the code you removed here might cause glitches and doesn't do what the comment says. i don't think that copying the pin input value to the output register before switching to output mode should be the way to go here; either set all bits in one atomic operation or set the output value first and then the direction to output to avoid output glitches
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 50: iomux_write8(gpio, iomux_ftn & 0x3);
Does GPIO_PIN_STS follow the pad state/voltage even before IOMUX is set to GPIO function?
good question. see my comment above on why it shouldn't matter
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/8/src/mainboard/pcengines/apu... PS8, Line 43: gpio_output(gpio, 0);
Left more unresolved comments about this sequence.
this isn't needed any more after my common gpio patches. See CB:49119
Attention is currently required from: Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 14:
(1 comment)
File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/comment/e12d1ef6_0dadcd80 PS8, Line 43: gpio_output(gpio, 0);
this isn't needed any more after my common gpio patches. […]
fyi: the patch that fixed the corresponding issue in soc/amd/common is submitted now, so the workaround in here can be removed
Attention is currently required from: Kyösti Mälkki. Hello build bot (Jenkins), Michał Żygowski, Angel Pons, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42521
to look at the new patch set (#15).
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
mb/pcengines/apu2: Switch to proper GPIO API
Use the abstractions <gpio.h> provides.
Change-Id: I348ba43a76287be5b24012ae3dfc28ed783da9c7 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/mainboard/pcengines/apu2/gpio_ftns.c M src/mainboard/pcengines/apu2/gpio_ftns.h M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/romstage.c 4 files changed, 45 insertions(+), 100 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/42521/15
Attention is currently required from: Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 15: Code-Review+1
(3 comments)
Patchset:
PS15: looks good to me. did you run a test on the board?
File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/comment/cbef38f0_57b23c88 PS8, Line 50: iomux_write8(gpio, iomux_ftn & 0x3);
good question. […]
i'd say this can be marked as resolved
File src/mainboard/pcengines/apu2/romstage.c:
https://review.coreboot.org/c/coreboot/+/42521/comment/ff743c55_faf0db54 PS8, Line 43: gpio_output(gpio, 0);
fyi: the patch that fixed the corresponding issue in soc/amd/common is submitted now, so the workaro […]
Done
Attention is currently required from: Kyösti Mälkki. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 15: Code-Review+1
Attention is currently required from: Michał Żygowski, Kyösti Mälkki, Piotr Król. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 15: Code-Review+2
(1 comment)
File src/mainboard/pcengines/apu2/gpio_ftns.c:
https://review.coreboot.org/c/coreboot/+/42521/comment/b6557555_9910c2de PS8, Line 50: iomux_write8(gpio, iomux_ftn & 0x3);
i'd say this can be marked as resolved
Ack
Attention is currently required from: Michał Żygowski, Felix Held, Piotr Król. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 15:
(1 comment)
Patchset:
PS15:
looks good to me. […]
Added 3mdeb guys. I don't have the variety of boards this changes.
Attention is currently required from: Kyösti Mälkki, Piotr Król. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
Patch Set 15: Code-Review+2
(1 comment)
Patchset:
PS15:
Added 3mdeb guys. I don't have the variety of boards this changes.
Ack
Michał Żygowski has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42521 )
Change subject: mb/pcengines/apu2: Switch to proper GPIO API ......................................................................
mb/pcengines/apu2: Switch to proper GPIO API
Use the abstractions <gpio.h> provides.
Change-Id: I348ba43a76287be5b24012ae3dfc28ed783da9c7 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42521 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/mainboard/pcengines/apu2/gpio_ftns.c M src/mainboard/pcengines/apu2/gpio_ftns.h M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/romstage.c 4 files changed, 45 insertions(+), 100 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Michał Żygowski: Looks good to me, approved
diff --git a/src/mainboard/pcengines/apu2/gpio_ftns.c b/src/mainboard/pcengines/apu2/gpio_ftns.c index c249c2d..28b9a74 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.c +++ b/src/mainboard/pcengines/apu2/gpio_ftns.c @@ -1,68 +1,9 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <stdint.h> #include <amdblocks/acpimmio.h> -#include <console/console.h> -#include <device/mmio.h> -#include <FchPlatform.h> +#include <gpio.h> #include "gpio_ftns.h"
-static u32 gpio_read_wrapper(u32 iomux_gpio) -{ - u32 gpio = iomux_gpio << 2; - - if (gpio < 0x100) - return gpio0_read32(gpio & 0xff); - else if (gpio >= 0x100 && gpio < 0x200) - return gpio1_read32(gpio & 0xff); - else if (gpio >= 0x200 && gpio < 0x300) - return gpio2_read32(gpio & 0xff); - - die("Invalid GPIO"); -} - -static void gpio_write_wrapper(u32 iomux_gpio, u32 setting) -{ - u32 gpio = iomux_gpio << 2; - - if (gpio < 0x100) - gpio0_write32(gpio & 0xff, setting); - else if (gpio >= 0x100 && gpio < 0x200) - gpio1_write32(gpio & 0xff, setting); - else if (gpio >= 0x200 && gpio < 0x300) - gpio2_write32(gpio & 0xff, setting); -} - -void configure_gpio(u32 gpio, u8 iomux_ftn, u32 setting) -{ - u32 bdata; - - bdata = gpio_read_wrapper(gpio); - /* out the data value to prevent glitches */ - bdata |= (setting & GPIO_OUTPUT_ENABLE); - gpio_write_wrapper(gpio, bdata); - - /* set direction and data value */ - bdata |= (setting & (GPIO_OUTPUT_ENABLE | GPIO_OUTPUT_VALUE - | GPIO_PULL_UP_ENABLE | GPIO_PULL_DOWN_ENABLE)); - gpio_write_wrapper(gpio, bdata); - - iomux_write8(gpio, iomux_ftn & 0x3); -} - -u8 read_gpio(u32 gpio) -{ - return (gpio_read_wrapper(gpio) & GPIO_PIN_STS) ? 1 : 0; -} - -void write_gpio(u32 gpio, u8 value) -{ - u32 status = gpio_read_wrapper(gpio); - status &= ~GPIO_OUTPUT_VALUE; - status |= (value > 0) ? GPIO_OUTPUT_VALUE : 0; - gpio_write_wrapper(gpio, status); -} - int get_spd_offset(void) { u8 index = 0; @@ -70,10 +11,10 @@ * One SPD file contains all 4 options, determine which index to * read here, then call into the standard routines. */ - if (gpio1_read8(0x02) & BIT0) - index |= BIT0; - if (gpio1_read8(0x06) & BIT0) - index |= BIT1; + if (gpio_get(GPIO_49)) + index |= 1 << 0; + if (gpio_get(GPIO_50)) + index |= 1 << 1;
return index; } diff --git a/src/mainboard/pcengines/apu2/gpio_ftns.h b/src/mainboard/pcengines/apu2/gpio_ftns.h index d1e76de..95c744f 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.h +++ b/src/mainboard/pcengines/apu2/gpio_ftns.h @@ -3,9 +3,6 @@ #ifndef GPIO_FTNS_H #define GPIO_FTNS_H
-void configure_gpio(u32 gpio, u8 iomux_ftn, u32 setting); -u8 read_gpio(u32 gpio); -void write_gpio(u32 gpio, u8 value); int get_spd_offset(void);
// @@ -28,10 +25,4 @@ #define GPIO_68 0x48 // PE4_WDIS (SIMSWAP1 on APU5) #define GPIO_71 0x4D // PROCHOT
-#define GPIO_OUTPUT_ENABLE BIT23 -#define GPIO_OUTPUT_VALUE BIT22 -#define GPIO_PULL_DOWN_ENABLE BIT21 -#define GPIO_PULL_UP_ENABLE BIT20 -#define GPIO_PIN_STS BIT16 - #endif /* GPIO_FTNS_H */ diff --git a/src/mainboard/pcengines/apu2/mainboard.c b/src/mainboard/pcengines/apu2/mainboard.c index 40941db..bad1a57 100644 --- a/src/mainboard/pcengines/apu2/mainboard.c +++ b/src/mainboard/pcengines/apu2/mainboard.c @@ -6,6 +6,7 @@ #include <console/console.h> #include <device/device.h> #include <device/pci_def.h> +#include <gpio.h> #include <southbridge/amd/pi/hudson/hudson.h> #include <southbridge/amd/pi/hudson/pci_devs.h> #include <southbridge/amd/pi/hudson/amd_pci_int_defs.h> @@ -265,8 +266,8 @@ // // Turn off LED 2 and LED 3 // - write_gpio(GPIO_58, 1); - write_gpio(GPIO_59, 1); + gpio_set(GPIO_58, 1); + gpio_set(GPIO_59, 1); }
/* diff --git a/src/mainboard/pcengines/apu2/romstage.c b/src/mainboard/pcengines/apu2/romstage.c index 0fc472f..e0a63f0 100644 --- a/src/mainboard/pcengines/apu2/romstage.c +++ b/src/mainboard/pcengines/apu2/romstage.c @@ -2,9 +2,10 @@
#include <stdint.h> #include <amdblocks/acpimmio.h> +#include <console/console.h> #include <device/pci_def.h> #include <device/pci_ops.h> -#include <console/console.h> +#include <gpio.h> #include <northbridge/amd/agesa/state_machine.h>
#include "gpio_ftns.h" @@ -28,52 +29,63 @@ pm_write8(0xea, 1); }
+static void pin_input(gpio_t gpio, u8 iomux_ftn) +{ + iomux_write8(gpio, iomux_ftn); + gpio_input(gpio); +} + +static void pin_low(gpio_t gpio, u8 iomux_ftn) +{ + iomux_write8(gpio, iomux_ftn); + gpio_output(gpio, 0); +} + +static void pin_high(gpio_t gpio, u8 iomux_ftn) +{ + iomux_write8(gpio, iomux_ftn); + gpio_output(gpio, 1); +} + static void early_lpc_init(void) { - u32 setting = 0x0; - // - // Configure output disabled, value low, pull up/down disabled + // Configure output disabled, pull up/down disabled // - if (CONFIG(BOARD_PCENGINES_APU5)) { - configure_gpio(GPIO_22, Function0, setting); - } + if (CONFIG(BOARD_PCENGINES_APU5)) + pin_input(GPIO_22, Function0);
if (CONFIG(BOARD_PCENGINES_APU2) || CONFIG(BOARD_PCENGINES_APU3) || CONFIG(BOARD_PCENGINES_APU4)) { - configure_gpio(GPIO_32, Function0, setting); + pin_input(GPIO_32, Function0); }
- configure_gpio(GPIO_49, Function2, setting); - configure_gpio(GPIO_50, Function2, setting); - configure_gpio(GPIO_71, Function0, setting); + pin_input(GPIO_49, Function2); + pin_input(GPIO_50, Function2); + pin_input(GPIO_71, Function0);
// // Configure output enabled, value low, pull up/down disabled // - setting = GPIO_OUTPUT_ENABLE; if (CONFIG(BOARD_PCENGINES_APU3) || CONFIG(BOARD_PCENGINES_APU4)) { - configure_gpio(GPIO_33, Function0, setting); + pin_low(GPIO_33, Function0); } - - configure_gpio(GPIO_57, Function1, setting); - configure_gpio(GPIO_58, Function1, setting); - configure_gpio(GPIO_59, Function3, setting); + pin_low(GPIO_57, Function1); + pin_low(GPIO_58, Function1); + pin_low(GPIO_59, Function3);
// // Configure output enabled, value high, pull up/down disabled // - setting = GPIO_OUTPUT_ENABLE | GPIO_OUTPUT_VALUE; - if (CONFIG(BOARD_PCENGINES_APU5)) { - configure_gpio(GPIO_32, Function0, setting); - configure_gpio(GPIO_33, Function0, setting); + pin_high(GPIO_32, Function0); + pin_high(GPIO_33, Function0); }
- configure_gpio(GPIO_51, Function2, setting); - configure_gpio(GPIO_55, Function3, setting); - configure_gpio(GPIO_64, Function2, setting); - configure_gpio(GPIO_68, Function0, setting); + pin_high(GPIO_51, Function2); + pin_high(GPIO_55, Function3); + pin_high(GPIO_64, Function2); + pin_high(GPIO_68, Function0); }
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Kyösti Mälkki (Code Review)
-
Michał Żygowski (Code Review)