| Message ID | 20190305175434.13107-4-wsa+renesas@sang-engineering.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: rcar: make DMA more robust | expand |
Hi Wolfram, On Tue, Mar 5, 2019 at 7:11 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > When sending with DMA, the driver transfers the first byte with PIO (as > documented). However, it started DMA right after the first byte was > written. This worked, but was not according to the datasheet which > suggests to wait until data register was empty again. Implement this. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for your patch! > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -480,6 +480,10 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) > if (!(msr & MDE)) > return; > > + /* Check if DMA can be enabled and take over */ > + if (priv->pos == 1 && rcar_i2c_dma(priv)) Shouldn't you check if MSR.MAT is set? > + return; Hence ICMCR.ESG is not cleared, violating 57.3.8 (Master Transmit Operation), Step 3? > + > if (priv->pos < msg->len) { > /* > * Prepare next data to ICRXTX register. > @@ -490,13 +494,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) > */ > rcar_i2c_write(priv, ICRXTX, msg->buf[priv->pos]); > priv->pos++; > - > - /* > - * Try to use DMA to transmit the rest of the data if > - * address transfer phase just finished. > - */ > - if (msr & MAT) > - rcar_i2c_dma(priv); > } else { > /* > * The last data was pushed to ICRXTX on _PREV_ empty irq. Gr{oetje,eeting}s, Geert
> > + /* Check if DMA can be enabled and take over */ > > + if (priv->pos == 1 && rcar_i2c_dma(priv)) > > Shouldn't you check if MSR.MAT is set? It was set before, when priv->pos == 0. > > > + return; > > Hence ICMCR.ESG is not cleared, violating 57.3.8 (Master Transmit > Operation), Step 3? Clearing ESG is the very first thing we do in the ISR. Otherwise, we run into the race issue we have on this HW and generate an unwanted repeated message.
Hi Wolfram, On Tue, Mar 12, 2019 at 1:50 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > + /* Check if DMA can be enabled and take over */ > > > + if (priv->pos == 1 && rcar_i2c_dma(priv)) > > > > Shouldn't you check if MSR.MAT is set? > > It was set before, when priv->pos == 0. > > > > > > + return; > > > > Hence ICMCR.ESG is not cleared, violating 57.3.8 (Master Transmit > > Operation), Step 3? > > Clearing ESG is the very first thing we do in the ISR. Otherwise, we run > into the race issue we have on this HW and generate an unwanted repeated > message. Thanks! Obviously I should work on improving my i2c-rcar foo ... ;-) Gr{oetje,eeting}s, Geert
> Obviously I should work on improving my i2c-rcar foo ... ;-)
There be dragons! ;)
Hi Wolfram, On Tue, Mar 12, 2019 at 2:18 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > Obviously I should work on improving my i2c-rcar foo ... ;-) > > There be dragons! ;) Fortunately I've enjoyed "How to Train Your Dragon 3" with my girls last weekend ;-) Gr{oetje,eeting}s, Geert
On Tue, Mar 05, 2019 at 06:54:34PM +0100, Wolfram Sang wrote: > When sending with DMA, the driver transfers the first byte with PIO (as > documented). However, it started DMA right after the first byte was > written. This worked, but was not according to the datasheet which > suggests to wait until data register was empty again. Implement this. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > drivers/i2c/busses/i2c-rcar.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 7e009febe97f..63286b3cc69b 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -480,6 +480,10 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) > if (!(msr & MDE)) > return; > > + /* Check if DMA can be enabled and take over */ > + if (priv->pos == 1 && rcar_i2c_dma(priv)) > + return; > + > if (priv->pos < msg->len) { > /* > * Prepare next data to ICRXTX register. > @@ -490,13 +494,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) > */ > rcar_i2c_write(priv, ICRXTX, msg->buf[priv->pos]); > priv->pos++; > - > - /* > - * Try to use DMA to transmit the rest of the data if > - * address transfer phase just finished. > - */ > - if (msr & MAT) > - rcar_i2c_dma(priv); > } else { > /* > * The last data was pushed to ICRXTX on _PREV_ empty irq. > -- > 2.11.0 >
On Tue, Mar 05, 2019 at 06:54:34PM +0100, Wolfram Sang wrote: > When sending with DMA, the driver transfers the first byte with PIO (as > documented). However, it started DMA right after the first byte was > written. This worked, but was not according to the datasheet which > suggests to wait until data register was empty again. Implement this. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 7e009febe97f..63286b3cc69b 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -480,6 +480,10 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) if (!(msr & MDE)) return; + /* Check if DMA can be enabled and take over */ + if (priv->pos == 1 && rcar_i2c_dma(priv)) + return; + if (priv->pos < msg->len) { /* * Prepare next data to ICRXTX register. @@ -490,13 +494,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) */ rcar_i2c_write(priv, ICRXTX, msg->buf[priv->pos]); priv->pos++; - - /* - * Try to use DMA to transmit the rest of the data if - * address transfer phase just finished. - */ - if (msr & MAT) - rcar_i2c_dma(priv); } else { /* * The last data was pushed to ICRXTX on _PREV_ empty irq.
When sending with DMA, the driver transfers the first byte with PIO (as documented). However, it started DMA right after the first byte was written. This worked, but was not according to the datasheet which suggests to wait until data register was empty again. Implement this. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rcar.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)