[U-Boot] [PATCH v3 0/2] mmc: Board-specific MMC power initializations

[U-Boot] [PATCH v3 0/2] mmc: Board-specific MMC power initializations

[Paul Kocialkowski]

Changelog:

v3: Changes to make v2 apply on master

v2: Implement board_mmc_power_init instead of define

[U-Boot] [PATCH v3 1/2] mmc: Board-specific MMC power initializations

[Paul Kocialkowski]

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     | 1 +
 2 files changed, 9 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;
+}
+
 int mmc_start_init(struct mmc *mmc)
 {
 	int err;
@@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
 	if (mmc->has_init)
 		return 0;
 
+	board_mmc_power_init();
+
 	/* made sure it's not NULL earlier */
 	err = mmc->cfg->ops->init(mmc);
 
diff --git a/include/mmc.h b/include/mmc.h
index d74a190..aaea644 100644
--- a/include/mmc.h
+++ b/include/mmc.h
@@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
 int mmc_legacy_init(int verbose);
 #endif
 
+int board_mmc_power_init(void);
 int board_mmc_init(bd_t *bis);
 int cpu_mmc_init(bd_t *bis);
 int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
-- 
1.9.1

[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations

[Paul Kocialkowski]

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 d0b0930..2e59618 100644
--- a/board/compulab/cm_t35/cm_t35.c
+++ b/board/compulab/cm_t35/cm_t35.c
@@ -462,6 +462,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 7b87cc2..70e7ca8 100644
--- a/board/isee/igep00x0/igep00x0.c
+++ b/board/isee/igep00x0/igep00x0.c
@@ -150,6 +150,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 1fd9f2c..1566a3b 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 9ef0026..2556290 100644
--- a/board/logicpd/zoom1/zoom1.c
+++ b/board/logicpd/zoom1/zoom1.c
@@ -109,6 +109,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 dfb8602..64ecc28 100644
--- a/board/overo/overo.c
+++ b/board/overo/overo.c
@@ -493,6 +493,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)
 static struct omap_usbhs_board_data usbhs_bdata = {
 	.port_mode[0] = OMAP_USBHS_PORT_MODE_UNUSED,
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 4c5e381..a5b7b3e 100644
--- a/board/ti/beagle/beagle.c
+++ b/board/ti/beagle/beagle.c
@@ -534,6 +534,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 ef2cbf9..6fb78b3 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -135,12 +135,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);
-- 
1.9.1

[U-Boot] [PATCH v3 1/2] mmc: Board-specific MMC power initializations

[Igor Grinberg]

Hi Paul,

On 11/01/14 12:52, 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     | 1 +
>  2 files changed, 9 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;
> +}
> +
>  int mmc_start_init(struct mmc *mmc)
>  {
>  	int err;
> @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
>  	if (mmc->has_init)
>  		return 0;
>  
> +	board_mmc_power_init();
> +

Shouldn't we have some error handling here?

>  	/* made sure it's not NULL earlier */
>  	err = mmc->cfg->ops->init(mmc);
>  
> diff --git a/include/mmc.h b/include/mmc.h
> index d74a190..aaea644 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
>  int mmc_legacy_init(int verbose);
>  #endif
>  
> +int board_mmc_power_init(void);
>  int board_mmc_init(bd_t *bis);
>  int cpu_mmc_init(bd_t *bis);
>  int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations

[Igor Grinberg]

Hi Paul,

On 11/01/14 12:52, Paul Kocialkowski wrote:
> 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>

This mostly looks good, several suggestions below though.

> ---
>  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 */

I guess all twl4030 based boards have to have this ramp up delay, right?
If so, why don't we want to hide it inside the twl4030_power_mmc_init()
function and not spread it across all boards?

> +#endif
> +	return 0;
> +}

Also, what do you think of the below suggestion:
Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c
(as it seems that many boards want it) as it currently is.
This way you will not need to patch each board file, yet a board
has the ability to control that call via the CONFIG_TWL4030_POWER
and have a board specific callback should it need one.

I would also change envelope the call to twl4030_power_mmc_init()
to something like CONFIG_TWL4030_MMC_POWER instead of
CONFIG_TWL4030_POWER, so it will be more flexible, but that has
little to do with this patch.

>  #endif
>  
>  #ifdef CONFIG_CMD_NET

[...]

> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> index ef2cbf9..6fb78b3 100644
> --- a/drivers/mmc/omap_hsmmc.c
> +++ b/drivers/mmc/omap_hsmmc.c
> @@ -135,12 +135,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);
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH v3 1/2] mmc: Board-specific MMC power initializations

[Paul Kocialkowski]

Hi Igor,

> On 11/01/14 12:52, 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     | 1 +
> >  2 files changed, 9 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;
> > +}
> > +
> >  int mmc_start_init(struct mmc *mmc)
> >  {
> >  	int err;
> > @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
> >  	if (mmc->has_init)
> >  		return 0;
> >  
> > +	board_mmc_power_init();
> > +
> 
> Shouldn't we have some error handling here?

I noticed how weak implementations tend to return -1 and not have their
return code checked so much (see the other two such functions in mmc.c).
I would be fine with checking the return code and returning 0 in the
weak implementation.

If you think that's better, I'll make a new version with that.

> >  	/* made sure it's not NULL earlier */
> >  	err = mmc->cfg->ops->init(mmc);
> >  
> > diff --git a/include/mmc.h b/include/mmc.h
> > index d74a190..aaea644 100644
> > --- a/include/mmc.h
> > +++ b/include/mmc.h
> > @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
> >  int mmc_legacy_init(int verbose);
> >  #endif
> >  
> > +int board_mmc_power_init(void);
> >  int board_mmc_init(bd_t *bis);
> >  int cpu_mmc_init(bd_t *bis);
> >  int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
> > 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141102/3aba81b0/attachment.pgp>

[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations

[Paul Kocialkowski]

Hi Igor, thanks for the review.

> On 11/01/14 12:52, Paul Kocialkowski wrote:
> > 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>
> 
> This mostly looks good, several suggestions below though.
> 
> > ---
> >  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 */
> 
> I guess all twl4030 based boards have to have this ramp up delay, right?
> If so, why don't we want to hide it inside the twl4030_power_mmc_init()
> function and not spread it across all boards?

That makes perfect sense, thanks for the suggestion. Will do in the next
version.

> > +#endif
> > +	return 0;
> > +}
> 
> Also, what do you think of the below suggestion:
> Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c
> (as it seems that many boards want it) as it currently is.
> This way you will not need to patch each board file, yet a board
> has the ability to control that call via the CONFIG_TWL4030_POWER
> and have a board specific callback should it need one.

One of the initial intents of this patch set was also to remove the
implication of CONFIG_TWL4030_POWER on twl4030_power_mmc_init, because
my device (LG Optimus Black P970) uses TWL4030 regulators in a way that
doesn't match other boards or any reference design (so I need
CONFIG_TWL4030_POWER but not twl4030_power_mmc_init).

> I would also change envelope the call to twl4030_power_mmc_init()
> to something like CONFIG_TWL4030_MMC_POWER instead of
> CONFIG_TWL4030_POWER, so it will be more flexible, but that has
> little to do with this patch.

That would be acceptable for me, but it would mean having two places
where the same thing happens: the new board_mmc_power_init function and
internally on omap_hsmmc. That's the sort of lack of consistance I
definitely do not like, but if you think it's the best way to do it, I
don't see any further issue with that.

Patching all the boards isn't really that big a deal (identifying the
boards is already done now anyway) and it makes similar sense to having
multiple board_mmc_init that all look very much alike.

In addition, perhaps I should add a dev_index parameter to
twl4030_power_mmc_init (in a separate patch) so that only the relevant
LDO is enabled (see my previous patch enabling VMMC2 too, that was
accepted): most devices only use one MMC controller, so there is no need
to enable all the MMC LDOs.

That would give some legitimacy to having one twl4030_power_mmc_init per
board, since it would reflect whether to enable one or both MMC LDOs.

What do you think?

> >  #endif
> >  
> >  #ifdef CONFIG_CMD_NET
> 
> [...]
> 
> > diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
> > index ef2cbf9..6fb78b3 100644
> > --- a/drivers/mmc/omap_hsmmc.c
> > +++ b/drivers/mmc/omap_hsmmc.c
> > @@ -135,12 +135,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);
> > 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141102/de1c266e/attachment.pgp>

[U-Boot] [PATCH v3 1/2] mmc: Board-specific MMC power initializations

[Igor Grinberg]

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

Hi Paul,

