[U-Boot] [PATCH v2 3/4] tegra2: Enable second SD port (J5) on Harmony

[Simon Glass]

Hi Stephen,

Have been on hols, sorry for the delay.

On Wed, Oct 5, 2011 at 8:46 AM, Stephen Warren <swarren at nvidia.com> wrote:
> Simon Glass wrote at Wednesday, October 05, 2011 8:39 AM:
>> Hi Stephen,
>>
>> On Tue, Oct 4, 2011 at 10:32 AM, Stephen Warren <swarren at nvidia.com> wrote:
>> > Seaboard uses SDMMC4, SDMMC3. Harmony uses SDMMC4, SDMMC2. Move
>> > board_init_mmc and gpio_config_mmc into board-specific files, so boards
>> > can choose which ports to set up. Split clock_init_mmc and pin_mux_mmc
>> > into per-port functions, and export them for use by boards.
>> >
>> > Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>
>> I am not thrilled with this patch. It seems to create a lot of new
>> functions, moves knowledge of MMC clock speed into the board file
>
> By "board file", do you mean board.c (which already had and still has this
> knowledge) or harmony.c/seaboard.c?

Yes, I'm afraid I am comparing the newer code in the Chromium tree to
your patch. It is true that we moved the clock setting into the driver
in our tree, since it already has to do it in response to MMC events,
and it seems wrong to have this 20MHz thing sitting in the board.

>
>> and means that we will always init MMC no matter what the CONFIG
>> settings say.
>
> All the MMC init calls are already only enabled based on config settings;
> they're wrapped inside:
>
> #ifdef CONFIG_TEGRA2_MMC

Yes - but I suppose if the code were in the driver then you wouldn't
need any #ifdefs - the code would just not be included in the build.
We will end up with a lot of #ifdefs in the board files (board.c,
seaboard.c etc.) if we go down this route.

>
>> From a boot time perspective we want to init only those periphs that
>> we need to use to boot. If we set up clocks and pinmuxes in the board
>> init, then this is not possible. We had a discussion on the list a few
>> months back about how to minimize this and the answer given was that
>> we should only init peripherals when they are used.
>
> Well, nothing in my patch changes anything related to that.
>
> I wasn't subscribed then, so I don't know exactly what was discussed. My
> take: It seems reasonable to defer /driver/ initialization to when a
> device is first used, but I'm not convinced it's reasonable to defer
> system-level initialization such as pinmux/gpio-ownership. By either way,
> that's beyond what these patches aim to do.

No you are right - I was thinking of the Chromium code base.

>
>> Looking ahead to fdt, I think the control point for what gets inited
>> and what the pinmux settings are will be in the device tree. My
>> understanding of the concept is that drivers will look at their fdt
>> node and init accordingly.
>
> Sure.
>
>> We already have a function:
>>
>> int tegra2_mmc_init(int dev_index, int bus_width)
>>
>> Could this not be enhanced to set the pinmux and clocks? It seems from
>> your patch that the pinmux is always fixed but if not it could be a
>> parameter.
>
> Yes, it does make perfect sense to pass the GPIOs into this function. I
> will rework the patches to do that. Note that the aim of my patches wasn't
> to re-factor the driver, but to follow existing practices though. This
> would then move the CD handling into the MMC driver. Note however that
> the CD function isn't actually used for Tegra; simply removing it may be
> the best change at this point?
>
> Perhaps the clock initialization should be moved into the driver too?
>

Not sure about removing CD - but certainly if the GPIO is available to
the driver then it might as well implement it.

> For the pinmux, I'm not convinced the MMC driver should be controlling
> this. It remains to be seen exactly what the DT binding is for pinmux.
> The pinmux information may or may not be related to the MMC nodes in the
> DT. I wouldn't want to make the MMC driver control the pinmux until that
> question was answered.

Yes that's the big question. If pinmux is driver-related then it will
make just-in-time device init a lot easier. I agree it is better to
resolve this later when all is known.

>
> Note that there certainly are different pinmuxing options for the MMC
> ports, or at least some of them, so the mux settings are board-specific.
> It just so happens that for ports that multiple currently-supported
> Boards use, all boards happen to use the same pinmux settings for that
> port. In the future, I could see say pin_mux_mmc4 being split into e.g.
> pin_mux_mmc4_xxx and pin_mux_mmc4_yyy or even just making the pinmux calls
> directly from individual board files if we end up supporting a lot of
> options.

OK I see - but I feel that having lots of pin_mux_mmcx_yyy functions
is not great. I depends on where this goes in the kernel, as you say,
but I have to express a preference for telling the driver what setting
is required.

Regards,
Simon

>
> --
> nvpublic
>
>