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.
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
