[PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Lukasz Majczak]

This patch extends the early bailout for probing procedure introduced in
commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
really probing"), in order to cover devices
based on STM microcontrollers. For touchpads based on STM uC,
the probe sequence needs to take into account the increased response time
for i2c transaction if the device already entered a deep power state.
STM specify the wakeup time as 400us between positive strobe of
the clock line. Deep sleep is controlled by Touchpad FW,
though some devices enter it pretty early to conserve power
in case of lack of activity on the i2c bus.
Failing to follow this requirement will result in touchpad device not being
detected due initial transaction being dropped by STM i2c slave controller.
By adding additional try, first transaction will wake up the touchpad
STM controller, and the second will produce proper detection response.

v1->v2:
* Updated commit message with short sha of a base commit and proper tags
* Rearranged while loop to perform check only once
* Loosened sleeping range

Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
Signed-off-by: Lukasz Majczak <lma@chromium.org>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2df1ab3c31cc..ece1a5815e0b 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1013,9 +1013,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
 	struct i2c_client *client = ihid->client;
 	struct hid_device *hid = ihid->hid;
 	int ret;
+	int tries = 2;
+
+	while (true) {
+		/* Make sure there is something at this address */
+		ret = i2c_smbus_read_byte(client);
+		tries--;
+		if (tries == 0 || ret >= 0)
+			break;
+		usleep_range(400, 500);
+	}
 
-	/* Make sure there is something at this address */
-	ret = i2c_smbus_read_byte(client);
 	if (ret < 0) {
 		i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
 		return -ENXIO;
-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Doug Anderson]

Hi,

On Fri, Apr 5, 2024 at 3:24 AM Lukasz Majczak <lma@chromium.org> wrote:
>
> This patch extends the early bailout for probing procedure introduced in
> commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> really probing"), in order to cover devices
> based on STM microcontrollers. For touchpads based on STM uC,
> the probe sequence needs to take into account the increased response time
> for i2c transaction if the device already entered a deep power state.
> STM specify the wakeup time as 400us between positive strobe of
> the clock line. Deep sleep is controlled by Touchpad FW,
> though some devices enter it pretty early to conserve power
> in case of lack of activity on the i2c bus.
> Failing to follow this requirement will result in touchpad device not being
> detected due initial transaction being dropped by STM i2c slave controller.
> By adding additional try, first transaction will wake up the touchpad
> STM controller, and the second will produce proper detection response.
>
> v1->v2:
> * Updated commit message with short sha of a base commit and proper tags
> * Rearranged while loop to perform check only once
> * Loosened sleeping range
>
> Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Lukasz Majczak <lma@chromium.org>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

As per my review comments in response to your v1 [1], this seems
reasonable to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[1] https://lore.kernel.org/r/20240325105452.529921-1-lma@chromium.org

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Johan Hovold]

On Fri, Apr 05, 2024 at 10:24:36AM +0000, Lukasz Majczak wrote:
> This patch extends the early bailout for probing procedure introduced in
> commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> really probing"), in order to cover devices
> based on STM microcontrollers. For touchpads based on STM uC,
> the probe sequence needs to take into account the increased response time
> for i2c transaction if the device already entered a deep power state.
> STM specify the wakeup time as 400us between positive strobe of
> the clock line. Deep sleep is controlled by Touchpad FW,
> though some devices enter it pretty early to conserve power
> in case of lack of activity on the i2c bus.
> Failing to follow this requirement will result in touchpad device not being
> detected due initial transaction being dropped by STM i2c slave controller.
> By adding additional try, first transaction will wake up the touchpad
> STM controller, and the second will produce proper detection response.

Can you please explain why this would not a problem for all future
transactions as well?

If it is, then it sounds like this needs to be addressed in the i2c
driver. If not, then perhaps the problem is really that you just need a
delay after enabling the power supplies? 
 
> v1->v2:
> * Updated commit message with short sha of a base commit and proper tags
> * Rearranged while loop to perform check only once
> * Loosened sleeping range
> 
> Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Lukasz Majczak <lma@chromium.org>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2df1ab3c31cc..ece1a5815e0b 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1013,9 +1013,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
>  	struct i2c_client *client = ihid->client;
>  	struct hid_device *hid = ihid->hid;
>  	int ret;
> +	int tries = 2;

Nit: move above the 'ret' declaration to maintain reverse xmas style
ordering.

Johan

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Łukasz Majczak]

Hi Johan,
>
> Can you please explain why this would not a problem for all future
> transactions as well?
>
> If it is, then it sounds like this needs to be addressed in the i2c
> driver. If not, then perhaps the problem is really that you just need a
> delay after enabling the power supplies?
>

