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

Wed Jun 5 22:11:01 CEST 2013

Hi Nishanth,

> 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.
Not quite so. The main goal in U-Boot is to guarantee proper setup
(even if power is provided prior automatically by the PMIC, or by
pure iron hardware) of the interface to the MMC channel connecting
removable media, which therefore must, by spec, support dual-voltage
signalling. Usually (and for the TI SoCs known to me, actually) we
have only one such dedicated channel, which is specific in respect
to internal silicon implementation of the interface: all that pbias
stuff exists here only, and requires some special setup. If we are
using a matching TI PMIC, we have also a dedicated LDO to power this
interface. The other MMC channels are not a concern here and should
not be messed with; they may have dedicated LDOs or may have not (TI
PMICs, for example, power on a eMMC automatically, and even have a
pin-configurable option to select the default eMMC core voltage).

> is'nt better for struct mmc to have a function pointer which is
> populated by the board(depending on PMIC) to setup voltage necessary?
Anyway U-Boot, at the hsmmc driver level, assumes that if we are
using a MMC channel, it is operational, that is, powered on, and
does not care how this is achieved. I don't think it is reasonable
to alter the mmc struct.

What could be more appropriate is perhaps to define a generic
omap_sdmmc_poweron() function that is called from within the
pbias setup routines for each SoC (can't avoid the #ifdefs here,
I'm afraid), which is to be implemented in the board files as
required by the particular platforms; could be empty if nothing
has to be done in software, or call the existing PMIC functions
(palmas_mmc1_poweron_ldo or whatever) for TI boards.

>
> 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
>
Best regards,
Lubo