diff mbox

[PATCH/RFC] i2c: rcar: Support ACK by HW auto restart after NACK

Message ID 1424011480-3551-1-git-send-email-ykaneko0929@gmail.com (mailing list archive)
State RFC
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Yoshihiro Kaneko Feb. 15, 2015, 2:44 p.m. UTC 
From: Ryo Kataoka <ryo.kataoka.wt@renesas.com>

Even if R-Car I2C received NACK, after that it might receive ACK
by HW auto restart. In case of that, driver would continue process.
If R-Car I2C didn't receive ACK, the driver would detect timeout
and would report NACK as -ENXIO.

Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---

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

 drivers/i2c/busses/i2c-rcar.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

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

On 2015-02-15 15:44, Yoshihiro Kaneko wrote:
> From: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
>
> Even if R-Car I2C received NACK, after that it might receive ACK
> by HW auto restart. In case of that, driver would continue process.
> If R-Car I2C didn't receive ACK, the driver would detect timeout
> and would report NACK as -ENXIO.
>
> Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

Excuse me, but what exactly is HW auto restart in this case? Is it a 
feature of the I2C slave?

Thanks,

    Wolfram

--
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
Simon Horman March 4, 2015, 4:06 a.m. UTC | #2 
Hi Wolfram,

On Mon, Feb 23, 2015 at 02:08:42PM +0100, Wolfram Sang wrote:
> Hi,
> 
> On 2015-02-15 15:44, Yoshihiro Kaneko wrote:
> >From: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
> >
> >Even if R-Car I2C received NACK, after that it might receive ACK
> >by HW auto restart. In case of that, driver would continue process.
> >If R-Car I2C didn't receive ACK, the driver would detect timeout
> >and would report NACK as -ENXIO.
> >
> >Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
> >Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> Excuse me, but what exactly is HW auto restart in this case? Is it a feature
> of the I2C slave?

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

    It is a feature of the i2c-rcar(H/W) master.

    If system(CPU) is busy, NACK procedure may have interrupt latency.
    Since the clear of ICMCR.ESG bit is delayed, i2c-rcar(H/W) may auto-restart
    after NACK. Please refer to ESG bit of H/W UM section 55.3.5.

    For example, this is I2C write transmitting.
    1.Start / 2.SlaveAddr,ACK / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop

    If No.2 has NACK and interruption has delay, this transmitting is as follows.
    1.Start / 2.SlaveAddr,NACK/ 1x.auto-restart / 2x.SlaveAddr,ACK
                                    / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop

    NACK of No.2 is invalidated by ACK of No.2x. It means recover.
--
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
Wolfram Sang March 6, 2015, 10:18 p.m. UTC | #3 
> > >Even if R-Car I2C received NACK, after that it might receive ACK
> > >by HW auto restart. In case of that, driver would continue process.
> > >If R-Car I2C didn't receive ACK, the driver would detect timeout
> > >and would report NACK as -ENXIO.
> > >
> > >Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
> > >Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > 
> > Excuse me, but what exactly is HW auto restart in this case? Is it a feature
> > of the I2C slave?
> 
> I asked Kataoka-san about this and his response was as follows:
> 
>     It is a feature of the i2c-rcar(H/W) master.
> 
>     If system(CPU) is busy, NACK procedure may have interrupt latency.
>     Since the clear of ICMCR.ESG bit is delayed, i2c-rcar(H/W) may auto-restart
>     after NACK. Please refer to ESG bit of H/W UM section 55.3.5.
> 
>     For example, this is I2C write transmitting.
>     1.Start / 2.SlaveAddr,ACK / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop
> 
>     If No.2 has NACK and interruption has delay, this transmitting is as follows.
>     1.Start / 2.SlaveAddr,NACK/ 1x.auto-restart / 2x.SlaveAddr,ACK
>                                     / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop
> 
>     NACK of No.2 is invalidated by ACK of No.2x. It means recover.

Does this make some I2C device work which did not work before?

Most I2C devices always ack their address, so NACK very often means
"nothing is there". I think it makes sense that the rcar driver returns
ENXIO in this case which is documented to be used for NACK after address
phase. Then, the i2c client driver should know if this means "not there"
or "currently busy". And it should know when is a good time for another
try. As I read the patch, the driver would use the auto-restart feature
until the timeout is reached. That would make bus scanning pretty slow,
too.
Wolfram Sang March 27, 2015, 1:11 p.m. UTC | #4 
On Fri, Mar 06, 2015 at 11:18:09PM +0100, Wolfram Sang wrote:
> 
> > > >Even if R-Car I2C received NACK, after that it might receive ACK
> > > >by HW auto restart. In case of that, driver would continue process.
> > > >If R-Car I2C didn't receive ACK, the driver would detect timeout
> > > >and would report NACK as -ENXIO.
> > > >
> > > >Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
> > > >Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > > 
> > > Excuse me, but what exactly is HW auto restart in this case? Is it a feature
> > > of the I2C slave?
> > 
> > I asked Kataoka-san about this and his response was as follows:
> > 
> >     It is a feature of the i2c-rcar(H/W) master.
> > 
> >     If system(CPU) is busy, NACK procedure may have interrupt latency.
> >     Since the clear of ICMCR.ESG bit is delayed, i2c-rcar(H/W) may auto-restart
> >     after NACK. Please refer to ESG bit of H/W UM section 55.3.5.
> > 
> >     For example, this is I2C write transmitting.
> >     1.Start / 2.SlaveAddr,ACK / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop
> > 
> >     If No.2 has NACK and interruption has delay, this transmitting is as follows.
> >     1.Start / 2.SlaveAddr,NACK/ 1x.auto-restart / 2x.SlaveAddr,ACK
> >                                     / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop
> > 
> >     NACK of No.2 is invalidated by ACK of No.2x. It means recover.
> 
> Does this make some I2C device work which did not work before?
> 
> Most I2C devices always ack their address, so NACK very often means
> "nothing is there". I think it makes sense that the rcar driver returns
> ENXIO in this case which is documented to be used for NACK after address
> phase. Then, the i2c client driver should know if this means "not there"
> or "currently busy". And it should know when is a good time for another
> try. As I read the patch, the driver would use the auto-restart feature
> until the timeout is reached. That would make bus scanning pretty slow,
> too.

