Message ID | 20191030114530.872-1-peter.ujfalusi@ti.com |
---|---|
Headers | show |
Series | gpio: Support for shared GPIO lines on boards | expand |
Hi, On 30/10/2019 13.45, Peter Ujfalusi wrote: > Hi, > > The shared GPIO line for external components tends to be a common issue and > there is no 'clean' way of handling it. I have missed Rob and the DT list from the recipients list, I'll send the RFC v2 asap. - Péter > I'm aware of the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag, which must be provided when > a driver tries to request a GPIO which is already in use. > However the driver must know that the component is going to be used in such a > way, which can be said to any external components with GPIO line, so in theory > all drivers must set this flag when requesting the GPIO... > > But with the GPIOD_FLAGS_BIT_NONEXCLUSIVE all clients have full control of the > GPIO line. For example any device using the same GPIO as reset/enable line can > reset/enable other devices, which is not something the other device might like > or can handle. > For example a device needs to be configured after it is enabled, but some other > driver would reset it while handling the same GPIO -> the device is not > operational anymmore as it lost it's configuration. > > With the gpio-shared gpiochip we can overcome this by giving the gpio-shared > the role of making sure that the GPIO line only changes state when it will not > disturb any of the clients sharing the same GPIO line. > > The 'sticky' state of the line depends on the board design, which can be > communicated with the hold-active-state property: > > GPIO_ACTIVE_HIGH: the line must be high as long as any of the clients want it to > be high > GPIO_ACTIVE_LOW: the line must be low as long as any of the clients want it to > be low > > In board DTS files it is just adding the node to descibe the shared GPIO line > and point the users of this line to the shared-gpio node instead of the real > GPIO. > > Something like this: > > codec_reset: gpio-shared0 { > compatible = "gpio-shared"; > gpio-controller; > #gpio-cells = <2>; > > root-gpios = <&audio_exp 0 GPIO_ACTIVE_HIGH>; > > branch-count = <2>; > hold-active-state = <GPIO_ACTIVE_HIGH>; > }; > > &main_i2c3 { > audio_exp: gpio@21 { > compatible = "ti,tca6416"; > reg = <0x21>; > gpio-controller; > #gpio-cells = <2>; > }; > > pcm3168a_a: audio-codec@47 { > compatible = "ti,pcm3168a"; > reg = <0x47>; > > #sound-dai-cells = <1>; > > rst-gpios = <&codec_reset 0 GPIO_ACTIVE_HIGH>; > ... > }; > > pcm3168a_b: audio-codec@46 { > compatible = "ti,pcm3168a"; > reg = <0x46>; > > #sound-dai-cells = <1>; > > rst-gpios = <&codec_reset 1 GPIO_ACTIVE_HIGH>; > ... > }; > }; > > If any of the codec requests the GPIO to be high, the line will go up and will > only going to be low when both of them set's their shared line to low. > > Note: other option would be to have something similar to gpio-hog (gpio-shared) > support in the core itself, but then all of the logic and state handling for the > users of the shared line needs to be moved there. > Simply counting the low and high requests would not work as the GPIO framework > by design does not refcounts the state, iow gpio_set(0) three times and > gpio_set(1) would set the line high. > > I have also looked at the reset framework, but again it can not be applied in a > generic way for GPIOs shared for other purposes and all existing drivers must > be converted to use the reset framework (and adding a linux only warpper on top > of reset GPIOs). > > Regards, > Peter > --- > Peter Ujfalusi (2): > dt-bindings: gpio: Add binding document for shared GPIO > gpio: Add new driver for handling 'shared' gpio lines on boards > > .../devicetree/bindings/gpio/gpio-shared.yaml | 100 ++++++++ > drivers/gpio/Kconfig | 6 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-shared.c | 229 ++++++++++++++++++ > 4 files changed, 336 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-shared.yaml > create mode 100644 drivers/gpio/gpio-shared.c > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi folks, cutting all the discussions in this thread 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>; }; 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.) 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.) Yours, Linus Walleij
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. 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. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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
On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > To start with I would let GPIO core to allow requesting the same GPIO > line by multiple consumers all the time. It already allows that with GPIOD_FLAGS_BIT_NONEXCLUSIVE. > 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. We already have that. Unless all users request it with the same GPIOD_FLAGS_BIT_NONEXCLUSIVE they will simply fail, it is an excellent warning since it creates a strict semantic requirement for all participating consumer to explicitly specify if they want to share a line. Adding a GPIOD_FLAGS_BIT_MIGHT_BE_SHARED with some soft semantics sort of allowing deviant nonstrict behavior seems like a real bad idea to me, it will likely create all kind of ambiguities we cannot foresee. > 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. I have burnt myself a bit on "let's poke in this transitional thing that I promise to remove later" and that later never happens so I'd say don't do that. > gpiod_deassert() would be equivalent to Philipp's > gpiod_politely_suggest_value() I don't intuitively understand the semantics of these calls. Consider Rusty Russells API design manifesto: http://sweng.the-davies.net/Home/rustys-api-design-manifesto > 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); This is what gpiod_set_value() already does today in a way just name gpiod_set_value() -> gpio_set_asserted() and change the second argument to a bool named "asserted". It seems like a totally different and entirely syntactic problem separate from the reset business you're trying to solve? We had this discussion before this week and yeah, if we historically named the logical levels on the line "asserted" and "deasserted" everywhere it would be great. It is up to someone driving the change and changing it everywhere in that case. Preferably with a semantic coccinelle patch or sed script since it is purely syntactic, then plan where and when to run that. Then do that first, wait a kernel cycle and scoop up any fallout and leftovers and then start the next thing. > Basically assert would set the level to the active state defined by the > DT. Or ACPI. Or machine descriptor tables. I suppose. Doing APIs becomes generic, the suggestion I had above was more like doing something like detecting a shared line *specifically* for device tree and nothing else and handle it in gpiolib-of.c but maybe that is not possible. > Gradually drivers can be moved to this API pair from gpiod_set_value() > when it makes sense. The problem I have right now as subsystem maintainer is people starting things like that and never finishing them. If you wanna do this I suggest a fix it everywhere in one swift stroke approach with broad buy-in from everyone-approach or fail totally. We have too many in-transit API changes. > 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? These APIs really need names that can be understood right off and they should be compile-time optional (a Kconfig option) so that drivers that really need them can select to have them explicitly. > 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. > > 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. > Another thing is that currently gpio core does not have refcounting and > most of the client drivers treat it like that. Notably all the drivers specifying GPIOD_FLAGS_BIT_NONEXCLUSIVE does not treat it like that and that is why they specify that flag. All regulators, I think. > 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); I guess you mean gpiod_set() > 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. I don't understand why that would not be allowed? Again I guess not really related to the original problem, so if you want to work on that it can be done in isolation. To the overall question of a refcounting GPIO API: OK to add a new API like that I would say first convert the regulators to use them so we have a strong buy-in from a subsystem that already does this. That way we can get rid of the existing GPIOD_FLAGS_BIT_NONEXCLUSIVE and pull the handling of shared GPIO lines into gpiolib using these new APIs. I'm all for this. But the general usability needs to be proven. It is not a very huge task: git grep BIT_NONEXCLUSIVE |wc -l 24 24 occurrences in the whole kernel. If the suggested API doesn't fit regulators as well it is dead in the water. Then the usecase is likely specific to resets, and what you would need to do is rather improve the available semantics in the reset subsystem. So begin with creating a way to pull the shared handling of regulators into gpiolib with these clearly cut semantics delete the NONEXCLUSIVE thing and then when you are done with that exploit the same infrastructure for GPIO reset. Yours, Linus Walleij
On 13/11/2019 19.06, Linus Walleij wrote: > On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> To start with I would let GPIO core to allow requesting the same GPIO >> line by multiple consumers all the time. > > It already allows that with GPIOD_FLAGS_BIT_NONEXCLUSIVE. > >> 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. > > We already have that. Unless all users request it with the same > GPIOD_FLAGS_BIT_NONEXCLUSIVE they will simply fail, > it is an excellent warning since it creates a strict semantic > requirement for all participating consumer to explicitly specify > if they want to share a line. > > Adding a GPIOD_FLAGS_BIT_MIGHT_BE_SHARED with some > soft semantics sort of allowing deviant nonstrict behavior seems > like a real bad idea to me, it will likely create all kind of ambiguities > we cannot foresee. Agree. >> 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. > > I have burnt myself a bit on "let's poke in this transitional > thing that I promise to remove later" and that later never > happens so I'd say don't do that. > >> gpiod_deassert() would be equivalent to Philipp's >> gpiod_politely_suggest_value() > > I don't intuitively understand the semantics of these calls. > Consider Rusty Russells API design manifesto: > http://sweng.the-davies.net/Home/rustys-api-design-manifesto imho the gpiod is API level -7 in the Rusty scale. >> 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); > > This is what gpiod_set_value() already does today in a way > just name > gpiod_set_value() -> gpio_set_asserted() and change > the second argument to a bool named "asserted". > > It seems like a totally different and entirely syntactic problem > separate from the reset business you're trying to solve? > > We had this discussion before this week and yeah, if we > historically named the logical levels on the line "asserted" > and "deasserted" everywhere it would be great. Yeah, it is extremely awkward currently: rst-gpois = <&gpio0 1 GPIO_ACTIVE_LOW>; devm_gpiod_get_optional(dev, "rst", GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE); Would set the initial output level to _high_ > It is up to someone driving the change and changing it > everywhere in that case. Preferably with a semantic > coccinelle patch or sed script since it is purely > syntactic, then plan where and when to > run that. Then do that first, wait a kernel cycle and scoop up > any fallout and leftovers and then start the next thing. > >> Basically assert would set the level to the active state defined by the >> DT. > > Or ACPI. Or machine descriptor tables. I suppose. Sure. > Doing APIs becomes generic, the suggestion I had > above was more like doing something like detecting > a shared line *specifically* for device tree and nothing > else and handle it in gpiolib-of.c but maybe that is not > possible. > >> Gradually drivers can be moved to this API pair from gpiod_set_value() >> when it makes sense. > > The problem I have right now as subsystem maintainer is people > starting things like that and never finishing them. > > If you wanna do this I suggest a fix it everywhere in one swift stroke > approach with broad buy-in from everyone-approach or fail totally. > We have too many in-transit API changes. I know, I'm trying to catch up on my promise on DMAengine API ;) >> 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? > > These APIs really need names that can be understood right off > and they should be compile-time optional (a Kconfig option) so > that drivers that really need them can select to have them > explicitly. At the end the goal is to have only assert/deassert API for GPIO, or do you want to keep set_value()? Imho the gpiod_direction_output_raw() should not be allowed to be used by drivers. CONFIG_GPIOLIB_ASSER_API as selectable config option? But it is one thing to change gpiod users as we have heavy use of the legacy gpio API: git grep gpio_request | wc -l 1868 That does not really matter in this case. >> 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. >> >> 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. > >> Another thing is that currently gpio core does not have refcounting and >> most of the client drivers treat it like that. > > Notably all the drivers specifying GPIOD_FLAGS_BIT_NONEXCLUSIVE > does not treat it like that and that is why they specify that > flag. All regulators, I think. > >> 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); > > I guess you mean gpiod_set() Yes. >> 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. > > I don't understand why that would not be allowed? > > Again I guess not really related to the original problem, > so if you want to work on that it can be done in isolation. > > To the overall question of a refcounting GPIO API: > > OK to add a new API like that I would say first convert the > regulators to use them so we have a strong buy-in from a > subsystem that already does this. That way we can get rid of > the existing GPIOD_FLAGS_BIT_NONEXCLUSIVE and pull > the handling of shared GPIO lines into gpiolib using these > new APIs. > > I'm all for this. The refcounting needs to be seeded with a level that needs to be preserved. It can be low or high. And we have cases probably when the board does not want to use refcounting on the shared line, juts pass-through (as it is with the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag currently). > But the general usability needs to be proven. > It is not a very huge task: > git grep BIT_NONEXCLUSIVE |wc -l > 24 > > 24 occurrences in the whole kernel. > > If the suggested API doesn't fit regulators as well it is dead > in the water. Then the usecase is likely specific to resets, > and what you would need to do is rather improve the > available semantics in the reset subsystem. Sure, I agree. > So begin with creating a way to pull the shared handling of > regulators into gpiolib with these clearly cut semantics > delete the NONEXCLUSIVE thing and then when you are > done with that exploit the same > infrastructure for GPIO reset. The logic is relatively simple, 229 lines in gpio-shared, but moving that into core will explode things a bit and going to add more complexity to all gpio lines. For one, we must maintain a list of clients requesting the line to be able to do proper refcounting and this needs to be done for all pins as we don't know beforehand that the given line is going to be shared. Or add gpio-shared block similar to gpio-hog to prepare a given line for sharing? I think this might be a better thing to do and some of the code from gpio-shared.c can be reused. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Linus, On 19/11/2019 10.34, Peter Ujfalusi wrote: >> So begin with creating a way to pull the shared handling of >> regulators into gpiolib with these clearly cut semantics >> delete the NONEXCLUSIVE thing and then when you are >> done with that exploit the same >> infrastructure for GPIO reset. > > The logic is relatively simple, 229 lines in gpio-shared, but moving > that into core will explode things a bit and going to add more > complexity to all gpio lines. > For one, we must maintain a list of clients requesting the line to be > able to do proper refcounting and this needs to be done for all pins as > we don't know beforehand that the given line is going to be shared. > > Or add gpio-shared block similar to gpio-hog to prepare a given line for > sharing? I think this might be a better thing to do and some of the code > from gpio-shared.c can be reused. I had some time today and now have a working support for shared GPIOs in the gpio core. Works fine with regulators and with my board which have two ocm3168a codec with shared reset GPIO. There are couple of things initially not going to work, like supporting clients with different GPIO_ACTIVE_HIGH/LOW as we have only one gpio_desc which got it's GPIO_ACTIVE_* form the gpio-shared child. The means that all clients has to have the same GPIO_ACTIVE_* as how the gpio-shared child is configured: gpio1 { p00 { gpio-shared; /* raw level high is refcounted */ gpios = <0 GPIO_ACTIVE_HIGH>; /* Initially set the gpio line to raw level high */ output-high; line-name = "CODEC RESET"; }; }; now clients can: ... enable-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>; ... enable-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>; ... I'm reusing some of the gpio-hog code for this from gpiolib-of.c For supporting different GPIO_ACTIVE_* on the client side might be tricky as we might need to create dummy gpio_desc for each client and put them on a list for the shared gpio_desc. In any case I try to clean up the patch and add some documentation and send it for review as RFC in couple of days. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Tue, Nov 19, 2019 at 9:33 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 13/11/2019 19.06, Linus Walleij wrote: > > On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> gpiod_deassert() would be equivalent to Philipp's > >> gpiod_politely_suggest_value() > > > > I don't intuitively understand the semantics of these calls. > > Consider Rusty Russells API design manifesto: > > http://sweng.the-davies.net/Home/rustys-api-design-manifesto > > imho the gpiod is API level -7 in the Rusty scale. I would agree that the double-inversion of DT or machine flags for active low/high and then gpiod_set_value() to high makes it low and vice versa mutatis mutandis is pretty -7 But my comment was not about that, but about the ambiguity of some *_politely_suggest_* API. I mean adding a new confusing API to the mess is hardly going to make things better, two wrongs doesn't make one right. > > It seems like a totally different and entirely syntactic problem > > separate from the reset business you're trying to solve? > > > > We had this discussion before this week and yeah, if we > > historically named the logical levels on the line "asserted" > > and "deasserted" everywhere it would be great. > > Yeah, it is extremely awkward currently: > rst-gpois = <&gpio0 1 GPIO_ACTIVE_LOW>; > > devm_gpiod_get_optional(dev, "rst", GPIOD_OUT_LOW | > GPIOD_FLAGS_BIT_NONEXCLUSIVE); > > Would set the initial output level to _high_ Yeah I understand what the problem is.... :/ > > These APIs really need names that can be understood right off > > and they should be compile-time optional (a Kconfig option) so > > that drivers that really need them can select to have them > > explicitly. > > At the end the goal is to have only assert/deassert API for GPIO, or do > you want to keep set_value()? The problem can me split in two, non-conflated things: 1. Rename gpiod_set_value() to e.g. gpiod_set_state(bool asserted) this is a purely syntactic change with no semantic effect. This makes it clear what happens and fixes the issue with the hard to understand name of the function, but does not change the semantic allowing you to say assert a GPIO line several times after another, as that would wreak havoc in the kernel. And after all there is nothing about that name that suggest it would be reference counting anything. 2. Add a new reference-counted stateful API that does not allow you to handle your usecase. I would recommend not conflating the two things. > Imho the gpiod_direction_output_raw() should not be allowed to be used > by drivers. So how do you suggest that drivers/w1/masters/w1-gpio.c handle the usecase of overriding a nominally open drain-flagged line to pump some voltage in the line at some regular intervals, notwithstanding the logical level of the line? This use of GPIO isn't dealing with something boolean logical, but something directly electromagnetic. It would be nice if GPIO was only about logical levels, but it is not, and that is why gpiod_set_raw() exists. There are other users in the kernel that are just cheating or thinking wrong, it'd be nice if those could be fixed. It doesn't mean the API has no valid usecases. > CONFIG_GPIOLIB_ASSER_API as selectable config option? CONFIG_GPIOLIB_REFCOUNTING_API is more to the point I think, because that is what you want to achieve. Then the function calls can very well be named something with *assert* in them. > But it is one thing to change gpiod users as we have heavy use of the > legacy gpio API: > git grep gpio_request | wc -l > 1868 That can be done with a sed script if someone takes on the task. > That does not really matter in this case. Nope. > > So begin with creating a way to pull the shared handling of > > regulators into gpiolib with these clearly cut semantics > > delete the NONEXCLUSIVE thing and then when you are > > done with that exploit the same > > infrastructure for GPIO reset. > > The logic is relatively simple, 229 lines in gpio-shared, but moving > that into core will explode things a bit and going to add more > complexity to all gpio lines. I would just add a flag such as in drivers/gpio/gpiolib.h: #define FLAG_REFERENCE_COUNTED 15 /* GPIO uses the reference counting API */ If anyone grabs a GPIO with this flag it needs to be accessed using the refcounting API and all other uses with the regular API denied. The same the other direction, if FLAG_REQUESTED is already set, the refcounting API should bail out. Do not try to support a use case such as allowing the gpiod to be grabbed unrefcounted and later turned into a refcounted gpiod. Only grab it through the refcount-specific API. This way it's almost as cleanly separated as the code is separated into regulator right now, just that it lives in a place where it can be reused by others needing reference counting. Then surround code with: if (IS_ENABLED(CONFIG_GPIOLIB_REFCOUNTING_API) /* test flag */ Then the code size impact should be zero if the refcounting API is not selected. Then just create an add-on that only affects the lines that explicitly want refcounting. Wrap a gpiod in another struct or something struct gpio_refcount_desc? > For one, we must maintain a list of clients requesting the line to be > able to do proper refcounting and this needs to be done for all pins as > we don't know beforehand that the given line is going to be shared. If you want to deny the same client to ask for the same line twice then you need a list like that indeed. (It's a good strict semantic check anyways.) > Or add gpio-shared block similar to gpio-hog to prepare a given line for > sharing? I think this might be a better thing to do and some of the code > from gpio-shared.c can be reused. I would just add a flag and try to keep this API entirely on the side for now. Yours, Linus Walleij
On 21/11/2019 16.47, Linus Walleij wrote: > On Tue, Nov 19, 2019 at 9:33 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 13/11/2019 19.06, Linus Walleij wrote: >>> On Fri, Nov 8, 2019 at 12:20 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >>>> gpiod_deassert() would be equivalent to Philipp's >>>> gpiod_politely_suggest_value() >>> >>> I don't intuitively understand the semantics of these calls. >>> Consider Rusty Russells API design manifesto: >>> http://sweng.the-davies.net/Home/rustys-api-design-manifesto >> >> imho the gpiod is API level -7 in the Rusty scale. > > I would agree that the double-inversion of DT or machine flags > for active low/high and then gpiod_set_value() to high makes it > low and vice versa mutatis mutandis is pretty -7 > > But my comment was not about that, but about the ambiguity > of some *_politely_suggest_* API. Yep, this is really a bad idea.. >>> These APIs really need names that can be understood right off >>> and they should be compile-time optional (a Kconfig option) so >>> that drivers that really need them can select to have them >>> explicitly. >> >> At the end the goal is to have only assert/deassert API for GPIO, or do >> you want to keep set_value()? > > The problem can me split in two, non-conflated things: > > 1. Rename gpiod_set_value() to e.g. gpiod_set_state(bool asserted) > this is a purely syntactic change with no semantic effect. > This makes it clear what happens and fixes the issue with > the hard to understand name of the function, but does not > change the semantic allowing you to say assert a GPIO line > several times after another, as that would wreak havoc in the > kernel. And after all there is nothing about that name that > suggest it would be reference counting anything. An intuitive name in place of gpiod_set_value()/gpiod_get_value() family would be really nice... Another thing which bugged me for a while is the _cansleep postfixing of the get/set. In most cases drivers do not care if the gpio access sleeps or not, but there are (few) cases when the access should not sleep. I think if a cleaner API is proposed it might worth to make the normal API to not warn when it might sleep and _nosleep postfixed versions must be used in place where it is not desired to sleep (and WARN if it would sleep). > 2. Add a new reference-counted stateful API that does not allow you to > handle your usecase. Not sure if we really need refcounted access just for fun. Imho even the shared GPIO access should not be refcounted as such, but have a logic to use the user's requested level to set the line's level. As the gpio-shared driver is doing in this RFC. > I would recommend not conflating the two things. > >> Imho the gpiod_direction_output_raw() should not be allowed to be used >> by drivers. > > So how do you suggest that drivers/w1/masters/w1-gpio.c > handle the usecase of overriding a nominally open drain-flagged > line to pump some voltage in the line at some regular intervals, > notwithstanding the logical level of the line? Sure, if there is a real use for it then it should be preserved. > This use of GPIO isn't dealing with something boolean logical, > but something directly electromagnetic. >> It would be nice if GPIO was only about logical levels, > but it is not, and that is why gpiod_set_raw() exists. I believe my problem starts with that I also see this from electromagnetic point and not logical level point ;) > There are other users in the kernel that are just cheating > or thinking wrong, it'd be nice if those could be fixed. > It doesn't mean the API has no valid usecases. > >> CONFIG_GPIOLIB_ASSER_API as selectable config option? > > CONFIG_GPIOLIB_REFCOUNTING_API is more to the > point I think, because that is what you want to achieve. > > Then the function calls can very well be named something > with *assert* in them. I'm still not convinced that refcounting API would help or give anything useful at the table. >> But it is one thing to change gpiod users as we have heavy use of the >> legacy gpio API: >> git grep gpio_request | wc -l >> 1868 > > That can be done with a sed script if someone takes on the > task. Which went nicely with the regulators ;) Adding GPIOD_FLAGS_BIT_NONEXCLUSIVE: b0ce7b29bfcd regulator/gpio: Allow nonexclusive GPIO access to fix: efdfeb079cc3 regulator: fixed: Convert to use GPIO descriptor only > >> That does not really matter in this case. > > Nope. > >>> So begin with creating a way to pull the shared handling of >>> regulators into gpiolib with these clearly cut semantics >>> delete the NONEXCLUSIVE thing and then when you are >>> done with that exploit the same >>> infrastructure for GPIO reset. >> >> The logic is relatively simple, 229 lines in gpio-shared, but moving >> that into core will explode things a bit and going to add more >> complexity to all gpio lines. > > I would just add a flag such as in drivers/gpio/gpiolib.h: > > #define FLAG_REFERENCE_COUNTED 15 /* GPIO uses the reference > counting API */ > > If anyone grabs a GPIO with this flag it needs to be accessed > using the refcounting API and all other uses with the > regular API denied. You mean that everyone must request it with this flag, right? At the first request this flag would be set to the gpio_desc. The upcoming requests would be denied if the requester is not using the proper API/flag to request it, right? For single user this does not matter apart from the fact that the gpio accesses will be refcounted for a single client as well (and API check as well). > The same the other direction, if FLAG_REQUESTED is already > set, the refcounting API should bail out. Hrm, so you want FLAG_REQUESTED / FLAG_REFERENCE_COUNTED to be exclusive? I don't see why FLAG_REQUESTED would be in the way of FLAG_REFERENCE_COUNTED. > Do not try to support a use case such as allowing the gpiod > to be grabbed unrefcounted and later turned into a refcounted > gpiod. Only grab it through the refcount-specific API. Sure, that would not work anyways. > This way it's almost as cleanly separated as the code is > separated into regulator right now, just that it lives in > a place where it can be reused by others needing > reference counting. > > Then surround code with: > > if (IS_ENABLED(CONFIG_GPIOLIB_REFCOUNTING_API) > /* test flag */ > > Then the code size impact should be zero if the refcounting API > is not selected. > > Then just create an add-on that only affects the lines that explicitly > want refcounting. Wrap a gpiod in another struct or something > struct gpio_refcount_desc? Ah, I see where you are heading now.. struct gpio_refcount_desc *gpiod_refcounted_get() and it's family with own set of API operating on top of struct gpio_refcount_desc? >> For one, we must maintain a list of clients requesting the line to be >> able to do proper refcounting and this needs to be done for all pins as >> we don't know beforehand that the given line is going to be shared. > > If you want to deny the same client to ask for the same line > twice then you need a list like that indeed. (It's a good strict semantic > check anyways.) It is not just that. Global refcounting is just not going to work. For simplicity: you have three codecs connected to same GPIO for ENABLE (active high). When the driver probes it will ask for the gpio to deasserted (no need to power on things when they are not in use). We have registered three low requests. Then one needs to be powered up, which gives us two low requests and one high request, but we must set the line high. How does the core know this? We should care for the asserted state? What if they have RESET pin which is active low? they will ask it to be asserted initially and when any of them needs to be powered it will ask for deassert. >> Or add gpio-shared block similar to gpio-hog to prepare a given line for >> sharing? I think this might be a better thing to do and some of the code >> from gpio-shared.c can be reused. > > I would just add a flag and try to keep this API entirely on the > side for now. > > Yours, > Linus Walleij > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki