[PATCH/RFC] i2c: rcar: Fix order of restart and clear status

Commit Message

Yoshihiro Kaneko Feb. 15, 2015, 2:43 p.m. UTC

From: Ryo Kataoka <[e-mail]>

In case of repeated START condition, the restart has to be kicked
before clear status (MSR register). If it is kicked after clear status,
R-Car I2C may transfer data (TXD register) or receive data (RXD register)
instead of transferring slave address (MAR register).

Signed-off-by: Ryo Kataoka <[e-mail]>
Signed-off-by: Yoshihiro Kaneko <[e-mail]>
---

This patch is based on i2c/for-next branch of Wolfram Sang's linux tree.

 drivers/i2c/busses/i2c-rcar.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Wolfram Sang Feb. 23, 2015, 1:07 p.m. UTC | #1

Hi,

On 2015-02-15 15:43, Yoshihiro Kaneko wrote:
> From: Ryo Kataoka <[e-mail]>
>
> In case of repeated START condition, the restart has to be kicked
> before clear status (MSR register). If it is kicked after clear 
> status,
> R-Car I2C may transfer data (TXD register) or receive data (RXD 
> register)
> instead of transferring slave address (MAR register).
>
> Signed-off-by: Ryo Kataoka <[e-mail]>
> Signed-off-by: Yoshihiro Kaneko <[e-mail]>

Thanks for the patch!

I wondered if we couldn't always kick the start/restart before clearing 
the status register?
I am not sure if this simulates the flaw accurately, but I inserted a 
udelay(500)
in the original code after clearing the status register. Then, I 
couldn't access
the audio codec on my Lager board anymore. By always first "kicking" 
and then
clearing, access was possible again. But as said, I am not sure if my 
scenario
matches yours. Have you considered to treat start and repeated start 
the same
and always kick the condition before clearing the status?

Kind regards,

    Wolfram

