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

[Igor Grinberg <grinberg@compulab.co.il>]

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