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

[Tom Warren]

Stephen,

On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.
Technically, they're not pinmux registers (PINMUX_AUX_ space), but GP
regs (APB_MISC_GP_ space). Since the pinmux _code_ (no pinmux driver
is used in Tegra U-Boot) for T30 is just a large table slam, I don't
think it makes sense to add GP pad reg writes there. These pads need
to be tuned when you've got a board w/an SD-card device hanging off of
them. So it makes sense to have these 2 register writes here in
pad_init_mmc(). I can take out the SDMMC1/3 test and just write both
SDIO1CFG and SDIO3CFG, since the values are the same.
>
> 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.
Why don't we get this in so we can move forward, and when there's a
kernel version of these params in the DT files, I can port it over.

I've got the compatible string changed to just 'nvidia,tegra30-sdhci'
in the dtsi file, and I've changed the pad_init_mmc() code to use the
mmc_id (PERIPH_ID_SDMMCx) instead of the base addresses to check for
an SDIO port before setting the pad config regs. I'll send that up as
V2 of the patchset, probably tomorrow.

Tom