The problem is that the probe function calling i2c_smbus_read_byte()
is not aware that
uC on the other end is in a deep sleep state so the first read will
fail and so the whole probe.

In a normal scenario, when a user touches the touchpad (when it is in
a deep sleep), the touch will first wake up the uC and
then generate an interrupt to AP, so in this case the touchpad is
fully awake and operational.

> Nit: move above the 'ret' declaration to maintain reverse xmas style
> ordering.
>
> Johan

Will do so.

Kind regards,
Lukasz

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Johan Hovold]

On Tue, Apr 09, 2024 at 12:53:43PM +0200, Łukasz Majczak wrote:

> > Can you please explain why this would not a problem for all future
> > transactions as well?

> The problem is that the probe function calling i2c_smbus_read_byte()
> is not aware that
> uC on the other end is in a deep sleep state so the first read will
> fail and so the whole probe.
> 
> In a normal scenario, when a user touches the touchpad (when it is in
> a deep sleep), the touch will first wake up the uC and
> then generate an interrupt to AP, so in this case the touchpad is
> fully awake and operational.

Sure, but what about other transactions that are initiated by the host
(e.g. SET_POWER)?

Perhaps this hack at probe is enough for your use case, but is an
incomplete hack and at a minimum you'd need to add a comment explaining
why it is there.

Johan

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Łukasz Majczak]

> Sure, but what about other transactions that are initiated by the host
> (e.g. SET_POWER)?
>
Somehow it is problematic only on reboot and works just fine on
suspend/resume and
set_power.
I will dig more and try to find out what the difference is.

> Perhaps this hack at probe is enough for your use case, but is an
> incomplete hack and at a minimum you'd need to add a comment explaining
> why it is there.
>
You mean a comment in the code ?

Lukasz

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Kai-Heng Feng]

Hi Łukasz,

On Thu, Apr 11, 2024 at 10:23 PM Łukasz Majczak <lma@chromium.org> wrote:
>
> > Sure, but what about other transactions that are initiated by the host
> > (e.g. SET_POWER)?
> >
> Somehow it is problematic only on reboot and works just fine on
> suspend/resume and
> set_power.
> I will dig more and try to find out what the difference is.

If cold boot doesn't have such issue, maybe I2C_HID_PWR_SLEEP
shouldn't be used by reboot?

Kai-Heng

>
> > Perhaps this hack at probe is enough for your use case, but is an
> > incomplete hack and at a minimum you'd need to add a comment explaining
> > why it is there.
> >
> You mean a comment in the code ?
>
> Lukasz

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Johan Hovold]

On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote:
> > Sure, but what about other transactions that are initiated by the host
> > (e.g. SET_POWER)?
> >
> Somehow it is problematic only on reboot and works just fine on
> suspend/resume and
> set_power.
> I will dig more and try to find out what the difference is.

Sounds like it may be related to the i2c_hid_set_power() on shutdown()
then as Kai-Heng pointed out.

That function already handles a similar retry for I2C_HID_PWR_ON during
resume.

> > Perhaps this hack at probe is enough for your use case, but is an
> > incomplete hack and at a minimum you'd need to add a comment explaining
> > why it is there.
> >
> You mean a comment in the code ?

Yes, if this turns out to be needed then there should be a comment
explaining why it is there (and currently also as the delays you used
seem specific for your particular platform).

But hopefully you can find a generic solution to this.

Johan

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Radoslaw Biernacki]

Hi Johan,

On Mon, Apr 15, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote:
> > > Sure, but what about other transactions that are initiated by the host
> > > (e.g. SET_POWER)?
> > >
> > Somehow it is problematic only on reboot and works just fine on
> > suspend/resume and
> > set_power.
> > I will dig more and try to find out what the difference is.
>
> Sounds like it may be related to the i2c_hid_set_power() on shutdown()
> then as Kai-Heng pointed out.
>
> That function already handles a similar retry for I2C_HID_PWR_ON during
> resume.
>
> > > Perhaps this hack at probe is enough for your use case, but is an
> > > incomplete hack and at a minimum you'd need to add a comment explaining
> > > why it is there.
> > >
> > You mean a comment in the code ?
>
> Yes, if this turns out to be needed then there should be a comment
> explaining why it is there (and currently also as the delays you used
> seem specific for your particular platform).
>
> But hopefully you can find a generic solution to this.

Yes, we might need a more generic solution though it is not yet clear
for us which would be the cleanest one.
As I wrote in the reply to Kenny, the design back in the day was made
to use events rather than
level driven IO line, to drive the power state of the device.
Consequence is we need to request
a low power state before the kernel goes down as there is no guarantee
the kernel will wake up soon
(prevent battery power leak). This event/level logic problem (event
design for level type problem).

Let us get back to you with more info as we look deeper into some
newly found power sequence limitations
of the second I2C node on this device.

