Hi Sir,
On 10/7/20 9:08 PM, jarkko.nikula@xxxxxxxxxxxxxxx wrote:
> Hi
>
> Unfortunately I don't have right now a setup to try myself but could you
> try these ideas to read and clear interrupt status in one place only and
> move I2C_SLAVE_WRITE_REQUESTED reporting after
> I2C_SLAVE_WRITE_RECEIVED
> like in a patch below?
>
> diff --git a/drivers/i2c/busses/i2c-designware-slave.c
> b/drivers/i2c/busses/i2c-designware-slave.c
> index 44974b53a626..97131e888e24 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -159,7 +159,6 @@ static int i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev)
> u32 raw_stat, stat, enabled, tmp;
> u8 val = 0, slave_activity;
>
> - regmap_read(dev->map, DW_IC_INTR_STAT, &stat);
> regmap_read(dev->map, DW_IC_ENABLE, &enabled);
> regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_stat);
> regmap_read(dev->map, DW_IC_STATUS, &tmp);
> @@ -168,13 +167,11 @@ static int i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev)
> if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY) || !dev->slave)
> return 0;
>
> + stat = i2c_dw_read_clear_intrbits_slave(dev);
> dev_dbg(dev->dev,
> "%#x STATUS SLAVE_ACTIVITY=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n",
> enabled, slave_activity, raw_stat, stat);
>
> - if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
> - i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
> -
> if (stat & DW_IC_INTR_RD_REQ) {
> if (slave_activity) {
> if (stat & DW_IC_INTR_RX_FULL) {
> @@ -188,11 +185,9 @@ static int i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev)
> val);
> }
> regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
> - stat = i2c_dw_read_clear_intrbits_slave(dev);
> } else {
> regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
> regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &tmp);
> - stat = i2c_dw_read_clear_intrbits_slave(dev);
> }
> if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_REQUESTED,
> @@ -207,7 +202,6 @@ static int i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev)
> regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
>
> i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> - stat = i2c_dw_read_clear_intrbits_slave(dev);
> return 1;
> }
>
> @@ -219,9 +213,11 @@ static int i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev)
> dev_vdbg(dev->dev, "Byte %X acked!", val);
> } else {
> i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> - stat = i2c_dw_read_clear_intrbits_slave(dev);
> }
>
> + if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
> + i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
> +
> return 1;
> }
>
> @@ -230,7 +226,6 @@ static irqreturn_t i2c_dw_isr_slave(int this_irq,
> void *dev_id)
> struct dw_i2c_dev *dev = dev_id;
> int ret;
>
> - i2c_dw_read_clear_intrbits_slave(dev);
> ret = i2c_dw_irq_handler_slave(dev);
> if (ret > 0)
> complete(&dev->cmd_complete);
After adding your suggestion, it seems work fine with drivers/i2c/i2c-slave-eeprom.c.
The log was usually:
# i2cset -f -y 2 0x42 0x00 0x41;dmesg -c
0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x710 : INTR_STAT=0x200
STOP
Sometimes it wound be:
# i2cset -f -y 2 0x42 0x08 0x45;dmesg -c
0x1 STATUS SLAVE_ACTIVITY=0x1 : RAW_INTR_STAT=0x514 : INTR_STAT=0x4
WRITE_RECEIVED
0x1 STATUS SLAVE_ACTIVITY=0x0 : RAW_INTR_STAT=0x714 : INTR_STAT=0x204
WRITE_RECEIVED
WRITE_REQUESTED
drivers/i2c/i2c-slave-eeprom.c does the same action when triggered by either I2C_SLAVE_STOP or I2C_SLAVE_WRITE_REQUESTED, so one of them being the last event are fine.
But Documentation/i2c/slave-interface.rst said that I2C_SLAVE_WRITE_REQUESTED comes without any data arrived. It means I2C_SLAVE_WRITE_REQUESTED should be front of any WRITE_RECEIVED in a write-request. The second log should be an illegal result.
I2C_SLAVE_WRITE_REQUESTED is made when RX_FULL and STOP_DET are both rising. What situation meet this condition? I guess this STOP_DET was left by previous transmission and then the system is finally handling it while data of the next request arrived. If yes, put "if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))" in front of other handling is correct but it violates I2C_SLAVE_WRITE_REQUESTED dentition when these two interrupts of one request have to handle in one ISR. I think there have to be a solution to distinguish these two cases.
Note that my Linux is 4.9.170 and back-ported drivers/i2c/i2c-designware-slave.c from 4.13 (commit 9f3e065c54b05b03bd39dbbcc5a44f2f1807994d).
--
BR,
Michael Wu
