| Message ID | f56773092681736140447f47962aa2f6c3df3773.1704440251.git.hanshu-oc@zhaoxin.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: add zhaoxin i2c controller driver | expand |
Hi, On Fri, Jan 05, 2024 at 03:51:47PM +0800, Hans Hu wrote: > v6->v7: > 1. some dirty patches were removed > 2. rename structure member 'to/ti' to 't1/t2' > to make it easier to understand. > 3. add a comment about arbitration. > Link: https://lore.kernel.org/all/b0f284621b6763c32133d39be83f05f1184b3635.1703830854.git.hanshu-oc@zhaoxin.com/ > > During each byte access, the host performs clock stretching. > In this case, the thread may be interrupted by preemption, > resulting in a long stretching time. > > However, some touchpad can only tolerate host clock stretching > of no more than 200 ms. We reduce the impact of this through > a retransmission mechanism. > > Since __i2c_lock_bus_helper() is used to ensure that the > current access will not be interrupted by the other access, > We don't need to worry about arbitration anymore. > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> Uh oh, NACK. We shouldn't limit clock stretching because some touchpad controllers can't handle it. The first thing I suggest is to move more handling to the interrupt context, like filling the next byte after the previous has been processed. Then, you are not interruptible anymore. If this all fails, we need to determine a bus specific property, but I am quite sure the above conversion will be enough. Maybe it is an idea to first get the driver converted to support your platform, and afterwards the conversion to more handling in interrupt. Kind regards, Wolfram
Hi Wolfram, On 2024/2/21 20:37, Wolfram Sang wrote: > Hi, > > On Fri, Jan 05, 2024 at 03:51:47PM +0800, Hans Hu wrote: >> v6->v7: >> 1. some dirty patches were removed >> 2. rename structure member 'to/ti' to 't1/t2' >> to make it easier to understand. >> 3. add a comment about arbitration. >> Link: https://lore.kernel.org/all/b0f284621b6763c32133d39be83f05f1184b3635.1703830854.git.hanshu-oc@zhaoxin.com/ >> >> During each byte access, the host performs clock stretching. >> In this case, the thread may be interrupted by preemption, >> resulting in a long stretching time. >> >> However, some touchpad can only tolerate host clock stretching >> of no more than 200 ms. We reduce the impact of this through >> a retransmission mechanism. >> >> Since __i2c_lock_bus_helper() is used to ensure that the >> current access will not be interrupted by the other access, >> We don't need to worry about arbitration anymore. >> >> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> >> Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> > Uh oh, NACK. We shouldn't limit clock stretching because some touchpad > controllers can't handle it. > > The first thing I suggest is to move more handling to the interrupt > context, like filling the next byte after the previous has been > processed. Then, you are not interruptible anymore. > > If this all fails, we need to determine a bus specific property, but I > am quite sure the above conversion will be enough. > > Maybe it is an idea to first get the driver converted to support your > platform, and afterwards the conversion to more handling in interrupt. > > Kind regards, > > Wolfram > Thanks for your suggestion, the purpose of this approach is to reduce the clock stretching caused by the system. Therefore, I try to put almost all of the processing in the interrupt context. Hans, Thank you
>> Thanks for your suggestion, the purpose of this approach is to >> reduce the clock stretching caused by the system. Therefore, >> I try to put almost all of the processing in the interrupt context. > Well, I think per-msg handling in interrupt context is enough. The > transfer (consisting of multiple messages) handling is usually best left > in process context. > OK, I understand now. Hans, Thank you
On Thu, Feb 22, 2024 at 06:42:20PM +0800, Hans Hu wrote: > > > > Thanks for your suggestion, the purpose of this approach is to > > > reduce the clock stretching caused by the system. Therefore, > > > I try to put almost all of the processing in the interrupt context. > > Well, I think per-msg handling in interrupt context is enough. The > > transfer (consisting of multiple messages) handling is usually best left > > in process context. > > > > OK, I understand now. If you need inspiration, check i2c-digicolor.c for a simple driver implementing it.
diff --git a/drivers/i2c/busses/i2c-viai2c-common.c b/drivers/i2c/busses/i2c-viai2c-common.c index 3e565d5ee4c7..0fd2554731ca 100644 --- a/drivers/i2c/busses/i2c-viai2c-common.c +++ b/drivers/i2c/busses/i2c-viai2c-common.c @@ -22,12 +22,37 @@ int viai2c_check_status(struct viai2c *i2c) { int ret = 0; unsigned long time_left; + unsigned long delta_ms; time_left = wait_for_completion_timeout(&i2c->complete, msecs_to_jiffies(500)); if (!time_left) return -ETIMEDOUT; + /* + * During each byte access, the host performs clock stretching. + * In this case, the thread may be interrupted by preemption, + * resulting in a long stretching time. + * + * However, some touchpad can only tolerate host clock stretching + * of no more than 200 ms. We reduce the impact of this through + * a retransmission mechanism. + * + * Since __i2c_lock_bus_helper() is used to ensure that the + * current access will not be interrupted by the other access, + * We don't need to worry about arbitration anymore. + */ + local_irq_disable(); + i2c->t2 = ktime_get(); + delta_ms = ktime_to_ms(ktime_sub(i2c->t2, i2c->t1)); + if (delta_ms > VIAI2C_STRETCHING_TIMEOUT) { + local_irq_enable(); + dev_warn(i2c->dev, "thread blocked more than %ldms\n", delta_ms); + return -EAGAIN; + } + i2c->t1 = i2c->t2; + local_irq_enable(); + if (i2c->cmd_status & VIAI2C_ISR_NACK_ADDR) ret = -EIO; @@ -158,6 +183,7 @@ int viai2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) int ret = 0; struct viai2c *i2c = i2c_get_adapdata(adap); + i2c->t2 = i2c->t1 = ktime_get(); for (i = 0; ret >= 0 && i < num; i++) { pmsg = &msgs[i]; if (!(pmsg->flags & I2C_M_NOSTART)) { diff --git a/drivers/i2c/busses/i2c-viai2c-common.h b/drivers/i2c/busses/i2c-viai2c-common.h index 28799e7e97f0..d0ffba22104d 100644 --- a/drivers/i2c/busses/i2c-viai2c-common.h +++ b/drivers/i2c/busses/i2c-viai2c-common.h @@ -49,6 +49,7 @@ #define VIAI2C_REG_MCR 0x0E #define VIAI2C_TIMEOUT (msecs_to_jiffies(1000)) +#define VIAI2C_STRETCHING_TIMEOUT 200 struct viai2c { struct i2c_adapter adapter; @@ -59,6 +60,8 @@ struct viai2c { u16 tcr; int irq; u16 cmd_status; + ktime_t t1; + ktime_t t2; }; int viai2c_wait_bus_not_busy(struct viai2c *i2c);