Le mardi 28 octobre 2014 à 20:02 +0200, Igor Grinberg a écrit :

Hi Paul,

On 10/28/14 19:25, Paul Kocialkowski wrote:

...

Some devices may use non-standard combinations of regulators to power MMC: this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.

Signed-off-by: Paul Kocialkowski contact@paulk.fr

arch/arm/include/asm/omap_mmc.h | 4 +++- drivers/mmc/omap_hsmmc.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h index 617e22f..b6a8325 100644 --- a/arch/arm/include/asm/omap_mmc.h +++ b/arch/arm/include/asm/omap_mmc.h @@ -164,5 +164,7 @@ struct hsmmc { int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio, int wp_gpio);

+#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT

I'm not a huge fan of that approach, but if you add yet another CONFIG_ option, I think it is a requirement to add a documentation for it.

That saddens me too, but I don't see how to do this in a better way for now.

...

+void omap_hsmmc_board_power_init(void);

Anyway, I would suggest adding a default __weak board_mmc_power_init() or something like this (which would be transfered into a callback in pdata once omap_hsmmc.c is).

Well, there are two distinct scenarios here: 1. the board follows some reference design and VMMC1 is used to power MMC1, VMMC2 for MMC2, then twl4030 has a function that will set everything up correctly (with the addition I added earlier) 2. the board uses the regulators in a more or less random way and VMMC1 is not mapped to MMC1 power at all. This is the case on the LG Optimus Black (P970). There is actually an auxiliary regulator IC that is in charge of powering MMC1. Hence the need for board-specific behavior.

I think it's perfectly fine to keep using the twl4030 function when the board follows the reference design.

Would that board_mmc_power_init you suggest be generic to all MMC drivers? Then at which point should it be called? What would a generic board_mmc_power_init have? TWL4030 is specific to OMAP3. Then perhaps there should be a board_mmc_power_init implementation for OMAP3, that can be overridden by a board-specific function?

I'm a bit new to the U-Boot code, so I'd appreciate pointers on what should go where.

Or... just no need for this patch at all, as board_mmc_init() can be used for this...

Not in the context of the SPL, which is precisely why I needed this.

Thanks for your review!

...

+#endif #endif /* OMAP_MMC_H_ */ diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index ef2cbf9..ef4c5cf 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc) pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); writel(pbias_lite, &t2_base->pbias_lite); #endif -#if defined(CONFIG_TWL4030_POWER) +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT)

• omap_hsmmc_board_power_init();

+#elif defined(CONFIG_TWL4030_POWER) twl4030_power_mmc_init(); mdelay(100); /* ramp-up delay from Linux code */ #endif

Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.

Signed-off-by: Paul Kocialkowski contact@paulk.fr --- board/comelit/dig297/dig297.c | 9 +++++++++ board/compulab/cm_t35/cm_t35.c | 11 +++++++++++ board/corscience/tricorder/tricorder.c | 11 +++++++++++ board/isee/igep00x0/igep00x0.c | 11 +++++++++++ board/logicpd/omap3som/omap3logic.c | 11 +++++++++++ board/logicpd/zoom1/zoom1.c | 9 +++++++++ board/matrix_vision/mvblx/mvblx.c | 9 +++++++++ board/nokia/rx51/rx51.c | 9 +++++++++ board/overo/overo.c | 11 +++++++++++ board/pandora/pandora.c | 9 +++++++++ board/technexion/tao3530/tao3530.c | 11 +++++++++++ board/ti/beagle/beagle.c | 11 +++++++++++ board/ti/evm/evm.c | 11 +++++++++++ board/ti/sdp3430/sdp.c | 9 +++++++++ board/timll/devkit8000/devkit8000.c | 11 +++++++++++ drivers/mmc/omap_hsmmc.c | 7 +------ 16 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/board/comelit/dig297/dig297.c b/board/comelit/dig297/dig297.c index 2b826df..784483b 100644 --- a/board/comelit/dig297/dig297.c +++ b/board/comelit/dig297/dig297.c @@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis) { return omap_mmc_init(0, 0, 0, -1, -1); } + +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} #endif

