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

[Stephen Warren]

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?

> 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

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

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

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.

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.

-- 
nvpublic