* [PATCH 0/3] i2c: qcom-geni: Make it possible to run i2cdetect @ 2022-07-17 3:50 Bjorn Andersson 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Bjorn Andersson @ 2022-07-17 3:50 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Vinod Koul, Wolfram Sang Cc: linux-arm-msm, linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-17 3:50 [PATCH 0/3] i2c: qcom-geni: Make it possible to run i2cdetect Bjorn Andersson @ 2022-07-17 3:50 ` Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul ` (3 more replies) 2022-07-17 3:50 ` [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() Bjorn Andersson 2022-07-17 3:50 ` [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT Bjorn Andersson 2 siblings, 4 replies; 19+ messages in thread From: Bjorn Andersson @ 2022-07-17 3:50 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Vinod Koul, Wolfram Sang Cc: linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson @ 2022-07-18 5:36 ` Vinod Koul 2022-07-18 22:28 ` Bjorn Andersson 2022-07-19 20:36 ` Andrew Halaney ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Vinod Koul @ 2022-07-18 5:36 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> > --- > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-18 5:36 ` Vinod Koul @ 2022-07-18 22:28 ` Bjorn Andersson 0 siblings, 0 replies; 19+ messages in thread From: Bjorn Andersson @ 2022-07-18 22:28 UTC (permalink / raw) To: Vinod Koul Cc: Konrad Dybcio, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> > > --- > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul @ 2022-07-19 20:36 ` Andrew Halaney 2022-07-20 4:31 ` Vinod Koul 2022-07-24 5:04 ` Wolfram Sang 3 siblings, 0 replies; 19+ messages in thread From: Andrew Halaney @ 2022-07-19 20:36 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Vinod Koul, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> > --- > 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 <ahalaney@redhat.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul 2022-07-19 20:36 ` Andrew Halaney @ 2022-07-20 4:31 ` Vinod Koul 2022-07-24 5:04 ` Wolfram Sang 3 siblings, 0 replies; 19+ messages in thread From: Vinod Koul @ 2022-07-20 4:31 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <vkoul@kernel.org> > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson ` (2 preceding siblings ...) 2022-07-20 4:31 ` Vinod Koul @ 2022-07-24 5:04 ` Wolfram Sang 2022-07-24 7:30 ` Johan Hovold 2022-07-24 18:39 ` Wolfram Sang 3 siblings, 2 replies; 19+ messages in thread From: Wolfram Sang @ 2022-07-24 5:04 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 524 bytes --] > - 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-24 5:04 ` Wolfram Sang @ 2022-07-24 7:30 ` Johan Hovold 2022-07-24 18:37 ` Wolfram Sang 2022-07-24 18:39 ` Wolfram Sang 1 sibling, 1 reply; 19+ messages in thread From: Johan Hovold @ 2022-07-24 7:30 UTC (permalink / raw) To: Wolfram Sang Cc: Bjorn Andersson, Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 919 bytes --] 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 <johan+linaro@kernel.org> Johan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-24 7:30 ` Johan Hovold @ 2022-07-24 18:37 ` Wolfram Sang 0 siblings, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2022-07-24 18:37 UTC (permalink / raw) To: Johan Hovold Cc: Bjorn Andersson, Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 208 bytes --] > 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! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] i2c: qcom-geni: Use the correct return value 2022-07-24 5:04 ` Wolfram Sang 2022-07-24 7:30 ` Johan Hovold @ 2022-07-24 18:39 ` Wolfram Sang 1 sibling, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2022-07-24 18:39 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 312 bytes --] 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 :) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() 2022-07-17 3:50 [PATCH 0/3] i2c: qcom-geni: Make it possible to run i2cdetect Bjorn Andersson 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson @ 2022-07-17 3:50 ` Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul ` (2 more replies) 2022-07-17 3:50 ` [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT Bjorn Andersson 2 siblings, 3 replies; 19+ messages in thread From: Bjorn Andersson @ 2022-07-17 3:50 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Vinod Koul, Wolfram Sang Cc: linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() 2022-07-17 3:50 ` [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() Bjorn Andersson @ 2022-07-18 5:36 ` Vinod Koul 2022-07-19 20:50 ` Andrew Halaney 2022-07-24 5:15 ` Wolfram Sang 2 siblings, 0 replies; 19+ messages in thread From: Vinod Koul @ 2022-07-18 5:36 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <vkoul@kernel.org> -- ~Vinod ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() 2022-07-17 3:50 ` [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul @ 2022-07-19 20:50 ` Andrew Halaney 2022-07-19 20:57 ` Andrew Halaney 2022-07-24 5:15 ` Wolfram Sang 2 siblings, 1 reply; 19+ messages in thread From: Andrew Halaney @ 2022-07-19 20:50 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Vinod Koul, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> > --- > 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() 2022-07-19 20:50 ` Andrew Halaney @ 2022-07-19 20:57 ` Andrew Halaney 2022-07-24 5:15 ` Wolfram Sang 0 siblings, 1 reply; 19+ messages in thread From: Andrew Halaney @ 2022-07-19 20:57 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Vinod Koul, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() 2022-07-19 20:57 ` Andrew Halaney @ 2022-07-24 5:15 ` Wolfram Sang 0 siblings, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2022-07-24 5:15 UTC (permalink / raw) To: Andrew Halaney Cc: Bjorn Andersson, Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 257 bytes --] > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() 2022-07-17 3:50 ` [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul 2022-07-19 20:50 ` Andrew Halaney @ 2022-07-24 5:15 ` Wolfram Sang 2 siblings, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2022-07-24 5:15 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 551 bytes --] 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 <bjorn.andersson@linaro.org> Applied to for-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT 2022-07-17 3:50 [PATCH 0/3] i2c: qcom-geni: Make it possible to run i2cdetect Bjorn Andersson 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson 2022-07-17 3:50 ` [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() Bjorn Andersson @ 2022-07-17 3:50 ` Bjorn Andersson 2022-07-18 5:37 ` Vinod Koul 2022-07-24 5:23 ` Wolfram Sang 2 siblings, 2 replies; 19+ messages in thread From: Bjorn Andersson @ 2022-07-17 3:50 UTC (permalink / raw) To: Bjorn Andersson, Konrad Dybcio, Vinod Koul, Wolfram Sang Cc: linux-arm-msm, linux-i2c, linux-kernel 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 <bjorn.andersson@linaro.org> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT 2022-07-17 3:50 ` [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT Bjorn Andersson @ 2022-07-18 5:37 ` Vinod Koul 2022-07-24 5:23 ` Wolfram Sang 1 sibling, 0 replies; 19+ messages in thread From: Vinod Koul @ 2022-07-18 5:37 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Wolfram Sang, linux-arm-msm, linux-i2c, linux-kernel 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 <vkoul@kernel.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT 2022-07-17 3:50 ` [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT Bjorn Andersson 2022-07-18 5:37 ` Vinod Koul @ 2022-07-24 5:23 ` Wolfram Sang 1 sibling, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2022-07-24 5:23 UTC (permalink / raw) To: Bjorn Andersson Cc: Konrad Dybcio, Vinod Koul, linux-arm-msm, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 581 bytes --] 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 <bjorn.andersson@linaro.org> 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-07-24 18:39 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-17 3:50 [PATCH 0/3] i2c: qcom-geni: Make it possible to run i2cdetect Bjorn Andersson 2022-07-17 3:50 ` [PATCH 1/3] i2c: qcom-geni: Use the correct return value Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul 2022-07-18 22:28 ` Bjorn Andersson 2022-07-19 20:36 ` Andrew Halaney 2022-07-20 4:31 ` Vinod Koul 2022-07-24 5:04 ` Wolfram Sang 2022-07-24 7:30 ` Johan Hovold 2022-07-24 18:37 ` Wolfram Sang 2022-07-24 18:39 ` Wolfram Sang 2022-07-17 3:50 ` [PATCH 2/3] i2c: qcom-geni: Propagate GENI_ABORT_DONE to geni_i2c_abort_xfer() Bjorn Andersson 2022-07-18 5:36 ` Vinod Koul 2022-07-19 20:50 ` Andrew Halaney 2022-07-19 20:57 ` Andrew Halaney 2022-07-24 5:15 ` Wolfram Sang 2022-07-24 5:15 ` Wolfram Sang 2022-07-17 3:50 ` [PATCH 3/3] i2c: qcom-geni: Silence NACK and GENI_TIMEOUT Bjorn Andersson 2022-07-18 5:37 ` Vinod Koul 2022-07-24 5:23 ` Wolfram Sang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.