>
> Johan

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Johan Hovold]

On Mon, Apr 15, 2024 at 02:26:42PM +0200, Radoslaw Biernacki wrote:
> On Mon, Apr 15, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote:
> > > > Sure, but what about other transactions that are initiated by the host
> > > > (e.g. SET_POWER)?
> > > >
> > > Somehow it is problematic only on reboot and works just fine on
> > > suspend/resume and
> > > set_power.
> > > I will dig more and try to find out what the difference is.
> >
> > Sounds like it may be related to the i2c_hid_set_power() on shutdown()
> > then as Kai-Heng pointed out.

> > Yes, if this turns out to be needed then there should be a comment
> > explaining why it is there (and currently also as the delays you used
> > seem specific for your particular platform).
> >
> > But hopefully you can find a generic solution to this.
> 
> Yes, we might need a more generic solution though it is not yet clear
> for us which would be the cleanest one.
> As I wrote in the reply to Kenny, the design back in the day was made
> to use events rather than
> level driven IO line, to drive the power state of the device.
> Consequence is we need to request
> a low power state before the kernel goes down as there is no guarantee
> the kernel will wake up soon
> (prevent battery power leak). This event/level logic problem (event
> design for level type problem).

Ok, and do I understand you correctly that it is indeed the SET_POWER
command on shutdown() that makes the device enter the sleep state and
which then needs an extra transaction on next boot to be woken up?

And that the firmware will not enter that low power state on its own
based on lack of activity as the current commit message suggests?

If so, then it seems a variant of this patch would be ok as long as the
commit message and a comment in the code explains the problem correctly.

Johan

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Kenny Levinsen]

> The problem is that the probe function calling i2c_smbus_read_byte()
> is not aware that
> uC on the other end is in a deep sleep state so the first read will
> fail and so the whole probe.
Well, the probe was just added to "avoid scary messages", so we could 
just do away with it and fix the "scary messages" instead.

I think it would be better to handle the wake-up near the command being 
sent that requires the device to be awake, just like is done for 
i2c_hid_set_power(). This would mean removing the smbus probe 
altogether, extending the HID descriptor fetch code to retry on 
EREMOTEIO, and to avoid the "scary messages", print something nice if 
the second attempt also fails with EREMOTEIO.

If the device can enter deep-sleep arbitrarily, then we presumably also 
have problems in i2c_hid_output_raw_report() and 
i2c_hid_get_raw_report() which could happen after the device has gone to 
sleep from inactivity. These places would also need EREMOTEIO retry logic.

All these places should have the same sleeping behavior as they are 
working around the same device glitch. I imagine the client ACK timeout 
is longer than your required 400µs, in which case you don't need any 
sleep on retry at all, as is the case in the current i2c_hid_set_power() 
implementation.

However, as we litter retry-code all over the place, Johan's suggestion 
about doing this in the I2C driver does become a bit more relevant...

Best regards,
Kenny Levinsen

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Radoslaw Biernacki]

Hi Kenny,

> If the device can enter deep-sleep arbitrarily, then we presumably also
> have problems in i2c_hid_output_raw_report() and
> i2c_hid_get_raw_report() which could happen after the device has gone to
> sleep from inactivity. These places would also need EREMOTEIO retry logic.

It does not enter deep-sleep arbitrarily and therefore it is not a problem with
other communication patterns.
The design which was chosen back in the day, to save the battery power
is event based
instead of level based (some HW line). Therefore to avoid power leak
we need to request
low power state (to prevent power leak in case the kernel will not
boot up soon).

Basically we need to take out the device from deep state logic by message,
without knowing if the device is on the bus or it is on the bus but
not responding.

>
> All these places should have the same sleeping behavior as they are
> working around the same device glitch. I imagine the client ACK timeout
> is longer than your required 400µs, in which case you don't need any
> sleep on retry at all, as is the case in the current i2c_hid_set_power()
> implementation.
>
> However, as we litter retry-code all over the place, Johan's suggestion
> about doing this in the I2C driver does become a bit more relevant...
>
> Best regards,
> Kenny Levinsen
>

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Radoslaw Biernacki]

ps: "without knowing if the device is, or is not on the bus ..."

