Re: [PATCH 1/3] pinctrl: add driver for Amlogic Meson SoCs

[Linus Walleij]

On Wed, Oct 22, 2014 at 10:06 PM, Beniamino Galvani <b.galvani@xxxxxxxxx> wrote:
> On Tue, Oct 21, 2014 at 03:39:25PM +0200, Linus Walleij wrote:

>> > +static int meson_gpio_of_xlate(struct gpio_chip *chip,
>> > +                              const struct of_phandle_args *gpiospec,
>> > +                              u32 *flags)
>> > +{
>> > +       unsigned gpio = gpiospec->args[0];
>> > +
>> > +       if (gpio < chip->base || gpio >= chip->base + chip->ngpio)
>> > +               return -EINVAL;
>> > +
>> > +       if (flags)
>> > +               *flags = gpiospec->args[1];
>> > +
>> > +       return gpio - chip->base;
>> > +}
>>
>> Why is this necessary? We want to get rid of all use of
>> chip->base so introducing new users is not nice.
>
> The driver instantiates one pinctrl device with 136 pins and two
> gpio_chips, one with 120 gpios and the other with 16 (for more
> details, see below). The macros for pins in the header file use a
> single range from 0 to 135 that matches the order of pins in the
> pinctrl device:
>
>         /* First gpio-chip */
>         #define GPIOX_0         0
>         [...]
>         #define BOOT_18         119
>
>         /* Second gpio-chip */
>         #define GPIOAO_0        120
>         [...]
>         #define GPIO_TEST_N     135
>
> Since the macros are also used in DT as GPIO numbers, this function
> translates that value to the relative offset for the gpio chip.

That is not wise. You should let each gpio_chip register its
chip with base = -1, so they get whatever random GPIO numbers
they get, then register the pin range relatively to what they
get from the gpiochip side using gpiochip_add_pin_range().

>> The default of_gpio_simple_xlate() should be enough,
>> don't you agree?
>
> Probably yes, but for it to work then I have either to:
>  - change the pin macros to use a relative value
>
>         /* First gpio-chip */
>         #define GPIOX_0         0
>         [...]
>         #define BOOT_18         119
>
>         /* Second gpio-chip */
>         #define GPIOAO_0        0
>         [...]
>         #define GPIO_TEST_N     15
>
>  - or use a single gpio chip

Or (C) register the pin ranges from the gpio_chip side instead
of doing it from the pin controller side. Look at how
gpiochip_add_pin_range() works, I inisist.

>> > +       /* Copy group and function definitions from domains to pinctrl */
>> > +       pc->groups = devm_kzalloc(pc->dev, pc->num_groups *
>> > +                                 sizeof(struct meson_pmx_group), GFP_KERNEL);
>> > +       pc->funcs = devm_kzalloc(pc->dev, pc->num_funcs *
>> > +                                 sizeof(struct meson_pmx_func), GFP_KERNEL);
>> > +       if (!pc->groups || !pc->funcs)
>> > +               return -ENOMEM;
>>
>> Again more copying. Why can't we just have one set of this data
>> and only pass pointers?
>
> This code copies information on groups and functions from the
> different domains and merge it in a single array so that the driver
> can look it up easily.
>
> Probably the initial information should not be splitted so that it can
> be reused without additional copies.

Yeah I will look at V2 and see how it looks...

>> Why can't static arrays and ARRAY_SIZE() be used throughout
>> instead, just pass around pointers?
>
> The goal of the dynamic code is to simplify the declaration of groups,
> which can be done with a single statement in the form:
>
>         GROUP(_name, _reg, _bit, _pins...)
>
> for example:
>
>         GROUP(sdxc_d0_c,        4,      30,     BOOT_0),
>         GROUP(sdxc_d13_c,       4,      29,     BOOT_1, BOOT_2, BOOT_3),
>         GROUP(sdxc_d47_c,       4,      28,     BOOT_4, BOOT_5, BOOT_6, BOOT_7),
>         GROUP(sdxc_cmd_c,       4,      27,     BOOT_16),
>         GROUP(sdxc_clk_c,       4,      26,     BOOT_17),
>         GROUP(nand_io,          2,      26,     BOOT_0, BOOT_1, BOOT_2, BOOT_3,
>                                                 BOOT_4, BOOT_5, BOOT_6, BOOT_7),
>
> instead of having to define an array of pins and reference it in the
> group declaration. The same goes for functions. I admit that this is a
> bit hackish, I will stick to the classical way in the next respin.

OK thanks.

>> > +/**
>> > + * struct meson_domain
>> > + *
>> > + * @reg_mux:   registers for mux settings
>> > + * @reg_pullen:        registers for pull-enable settings
>> > + * @reg_pull:  registers for pull settings
>> > + * @reg_gpio:  registers for gpio settings
>> > + * @mux_size:  size of mux register range (in words)
>> > + * @pullen_size:size of pull-enable register range
>> > + * @pull_size: size of pull register range
>> > + * @gpio_size: size of gpio register range
>> > + * @chip:      gpio chip associated with the domain
>> > + * @data;      platform data for the domain
>> > + * @node:      device tree node for the domain
>> > + * @gpio_range:        gpio range that maps domain gpios to the pin controller
>> > + * @lock:      spinlock for accessing domain registers
>> > + *
>> > + * A domain represents a set of banks controlled by the same set of
>> > + * registers. Typically there is a domain for the normal banks and
>> > + * another one for the Always-On bus.
>> > + */
>>
>> Can I get a long-ish explanation of the domains vs banks etc
>> because that's really key to understanding this driver!
>
> Yes, I hope that the following explanation is clear enough. If anyone
> wants to know the details, the relevant information can be found in
> the Amlogic sources available at:
>
> http://openlinux.amlogic.com:8000/download/ARM/kernel/
>
> The GPIOs are organized in banks (X,Y,DV,H,Z,BOOT,CARD,AO) and each
> bank has a number of GPIOs variable from 7 to 30.
>
> There are different register sets for changing pins properties:
>  - for all banks except AO:
>         <0xc11080b0 0x28> for mux configuration
>         <0xc11080e4 0x18> for pull-enable configuration
>         <0xc1108120 0x18> for pull configuration
>         <0xc1108030 0x30> for gpio (in/out/dir) configuration
>
>  - for AO bank
>         <0xc8100014 0x4> for mux configuration
>         <0xc810002c 0x4> for pull and pull-enable configuration
>         <0xc8100024 0x8> for gpio (in/out/dir) configuration
>
> The regular banks belong to the "standard" power domain, while AO
> belongs to the Always-On power domain, which, like the name says,
> can't be powered off.
>
> Each bank uses contiguous ranges of bits in the register sets
> described above and the same register can be used by different banks
> (for example, for the pull-enable configuration the BOOT bank uses
> bits [0:18] of 0xc11080f0 and the CARD bank uses bits [20:25]).
>
> In addition to this, there seem to be some other registers, shared
> between all the banks, for the interrupt functionality.

I get it now I think, thanks!

Arguably the whole shebang is one big hardware unit so the right
thing is indeed to have a single driver for all of it.

> The driver then instantiates a gpio_chip for each subnode and a single
> pinctrl device.

This is the right design. We just need to hash out the details.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/