[RFC PATCH 0/1] i2c: rcar: handle RXDMA HW bug on Gen3

[RFC PATCH 0/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

As mentioned in the patch description, this patch works around a HW bug on
Renesas R-Car Gen3 SoCs. It had not been discovered yet because it only happens
when clearing DMA enable bits is too late because of latency. To reproduce the
problem reliably, the below patch was used [1].

To the best of my knowledge, all Gen3 SoCs are affected, so I didn't add any
soc_device_match kind of matching for now. If the situation changes, we can add
it. For now, priv->devtype is good.

The improvement can be shown by using the following command. Before the patch,
executed on a Salvator-X / R-Car M3-W ES1.0:

# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00
# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00
0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00

Note the broken first byte (0xfe) after the first successful RXDMA transfer.
After the patch (tested with and without CONFIG_RESET_CONTROLLER):

# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
# i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00
0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00

Looks way better!

Patch was tested with current renesas-drivers/master, but should also apply cleanly
on i2c/for-next or v4.17-rc7. Please test and comment.

Kind regards,

   Wolfram


Wolfram Sang (1):
  i2c: rcar: handle RXDMA HW bug on Gen3

 drivers/i2c/busses/i2c-rcar.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)


[1] Testcase patch:

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d445b5891280..e42cd0121862 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -353,6 +353,7 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
 	struct dma_chan *chan = priv->dma_direction == DMA_FROM_DEVICE
 		? priv->dma_rx : priv->dma_tx;
 
+	mdelay(10);
 	/* Disable DMA Master Received/Transmitted */
 	rcar_i2c_write(priv, ICDMAER, 0);
 
-- 
2.11.0

[RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

On Gen3, we can only do RXDMA once per transfer reliably. For that, we
must reset the device, then we can have RXDMA once. This patch
implements this. When there is no reset controller or the reset fails,
RXDMA will be blocked completely. Otherwise, it will be disabled after
the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu
Yamasaki, yet completely refactored to handle multiple read messages
within one transfer.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e42cd0121862..27277f42710d 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -24,6 +24,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 
 /* register offsets */
@@ -103,8 +104,9 @@
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
 /* persistent flags */
+#define ID_P_NO_RXDMA	(1 << 30) /* HW bug forbids RXDMA sometimes */
 #define ID_P_PM_BLOCKED	(1 << 31)
-#define ID_P_MASK	ID_P_PM_BLOCKED
+#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NO_RXDMA)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -133,6 +135,8 @@ struct rcar_i2c_priv {
 	struct dma_chan *dma_rx;
 	struct scatterlist sg;
 	enum dma_data_direction dma_direction;
+
+	struct reset_control *rstc;
 };
 
 #define rcar_i2c_priv_to_dev(p)		((p)->adap.dev.parent)
@@ -363,6 +367,11 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
 	dma_unmap_single(chan->device->dev, sg_dma_address(&priv->sg),
 			 sg_dma_len(&priv->sg), priv->dma_direction);
 
+	/* Gen3 can only do one RXDMA per transfer */
+	if (priv->devtype == I2C_RCAR_GEN3 &&
+	    priv->dma_direction == DMA_FROM_DEVICE)
+		priv->flags |= ID_P_NO_RXDMA;
+
 	priv->dma_direction = DMA_NONE;
 }
 
@@ -400,8 +409,9 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv)
 	unsigned char *buf;
 	int len;
 
