[U-Boot] [PATCH V2 09/12] mmc: omap_hsmmc: add mmc1 pbias, ldo1

[Nishanth Menon]

On Wed, Jun 5, 2013 at 11:35 AM, Lubomir Popov <lpopov at mm-sol.com> wrote:
> Hi Nishanth,
>
> On 05/06/13 17:01, Nishanth Menon wrote:
>> On Wed, Jun 5, 2013 at 3:03 AM, Lubomir Popov <lpopov at mm-sol.com> wrote:
>>> Hi Tom,
>>>
>>> On 05/06/13 00:06, Tom Rini wrote:
>>>> On Mon, Jun 03, 2013 at 10:58:27PM +0300, Lubomir Popov wrote:
>>>>> Hi Lokesh,
>>>>>
>>>>>> Hi Lubomir,
>>>>>> On Thursday 30 May 2013 07:56 PM, Lubomir Popov wrote:
>>>>>>> Hi Lokesh,
>>>>>>>
>>>>>>> On 30/05/13 16:19, Lokesh Vutla wrote:
>>>>>>>> From: Balaji T K <balajitk at ti.com>
>>>>>>>>
>>>>>>>> add dra mmc pbias support and ldo1 power on
>>>>>>>>
>>>>>>>> Signed-off-by: Balaji T K <balajitk at ti.com>
>>>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
>>>>>>>> ---
>>>>>>>>   arch/arm/include/asm/arch-omap5/omap.h |    3 ++-
>>>>>>>>   drivers/mmc/omap_hsmmc.c               |   26 ++++++++++++++------------
>>>>>>>>   drivers/power/palmas.c                 |   25 ++++++++++++++++++++++++-
>>>>>>>>   include/configs/omap5_common.h         |    4 ++++
>>>>>>>>   include/configs/omap5_uevm.h           |    5 -----
>>>>>>>>   include/palmas.h                       |    6 +++++-
>>>>>>>>   6 files changed, 49 insertions(+), 20 deletions(-)
>>>>>>>>
>>>>> [snip]
>>>>>>>> + /* set LDO9 TWL6035 to 3V */
>>>>>>> LDO9? TWL6035? If this function is used on the DRA7xx boards only (with
>>>>>>> TPS659038), you should add some comment above.
>>>>>> Ok ll add the comment.
>>>>>>>
>>>>>>>> + val = 0x2b; /* (3 - 0.9) * 20 + 1 */
>>>>>>> Why not use definitions for the voltage? You could take them from
>>>>>>> http://patchwork.ozlabs.org/patch/244103/ where some values are
>>>>>>> defined.
>>>>>> Yes, Ill rebase this patch on top of your patch and use those defines.
>>>>> Please be aware that my above mentioned patch has not been reviewed/
>>>>> tested/acked/nacked/whatever by nobody (except possibly a quick look by
>>>>> Nishanth Menon, who had some objections). I wrote it when bringing up a
>>>>> custom OMAP5 board, and most probably it shall not go into mainline in
>>>>> its current form, if ever. I gave it only as an example of how things
>>>>> could be done cleaner. Feel free to use the code as you wish, but I'm
>>>>> afraid that applying it as a patch to your tree and basing upon it might
>>>>> run you into problems when you later sync with mainline.
>>>>>
>>>>> Tom, your opinion?
>>>>
>>>> OK, so at the time it was "nothing will really use this code except test
>>>> functions".  Looks like we have a use for mmc1_ldo9 code at least, so
>>>> lets rework the first patch for adding that + cleanups wrt constants.
>>> Well, I'm not quite sure that this LDO9 function would be the only one
>>> used (or LDO1 on the DRA7xx board). Judging from omapboot for the OMAP5
>>> boards for example, SMPS7 (it delivers the common 1.8 V I/O supply) is
>>> set to 'Forced PWM' mode in order to reduce board noise - there sure has
>>> been a reason to do so and sacrifice converter efficiency. Therefore I
>>> added similar functionality in my patch to the Palmas driver (and am
>>> explicitly calling it in my board init).
>>> The option to bypass LDO9 on OMAP5+TWL603x boards seems quite mandatory
>>> as well, if hardware is designed such that the SD card socket has a
>>> separate fixed 3.3 V supply which also powers the LDO9 input (the
>>> uEVM for example). On the DRA7xx+TPS659038 board the power scheme is
>>> different and this does not apply.
>>>
>>
>> I hate this code for many reasons -
>> a) hsmmc is used on many OMAP and DM platforms to my knowledge.
>> b) what is being done here is to power on the LDO supplying MMC.
> Sorry, but I can't get if hsmmc is discussed here, or power.
>
> For OMAP5+TWL603x the LDO powering MMC (actually the removable card
> interface only; eMMC is another story) is turned on automatically at
> power-on by the PMIC sequencer, with a default voltage and mode --
> otherwise we would not be able to boot from a card (ROM code does not
> touch the PMIC at all). We are talking here about the possibility to
> have additional control over this LDO, which should be board-specific,
> I agree. On the OMAP5 boards, for example, the call to
> palmas_mmc1_poweron_ldo() from within omap_hsmmc actually does not
> turn on LDO9 - it is on at this moment anyway. The call just makes
> it switch from the default bypass mode (with Vout = Vin = 3.3 V) to
> regulation mode and Vout = 3.0 V. Why is this done is yet another
> question; to me it seems useless (and possibly wrong) when the card
> is powered with a fixed voltage of 3.3 V.
> Therefore it seems reasonable to count on the PMIC defaults and
> remove this call from omap_hsmmc altogether, thus disengaging the
> PMIC driver from hsmmc, at least for OMAP5.
>
> For OMAP4 things are somewhat different. Here the TWL6030 PMIC powers
> both the OMAP interface and the card socket, and in addition can
> automatically power off the MMC LDO upon detecting card removal.
> ROM code *does* access the MMC LDO to turn it on and set it to 3.0 V
> (it starts by default at 1.8 V), but only if booting from a card.
> So here the call to PMIC driver should stay.
>
> Other OMAPs and derivatives - other scenarios.
>
> Anyway, omap_hsmmc.c is built for TI platforms only. If you mean
> the #ifdefs here, yes, things could be cleaned up by moving the SoC-
> specific pbias stuff to the corresponding board files (with the
> expense of redundancy), but this is quite an amount of work... I'm
> not volunteering... ;) Moreover, this particular patch is not mine.
>
I understand approximately why we do this, but there are more than a
single MMC on OMAP, in general. and different platforms use different
PMICs
I am just looking at http://patchwork.ozlabs.org/patch/248928/
and I am like - wait a minute, how many #ifdef #else is one going to
add per MMC/board? switching on LDOx meant for MMC1 will work for
MMC2?
makes no sense to me.
is'nt better for struct mmc to have a function pointer which is
populated by the board(depending on PMIC) to setup voltage necessary?

the trouble I have is not that omap_hsmmc is meant for TI SoCs. I get
that. but the fact that the code is starting to look so convoluted
that it will ONLY work on TI boards!!! I dont get that!

Regards,
Nishanth Menon