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

[Tom Warren]

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.

>
>> +{
>> +     uchar reg, data_buffer[1];
>> +     int i;
>> +
>> +     i2c_set_bus_num(0);     /* PMU is on bus 0 */
>> +
>> +     data_buffer[0] = 0x65;
>> +     reg = 0x32;
>
> We should at least comment what those register numbers and values mean.
Unfortunately, that's not documented in any downstream/NV-generated
code that I've seen. I'll have to find the PMU spec and decode what
these writes are doing.

>
> 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?

>
>> 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?

Tom