| Message ID | 20220525074757.7519-4-michael.zaidman@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: ft260: fixes and performance improvements | expand |
Le mer. 25 mai 2022 à 03:48, Michael Zaidman <michael.zaidman@gmail.com> a écrit : > > To support longer than one HID report size write, the driver splits a single > i2c message data payload into multiple i2c messages of HID report size. > However, it does not replicate the offset bytes within the EEPROM chip in > every consequent HID report because it is not and should not be aware of > the EEPROM type. It breaks the i2c write message integrity and causes the > EEPROM device not to acknowledge the second HID report keeping the i2c bus > busy until the ft260 controller reports failure. > I tested this whole patchset and it resolves the issue I raised https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/, thanks. > This patch preserves the i2c write message integrity by manipulating the > i2c flag bits across multiple HID reports to be seen by the EEPROM device > as a single i2c write transfer. > > Before: > > $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S > Error: Sending messages failed: Input/output error > > [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0 > [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64 > [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100 > [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0 > [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10 > [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100 > [ +0.000241] ft260_i2c_reset: done > [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5 > > After: > > $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S > > Fill block with increment via i2ctransfer by chunks > ------------------------------------------------------------------- > data rate(bps) efficiency(%) data size(B) total IOs IO size(B) > ------------------------------------------------------------------- > 58986 86 256 2 128 > > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> > --- > drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c > index 44106cadd746..bfda5b191a3a 100644 > --- a/drivers/hid/hid-ft260.c > +++ b/drivers/hid/hid-ft260.c > @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev, > } > > static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data, > - int data_len, u8 flag) > + int len, u8 flag) > { > - int len, ret, idx = 0; > + int ret, wr_len, idx = 0; > + bool first = true; > struct hid_device *hdev = dev->hdev; > struct ft260_i2c_write_request_report *rep = > (struct ft260_i2c_write_request_report *)dev->write_buf; > > do { > - if (data_len <= FT260_WR_DATA_MAX) > - len = data_len; > - else > - len = FT260_WR_DATA_MAX; > + rep->flag = 0; > + if (first) { > + rep->flag = FT260_FLAG_START; I feel like multi packet transactions must still honor flag sent to ft20_i2c_write. This adds a START even if ft260_i2c_write is called with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE. > + first = false; > + } > + > + if (len <= FT260_WR_DATA_MAX) { > + wr_len = len; > + if (flag == FT260_FLAG_START_STOP) > + rep->flag |= FT260_FLAG_STOP; > + } else { > + wr_len = FT260_WR_DATA_MAX; > + } > > - rep->report = FT260_I2C_DATA_REPORT_ID(len); > + rep->report = FT260_I2C_DATA_REPORT_ID(wr_len); > rep->address = addr; > - rep->length = len; > - rep->flag = flag; > + rep->length = wr_len; > > - memcpy(rep->data, &data[idx], len); > + memcpy(rep->data, &data[idx], wr_len); > > - ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n", > - rep->report, addr, idx, len, data[0]); > + ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n", > + rep->report, addr, idx, len, wr_len, > + rep->flag, data[0]); > > ret = ft260_hid_output_report_check_status(dev, (u8 *)rep, > - len + 4); > + wr_len + 4); > if (ret < 0) { > - hid_err(hdev, "%s: failed to start transfer, ret %d\n", > - __func__, ret); > + hid_err(hdev, "%s: failed with %d\n", __func__, ret); > return ret; > } > > - data_len -= len; > - idx += len; > + len -= wr_len; > + idx += wr_len; > > - } while (data_len > 0); > + } while (len > 0); > > return 0; > } > -- > 2.25.1 >
On Wed, May 25, 2022 at 11:44:09AM -0400, Guillaume Champagne wrote: > Le mer. 25 mai 2022 à 03:48, Michael Zaidman > <michael.zaidman@gmail.com> a écrit : > > > > To support longer than one HID report size write, the driver splits a single > > i2c message data payload into multiple i2c messages of HID report size. > > However, it does not replicate the offset bytes within the EEPROM chip in > > every consequent HID report because it is not and should not be aware of > > the EEPROM type. It breaks the i2c write message integrity and causes the > > EEPROM device not to acknowledge the second HID report keeping the i2c bus > > busy until the ft260 controller reports failure. > > > > I tested this whole patchset and it resolves the issue I raised > https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/, > thanks. Much appreciated! I will add your tested-by in the second version of the patchset. > > > This patch preserves the i2c write message integrity by manipulating the > > i2c flag bits across multiple HID reports to be seen by the EEPROM device > > as a single i2c write transfer. > > > > Before: > > > > $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S > > Error: Sending messages failed: Input/output error > > > > [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0 > > [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64 > > [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100 > > [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0 > > [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10 > > [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100 > > [ +0.000241] ft260_i2c_reset: done > > [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5 > > > > After: > > > > $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S > > > > Fill block with increment via i2ctransfer by chunks > > ------------------------------------------------------------------- > > data rate(bps) efficiency(%) data size(B) total IOs IO size(B) > > ------------------------------------------------------------------- > > 58986 86 256 2 128 > > > > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> > > --- > > drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++----------------- > > 1 file changed, 27 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c > > index 44106cadd746..bfda5b191a3a 100644 > > --- a/drivers/hid/hid-ft260.c > > +++ b/drivers/hid/hid-ft260.c > > @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev, > > } > > > > static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data, > > - int data_len, u8 flag) > > + int len, u8 flag) > > { > > - int len, ret, idx = 0; > > + int ret, wr_len, idx = 0; > > + bool first = true; > > struct hid_device *hdev = dev->hdev; > > struct ft260_i2c_write_request_report *rep = > > (struct ft260_i2c_write_request_report *)dev->write_buf; > > > > do { > > - if (data_len <= FT260_WR_DATA_MAX) > > - len = data_len; > > - else > > - len = FT260_WR_DATA_MAX; > > + rep->flag = 0; > > + if (first) { > > + rep->flag = FT260_FLAG_START; > > I feel like multi packet transactions must still honor flag sent to > ft20_i2c_write. This adds a START even if ft260_i2c_write is called > with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE. We use the FT260_FLAG_START_REPEATED to precede the Read message following the Write message in the i2c combined transaction. Am I missing any i2c protocol case using the Repeated Start in the Write path? The FT260_FLAG_NONE should not be passed into the ft20_i2c_write as well. So, we can keep it simple.
Le mer. 25 mai 2022 à 15:42, Michael Zaidman <michael.zaidman@gmail.com> a écrit : > > On Wed, May 25, 2022 at 11:44:09AM -0400, Guillaume Champagne wrote: > > Le mer. 25 mai 2022 à 03:48, Michael Zaidman > > <michael.zaidman@gmail.com> a écrit : > > > > > > To support longer than one HID report size write, the driver splits a single > > > i2c message data payload into multiple i2c messages of HID report size. > > > However, it does not replicate the offset bytes within the EEPROM chip in > > > every consequent HID report because it is not and should not be aware of > > > the EEPROM type. It breaks the i2c write message integrity and causes the > > > EEPROM device not to acknowledge the second HID report keeping the i2c bus > > > busy until the ft260 controller reports failure. > > > > > > > I tested this whole patchset and it resolves the issue I raised > > https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/, > > thanks. > > Much appreciated! > I will add your tested-by in the second version of the patchset. > > > > > > This patch preserves the i2c write message integrity by manipulating the > > > i2c flag bits across multiple HID reports to be seen by the EEPROM device > > > as a single i2c write transfer. > > > > > > Before: > > > > > > $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S > > > Error: Sending messages failed: Input/output error > > > > > > [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0 > > > [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64 > > > [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100 > > > [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0 > > > [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10 > > > [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100 > > > [ +0.000241] ft260_i2c_reset: done > > > [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5 > > > > > > After: > > > > > > $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S > > > > > > Fill block with increment via i2ctransfer by chunks > > > ------------------------------------------------------------------- > > > data rate(bps) efficiency(%) data size(B) total IOs IO size(B) > > > ------------------------------------------------------------------- > > > 58986 86 256 2 128 > > > > > > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> > > > --- > > > drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++----------------- > > > 1 file changed, 27 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c > > > index 44106cadd746..bfda5b191a3a 100644 > > > --- a/drivers/hid/hid-ft260.c > > > +++ b/drivers/hid/hid-ft260.c > > > @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev, > > > } > > > > > > static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data, > > > - int data_len, u8 flag) > > > + int len, u8 flag) > > > { > > > - int len, ret, idx = 0; > > > + int ret, wr_len, idx = 0; > > > + bool first = true; > > > struct hid_device *hdev = dev->hdev; > > > struct ft260_i2c_write_request_report *rep = > > > (struct ft260_i2c_write_request_report *)dev->write_buf; > > > > > > do { > > > - if (data_len <= FT260_WR_DATA_MAX) > > > - len = data_len; > > > - else > > > - len = FT260_WR_DATA_MAX; > > > + rep->flag = 0; > > > + if (first) { > > > + rep->flag = FT260_FLAG_START; > > > > I feel like multi packet transactions must still honor flag sent to > > ft20_i2c_write. This adds a START even if ft260_i2c_write is called > > with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE. > > We use the FT260_FLAG_START_REPEATED to precede the Read message following > the Write message in the i2c combined transaction. Am I missing any i2c > protocol case using the Repeated Start in the Write path? > None that I know of. My point was that software wise it may be less surprising to the programmer if "flag" is replicated as is when calling ft260_i2c_write. For example, calling it with FT260_FLAG_STOP only sends a START, no STOP. I agree that it isn't currently called that way and that it may never be. > The FT260_FLAG_NONE should not be passed into the ft20_i2c_write as well. > > So, we can keep it simple. Agreed. >
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c index 44106cadd746..bfda5b191a3a 100644 --- a/drivers/hid/hid-ft260.c +++ b/drivers/hid/hid-ft260.c @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev, } static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data, - int data_len, u8 flag) + int len, u8 flag) { - int len, ret, idx = 0; + int ret, wr_len, idx = 0; + bool first = true; struct hid_device *hdev = dev->hdev; struct ft260_i2c_write_request_report *rep = (struct ft260_i2c_write_request_report *)dev->write_buf; do { - if (data_len <= FT260_WR_DATA_MAX) - len = data_len; - else - len = FT260_WR_DATA_MAX; + rep->flag = 0; + if (first) { + rep->flag = FT260_FLAG_START; + first = false; + } + + if (len <= FT260_WR_DATA_MAX) { + wr_len = len; + if (flag == FT260_FLAG_START_STOP) + rep->flag |= FT260_FLAG_STOP; + } else { + wr_len = FT260_WR_DATA_MAX; + } - rep->report = FT260_I2C_DATA_REPORT_ID(len); + rep->report = FT260_I2C_DATA_REPORT_ID(wr_len); rep->address = addr; - rep->length = len; - rep->flag = flag; + rep->length = wr_len; - memcpy(rep->data, &data[idx], len); + memcpy(rep->data, &data[idx], wr_len); - ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n", - rep->report, addr, idx, len, data[0]); + ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n", + rep->report, addr, idx, len, wr_len, + rep->flag, data[0]); ret = ft260_hid_output_report_check_status(dev, (u8 *)rep, - len + 4); + wr_len + 4); if (ret < 0) { - hid_err(hdev, "%s: failed to start transfer, ret %d\n", - __func__, ret); + hid_err(hdev, "%s: failed with %d\n", __func__, ret); return ret; } - data_len -= len; - idx += len; + len -= wr_len; + idx += wr_len; - } while (data_len > 0); + } while (len > 0); return 0; }
To support longer than one HID report size write, the driver splits a single i2c message data payload into multiple i2c messages of HID report size. However, it does not replicate the offset bytes within the EEPROM chip in every consequent HID report because it is not and should not be aware of the EEPROM type. It breaks the i2c write message integrity and causes the EEPROM device not to acknowledge the second HID report keeping the i2c bus busy until the ft260 controller reports failure. This patch preserves the i2c write message integrity by manipulating the i2c flag bits across multiple HID reports to be seen by the EEPROM device as a single i2c write transfer. Before: $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S Error: Sending messages failed: Input/output error [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0 [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64 [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100 [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0 [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10 [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100 [ +0.000241] ft260_i2c_reset: done [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5 After: $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S Fill block with increment via i2ctransfer by chunks ------------------------------------------------------------------- data rate(bps) efficiency(%) data size(B) total IOs IO size(B) ------------------------------------------------------------------- 58986 86 256 2 128 Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com> --- drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 18 deletions(-)