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

Message ID 20180529105847.15663-2-wsa+renesas@sang-engineering.com
State New
Headers show
Series
  • [RFC,1/1] i2c: rcar: handle RXDMA HW bug on Gen3
Related show

Commit Message

Wolfram Sang May 29, 2018, 10:58 a.m.
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(-)

Comments

Yoshihiro Shimoda May 30, 2018, 8:35 a.m. | #1
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
Wolfram Sang May 31, 2018, 8:31 a.m. | #2
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
Geert Uytterhoeven May 31, 2018, 8:45 a.m. | #3
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
Yoshihiro Shimoda May 31, 2018, 9:12 a.m. | #4
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
Yoshihiro Shimoda May 31, 2018, 9:22 a.m. | #5
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
Yoshihiro Shimoda June 7, 2018, 5:36 a.m. | #6
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
Geert Uytterhoeven June 7, 2018, 10:08 a.m. | #7
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

Patch

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;