Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
mb/pcengines/apu2: Update GPIO Reads & writes
The APU2 was using the soc/amd/common functions to do GPIO reads and writes. The functions that were being used are getting eliminated in the SOC directory, but since the APU isn't using the rest of that code (as it's not using the rest of the SOC codebase), it proved to be problematic to use the updated functions.
The solution I've put in place here is to pull everything needed for the GPIO reads & writes into the gpio_ftns.c & h files.
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: Ied39c114bdf3637977d21f56fd7db428c52e4706 --- M src/mainboard/pcengines/apu2/gpio_ftns.c M src/mainboard/pcengines/apu2/gpio_ftns.h 2 files changed, 11 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/42484/1
diff --git a/src/mainboard/pcengines/apu2/gpio_ftns.c b/src/mainboard/pcengines/apu2/gpio_ftns.c index c249c2d..7bf0bc9 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.c +++ b/src/mainboard/pcengines/apu2/gpio_ftns.c @@ -12,11 +12,11 @@ u32 gpio = iomux_gpio << 2;
if (gpio < 0x100) - return gpio0_read32(gpio & 0xff); + return read32((void *)(ACPIMMIO_GPIO0_BASE + (gpio & 0xff))); else if (gpio >= 0x100 && gpio < 0x200) - return gpio1_read32(gpio & 0xff); + return read32((void *)(ACPIMMIO_GPIO1_BASE + (gpio & 0xff))); else if (gpio >= 0x200 && gpio < 0x300) - return gpio2_read32(gpio & 0xff); + return read32((void *)(ACPIMMIO_GPIO2_BASE + (gpio & 0xff)));
die("Invalid GPIO"); } @@ -26,11 +26,11 @@ u32 gpio = iomux_gpio << 2;
if (gpio < 0x100) - gpio0_write32(gpio & 0xff, setting); + write32((void *)(ACPIMMIO_GPIO0_BASE + (gpio & 0xff)), setting); else if (gpio >= 0x100 && gpio < 0x200) - gpio1_write32(gpio & 0xff, setting); + write32((void *)(ACPIMMIO_GPIO1_BASE + (gpio & 0xff)), setting); else if (gpio >= 0x200 && gpio < 0x300) - gpio2_write32(gpio & 0xff, setting); + write32((void *)(ACPIMMIO_GPIO2_BASE + (gpio & 0xff)), setting); }
void configure_gpio(u32 gpio, u8 iomux_ftn, u32 setting) @@ -70,9 +70,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 (read32((void *)(ACPIMMIO_GPIO1_BASE + 0x02)) & BIT0) index |= BIT0; - if (gpio1_read8(0x06) & BIT0) + if (read32((void *)(ACPIMMIO_GPIO1_BASE + 0x06)) & BIT0) index |= BIT1;
return index; diff --git a/src/mainboard/pcengines/apu2/gpio_ftns.h b/src/mainboard/pcengines/apu2/gpio_ftns.h index d1e76de..a0ce03c 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.h +++ b/src/mainboard/pcengines/apu2/gpio_ftns.h @@ -8,6 +8,9 @@ void write_gpio(u32 gpio, u8 value); int get_spd_offset(void);
+#define ACPIMMIO_GPIO0_BASE 0xfed81500 +#define ACPIMMIO_GPIO1_BASE 0xfed81600 +#define ACPIMMIO_GPIO2_BASE 0xfed81700 // // Based on PC Engines APU2C and APU3A schematics // http://www.pcengines.ch/schema/apu2c.pdf
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
Patch Set 1: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
Patch Set 1:
And with a different approach on the issue we reach CB:42521.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
Patch Set 1:
I'm not even sure why there is code like this in mainboard to begin with.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
Patch Set 1: Code-Review+2
Patch Set 1:
I'm not even sure why there is code like this in mainboard to begin with.
It is used to turn off the LEDs in the final booting stage. Also helps indicating a normal user that the boot process is finished
Michał Żygowski has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
mb/pcengines/apu2: Update GPIO Reads & writes
The APU2 was using the soc/amd/common functions to do GPIO reads and writes. The functions that were being used are getting eliminated in the SOC directory, but since the APU isn't using the rest of that code (as it's not using the rest of the SOC codebase), it proved to be problematic to use the updated functions.
The solution I've put in place here is to pull everything needed for the GPIO reads & writes into the gpio_ftns.c & h files.
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: Ied39c114bdf3637977d21f56fd7db428c52e4706 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42484 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/mainboard/pcengines/apu2/gpio_ftns.c M src/mainboard/pcengines/apu2/gpio_ftns.h 2 files changed, 11 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved 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..7bf0bc9 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.c +++ b/src/mainboard/pcengines/apu2/gpio_ftns.c @@ -12,11 +12,11 @@ u32 gpio = iomux_gpio << 2;
if (gpio < 0x100) - return gpio0_read32(gpio & 0xff); + return read32((void *)(ACPIMMIO_GPIO0_BASE + (gpio & 0xff))); else if (gpio >= 0x100 && gpio < 0x200) - return gpio1_read32(gpio & 0xff); + return read32((void *)(ACPIMMIO_GPIO1_BASE + (gpio & 0xff))); else if (gpio >= 0x200 && gpio < 0x300) - return gpio2_read32(gpio & 0xff); + return read32((void *)(ACPIMMIO_GPIO2_BASE + (gpio & 0xff)));
die("Invalid GPIO"); } @@ -26,11 +26,11 @@ u32 gpio = iomux_gpio << 2;
if (gpio < 0x100) - gpio0_write32(gpio & 0xff, setting); + write32((void *)(ACPIMMIO_GPIO0_BASE + (gpio & 0xff)), setting); else if (gpio >= 0x100 && gpio < 0x200) - gpio1_write32(gpio & 0xff, setting); + write32((void *)(ACPIMMIO_GPIO1_BASE + (gpio & 0xff)), setting); else if (gpio >= 0x200 && gpio < 0x300) - gpio2_write32(gpio & 0xff, setting); + write32((void *)(ACPIMMIO_GPIO2_BASE + (gpio & 0xff)), setting); }
void configure_gpio(u32 gpio, u8 iomux_ftn, u32 setting) @@ -70,9 +70,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 (read32((void *)(ACPIMMIO_GPIO1_BASE + 0x02)) & BIT0) index |= BIT0; - if (gpio1_read8(0x06) & BIT0) + if (read32((void *)(ACPIMMIO_GPIO1_BASE + 0x06)) & BIT0) index |= BIT1;
return index; diff --git a/src/mainboard/pcengines/apu2/gpio_ftns.h b/src/mainboard/pcengines/apu2/gpio_ftns.h index d1e76de..a0ce03c 100644 --- a/src/mainboard/pcengines/apu2/gpio_ftns.h +++ b/src/mainboard/pcengines/apu2/gpio_ftns.h @@ -8,6 +8,9 @@ void write_gpio(u32 gpio, u8 value); int get_spd_offset(void);
+#define ACPIMMIO_GPIO0_BASE 0xfed81500 +#define ACPIMMIO_GPIO1_BASE 0xfed81600 +#define ACPIMMIO_GPIO2_BASE 0xfed81700 // // Based on PC Engines APU2C and APU3A schematics // http://www.pcengines.ch/schema/apu2c.pdf
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
Patch Set 2:
Patch Set 1: Code-Review+2
Patch Set 1:
I'm not even sure why there is code like this in mainboard to begin with.
It is used to turn off the LEDs in the final booting stage. Also helps indicating a normal user that the boot process is finished
The comment was really about the quality of the code, not its existence. Or that's how I interpreted it. Expect to see a revert/rewrite of this in near future, apuX was sort of abusing the acpimmio/gpio_banks API.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42484 )
Change subject: mb/pcengines/apu2: Update GPIO Reads & writes ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1: Code-Review+2
Patch Set 1:
I'm not even sure why there is code like this in mainboard to begin with.
It is used to turn off the LEDs in the final booting stage. Also helps indicating a normal user that the boot process is finished
The comment was really about the quality of the code, not its existence. Or that's how I interpreted it. Expect to see a revert/rewrite of this in near future, apuX was sort of abusing the acpimmio/gpio_banks API.
Yes, that the intention of my comment, Michał. It wasn't the functionality it was the implementation. We largely try to keep chipset specific implementation knowledge out of mainboard implementations and defer to the chipset code to provide the necessary abstractions.
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Michał Żygowski (Code Review)