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

[Lokesh Vutla]

Hi Lubomir,
On Thursday 06 June 2013 12:55 PM, Lubomir Popov wrote:
> Hi Tom,
>
> On 05/06/13 16:45, Tom Rini wrote:
>> On Wed, Jun 05, 2013 at 11:03:26AM +0300, Lubomir Popov 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.
>>
>> OK, lets see.  That so lets keep your patch as-is, since we've now got
>> -ffunction-sections/-fdata-sections/--gc-sections on ARM for main
>> U-Boot, these small things won't hurt like they used to.
>>
> OK, but then I would like to do some cleanup first - remove the audio
> power stuff (shall have it in my board file), as well as either sort out
> the function naming:
>
> - Those functions that are specific to a SoC+PMIC combination are
> named e.g. twl603x_... or tps659038_... so that they explicitly
> indicate the hardware that they are working with (actually almost all
> functions are such). This is however sort of regression, and requires
> fixes in the files calling these functions;
>
> or, alternatively:
>
> - Introduce generic functions with fixed names, palmas_bla_bla(),
> sort of wrappers, which in their bodies perform the appropriate action
> based on the #ifdefs defining the platform hardware (where we could also
> define the particular LDO which for example a palmas_mmc1_poweron_ldo()
> generic function would manipulate). Drawback: again #ifdefs.
> Advantage: single place where this stuff is located, and where other
> PMIC/LDO combinations can be added without affecting other code.
I think, we can have function pointers for and can populate data in the
beginning or from board file based on Soc, similarly what we did for
prcm structure.
Regards,
Lokesh
> And this generic palmas_mmc1_poweron_ldo() function would be called
> by another generic function, e.g. omap_sdmmc_poweron(), located in
> the board file, only if needed by the particular hardware.
> omap_sdmmc_poweron(), on its hand, is the function that is to be called
> from within the pbias routines in omap_hsmmc.c, and not the hardware-
> dependant functions directly. So we get the abstraction.
>
> What do you think? Lokesh, your opinion?
>
> Regards,
> Lubo
>