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

T30 requires specific SDMMC pad programming, and bus power-rail bringup.

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
v2:
- rewrite pad_init_mmc to use mmc_id instead of SDMMC base address
- add PMU-specific comments to board_sdmmc_voltage_init sequence

 board/nvidia/cardhu/cardhu.c |   51 ++++++++++++++++++++++++++++++++++++++
 board/nvidia/common/board.c  |   55 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 1 deletions(-)

On 03/05/2013 09:29 AM, Tom Warren wrote:
> T30 requires specific SDMMC pad programming, and bus power-rail bringup.

> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c

> +void pad_init_mmc(struct mmc_host *host)

> +	if (id == PERIPH_ID_SDMMC1) {
> +		val = readl(&gpc->sdio1cfg);
> +		val &= padmask;
> +		val |= padcfg;
> +		writel(val, &gpc->sdio1cfg);
> +		debug(" wrote 0x%08X to %p\n", val, &gpc->sdio1cfg);
> +	} else {
> +		val = readl(&gpc->sdio3cfg);
> +		val &= padmask;
> +		val |= padcfg;
> +		writel(val, &gpc->sdio3cfg);
> +		debug(" wrote 0x%08X to %p\n", val, &gpc->sdio3cfg);
> +	}

This isn't generic enough, although the problems may not show up for the
SDMMC1/3 controllers since it looks like those controllers always use a
specific set of pins; the pinmux HW doesn't allow those controllers to
be routed to different places.

However, SDMMC1 and SDMMC4 would also need entries in the above if
statement for it to be complete. Those controllers can definitely be
switched between different sets of pins. This function would then
somehow have to know which set of pins to apply the pinmux configuration
to, which would imply searching through the pinmux registers to find the
pins which have their mux function set to point at the controller in
question.

This implies that configuring the pinmux here isn't the right way to do
this. As I wrote in my immediately previous email, I think the pingroup
drive registers (named "*cfg") should be initialized based on a table
provided by the board file (and later by DT; the values are already part
of the DT pinctrl bindings). The table or DT content will be
board-specific, and hence able to describe which pingroup should be
configured based on the board design.

On Tue, Mar 5, 2013 at 10:55 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/05/2013 09:29 AM, Tom Warren wrote:
>> T30 requires specific SDMMC pad programming, and bus power-rail bringup.
>
>> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
>
>> +void pad_init_mmc(struct mmc_host *host)
>
>> +     if (id == PERIPH_ID_SDMMC1) {
>> +             val = readl(&gpc->sdio1cfg);
>> +             val &= padmask;
>> +             val |= padcfg;
>> +             writel(val, &gpc->sdio1cfg);
>> +             debug(" wrote 0x%08X to %p\n", val, &gpc->sdio1cfg);
>> +     } else {
>> +             val = readl(&gpc->sdio3cfg);
>> +             val &= padmask;
>> +             val |= padcfg;
>> +             writel(val, &gpc->sdio3cfg);
>> +             debug(" wrote 0x%08X to %p\n", val, &gpc->sdio3cfg);
>> +     }
>
> This isn't generic enough, although the problems may not show up for the
> SDMMC1/3 controllers since it looks like those controllers always use a
> specific set of pins; the pinmux HW doesn't allow those controllers to
> be routed to different places.
>
> However, SDMMC1 and SDMMC4 would also need entries in the above if
SDMMC1 is already there. I assume you mean SDMMC2.
> statement for it to be complete. Those controllers can definitely be
> switched between different sets of pins. This function would then
> somehow have to know which set of pins to apply the pinmux configuration
> to, which would imply searching through the pinmux registers to find the
> pins which have their mux function set to point at the controller in
> question.
Read the SDMMC section of the T30 TRM (24.6.1.2 - 5), and you'll see
that there is no config info given for SDMMC2 or SDMMC4 GP regs.
SDIO1CFG covers SDMMC1, SDIO3CFG covers SDMMC3 thru DAT3, and SDIO2CFG
covers the other 4 DAT pins, which are unused for SDIO on Cardhu.  So
there will never be a need to write to any GP SDIOxCFG registers for
SDMMC2 or SDMMC4 on Tegra30, at least as per this section of the TRM.
Note that it seems SDMMC2 doesn't even have a 'cfg' pingroup register
called out in Table 29, and SDMMC4 is split between 3 different
gm[abcd]cfg groups.  So the above code covers all SDIO pad cfg
settings as per the TRM.

>
> This implies that configuring the pinmux here isn't the right way to do
> this.
Disagree, see above.

> As I wrote in my immediately previous email, I think the pingroup
> drive registers (named "*cfg") should be initialized based on a table
> provided by the board file (and later by DT; the values are already part
> of the DT pinctrl bindings). The table or DT content will be
> board-specific, and hence able to describe which pingroup should be
> configured based on the board design.
That's certainly doable, and probably a good idea if there were any
other code that requires pad cfg to be tuned, but I don't see that in
the TRM anywhere. This patchset is specifically to enable MMC on
Tegra30. Adding pad_cfg_ctrl tables/macros, and code to handle them is
something I see happening in a separate patchset, if at all, since I
don't see any other pad cfg registers that ever need writing (beyond
SDMMC1/3).