On 11/02/14 20:51, Paul Kocialkowski wrote:
> Hi Igor,
> 
>> On 11/01/14 12:52, 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     | 1 +
>>>  2 files changed, 9 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;
>>> +}
>>> +
>>>  int mmc_start_init(struct mmc *mmc)
>>>  {
>>>  	int err;
>>> @@ -1293,6 +1299,8 @@ int mmc_start_init(struct mmc *mmc)
>>>  	if (mmc->has_init)
>>>  		return 0;
>>>  
>>> +	board_mmc_power_init();
>>> +
>>
>> Shouldn't we have some error handling here?
> 
> I noticed how weak implementations tend to return -1 and not have their
> return code checked so much (see the other two such functions in mmc.c).
> I would be fine with checking the return code and returning 0 in the
> weak implementation.
> 
> If you think that's better, I'll make a new version with that.

Well, otherwise it does not make sense to return int, does it?
IMO it is a good practice to check the return value, and may be
emit a warning or something (I'm not really sure if it should
abort the init sequence).
Since I can't propose a good handling now, may be we should leave
it for now and see if someone comes up with a good case for it.
I'm fine with your decision.

> 
>>>  	/* made sure it's not NULL earlier */
>>>  	err = mmc->cfg->ops->init(mmc);
>>>  
>>> diff --git a/include/mmc.h b/include/mmc.h
>>> index d74a190..aaea644 100644
>>> --- a/include/mmc.h
>>> +++ b/include/mmc.h
>>> @@ -385,6 +385,7 @@ struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
>>>  int mmc_legacy_init(int verbose);
>>>  #endif
>>>  
>>> +int board_mmc_power_init(void);
>>>  int board_mmc_init(bd_t *bis);
>>>  int cpu_mmc_init(bd_t *bis);
>>>  int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
>>>
>>
> 

- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUVzBHAAoJEBDE8YO64EfayhsP/jV2YAR6jH6gZJBNn/wGnHId
a8U0YGG+f30RBPCdfCiraSRpL1C876wsf+InNDAHgo4HSaRdRwF4tUqWj78SpH9F
QTo3qcA6S8S4GIRJ2+q+3BmtlEuYPWOZOi41erhaUF3kMLw6cMt+gL0Ggx4OXQAo
2bhSeQaaGynoFJwC0dIf0QhDjRkPCHbNh2IgROfRye0a7/0pF4tN/tvjLSxw8w54
3457N4TYreGEmMMrtNi/psekMYTjR6JFve3CwiMvOXiwOJGz/d6qj7vhw4Ba7q5i
LE385Zj24Bi/+m7ls7hp5I522O0tkShGgPI3XvCiT4xrjXDBihsWG9RGsLSc5vBY
EimiztfLVu3alc1cxclK9tq0p+FwyUNwXsUED+oR6t56/qr/BSWiSIDAMfN1zcfI
paK9zXDoGvhg6T0pK+SsRvAE4D7vg+XT8WSU52l6/h8TRKB6r3cupSMXAyjmUpLl
1pEozVsg4qW6rJawQOiCjl0YU3ixeVwk0vmWWh+gBOMMLsuikQpFI5s6zUXteqnl
IHFS6oiYW/SVppHrd95624suZQPTsrH6+4CvmBwmtBGbmErchsgfj8qjP/ilpn1a
Y6Kv0mj2ndkgdFAc7zIEpwzF9yPsewVg8nc+vYLezyT7ePeTNYLpPWgQTL6qEdHH
lcK7qYNEaYWIUMkbQM5P
=7kRq
-----END PGP SIGNATURE-----

[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations

[Igor Grinberg]

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

On 11/02/14 21:01, Paul Kocialkowski wrote:
> Hi Igor, thanks for the review.
> 
>> On 11/01/14 12:52, Paul Kocialkowski wrote:
>>> 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>
>>
>> This mostly looks good, several suggestions below though.
>>
>>> ---
>>>  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 */
>>
>> I guess all twl4030 based boards have to have this ramp up delay, right?
>> If so, why don't we want to hide it inside the twl4030_power_mmc_init()
>> function and not spread it across all boards?
> 
> That makes perfect sense, thanks for the suggestion. Will do in the next
> version.
> 
>>> +#endif
>>> +	return 0;
>>> +}
>>
>> Also, what do you think of the below suggestion:
>> Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c
>> (as it seems that many boards want it) as it currently is.
>> This way you will not need to patch each board file, yet a board
>> has the ability to control that call via the CONFIG_TWL4030_POWER
>> and have a board specific callback should it need one.
> 
> One of the initial intents of this patch set was also to remove the
> implication of CONFIG_TWL4030_POWER on twl4030_power_mmc_init, because
> my device (LG Optimus Black P970) uses TWL4030 regulators in a way that
> doesn't match other boards or any reference design (so I need
> CONFIG_TWL4030_POWER but not twl4030_power_mmc_init).