> ---
>
> This patch is based on i2c/for-next branch of Wolfram Sang's linux 
> tree.
>
>  drivers/i2c/busses/i2c-rcar.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c 
> b/drivers/i2c/busses/i2c-rcar.c
> index 71a6e07..96c349f 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -2,6 +2,7 @@
>   * Driver for the Renesas RCar I2C unit
>   *
>   * Copyright (C) 2014 Wolfram Sang <[e-mail]>
> + * Copyright (C) 2013-2014 Renesas Electronics Corporation
>   *
>   * Copyright (C) 2012-14 Renesas Solutions Corp.
>   * Kuninori Morimoto <[e-mail]>
> @@ -99,6 +100,7 @@
>  #define ID_DONE		(1 << 2)
>  #define ID_ARBLOST	(1 << 3)
>  #define ID_NACK		(1 << 4)
> +#define ID_FIRST_MSG	(1 << 5)
>
>  enum rcar_i2c_type {
>  	I2C_RCAR_GEN1,
> @@ -125,6 +127,7 @@ struct rcar_i2c_priv {
>  #define rcar_i2c_is_recv(p)		((p)->msg->flags & I2C_M_RD)
>
>  #define rcar_i2c_flags_set(p, f)	((p)->flags |= (f))
> +#define rcar_i2c_flags_clr(p, f)	((p)->flags &= ~(f))
>  #define rcar_i2c_flags_has(p, f)	((p)->flags & (f))
>
>  #define LOOP_TIMEOUT	1024
> @@ -257,8 +260,14 @@ static void rcar_i2c_prepare_msg(struct
> rcar_i2c_priv *priv)
>  	int read = !!rcar_i2c_is_recv(priv);
>
>  	rcar_i2c_write(priv, ICMAR, (priv->msg->addr << 1) | read);
> -	rcar_i2c_write(priv, ICMSR, 0);
> -	rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> +	if (rcar_i2c_flags_has(priv, ID_FIRST_MSG)) {	/* start */
> +		rcar_i2c_write(priv, ICMSR, 0);
> +		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> +		rcar_i2c_flags_clr(priv, ID_FIRST_MSG);
> +	} else {	/* restart */
> +		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> +		rcar_i2c_write(priv, ICMSR, 0);
> +	}
>  	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
>  }
>
> @@ -524,6 +533,8 @@ static int rcar_i2c_master_xfer(struct 
> i2c_adapter *adap,
>  		priv->msg	= &msgs[i];
>  		priv->pos	= 0;
>  		priv->flags	= 0;
> +		if (i == 0)
> +			rcar_i2c_flags_set(priv, ID_FIRST_MSG);
>  		if (i == num - 1)
>  			rcar_i2c_flags_set(priv, ID_LAST_MSG);

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Simon Horman March 4, 2015, 4:08 a.m. UTC | #2

Hi Wolfram,

On Mon, Feb 23, 2015 at 02:07:18PM +0100, Wolfram Sang wrote:
> Hi,
> 
> On 2015-02-15 15:43, Yoshihiro Kaneko wrote:
> >From: Ryo Kataoka <[e-mail]>
> >
> >In case of repeated START condition, the restart has to be kicked
> >before clear status (MSR register). If it is kicked after clear status,
> >R-Car I2C may transfer data (TXD register) or receive data (RXD register)
> >instead of transferring slave address (MAR register).
> >
> >Signed-off-by: Ryo Kataoka <[e-mail]>
> >Signed-off-by: Yoshihiro Kaneko <[e-mail]>
> 
> Thanks for the patch!
> 
> I wondered if we couldn't always kick the start/restart before clearing
> the status register?

I asked Kataoka-san about this and his response was as follows:

If system(CPU) is busy, the driver can't clear the status register soon
after kicking start.

If sequence of first start is as follows, there is a problem.
Because H/W starts by 1.
But sequence of re-start is as follows, there is no problem.
Because H/W starts by 2.

  1. Issue START condition by ESG bit of ICMCR register.
                  <--- If there is too much time, H/W finish transmitting
                       and set status in status register.
  2. Clear interrupt status (ICMSR).
  3. Open interrupt mask.
  4. Wait interrupt.
                  <--- If status is cleared, interrupt does not occur.

> I am not sure if this simulates the flaw accurately, but I inserted a
> udelay(500) in the original code after clearing the status register.
> Then, I couldn't access the audio codec on my Lager board anymore. By
> always first "kicking" and then clearing, access was possible again. But
> as said, I am not sure if my scenario matches yours. Have you considered
> to treat start and repeated start the same and always kick the condition
> before clearing the status?
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wolfram Sang March 6, 2015, 10:05 p.m. UTC | #3

> I asked Kataoka-san about this and his response was as follows:

Thanks, Simon!

> If system(CPU) is busy, the driver can't clear the status register soon
> after kicking start.
> 
> If sequence of first start is as follows, there is a problem.
> Because H/W starts by 1.
> But sequence of re-start is as follows, there is no problem.
> Because H/W starts by 2.
> 
>   1. Issue START condition by ESG bit of ICMCR register.
>                   <--- If there is too much time, H/W finish transmitting
>                        and set status in status register.
>   2. Clear interrupt status (ICMSR).
>   3. Open interrupt mask.
>   4. Wait interrupt.
>                   <--- If status is cleared, interrupt does not occur.

I understand. I'll add this explanation to the patch and apply it soon.

Wolfram Sang March 7, 2015, 10:33 a.m. UTC | #4

On Fri, Mar 06, 2015 at 11:05:31PM +0100, Wolfram Sang wrote:
> 
> > I asked Kataoka-san about this and his response was as follows:
> 
> Thanks, Simon!
> 
> > If system(CPU) is busy, the driver can't clear the status register soon
> > after kicking start.
> > 
> > If sequence of first start is as follows, there is a problem.
> > Because H/W starts by 1.
> > But sequence of re-start is as follows, there is no problem.
> > Because H/W starts by 2.
> > 
> >   1. Issue START condition by ESG bit of ICMCR register.
> >                   <--- If there is too much time, H/W finish transmitting
> >                        and set status in status register.
> >   2. Clear interrupt status (ICMSR).
> >   3. Open interrupt mask.
> >   4. Wait interrupt.
> >                   <--- If status is cleared, interrupt does not occur.
> 
> I understand. I'll add this explanation to the patch and apply it soon.

Sorry, another question came up while applying:

How can this interruption happen? The function is called in a
spin_lock_irqsave protected area. Is this an RT_PREEMPT related issue?
Am I missing something?

Wolfram Sang March 27, 2015, 1:10 p.m. UTC | #5

On Sat, Mar 07, 2015 at 11:33:49AM +0100, Wolfram Sang wrote:
> On Fri, Mar 06, 2015 at 11:05:31PM +0100, Wolfram Sang wrote:
> > 
> > > I asked Kataoka-san about this and his response was as follows:
> > 
> > Thanks, Simon!
> > 
> > > If system(CPU) is busy, the driver can't clear the status register soon
> > > after kicking start.
> > > 
> > > If sequence of first start is as follows, there is a problem.
> > > Because H/W starts by 1.
> > > But sequence of re-start is as follows, there is no problem.
> > > Because H/W starts by 2.
> > > 
> > >   1. Issue START condition by ESG bit of ICMCR register.
> > >                   <--- If there is too much time, H/W finish transmitting
> > >                        and set status in status register.
> > >   2. Clear interrupt status (ICMSR).
> > >   3. Open interrupt mask.
> > >   4. Wait interrupt.
> > >                   <--- If status is cleared, interrupt does not occur.
> > 
> > I understand. I'll add this explanation to the patch and apply it soon.
> 
> Sorry, another question came up while applying:
> 
> How can this interruption happen? The function is called in a
> spin_lock_irqsave protected area. Is this an RT_PREEMPT related issue?
> Am I missing something?

Hi,

any news on this one?

Kind regards,

   Wolfram

Simon Horman March 30, 2015, 12:15 a.m. UTC | #6

On Fri, Mar 27, 2015 at 02:10:45PM +0100, Wolfram Sang wrote:
> On Sat, Mar 07, 2015 at 11:33:49AM +0100, Wolfram Sang wrote:
> > On Fri, Mar 06, 2015 at 11:05:31PM +0100, Wolfram Sang wrote:
> > > 
> > > > I asked Kataoka-san about this and his response was as follows:
> > > 
> > > Thanks, Simon!
> > > 
> > > > If system(CPU) is busy, the driver can't clear the status register soon
> > > > after kicking start.
> > > > 
> > > > If sequence of first start is as follows, there is a problem.
> > > > Because H/W starts by 1.
> > > > But sequence of re-start is as follows, there is no problem.
> > > > Because H/W starts by 2.
> > > > 
> > > >   1. Issue START condition by ESG bit of ICMCR register.
> > > >                   <--- If there is too much time, H/W finish transmitting
> > > >                        and set status in status register.
> > > >   2. Clear interrupt status (ICMSR).
> > > >   3. Open interrupt mask.
> > > >   4. Wait interrupt.
> > > >                   <--- If status is cleared, interrupt does not occur.
> > > 
> > > I understand. I'll add this explanation to the patch and apply it soon.
> > 
> > Sorry, another question came up while applying:
> > 
> > How can this interruption happen? The function is called in a
> > spin_lock_irqsave protected area. Is this an RT_PREEMPT related issue?
> > Am I missing something?
> 
> Hi,
> 
> any news on this one?

Sorry, I seem to have dropped the ball here. I've (finally) forwarded
your question on to the BSP team.

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Simon Horman May 18, 2015, 6:04 a.m. UTC | #7

Hi Wolfram,

On Mon, Mar 30, 2015 at 09:15:30AM +0900, Simon Horman wrote:
> On Fri, Mar 27, 2015 at 02:10:45PM +0100, Wolfram Sang wrote:
> > On Sat, Mar 07, 2015 at 11:33:49AM +0100, Wolfram Sang wrote:
> > > On Fri, Mar 06, 2015 at 11:05:31PM +0100, Wolfram Sang wrote:
> > > > 
> > > > > I asked Kataoka-san about this and his response was as follows:
> > > > 
> > > > Thanks, Simon!
> > > > 
> > > > > If system(CPU) is busy, the driver can't clear the status register soon
> > > > > after kicking start.
> > > > > 
> > > > > If sequence of first start is as follows, there is a problem.
> > > > > Because H/W starts by 1.
> > > > > But sequence of re-start is as follows, there is no problem.
> > > > > Because H/W starts by 2.
> > > > > 
> > > > >   1. Issue START condition by ESG bit of ICMCR register.
> > > > >                   <--- If there is too much time, H/W finish transmitting
> > > > >                        and set status in status register.
> > > > >   2. Clear interrupt status (ICMSR).
> > > > >   3. Open interrupt mask.
> > > > >   4. Wait interrupt.
> > > > >                   <--- If status is cleared, interrupt does not occur.
> > > > 
> > > > I understand. I'll add this explanation to the patch and apply it soon.
> > > 
> > > Sorry, another question came up while applying:
> > > 
> > > How can this interruption happen? The function is called in a
> > > spin_lock_irqsave protected area. Is this an RT_PREEMPT related issue?
> > > Am I missing something?
> > 
> > Hi,
> > 
> > any news on this one?
> 
> Sorry, I seem to have dropped the ball here. I've (finally) forwarded
> your question on to the BSP team.

Thanks for your patience, I have a response from the BSP team:

rcar_i2c_start(), rcar_i2c_recv() and rcar_i2c_send() are called in a
spin_lock_irqsave protected area. Therefore, interruption and preemption
doesn't happen in these functions.

But I think the driver should work fine even if the CPU is very slow.
If above sequence 1 and 2 are slow, and if H/W is fast, there is a problem.

Since H/W is kicked by sequence 1 then the driver waits the change of status,
sequence 2 should be executed before sequence 1 even if the CPU is fast enough.
I think it is a custom of software.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 71a6e07..96c349f 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -2,6 +2,7 @@ 
  * Driver for the Renesas RCar I2C unit
  *
  * Copyright (C) 2014 Wolfram Sang <[e-mail]>
+ * Copyright (C) 2013-2014 Renesas Electronics Corporation
  *
  * Copyright (C) 2012-14 Renesas Solutions Corp.
  * Kuninori Morimoto <[e-mail]>
@@ -99,6 +100,7 @@ 
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
+#define ID_FIRST_MSG	(1 << 5)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -125,6 +127,7 @@  struct rcar_i2c_priv {
 #define rcar_i2c_is_recv(p)		((p)->msg->flags & I2C_M_RD)
 
 #define rcar_i2c_flags_set(p, f)	((p)->flags |= (f))
+#define rcar_i2c_flags_clr(p, f)	((p)->flags &= ~(f))
 #define rcar_i2c_flags_has(p, f)	((p)->flags & (f))
 
 #define LOOP_TIMEOUT	1024
@@ -257,8 +260,14 @@  static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 	int read = !!rcar_i2c_is_recv(priv);
 
 	rcar_i2c_write(priv, ICMAR, (priv->msg->addr << 1) | read);
-	rcar_i2c_write(priv, ICMSR, 0);
-	rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+	if (rcar_i2c_flags_has(priv, ID_FIRST_MSG)) {	/* start */
+		rcar_i2c_write(priv, ICMSR, 0);
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+		rcar_i2c_flags_clr(priv, ID_FIRST_MSG);
+	} else {	/* restart */
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+		rcar_i2c_write(priv, ICMSR, 0);
+	}
 	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 }
 
@@ -524,6 +533,8 @@  static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 		priv->msg	= &msgs[i];
 		priv->pos	= 0;
 		priv->flags	= 0;
+		if (i == 0)
+			rcar_i2c_flags_set(priv, ID_FIRST_MSG);
 		if (i == num - 1)
 			rcar_i2c_flags_set(priv, ID_LAST_MSG);