Message-ID: <34f04bc5-ce80-ba3e-3b8b-00dfc3d54964@ti.com>
Date:   Tue, 12 Nov 2019 10:17:55 +0200
From:   Peter Ujfalusi <peter.ujfalusi@...com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Grant Likely <glikely@...retlab.ca>
CC:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Mark Brown <broonie@...nel.org>, Tero Kristo <t-kristo@...com>,
        Maxime Ripard <mripard@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>
Subject: Re: [RFC 0/2] gpio: Support for shared GPIO lines on boards

Hi,

On 08/11/2019 13.21, Peter Ujfalusi wrote:
> Hi Linus,
> 
> On 01/11/2019 17.56, Linus Walleij wrote:
>> Hi folks,
>>
>> cutting all the discussions in this thread
> 
> the mail got disconnected from the thread and almost missed it ;)
> 
>> we need to see the bigger
>> pattern:
>>
>> On GPIO rails
>>
>> People want "something like rails" for GPIO. In power supplies
>> and thus the regulator subsystem, rails are connected to many
>> logical endpoints.
>>
>> - The suggested inverter bindings would be effectively an
>>   inverter on a GPIO rail.
>>
>> - This suggestion would be equal to many power consumers
>>   on a rail, such as the usecase of shared gpio-enable lines in
>>   the regulator subsystem already provides.
>>
>> The former seems to have been identified as solveable for the
>> userspace that needed it and absorbed into the drafts for a
>> virtualized GPIO controller. (Aggregating and creating a new
>> virtual GPIO chip for some select physical GPIO lines.)
>>
>> I haven't seen an exact rationale from the DT community as
>> to why these things should not be modeled, but as can be
>> clearly seen in
>> Documentation/devicetree/bindings/regulator/regulator.yaml
>> the "rail abstraction" from the regulator subsystem which
>> is in effect struct regulation_constraints and it sibling
>> struct regulator_init_data is not in the DT bindings, instead
>> this is encoded as properties in the regulator itself, so this
>> is pretty consistent: the phandle from regulator to consumer
>> *is* the rail.
>>
>> This goes back to Rajendras initial DT regulator support code
>> see:
>> git log -p 69511a452e6d
>>
>> So it would be logical then to just have:
>>
>> - More than one phandle taking the same GPIO line
>> - Figure this situation out in the gpiolib OF core
>> - Resolve the manageability of the situation (same
>>   consumer flags etc)
>> - Instantiate a kernel component as suggested,
>>   mediating requests.
>> - Handle it from there.
>>
>> So:
>>
>> gpio: gpio-controller@0 {
>>         compatible = "foo,gpio";
>>         gpio-controller;
>>         #gpio-cells = <2>;
>> };
>>
>> consumer-a {
>>        compatible = "foo,consumer-a";
>>        rst-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
>> };
>>
>> consumer-b {
>>        compatible = "foo,consumer-b";
>>        rst-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
>> };
> 
> Should be fine I think.
> Again, I would trust the board design to take into consideration of the
> consumer's needs when sharing the same GPIO line for any purpose.
> 
>> Hi kernel: figure it out.
>>
>> From this point the kernel driver(s) have to figure it out.
>>
>> I don't think this requires any changes to the DT bindings
>> other than perhaps spelling out that if you link more than one
>> phandle to a GPIO line, magic will happen. (We should probably
>> make very verbose dmesg prints about this magic.)
> 
> To start with I would let GPIO core to allow requesting the same GPIO
> line by multiple consumers all the time.
> 
> If the flags for the gpio_request does not contain
> GPIOD_FLAGS_BIT_NONEXCLUSIVE (probably we can have another define for
> BIT(4) as GPIOD_FLAGS_BIT_MIGHT_BE_SHARED?) then print with dev_warn()
> to get the attention of the developer that all users of the shared GPIO
> line must be checked and change the current dev_info() to dev_dbg() when
> the flag is provided.
> 
> When the consumer drivers are checked (and modified if needed) that they
> behave OK in this situation we can snap the
> GPIOD_FLAGS_BIT_MIGHT_BE_SHARED to silence the warning.
> 
>> This is enough to start with. After that we can discuss adding
>> flags and constraint properties to a certain GPIO line if
>> need be. (That will be a big discussion as well, as we haven't
>> even figured out how to assign default values to individual
>> GPIO lines yet.)
> 
> Not sure how the core would refcount things, but to align with what Rob
> was saying about the misleading API naming:
> gpiod_set_value(priv->en_gpio, 1/0) against the DT's
> GPIO_ACTIVE_HIGH/LOW of the line's active state we might want to have:
> gpiod_assert(priv->en_gpio);
> gpiod_deassert(priv->en_gpio);
> 
> Basically assert would set the level to the active state defined by the
> DT. deassert would, well, de-assert the active state.
> 
> gpiod_deassert() would be equivalent to Philipp's
> gpiod_politely_suggest_value()
> 
> Gradually drivers can be moved to this API pair from gpiod_set_value()
> when it makes sense.
> The current gpiod_set_* would operate like as it is right now by not
> asking politely a level, whatever it is.
> 
> Hrm, probably both gpiod_assert() and gpiod_deassert() should be polite
> when asking for level change?
> 
> If all consumers of the shared line is using gpiod_assert/deassert, then
> the core should 'protect' the raw level of the gpiod_assert() calls.

Well, this is not going to work either. The core must know what level it
needs to 'protect'.

If we have two devices, both is enabled when their RST/ENABLE pin is
high, but D1 documents it's RST line as low active reset (low=reset,
high=enabled), D2 documents it's ENABLE line as high active (low=reset,
high=enabled).

dpiod_assert() would want to set the line low for D1 and high for D2 as
per how we supposed to interpret the GPIO bindings.

> At the end we will see drivers converted to assert/deassert API when a
> developer faces issues that they use shared GPIO line on a board.
> 
> gpiod_get should also use the polite version when setting the initial
> level most likely.
> 
> Another thing is that currently gpio core does not have refcounting and
> most of the client drivers treat it like that. It is perfectly fine to
> gpiod_get(priv->en_gpio,1);
> gpiod_get(priv->en_gpio,1);
> gpiod_get(priv->en_gpio,1);
> gpiod_get(priv->en_gpio,0);
> 
> at the last call the GPIO value is going to be set to 0 no matter if it
> was set to 1 three times prior, but I guess this can be worked out when
> the driver(s) are converted to assert/deassert.

Another thing which came to my mind is that the gpiod_get's are also
need to be refcounted to make sure that when used by different drivers
and one of them is removed the GPIO is still usable by the remaining
users (and to remove the constraints placed by the removed module).

So convert things to look more like the regulator core than what we have
atm?
But the difference is that in regulators we protect the enabled state
(if anyone needs the given regulator on then it is on), but in GPIO the
level we might want to protect is either high or low depending on the
users of the shared line.
If the devices enabled when the line is high -> protect high
If devices enabled when the line is low -> protect low

All of this can be handled with the RFC gpio-shared driver and
representation of the hardware in DT...

> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Powered by blists - more mailing lists