From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lokesh Vutla Date: Thu, 6 Jun 2013 16:56:51 +0530 Subject: [U-Boot] [PATCH V2 09/12] mmc: omap_hsmmc: add mmc1 pbias, ldo1 In-Reply-To: <51B0395F.7050401@mm-sol.com> References: <1369919979-26497-1-git-send-email-lokeshvutla@ti.com> <1369919979-26497-10-git-send-email-lokeshvutla@ti.com> <51A7617B.8040503@mm-sol.com> <51AC7790.8010600@ti.com> <5573fc255a96bc1866e356d767a2b44b.squirrel@www.mm-sol.com> <20130604210631.GA10720@bill-the-cat> <51AEF0CE.1000500@mm-sol.com> <20130605134555.GU10720@bill-the-cat> <51B0395F.7050401@mm-sol.com> Message-ID: <51B071FB.5060601@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>>> >>>>>>>> add dra mmc pbias support and ldo1 power on >>>>>>>> >>>>>>>> Signed-off-by: Balaji T K >>>>>>>> Signed-off-by: Lokesh Vutla >>>>>>>> --- >>>>>>>> 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 >