| Message ID | 20240405102436.3479210-1-lma@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition | expand |
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
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
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
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
> 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
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
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
> 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
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 >
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 > >
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
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
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
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/
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
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.
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;
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(-)