[U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC power initializations
Igor Grinberg
grinberg at compulab.co.il
Mon Nov 3 08:55:21 CET 2014
-----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 at 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-----
More information about the U-Boot
mailing list