-	/* Do not use DMA if it's not available or for messages < 8 bytes */
-	if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE))
+	/* Do various checks to see if DMA is feasible at all */
+	if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE) ||
+	    (read && priv->flags & ID_P_NO_RXDMA))
 		return;
 
 	if (read) {
@@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 	pm_runtime_get_sync(dev);
 
+	/* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
+	if (priv->devtype == I2C_RCAR_GEN3) {
+		priv->flags |= ID_P_NO_RXDMA;
+		if (!IS_ERR(priv->rstc)) {
+			ret = reset_control_reset(priv->rstc);
+			if (ret == 0)
+				priv->flags &= ~ID_P_NO_RXDMA;
+		}
+	}
+
 	rcar_i2c_init(priv);
 
 	ret = rcar_i2c_bus_barrier(priv);
@@ -913,6 +933,9 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto out_pm_put;
 
+	if (priv->devtype == I2C_RCAR_GEN3)
+		priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
+
 	/* Stay always active when multi-master to keep arbitration working */
 	if (of_property_read_bool(dev->of_node, "multi-master"))
 		priv->flags |= ID_P_PM_BLOCKED;
-- 
2.11.0

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM
> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

If possible, I'd like to replace "bug" with "specification" or other words :)

<snip>
> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
> 
>  	pm_runtime_get_sync(dev);
> 
> +	/* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
> +	if (priv->devtype == I2C_RCAR_GEN3) {
> +		priv->flags |= ID_P_NO_RXDMA;
> +		if (!IS_ERR(priv->rstc)) {
> +			ret = reset_control_reset(priv->rstc);

According to the datasheet Rev.1.00 page 57-69, we should do:
	reset_control_assert();
	udelay(1);
	reset_control_deassert();
	while (reset_control_status())
		;
instead of reset_control_reset(), I think.

Best regards,
Yoshihiro Shimoda

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

Hello Shimoda-san,

> > Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
> 
> If possible, I'd like to replace "bug" with "specification" or other words :)

"behaviour" maybe is a better word?

> > +	/* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
> > +	if (priv->devtype == I2C_RCAR_GEN3) {
> > +		priv->flags |= ID_P_NO_RXDMA;
> > +		if (!IS_ERR(priv->rstc)) {
> > +			ret = reset_control_reset(priv->rstc);
> 
> According to the datasheet Rev.1.00 page 57-69, we should do:
> 	reset_control_assert();
> 	udelay(1);
> 	reset_control_deassert();
> 	while (reset_control_status())
> 		;
> instead of reset_control_reset(), I think.

I was following Geert's suggestion here from the mail thread '[periperi] About
BSP patch "i2c: rcar: Fix I2C DMA reception by adding reset':

===

>> reset_control_assert() + reset_control_deassert() can be replaced by
>> reset_control_assert().
>
> Do you mean 'reset_control_reset' maybe? I am not sure, I don't know
> this API very well... but two time *_assert looks suspicious.

Of course. Silly c&p.

>> In addition, that will make sure the delay of one cycle of the RCLK clock
>> is taken into account, which is not the case with the current code.
> 
> I guess this is why there is now this patch in the BSP which Shimoda-san
> mentioned in a later mail:
>
> f0cd22525f73 ("i2c: rcar: Fix module reset function for hardware specification")

Exactly.

===

As far as I understood it takes care of the proper reset mechanism with the delay?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Geert Uytterhoeven]

Hi Shimoda-san,

On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM
>> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
>
> If possible, I'd like to replace "bug" with "specification" or other words :)
>
> <snip>
>> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>>
>>       pm_runtime_get_sync(dev);
>>
>> +     /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
>> +     if (priv->devtype == I2C_RCAR_GEN3) {
>> +             priv->flags |= ID_P_NO_RXDMA;
>> +             if (!IS_ERR(priv->rstc)) {
>> +                     ret = reset_control_reset(priv->rstc);
>
> According to the datasheet Rev.1.00 page 57-69, we should do:
>         reset_control_assert();
>         udelay(1);
>         reset_control_deassert();
>         while (reset_control_status())
>                 ;
> instead of reset_control_reset(), I think.

The i2c-specific procedure documented at page 57-69 thus differs from
the generic one at page 8A-58, which is what cpg_mssr_reset() implements.

The latter waits 35µs instead of 1µs, so that should be safe.
But it doesn't check the status bit. Is the longer delay sufficient, or should
a check polling the status bit be added to cpg_mssr_reset()?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hi Geert-san, Wolfram-san,

> From: Geert Uytterhoeven, Sent: Thursday, May 31, 2018 5:45 PM
> 
> Hi Shimoda-san,
> 
> On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM
> >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
> >
> > If possible, I'd like to replace "bug" with "specification" or other words :)
> >
> > <snip>
> >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
> >>
> >>       pm_runtime_get_sync(dev);
> >>
> >> +     /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
> >> +     if (priv->devtype == I2C_RCAR_GEN3) {
> >> +             priv->flags |= ID_P_NO_RXDMA;
> >> +             if (!IS_ERR(priv->rstc)) {
> >> +                     ret = reset_control_reset(priv->rstc);
> >
> > According to the datasheet Rev.1.00 page 57-69, we should do:
> >         reset_control_assert();
> >         udelay(1);
> >         reset_control_deassert();
> >         while (reset_control_status())
> >                 ;
> > instead of reset_control_reset(), I think.
> 
> The i2c-specific procedure documented at page 57-69 thus differs from
> the generic one at page 8A-58, which is what cpg_mssr_reset() implements.
> 
> The latter waits 35µs instead of 1µs, so that should be safe.
> But it doesn't check the status bit. Is the longer delay sufficient, or should
> a check polling the status bit be added to cpg_mssr_reset()?

Thank you for the pointed out.
I agree we should wait 35us for safe.
But, anyway I'll ask HW team about this contradiction and really need polling the status.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hello Wolfram-san,

> From: Wolfram Sang, Sent: Thursday, May 31, 2018 5:31 PM
> 
> Hello Shimoda-san,
> 
> > > Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
> >
> > If possible, I'd like to replace "bug" with "specification" or other words :)
> 
> "behaviour" maybe is a better word?

It sounds good to me.

> > > +	/* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
> > > +	if (priv->devtype == I2C_RCAR_GEN3) {
> > > +		priv->flags |= ID_P_NO_RXDMA;
> > > +		if (!IS_ERR(priv->rstc)) {
> > > +			ret = reset_control_reset(priv->rstc);
> >
> > According to the datasheet Rev.1.00 page 57-69, we should do:
> > 	reset_control_assert();
> > 	udelay(1);
> > 	reset_control_deassert();
> > 	while (reset_control_status())
> > 		;
> > instead of reset_control_reset(), I think.
> 
> I was following Geert's suggestion here from the mail thread '[periperi] About
> BSP patch "i2c: rcar: Fix I2C DMA reception by adding reset':
> 
> ===
> 
> >> reset_control_assert() + reset_control_deassert() can be replaced by
> >> reset_control_assert().
> >
> > Do you mean 'reset_control_reset' maybe? I am not sure, I don't know
> > this API very well... but two time *_assert looks suspicious.
> 
> Of course. Silly c&p.
> 
> >> In addition, that will make sure the delay of one cycle of the RCLK clock
> >> is taken into account, which is not the case with the current code.
> >
> > I guess this is why there is now this patch in the BSP which Shimoda-san
> > mentioned in a later mail:
> >
> > f0cd22525f73 ("i2c: rcar: Fix module reset function for hardware specification")
> 
> Exactly.
> 
> ===
> 
> As far as I understood it takes care of the proper reset mechanism with the delay?

I missed this email... As I replied to Geert-san on other email thread,
I'll ask HW team about this topic.

Best regards,
Yoshihiro Shimoda

> Kind regards,
> 
>    Wolfram

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Yoshihiro Shimoda, Sent: Thursday, May 31, 2018 6:12 PM
> 
> Hi Geert-san, Wolfram-san,
> 
> > From: Geert Uytterhoeven, Sent: Thursday, May 31, 2018 5:45 PM
> >
> > Hi Shimoda-san,
> >
> > On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM
> > >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
> > >
> > > If possible, I'd like to replace "bug" with "specification" or other words :)
> > >
> > > <snip>
> > >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
> > >>
> > >>       pm_runtime_get_sync(dev);
> > >>
> > >> +     /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
> > >> +     if (priv->devtype == I2C_RCAR_GEN3) {
> > >> +             priv->flags |= ID_P_NO_RXDMA;
> > >> +             if (!IS_ERR(priv->rstc)) {
> > >> +                     ret = reset_control_reset(priv->rstc);
> > >
> > > According to the datasheet Rev.1.00 page 57-69, we should do:
> > >         reset_control_assert();
> > >         udelay(1);
> > >         reset_control_deassert();
> > >         while (reset_control_status())
> > >                 ;
> > > instead of reset_control_reset(), I think.
> >
> > The i2c-specific procedure documented at page 57-69 thus differs from
> > the generic one at page 8A-58, which is what cpg_mssr_reset() implements.
> >
> > The latter waits 35µs instead of 1µs, so that should be safe.
> > But it doesn't check the status bit. Is the longer delay sufficient, or should
> > a check polling the status bit be added to cpg_mssr_reset()?
> 
> Thank you for the pointed out.
> I agree we should wait 35us for safe.
> But, anyway I'll ask HW team about this contradiction and really need polling the status.

I got information about this topic.

< In CPG / MSSR point of view >
 - This needs 35 usec wait while asserting.
 - After deasserted a reset, no wait needs.
  - In other words, there is each hardware IP dependence.

< In I2C point of view >
 - After deasserted the reset, this nees SRCR register polling.

So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset.
But, what do you think?

Best regards,
Yoshihiro Shimoda

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Jun 7, 2018 at 7:36 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Yoshihiro Shimoda, Sent: Thursday, May 31, 2018 6:12 PM
>> > From: Geert Uytterhoeven, Sent: Thursday, May 31, 2018 5:45 PM
>> > On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda
>> > <yoshihiro.shimoda.uh@renesas.com> wrote:
>> > >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM
>> > >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3
>> > >
>> > > If possible, I'd like to replace "bug" with "specification" or other words :)
>> > >
>> > > <snip>
>> > >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>> > >>
>> > >>       pm_runtime_get_sync(dev);
>> > >>
>> > >> +     /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */
>> > >> +     if (priv->devtype == I2C_RCAR_GEN3) {
>> > >> +             priv->flags |= ID_P_NO_RXDMA;
>> > >> +             if (!IS_ERR(priv->rstc)) {
>> > >> +                     ret = reset_control_reset(priv->rstc);
>> > >
>> > > According to the datasheet Rev.1.00 page 57-69, we should do:
>> > >         reset_control_assert();
>> > >         udelay(1);
>> > >         reset_control_deassert();
>> > >         while (reset_control_status())
>> > >                 ;
>> > > instead of reset_control_reset(), I think.
>> >
>> > The i2c-specific procedure documented at page 57-69 thus differs from
>> > the generic one at page 8A-58, which is what cpg_mssr_reset() implements.
>> >
>> > The latter waits 35µs instead of 1µs, so that should be safe.
>> > But it doesn't check the status bit. Is the longer delay sufficient, or should
>> > a check polling the status bit be added to cpg_mssr_reset()?
>>
>> Thank you for the pointed out.
>> I agree we should wait 35us for safe.
>> But, anyway I'll ask HW team about this contradiction and really need polling the status.
>
> I got information about this topic.
>
> < In CPG / MSSR point of view >
>  - This needs 35 usec wait while asserting.
>  - After deasserted a reset, no wait needs.
>   - In other words, there is each hardware IP dependence.

Let's call the above procedure A.

> < In I2C point of view >
>  - After deasserted the reset, this nees SRCR register polling.

Let's call the above procedure B.

> So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset.
> But, what do you think?

cpg_mssr_reset() indeed does not check the status bit after deasserting
the reset, as it follows procedure A.

Such a check could be added, though. Then it'll become
(let's call this procedure C):

        /* Reset module */
        spin_lock_irqsave(&priv->rmw_lock, flags);
        value = readl(priv->base + SRCR(reg));
        value |= bitmask;
        writel(value, priv->base + SRCR(reg));
        spin_unlock_irqrestore(&priv->rmw_lock, flags);

        /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
        udelay(35);

        /* Release module from reset state */
        writel(bitmask, priv->base + SRSTCLR(reg));

+       /* Wait until deassertion has completed */
+       while (readl(priv->base + SRCR(reg)) & bitmask)
+               cpu_relax();

Probably we need an upper limit on the number of loops, and call udelay(1)
after each iteration?

        for (i 0; i < 35; i++) {
                if (!(readl(priv->base + SRCR(reg)) & bitmask))
                        return 0;
                udelay(1);
        }
        return -EIO;

Any advice from the hardware team about that?

But according to procedure A, the check is not needed?
Probably because 35µs is an upper limit satisfying all individual hardware
modules?

I'm wondering whether we could use procedure B in the general case,
as it explicitly checks for completion?

Procedure C is safest, though, so probably the way to go.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]


> > I got information about this topic.
> >
> > < In CPG / MSSR point of view >
> >  - This needs 35 usec wait while asserting.
> >  - After deasserted a reset, no wait needs.
> >   - In other words, there is each hardware IP dependence.
> 
> Let's call the above procedure A.
> 
> > < In I2C point of view >
> >  - After deasserted the reset, this nees SRCR register polling.
> 
> Let's call the above procedure B.
> 
> > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset.
> > But, what do you think?
> 
> cpg_mssr_reset() indeed does not check the status bit after deasserting
> the reset, as it follows procedure A.
> 
> Such a check could be added, though. Then it'll become
> (let's call this procedure C):
> 
>         /* Reset module */
>         spin_lock_irqsave(&priv->rmw_lock, flags);
>         value = readl(priv->base + SRCR(reg));
>         value |= bitmask;
>         writel(value, priv->base + SRCR(reg));
>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> 
>         /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
>         udelay(35);
> 
>         /* Release module from reset state */
>         writel(bitmask, priv->base + SRSTCLR(reg));
> 
> +       /* Wait until deassertion has completed */
> +       while (readl(priv->base + SRCR(reg)) & bitmask)
> +               cpu_relax();
> 
> Probably we need an upper limit on the number of loops, and call udelay(1)
> after each iteration?
> 
>         for (i 0; i < 35; i++) {
>                 if (!(readl(priv->base + SRCR(reg)) & bitmask))
>                         return 0;
>                 udelay(1);
>         }
>         return -EIO;
> 
> Any advice from the hardware team about that?
> 
> But according to procedure A, the check is not needed?
> Probably because 35µs is an upper limit satisfying all individual hardware
> modules?
> 
> I'm wondering whether we could use procedure B in the general case,
> as it explicitly checks for completion?
> 
> Procedure C is safest, though, so probably the way to go.

Any news about this topic?

And how to upstream all this? My I2C patch is clearly a bugfix, but the
necessary CPG update technically isn't? Not sure about this one...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hi Wolfram-san, Geert-san,

I'm sorry for delayed response. I completely overlooked this email...

> From: Wolfram Sang <wsa@the-dreams.de>, Sent: Tuesday, June 26, 2018 12:05 PM
> 
> > > I got information about this topic.
> > >
> > > < In CPG / MSSR point of view >
> > >  - This needs 35 usec wait while asserting.
> > >  - After deasserted a reset, no wait needs.
> > >   - In other words, there is each hardware IP dependence.
> >
> > Let's call the above procedure A.
> >
> > > < In I2C point of view >
> > >  - After deasserted the reset, this nees SRCR register polling.
> >
> > Let's call the above procedure B.
> >
> > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset.
> > > But, what do you think?
> >
> > cpg_mssr_reset() indeed does not check the status bit after deasserting
> > the reset, as it follows procedure A.
> >
> > Such a check could be added, though. Then it'll become
> > (let's call this procedure C):
> >
> >         /* Reset module */
> >         spin_lock_irqsave(&priv->rmw_lock, flags);
> >         value = readl(priv->base + SRCR(reg));
> >         value |= bitmask;
> >         writel(value, priv->base + SRCR(reg));
> >         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >
> >         /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
> >         udelay(35);
> >
> >         /* Release module from reset state */
> >         writel(bitmask, priv->base + SRSTCLR(reg));
> >
> > +       /* Wait until deassertion has completed */
> > +       while (readl(priv->base + SRCR(reg)) & bitmask)
> > +               cpu_relax();
> > 
> > Probably we need an upper limit on the number of loops, and call udelay(1)
> > after each iteration?
> >
> >         for (i 0; i < 35; i++) {
> >                 if (!(readl(priv->base + SRCR(reg)) & bitmask))
> >                         return 0;
> >                 udelay(1);
> >         }
> >         return -EIO;
> >
> > Any advice from the hardware team about that?

The hardware team said:
 - In CPG point of view:
   - such polling doesn't need (because the reset pulse is generated correctly).
   - About the interval after deassert the reset, this is depend on each hardware module.
     (In other words, if each hardware module has a special handling about after the deassert interval,
      we should follow the special handling.)
 - The I2C controller needs an interval of reading SRCR at least (this is a special handling).

So, I think adding this code is not good fit in CPG point of view.

> > But according to procedure A, the check is not needed?

As hardware team said, the check (that means procedure C) is not needed.

> > Probably because 35µs is an upper limit satisfying all individual hardware
> > modules?
> >
> > I'm wondering whether we could use procedure B in the general case,
> > as it explicitly checks for completion?
> >
> > Procedure C is safest, though, so probably the way to go.
> 
> Any news about this topic?
> 
> And how to upstream all this? My I2C patch is clearly a bugfix, but the
> necessary CPG update technically isn't? Not sure about this one...

I think we have to add reset_control_status() calling into the i2c-rcar.c
to follow procedure B.
But, Geert-san, what do you think?

Best regards,
Yoshihiro Shimoda

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Geert Uytterhoeven]

Hi Shimoda-san,

On Tue, Jun 26, 2018 at 10:38 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Wolfram Sang <wsa@the-dreams.de>, Sent: Tuesday, June 26, 2018 12:05 PM
> > > > I got information about this topic.
> > > >
> > > > < In CPG / MSSR point of view >
> > > >  - This needs 35 usec wait while asserting.
> > > >  - After deasserted a reset, no wait needs.
> > > >   - In other words, there is each hardware IP dependence.
> > >
> > > Let's call the above procedure A.
> > >
> > > > < In I2C point of view >
> > > >  - After deasserted the reset, this nees SRCR register polling.
> > >
> > > Let's call the above procedure B.
> > >
> > > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset.
> > > > But, what do you think?
> > >
> > > cpg_mssr_reset() indeed does not check the status bit after deasserting
> > > the reset, as it follows procedure A.
> > >
> > > Such a check could be added, though. Then it'll become
> > > (let's call this procedure C):
> > >
> > >         /* Reset module */
> > >         spin_lock_irqsave(&priv->rmw_lock, flags);
> > >         value = readl(priv->base + SRCR(reg));
> > >         value |= bitmask;
> > >         writel(value, priv->base + SRCR(reg));
> > >         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > >
> > >         /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
> > >         udelay(35);
> > >
> > >         /* Release module from reset state */
> > >         writel(bitmask, priv->base + SRSTCLR(reg));
> > >
> > > +       /* Wait until deassertion has completed */
> > > +       while (readl(priv->base + SRCR(reg)) & bitmask)
> > > +               cpu_relax();
> > >
> > > Probably we need an upper limit on the number of loops, and call udelay(1)
> > > after each iteration?
> > >
> > >         for (i 0; i < 35; i++) {
> > >                 if (!(readl(priv->base + SRCR(reg)) & bitmask))
> > >                         return 0;
> > >                 udelay(1);
> > >         }
> > >         return -EIO;
> > >
> > > Any advice from the hardware team about that?
>
> The hardware team said:
>  - In CPG point of view:
>    - such polling doesn't need (because the reset pulse is generated correctly).
>    - About the interval after deassert the reset, this is depend on each hardware module.
>      (In other words, if each hardware module has a special handling about after the deassert interval,
>       we should follow the special handling.)
>  - The I2C controller needs an interval of reading SRCR at least (this is a special handling).
>
> So, I think adding this code is not good fit in CPG point of view.
>
> > > But according to procedure A, the check is not needed?
>
> As hardware team said, the check (that means procedure C) is not needed.
>
> > > Probably because 35µs is an upper limit satisfying all individual hardware
> > > modules?
> > >
> > > I'm wondering whether we could use procedure B in the general case,
> > > as it explicitly checks for completion?
> > >
> > > Procedure C is safest, though, so probably the way to go.
> >
> > Any news about this topic?
> >
> > And how to upstream all this? My I2C patch is clearly a bugfix, but the
> > necessary CPG update technically isn't? Not sure about this one...
>
> I think we have to add reset_control_status() calling into the i2c-rcar.c
> to follow procedure B.
> But, Geert-san, what do you think?

Calling reset_control_status() from i2c-rcar is fine for me.

Note that reset controller support is optional, so we want to add

        select RESET_CONTROLLER if ARCH_RENESAS && ARM64

to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
silently.

This hardware bug is restricted to R-Car Gen3?
If it applies to R-Car Gen2, too, the "&& ARM64" has to be dropped.
If it applies to R-Car Gen1, too, we have to write an R-Car Gen1 reset
controller driver (R-Car Gen1 uses the legacy CPG and MSTP clock drivers,
and cannot be migrated to the CPG/MSSR driver, as its MSTP and RESET
functionalities are in separate modules).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, June 26, 2018 5:58 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Jun 26, 2018 at 10:38 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Wolfram Sang <wsa@the-dreams.de>, Sent: Tuesday, June 26, 2018 12:05 PM
> > > > > I got information about this topic.
> > > > >
> > > > > < In CPG / MSSR point of view >
> > > > >  - This needs 35 usec wait while asserting.
> > > > >  - After deasserted a reset, no wait needs.
> > > > >   - In other words, there is each hardware IP dependence.
> > > >
> > > > Let's call the above procedure A.
> > > >
> > > > > < In I2C point of view >
> > > > >  - After deasserted the reset, this nees SRCR register polling.
> > > >
> > > > Let's call the above procedure B.
> > > >
> > > > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset.
> > > > > But, what do you think?
> > > >
> > > > cpg_mssr_reset() indeed does not check the status bit after deasserting
> > > > the reset, as it follows procedure A.
> > > >
> > > > Such a check could be added, though. Then it'll become
> > > > (let's call this procedure C):
> > > >
> > > >         /* Reset module */
> > > >         spin_lock_irqsave(&priv->rmw_lock, flags);
> > > >         value = readl(priv->base + SRCR(reg));
> > > >         value |= bitmask;
> > > >         writel(value, priv->base + SRCR(reg));
> > > >         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > >
> > > >         /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
> > > >         udelay(35);
> > > >
> > > >         /* Release module from reset state */
> > > >         writel(bitmask, priv->base + SRSTCLR(reg));
> > > >
> > > > +       /* Wait until deassertion has completed */
> > > > +       while (readl(priv->base + SRCR(reg)) & bitmask)
> > > > +               cpu_relax();
> > > >
> > > > Probably we need an upper limit on the number of loops, and call udelay(1)
> > > > after each iteration?
> > > >
> > > >         for (i 0; i < 35; i++) {
> > > >                 if (!(readl(priv->base + SRCR(reg)) & bitmask))
> > > >                         return 0;
> > > >                 udelay(1);
> > > >         }
> > > >         return -EIO;
> > > >
> > > > Any advice from the hardware team about that?
> >
> > The hardware team said:
> >  - In CPG point of view:
> >    - such polling doesn't need (because the reset pulse is generated correctly).
> >    - About the interval after deassert the reset, this is depend on each hardware module.
> >      (In other words, if each hardware module has a special handling about after the deassert interval,
> >       we should follow the special handling.)
> >  - The I2C controller needs an interval of reading SRCR at least (this is a special handling).
> >
> > So, I think adding this code is not good fit in CPG point of view.
> >
> > > > But according to procedure A, the check is not needed?
> >
> > As hardware team said, the check (that means procedure C) is not needed.
> >
> > > > Probably because 35µs is an upper limit satisfying all individual hardware
> > > > modules?
> > > >
> > > > I'm wondering whether we could use procedure B in the general case,
> > > > as it explicitly checks for completion?
> > > >
> > > > Procedure C is safest, though, so probably the way to go.
> > >
> > > Any news about this topic?
> > >
> > > And how to upstream all this? My I2C patch is clearly a bugfix, but the
> > > necessary CPG update technically isn't? Not sure about this one...
> >
> > I think we have to add reset_control_status() calling into the i2c-rcar.c
> > to follow procedure B.
> > But, Geert-san, what do you think?
> 
> Calling reset_control_status() from i2c-rcar is fine for me.

Thank you for your comment!

> Note that reset controller support is optional, so we want to add
> 
>         select RESET_CONTROLLER if ARCH_RENESAS && ARM64
> 
> to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> silently.

Thanks for this information. It's helpful to me because I'll add reset
controller support on renesas_usbhs driver later.

> This hardware bug is restricted to R-Car Gen3?

Yes, this is hardware "behaviour" on R-Car Gen3 :)
And older SoCs' I2C doesn't support DMA transfer.

> If it applies to R-Car Gen2, too, the "&& ARM64" has to be dropped.
> If it applies to R-Car Gen1, too, we have to write an R-Car Gen1 reset
> controller driver (R-Car Gen1 uses the legacy CPG and MSTP clock drivers,
> and cannot be migrated to the CPG/MSSR driver, as its MSTP and RESET
> functionalities are in separate modules).

So, they are not needed, I think.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

Hi,

thanks for the discussion on this topic!

> > The hardware team said:
> >  - In CPG point of view:
> >    - such polling doesn't need (because the reset pulse is generated correctly).
> >    - About the interval after deassert the reset, this is depend on each hardware module.
> >      (In other words, if each hardware module has a special handling about after the deassert interval,
> >       we should follow the special handling.)
> >  - The I2C controller needs an interval of reading SRCR at least (this is a special handling).
> >
> > So, I think adding this code is not good fit in CPG point of view.
> 
> Calling reset_control_status() from i2c-rcar is fine for me.
> 

Doesn't make this writing device drivers a bit harder, I wonder? If we
follow the above, we need to know per driver (like i2c-rcar.c) if
reset_control_reset() is enough or if we need to call
reset_control_status() additionally. For a driver author, it would be
much less error prone IMHO, if reset_control_reset() just does the right
thing. It has also the advantage that we can handle special handling on
different SoCs differently (if that is ever needed) because MSSR driver
is per SoC.

Where is this special handling documented BTW, I can't find it. The BSP
waits for maximum 1024ms which seems excessive to me.

> Note that reset controller support is optional, so we want to add
> 
>         select RESET_CONTROLLER if ARCH_RENESAS && ARM64
> 
> to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> silently.

Technically, it should also be "&& HAS_DMA". But this is maybe a tad too
defensive?

> This hardware bug is restricted to R-Car Gen3?

As already mentioned by Shimoda-san, only Gen3 has DMA support.

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Jun 27, 2018 at 10:44 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > The hardware team said:
> > >  - In CPG point of view:
> > >    - such polling doesn't need (because the reset pulse is generated correctly).
> > >    - About the interval after deassert the reset, this is depend on each hardware module.
> > >      (In other words, if each hardware module has a special handling about after the deassert interval,
> > >       we should follow the special handling.)
> > >  - The I2C controller needs an interval of reading SRCR at least (this is a special handling).
> > >
> > > So, I think adding this code is not good fit in CPG point of view.
> >
> > Calling reset_control_status() from i2c-rcar is fine for me.
> >
>
> Doesn't make this writing device drivers a bit harder, I wonder? If we
> follow the above, we need to know per driver (like i2c-rcar.c) if
> reset_control_reset() is enough or if we need to call
> reset_control_status() additionally. For a driver author, it would be
> much less error prone IMHO, if reset_control_reset() just does the right
> thing. It has also the advantage that we can handle special handling on
> different SoCs differently (if that is ever needed) because MSSR driver
> is per SoC.

True.

But this seems to be a bug in the R-Car Gen3 I2C controller implementation.

> > Note that reset controller support is optional, so we want to add
> >
> >         select RESET_CONTROLLER if ARCH_RENESAS && ARM64
> >
> > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> > silently.
>
> Technically, it should also be "&& HAS_DMA". But this is maybe a tad too
> defensive?

s/HAS_DMA/RCAR_DMAC/ :-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]


> > Doesn't make this writing device drivers a bit harder, I wonder? If we
> > follow the above, we need to know per driver (like i2c-rcar.c) if
> > reset_control_reset() is enough or if we need to call
> > reset_control_status() additionally. For a driver author, it would be
> > much less error prone IMHO, if reset_control_reset() just does the right
> > thing. It has also the advantage that we can handle special handling on
> > different SoCs differently (if that is ever needed) because MSSR driver
> > is per SoC.
> 
> True.
> 
> But this seems to be a bug in the R-Car Gen3 I2C controller implementation.

I see. If this is more of "a bug in the IP core", then I can live with
having the workaround in the I2C driver. With a comment explaining the
situation, of course.

If it was more like "intended" special handling needed for different IP
cores (not only I2C), then we should maybe reconsider.

> > > Note that reset controller support is optional, so we want to add
> > >
> > >         select RESET_CONTROLLER if ARCH_RENESAS && ARM64
> > >
> > > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> > > silently.
> >
> > Technically, it should also be "&& HAS_DMA". But this is maybe a tad too
> > defensive?
> 
> s/HAS_DMA/RCAR_DMAC/ :-)

Yes, this is better.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, June 27, 2018 5:44 PM
> 
> Hi,
> 
> thanks for the discussion on this topic!
> 
> > > The hardware team said:
> > >  - In CPG point of view:
> > >    - such polling doesn't need (because the reset pulse is generated correctly).
> > >    - About the interval after deassert the reset, this is depend on each hardware module.
> > >      (In other words, if each hardware module has a special handling about after the deassert interval,
> > >       we should follow the special handling.)
> > >  - The I2C controller needs an interval of reading SRCR at least (this is a special handling).
> > >
> > > So, I think adding this code is not good fit in CPG point of view.
> >
> > Calling reset_control_status() from i2c-rcar is fine for me.
> >
< snip >
> 
> Where is this special handling documented BTW, I can't find it.

Please refer to "57.5.4 Usage note for the transmission and reception procedure" in the datasheet Rev. 1.00.
I agree waiting for maximum 1024ms 


> The BSP waits for maximum 1024ms which seems excessive to me.

The BSP waits for maximum 1024us, not 1024ms.

Best regards,
Yoshihiro Shimoda

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

Shimoda-san,

> Please refer to "57.5.4 Usage note for the transmission and reception
> procedure" in the datasheet Rev. 1.00.

Thank you. I'll send a new version with the workaround in the i2c
driver.

> The BSP waits for maximum 1024us, not 1024ms.

You are right. Sorry, my mistake.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]


> Note that reset controller support is optional, so we want to add
> 
>         select RESET_CONTROLLER if ARCH_RENESAS && ARM64
> 
> to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> silently.

Actually, no. I use a non-optional reset_controller_get[1], so I will get
an ERR_PTR when !RESET_CONTROLLER. And I check for this ERR_PTR. So, it
will work with !RESET_CONTROLLER, just without RXDMA always then. That
worked fine with the old patch, need to test my new version now.

[1] In fact, this function is deprecated and I replaced it with the now
more explicit reset_control_get_exclusive(). Plus devm_*, of course.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]