On 03/05/2013 11:23 AM, Tom Warren wrote:
> On Tue, Mar 5, 2013 at 10:55 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 03/05/2013 09:29 AM, Tom Warren wrote:
>>> T30 requires specific SDMMC pad programming, and bus power-rail bringup.
>>
>>> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
>>
>>> +void pad_init_mmc(struct mmc_host *host)
>>
>>> +     if (id == PERIPH_ID_SDMMC1) {
>>> +             val = readl(&gpc->sdio1cfg);
>>> +             val &= padmask;
>>> +             val |= padcfg;
>>> +             writel(val, &gpc->sdio1cfg);
>>> +             debug(" wrote 0x%08X to %p\n", val, &gpc->sdio1cfg);
>>> +     } else {
>>> +             val = readl(&gpc->sdio3cfg);
>>> +             val &= padmask;
>>> +             val |= padcfg;
>>> +             writel(val, &gpc->sdio3cfg);
>>> +             debug(" wrote 0x%08X to %p\n", val, &gpc->sdio3cfg);
>>> +     }
>>
>> This isn't generic enough, although the problems may not show up for the
>> SDMMC1/3 controllers since it looks like those controllers always use a
>> specific set of pins; the pinmux HW doesn't allow those controllers to
>> be routed to different places.
>>
>> However, SDMMC1 and SDMMC4 would also need entries in the above if
>
> SDMMC1 is already there. I assume you mean SDMMC2.

Yes.

>> statement for it to be complete. Those controllers can definitely be
>> switched between different sets of pins. This function would then
>> somehow have to know which set of pins to apply the pinmux configuration
>> to, which would imply searching through the pinmux registers to find the
>> pins which have their mux function set to point at the controller in
>> question.
>
> Read the SDMMC section of the T30 TRM (24.6.1.2 - 5), and you'll see
> that there is no config info given for SDMMC2 or SDMMC4 GP regs.

The SDMMC section doesn't mandate a specific configuration. However,
almost all pins that are affected by the pinmux are included in some
group that has a "cfg" register with those exact same set of fields.
Hence, it is almost always possible to set some value in those fields.
We just didn't document what those values were for SDMMC2 and SDMMC4.

> SDIO1CFG covers SDMMC1, SDIO3CFG covers SDMMC3 thru DAT3, and SDIO2CFG
> covers the other 4 DAT pins, which are unused for SDIO on Cardhu.  So
> there will never be a need to write to any GP SDIOxCFG registers for
> SDMMC2 or SDMMC4 on Tegra30, at least as per this section of the TRM.

That doesn't follow.

First off, Cardhu is not the only Tegra30 board, so you can't generalize
based on which SDMMC controllers are used on Cardhu.

Second, Cardhu does actually use all of SDMMC1, SDMMC2, SDMMC4; the
other controller is for SDIO for WiFi. Admittedly it's fairly unlikely
we'll support SDIO-based WiFi in U-Boot, but we likely do want to at
least not preclude support for that due to the way the pinmux register
logic is written. Besides, we might also want to assume that U-Boot sets
up 100% of the pinmux registers; that's certainly the approach already
taken for the mux registers.

> Note that it seems SDMMC2 doesn't even have a 'cfg' pingroup register
> called out in Table 29, and SDMMC4 is split between 3 different
> gm[abcd]cfg groups.  So the above code covers all SDIO pad cfg
> settings as per the TRM.

That's exactly my point. The pinmux can route the signals from the other
two SDMMC controllers to different sets of pins. Those different sets of
pins are in different pin groups for the "cfg" registers. Hence, if you
extend this code in question to support all SDMMC controllers, as we
will almost certainly have to do for complete board support, we can't
hard-code what "cfg" registers we choose to write to here, since the
register is board-specific due to the pinmux configuration being
board-specific.

>> As I wrote in my immediately previous email, I think the pingroup
>> drive registers (named "*cfg") should be initialized based on a table
>> provided by the board file (and later by DT; the values are already part
>> of the DT pinctrl bindings). The table or DT content will be
>> board-specific, and hence able to describe which pingroup should be
>> configured based on the board design.
>
> That's certainly doable, and probably a good idea if there were any
> other code that requires pad cfg to be tuned, but I don't see that in
> the TRM anywhere.

The TRM is notoriously incomplete.

> This patchset is specifically to enable MMC on
> Tegra30. Adding pad_cfg_ctrl tables/macros, and code to handle them is
> something I see happening in a separate patchset, if at all, since I
> don't see any other pad cfg registers that ever need writing (beyond
> SDMMC1/3).

Again, the "cfg" registers are part of the pinmux HW, not MMC. If
there's a need to write to them, the pinmux driver needs to do it. We
shouldn't have the MMC driver randomly trample on registers owned by
other code.