Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out EMMC GPIO pin mux setup ......................................................................
mb/amd/mandolin: factor out EMMC GPIO pin mux setup
This also makes the calling of the EMMC GPIO mux setup function conditional on PICASSO_LPC_IOMUX instead of AMD_LPC_DEBUG_CARD which only makes the selection of PICASSO_LPC_IOMUX user-configurable.
Change-Id: Ic49a668f82fbc1b851c07d312b543d2889fe4734 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/mainboard/amd/mandolin/Makefile.inc A src/mainboard/amd/mandolin/emmc_gpio.c M src/mainboard/amd/mandolin/gpio.c M src/mainboard/amd/mandolin/gpio.h M src/mainboard/amd/mandolin/mainboard.c 5 files changed, 38 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/42842/1
diff --git a/src/mainboard/amd/mandolin/Makefile.inc b/src/mainboard/amd/mandolin/Makefile.inc index 56dd5e2..761c3f8 100644 --- a/src/mainboard/amd/mandolin/Makefile.inc +++ b/src/mainboard/amd/mandolin/Makefile.inc @@ -6,6 +6,10 @@ ramstage-y += gpio.c ramstage-y += port_descriptors.c
+ifneq ($(CONFIG_PICASSO_LPC_IOMUX),y) +ramstage-y += emmc_gpio.c +endif # CONFIG_PICASSO_LPC_IOMUX + # APCB_mandolin.bin APCB_SOURCES = mandolin
diff --git a/src/mainboard/amd/mandolin/emmc_gpio.c b/src/mainboard/amd/mandolin/emmc_gpio.c new file mode 100644 index 0000000..2ae72a6 --- /dev/null +++ b/src/mainboard/amd/mandolin/emmc_gpio.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <soc/gpio.h> +#include "gpio.h" + +/* eMMC controller driving either an SD card or eMMC device. */ +static const struct soc_amd_gpio emmc_gpios[] = { + PAD_NF(GPIO_21, EMMC_CMD, PULL_UP), + PAD_NF(GPIO_22, EMMC_PRW_CTRL, PULL_UP), + PAD_NF(GPIO_68, EMMC_CD, PULL_UP), + PAD_NF(GPIO_70, EMMC_CLK, PULL_NONE), + PAD_NF(GPIO_104, EMMC_DATA0, PULL_UP), + PAD_NF(GPIO_105, EMMC_DATA1, PULL_UP), + PAD_NF(GPIO_106, EMMC_DATA2, PULL_UP), + PAD_NF(GPIO_107, EMMC_DATA3, PULL_NONE), + PAD_NF(GPIO_74, EMMC_DATA4, PULL_UP), + PAD_NF(GPIO_75, EMMC_DATA6, PULL_UP), + PAD_NF(GPIO_87, EMMC_DATA7, PULL_UP), + PAD_NF(GPIO_88, EMMC_DATA5, PULL_UP), + PAD_NF(GPIO_109, EMMC_DS, PULL_UP), +}; + +/* Don't call this if the board uses the LPC bus. */ +void mainboard_program_emmc_gpios(void) +{ + program_gpios(emmc_gpios, ARRAY_SIZE(emmc_gpios)); +} diff --git a/src/mainboard/amd/mandolin/gpio.c b/src/mainboard/amd/mandolin/gpio.c index a60f00e..0d2e2fa 100644 --- a/src/mainboard/amd/mandolin/gpio.c +++ b/src/mainboard/amd/mandolin/gpio.c @@ -1,8 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <console/console.h> #include <soc/gpio.h> - #include "gpio.h"
/* @@ -32,30 +30,7 @@ PAD_GPO(GPIO_89, HIGH), };
-/* eMMC controller driving either an SD card or eMMC device. */ -static const struct soc_amd_gpio emmc_gpios[] = { - PAD_NF(GPIO_21, EMMC_CMD, PULL_UP), - PAD_NF(GPIO_22, EMMC_PRW_CTRL, PULL_UP), - PAD_NF(GPIO_68, EMMC_CD, PULL_UP), - PAD_NF(GPIO_70, EMMC_CLK, PULL_NONE), - PAD_NF(GPIO_104, EMMC_DATA0, PULL_UP), - PAD_NF(GPIO_105, EMMC_DATA1, PULL_UP), - PAD_NF(GPIO_106, EMMC_DATA2, PULL_UP), - PAD_NF(GPIO_107, EMMC_DATA3, PULL_NONE), - PAD_NF(GPIO_74, EMMC_DATA4, PULL_UP), - PAD_NF(GPIO_75, EMMC_DATA6, PULL_UP), - PAD_NF(GPIO_87, EMMC_DATA7, PULL_UP), - PAD_NF(GPIO_88, EMMC_DATA5, PULL_UP), - PAD_NF(GPIO_109, EMMC_DS, PULL_UP), -}; - void mainboard_program_gpios(void) { program_gpios(gpio_set_stage_ram, ARRAY_SIZE(gpio_set_stage_ram)); - - /* Re-muxing LPCCLK0 can hang the system if LPC is in use. */ - if (CONFIG(AMD_LPC_DEBUG_CARD)) - printk(BIOS_INFO, "eMMC not available due to LPC requirement\n"); - else - program_gpios(emmc_gpios, ARRAY_SIZE(emmc_gpios)); } diff --git a/src/mainboard/amd/mandolin/gpio.h b/src/mainboard/amd/mandolin/gpio.h index 04c98c5..f4d0b5c 100644 --- a/src/mainboard/amd/mandolin/gpio.h +++ b/src/mainboard/amd/mandolin/gpio.h @@ -5,5 +5,6 @@
void mainboard_program_early_gpios(void); /* bootblock GPIO configuration */ void mainboard_program_gpios(void); /* ramstage GPIO configuration */ +void mainboard_program_emmc_gpios(void); /* ramstage EMMC pin mux configuration */
#endif /* MAINBOARD_GPIO_H */ diff --git a/src/mainboard/amd/mandolin/mainboard.c b/src/mainboard/amd/mandolin/mainboard.c index c0b9dac..c22ed34 100644 --- a/src/mainboard/amd/mandolin/mainboard.c +++ b/src/mainboard/amd/mandolin/mainboard.c @@ -115,6 +115,12 @@ cfg->sd_emmc_config = SD_EMMC_EMMC_HS400;
mainboard_program_gpios(); + + /* Re-muxing LPCCLK0 can hang the system if LPC is in use. */ + if (CONFIG(PICASSO_LPC_IOMUX)) + printk(BIOS_INFO, "eMMC not available due to LPC requirement\n"); + else + mainboard_program_emmc_gpios(); }
static void mandolin_enable(struct device *dev)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out EMMC GPIO pin mux setup ......................................................................
Patch Set 1: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/42842/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42842/1//COMMIT_MSG@7 PS1, Line 7: EMMC eMMC
https://review.coreboot.org/c/coreboot/+/42842/1//COMMIT_MSG@9 PS1, Line 9: EMMC eMMC
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 11: # CONFIG_PICASSO_LPC_IOMUX Not sure if this comment is worth having
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/gpio.h:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 8: EMMC eMMC
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42842
to look at the new patch set (#2).
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
mb/amd/mandolin: factor out eMMC GPIO pin mux setup
This also makes the calling of the eMMC GPIO mux setup function conditional on PICASSO_LPC_IOMUX instead of AMD_LPC_DEBUG_CARD which only makes the selection of PICASSO_LPC_IOMUX user-configurable.
Change-Id: Ic49a668f82fbc1b851c07d312b543d2889fe4734 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/mainboard/amd/mandolin/Makefile.inc A src/mainboard/amd/mandolin/emmc_gpio.c M src/mainboard/amd/mandolin/gpio.c M src/mainboard/amd/mandolin/gpio.h M src/mainboard/amd/mandolin/mainboard.c 5 files changed, 38 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/42842/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42842/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42842/1//COMMIT_MSG@7 PS1, Line 7: EMMC
eMMC
Done
https://review.coreboot.org/c/coreboot/+/42842/1//COMMIT_MSG@9 PS1, Line 9: EMMC
eMMC
Done
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 11: # CONFIG_PICASSO_LPC_IOMUX
Not sure if this comment is worth having
yeah, it's close enough together, so i dropped that comment
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/gpio.h:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 8: EMMC
eMMC
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 11: # CONFIG_PICASSO_LPC_IOMUX
yeah, it's close enough together, so i dropped that comment
I believe, the style in coreboot is to always add these. There was such a cleanup some years ago.
I’d also say, how does having the comment hurt?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 11: # CONFIG_PICASSO_LPC_IOMUX
I believe, the style in coreboot is to always add these. There was such a cleanup some years ago. […]
I don't see such style rule being enforced anywhere.
Plus, I never said the comment hurt. I did say that it was not useful, because it's a very tiny if-block and, if anything, only adds more clutter.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... File src/mainboard/amd/mandolin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42842/1/src/mainboard/amd/mandolin/... PS1, Line 11: # CONFIG_PICASSO_LPC_IOMUX
I don't see such style rule being enforced anywhere. […]
if the endif statement is far way from the ifeq/ifneq one, it should definitely be there, but in this case those are very close together, so I don't think that it's necessary or really useful here
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42842 )
Change subject: mb/amd/mandolin: factor out eMMC GPIO pin mux setup ......................................................................
mb/amd/mandolin: factor out eMMC GPIO pin mux setup
This also makes the calling of the eMMC GPIO mux setup function conditional on PICASSO_LPC_IOMUX instead of AMD_LPC_DEBUG_CARD which only makes the selection of PICASSO_LPC_IOMUX user-configurable.
Change-Id: Ic49a668f82fbc1b851c07d312b543d2889fe4734 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/42842 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/amd/mandolin/Makefile.inc A src/mainboard/amd/mandolin/emmc_gpio.c M src/mainboard/amd/mandolin/gpio.c M src/mainboard/amd/mandolin/gpio.h M src/mainboard/amd/mandolin/mainboard.c 5 files changed, 38 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/amd/mandolin/Makefile.inc b/src/mainboard/amd/mandolin/Makefile.inc index 56dd5e2..de850b2 100644 --- a/src/mainboard/amd/mandolin/Makefile.inc +++ b/src/mainboard/amd/mandolin/Makefile.inc @@ -6,6 +6,10 @@ ramstage-y += gpio.c ramstage-y += port_descriptors.c
+ifneq ($(CONFIG_PICASSO_LPC_IOMUX),y) +ramstage-y += emmc_gpio.c +endif + # APCB_mandolin.bin APCB_SOURCES = mandolin
diff --git a/src/mainboard/amd/mandolin/emmc_gpio.c b/src/mainboard/amd/mandolin/emmc_gpio.c new file mode 100644 index 0000000..2ae72a6 --- /dev/null +++ b/src/mainboard/amd/mandolin/emmc_gpio.c @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <soc/gpio.h> +#include "gpio.h" + +/* eMMC controller driving either an SD card or eMMC device. */ +static const struct soc_amd_gpio emmc_gpios[] = { + PAD_NF(GPIO_21, EMMC_CMD, PULL_UP), + PAD_NF(GPIO_22, EMMC_PRW_CTRL, PULL_UP), + PAD_NF(GPIO_68, EMMC_CD, PULL_UP), + PAD_NF(GPIO_70, EMMC_CLK, PULL_NONE), + PAD_NF(GPIO_104, EMMC_DATA0, PULL_UP), + PAD_NF(GPIO_105, EMMC_DATA1, PULL_UP), + PAD_NF(GPIO_106, EMMC_DATA2, PULL_UP), + PAD_NF(GPIO_107, EMMC_DATA3, PULL_NONE), + PAD_NF(GPIO_74, EMMC_DATA4, PULL_UP), + PAD_NF(GPIO_75, EMMC_DATA6, PULL_UP), + PAD_NF(GPIO_87, EMMC_DATA7, PULL_UP), + PAD_NF(GPIO_88, EMMC_DATA5, PULL_UP), + PAD_NF(GPIO_109, EMMC_DS, PULL_UP), +}; + +/* Don't call this if the board uses the LPC bus. */ +void mainboard_program_emmc_gpios(void) +{ + program_gpios(emmc_gpios, ARRAY_SIZE(emmc_gpios)); +} diff --git a/src/mainboard/amd/mandolin/gpio.c b/src/mainboard/amd/mandolin/gpio.c index a60f00e..0d2e2fa 100644 --- a/src/mainboard/amd/mandolin/gpio.c +++ b/src/mainboard/amd/mandolin/gpio.c @@ -1,8 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <console/console.h> #include <soc/gpio.h> - #include "gpio.h"
/* @@ -32,30 +30,7 @@ PAD_GPO(GPIO_89, HIGH), };
-/* eMMC controller driving either an SD card or eMMC device. */ -static const struct soc_amd_gpio emmc_gpios[] = { - PAD_NF(GPIO_21, EMMC_CMD, PULL_UP), - PAD_NF(GPIO_22, EMMC_PRW_CTRL, PULL_UP), - PAD_NF(GPIO_68, EMMC_CD, PULL_UP), - PAD_NF(GPIO_70, EMMC_CLK, PULL_NONE), - PAD_NF(GPIO_104, EMMC_DATA0, PULL_UP), - PAD_NF(GPIO_105, EMMC_DATA1, PULL_UP), - PAD_NF(GPIO_106, EMMC_DATA2, PULL_UP), - PAD_NF(GPIO_107, EMMC_DATA3, PULL_NONE), - PAD_NF(GPIO_74, EMMC_DATA4, PULL_UP), - PAD_NF(GPIO_75, EMMC_DATA6, PULL_UP), - PAD_NF(GPIO_87, EMMC_DATA7, PULL_UP), - PAD_NF(GPIO_88, EMMC_DATA5, PULL_UP), - PAD_NF(GPIO_109, EMMC_DS, PULL_UP), -}; - void mainboard_program_gpios(void) { program_gpios(gpio_set_stage_ram, ARRAY_SIZE(gpio_set_stage_ram)); - - /* Re-muxing LPCCLK0 can hang the system if LPC is in use. */ - if (CONFIG(AMD_LPC_DEBUG_CARD)) - printk(BIOS_INFO, "eMMC not available due to LPC requirement\n"); - else - program_gpios(emmc_gpios, ARRAY_SIZE(emmc_gpios)); } diff --git a/src/mainboard/amd/mandolin/gpio.h b/src/mainboard/amd/mandolin/gpio.h index 04c98c5..1881fe0 100644 --- a/src/mainboard/amd/mandolin/gpio.h +++ b/src/mainboard/amd/mandolin/gpio.h @@ -5,5 +5,6 @@
void mainboard_program_early_gpios(void); /* bootblock GPIO configuration */ void mainboard_program_gpios(void); /* ramstage GPIO configuration */ +void mainboard_program_emmc_gpios(void); /* ramstage eMMC pin mux configuration */
#endif /* MAINBOARD_GPIO_H */ diff --git a/src/mainboard/amd/mandolin/mainboard.c b/src/mainboard/amd/mandolin/mainboard.c index c0b9dac..c22ed34 100644 --- a/src/mainboard/amd/mandolin/mainboard.c +++ b/src/mainboard/amd/mandolin/mainboard.c @@ -115,6 +115,12 @@ cfg->sd_emmc_config = SD_EMMC_EMMC_HS400;
mainboard_program_gpios(); + + /* Re-muxing LPCCLK0 can hang the system if LPC is in use. */ + if (CONFIG(PICASSO_LPC_IOMUX)) + printk(BIOS_INFO, "eMMC not available due to LPC requirement\n"); + else + mainboard_program_emmc_gpios(); }
static void mandolin_enable(struct device *dev)
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Paul Menzel (Code Review)