[U-Boot] [PATCH 3/5] Tegra30: MMC: Add SD bus power-rail and SDMMC pad init routines

[Stephen Warren]

On 02/27/2013 09:59 AM, Tom Warren wrote:
> Stephen,
> 
> On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 02/26/2013 01:46 PM, Tom Warren wrote:
>>> T30 requires specific SDMMC pad programming, and bus power-rail bringup.
>>
>>> diff --git a/board/nvidia/cardhu/cardhu.c b/board/nvidia/cardhu/cardhu.c
>>
>>> +/*
>>> + * Do I2C/PMU writes to bring up SD card bus power
>>> + *
>>> + */
>>> +void board_sdmmc_voltage_init(void)
>>
>> We really shouldn't be adding to board files if we're remotely serious
>> about device tree; the whole point of DT is to remove code from the
>> board files, and describe the desired configuration as data in DT instead.
>>
>> This function should be replaced by regulator nodes/properties in the
>> device tree, and the MMC (core?) driver calling into the board-specific
>> regulator driver to request the desired voltages.
>>
>> But so long as we file a bug to replace this code with an explicit
>> regulator driver in the future, I guess it's fine for now.
>
> I'll file a bug for doing all PMU/power rail work from DT. I think it
> makes much more sense as a separate (non-MMC) patch.

Yes, certainly a separate patch. Ideally it'd be implemented before
other code that relies on it. That's why I think we need to take a
higher-level look at DT support in U-Boot, rather than simply finding
these issue accidentally while implementing the features we already know
we need.

>> BTW, I just noticed that commit f01b631 "Tegra30: Add/enable Cardhu
>> build (T30 reference board)" adds a file called
>> board/nvidia/cardhu/cardhu.c.mmc. That's a mistake, right?
>
> Yep, that's the Cardhu file I was hacking on for MMC support way back
> when. I can remove it in V2 of these patches, or would you prefer a
> single, separate patch to do that?

Is the commit that adds that file already pulled into higher-level
repos? If not, I would simply rebase it to remove that file. If it has,
then a separate patch to delete it before this series would make sense.

>>> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
>>
>> Hmm. This seems like SoC code, not board code...
>>
>>> +void pad_init_mmc(struct tegra_mmc *reg)
>>> +{
>>> +#if defined(CONFIG_TEGRA30)
>>
>>> +     /* Set the pad drive strength for SDMMC1 or 3 only */
>>> +     if (offset != TEGRA_SDMMC1_BASE && offset != TEGRA_SDMMC3_BASE) {
>>> +             debug("%s: settings are only valid for SDMMC1/SDMMC3!\n",
>>> +                     __func__);
>>> +             return;
>>> +     }
>>
>> Perhaps pass in the MMC instance ID instead of the base address. That'd
>> avoid having to know the base addresses in this code.
>
> I still need to know if I've got a SDIO or eMMC ID, though, and I
> don't think the flags for that in the mmc/host structs (mmc->version,
> etc.) get populated until the mmc driver has done some I/O with the
> device (eMMC or SD-card), and I need to set up the pads before that.
> 
>> In fact, just putting this code into the pinmux driver (which owns these
>> registers) seems like a better idea; there's no need to only do this
>> when the SD controller is enabled, is there?
>
> Half of these regs are in the SD controller register space
> (sdmemcmpctl and autocalcfg), and get reset when the controller gets
> reset (mmc_reset). So they need to be set each time a reset occurs. It
> makes sense to keep the GP SDIOCFG writes here, too.
> 
> As to where the pad_init_mmc function belongs, it is SoC-specific,
> yes. T20 doesn't need it (no sdmemcmpctl or autocalcfg regs on T20
> SDMMC), and T30 and T114 use slightly different bits/values for GP
> SDIOCFG and sdmemcmpctl/autocalcfg. But the differences are small
> enough that I thought this routine should be placed in a common area,
> and not duplicated for each SoC, so I put it here.
> 
> Do you have a suggestion of where it would be better placed?

For the pinmux registers, I think they should be programmed by the
pinmux driver at the same time as the rest of the pinmux is programmed.

For the SD registers, I guess we need to program them from the SD driver
as you say, but need to add some more properties to the DT in order to
parameterize this.