#ifdef CONFIG_CMD_NET diff --git a/board/compulab/cm_t35/cm_t35.c b/board/compulab/cm_t35/cm_t35.c index 0944903..93de1de 100644 --- a/board/compulab/cm_t35/cm_t35.c +++ b/board/compulab/cm_t35/cm_t35.c @@ -473,6 +473,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + /* * Routine: setup_net_chip_gmpc * Description: Setting up the configuration GPMC registers specific to the diff --git a/board/corscience/tricorder/tricorder.c b/board/corscience/tricorder/tricorder.c index 9e81bf3..9ade210 100644 --- a/board/corscience/tricorder/tricorder.c +++ b/board/corscience/tricorder/tricorder.c @@ -147,6 +147,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + /* * Routine: get_board_mem_timings * Description: If we use SPL then there is no x-loader nor config header diff --git a/board/isee/igep00x0/igep00x0.c b/board/isee/igep00x0/igep00x0.c index 3b2b1f1..dd88b75 100644 --- a/board/isee/igep00x0/igep00x0.c +++ b/board/isee/igep00x0/igep00x0.c @@ -137,6 +137,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + void set_fdt(void) { switch (gd->bd->bi_arch_number) { diff --git a/board/logicpd/omap3som/omap3logic.c b/board/logicpd/omap3som/omap3logic.c index 075fe94..8e19b6b 100644 --- a/board/logicpd/omap3som/omap3logic.c +++ b/board/logicpd/omap3som/omap3logic.c @@ -128,6 +128,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + #ifdef CONFIG_SMC911X /* GPMC CS1 settings for Logic SOM LV/Torpedo LAN92xx Ethernet chip */ static const u32 gpmc_lan92xx_config[] = { diff --git a/board/logicpd/zoom1/zoom1.c b/board/logicpd/zoom1/zoom1.c index 461a852..c06299c 100644 --- a/board/logicpd/zoom1/zoom1.c +++ b/board/logicpd/zoom1/zoom1.c @@ -96,6 +96,15 @@ int board_mmc_init(bd_t *bis) { return omap_mmc_init(0, 0, 0, -1, -1); } + +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} #endif

#ifdef CONFIG_CMD_NET diff --git a/board/matrix_vision/mvblx/mvblx.c b/board/matrix_vision/mvblx/mvblx.c index a69359f..1f4ca47 100644 --- a/board/matrix_vision/mvblx/mvblx.c +++ b/board/matrix_vision/mvblx/mvblx.c @@ -94,6 +94,15 @@ int board_mmc_init(bd_t *bis) omap_mmc_init(1, 0, 0, -1, -1); return 0; } + +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} #endif

#if defined(CONFIG_CMD_NET) diff --git a/board/nokia/rx51/rx51.c b/board/nokia/rx51/rx51.c index c2e07db..e313fc2 100644 --- a/board/nokia/rx51/rx51.c +++ b/board/nokia/rx51/rx51.c @@ -659,3 +659,12 @@ int board_mmc_init(bd_t *bis) omap_mmc_init(1, 0, 0, -1, -1); return 0; } + +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} diff --git a/board/overo/overo.c b/board/overo/overo.c index 13220c5..d86eca4 100644 --- a/board/overo/overo.c +++ b/board/overo/overo.c @@ -437,3 +437,14 @@ int board_mmc_init(bd_t *bis) return omap_mmc_init(0, 0, 0, -1, -1); } #endif + +#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif diff --git a/board/pandora/pandora.c b/board/pandora/pandora.c index 146dcea..8fa0119 100644 --- a/board/pandora/pandora.c +++ b/board/pandora/pandora.c @@ -126,4 +126,13 @@ int board_mmc_init(bd_t *bis) { return omap_mmc_init(0, 0, 0, -1, -1); } + +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} #endif diff --git a/board/technexion/tao3530/tao3530.c b/board/technexion/tao3530/tao3530.c index 44a8240..64f2313 100644 --- a/board/technexion/tao3530/tao3530.c +++ b/board/technexion/tao3530/tao3530.c @@ -188,6 +188,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + #if defined(CONFIG_USB_EHCI) && !defined(CONFIG_SPL_BUILD) /* Call usb_stop() before starting the kernel */ void show_boot_progress(int val) diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c index 94b99bf..b47e44b 100644 --- a/board/ti/beagle/beagle.c +++ b/board/ti/beagle/beagle.c @@ -521,6 +521,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + #if defined(CONFIG_USB_EHCI) && !defined(CONFIG_SPL_BUILD) /* Call usb_stop() before starting the kernel */ void show_boot_progress(int val) diff --git a/board/ti/evm/evm.c b/board/ti/evm/evm.c index 81dd081..c41a760 100644 --- a/board/ti/evm/evm.c +++ b/board/ti/evm/evm.c @@ -264,3 +264,14 @@ int board_mmc_init(bd_t *bis) return omap_mmc_init(0, 0, 0, -1, -1); } #endif + +#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif diff --git a/board/ti/sdp3430/sdp.c b/board/ti/sdp3430/sdp.c index 957940d..44e0418 100644 --- a/board/ti/sdp3430/sdp.c +++ b/board/ti/sdp3430/sdp.c @@ -195,4 +195,13 @@ int board_mmc_init(bd_t *bis) { return omap_mmc_init(0, 0, 0, -1, -1); } + +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} #endif diff --git a/board/timll/devkit8000/devkit8000.c b/board/timll/devkit8000/devkit8000.c index bcbee73..ef7fd0c 100644 --- a/board/timll/devkit8000/devkit8000.c +++ b/board/timll/devkit8000/devkit8000.c @@ -124,6 +124,17 @@ int board_mmc_init(bd_t *bis) } #endif

