[U-Boot] [PATCH v3 0/2] mmc: Board-specific MMC power initializations
Changelog:
v3: Changes to make v2 apply on master
v2: Implement board_mmc_power_init instead of define
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);
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);
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);
-----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.
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);
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);
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);
-----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.
participants (2)
-
Igor Grinberg -
Paul Kocialkowski