Re: [PATCH v2] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Mukesh Kumar Savaliya]

On 3/7/2024 3:23 PM, Dmitry Baryshkov wrote:
> On Thu, 7 Mar 2024 at 11:36, Mukesh Kumar Savaliya
> <quic_msavaliy@xxxxxxxxxxx> wrote:
> > We are seeing transfer failure instead of NACK error for simple
> > device scan test.Ideally it should report exact error like NACK
> > if device is not present.
> >
> > We may also expect errors like BUS_PROTO or ARB_LOST, hence we are
> > adding such error support in GSI mode and reporting it accordingly
> > by adding respective error logs.
>
> Please take a look at the
> Documentation/process/submitting-patches.rst. This is not the expected
> style of commit messages.

Thanks Dmitry ! Gone through the link and tried to align to the 
guidance. I will be adding into the actual upload in V3.

When we run scan test for i2c devices, we see transfer failures instead
of NACK. This is wrong because there is no data transfer failure but 
it's a slave response to the i2c master controller.

This change correctly identifies NACK error. Also adds support for other
protocol errors like BUS_PROTO and ARB_LOST. This helps to exactly know
the response on the bus.

Function geni_i2c_gpi_xfer() gets called for any i2c GSI mode transfer
and waits for the response as success OR failure. If slave is not 
present OR NACKing, GSI generates an error interrupt which calls ISR and 
it further calls gpi_process_xfer_compl_event(). Now 
dmaengine_desc_callback_invoke() will call i2c_gpi_cb_result() where we 
have added parsing status parameters to identify respective errors.

> > During geni_i2c_gpi_xfer(), we should expect callback param as a
> > transfer result. For that we have added a new structure named
> > gpi_i2c_result, which will store xfer result.
> >
> > Upon receiving an interrupt, gpi_process_xfer_compl_event() will
> > store transfer result into status variable and then call the
> > dmaengine_desc_callback_invoke(). Hence i2c_gpi_cb_result() can
> > parse the respective errors.
> >
> > while parsing error from the status param, use FIELD_GET with the
>
> Sentences start with the uppercase letter.

Sure, will do while/While change. Will take care in next patch.

> > mask instead of multiple shifting operations for each error.
>
>
> > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> > Co-developed-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
> > Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
> > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > ---
> > ---
Sorry, i Missed to add V1 -> V2 : will add into next patch upload.
> > - Commit log changed we->We.
> > - Explained the problem that we are not detecing NACK error.
> > - Removed Heap based memory allocation and hence memory leakage issue.
> > - Used FIELD_GET and removed shiting and masking every time as suggested by Bjorn.
> > - Changed commit log to reflect the code changes done.
> > - Removed adding anything into struct gpi_i2c_config and created new structure
> >    for error status as suggested by Bjorn.
> > ---
> >   drivers/dma/qcom/gpi.c             | 12 +++++++++++-
> >   drivers/i2c/busses/i2c-qcom-geni.c | 19 +++++++++++++++----
> >   include/linux/dma/qcom-gpi-dma.h   | 10 ++++++++++
> >   3 files changed, 36 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> > index 1c93864e0e4d..e3508d51fdc9 100644
> > --- a/drivers/dma/qcom/gpi.c
> > +++ b/drivers/dma/qcom/gpi.c
> > @@ -1076,7 +1076,17 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
> >          dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue);
> >
> >          dma_cookie_complete(&vd->tx);
> > -       dmaengine_desc_get_callback_invoke(&vd->tx, &result);
> > +       if (gchan->protocol == QCOM_GPI_I2C) {
> > +               struct dmaengine_desc_callback cb;
> > +               struct gpi_i2c_result *i2c;
> > +
> > +               dmaengine_desc_get_callback(&vd->tx, &cb);
> > +               i2c = cb.callback_param;
> > +               i2c->status = compl_event->status;
> > +               dmaengine_desc_callback_invoke(&cb, &result);
> > +       } else {
> > +               dmaengine_desc_get_callback_invoke(&vd->tx, &result);
>
> Is there such error reporting for SPI or UART protocols?

Such errors are not there for SPI or UART because 
NACK/BUS_PROTO/ARB_LOST errors are protocol specific errors. These error 
comes in
middle of the transfers. As these are like expected protocol errors 
depending on the slave device/s response.

> > +       }
> >
> >   gpi_free_desc:
> >          spin_lock_irqsave(&gchan->vc.lock, flags);
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index da94df466e83..36a7c0c0ff54 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -66,6 +66,7 @@ enum geni_i2c_err_code {
> >          GENI_TIMEOUT,
> >   };
> >
> > +#define I2C_DMA_TX_IRQ_MASK    GENMASK(12, 5)
> >   #define DM_I2C_CB_ERR          ((BIT(NACK) | BIT(BUS_PROTO) | BIT(ARB_LOST)) \
> >                                                                          << 5)
> >
> > @@ -99,6 +100,7 @@ struct geni_i2c_dev {
> >          struct dma_chan *rx_c;
> >          bool gpi_mode;
> >          bool abort_done;
> > +       struct gpi_i2c_result i2c_result;
> >   };
> >
> >   struct geni_i2c_desc {
> > @@ -484,9 +486,18 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> >
> >   static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
> >   {
> > -       struct geni_i2c_dev *gi2c = cb;
> > -
> > -       if (result->result != DMA_TRANS_NOERROR) {
> > +       struct gpi_i2c_result *i2c_res = cb;
> > +       struct geni_i2c_dev *gi2c = container_of(i2c_res, struct geni_i2c_dev, i2c_result);
> > +       u32 status;
> > +
> > +       status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
> > +       if (status == BIT(NACK)) {
> > +               geni_i2c_err(gi2c, NACK);
> > +       } else if (status == BIT(BUS_PROTO)) {
> > +               geni_i2c_err(gi2c, BUS_PROTO);
> > +       } else if (status == BIT(ARB_LOST)) {
> > +               geni_i2c_err(gi2c, ARB_LOST);
> > +       } else if (result->result != DMA_TRANS_NOERROR) {
> >                  dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
> >                  gi2c->err = -EIO;
> >          } else if (result->residue) {
> > @@ -568,7 +579,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> >          }
> >
> >          desc->callback_result = i2c_gpi_cb_result;
> > -       desc->callback_param = gi2c;
> > +       desc->callback_param = &gi2c->i2c_result;
> >
> >          dmaengine_submit(desc);
> >          *buf = dma_buf;
> > diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
> > index 6680dd1a43c6..f585c6a35e51 100644
> > --- a/include/linux/dma/qcom-gpi-dma.h
> > +++ b/include/linux/dma/qcom-gpi-dma.h
> > @@ -80,4 +80,14 @@ struct gpi_i2c_config {
> >          bool multi_msg;
> >   };
> >
> > +/**
> > + * struct gpi_i2c_result - i2c transfer status result in GSI mode
> > + *
> > + * @status: store txfer status value as part of callback
> > + *
> > + */
> > +struct gpi_i2c_result {
> > +       u32 status;
> > +};
> > +
> >   #endif /* QCOM_GPI_DMA_H */
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project