+#if defined(CONFIG_GENERIC_MMC) +int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER) + twl4030_power_mmc_init(); + mdelay(100); /* ramp-up delay from Linux code */ +#endif + return 0; +} +#endif + #if defined(CONFIG_DRIVER_DM9000) & !defined(CONFIG_SPL_BUILD) /* * Routine: board_eth_init diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index 5b0c302..6e93a07 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -130,12 +130,7 @@ static unsigned char mmc_board_init(struct mmc *mmc) pbias_lite = readl(&t2_base->pbias_lite); pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); writel(pbias_lite, &t2_base->pbias_lite); -#endif -#if defined(CONFIG_TWL4030_POWER) - twl4030_power_mmc_init(); - mdelay(100); /* ramp-up delay from Linux code */ -#endif -#if defined(CONFIG_OMAP34XX) + writel(pbias_lite | PBIASLITEPWRDNZ1 | PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0, &t2_base->pbias_lite);

...

Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.

[snip]

...

+int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER)

• twl4030_power_mmc_init();
• mdelay(100); /* ramp-up delay from Linux code */

+#endif

• return 0;

+} #endif

I think we can do away with the #if defined(CONFIG_TWL4030_POWER) part here since all of these boards will fail pretty spectacularly without that being set and if someone does remove that a link error is better than a crazy runtime problem.

That makes sense, I'll do that in v5.

[snip]

...

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index 5b0c302..6e93a07 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -130,12 +130,7 @@ static unsigned char mmc_board_init(struct mmc *mmc) pbias_lite = readl(&t2_base->pbias_lite); pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); writel(pbias_lite, &t2_base->pbias_lite); -#endif -#if defined(CONFIG_TWL4030_POWER)

• twl4030_power_mmc_init();
• mdelay(100); /* ramp-up delay from Linux code */

-#endif -#if defined(CONFIG_OMAP34XX)

• writel(pbias_lite | PBIASLITEPWRDNZ1 | PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0, &t2_base->pbias_lite);

Since it's not clear from the context here, it's OK to drop the check on CONFIG_OMAP34XX here since there's already a test on it above where diff cut us off? Thanks!

Indeed, that's the case (not sure this was actually more of a question than a statement).

Thanks for the review!

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Hi Tom,

On 11/04/14 17:56, Tom Rini wrote:

On Sat, Nov 01, 2014 at 11:35:43AM +0100, Paul Kocialkowski wrote:

...

Some devices may use non-standard combinations of regulators to power MMC: this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.

Signed-off-by: Paul Kocialkowski contact@paulk.fr

drivers/mmc/mmc.c | 8 ++++++++ include/mmc.h | 2 ++ 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 44a4feb..125f347 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev) } #endif

+/* board-specific MMC power initializations. */ +__weak int board_mmc_power_init(void) +{

• return -1;

+}

Since we don't check error return here which I think is fine just make this a void? Thanks!

There is v3 posted a while ago... We have also agreed on v4..

- -- Regards, Igor.

Hi there, thanks for the review,

Le mardi 04 novembre 2014 à 13:32 -0500, Tom Rini a écrit :

On Tue, Nov 04, 2014 at 07:58:38PM +0200, Igor Grinberg wrote:

...

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Hi Tom,

On 11/04/14 17:56, Tom Rini wrote:

...

On Sat, Nov 01, 2014 at 11:35:43AM +0100, Paul Kocialkowski wrote:

...

Some devices may use non-standard combinations of regulators to power MMC: this allows these devices to provide a board-specific MMC power init function to set everything up in their own way.

Signed-off-by: Paul Kocialkowski contact@paulk.fr

drivers/mmc/mmc.c | 8 ++++++++ include/mmc.h | 2 ++ 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 44a4feb..125f347 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -1277,6 +1277,12 @@ block_dev_desc_t *mmc_get_dev(int dev) } #endif

+/* board-specific MMC power initializations. */ +__weak int board_mmc_power_init(void) +{

• return -1;

+}

Since we don't check error return here which I think is fine just make this a void? Thanks!

There is v3 posted a while ago... We have also agreed on v4..

Note that v3 and v4 are the same, except that v3 didn't apply on top of master.

Yeah, oops, didn't delete these after catch-up. I'm still not sure we should continue adding more unchecked return values "just because".

I agree that we shouldn't have an unchecked return value. So we could either check the return value and print a warning, without aborting the init sequence (what Igor proposed initially) or just make this return void (what you both seem to agree on).

I'm fine with both solutions. I guess that enabling a regulator could fail (say, because of an i2c error), so there is still sense in returning int.

Let me know of what your definitive answer on this is. I'll make a new patchset probably this friday (I'm running on a very tight schedule until then).