On 3/7/2024 8:15 PM, Dmitry Baryshkov wrote:
On Thu, 7 Mar 2024 at 15:46, Mukesh Kumar Savaliya
<quic_msavaliy@xxxxxxxxxxx> wrote:
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.
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@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.
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
