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

[Tom Warren]

On Mon, Mar 4, 2013 at 5:28 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 03/04/2013 04:11 PM, Tom Warren wrote:
>> 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)
>
> The distinction isn't relevant for this discussion. Anyway, there really
> is a driver...
I must be dense. Please point it out to me. It is relevant, because
you've asked to put the GP register writes in the pinmux driver.

>
>> 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.
>
> Are these value board-specific or SoC-specific? I thought that the
> values came from the TRM and simply had to be set as described.
>
> If the values are SoC-specific, I think we can just key off the
> compatible value to determine whether to apply them.
Correct. They are SoC-specific as per the TRM. I am using the mmc_id
(periph_id value) to determine whether to write SDIO1CFG or SDIO3CFG
in V2, going up this morning.

>
> If the values are board-specific, I really think we must put them into
> device tree. Otherwise, we cannot claim that we have actually supported
> DT in the driver - we're only doing half the job, and can't support new
> boards simply by providing a new DT.
>
> The one out might be that we could claim the current values are the
> default if the DT provides no other values, even once we do define a way
> for DT to provide those values. That would at least be
> backwards-compatible. However, that's again going down the path of
> initially defining the minimal DT binding required to get some system
> running, but planning to incrementally enhance the DT binding later.
> While we aren't getting strong pushback on this right now from upstream,
> I can guarantee we will do very soon, so we really ought to just get
> into the habit of completely defining DT bindings from the start. Still,
> I suppose I won't nak this patch because of that yet.