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

[Simon Glass]

Hi Stephen,

On 5 May 2014 11:29, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 05/05/2014 10:09 AM, Simon Glass wrote:
>> This is an implementation of GPIOs for Tegra that uses driver model. It has
>> been tested on trimslice and also using the new iotrace feature.
>>
>> The implementation uses a top-level GPIO device (which has no actual GPIOS).
>> Under this all the banks are created as separate GPIO devices.
>
> I don't think that dividing the GPIOs into banks is appropriate. While
> the textual naming of Tegra GPIOs is based on the bank concept, I think
> that division should only apply to the textual naming, and not any data
> structures or device registration, etc. In fact, I often refer to Tegra
> GPIOs using their integer ID rather than their name, and dividing the
> controller into separate banks probably technically disallows this,
> since there would be no architected guarantee that the numbering of the
> banks would be sequential. Contiguous numbering of all GPIOs within the
> controller is also useful for correlation with pinmux numbering of pins.

This is how the GPIO uclass works at present. Do you think that should
change also? It would mean enhancing the uclass to support multiple
GPIO bank names, with each having a number of GPIOs attached. This is
the sort of complexity that adds to code size. On the other hand it's
something that we may want to do anyway as more SOC's GPIO drivers are
converted.

An alternative might be to leave most the Tegra driver (the static
functions at the top) using an absolute GPIO number, but have a
separate device for each bank. That would be easy enough and would not
add to code.

>
> ...
>> Since driver model is not yet available before relocation, or in SPL, a
>> special function is provided for seaboard's SPL code.
>
> Perhaps we should just remove that code from Seaboard, since sharing the
> UART/SPI pins really isn't a useful feature.

saveenv to SPI won't work without it though.

>
> Still, we will need to maintain some low-level APIs so that the SPL can
> initialize GPIOs at boot, since doing so is part of Tegra's HW-defined
> pinmux initialization mechanism. For reference, see the series I posted:
>
> https://www.mail-archive.com/u-boot@lists.denx.de/msg136607.html
>
> In particular:
> ARM: tegra: add GPIO initialization table function
> ARM: tegra: make use of GPIO init table on Jetson TK1

OK.

>
>> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
>
>> +struct tegra_gpio_platdata {
>> +     struct gpio_ctlr_bank *bank;
>> +     const char *port_name;  /* Name of port, e.g. "B" */
>> +     int base_port;          /* Port number for this port (0, 1,.., n-1) */
>> +};
>
> I'm not sure what "platdata" means. Would "tegra_gpio_bank" be a better
> name?
>
> Oh, looking later in the patch, this seems to be about passing data to
> probe(). It would be better to get rid of the bank concept, have a
> single device, and just pass the GPIO count or SoC type to probe.

Probably better to improve the name - I wanted to distinguish between
platform data and run-time data.

>
>> +/* Information about each port at run-time */
>> +struct tegra_port_info {
>> +     char label[TEGRA_GPIOS_PER_PORT][GPIO_NAME_SIZE];
>> +     struct gpio_ctlr_bank *bank;
>> +     int base_port;          /* Port number for this port (0, 1,.., n-1) */
>> +};
>
> It seems odd to have two data-structures with Tegra-specific data
> related to a struct gpio_ctrl_bank. Perhaps these can be combined into one?

One is static platform data (not supposed to be changed). It is
created before the device is bound. The other is the run-time
information, that only exists when the device is active / probed.

>
>> +#define GPIO_NUM(state, offset) \
>> +     (((state)->base_port * TEGRA_GPIOS_PER_PORT) + (offset))
>
> It's not clear whether "state" is supposed to be a "tegra_gpio_platdata"
> or a "tegra_port_info". I guess the macro works with either, but that
> seems a bit dangerous.

Should probably be a function.

>
>>  /* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
>> +static int get_config(struct tegra_port_info *state, int offset)
>
> Hmm. I guess the name "state" for the GPIO_NUM() macro parameter comes
> from the fact that the functions in this file have a parameter named
> "state". That seems nasty; it'd be better to name that port_info so it
> represents that type it contains. Still, that's a pre-existing issue
> unrelated to this patch.
>
> That said, naming a macro parameter "state" and using it in functions
> that typically have a parameter named "state" seems dangerous. Perhaps
> prefix and suffix the macro parameter name with _ to make the
> distinction obvious - i.e.. _state_. That's typical good practice for
> macros.

OK

>
>> +static int tegra_gpio_get_state(struct device *dev, unsigned int offset,
>> +                             char *buf, int bufsize)
>
>> +     is_gpio = get_config(state, offset);            /* GPIO, not SFPIO */
>
> Typo in SFIO there.
>
>> +     size = snprintf(buf, bufsize, "%s%d: ",
>> +                     uc_priv->bank_name ? uc_priv->bank_name : "", offset);
>> +     buf += size;
>> +     bufsize -= size;
>
> size can be larger than bufsize if the string would have overflowed. Do
> the later calls to snprintf() handle negative size arguments correctly?

Should do - I checked that when I upstreamed that code.

>
>> +static char *gpio_port_name(int base_port)
>>  {
>> +     char *name, *s;
>> +
>> +     name = malloc(3);
>
> 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.

>
>> +static int gpio_tegra_bind(struct device *parent)
>
>> +     /*
>> +      * This driver does not make use of interrupts, other than to figure
>> +      * out the number of GPIO banks
>> +      */
>> +     if (!fdt_getprop(gd->fdt_blob, parent->of_offset, "interrupts", &len))
>> +             return -EINVAL;
>> +     bank_count = len / 3 / sizeof(u32);
>> +     ctlr = (struct gpio_ctlr *)fdtdec_get_addr(gd->fdt_blob,
>> +                                                parent->of_offset, "reg");
>
> It's dangerous to assume that #interrupt-cells of the GIC is 3. The GIC
> driver is the only thing that can define/parse GIC IRQ specifiers...

Which GIC driver would that be? :-)

>
>> diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
>
>>  #define CONFIG_DM
>> +#define CONFIG_DM_GPIO
>> +#define CONFIG_CMD_DM
>
> I think the second of those lines should be part of patch 12/13
> shouldn't it?
>

Yes, will move it.

Thanks for looking at this. BTW if we decide to change the bank
approach later I'm happy to adjust this driver.

Regards,
Simon