| Message ID | 1447948611-2615-10-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello. On 11/19/2015 06:56 PM, Wolfram Sang wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > If we don't clear START generation as soon as possible, it may cause > another message to be generated, e.g. when receiving NACK in address > phase. To keep the race window as small as possible, we clear it right > at the beginning of the interrupt. We don't need any checks since we > always want to stop START and STOP generation on the next occasion after > we started it. > > This patch improves the situation but sadly does not completely fix it. > It is still to be researched if we can do better given this HW design. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for a great work, Wolfram! We need this patch in -stable kernels. The R-Car audio just doesn't work without it... > --- > drivers/i2c/busses/i2c-rcar.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index f237b4fc5b5e55..40979130200935 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -83,6 +83,7 @@ > > #define RCAR_BUS_PHASE_START (MDBS | MIE | ESG) > #define RCAR_BUS_PHASE_DATA (MDBS | MIE) > +#define RCAR_BUS_MASK_DATA (~(ESG | FSB) & 0xFF) > #define RCAR_BUS_PHASE_STOP (MDBS | MIE | FSB) > > #define RCAR_IRQ_SEND (MNR | MAL | MST | MAT | MDE) > @@ -289,13 +290,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) > if (!(msr & MDE)) > return; > > - /* > - * If address transfer phase finished, > - * goto data phase. > - */ > - if (msr & MAT) > - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); > - > if (priv->pos < msg->len) { > /* > * Prepare next data to ICRXTX register. > @@ -345,11 +339,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) > return; > > if (msr & MAT) { > - /* > - * Address transfer phase finished, > - * but, there is no data at this point. > - * Do nothing. > - */ > + /* Address transfer phase finished, but no data at this point. */ > } else if (priv->pos < msg->len) { > /* > * get received data > @@ -365,8 +355,6 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) > */ > if (priv->pos + 1 >= msg->len) > rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); > - else > - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); > > if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) > rcar_i2c_next_msg(priv); > @@ -432,7 +420,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv) > static irqreturn_t rcar_i2c_irq(int irq, void *ptr) > { > struct rcar_i2c_priv *priv = ptr; > - u32 msr; > + u32 msr, val; > + > + /* Clear START or STOP as soon as we can */ > + val = rcar_i2c_read(priv, ICMCR); > + rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA); > > msr = rcar_i2c_read(priv, ICMSR); > > @@ -454,7 +446,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr) > /* Nack */ > if (msr & MNR) { > /* HW automatically sends STOP after received NACK */ > - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); > rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP); > rcar_i2c_flags_set(priv, ID_NACK); > goto out; MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 01, 2016 at 12:02:56AM +0300, Sergei Shtylyov wrote: > Hello. > > On 11/19/2015 06:56 PM, Wolfram Sang wrote: > > >From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > >If we don't clear START generation as soon as possible, it may cause > >another message to be generated, e.g. when receiving NACK in address > >phase. To keep the race window as small as possible, we clear it right > >at the beginning of the interrupt. We don't need any checks since we > >always want to stop START and STOP generation on the next occasion after > >we started it. > > > >This patch improves the situation but sadly does not completely fix it. > >It is still to be researched if we can do better given this HW design. > > > >Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Thanks for a great work, Wolfram! > We need this patch in -stable kernels. The R-Car audio just doesn't work > without it... Really only this patch? IIRC my tests showed that if you don't remove the spinlocks (patch 4), the interrupt latency will already be too high again. In any case, you'd need to do some careful backporting to rip this out of the whole refactoring series. But maybe you did that already and have good experiences?
Hello. On 04/01/2016 01:48 AM, Wolfram Sang wrote: >>> From: Wolfram Sang <wsa+renesas@sang-engineering.com> >>> >>> If we don't clear START generation as soon as possible, it may cause >>> another message to be generated, e.g. when receiving NACK in address >>> phase. To keep the race window as small as possible, we clear it right >>> at the beginning of the interrupt. We don't need any checks since we >>> always want to stop START and STOP generation on the next occasion after >>> we started it. >>> >>> This patch improves the situation but sadly does not completely fix it. >>> It is still to be researched if we can do better given this HW design. >>> >>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> >> Thanks for a great work, Wolfram! >> We need this patch in -stable kernels. The R-Car audio just doesn't work >> without it... > Really only this patch? Well, my "reverse" bisection pointed at it. :-) > IIRC my tests showed that if you don't remove > the spinlocks (patch 4), the interrupt latency will already be too high > again. Thank you for the valuable info! > In any case, you'd need to do some careful backporting to rip > this out of the whole refactoring series. Yes, I've already figured that. > But maybe you did that already > and have good experiences? Not yet, I will report back after more backporting/testing. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >IIRC my tests showed that if you don't remove > >the spinlocks (patch 4), the interrupt latency will already be too high > >again. > > Thank you for the valuable info! Oh, and add patch 6 to it. The context switches are a major problem in this.
On 04/01/2016 11:14 PM, Wolfram Sang wrote: > >>> IIRC my tests showed that if you don't remove >>> the spinlocks (patch 4), the interrupt latency will already be too high >>> again. >> >> Thank you for the valuable info! > > Oh, and add patch 6 to it. The context switches are a major problem in > this. Thanks again! I ended up backporting all the patches up to #9. Not sure it'd be acceptable for the -stable kernels since there's a lot of refactors involved... MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Thanks again! I ended up backporting all the patches up to #9. Not sure > it'd be acceptable for the -stable kernels since there's a lot of refactors > involved... That's what I thought, too.
On 4/2/2016 10:27 AM, Wolfram Sang wrote: >> Thanks again! I ended up backporting all the patches up to #9. Not sure >> it'd be acceptable for the -stable kernels since there's a lot of refactors >> involved... > > That's what I thought, too. And I still don't feel good about the patch removing the spinlock -- I'm not sure it's SMP-safe... MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> And I still don't feel good about the patch removing the spinlock -- I'm > not sure it's SMP-safe... In what scenario?
On 04/03/2016 11:25 AM, Wolfram Sang wrote: >> And I still don't feel good about the patch removing the spinlock -- I'm >> not sure it's SMP-safe... > > In what scenario? I've reviewed the patches once again and it looked like they are safe. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> And I still don't feel good about the patch removing the spinlock -- I'm > >>not sure it's SMP-safe... > > > >In what scenario? > > I've reviewed the patches once again and it looked like they are safe. Thanks.
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index f237b4fc5b5e55..40979130200935 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -83,6 +83,7 @@ #define RCAR_BUS_PHASE_START (MDBS | MIE | ESG) #define RCAR_BUS_PHASE_DATA (MDBS | MIE) +#define RCAR_BUS_MASK_DATA (~(ESG | FSB) & 0xFF) #define RCAR_BUS_PHASE_STOP (MDBS | MIE | FSB) #define RCAR_IRQ_SEND (MNR | MAL | MST | MAT | MDE) @@ -289,13 +290,6 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) if (!(msr & MDE)) return; - /* - * If address transfer phase finished, - * goto data phase. - */ - if (msr & MAT) - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); - if (priv->pos < msg->len) { /* * Prepare next data to ICRXTX register. @@ -345,11 +339,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) return; if (msr & MAT) { - /* - * Address transfer phase finished, - * but, there is no data at this point. - * Do nothing. - */ + /* Address transfer phase finished, but no data at this point. */ } else if (priv->pos < msg->len) { /* * get received data @@ -365,8 +355,6 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) */ if (priv->pos + 1 >= msg->len) rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); - else - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) rcar_i2c_next_msg(priv); @@ -432,7 +420,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv) static irqreturn_t rcar_i2c_irq(int irq, void *ptr) { struct rcar_i2c_priv *priv = ptr; - u32 msr; + u32 msr, val; + + /* Clear START or STOP as soon as we can */ + val = rcar_i2c_read(priv, ICMCR); + rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA); msr = rcar_i2c_read(priv, ICMSR); @@ -454,7 +446,6 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr) /* Nack */ if (msr & MNR) { /* HW automatically sends STOP after received NACK */ - rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP); rcar_i2c_flags_set(priv, ID_NACK); goto out;