Re: [PATCH v2] i2c: imx: honour I2C_M_IGNORE_NAK

[Carlos Song <carlos.song@oss.nxp.com>]

Hi, Haobin

Are you using AI to make this patch?
You can refer the Linux doc:Documentation/process/generated-content.rst.

Please adjust all commit log also code comment,there are two spaces after every period.
Before V3, I have some questions about this patch.

> The i.MX controller has no hardware "ignore NAK" enable bit and checks
> each byte in software after transmitting it: i2c_imx_acked() on the
> atomic paths, i2c_imx_isr_acked() on the ISR path.  Any NAK is
> therefore treated as a fatal error and aborts the transfer with -ENXIO.
>
> There are legitimate cases where a target NAKs on purpose and the
> transfer must continue anyway.  The standard way to express this is
> I2C_M_IGNORE_NAK, which the core passes through to the bus driver
> unchanged: i2c_transfer_buffer_flags() performs no validation of the
> message flags, and the i.MX driver has never advertised
> I2C_FUNC_PROTOCOL_MANGLING.  Today such a transfer transmits bytes
> until the first NAK and then fails with -ENXIO, silently dropping the
> flag.  One in-tree caller that hits this on i.MX buses is the wake
> sequence of the Atmel crypto drivers (drivers/crypto/atmel-i2c.c, used
> by atmel-ecc.c for the ATECC508A/608A): the sleeping device NAKs the
> wake token and the write must complete anyway; the ATECC508A is wired
> to an i.MX I2C controller e.g. on the Gateworks GW5904.

Have you meet one issue on one real platform? Can you share the detail about your
devices and platform?

> Advertise I2C_FUNC_PROTOCOL_MANGLING and honour I2C_M_IGNORE_NAK at
> every software ACK check: on the PIO write paths, which check the ACK
> of the address and of every data byte, and on the PIO read paths, which
> check the target's ACK of the address byte, mirroring i2c-img-scb.
> With the flag set a NAK no longer aborts the transfer and the byte loop
> continues so the remaining bytes are still transmitted.  Every transfer
> without the flag keeps the previous behaviour unchanged.
>
> A NAK'd byte does not generate a further DMA request, so a DMA write to
> a deliberately-NAKing target would stall until DMA_TIMEOUT instead of
> completing.  Force the PIO path for every message carrying
> I2C_M_IGNORE_NAK, again mirroring i2c-img-scb, which implements the
> flag in atomic mode only.  The DMA write path therefore never sees the
> flag, but it passes the flag through at the final ACK check like the
> other paths.
>
> I2C_FUNC_PROTOCOL_MANGLING is a blanket capability bit, so advertising
> it while implementing only I2C_M_IGNORE_NAK would silently ignore the
> other mangling flags.  Reject I2C_M_NO_RD_ACK and I2C_M_REV_DIR_ADDR in
> xfer_common() instead; no in-tree user of these flags transfers through
> i2c-imx.  I2C_M_NO_RD_ACK is not implementable at all on this
> controller: it always clocks the ninth acknowledge bit and I2CR[TXAK]
> only selects ACK versus NAK.  I2C_M_STOP is deliberately not rejected:
> the controller already terminates every transfer with a STOP and
> separates messages with repeated STARTs, so leaving the flag unhandled
> changes nothing on the bus, while rejecting it would break the ov2659
> camera on imx6qdl-ds: I2C_CLIENT_SCCB is I2C_M_STOP | I2C_M_IGNORE_NAK
> and the driver stamps client->flags on every message, including the
> chip-ID reads done at probe time.

You don't really support theI2C_M_STOP in the code and intentionallyignore I2C_M_STOP check to avoid breaking the imx6qdl-ds,right? Do you work on this platform?

> No retry is introduced: the meaning of a NAK is device-specific [1], so
> it stays a device-driver decision.  I2C_M_IGNORE_NAK is only honoured
> when the device driver explicitly sets it, which is exactly how the
> flag is meant to be used.
>
> Link: https://patchwork.ozlabs.org/project/linux-i2c/patch/1467900229-5262-1-git-send-email-tharvey@gateworks.com/ # [1]
> Link: https://lore.kernel.org/linux-i2c/4D46D571.5010907@armadeus.com/ # [2]
> Link: https://patchwork.ozlabs.org/project/linux-i2c/patch/1378857490-30968-1-git-send-email-luka@openwrt.org/ # [3]
> Signed-off-by: Haobin Jiang <lemonoutput@foxmail.com>

Because there is a similar patch[4] in upstream thread. It is to add I2C_M_REV_DIR_ADDR and
I2C_M_NOSTART flags. So your patch will confict with this patch[4].

So can you please rebase your patch on patch[4]?

[4] https://lore.kernel.org/linux-i2c/apcYp0Zc7CrJMiOh@lizhi-Precision-Tower-5810/T/#t

