Message-ID: <eeefab74-1303-470f-bd3c-618d9522d24b@quicinc.com>
Date: Fri, 8 Mar 2024 02:27:49 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <konrad.dybcio@...aro.org>, <vkoul@...nel.org>, <andi.shyti@...nel.org>,
        <wsa@...nel.org>, <linux-arm-msm@...r.kernel.org>,
        <dmaengine@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-i2c@...r.kernel.org>, <quic_vdadhani@...cinc.com>,
        Bjorn Andersson
	<andersson@...nel.org>
Subject: Re: [PATCH v2] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI
 mode



On 3/7/2024 8:15 PM, Dmitry Baryshkov wrote:
> On Thu, 7 Mar 2024 at 15:46, Mukesh Kumar Savaliya
> <quic_msavaliy@...cinc.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@...cinc.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@...cinc.com>
>>>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.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
>>>>
>>>>
>>>
>>>
> 
> 
> 

Powered by blists - more mailing lists