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

[Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com>]

Chen,

> On 21 Feb 2017, at 04:48, Chen-Yu Tsai <wens@csie.org> wrote:
> 
> On Sat, Feb 18, 2017 at 1:52 AM, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@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
(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)
(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.

> 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 <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.

>> +#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]

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).

> 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.

Before we revert to this model, I?ll wait if a consensus emerges from the
discussion here.


Regards,
Philipp.