| Message ID | 20240307093605.4142639-1-quic_msavaliy@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode | expand |
On Thu, 7 Mar 2024 at 11:36, Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> 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. > > 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. > 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@quicinc.com> > Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > --- > - 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? > + } > > 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 > >
On 3/7/2024 3:23 PM, Dmitry Baryshkov wrote: > On Thu, 7 Mar 2024 at 11:36, Mukesh Kumar Savaliya > <quic_msavaliy@quicinc.com> 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@quicinc.com> >> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> >> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> >> --- >> --- 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 >> >> > >
On Thu, 7 Mar 2024 at 15:46, Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> wrote: > > > > > On 3/7/2024 3:23 PM, Dmitry Baryshkov wrote: > > On Thu, 7 Mar 2024 at 11:36, Mukesh Kumar Savaliya > > <quic_msavaliy@quicinc.com> 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. Let me quote the relevant part for you: Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. > > 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@quicinc.com> > >> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> > >> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > >> --- > >> --- > 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. Yes, these particular errors are I2C specific. My question was more generic: do we have any similar errors for SPI or UART GENI protocols that we should report from GPI to the corresponding driver? > > >> + } > >> > >> 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 > >> > >> > > > >
On 3/7/2024 8:15 PM, Dmitry Baryshkov wrote: > On Thu, 7 Mar 2024 at 15:46, Mukesh Kumar Savaliya > <quic_msavaliy@quicinc.com> wrote: >> >> >> >> >> On 3/7/2024 3:23 PM, Dmitry Baryshkov wrote: >>> On Thu, 7 Mar 2024 at 11:36, Mukesh Kumar Savaliya >>> <quic_msavaliy@quicinc.com> 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. > > Let me quote the relevant part for you: > > Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour. > Ahh, i got it. Thanks ! I hope i could write in an expected imperative mood. Uploaded V3 with the enhanced commit log. >> >> 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@quicinc.com> >>>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> >>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> >>>> --- >>>> --- >> 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. > > Yes, these particular errors are I2C specific. My question was more > generic: do we have any similar errors for SPI or UART GENI protocols > that we should report from GPI to the corresponding driver? > Got it. Reviewed and confirming that UART and SPI GENI drivers doesn't have such error bits unlike I2C, it simply reports transfer fail OR success. >> >>>> + } >>>> >>>> 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 >>>> >>>> >>> >>> > > >
On Thu, 7 Mar 2024 at 22:58, Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> wrote: > > > > On 3/7/2024 8:15 PM, Dmitry Baryshkov wrote: > > On Thu, 7 Mar 2024 at 15:46, Mukesh Kumar Savaliya > > <quic_msavaliy@quicinc.com> wrote: > >> > >> > >> > >> > >> On 3/7/2024 3:23 PM, Dmitry Baryshkov wrote: > >>> On Thu, 7 Mar 2024 at 11:36, Mukesh Kumar Savaliya > >>> <quic_msavaliy@quicinc.com> wrote: > >>>> 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. > > > > Yes, these particular errors are I2C specific. My question was more > > generic: do we have any similar errors for SPI or UART GENI protocols > > that we should report from GPI to the corresponding driver? > > > > Got it. Reviewed and confirming that UART and SPI GENI drivers doesn't > have such error bits unlike I2C, it simply reports transfer fail OR > success. Thank you for the confirmation!
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); + } 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 */
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. 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 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@quicinc.com> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> --- --- - 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(-)