>
> ---
> v1 -> v2:
>          - Honour I2C_M_IGNORE_NAK on the read path as well, at the checks
>            of the target's ACK of the address byte (i2c_imx_isr_read(),
>            i2c_imx_prepare_read()), mirroring i2c-img-scb, instead of
>            silently dropping the flag there.  Read requests carrying the
>            flag cannot be rejected, because in-tree SCCB sensors such as
>            ov2659 set I2C_CLIENT_SCCB, i.e. I2C_M_STOP |
>            I2C_M_IGNORE_NAK, on their read messages too.
>          - Force the PIO path for every message with I2C_M_IGNORE_NAK: a
>            NAK'd byte does not generate a further DMA request, so a DMA
>            write to a deliberately-NAKing target would stall until
>            DMA_TIMEOUT (mirrors i2c-img-scb).  i2c_imx_dma_write() can
>            therefore never see the flag, though it still passes it
>            through at the final ACK check like the other paths.
>          - Reject I2C_M_NO_RD_ACK and I2C_M_REV_DIR_ADDR in
>            xfer_common() instead of advertising blanket support and
>            silently ignoring them.  I2C_M_STOP is not rejected: the
>            controller already emits a STOP after the final message and
>            repeated STARTs between messages, and rejecting it would break
>            the ov2659 camera on imx6qdl-ds.
>          - Correct the description of the status quo: the core does not
>            reject I2C_M_IGNORE_NAK; i2c_transfer_buffer_flags() passes the
>            message flags through unchanged, so the current driver sends
>            bytes until the first NAK and then fails with -ENXIO.
> ---
>   drivers/i2c/busses/i2c-imx.c | 44 ++++++++++++++++++++++++++++--------
>   1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37..5b22a497a791 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -612,9 +612,12 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic)
>          return 0;
>   }
>
> -static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
> +static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx, bool ignore_nak)
>   {
>          if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
> +               if (ignore_nak)
> +                       return 0;
> +
>                  dev_dbg(&i2c_imx->adapter.dev, "<%s> No ACK\n", __func__);
>                  return -ENXIO;  /* No ACK */
>          }
> @@ -968,11 +971,15 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
>          return ret;
>   }

After meet NACK, can i2c-imx controller continue to write or read data from FIFO,
after NACK normally stop or repeat start, so I am not sure the i2c-imx controller HW
will work normally, do you test this?

> -static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx)
> +static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx,
> +                                   bool ignore_nak)
>   {
>          i2c_imx->isr_result = 0;
>
>          if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
> +               if (ignore_nak)
> +                       return 0;
> +
>                  i2c_imx->state = IMX_I2C_STATE_FAILED;
>                  i2c_imx->isr_result = -ENXIO;
>                  wake_up(&i2c_imx->queue);
> @@ -985,7 +992,7 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
>   {
>          int result;
>
> -       result = i2c_imx_isr_acked(i2c_imx);
> +       result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>
> @@ -1002,7 +1009,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
>          int result;
>          unsigned int temp;
>
> -       result = i2c_imx_isr_acked(i2c_imx);
> +       result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>
> @@ -1213,7 +1220,8 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>          if (result)
>                  return result;
>
> -       return i2c_imx_acked(i2c_imx);

Always goto the PIO path with I2C_M_IGNORE_NAK flag,
it is better "return i2c_imx_acked(i2c_imx, false);"?

> +       /* I2C_M_IGNORE_NAK messages always take the PIO path. */
> +       return i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>   }
>
>   static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
> @@ -1227,7 +1235,7 @@ static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
>          result = i2c_imx_trx_complete(i2c_imx, !use_dma);
>          if (result)
>                  return result;
> -       result = i2c_imx_acked(i2c_imx);
> +       result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>
> @@ -1358,7 +1366,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
>          result = i2c_imx_trx_complete(i2c_imx, true);
>          if (result)
>                  return result;
> -       result = i2c_imx_acked(i2c_imx);
> +       result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>          if (result)
>                  return result;
>          dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
> @@ -1372,7 +1380,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
>                  result = i2c_imx_trx_complete(i2c_imx, true);
>                  if (result)
>                          return result;
> -               result = i2c_imx_acked(i2c_imx);
> +               result = i2c_imx_acked(i2c_imx, msgs->flags & I2C_M_IGNORE_NAK);
>                  if (result)
>                          return result;
>          }
> @@ -1549,6 +1557,16 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>          bool is_lastmsg = false;
>          struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
>          int use_dma = 0;

This code can be simple if rebase on patch[4] I think.

Regards,
Carlos

> +       u16 mangling = I2C_M_NO_RD_ACK | I2C_M_REV_DIR_ADDR;
> +
> +       /*
> +        * Only I2C_M_IGNORE_NAK is implemented; reject the unsupported
> +        * mangling flags.  I2C_M_STOP is the controller's native
> +        * behaviour and must not be rejected.
> +        */
> +       for (i = 0; i < num; i++)
> +               if (msgs[i].flags & mangling)
> +                       return -EOPNOTSUPP;
>
>          /* Start I2C transfer */
>          result = i2c_imx_start(i2c_imx, atomic);
> @@ -1604,6 +1622,14 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>
>                  use_dma = i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD &&
>                          msgs[i].flags & I2C_M_DMA_SAFE;
> +
> +               /*
> +                * I2C_M_IGNORE_NAK is honoured on the PIO paths only; a
> +                * NAK'd byte does not generate a further DMA request
> +                * (mirrors i2c-img-scb).
> +                */
> +               if (msgs[i].flags & I2C_M_IGNORE_NAK)
> +                       use_dma = false;
>                  if (msgs[i].flags & I2C_M_RD) {
>                          int block_data = msgs->flags & I2C_M_RECV_LEN;
>
> @@ -1697,7 +1723,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>   static u32 i2c_imx_func(struct i2c_adapter *adapter)
>   {
>          return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> -               | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> +               | I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_PROTOCOL_MANGLING;
>   }
>
>   static const struct i2c_algorithm i2c_imx_algo = {
>
> base-commit: 04e9bf1648f846976b543e91c1838a712433772a
> --
> 2.34.1

-- 
Best Regards,
Carlos