Attempting to run i2cdetect shows that error handling in the GENI I2C driver
could be better.
The first issue is that errors aren't returned from the driver. Following this
is an issue that if a timeout occurs the current operation is aborted, but the
abort code races with the isr so the abort times out as well. Lastly when this
happens, the driver is quite noisy, making it impossible to run i2cdetect on
the serial console.
With this series in place, I was able to run i2cdetect on the db845c and two
sc8180x devices - and get useful output.
Bjorn Andersson (3):
i2c: qcom-geni: Use the correct return value
i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer()
i2c: qcom-geni: Silence NACK and GENI_TIMEOUT
drivers/i2c/busses/i2c-qcom-geni.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
--
2.35.1
Turn NACK and GENI_TIMEOUT into debug prints to silence the kernel log
when running things such as i2cdetect to scan the bus.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/i2c/busses/i2c-qcom-geni.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index e212e7ae7ad2..6ac179a373ff 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -208,6 +208,10 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
case GENI_ABORT_DONE:
gi2c->abort_done = true;
break;
+ case NACK:
+ case GENI_TIMEOUT:
+ dev_dbg(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
+ break;
default:
dev_err(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
geni_i2c_err_misc(gi2c);
--
2.35.1
The introduction of GPI support moved things around and instead of
returning the result from geni_i2c_xfer() the number of messages in the
request was returned, ignoring the actual result. Fix this.
Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/i2c/busses/i2c-qcom-geni.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 6ac402ea58fb..3bec7c782824 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
pm_runtime_put_autosuspend(gi2c->se.dev);
gi2c->cur = NULL;
gi2c->err = 0;
- return num;
+ return ret;
}
static u32 geni_i2c_func(struct i2c_adapter *adap)
--
2.35.1
Waiting for M_CMD_ABORT_EN in geni_i2c_abort_xfer() races with the
interrupt handler which will read and clear the abort bit, the result is
that every abort attempt takes 1 second and is followed by a message
about the abort having times out.
Introduce a new state variable to carry the abort_done state from the
interrupt handler back to geni_i2c_abort_xfer().
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/i2c/busses/i2c-qcom-geni.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 3bec7c782824..e212e7ae7ad2 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -97,6 +97,7 @@ struct geni_i2c_dev {
struct dma_chan *tx_c;
struct dma_chan *rx_c;
bool gpi_mode;
+ bool abort_done;
};
struct geni_i2c_err_log {
@@ -203,9 +204,14 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
dev_dbg(gi2c->se.dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n",
gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags);
- if (err != NACK && err != GENI_ABORT_DONE) {
+ switch (err) {
+ case GENI_ABORT_DONE:
+ gi2c->abort_done = true;
+ break;
+ default:
dev_err(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
geni_i2c_err_misc(gi2c);
+ break;
}
}
@@ -311,21 +317,21 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
{
- u32 val;
unsigned long time_left = ABORT_TIMEOUT;
unsigned long flags;
spin_lock_irqsave(&gi2c->lock, flags);
geni_i2c_err(gi2c, GENI_TIMEOUT);
gi2c->cur = NULL;
+ gi2c->abort_done = false;
geni_se_abort_m_cmd(&gi2c->se);
spin_unlock_irqrestore(&gi2c->lock, flags);
+
do {
time_left = wait_for_completion_timeout(&gi2c->done, time_left);
- val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
- } while (!(val & M_CMD_ABORT_EN) && time_left);
+ } while (!gi2c->abort_done && time_left);
- if (!(val & M_CMD_ABORT_EN))
+ if (!time_left)
dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
}
--
2.35.1
On 16-07-22, 20:50, Bjorn Andersson wrote:
> Turn NACK and GENI_TIMEOUT into debug prints to silence the kernel log
> when running things such as i2cdetect to scan the bus.
Reviewed-by: Vinod Koul <[email protected]>
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index e212e7ae7ad2..6ac179a373ff 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -208,6 +208,10 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
> case GENI_ABORT_DONE:
> gi2c->abort_done = true;
> break;
> + case NACK:
> + case GENI_TIMEOUT:
> + dev_dbg(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
> + break;
> default:
> dev_err(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
> geni_i2c_err_misc(gi2c);
> --
> 2.35.1
--
~Vinod
On 16-07-22, 20:50, Bjorn Andersson wrote:
> The introduction of GPI support moved things around and instead of
> returning the result from geni_i2c_xfer() the number of messages in the
> request was returned, ignoring the actual result. Fix this.
Thanks for the fix, looking at master_xfer() it does expect error
return, so look good with one nit:
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 6ac402ea58fb..3bec7c782824 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> pm_runtime_put_autosuspend(gi2c->se.dev);
> gi2c->cur = NULL;
> gi2c->err = 0;
Unrelated, should gi2c->err be set to ret here..?
> - return num;
> + return ret;
> }
>
> static u32 geni_i2c_func(struct i2c_adapter *adap)
> --
> 2.35.1
--
~Vinod
On 16-07-22, 20:50, Bjorn Andersson wrote:
> Waiting for M_CMD_ABORT_EN in geni_i2c_abort_xfer() races with the
> interrupt handler which will read and clear the abort bit, the result is
> that every abort attempt takes 1 second and is followed by a message
> about the abort having times out.
>
> Introduce a new state variable to carry the abort_done state from the
> interrupt handler back to geni_i2c_abort_xfer().
Reviewed-by: Vinod Koul <[email protected]>
--
~Vinod
On Mon 18 Jul 00:36 CDT 2022, Vinod Koul wrote:
> On 16-07-22, 20:50, Bjorn Andersson wrote:
> > The introduction of GPI support moved things around and instead of
> > returning the result from geni_i2c_xfer() the number of messages in the
> > request was returned, ignoring the actual result. Fix this.
>
> Thanks for the fix, looking at master_xfer() it does expect error
> return, so look good with one nit:
>
> >
> > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-qcom-geni.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 6ac402ea58fb..3bec7c782824 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> > pm_runtime_put_autosuspend(gi2c->se.dev);
> > gi2c->cur = NULL;
> > gi2c->err = 0;
>
> Unrelated, should gi2c->err be set to ret here..?
>
When we reach this point we have concluded the current transfer
(successfully or not...), so I believe that the purpose of this line is
to clear the "error state" that might have occurred during that transfer.
I believe this line could be removed, as the first step in a transfer is
to clear the error state again. But as you suggest this is separate to
the proposed change.
May I have a R-b?
Regards,
Bjorn
> > - return num;
> > + return ret;
> > }
> >
> > static u32 geni_i2c_func(struct i2c_adapter *adap)
> > --
> > 2.35.1
>
> --
> ~Vinod
On Sat, Jul 16, 2022 at 08:50:25PM -0700, Bjorn Andersson wrote:
> The introduction of GPI support moved things around and instead of
> returning the result from geni_i2c_xfer() the number of messages in the
> request was returned, ignoring the actual result. Fix this.
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 6ac402ea58fb..3bec7c782824 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> pm_runtime_put_autosuspend(gi2c->se.dev);
> gi2c->cur = NULL;
> gi2c->err = 0;
> - return num;
> + return ret;
> }
>
> static u32 geni_i2c_func(struct i2c_adapter *adap)
> --
> 2.35.1
>
Reviewed-by: Andrew Halaney <[email protected]>
On Sat, Jul 16, 2022 at 08:50:26PM -0700, Bjorn Andersson wrote:
> Waiting for M_CMD_ABORT_EN in geni_i2c_abort_xfer() races with the
> interrupt handler which will read and clear the abort bit, the result is
> that every abort attempt takes 1 second and is followed by a message
> about the abort having times out.
>
> Introduce a new state variable to carry the abort_done state from the
> interrupt handler back to geni_i2c_abort_xfer().
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 3bec7c782824..e212e7ae7ad2 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -97,6 +97,7 @@ struct geni_i2c_dev {
> struct dma_chan *tx_c;
> struct dma_chan *rx_c;
> bool gpi_mode;
> + bool abort_done;
> };
>
> struct geni_i2c_err_log {
> @@ -203,9 +204,14 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
> dev_dbg(gi2c->se.dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n",
> gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags);
>
> - if (err != NACK && err != GENI_ABORT_DONE) {
> + switch (err) {
> + case GENI_ABORT_DONE:
> + gi2c->abort_done = true;
> + break;
> + default:
> dev_err(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
> geni_i2c_err_misc(gi2c);
> + break;
> }
> }
>
If I'm reading this right this changes the behavior on a NACK error now,
right? Was that intentional?
Otherwise looks good to me.
> @@ -311,21 +317,21 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
>
> static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
> {
> - u32 val;
> unsigned long time_left = ABORT_TIMEOUT;
> unsigned long flags;
>
> spin_lock_irqsave(&gi2c->lock, flags);
> geni_i2c_err(gi2c, GENI_TIMEOUT);
> gi2c->cur = NULL;
> + gi2c->abort_done = false;
> geni_se_abort_m_cmd(&gi2c->se);
> spin_unlock_irqrestore(&gi2c->lock, flags);
> +
> do {
> time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> - val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
> - } while (!(val & M_CMD_ABORT_EN) && time_left);
> + } while (!gi2c->abort_done && time_left);
>
> - if (!(val & M_CMD_ABORT_EN))
> + if (!time_left)
> dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
> }
>
> --
> 2.35.1
>
On Tue, Jul 19, 2022 at 03:50:54PM -0500, Andrew Halaney wrote:
> On Sat, Jul 16, 2022 at 08:50:26PM -0700, Bjorn Andersson wrote:
> > Waiting for M_CMD_ABORT_EN in geni_i2c_abort_xfer() races with the
> > interrupt handler which will read and clear the abort bit, the result is
> > that every abort attempt takes 1 second and is followed by a message
> > about the abort having times out.
> >
> > Introduce a new state variable to carry the abort_done state from the
> > interrupt handler back to geni_i2c_abort_xfer().
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-qcom-geni.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 3bec7c782824..e212e7ae7ad2 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -97,6 +97,7 @@ struct geni_i2c_dev {
> > struct dma_chan *tx_c;
> > struct dma_chan *rx_c;
> > bool gpi_mode;
> > + bool abort_done;
> > };
> >
> > struct geni_i2c_err_log {
> > @@ -203,9 +204,14 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
> > dev_dbg(gi2c->se.dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n",
> > gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags);
> >
> > - if (err != NACK && err != GENI_ABORT_DONE) {
> > + switch (err) {
> > + case GENI_ABORT_DONE:
> > + gi2c->abort_done = true;
> > + break;
> > + default:
> > dev_err(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
> > geni_i2c_err_misc(gi2c);
> > + break;
> > }
> > }
> >
>
> If I'm reading this right this changes the behavior on a NACK error now,
> right? Was that intentional?
>
> Otherwise looks good to me.
>
Hmmm (I should really review the whole series before hitting send :P ),
it seems you cleaned the NACK stuff up in the next patch in the series.
I guess one more thing, does this patch deserve a Fixes: tag? If so
might be nice to make NACK not so loud in this patch in case it gets
backported without the next patch.
Thanks,
Andrew
> > @@ -311,21 +317,21 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
> >
> > static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
> > {
> > - u32 val;
> > unsigned long time_left = ABORT_TIMEOUT;
> > unsigned long flags;
> >
> > spin_lock_irqsave(&gi2c->lock, flags);
> > geni_i2c_err(gi2c, GENI_TIMEOUT);
> > gi2c->cur = NULL;
> > + gi2c->abort_done = false;
> > geni_se_abort_m_cmd(&gi2c->se);
> > spin_unlock_irqrestore(&gi2c->lock, flags);
> > +
> > do {
> > time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> > - val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
> > - } while (!(val & M_CMD_ABORT_EN) && time_left);
> > + } while (!gi2c->abort_done && time_left);
> >
> > - if (!(val & M_CMD_ABORT_EN))
> > + if (!time_left)
> > dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
> > }
> >
> > --
> > 2.35.1
> >
On 16-07-22, 20:50, Bjorn Andersson wrote:
> The introduction of GPI support moved things around and instead of
> returning the result from geni_i2c_xfer() the number of messages in the
> request was returned, ignoring the actual result. Fix this.
Reviewed-by: Vinod Koul <[email protected]>
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 6ac402ea58fb..3bec7c782824 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -688,7 +688,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> pm_runtime_put_autosuspend(gi2c->se.dev);
> gi2c->cur = NULL;
> gi2c->err = 0;
> - return num;
> + return ret;
> }
>
> static u32 geni_i2c_func(struct i2c_adapter *adap)
> --
> 2.35.1
--
~Vinod
> - return num;
> + return ret;
Not quite. Kdoc describes the retval in i2c_transfer.
2136 * i2c_transfer - execute a single or combined I2C message
2137 * @adap: Handle to I2C bus
2138 * @msgs: One or more messages to execute before STOP is issued to
2139 * terminate the operation; each message begins with a START.
2140 * @num: Number of messages to be executed.
2141 *
2142 * Returns negative errno, else the number of messages executed.
I agree this needs better documentation in i2c.h, I will fix it.
> I guess one more thing, does this patch deserve a Fixes: tag? If so
> might be nice to make NACK not so loud in this patch in case it gets
> backported without the next patch.
I squashed the two patches to keep the NACK behaviour and ease
backporting.
On Sat, Jul 16, 2022 at 08:50:26PM -0700, Bjorn Andersson wrote:
> Waiting for M_CMD_ABORT_EN in geni_i2c_abort_xfer() races with the
> interrupt handler which will read and clear the abort bit, the result is
> that every abort attempt takes 1 second and is followed by a message
> about the abort having times out.
>
> Introduce a new state variable to carry the abort_done state from the
> interrupt handler back to geni_i2c_abort_xfer().
>
> Signed-off-by: Bjorn Andersson <[email protected]>
Applied to for-next, thanks!
On Sat, Jul 16, 2022 at 08:50:27PM -0700, Bjorn Andersson wrote:
> Turn NACK and GENI_TIMEOUT into debug prints to silence the kernel log
> when running things such as i2cdetect to scan the bus.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
As mentioned, I squashed it with the previous patch.
> + case NACK:
> + case GENI_TIMEOUT:
> + dev_dbg(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
> + break;
Additional note: the dbg messages in this function are superfluous. We
already have all this information with the tracepoints in i2c_transfer.
On Sun, Jul 24, 2022 at 07:04:12AM +0200, Wolfram Sang wrote:
>
> > - return num;
> > + return ret;
>
> Not quite. Kdoc describes the retval in i2c_transfer.
>
> 2136 * i2c_transfer - execute a single or combined I2C message
> 2137 * @adap: Handle to I2C bus
> 2138 * @msgs: One or more messages to execute before STOP is issued to
> 2139 * terminate the operation; each message begins with a START.
> 2140 * @num: Number of messages to be executed.
> 2141 *
> 2142 * Returns negative errno, else the number of messages executed.
And this is exactly what the driver is returning after this fix.
Note that ret above is assigned based on the return value from two
helper functions that return either a negative errno or the number
messages executed.
So I believe this patch is still needed to fix the error handling.
Reviewed-by: Johan Hovold <[email protected]>
Johan
> Note that ret above is assigned based on the return value from two
> helper functions that return either a negative errno or the number
> messages executed.
Ah, cool. I see now. Thanks for the heads up!
On Sun, Jul 24, 2022 at 07:04:12AM +0200, Wolfram Sang wrote:
>
> > - return num;
> > + return ret;
>
> Not quite. Kdoc describes the retval in i2c_transfer.
Reconsidered and applied to for-next, thanks!
> I agree this needs better documentation in i2c.h, I will fix it.
I'll still do this :)