From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Grinberg Date: Mon, 03 Nov 2014 09:55:21 +0200 Subject: [U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations In-Reply-To: <1414954916.17533.13.camel@collins> References: <1414839138-6587-1-git-send-email-contact@paulk.fr> <1414839138-6587-3-git-send-email-contact@paulk.fr> <54563430.30309@compulab.co.il> <1414954916.17533.13.camel@collins> Message-ID: <545734E9.7030208@compulab.co.il> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de -----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 >> >> 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-----