In that case the below proposal suits you even better...

> 
>> I would also change envelope the call to twl4030_power_mmc_init()
>> to something like CONFIG_TWL4030_MMC_POWER instead of
>> CONFIG_TWL4030_POWER, so it will be more flexible, but that has
>> little to do with this patch.
> 
> That would be acceptable for me, but it would mean having two places
> where the same thing happens: the new board_mmc_power_init function and
> internally on omap_hsmmc. That's the sort of lack of consistance I
> definitely do not like, but if you think it's the best way to do it, I
> don't see any further issue with that.

Well, I fine with this.
Having the mmc power initialized in a "centralized" manner is good.
I just wanted to spare patching multiple boards, but in light of
the said below I think you are right.

> 
> Patching all the boards isn't really that big a deal (identifying the
> boards is already done now anyway) and it makes similar sense to having
> multiple board_mmc_init that all look very much alike.

Ok.

> 
> In addition, perhaps I should add a dev_index parameter to
> twl4030_power_mmc_init (in a separate patch) so that only the relevant
> LDO is enabled (see my previous patch enabling VMMC2 too, that was
> accepted): most devices only use one MMC controller, so there is no need
> to enable all the MMC LDOs.

That is a great idea.
This actually aroused in my mind when I first saw the VMMC2 patch.

> 
> That would give some legitimacy to having one twl4030_power_mmc_init per
> board, since it would reflect whether to enable one or both MMC LDOs.

Yep. I think, you should do this now, just before adding the multiple
instances of twl4030_power_mmc_init() call and patching it later.

> 
> What do you think?

All said above makes a good sense. So let's do this.

Also, I think having a DM for MMC will make things much less
"wild west" here, but that is a separate issue.

> 
>>>  #endif
>>>  
>>>  #ifdef CONFIG_CMD_NET
>>
>> [...]
>>
>>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
>>> index ef2cbf9..6fb78b3 100644
>>> --- a/drivers/mmc/omap_hsmmc.c
>>> +++ b/drivers/mmc/omap_hsmmc.c
>>> @@ -135,12 +135,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);
>>>
>>
> 

- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUVzTpAAoJEBDE8YO64EfavjcP/A0K8waMwn2XsBFgVyTLB7Q+
MUhufFKVHr/7ddFMutIQXzpG3BMzN+Q3Iwi3GzNCoJetHLBNkLAST4nqzTxYYuhu
uW16d+Jru40KsNX8JD1waoW2Igdp4AWtY30KT8Khh+OUNvXXbxExkedYPdDyoRz5
gmKigvGtMMyyWpclMqyXewLT6jJjmBEOkccmvbEWbu3xXf71Oxb2SSdM4fwZtdAU
PJIPW8wYHDQuYKNWhPUSsfiu/NQ63xs5/mW8YePKpwE54YCutT97Gvme05G+xUE9
ck4ZI5C49haHSq28HzVDmhXu8oAcCZke9998abqKD+3oDb5w72Hnoy6apx9ZtWSR
dRFJp/TMEG3sClftxI0zvZc85430On6TnzmSEb5LlO1HnmEF6QgKTEf6dwsjqCvD
WU0V1b0p3FyYxnIc+GlAWpis1pG+dtXiXkyLIEbI4Xcvnn7fo48kUVsrHGiENDcc
sCR2ewcnLxVtzQfbSs7AwTiThwxO9Nb5vsQh3Y+Z+1VLrZq5jOAnEB7EbgwqI/K8
snApLZyLFwiazOh/MVzxaonYUQRGYs0mLesjhXORyyQRcCvlUUSvS3AEMDV6EHP/
sRU8iXhZ1s3qlCVEL4pkSLiqQUJSZskumbt9/qrpavz4TPWDUSqc5/b5+7yqEPLm
dBofkwkw/9oqiUyPD0ps
=FA1/
-----END PGP SIGNATURE-----