| Message ID | 20240423134611.31979-4-johan+linaro@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on | expand |
On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote: > On 23/04/2024 15:46, Johan Hovold wrote: > > When the power supply is shared with other peripherals the reset line > > can be wired in such a way that it can remain deasserted regardless of > > whether the supply is on or not. > > To clarify: the reset line is still present and working in such case? Yes. > > This is important as it can be used to avoid holding the controller in > > reset for extended periods of time when it remains powered, something > > which can lead to increased power consumption. Leaving reset deasserted > > also avoids leaking current through the reset circuitry pull-up > > resistors. > > > > Add a new 'no-reset-on-power-off' devicetree property which can be used > > by the OS to determine when reset needs to be asserted on power down. > > > > Note that this property can also be used when the supply cannot be > > turned off by the OS at all. > > reset-gpios: > > description: Reset GPIO; not all touchscreens using eKTH6915 hook this up. > > > > + no-reset-on-power-off: > > Missing vendor prefix. Unless you want to re-use existing property > "keep-power-in-suspend", but the case here mentions power off, not suspend. No, I left out the prefix on purpose as I mentioned in the cover letter. There is nothing vendor specific about this property and I expect it to be reused for other devices. And "keep-power-in-suspend" is too specific and indeed looks like instruction to the OS rather than hw description (more below), but importantly it is not related to the problem here (which is about reset, not power). > Anyway, the property sounds like what the OS should be doing, which is > not what we want. You basically instruct driver what to do. We want a > described hardware configuration or hardware specifics. Right, and this was why I at first rejected a property name like this in favour of 'reset-pulled-to-supply' in my first draft. That name obviously does not work as the 'supply' suffix is already claimed, but I also realised that it doesn't really describe the hardware property that allows the reset line to remain asserted. The key feature in this hardware design is that the reset line will not just be pulled to the supply voltage (what other voltage would it be pulled to), but that it is also pulled to ground when the supply is disabled. Rather than trying to encode this in the property name, I settled on the descriptive 'no-reset-on-power-off' after the seeing the prior art in 'goodix,no-reset-during-suspend' property. The latter is too specific and encodes policy, but the former could still be considered hardware description and would also apply to other designs which have the property that the reset line should be left deasserted. One such example is when the supply can not be disabled at all (e.g. the Goodix case), but I can imagine there being more than one way to design such reset circuits. > Reset is pulled to something? What is exactly different in this hardware > configuration comparing to other hardware setup (regular)? The power supply is shared with other peripherals and the hardware designers have made provisions so that the reset line can remain deasserted regardless of the state of the supply in order to save power. > > + type: boolean > > + description: > > + Reset line is wired so that it can be left deasserted when the power > > + supply is off. Johan
On 24/04/2024 09:34, Johan Hovold wrote: > On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote: >> On 23/04/2024 15:46, Johan Hovold wrote: >>> When the power supply is shared with other peripherals the reset line >>> can be wired in such a way that it can remain deasserted regardless of >>> whether the supply is on or not. >> >> To clarify: the reset line is still present and working in such case? > > Yes. > >>> This is important as it can be used to avoid holding the controller in >>> reset for extended periods of time when it remains powered, something >>> which can lead to increased power consumption. Leaving reset deasserted >>> also avoids leaking current through the reset circuitry pull-up >>> resistors. >>> >>> Add a new 'no-reset-on-power-off' devicetree property which can be used >>> by the OS to determine when reset needs to be asserted on power down. >>> >>> Note that this property can also be used when the supply cannot be >>> turned off by the OS at all. > >>> reset-gpios: >>> description: Reset GPIO; not all touchscreens using eKTH6915 hook this up. >>> >>> + no-reset-on-power-off: >> >> Missing vendor prefix. Unless you want to re-use existing property >> "keep-power-in-suspend", but the case here mentions power off, not suspend. > > No, I left out the prefix on purpose as I mentioned in the cover letter. > There is nothing vendor specific about this property and I expect it to > be reused for other devices. > > And "keep-power-in-suspend" is too specific and indeed looks like > instruction to the OS rather than hw description (more below), but > importantly it is not related to the problem here (which is about > reset, not power). > >> Anyway, the property sounds like what the OS should be doing, which is >> not what we want. You basically instruct driver what to do. We want a >> described hardware configuration or hardware specifics. > > Right, and this was why I at first rejected a property name like this in > favour of 'reset-pulled-to-supply' in my first draft. That name > obviously does not work as the 'supply' suffix is already claimed, but I > also realised that it doesn't really describe the hardware property that > allows the reset line to remain asserted. > > The key feature in this hardware design is that the reset line will not > just be pulled to the supply voltage (what other voltage would it be > pulled to), but that it is also pulled to ground when the supply is > disabled. OK, if the property was specific to the hardware, then I would propose something more hardware-related, e.g. "reset-supply-tied". However : > Rather than trying to encode this in the property name, I settled on the > descriptive 'no-reset-on-power-off' after the seeing the prior art in > 'goodix,no-reset-during-suspend' property. The latter is too specific > and encodes policy, but the former could still be considered hardware > description and would also apply to other designs which have the > property that the reset line should be left deasserted. > > One such example is when the supply can not be disabled at all (e.g. the > Goodix case), but I can imagine there being more than one way to design > such reset circuits. It seems it is common problem. LEDs have property "retain-state-shutdown", to indicate that during system shutdown we should not touch them (like power off). Would some variant be applicable here? First, do we talk here about power off like system shutdown or runtime PM, thus suspend? Best regards, Krzysztof
Hi Krzysztof, and sorry about the late reply. Got side-tracked. On Thu, Apr 25, 2024 at 11:39:24AM +0200, Krzysztof Kozlowski wrote: > On 24/04/2024 09:34, Johan Hovold wrote: > > On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote: > >> On 23/04/2024 15:46, Johan Hovold wrote: > >>> When the power supply is shared with other peripherals the reset line > >>> can be wired in such a way that it can remain deasserted regardless of > >>> whether the supply is on or not. > >>> This is important as it can be used to avoid holding the controller in > >>> reset for extended periods of time when it remains powered, something > >>> which can lead to increased power consumption. Leaving reset deasserted > >>> also avoids leaking current through the reset circuitry pull-up > >>> resistors. > >>> > >>> Add a new 'no-reset-on-power-off' devicetree property which can be used > >>> by the OS to determine when reset needs to be asserted on power down. > >>> > >>> Note that this property can also be used when the supply cannot be > >>> turned off by the OS at all. > > > >>> reset-gpios: > >>> description: Reset GPIO; not all touchscreens using eKTH6915 hook this up. > >>> > >>> + no-reset-on-power-off: > >> Anyway, the property sounds like what the OS should be doing, which is > >> not what we want. You basically instruct driver what to do. We want a > >> described hardware configuration or hardware specifics. > > > > Right, and this was why I at first rejected a property name like this in > > favour of 'reset-pulled-to-supply' in my first draft. That name > > obviously does not work as the 'supply' suffix is already claimed, but I > > also realised that it doesn't really describe the hardware property that > > allows the reset line to remain asserted. > > > > The key feature in this hardware design is that the reset line will not > > just be pulled to the supply voltage (what other voltage would it be > > pulled to), but that it is also pulled to ground when the supply is > > disabled. > > OK, if the property was specific to the hardware, then I would propose > something more hardware-related, e.g. "reset-supply-tied". However : > > > Rather than trying to encode this in the property name, I settled on the > > descriptive 'no-reset-on-power-off' after the seeing the prior art in > > 'goodix,no-reset-during-suspend' property. The latter is too specific > > and encodes policy, but the former could still be considered hardware > > description and would also apply to other designs which have the > > property that the reset line should be left deasserted. > > > > One such example is when the supply can not be disabled at all (e.g. the > > Goodix case), but I can imagine there being more than one way to design > > such reset circuits. > > It seems it is common problem. LEDs have property > "retain-state-shutdown", to indicate that during system shutdown we > should not touch them (like power off). Would some variant be applicable > here? First, do we talk here about power off like system shutdown or > runtime PM, thus suspend? A name like 'retain-state-shutdown' would also be too specific as what I'm describing here is that the reset line should be (or can be) left deasserted whenever the OS wants to power off the device. That could be during suspend, but more generally whenever the OS determines that the device does not need to be powered (e.g. when closing a character device). Johan
Hi Johan, thanks for your patch! On Tue, Apr 23, 2024 at 3:46 PM Johan Hovold <johan+linaro@kernel.org> wrote: > When the power supply is shared with other peripherals the reset line > can be wired in such a way that it can remain deasserted regardless of > whether the supply is on or not. > > This is important as it can be used to avoid holding the controller in > reset for extended periods of time when it remains powered, something > which can lead to increased power consumption. Leaving reset deasserted > also avoids leaking current through the reset circuitry pull-up > resistors. So the reset line in this case is a GPIO as seen from the context above. To me that means that the line should have the GPIO_OPEN_DRAIN flag set in the device tree node for reset-gpios. As it has pull-up resistors, setting the line to high impedance takes the device out of reset, and thus it is effectively open drain. > Add a new 'no-reset-on-power-off' devicetree property which can be used > by the OS to determine when reset needs to be asserted on power down. If the above holds true, the driver can then just check for the open drain flag in the reset-gpios phandle, and if that is set, conclude that it should not actively drive the line low in the poweroff state. I'd like Krzysztof's input on this though, as he's been all over the reset code recently and knows it better than me. Yours, Linus Walleij
On Fri, May 03, 2024 at 09:40:43AM +0200, Linus Walleij wrote: > On Tue, Apr 23, 2024 at 3:46 PM Johan Hovold <johan+linaro@kernel.org> wrote: > > > When the power supply is shared with other peripherals the reset line > > can be wired in such a way that it can remain deasserted regardless of > > whether the supply is on or not. > > > > This is important as it can be used to avoid holding the controller in > > reset for extended periods of time when it remains powered, something > > which can lead to increased power consumption. Leaving reset deasserted > > also avoids leaking current through the reset circuitry pull-up > > resistors. > > So the reset line in this case is a GPIO as seen from the context above. > > To me that means that the line should have the GPIO_OPEN_DRAIN flag > set in the device tree node for reset-gpios. As it has pull-up resistors, > setting the line to high impedance takes the device out of reset, and > thus it is effectively open drain. If you look at the devicetree patch later in the series this is exactly what is done. > > Add a new 'no-reset-on-power-off' devicetree property which can be used > > by the OS to determine when reset needs to be asserted on power down. > > If the above holds true, the driver can then just check for the open drain flag > in the reset-gpios phandle, and if that is set, conclude that it should not > actively drive the line low in the poweroff state. That is an alternative I considered but rejected as just knowing that the gpio is open-drain is not necessarily sufficient, for example, if the reset line is pulled to always-on rail while power to the device can be cut. Perhaps no one would ever construct hardware like that, but it does not seem like the hardware property I'm trying to encode necessarily follows from having an open-drain reset line. And then the OS should probably not make assumptions like that either, especially since getting it wrong can potentially lead to damaged hardware. Johan
On 02/05/2024 11:56, Johan Hovold wrote: > Hi Krzysztof, > > and sorry about the late reply. Got side-tracked. > > On Thu, Apr 25, 2024 at 11:39:24AM +0200, Krzysztof Kozlowski wrote: >> On 24/04/2024 09:34, Johan Hovold wrote: >>> On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote: >>>> On 23/04/2024 15:46, Johan Hovold wrote: >>>>> When the power supply is shared with other peripherals the reset line >>>>> can be wired in such a way that it can remain deasserted regardless of >>>>> whether the supply is on or not. > >>>>> This is important as it can be used to avoid holding the controller in >>>>> reset for extended periods of time when it remains powered, something >>>>> which can lead to increased power consumption. Leaving reset deasserted >>>>> also avoids leaking current through the reset circuitry pull-up >>>>> resistors. >>>>> >>>>> Add a new 'no-reset-on-power-off' devicetree property which can be used >>>>> by the OS to determine when reset needs to be asserted on power down. >>>>> >>>>> Note that this property can also be used when the supply cannot be >>>>> turned off by the OS at all. >>> >>>>> reset-gpios: >>>>> description: Reset GPIO; not all touchscreens using eKTH6915 hook this up. >>>>> >>>>> + no-reset-on-power-off: > >>>> Anyway, the property sounds like what the OS should be doing, which is >>>> not what we want. You basically instruct driver what to do. We want a >>>> described hardware configuration or hardware specifics. >>> >>> Right, and this was why I at first rejected a property name like this in >>> favour of 'reset-pulled-to-supply' in my first draft. That name >>> obviously does not work as the 'supply' suffix is already claimed, but I >>> also realised that it doesn't really describe the hardware property that >>> allows the reset line to remain asserted. >>> >>> The key feature in this hardware design is that the reset line will not >>> just be pulled to the supply voltage (what other voltage would it be >>> pulled to), but that it is also pulled to ground when the supply is >>> disabled. >> >> OK, if the property was specific to the hardware, then I would propose >> something more hardware-related, e.g. "reset-supply-tied". However : >> >>> Rather than trying to encode this in the property name, I settled on the >>> descriptive 'no-reset-on-power-off' after the seeing the prior art in >>> 'goodix,no-reset-during-suspend' property. The latter is too specific >>> and encodes policy, but the former could still be considered hardware >>> description and would also apply to other designs which have the >>> property that the reset line should be left deasserted. >>> >>> One such example is when the supply can not be disabled at all (e.g. the >>> Goodix case), but I can imagine there being more than one way to design >>> such reset circuits. >> >> It seems it is common problem. LEDs have property >> "retain-state-shutdown", to indicate that during system shutdown we >> should not touch them (like power off). Would some variant be applicable >> here? First, do we talk here about power off like system shutdown or >> runtime PM, thus suspend? > > A name like 'retain-state-shutdown' would also be too specific as what > I'm describing here is that the reset line should be (or can be) left > deasserted whenever the OS wants to power off the device. I don't think it is more specific than yours. It is actually more generic. First, shutdown=poweroff, so that part is the same. retain-state means keep things enabled, asserted, deasserted, etc, so multiple cases. Your wording is specific - only one state is kept during power off: reset deassert. Best regards, Krzysztof
On Fri, May 03, 2024 at 11:11:16AM +0200, Krzysztof Kozlowski wrote: > On 02/05/2024 11:56, Johan Hovold wrote: > > On Thu, Apr 25, 2024 at 11:39:24AM +0200, Krzysztof Kozlowski wrote: > >> It seems it is common problem. LEDs have property > >> "retain-state-shutdown", to indicate that during system shutdown we > >> should not touch them (like power off). Would some variant be applicable > >> here? First, do we talk here about power off like system shutdown or > >> runtime PM, thus suspend? > > > > A name like 'retain-state-shutdown' would also be too specific as what > > I'm describing here is that the reset line should be (or can be) left > > deasserted whenever the OS wants to power off the device. > > I don't think it is more specific than yours. It is actually more > generic. First, shutdown=poweroff, so that part is the same. My point is that 'shutdown' is a specific OS concept (i.e. turning the whole system off), while powering off a *device* can be done for a number of reasons including closing a character device, suspend and system-wide shutdown. The policy decision of when to power off a device is left up to kernel (e.g. if a device needs to be kept on as it is currently configured as a wakeup device). Johan
On Fri, May 3, 2024 at 10:47 AM Johan Hovold <johan@kernel.org> wrote: > > If the above holds true, the driver can then just check for the open drain flag > > in the reset-gpios phandle, and if that is set, conclude that it should not > > actively drive the line low in the poweroff state. > > That is an alternative I considered but rejected as just knowing that > the gpio is open-drain is not necessarily sufficient, for example, if > the reset line is pulled to always-on rail while power to the device can > be cut. > > Perhaps no one would ever construct hardware like that, but it does not > seem like the hardware property I'm trying to encode necessarily follows > from having an open-drain reset line. > > And then the OS should probably not make assumptions like that either, > especially since getting it wrong can potentially lead to damaged > hardware. OK it's a fair point. I was worried about over-specification of behaviour, as that always leads to contradictions. + no-reset-on-power-off: + type: boolean + description: + Reset line is wired so that it can be left deasserted when the power + supply is off. To be nitpicky: *should* be left deasserted rather than *can* be left deasserted, right? If the behaviour is desirable but not strictly required. Yours, Linus Walleij
On Mon, May 06, 2024 at 08:29:40AM +0200, Linus Walleij wrote: > + no-reset-on-power-off: > + type: boolean > + description: > + Reset line is wired so that it can be left deasserted when the power > + supply is off. > > To be nitpicky: *should* be left deasserted rather than *can* be left > deasserted, right? If the behaviour is desirable but not strictly > required. I considered that too, but settled on the above description as it is pure hardware description and leaving the decision to act on it up to the OS (e.g. if support is implemented). On the other hand, the "should" is already implied by the property name so perhaps there's no reason not to include it also in the description: + no-reset-on-power-off: + type: boolean + description: + Reset line is wired so that it can (and should) be left + deasserted when the power supply is off. And "should" (unlike "shall") still leaves room for an OS to ignore it at the cost of increased power consumption. Johan
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml index c3a6f901ff45..3d20673f10b2 100644 --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml @@ -37,6 +37,12 @@ properties: reset-gpios: description: Reset GPIO; not all touchscreens using eKTH6915 hook this up. + no-reset-on-power-off: + type: boolean + description: + Reset line is wired so that it can be left deasserted when the power + supply is off. + vcc33-supply: description: The 3.3V supply to the touchscreen.
When the power supply is shared with other peripherals the reset line can be wired in such a way that it can remain deasserted regardless of whether the supply is on or not. This is important as it can be used to avoid holding the controller in reset for extended periods of time when it remains powered, something which can lead to increased power consumption. Leaving reset deasserted also avoids leaking current through the reset circuitry pull-up resistors. Add a new 'no-reset-on-power-off' devicetree property which can be used by the OS to determine when reset needs to be asserted on power down. Note that this property can also be used when the supply cannot be turned off by the OS at all. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 6 ++++++ 1 file changed, 6 insertions(+)