-----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.