[U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model

[Simon Glass]

Hi Stephen,

On 5 May 2014 16:07, Stephen Warren <swarren at wwwdotorg.org> wrote:

> On 05/05/2014 03:30 PM, Simon Glass wrote:
> ...
> > I think you have it backwards...the current implementation has a
> > single level of hierarchy. Each driver handles one bank (or 'port' in
> > the case of Tegra). What you are talking about is having a single
> > driver handle multiple banks, thus requiring that the driver have a
> > second level to deal with banks, over and above the device. We might
> > end up with that, but I would prefer to move to it when we have
> > evidence that it is a general need.
>
> Sigh. This is getting silly. All the APIs in SW need to just take a GPIO
> ID in the flat range 0..N (N==223 for Tegra20) and deal with it.
> Anything that expose banks anywhere, either as a parameter to public
> functions exported from the GPIO controller driver, or as the existence
> of separate drivers for separate banks, or as a command-line argument
> that the user sees, or ..., whether it be the U-Boot GPIO core or the
> Tegra GPIO driver itself that causes this, is just pointless.
>

For exynos, the banks are not contiguous and there is quite a bit of
fiddling to make this all work. You may have seen the series going in at
the moment to number the GPIOs properly.

I understand that Tegra is very straightforward though.


> Please take a look at what the Linux driver does, and just model that.
> It's very trivial. The following is the entire implementation of
> gpio_set():
>
> > // helper macros used by all register accesses:
> > #define GPIO_BANK(x)            ((x) >> 5)
> > #define GPIO_PORT(x)            (((x) >> 3) & 0x3)
> > #define GPIO_BIT(x)             ((x) & 0x7)
> >
> > #define GPIO_REG(x)             (GPIO_BANK(x) * tegra_gpio_bank_stride +
> \
> >                                         GPIO_PORT(x) * 4)
> >
> > // one define per register
> > #define GPIO_MSK_OUT(x)         (GPIO_REG(x) + tegra_gpio_upper_offset +
> 0x20)
> >
> > // helper used by a lot of set/clear functions
> > static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
> > {
> >         u32 val;
> >
> >         val = 0x100 << GPIO_BIT(gpio);
> >         if (value)
> >                 val |= 1 << GPIO_BIT(gpio);
> >         tegra_gpio_writel(val, reg);
> > }
> >
> > static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int
> value)
> > {
> >         tegra_gpio_mask_write(GPIO_MSK_OUT(offset), offset, value);
> > }
>
> i.e. a single register write with a calculated register address, using
> just a few shifts/masks.
>
> >>>> It seems like this should be statically allocated (e.g. as part of
> *plat
> >>>> in gpio_tegra_bind() or something?). Still, if we stop splitting
> things
> >>>> into banks, we can completely get rid of this.
> >>>
> >>> No, we still need the name so that 'gpio input T3' works corrrectly.
> >>
> >> Why do we need GPIO names at all? "gpio input 155" works just as well,
> >> and to be honest is often a lot more convenient that trying to maps
> >> things back to a name.
> >
> > Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
> > number which drivers people back and forth to the datasheets, their
> > calculators, a long table, etc. Even the Tegra device tree has moved
> > away from numbers to GPIO names, I notice.
>
> The GPIO names are meaningless. I say this because all the Tegra
> schematics (and documentation that drives them) use the pin/pad name,
> which is almost always entirely different from the GPIO name. You have
> to map the pin name back to either the GPIO name or ID using a lookup
> table (such as the kernel's drivers/pinctrl/pinctrl-tegra20.c). Given
> the need for a lookup table, we should just use the simpler GPIO ID and
> not worry about GPIO names. There's no point screwing around with text
> names when we can just use simple numbers.
>
> In DT, GPIOs are specified by integer ID too. Admittedly we have macros
> that calculate those IDs from the bank/port/offset, but that was
> probably a mistake, since the bank/port/offset names aren't meaningful.
>

U-Boot provides a friendly named interface for GPIOs. I see that as a
requirement for driver model too. As someone who has spent a lot of time at
the command line fiddling with hardware, I don't want to go backwards in
the driver model conversion. Similarly, using numbers in the DT is very
unfriendly and painful IMO.

If we can agree on the friendly names, then let's talk about how to
simplify things.

Regards,
Simon