[U-Boot] [PATCH] omap_hsmmc: Allow for board-specific MMC power init

[Igor Grinberg]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Paul,

On 10/28/14 20:11, Paul Kocialkowski wrote:
> Le mardi 28 octobre 2014 à 20:02 +0200, Igor Grinberg a écrit :
>> Hi Paul,
>>
>> On 10/28/14 19:25, 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 at paulk.fr>
>>> ---
>>>  arch/arm/include/asm/omap_mmc.h |    4 +++-
>>>  drivers/mmc/omap_hsmmc.c        |    4 +++-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h
>>> index 617e22f..b6a8325 100644
>>> --- a/arch/arm/include/asm/omap_mmc.h
>>> +++ b/arch/arm/include/asm/omap_mmc.h
>>> @@ -164,5 +164,7 @@ struct hsmmc {
>>>  int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio,
>>>  		int wp_gpio);
>>>  
>>> -
>>> +#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT
>>
>> I'm not a huge fan of that approach, but if you add
>> yet another CONFIG_ option, I think it is a requirement to add
>> a documentation for it.
> 
> That saddens me too, but I don't see how to do this in a better way for
> now.
> 
>>> +void omap_hsmmc_board_power_init(void);
>>
>> Anyway, I would suggest adding a default
>> __weak board_mmc_power_init() or something like this
>> (which would be transfered into a callback in pdata once omap_hsmmc.c is).
> 
> Well, there are two distinct scenarios here:
> 1. the board follows some reference design and VMMC1 is used to power
> MMC1, VMMC2 for MMC2, then twl4030 has a function that will set
> everything up correctly (with the addition I added earlier)
> 2. the board uses the regulators in a more or less random way and VMMC1
> is not mapped to MMC1 power at all. This is the case on the LG Optimus
> Black (P970). There is actually an auxiliary regulator IC that is in
> charge of powering MMC1. Hence the need for board-specific behavior.
> 
> I think it's perfectly fine to keep using the twl4030 function when the
> board follows the reference design.
> 
> Would that board_mmc_power_init you suggest be generic to all MMC
> drivers?

That would be perfect, I think.

> Then at which point should it be called? What would a generic
> board_mmc_power_init have?

Well, those are the question you usually should answer when you write
common code. I hopefully will have time to look at this next week.

> TWL4030 is specific to OMAP3.

Well, in practice, it is, but it does not have to be...
Also, it is a question of how the board is wired...

> Then perhaps
> there should be a board_mmc_power_init implementation for OMAP3, that
> can be overridden by a  board-specific function?

Or, as you have proposed already, a common board_mmc_power_init() function
and a board can choose to run the twl4030_power_mmc_init()?

> 
> I'm a bit new to the U-Boot code, so I'd appreciate pointers on what
> should go where.
> 
>> Or... just no need for this patch at all, as board_mmc_init()
>> can be used for this...
> 
> Not in the context of the SPL, which is precisely why I needed this.

This might lead to a question - is current spl flexible enough to let
a board deal with the mmc power?

Tom?

> 
> Thanks for your review!
> 
>>> +#endif
>>>  #endif /* OMAP_MMC_H_ */
>>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
>>> index ef2cbf9..ef4c5cf 100644
>>> --- a/drivers/mmc/omap_hsmmc.c
>>> +++ b/drivers/mmc/omap_hsmmc.c
>>> @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc)
>>>  	pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0);
>>>  	writel(pbias_lite, &t2_base->pbias_lite);
>>>  #endif
>>> -#if defined(CONFIG_TWL4030_POWER)
>>> +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT)
>>> +	omap_hsmmc_board_power_init();
>>> +#elif defined(CONFIG_TWL4030_POWER)
>>>  	twl4030_power_mmc_init();
>>>  	mdelay(100);	/* ramp-up delay from Linux code */
>>>  #endif
>>>
>>
> 

- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUUOcyAAoJEBDE8YO64EfaA00P/RRjiz3AyVqGJdVcN85eRim4
oYxr2/q5jaj67zD6x6UD4eT/VSnJQXaxNEmFQVNjoZc4U6U0V7qjrTC2/USh6v6Q
GROIH7zK/uP9+rRnJWJYdjAjZMJNShNmUvP9/oIb8ZtvVKG1aEuRoRWYwlyirJr/
cmtzeFS/TwaWSgFrtDWmCXuXzyKUdB+A/AOjAIj7HMZjKV+gaSZMSSyv5H6uQK+3
eI7KUskYvTJFCKKez0jnXv3IkhKjeJxy7eKQ147nfQMcZJ+dby3cbJuS3aZSG2xI
jx+w6ruSm4DMYJeCsEoWhW1wFJtKrtqZ4TyYEQeC9PWoON/8b67Jy+SYTHnVBkJk
qR7UFEyttUIE6zulD5rf2SlxS0e96HkLqNOfaI2K8rqbKuNOy+ZlA8TKsaVx5QAH
Fsxn494TY38vgBTYQbf74pv7psw2mPT//FUF//HqTuAID9ne6pMgM+j+WE0GwWOI
CZw9ddutxj1ksFu0kvnldvfZ6F4yd4aJFPUPQEDa6/qkVrKEW7Y3Alov5gHQMHQ8
zkhyv60s3owlmtcvkRtmb4LcRh+/lm8+gO9u7jmFmtiIhkCuEQA7IopqqYRdibXC
SiKyGH53TTLg/zocixO1/vY/a5C2YHADaVJ6Rzzea6IDUeHBoWTKhU4vUudjOb5z
NCH2ncJjDN8moX9CBENr
=3a3G
-----END PGP SIGNATURE-----