On Mon, Apr 15, 2024 at 2:10 PM Radoslaw Biernacki <biernacki@google.com> wrote:
>
> Hi Kenny,
>
> > If the device can enter deep-sleep arbitrarily, then we presumably also
> > have problems in i2c_hid_output_raw_report() and
> > i2c_hid_get_raw_report() which could happen after the device has gone to
> > sleep from inactivity. These places would also need EREMOTEIO retry logic.
>
> It does not enter deep-sleep arbitrarily and therefore it is not a problem with
> other communication patterns.
> The design which was chosen back in the day, to save the battery power
> is event based
> instead of level based (some HW line). Therefore to avoid power leak
> we need to request
> low power state (to prevent power leak in case the kernel will not
> boot up soon).
>
> Basically we need to take out the device from deep state logic by message,
> without knowing if the device is on the bus or it is on the bus but
> not responding.
>
> >
> > All these places should have the same sleeping behavior as they are
> > working around the same device glitch. I imagine the client ACK timeout
> > is longer than your required 400µs, in which case you don't need any
> > sleep on retry at all, as is the case in the current i2c_hid_set_power()
> > implementation.
> >
> > However, as we litter retry-code all over the place, Johan's suggestion
> > about doing this in the I2C driver does become a bit more relevant...
> >
> > Best regards,
> > Kenny Levinsen
> >

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Johan Hovold]

On Mon, Apr 15, 2024 at 12:28:28PM +0200, Kenny Levinsen wrote:
> > The problem is that the probe function calling i2c_smbus_read_byte()
> > is not aware that
> > uC on the other end is in a deep sleep state so the first read will
> > fail and so the whole probe.

> Well, the probe was just added to "avoid scary messages", so we could 
> just do away with it and fix the "scary messages" instead.

We also use it for devices that may not be populated (e.g. an optional
touchscreen) and in that case we should not print anything.

Johan

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Kenny Levinsen]

On 4/15/24 3:22 PM, Johan Hovold wrote:
> We also use it for devices that may not be populated (e.g. an optional
> touchscreen) and in that case we should not print anything.

I sent a patch series[0] that shows how the same can be achieved 
(gracefully handling missing device + retry to wake device) without any 
smbus probes, following the existing retry style in i2c_hid_set_power().

Radoslaw and Lukasz, it somehow felt rude to tag and blame you for code 
you hadn't seen yet. If my patch ends up favored we should make sure to 
have the appropriate attribution tags added...

[0]: https://lore.kernel.org/all/20240415170517.18780-1-kl@kl.wtf/

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Łukasz Majczak]

On Mon, Apr 15, 2024 at 7:14 PM Kenny Levinsen <kl@kl.wtf> wrote:
>
> On 4/15/24 3:22 PM, Johan Hovold wrote:
> > We also use it for devices that may not be populated (e.g. an optional
> > touchscreen) and in that case we should not print anything.
>
> I sent a patch series[0] that shows how the same can be achieved
> (gracefully handling missing device + retry to wake device) without any
> smbus probes, following the existing retry style in i2c_hid_set_power().
>
> Radoslaw and Lukasz, it somehow felt rude to tag and blame you for code
> you hadn't seen yet. If my patch ends up favored we should make sure to
> have the appropriate attribution tags added...
>
> [0]: https://lore.kernel.org/all/20240415170517.18780-1-kl@kl.wtf/
>
Hi Kenny,

Unfortunately, your fix doesn't help in our case as there is no sleep
before the second call to
i2c_hid_fetch_hid_descriptor().

Saying more, this STM exposes two i2c addresses (connected physically
to the same bus)
one is the HID interface and the other one gives an access to the base
firmware and is
served by cros_ec_i2c driver and actually thanks to this driver,
touchpad is woken up because
In the resume path cros_ec_i2c "starts talking" through the i2c bus
thus generating a wakeup
interrupt.

So we need to send a dummy (or any other) transaction that may fail to
wake up the controller after
a whole chromebook rebook and on the resume path.

Best regards,
Lukasz

Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

[Kenny Levinsen]

On 4/23/24 1:32 PM, Łukasz Majczak wrote:
> Unfortunately, your fix doesn't help in our case as there is no sleep
> before the second call to
> i2c_hid_fetch_hid_descriptor().

Yeah, I checked with a logic analyzer and only see ~50µs delay from the 
I2C timeout, and 50 is according to my quick math less than the 400 you 
mention is the requirement. That means that the current resume path also 
lacked a sleep, as it tried power commands in immediate succession.

I have made a v2 with your sleeps added, and added you as Co-developed-by.

Link: https://lore.kernel.org/all/20240423122518.34811-1-kl@kl.wtf/

> Saying more, this STM exposes two i2c addresses (connected physically
> to the same bus)
> one is the HID interface and the other one gives an access to the base
> firmware and is
> served by cros_ec_i2c driver and actually thanks to this driver,
> touchpad is woken up because
> In the resume path cros_ec_i2c "starts talking" through the i2c bus
> thus generating a wakeup
> interrupt.

Ah, that explains why you did not find an issue with the resume path. In 
the patch-series I sent, the boot (hid descriptor fetch) and resume 
(power on) wake-up paths are fully aligned so neither have to rely on 
such "adjacent drivers" waking up the i2c-hid device.