Hi,

any news on this one?

Kind regards,

   Wolfram
Simon Horman March 27, 2015, 10:04 p.m. UTC | #5 
Hi Wolfram,

On Fri, Mar 27, 2015 at 02:11:04PM +0100, Wolfram Sang wrote:
> On Fri, Mar 06, 2015 at 11:18:09PM +0100, Wolfram Sang wrote:
> > 
> > > > >Even if R-Car I2C received NACK, after that it might receive ACK
> > > > >by HW auto restart. In case of that, driver would continue process.
> > > > >If R-Car I2C didn't receive ACK, the driver would detect timeout
> > > > >and would report NACK as -ENXIO.
> > > > >
> > > > >Signed-off-by: Ryo Kataoka <ryo.kataoka.wt@renesas.com>
> > > > >Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > > > 
> > > > Excuse me, but what exactly is HW auto restart in this case? Is it a feature
> > > > of the I2C slave?
> > > 
> > > I asked Kataoka-san about this and his response was as follows:
> > > 
> > >     It is a feature of the i2c-rcar(H/W) master.
> > > 
> > >     If system(CPU) is busy, NACK procedure may have interrupt latency.
> > >     Since the clear of ICMCR.ESG bit is delayed, i2c-rcar(H/W) may auto-restart
> > >     after NACK. Please refer to ESG bit of H/W UM section 55.3.5.
> > > 
> > >     For example, this is I2C write transmitting.
> > >     1.Start / 2.SlaveAddr,ACK / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop
> > > 
> > >     If No.2 has NACK and interruption has delay, this transmitting is as follows.
> > >     1.Start / 2.SlaveAddr,NACK/ 1x.auto-restart / 2x.SlaveAddr,ACK
> > >                                     / 3.RegAddr,ACK / 4.RegData,ACK / 5.Stop
> > > 
> > >     NACK of No.2 is invalidated by ACK of No.2x. It means recover.
> > 
> > Does this make some I2C device work which did not work before?
> > 
> > Most I2C devices always ack their address, so NACK very often means
> > "nothing is there". I think it makes sense that the rcar driver returns
> > ENXIO in this case which is documented to be used for NACK after address
> > phase. Then, the i2c client driver should know if this means "not there"
> > or "currently busy". And it should know when is a good time for another
> > try. As I read the patch, the driver would use the auto-restart feature
> > until the timeout is reached. That would make bus scanning pretty slow,
> > too.
> 
> Hi,
> 
> any news on this one?

Sorry for not responding earlier. I have the following information from
Kataoka-san.

* There are now several patches in the BSP that address this issue;
  this patch and two follow up fix patches. Its probably best if these
  patches were all squashed into a single patch for your consideration.

  For reference I am referring to the following patches in the BSP:

  i2c: rcar: Support ACK by HW auto restart after NACK
  i2c: rcar: Fix status clear after NACK
  i2c: rcar: Fix flag clear for recovering by HW auto restart

* In answer to your questions above:

  "When NACK interruption occurs, the i2c rcar driver sets ICMCR register
   from (MDBS|MIE|ESG) to (MDBS|MIE|FSB) in rcar_i2c_bus_phase().  It means
   clearing of ESG bit. If this procedure is late, H/W auto-restarts.  After
   that, if NACK occurs again, H/W doesn't auto-restart because ESG bit was
   already cleared.

   S/W doesn't know whether H/W did auto-restart or did not auto-restart.
   So, S/W needs to wait. The return of ENXIO is pretty slow.

   It takes long time but I think it's ok because NACK is unjust case."
--
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
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 71a6e07..dde8abf 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -455,14 +455,20 @@  static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	if (msr & MNR) {
 		/* go to stop phase */
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-		rcar_i2c_write(priv, ICMIER, RCAR_IRQ_STOP);
 		rcar_i2c_flags_set(priv, ID_NACK);
 		goto out;
 	}
 
 	/* Stop */
 	if (msr & MST) {
-		rcar_i2c_flags_set(priv, ID_DONE);
+		if (rcar_i2c_flags_has(priv, ID_NACK)) {
+			/* don't set ID_DONE for expecting ACK
+					after auto-restart by HW */
+			rcar_i2c_write(priv, ICMSR, ~MST);
+		} else {
+			rcar_i2c_flags_set(priv, ID_DONE);
+			rcar_i2c_write(priv, ICMSR, 0);
+		}
 		goto out;
 	}
 
@@ -536,15 +542,14 @@  static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 					     rcar_i2c_flags_has(priv, ID_DONE),
 					     5 * HZ);
 		if (!timeout) {
+			if (rcar_i2c_flags_has(priv, ID_NACK)) {
+				ret = -ENXIO;
+				break;
+			}
 			ret = -ETIMEDOUT;
 			break;
 		}
 
-		if (rcar_i2c_flags_has(priv, ID_NACK)) {
-			ret = -ENXIO;
-			break;
-		}
-
 		if (rcar_i2c_flags_has(priv, ID_ARBLOST)) {
 			ret = -EAGAIN;
 			break;