[U-Boot] [PATCH v1 02/11] sunxi: add pinctrl (UCLASS_PINCTRL) support for sunxi and tie back into GPIO

[Chen-Yu Tsai]

On Tue, Feb 21, 2017 at 5:39 PM, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
> Chen,

It's ChenYu. :)

>
> On 21 Feb 2017, at 04:48, Chen-Yu Tsai <wens at csie.org> wrote:
>
> On Sat, Feb 18, 2017 at 1:52 AM, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>
> This change adds a full device-model pinctrl driver for sunxi (tested with
> sun50iw1p1) based on the support available in Linux.
>
> Details are:
> * implements a driver for pinctrl devices and assigns sun50i-a64-pinctrl
>   and sun50i-a64-r-pinctrl to it
> * defines and implements a binding for sunxi-style GPIO banks (to make it
>   easier to describe controllers like the A64 which may not start at 'A')
>   and provide the necessary translation mechanisms:
>   - As all our gpio-reference point back to either <&pio ..> or <&r_pio ..>
>     the device framework will try to access a UCLASS_GPIO device bound to
>     the same node id as the pinctrl device to perform it's of_xlate lookup.
>     For this, we provide a 'gpiobridge' driver (which needs to access the
>     platdata of the pinctrl device) and which can then map accesses to an
>     actual GPIO bank device.
>   - For the individual GPIO banks, we use a new driver (which shares most
>     of its ops with the existing sunxi_gpio driver, except probe and bind)
>     and provides configuration without any platdata structure.
>
>
> Why is the new binding and driver necessary? The existing driver in U-boot
> is
> already DM enabled and properly xlates GPIO phandles to its child GPIO
> banks.
> I specifically fixed this in commit 4694dc56e974 ("sunxi: gpio: Add .xlate
> function for gpio phandle resolution”).
>
>
> The problem is that all GPIO is referenced to the pinctrl-node.  As we need
> to
> provide a pinctrl driver (so the configuration of pins can go through that)
> for
> the pinconfig to work, the GPIOs are then referenced to that node.
>
> We had considered multiple solutions
> (a) the one we’ve chosen, which avoids having to provide additional data
> outside the device-tree to track the number of banks and bank-size

You already have this data in the pinctrl driver.

> (b) binding the existing gpio driver to the same node (but as the driverdata
> needs to be matched as well, we’d need to select this based on the
> compatible string and have a tighter coupling between the drivers than
> strictly necessary)

They really should be the same driver. They control the same piece of hardware,
just exporting different functions to the 2 subsystems. There should be proper
locking between the 2, as Allwinner's hardware cannot simultaneously mux a pin
to some function and provide GPIO functionality. The user should not be able
to request a GPIO on a pin that is already claimed by a UART or MMC card.

> (c) dynamically creating gpio subnodes (just as done by the top-level
> sunxi_gpio instances) for each of the banks from within the pinctrl driver
>
> Another reason why we felt the gpiobank to be helpful is that it allows us
> to
> easily describe where the register sets (i.e. PIO and IRQ) are for each
> bank.

These are always at fixed multiple offsets in Allwinner's design though.
The driver can always calculate which registers it needs to access from
the pin name/number. IRQ bank relations are also encoded in the pinctrl
tables you imported. Everything needed is available in code.

>
> You are also not using the new gpiobank nodes anywhere in your dts changes.
>
>
> The DTS change is in the same patch as the the pinctrl driver:
> http://lists.denx.de/pipermail/u-boot/2017-February/281637.html
>
> I had generated this with full function context, which makes the diff in the
> DTS
> hard to spot.

What I meant was you are not actually referencing them. Since you can do
without them, what's the point of adding them, beyond making the driver
slightly easier to write?

>
> +#if defined(CONFIG_SUNXI_PINCTRL)
> +
> +/* When we use the sunxi pinctrl infrastructure, we have two issues that
> + * are resolved by the gpiobank and gpiobrige drivers:
> + *
> + *  - We need to have a UCLASS_GPIO device bound to the pinctrl-nodes for
> + *    translating between gpio entries (e.g. <&pio 3 24 GPIO_ACTIVE_LOW>;)
> + *    in the DT and actual gpio banks. This is done using the gpiobridge
> + *    driver, which provides only a lookup/translation mechanism.
> + *    This mechanism lives in pinctrl-sunxi.c
> + *
> + *  - We introduce a generic gpiobank device, which resolves the need to
> + *    have distinct soc_data structure for each device and avoids having
> + *    to share data structures and config data between this file and the
> + *    sunxi pinctrl (the other option would be to have the soc_data info
> + *    visible in pinctrl-sunxi.c (or merge it into this file) and bind a
> + *    gpio_sunxi device that is set up with the appropriate soc_data) to
> + *    the same node as the pinctrl device.
>
>
> Pushing hardware internals into the DT is not preferred.
>
>
> That’s exactly the point: the current DT format encapsulates the internal
> grouping of the hardware blocks into larger address-decoding groups. For
> the case of pinctrl and GPIO, the actual structure looks as follows:
> + “bunch of registers”
> + N x [pinctrl functionality for a pin-group]
> + N x [irq-control for a pin-group]
> + N x [GPIO functionality for a pin-group]

Which are part of the same hardware block, which exposes N x M pins with
varying functionality.

> So if I were asked to model this from scratch, I’d use a regmap-device for
> the PIO and R_PIO address spaces and then reference the pinctrl and gpio
> functionality back to it (hiding the internals of how each bank assigns bits
> and where).

However we have an existing binding that should be followed, unless something
is seriously wrong with it, then it should be fixed. Also, AFAIK U-boot sunxi
treats the kernel as the canonical source for device tree files and bindings.

> Since the pinctrl and gpio drivers actually driver the same hardware block,
> just in different ways, and there should be some locking between the two,
> I think it would make sense to merge the two.
>
> You can also get away with not having soc_data by just registering the full
> set of GPIO banks and pins (we aren't counting extra pins anyway).
>
>
> Merging the pinctrl and gpio drivers is going to be tricky, as a driver can
> only
> have a single class.
>
> The more immediate solution would be to simply derive the appropriate driver
> data for the existing gpio class and manually bind a driver to the node (see
> option “b” from above).
>
> This will then require sharing the (internal) driver data structure from the
> gpio-driver with pinctrl and populating it from the pinctrl probe… i.e. we
> had
> this in an earlier version of the changes, but it looked messy.

The Linux kernel driver also has pinctrl and gpio in the same driver. However
the difference is that the kernel probes device tree nodes as platform devices,
and the device driver then registers pinctrl and gpio functions.

The U-boot model is a simplified one assuming that one device only has one
function, which is not always true. You're going to run into the same issue
with the CCU, which provides clocks and reset controls.


Regards
ChenYu


> Before we revert to this model, I’ll wait if a consensus emerges from the
> discussion here.
>
>
> Regards,
> Philipp.