| Message ID | 20200917052256.5770-1-sultan@kerneltoast.com |
|---|---|
| Headers | show |
| Series | i2c-hid: Save power by reducing i2c xfers with block reads | expand |
On Thu, Sep 17, 2020 at 8:26 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote: > > From: Sultan Alsawaf <sultan@kerneltoast.com> > > This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c > xfers with block reads". That original patchset did not have enough fixes for > the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented > extensively in the original email thread. > > Here is the original cover letter, which still applies: > "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my > finger on the touchpad would increase my system's power consumption by 4W, which > is quite considerable. Resting my finger on the touchpad would generate roughly > 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. > > Upon closer inspection, I noticed that the i2c-hid driver would always transfer > the maximum report size over i2c (which is 60 bytes for my touchpad), but all of > my touchpad's normal touch events are only 32 bytes long according to the length > byte contained in the buffer sequence. > > Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN > flag in i2c-hid, which says to look for the payload length in the first byte of > the transfer buffer and adjust the i2c transaction accordingly. The only problem > though is that my i2c controller's driver allows bytes other than the first one > to be used to retrieve the payload length, which is incorrect according to the > SMBus spec, and would break my i2c-hid change since not *all* of the reports > from my touchpad are conforming SMBus block reads. > > This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and > modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the > peripheral controlled by i2c-hid doesn't support block reads, the i2c controller > drivers should cope with this and proceed with the i2c transfer using the > original requested length." Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> for I²C DesignWare patches. > > Sultan > > Sultan Alsawaf (4): > i2c: designware: Fix transfer failures for invalid SMBus block reads > i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads > i2c: designware: Allow SMBus block reads up to 255 bytes in length > HID: i2c-hid: Use block reads when possible to save power > > drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++- > drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) > > -- > 2.28.0 >
On Wed, 16 Sep 2020, Sultan Alsawaf wrote: > From: Sultan Alsawaf <sultan@kerneltoast.com> > > This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c > xfers with block reads". That original patchset did not have enough fixes for > the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented > extensively in the original email thread. > > Here is the original cover letter, which still applies: > "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my > finger on the touchpad would increase my system's power consumption by 4W, which > is quite considerable. Resting my finger on the touchpad would generate roughly > 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. > > Upon closer inspection, I noticed that the i2c-hid driver would always transfer > the maximum report size over i2c (which is 60 bytes for my touchpad), but all of > my touchpad's normal touch events are only 32 bytes long according to the length > byte contained in the buffer sequence. > > Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN > flag in i2c-hid, which says to look for the payload length in the first byte of > the transfer buffer and adjust the i2c transaction accordingly. The only problem > though is that my i2c controller's driver allows bytes other than the first one > to be used to retrieve the payload length, which is incorrect according to the > SMBus spec, and would break my i2c-hid change since not *all* of the reports > from my touchpad are conforming SMBus block reads. > > This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and > modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the > peripheral controlled by i2c-hid doesn't support block reads, the i2c controller > drivers should cope with this and proceed with the i2c transfer using the > original requested length." > > Sultan > > Sultan Alsawaf (4): > i2c: designware: Fix transfer failures for invalid SMBus block reads > i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads > i2c: designware: Allow SMBus block reads up to 255 bytes in length > HID: i2c-hid: Use block reads when possible to save power > > drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++- > drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) Hans, Benjamin, could you please give this patchset some smoke-testing? It looks good to me, but I'd like it to get some testing from your testing machinery before merging. Thanks,
> Hans, Benjamin, could you please give this patchset some smoke-testing? It > looks good to me, but I'd like it to get some testing from your testing > machinery before merging. Please give me some more days. I am not fully convinced yet that this use of I2C_M_RECV_LEN is not broken on some controllers. Plus, I'd favor if this could go via I2C tree. It is within I2C where the non-trivial changes are. The HID part is just the final bit. Can we agree on that?
On Tue, 22 Sep 2020, Wolfram Sang wrote: > > Hans, Benjamin, could you please give this patchset some smoke-testing? It > > looks good to me, but I'd like it to get some testing from your testing > > machinery before merging. > > Please give me some more days. I am not fully convinced yet that this > use of I2C_M_RECV_LEN is not broken on some controllers. > > Plus, I'd favor if this could go via I2C tree. It is within I2C where > the non-trivial changes are. The HID part is just the final bit. Can we > agree on that? Absolutely no problem with that. But I'd like to have this ran through Benjamin/Hans first too. Thanks,
On Tue, Sep 22, 2020 at 09:59:44PM +0200, Jiri Kosina wrote: > On Tue, 22 Sep 2020, Wolfram Sang wrote: > > > > Hans, Benjamin, could you please give this patchset some smoke-testing? It > > > looks good to me, but I'd like it to get some testing from your testing > > > machinery before merging. > > > > Please give me some more days. I am not fully convinced yet that this > > use of I2C_M_RECV_LEN is not broken on some controllers. > > > > Plus, I'd favor if this could go via I2C tree. It is within I2C where > > the non-trivial changes are. The HID part is just the final bit. Can we > > agree on that? > > Absolutely no problem with that. But I'd like to have this ran through > Benjamin/Hans first too. > > Thanks, > > -- > Jiri Kosina > SUSE Labs > I suppose the HID part does need to be held off until all the adapters are updated with functional I2C_M_RECV_LEN bits. I just got a Ryzen laptop which panics when using I2C_M_RECV_LEN. So it looks like only the designware changes can be considered for merging now. Sultan
On 9/17/20 5:02 PM, Andy Shevchenko wrote: > On Thu, Sep 17, 2020 at 8:26 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote: >> >> From: Sultan Alsawaf <sultan@kerneltoast.com> >> >> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c >> xfers with block reads". That original patchset did not have enough fixes for >> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented >> extensively in the original email thread. >> >> Here is the original cover letter, which still applies: >> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my >> finger on the touchpad would increase my system's power consumption by 4W, which >> is quite considerable. Resting my finger on the touchpad would generate roughly >> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. >> >> Upon closer inspection, I noticed that the i2c-hid driver would always transfer >> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of >> my touchpad's normal touch events are only 32 bytes long according to the length >> byte contained in the buffer sequence. >> >> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN >> flag in i2c-hid, which says to look for the payload length in the first byte of >> the transfer buffer and adjust the i2c transaction accordingly. The only problem >> though is that my i2c controller's driver allows bytes other than the first one >> to be used to retrieve the payload length, which is incorrect according to the >> SMBus spec, and would break my i2c-hid change since not *all* of the reports >> from my touchpad are conforming SMBus block reads. >> >> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and >> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the >> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller >> drivers should cope with this and proceed with the i2c transfer using the >> original requested length." > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > for I²C DesignWare patches. > Also for i2c-designware Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Hi, On 9/22/20 11:19 AM, Jiri Kosina wrote: > On Wed, 16 Sep 2020, Sultan Alsawaf wrote: > >> From: Sultan Alsawaf <sultan@kerneltoast.com> >> >> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c >> xfers with block reads". That original patchset did not have enough fixes for >> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented >> extensively in the original email thread. >> >> Here is the original cover letter, which still applies: >> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my >> finger on the touchpad would increase my system's power consumption by 4W, which >> is quite considerable. Resting my finger on the touchpad would generate roughly >> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. >> >> Upon closer inspection, I noticed that the i2c-hid driver would always transfer >> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of >> my touchpad's normal touch events are only 32 bytes long according to the length >> byte contained in the buffer sequence. >> >> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN >> flag in i2c-hid, which says to look for the payload length in the first byte of >> the transfer buffer and adjust the i2c transaction accordingly. The only problem >> though is that my i2c controller's driver allows bytes other than the first one >> to be used to retrieve the payload length, which is incorrect according to the >> SMBus spec, and would break my i2c-hid change since not *all* of the reports >> from my touchpad are conforming SMBus block reads. >> >> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and >> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the >> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller >> drivers should cope with this and proceed with the i2c transfer using the >> original requested length." >> >> Sultan >> >> Sultan Alsawaf (4): >> i2c: designware: Fix transfer failures for invalid SMBus block reads >> i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads >> i2c: designware: Allow SMBus block reads up to 255 bytes in length >> HID: i2c-hid: Use block reads when possible to save power >> >> drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++- >> drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------ >> 2 files changed, 13 insertions(+), 7 deletions(-) > > Hans, Benjamin, could you please give this patchset some smoke-testing? It > looks good to me, but I'd like it to get some testing from your testing > machinery before merging. Sorry for being slow to respond to this. I have not gotten around to testing this, but I saw another email that this breaks things on at least AMD platforms, so I guess that this is on hold for now ? Regards, Hans
On Fri, Oct 16, 2020 at 01:16:18PM +0200, Hans de Goede wrote: > Hi, > > On 9/22/20 11:19 AM, Jiri Kosina wrote: > > On Wed, 16 Sep 2020, Sultan Alsawaf wrote: > > > >> From: Sultan Alsawaf <sultan@kerneltoast.com> > >> > >> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c > >> xfers with block reads". That original patchset did not have enough fixes for > >> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented > >> extensively in the original email thread. > >> > >> Here is the original cover letter, which still applies: > >> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my > >> finger on the touchpad would increase my system's power consumption by 4W, which > >> is quite considerable. Resting my finger on the touchpad would generate roughly > >> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. > >> > >> Upon closer inspection, I noticed that the i2c-hid driver would always transfer > >> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of > >> my touchpad's normal touch events are only 32 bytes long according to the length > >> byte contained in the buffer sequence. > >> > >> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN > >> flag in i2c-hid, which says to look for the payload length in the first byte of > >> the transfer buffer and adjust the i2c transaction accordingly. The only problem > >> though is that my i2c controller's driver allows bytes other than the first one > >> to be used to retrieve the payload length, which is incorrect according to the > >> SMBus spec, and would break my i2c-hid change since not *all* of the reports > >> from my touchpad are conforming SMBus block reads. > >> > >> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and > >> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the > >> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller > >> drivers should cope with this and proceed with the i2c transfer using the > >> original requested length." > >> > >> Sultan > >> > >> Sultan Alsawaf (4): > >> i2c: designware: Fix transfer failures for invalid SMBus block reads > >> i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads > >> i2c: designware: Allow SMBus block reads up to 255 bytes in length > >> HID: i2c-hid: Use block reads when possible to save power > >> > >> drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++- > >> drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------ > >> 2 files changed, 13 insertions(+), 7 deletions(-) > > > > Hans, Benjamin, could you please give this patchset some smoke-testing? It > > looks good to me, but I'd like it to get some testing from your testing > > machinery before merging. > > Sorry for being slow to respond to this. I have not gotten around to testing > this, but I saw another email that this breaks things on at least AMD > platforms, so I guess that this is on hold for now ? > > Regards, > > Hans Hi, Only the i2c-hid-core.c hunk is on hold. It is on hold until every i2c adapter gets updated for proper block read functionality up to the new block read limit of 255 bytes. This is not really a surprise. The designware patches are good to go. Please let me know what you think of them. Sultan
From: Sultan Alsawaf <sultan@kerneltoast.com> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c xfers with block reads". That original patchset did not have enough fixes for the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented extensively in the original email thread. Here is the original cover letter, which still applies: "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my finger on the touchpad would increase my system's power consumption by 4W, which is quite considerable. Resting my finger on the touchpad would generate roughly 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq. Upon closer inspection, I noticed that the i2c-hid driver would always transfer the maximum report size over i2c (which is 60 bytes for my touchpad), but all of my touchpad's normal touch events are only 32 bytes long according to the length byte contained in the buffer sequence. Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN flag in i2c-hid, which says to look for the payload length in the first byte of the transfer buffer and adjust the i2c transaction accordingly. The only problem though is that my i2c controller's driver allows bytes other than the first one to be used to retrieve the payload length, which is incorrect according to the SMBus spec, and would break my i2c-hid change since not *all* of the reports from my touchpad are conforming SMBus block reads. This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the peripheral controlled by i2c-hid doesn't support block reads, the i2c controller drivers should cope with this and proceed with the i2c transfer using the original requested length." Sultan Sultan Alsawaf (4): i2c: designware: Fix transfer failures for invalid SMBus block reads i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads i2c: designware: Allow SMBus block reads up to 255 bytes in length HID: i2c-hid: Use block reads when possible to save power drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++- drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-)