Message ID | 20191030120440.3699-1-peter.ujfalusi@ti.com |
---|---|
Headers | show |
Series | gpio: Support for shared GPIO lines on boards | expand |
On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> 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'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. Why can't we just add a shared flag like we have for interrupts? Effectively, we have that for resets too, it's just hardcoded in the the drivers. Rob
On 30/10/2019 15.12, Rob Herring wrote: > On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> 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'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. > > Why can't we just add a shared flag like we have for interrupts? > Effectively, we have that for resets too, it's just hardcoded in the > the drivers. This would be kind of the same thing what the GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for fixed-regulators afaik. But let's say that a board design will pick two components (C1 and C2) and use the same GPIO line to enable them. We already have the drivers for them and they are used in boards already. Both needs the GPIO line to be high for normal operation. One or both of them needs register writes after they are enabled. During boot both requests the GPIO (OUTPUT_LOW) and sets it high, then run the register setup. C1 request GPIO (LOW) C1 gpio_set(1) C1 register writes C2 requests GPIO (LOW) C1 placed to reset and looses the configuration C2 gpio_set(1) C1 also enabled C2 register writes At this point C2 is operational, C1 is not. In shared GPIO case the GPIO should be handled like a regulator with a twist that the 'sticky' state of the GPIO might be low or high depending on the needs of the components it is connected to. The shared GPIO line is a board design quirk and basically any device which have reset/enable GPIO must be able to work in a situation when they are sharing that line with other components and the driver should not know much about this small detail. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Wed, 2019-10-30 at 15:32 +0200, Peter Ujfalusi wrote: > > On 30/10/2019 15.12, Rob Herring wrote: > > On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> 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'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. > > > > Why can't we just add a shared flag like we have for interrupts? > > Effectively, we have that for resets too, it's just hardcoded in the > > the drivers. > > This would be kind of the same thing what the > GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for > fixed-regulators afaik. > > But let's say that a board design will pick two components (C1 and C2) > and use the same GPIO line to enable them. We already have the drivers > for them and they are used in boards already. > > Both needs the GPIO line to be high for normal operation. > One or both of them needs register writes after they are enabled. > > During boot both requests the GPIO (OUTPUT_LOW) and sets it high, then > run the register setup. > > C1 request GPIO (LOW) > C1 gpio_set(1) > C1 register writes > C2 requests GPIO (LOW) > C1 placed to reset and looses the configuration > C2 gpio_set(1) > C1 also enabled > C2 register writes > > At this point C2 is operational, C1 is not. > > In shared GPIO case the GPIO should be handled like a regulator with a > twist that the 'sticky' state of the GPIO might be low or high depending > on the needs of the components it is connected to. > > The shared GPIO line is a board design quirk and basically any device > which have reset/enable GPIO must be able to work in a situation when > they are sharing that line with other components and the driver should > not know much about this small detail. What about components that require a register write right after being enabled, for example to put the device into a low power state, to silence it on a bus, or to mask some initially enabled interrupts? regards Philipp
On 30/10/2019 15.51, Philipp Zabel wrote: > On Wed, 2019-10-30 at 15:32 +0200, Peter Ujfalusi wrote: >> >> On 30/10/2019 15.12, Rob Herring wrote: >>> On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> 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'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. >>> >>> Why can't we just add a shared flag like we have for interrupts? >>> Effectively, we have that for resets too, it's just hardcoded in the >>> the drivers. >> >> This would be kind of the same thing what the >> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for >> fixed-regulators afaik. >> >> But let's say that a board design will pick two components (C1 and C2) >> and use the same GPIO line to enable them. We already have the drivers >> for them and they are used in boards already. >> >> Both needs the GPIO line to be high for normal operation. >> One or both of them needs register writes after they are enabled. >> >> During boot both requests the GPIO (OUTPUT_LOW) and sets it high, then >> run the register setup. >> >> C1 request GPIO (LOW) >> C1 gpio_set(1) >> C1 register writes >> C2 requests GPIO (LOW) >> C1 placed to reset and looses the configuration >> C2 gpio_set(1) >> C1 also enabled >> C2 register writes >> >> At this point C2 is operational, C1 is not. >> >> In shared GPIO case the GPIO should be handled like a regulator with a >> twist that the 'sticky' state of the GPIO might be low or high depending >> on the needs of the components it is connected to. >> >> The shared GPIO line is a board design quirk and basically any device >> which have reset/enable GPIO must be able to work in a situation when >> they are sharing that line with other components and the driver should >> not know much about this small detail. > > What about components that require a register write right after being > enabled, for example to put the device into a low power state, to > silence it on a bus, or to mask some initially enabled interrupts? You are right, if a device needs driver to silence it when enabled (we might not have the driver compiled) then this can be a problem. But the same thing applies to components without enable/reset GPIO and only needing power, no? I would trust (I know...) on the board designers to not bundle components of such kinds. > > regards > Philipp > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 30/10/2019 20.49, Rob Herring wrote: > On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> >> >> >> On 30/10/2019 16.17, Mark Brown wrote: >>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote: >>>> On 30/10/2019 15.12, Rob Herring wrote: >>> >>>>> Why can't we just add a shared flag like we have for interrupts? >>>>> Effectively, we have that for resets too, it's just hardcoded in the >>>>> the drivers. >>> >>>> This would be kind of the same thing what the >>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for >>>> fixed-regulators afaik. >>> >>> The theory with that was that any usage of this would need the >>> higher level code using the GPIO to cooperate so they didn't step >>> on each other's toes so the GPIO code should just punt to it. >> >> But from the client driver point of view a GPIO is still GPIO and if the >> components are unrelated then it is hard to patch things together from >> the top. > > You can't escape a driver being aware. If a driver depends on that > GPIO to actually be set to states the driver says, then it can't be > guaranteed to work. For example, maybe the driver assumes the device > is in reset state after toggling reset and doesn't work if not in > reset state. The driver has to be aware no matter what you do in DT. That's true for some device, but it is also true that some can not tolerate being reset without them knowing it. If all users of the shared GPIO have full control over it then they can just toggle it whatever way they want. How would a regulator, codec, amplifier would negotiate on what to do with the shared GPIO? Another not uncommon setup is when the two components needs different level: C1: ENABLE is high active C2: RESET is high active To enable C1, the GPIO should be high. To enable C2 the GPIO must be low. In the board one of the branch of the shared GPIO needs (and have) a logic inverter. If they both control the same GPIO then they must have requested it with different GPIO_ACTIVE_ since the drivers are written according to chip spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one of them must happen in gpio core, right? It should be possible to add pass-through mode for gpio-shared so that all requests would propagate to the root GPIO if that's what needed for some setups. That way the gpio-shared would nicely handle the GPIO inversions, would be able to handle cases to avoid unwanted reset/enable of components or allow components to be ninja-reset. I think it would be possible to add gpiod_is_shared(struct gpio_desc *desc) so users can check if the GPIO is shared - it would only return true if the gpio-shared is not in pass-through mode so they can know that the state they see on their gpio desc is not necessary matching with reality. Probably another gpiod_shared_get_root_value() to fetch the root's state? I intentionally not returning that in the driver as clients might skip a gpio_set_value() seeing that the GPIO line is already in a state they would want it, but that would not register their needs for the level. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Thu, Oct 31, 2019 at 3:00 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > > > > On 30/10/2019 20.49, Rob Herring wrote: > > On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> > >> > >> > >> On 30/10/2019 16.17, Mark Brown wrote: > >>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote: > >>>> On 30/10/2019 15.12, Rob Herring wrote: > >>> > >>>>> Why can't we just add a shared flag like we have for interrupts? > >>>>> Effectively, we have that for resets too, it's just hardcoded in the > >>>>> the drivers. > >>> > >>>> This would be kind of the same thing what the > >>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for > >>>> fixed-regulators afaik. > >>> > >>> The theory with that was that any usage of this would need the > >>> higher level code using the GPIO to cooperate so they didn't step > >>> on each other's toes so the GPIO code should just punt to it. > >> > >> But from the client driver point of view a GPIO is still GPIO and if the > >> components are unrelated then it is hard to patch things together from > >> the top. > > > > You can't escape a driver being aware. If a driver depends on that > > GPIO to actually be set to states the driver says, then it can't be > > guaranteed to work. For example, maybe the driver assumes the device > > is in reset state after toggling reset and doesn't work if not in > > reset state. The driver has to be aware no matter what you do in DT. > > That's true for some device, but it is also true that some can not > tolerate being reset without them knowing it. You mean a reset when the driver is not loaded would not work? How could that ever work? I don't think you can have any reset control in the drivers in that case. > If all users of the shared GPIO have full control over it then they can > just toggle it whatever way they want. How would a regulator, codec, > amplifier would negotiate on what to do with the shared GPIO? > > Another not uncommon setup is when the two components needs different level: > C1: ENABLE is high active > C2: RESET is high active > > To enable C1, the GPIO should be high. To enable C2 the GPIO must be low. > In the board one of the branch of the shared GPIO needs (and have) a > logic inverter. > > If they both control the same GPIO then they must have requested it with > different GPIO_ACTIVE_ since the drivers are written according to chip > spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one > of them must happen in gpio core, right? No, drivers are written to set the state to active/inactive. The DT GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there was a recent attempt to do an inverter binding). > It should be possible to add pass-through mode for gpio-shared so that > all requests would propagate to the root GPIO if that's what needed for > some setups. > > That way the gpio-shared would nicely handle the GPIO inversions, would > be able to handle cases to avoid unwanted reset/enable of components or > allow components to be ninja-reset. What does ninja-reset mean? > I think it would be possible to add gpiod_is_shared(struct gpio_desc > *desc) so users can check if the GPIO is shared - it would only return > true if the gpio-shared is not in pass-through mode so they can know > that the state they see on their gpio desc is not necessary matching > with reality. > Probably another gpiod_shared_get_root_value() to fetch the root's state? > > I intentionally not returning that in the driver as clients might skip a > gpio_set_value() seeing that the GPIO line is already in a state they > would want it, but that would not register their needs for the level. > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 01/11/2019 15.46, Rob Herring wrote: > On Thu, Oct 31, 2019 at 3:00 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> >> >> >> On 30/10/2019 20.49, Rob Herring wrote: >>> On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >>>> >>>> >>>> >>>> On 30/10/2019 16.17, Mark Brown wrote: >>>>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote: >>>>>> On 30/10/2019 15.12, Rob Herring wrote: >>>>> >>>>>>> Why can't we just add a shared flag like we have for interrupts? >>>>>>> Effectively, we have that for resets too, it's just hardcoded in the >>>>>>> the drivers. >>>>> >>>>>> This would be kind of the same thing what the >>>>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for >>>>>> fixed-regulators afaik. >>>>> >>>>> The theory with that was that any usage of this would need the >>>>> higher level code using the GPIO to cooperate so they didn't step >>>>> on each other's toes so the GPIO code should just punt to it. >>>> >>>> But from the client driver point of view a GPIO is still GPIO and if the >>>> components are unrelated then it is hard to patch things together from >>>> the top. >>> >>> You can't escape a driver being aware. If a driver depends on that >>> GPIO to actually be set to states the driver says, then it can't be >>> guaranteed to work. For example, maybe the driver assumes the device >>> is in reset state after toggling reset and doesn't work if not in >>> reset state. The driver has to be aware no matter what you do in DT. >> >> That's true for some device, but it is also true that some can not >> tolerate being reset without them knowing it. > > You mean a reset when the driver is not loaded would not work? How > could that ever work? No, what I mean is that one device is reset because the driver for the other device toggles the GPIO line. If one driver toggles the GPIO line directly then the GPIO line is going to be toggled for all the devices the GPIO line is connected to. > I don't think you can have any reset control in > the drivers in that case. The device needs the RST line to be high, otherwise it is not accessible. If it does not have reset control how can we make sure that the GPIO line is in correct state? gpio-hog does not work all the time because we can not trust probe order and w/o gpio binding on the user deferred probing is not possible. If for some reason the gpio controller is probed after the drivers depending on the reset/enable GPIO then there's not much we can do. >> If all users of the shared GPIO have full control over it then they can >> just toggle it whatever way they want. How would a regulator, codec, >> amplifier would negotiate on what to do with the shared GPIO? >> >> Another not uncommon setup is when the two components needs different level: >> C1: ENABLE is high active >> C2: RESET is high active >> >> To enable C1, the GPIO should be high. To enable C2 the GPIO must be low. >> In the board one of the branch of the shared GPIO needs (and have) a >> logic inverter. >> >> If they both control the same GPIO then they must have requested it with >> different GPIO_ACTIVE_ since the drivers are written according to chip >> spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one >> of them must happen in gpio core, right? > > No, drivers are written to set the state to active/inactive. I think the drivers are written in a way to follow what their datasheets are tells. If it say that the GPIO line must be high to enable the device then they gpiod_set_value(1), if the line must be low to enable them then they will gpiod_set_value(0). > The DT GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there > was a recent attempt to do an inverter binding). Yes. If the line is inverted on the board, than the DT GPIO_ACTIVE_LOW will invert it to the correct level. We have two off the shelf components, C1 and C2. They have a driver written based on the datasheets. C1 needs HIGH (LOW reset/disable) uses gpiod_set_value(1) to enable the device C2 needs LOW (HIGH reset/disable) uses gpiod_set_value(0) to enable the device When they are connected to a dedicated GPIO the DT binding has GPIO_ACTIVE_HIGH since when the GPIO is set to 1 it goes HIGH, right? If two device is connected to one GPIO one of them needs an inverter on the GPIO line after it is split into two, let say C2 got inverted line: C1 tells in DT that the line is not inverted: GPIO_ACTIVE_HOGH C2 tells in DT that the line is inverted: GPIO_ACTIVE_LOW GPIO HIGH -> D1 is enabled -> !HIGH -> LOW -> D2 is enabled If both would request the same physical GPIO then how would this work? A single GPIO can not be handled in inverted and non inverted way at the same time. But this is just a side effect that this would be easy to handle with this DT binding and driver. After all, it will describe the GPIO line split. >> It should be possible to add pass-through mode for gpio-shared so that >> all requests would propagate to the root GPIO if that's what needed for >> some setups. >> >> That way the gpio-shared would nicely handle the GPIO inversions, would >> be able to handle cases to avoid unwanted reset/enable of components or >> allow components to be ninja-reset. > > What does ninja-reset mean? Ninjas attack from ambush ;) The device is reset w/o it's driver being aware that it ever happened as other driver toggled the shared GPIO line. >> I think it would be possible to add gpiod_is_shared(struct gpio_desc >> *desc) so users can check if the GPIO is shared - it would only return >> true if the gpio-shared is not in pass-through mode so they can know >> that the state they see on their gpio desc is not necessary matching >> with reality. >> Probably another gpiod_shared_get_root_value() to fetch the root's state? >> >> I intentionally not returning that in the driver as clients might skip a >> gpio_set_value() seeing that the GPIO line is already in a state they >> would want it, but that would not register their needs for the level. >> >> - 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 Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote: > [Peter] > > The device needs the RST line to be high, otherwise it is not > > accessible. If it does not have reset control how can we make sure that > > the GPIO line is in correct state? > > Just like the reset code, drivers register their use of the reset and > the core tracks users and prevents resetting when not safe. Maybe the > reset subsystem needs to learn about GPIO resets. (...) I agree. Certainly the reset subsystem can do what the regulator subsystem is already doing: request the GPIO line nonexclusive and handle any reference counting and/or quirks that are needed in a hypothetical drivers/reset/reset-gpio.c driver. There is no such driver today, just a "reset" driver in drivers/power/reset that resets the whole system. But I see no problem in creating a proper reset driver in drivers/reset to handle a few peripherals with a shared GPIO reset line. Yours, Linus Walleij
On 05/11/2019 11.58, Linus Walleij wrote: > On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote: >> [Peter] >>> The device needs the RST line to be high, otherwise it is not >>> accessible. If it does not have reset control how can we make sure that >>> the GPIO line is in correct state? >> >> Just like the reset code, drivers register their use of the reset and >> the core tracks users and prevents resetting when not safe. Maybe the >> reset subsystem needs to learn about GPIO resets. (...) > > I agree. Certainly the reset subsystem can do what the regulator > subsystem is already doing: request the GPIO line nonexclusive > and handle any reference counting and/or quirks that are needed > in a hypothetical drivers/reset/reset-gpio.c driver. I did wrote the reset-gpio driver first ;) then it failed the thought test on several levels. to get a reset control one either use the shared or exclusive API. Depending on which one you use, the behavior changes. With exclusive it works like a GPIO (no refcounting of asserts), with shared it refcounts. It fails flat if I boot with old dtb blob which did not had the "resets" and "#reset-cells" (from the user's point of view). Even if the old dtb had rst/enable/reset-gpios defined. It is kind of hard to use it for 'Output Enable' type of gpios. They are not reset or enable signals for the peripheral, but to open a gate to outside, for example allow an amplifier to drive the analog line on (one of) it's output for example. > There is no such driver today, just a "reset" driver in > drivers/power/reset that resets the whole system. Yep, I have checked that as well before I wrote my own gpio-reset > But I see no problem in creating a proper reset driver in drivers/reset > to handle a few peripherals with a shared GPIO reset line. Even if we have a reset-gpio driver we will have the same issue that the regulator might reset things underneath the tiddly refcounted reset line for non regulator users, plus one extra which is using the line as output enable. With the gpio-shared all of these can be handled in a nice way and we can add the pass-through mode to it which is assumed by some setups or use refcounting as it is in the initial patch. And we need to modify the drivers to ask for shared/nonexclusive reset/gpio. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 04/11/2019 21.11, Rob Herring wrote: >> If one driver toggles the GPIO line directly then the GPIO line is going >> to be toggled for all the devices the GPIO line is connected to. > > Of course. That would be the typical case. I'd assume we would want to > handle that the same way as shared resets. Reset can only be asserted > when all clients want reset asserted. I guess when the first client > probes, it asserts and deasserts the reset. The exclusive flavor of reset API acts like a GPIO, while the shared ones will result in refcounted behavior. With describing the hw via gpio-shared we could support different modes: - pass-through: like a GPIO with nonexclusive, any request will propagate to the root-gpio - refcounted low: the line is kept low as long as at least one client is requiring to to be low - refcounted high: the line is kept high as long as at least one client is requiring to to be high >>> I don't think you can have any reset control in >>> the drivers in that case. >> >> The device needs the RST line to be high, otherwise it is not >> accessible. If it does not have reset control how can we make sure that >> the GPIO line is in correct state? > > Just like the reset code, drivers register their use of the reset and > the core tracks users and prevents resetting when not safe. Maybe the > reset subsystem needs to learn about GPIO resets. It could even > default to knowing 'reset-gpios' property as we've somewhat > standardized that. Then you just register your GPIO reset line with > the reset subsystem. When it gets the same line registered more than > once, then it knows to handle sharing the line. If you need to know > the line is shared before then, then you need something in DT. A flag > is enough for that. What about things where the gpio is not a reset or enable for the chip, but an enable for one of it's output? Or if a shared GPIO is connected to address select pins? >>> No, drivers are written to set the state to active/inactive. >> >> I think the drivers are written in a way to follow what their datasheets >> are tells. If it say that the GPIO line must be high to enable the >> device then they gpiod_set_value(1), if the line must be low to enable >> them then they will gpiod_set_value(0). > > gpiod_set_value(1) sets the line to the active state defined in DT > GPIO flags, not the electrical level of the signal. This issue is a > good example of precisely why the gpiod API was defined this way. I do > think it is a bit confusing though. Perhaps reusing _{get,set}_value > API was not the best naming. Yes, it is confusing and I think most drivers are using it following their corresponding datasheets, iow if the datasheet say the line must be low then set_value(0) is used. It does look weird to use set_value(reset_gpio, 1) in the code when the documentation say that the reset pin must be _low_ to place the part into reset, even if you look up the DT documentation and dts files for GPIO_ACTIVE_HIGH/LOW usage among boards. Especially if you have two peripherals where both is enabled when their pin is LOW, but one names it RST and is high active, the other names it EN and it is low active. especially that lots of devices do not even states any active mode for the pin, just if high, it is in reset, if low then it is enabled. I get the notion of how it is, but it does feel a bit unnatural in times. >>> The DT GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there >>> was a recent attempt to do an inverter binding). >> >> Yes. >> If the line is inverted on the board, than the DT GPIO_ACTIVE_LOW will >> invert it to the correct level. > > Yes, if the signal is normally GPIO_ACTIVE_HIGH. > >> We have two off the shelf components, C1 and C2. They have a driver >> written based on the datasheets. >> C1 needs HIGH (LOW reset/disable) >> uses gpiod_set_value(1) to enable the device > > No. The active state for a 'reset-gpios' is the state in which reset > is active/asserted. So gpiod_set_value(1) should always mean 'assert > reset'. > > If we're talking about an 'enable-gpios', then the active state is > when the device is active/enabled. So it's the inverse of > 'reset-gpios'. > >> C2 needs LOW (HIGH reset/disable) >> uses gpiod_set_value(0) to enable the device > > Yes. The GPIO flag would be GPIO_ACTIVE_HIGH and gpiod_set_value(0) is > reset de-asserted. > >> When they are connected to a dedicated GPIO the DT binding has >> GPIO_ACTIVE_HIGH since when the GPIO is set to 1 it goes HIGH, right? > > No, as explained above. C2 would be GPIO_ACTIVE_HIGH, C1 would be > GPIO_ACTIVE_LOW normally. > >> If two device is connected to one GPIO one of them needs an inverter on >> the GPIO line after it is split into two, let say C2 got inverted line: >> C1 tells in DT that the line is not inverted: GPIO_ACTIVE_HOGH >> C2 tells in DT that the line is inverted: GPIO_ACTIVE_LOW > > C1 needs GPIO_ACTIVE_LOW here. Hrm, so the GPIO_ACTIVE_ can not be used as a means to tell that if the gpio line is active (set to 1) at the source then the signal at the component's pin is going to be high (GPIO_ACTIVE_HIGH) or low (GPIO_ACTIVE_LOW)? > >> GPIO HIGH -> D1 is enabled >> -> !HIGH -> LOW -> D2 is enabled >> >> If both would request the same physical GPIO then how would this work? A >> single GPIO can not be handled in inverted and non inverted way at the >> same time. >> >> But this is just a side effect that this would be easy to handle with >> this DT binding and driver. >> After all, it will describe the GPIO line split. >> >>>> It should be possible to add pass-through mode for gpio-shared so that >>>> all requests would propagate to the root GPIO if that's what needed for >>>> some setups. >>>> >>>> That way the gpio-shared would nicely handle the GPIO inversions, would >>>> be able to handle cases to avoid unwanted reset/enable of components or >>>> allow components to be ninja-reset. >>> >>> What does ninja-reset mean? >> >> Ninjas attack from ambush ;) >> The device is reset w/o it's driver being aware that it ever happened as >> other driver toggled the shared GPIO line. >> >>>> I think it would be possible to add gpiod_is_shared(struct gpio_desc >>>> *desc) so users can check if the GPIO is shared - it would only return >>>> true if the gpio-shared is not in pass-through mode so they can know >>>> that the state they see on their gpio desc is not necessary matching >>>> with reality. >>>> Probably another gpiod_shared_get_root_value() to fetch the root's state? >>>> >>>> I intentionally not returning that in the driver as clients might skip a >>>> gpio_set_value() seeing that the GPIO line is already in a state they >>>> would want it, but that would not register their needs for the level. >>>> >>>> - 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 - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 05/11/2019 14:32, Peter Ujfalusi wrote: > > > On 05/11/2019 14.15, Grygorii Strashko wrote: >> >> >> On 05/11/2019 11:58, Linus Walleij wrote: >>> On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote: >>>> [Peter] >>>>> The device needs the RST line to be high, otherwise it is not >>>>> accessible. If it does not have reset control how can we make sure that >>>>> the GPIO line is in correct state? >>>> >>>> Just like the reset code, drivers register their use of the reset and >>>> the core tracks users and prevents resetting when not safe. Maybe the >>>> reset subsystem needs to learn about GPIO resets. (...) >>> >>> I agree. Certainly the reset subsystem can do what the regulator >>> subsystem is already doing: request the GPIO line nonexclusive >>> and handle any reference counting and/or quirks that are needed >>> in a hypothetical drivers/reset/reset-gpio.c driver. >>> >>> There is no such driver today, just a "reset" driver in >>> drivers/power/reset that resets the whole system. >>> >>> But I see no problem in creating a proper reset driver in drivers/reset >>> to handle a few peripherals with a shared GPIO reset line. >> >> Personally, I agree with Mark's comment here: >> >>> [Mark] >>> The theory with that was that any usage of this would need the >>> higher level code using the GPIO to cooperate so they didn't step >>> on each other's toes so the GPIO code should just punt to it. >>> >>>> But let's say that a board design will pick two components (C1 and C2) >>>> and use the same GPIO line to enable them. We already have the drivers >>>> for them and they are used in boards already. >>> >>> This is basically an attempt to make a generic implementation of >>> that cooperation for simple cases. >>> >> >> This looks like unsolvable problem in generic way. >> Lets assume there are some generic shared reset controller invented, but >> then >> - What if some driver is loaded/unloaded and corresponding device uses >> shared >> reset which is de-asserted already? >> In this case, driver should never ever expect that target device has all >> registers in default state. >> - What if reset is required as part of error recovery procedure? The >> error recovery >> will not be supported by such design. >> - PM: Device reset could be part of suspend/resume sequence. if one of >> the devices >> is wake-up source, but other are not, those devices might be in very >> unexpected state during resume. >> - There could be dependencies on reset timings, shared reset might work for >> similar devices (like set of net phys) and does not work if connected >> devices are different. > > and some driver shamelessly implements runtime power/reset control while > other driver does not (they were never used on board where they had > shared GPIO, probably power at most) > >> >> It seems, the only one case when it might help is system boot when: >> - similar devices are connected to the reset line >> - drivers are not expected to be re-loaded >> - device reset is not part of any recovery procedure >> - safe reset timings can be defined for all connected devices >> (but hey - if this is boot only then gpio-hogs should work. Are they?) > > That is another thing which almost works ;) > w/o gpio binding deferred probing is not possible if the GPIO controller > is probed later. > In some cases it might be even impossible to make sure that the GPIO > controller would probe first (GPIO extender on different i2c bus than > the user(s) of the gpio line) > In some cases moving around nodes in DT might artificially make things > work, but then someone compiles the expander as module, or some 'small' > change in kernel and the probe order on the bus changes. > I don't think it is a valid thing to have commits on the DT files > saying: move the expander front/after the hog affected user since since > Monday the probe order has changed. Then move it back two weeks later ;) > Ok. Above sounds like real problem. The implicit dependence is exist, but can't be resolved if any driver depends on gpio-hog of some gpio-controller. Probe deferring of gpio-controller will not lead to probe differing of dependent driver. Question: will gpio-hog mechanism resolve your case if it works (and probe differing issues)? -- Best regards, grygorii
On 05/11/2019 20.07, Grygorii Strashko wrote: >>> (but hey - if this is boot only then gpio-hogs should work. Are they?) >> >> That is another thing which almost works ;) >> w/o gpio binding deferred probing is not possible if the GPIO controller >> is probed later. >> In some cases it might be even impossible to make sure that the GPIO >> controller would probe first (GPIO extender on different i2c bus than >> the user(s) of the gpio line) >> In some cases moving around nodes in DT might artificially make things >> work, but then someone compiles the expander as module, or some 'small' >> change in kernel and the probe order on the bus changes. >> I don't think it is a valid thing to have commits on the DT files >> saying: move the expander front/after the hog affected user since since >> Monday the probe order has changed. Then move it back two weeks later ;) >> > > Ok. Above sounds like real problem. The implicit dependence is exist, > but can't > be resolved if any driver depends on gpio-hog of some gpio-controller. > Probe deferring of gpio-controller will not lead to probe differing of > dependent driver. > > Question: will gpio-hog mechanism resolve your case if it works (and > probe differing issues)? I see gpio-hog to fulfill different role, use cases. It is more like controlling muxes on boards to select between different exclusive features. Things like route the I2S lines to analog codec or HDMI, route RGB video to LCD panel or to HDMI, etc. But, if it would work it could be used for components which can be enabled all the time. On the other hand, if a device has reset/enable line then the driver should have a way to control it. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Wed, 2019-11-06 at 11:23 +0200, Peter Ujfalusi wrote: > > On 05/11/2019 20.07, Grygorii Strashko wrote: > > > > (but hey - if this is boot only then gpio-hogs should work. Are they?) > > > > > > That is another thing which almost works ;) > > > w/o gpio binding deferred probing is not possible if the GPIO controller > > > is probed later. > > > In some cases it might be even impossible to make sure that the GPIO > > > controller would probe first (GPIO extender on different i2c bus than > > > the user(s) of the gpio line) > > > In some cases moving around nodes in DT might artificially make things > > > work, but then someone compiles the expander as module, or some 'small' > > > change in kernel and the probe order on the bus changes. > > > I don't think it is a valid thing to have commits on the DT files > > > saying: move the expander front/after the hog affected user since since > > > Monday the probe order has changed. Then move it back two weeks later ;) > > > > > > > Ok. Above sounds like real problem. The implicit dependence is exist, > > but can't > > be resolved if any driver depends on gpio-hog of some gpio-controller. > > Probe deferring of gpio-controller will not lead to probe differing of > > dependent driver. > > > > Question: will gpio-hog mechanism resolve your case if it works (and > > probe differing issues)? > > I see gpio-hog to fulfill different role, use cases. It is more like > controlling muxes on boards to select between different exclusive > features. Things like route the I2S lines to analog codec or HDMI, route > RGB video to LCD panel or to HDMI, etc. > > But, if it would work it could be used for components which can be > enabled all the time. On the other hand, if a device has reset/enable > line then the driver should have a way to control it. I wonder if it would be useful to differentiate between required and suggested state in the consumer facing GPIO API for nonexclusive GPIOs. A driver that is ok with the enable line going into active state at any time while the device is suspended could use gpiod_set_value(en_gpio, 1); to resume, but gpiod_politely_suggest_value(en_gpio, 0); or similar to suspend, and the core could allow other drivers to override this state. Similarly to how the regulator framework allows consumers to set a voltage range, and the core decides on the actual voltage that fits the constraints. regards Philipp
Hi, On Mon, 2019-11-18 at 13:15 +0100, Enrico Weigelt, metux IT consult wrote: > On 30.10.19 13:04, Peter Ujfalusi wrote: [...] > Let's sit back and rethink what the driver really wants to tell in those > cases. For the enable lines we have: > > a) make sure the device is enabled/powered > b) device does not need to be enabled/powered anymore > c) device must be powercycled > > You see, it's actually tristate, which gets relevant if multiple devices > on one line. Is this just a GPIO-controlled power domain? > Now add reset lines: > > a) force device into reset state > b) force device out of reset state > c) allow device going into reset state (but no need to force) > d) allow device coming out of reset state (but no need to force) > > It even gets more weird if a device can be reset or powercycled > externally. And some drivers just require "b), but device must have been in reset state at any point in the past". > hmm, not entirely trivial ... > > > 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. > > Yeah, at least we need some signalling to the driver, so it can do the > necessary steps. From the driver's PoV, it's an "foreign reset". This could become complicated fast. It's trivial to add a notification mechanism and to let notified drivers veto the foreign reset. But what if driver (a) wants to reset its hardware and driver (b) could save its state and handle being reset, but only after some currently active transfer is finished. Now whether the reset succeeds would depend on how long driver (b) expects its transfer to last and on how long driver (a) would be willing to wait for the reset? [...] > > and all existing drivers must > > be converted to use the reset framework (and adding a linux only warpper on top > > of reset GPIOs). > > Maybe a bit time consuming, but IMHO not difficult. We could add generic > helpers for creating a reset driver on a gpio. So the drivers wouldn't > even care about gpio itself anymore, but let the reset subsystem so it > all (eg. look for DT node and request corresponding gpio, etc). > > IMHO, that's something we should do nevertheless, even if it's just for > cleaner code. We can't change the current DT bindings though. One thing we could do is teach the reset controller framework to handle reset-gpios properties itself. Still, that wouldn't help with the enable and powerdown GPIOs. > After that we could put any kind of funny logic behind the scenes (eg. > one could connect the reset pin to a spare uart instead of gpio, etc), > w/o ever touching the individual drivers. I'm not convinced at all that this is a good thing to do behind the scenes. For those cases I'd prefer adding a "resets" property to the device bindings and explicitly describing a "uart-reset-controller" in the device tree, see for example the "pwm-clock". regards Philipp
Hi, On 18/11/2019 14.15, Enrico Weigelt, metux IT consult wrote: > On 30.10.19 13:04, Peter Ujfalusi wrote: > > Hi, > >> 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. > > IMHO, for such cases, invidual drivers shouldn't fiddle w/ raw gpio's > directly, but be connected to (gpio-based) reset controllers or > regulators instead. Which is a (linux) software abstraction of an electric wire coming out from the gpio (or gpo) controller then split into two (or more branch) and connect to external components... > I believe, GPIO isn't the correct abstraction layer > for such cases: it's not even IO, just O. A GPIO pin configured as output is O ;) > Let's sit back and rethink what the driver really wants to tell in those > cases. For the enable lines we have: > > a) make sure the device is enabled/powered > b) device does not need to be enabled/powered anymore > c) device must be powercycled > > You see, it's actually tristate, which gets relevant if multiple devices > on one line. Yes. Things gets a bit blurry when a GPIO line is used to enable/gate signals from/to a chip on top of enable/disable, like muting an amplifier's analog output. > Now add reset lines: > > a) force device into reset state > b) force device out of reset state > c) allow device going into reset state (but no need to force) > d) allow device coming out of reset state (but no need to force) I would say that coming out of reset is always forced as there is a reason why you want to take it out - it is going to be used. > It even gets more weird if a device can be reset or powercycled > externally. > > hmm, not entirely trivial ... When we have only one user of the GPIO reset/enable line we will have a) and b) happening. If the GPIO is shared most likely the intention of the hw design dictates c) and d) >> 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. > > Yeah, at least we need some signalling to the driver, so it can do the > necessary steps. From the driver's PoV, it's an "foreign reset". Notification callback for state change? >> 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. > > How exactly do we know when such disturbance can / cannot happen ? > That would be depending on individual chips *and* how they're wired on > the board. We'd end up with some logical multiplexer, that's board > specific. > > <snip> > >> 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. > > So, if one driver request reset, all attached devices will be reset ? > Or if all drivers request reset, all attached devices will be reset ? The later. > Doesn't look so quite non-disturbing to me :o This is what regulators and the reset framework is doing, no? >> 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 > > What are the exact scenarios you have in mind ? grep -R enable-gpios Documentation/devicetree/bindings/* pick two random device from the output, place it on a board with shared enable GPIO line. I know I over simplify (or complicate) the real world use. >> and all existing drivers must >> be converted to use the reset framework (and adding a linux only warpper on top >> of reset GPIOs). > > Maybe a bit time consuming, but IMHO not difficult. We could add generic > helpers for creating a reset driver on a gpio. So the drivers wouldn't > even care about gpio itself anymore, but let the reset subsystem so it > all (eg. look for DT node and request corresponding gpio, etc). You mean that users would use reset_control_get_optional_shared() only and if there is no valid reset binding that the reset core would look for a gpio binding and instantiate a gpio-reset controller? But before instantiating it, it would look around in some list to see if the gpio-reset controller for the same gpio line is already exist? > IMHO, that's something we should do nevertheless, even if it's just for > cleaner code. I'm not sure about that. D1 have ENABLE pin (enable-gpios as per dt documentation), if the line is high, the device is enabled, if low it is disabled. D2 have RESET pin (reset-gpios as per dt documentation), if the line is high, the device is enabled, if low it is disabled. D1's driver would: enable-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>; priv->reset = reset_control_get_optional_shared(dev, "enable"); /* Place it to reset: ENABLE pin should be pulled low */ reset_control_assert(priv->reset); /* Remove from reset: ENABLE pin should be high */ reset_control_deassert(priv->reset); D2's driver would: reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>; priv->reset = reset_control_get_optional_shared(dev, "reset"); /* Place it to reset: RESET pin should be pulled low */ reset_control_assert(priv->reset); /* Remove from reset: RESET pin should be high */ reset_control_deassert(priv->reset); The reset framework must know somehow that the reset control for D1 is an enable type of gpio, so it must treat it as inverted polarity while the reset type of binding should be follow the selected active level. Then it must protect (most likely) the deasserted state: it does not mater if there is any assert request for the reset_control if we have one deassert active as at least one device must be enabled. For new dts files the virtual reset-gpio controller node can be present and the level of assert and deassert is told to it via the gpio binding. Something like this? > After that we could put any kind of funny logic behind the scenes (eg. > one could connect the reset pin to a spare uart instead of gpio, etc), > w/o ever touching the individual drivers. Not sure if I follow you here. On the other hand the gpio line itself can be seen as a regultator itself (3.3V most of the time) so in theory all GPIOs can be regulators as well, but regulator framework protects the >0 volt state while there are devices which can be enabled when the ENABLE/RST pin is pulled low. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki