i2c: rcar: add FastMode+ support

[PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Wolfram Sang 2 years, 5 months ago

So far, we treated Gen4 as Gen3. But we are soon adding FM+ as a Gen4
specific feature, so prepare the code for the new devtype.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f2b953df0c4d..76aa16bf17b2 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -114,6 +114,7 @@ enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
 	I2C_RCAR_GEN2,
 	I2C_RCAR_GEN3,
+	I2C_RCAR_GEN4,
 };
 
 struct rcar_i2c_priv {
@@ -218,7 +219,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 	/* start clock */
 	rcar_i2c_write(priv, ICCCR, priv->icccr);
 
-	if (priv->devtype == I2C_RCAR_GEN3)
+	if (priv->devtype >= I2C_RCAR_GEN3)
 		rcar_i2c_write(priv, ICFBSCR, TCYC17);
 
 }
@@ -251,22 +252,11 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 		.scl_int_delay_ns	= 50,
 	};
 
+	cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
+
 	/* Fall back to previously used values if not supplied */
 	i2c_parse_fw_timings(dev, &t, false);
 
-	switch (priv->devtype) {
-	case I2C_RCAR_GEN1:
-		cdf_width = 2;
-		break;
-	case I2C_RCAR_GEN2:
-	case I2C_RCAR_GEN3:
-		cdf_width = 3;
-		break;
-	default:
-		dev_err(dev, "device type error\n");
-		return -EIO;
-	}
-
 	/*
 	 * calculate SCL clock
 	 * see
@@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
 	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
 	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
 	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
+	/* S4 has no FM+ bit */
+	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
 	{ .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
 	{ .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
 	{ .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
-	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
+	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);
@@ -1101,6 +1093,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 		irqhandler = rcar_i2c_gen2_irq;
 	}
 
+	/* Gen3 needs reset for RXDMA */
 	if (priv->devtype == I2C_RCAR_GEN3) {
 		priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 		if (!IS_ERR(priv->rstc)) {
-- 
2.35.1

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Geert Uytterhoeven 2 years, 5 months ago

Hi Wolfram,

On Mon, Sep 4, 2023 at 3:59 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> So far, we treated Gen4 as Gen3. But we are soon adding FM+ as a Gen4
> specific feature, so prepare the code for the new devtype.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c

> @@ -218,7 +219,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
>         /* start clock */
>         rcar_i2c_write(priv, ICCCR, priv->icccr);
>
> -       if (priv->devtype == I2C_RCAR_GEN3)
> +       if (priv->devtype >= I2C_RCAR_GEN3)
>                 rcar_i2c_write(priv, ICFBSCR, TCYC17);

Note that R-Car Gen4 (incl. R-Car S4) has ICFBSCR bits related to
Slave Clock Stretch Select (which is not yet supported by the driver).

> @@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
>         { .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
>         { .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
>         { .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> +       /* S4 has no FM+ bit */
> +       { .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
>         { .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
>         { .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
>         { .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
> -       { .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> +       { .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);
> @@ -1101,6 +1093,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>                 irqhandler = rcar_i2c_gen2_irq;
>         }
>
> +       /* Gen3 needs reset for RXDMA */
>         if (priv->devtype == I2C_RCAR_GEN3) {

According to the Programming Examples in the docs for R-Car Gen3,
R-Car V3U, S4-8, and V4H, I2C must be reset "at the beginning of
transmission and reception procedure", so not only for DMA.

>                 priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>                 if (!IS_ERR(priv->rstc)) {

Also, you didn't the touch the checks in rcar_i2c_cleanup_dma():

    /* Gen3 can only do one RXDMA per transfer and we just completed it */
    if (priv->devtype == I2C_RCAR_GEN3 && ...) ...

and rcar_i2c_master_xfer():

    /* Gen3 needs a reset before allowing RXDMA once */
    if (priv->devtype == I2C_RCAR_GEN3) {
            ...
    }

Don't these apply to R-Car Gen4? I can't easily find where this quirk
is documented (perhaps just as a commit in the BSP?), but at least the
"Usage note for DMA mode of Receive Operation" looks identical for
R-Car Gen3 and for the various R-Car Gen4 variants.

So either:
  1. These checks must be updated, too, or
  2. The commit description must explain why this is not needed, as
     switching to I2C_RCAR_GEN4 changes behavior for R-Car Gen4 SoCs
     using the family-specific fallback.

BTW, depending on the answers to my questions above, you may want to
replace the rcar_i2c_type enum by a feature mask...

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: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Wolfram Sang 2 years, 5 months ago

Hi Geert,

thank you for the review!

> Note that R-Car Gen4 (incl. R-Car S4) has ICFBSCR bits related to
> Slave Clock Stretch Select (which is not yet supported by the driver).

Thanks for the heads up. I'd need more information about the use case of
these bits. Seperate task.

> According to the Programming Examples in the docs for R-Car Gen3,
> R-Car V3U, S4-8, and V4H, I2C must be reset "at the beginning of
> transmission and reception procedure", so not only for DMA.

Sadly, this is vague. If you look at the example for a combined
write-then-read transfer, then you see that only one reset is done,

i.e.: reset -> write -> rep_start -> read

That would mean that we don't need a reset per read/write message of a
transfer. But a reset per transfer then? I would wonder why because we
could also have a super long transfer with lots of read/write messages
in it. Do we need a reset then inbetween? Or is it really dependant on
the STOP bit being transferred? I guess these are all questions for the
HW team, though.

I was reluctant to add the reset too often because my measurements back
then showed that it costs around 5us every time. Annoying. Maybe I
should take it easy and follow the documentation. But then I am still
not sure if a large transfer with way more than two messages are OK
without reset? I will ask the HW team.

> Also, you didn't the touch the checks in rcar_i2c_cleanup_dma():
...
> and rcar_i2c_master_xfer():
...
> 
> Don't these apply to R-Car Gen4? I can't easily find where this quirk
> is documented (perhaps just as a commit in the BSP?), but at least the
> "Usage note for DMA mode of Receive Operation" looks identical for
> R-Car Gen3 and for the various R-Car Gen4 variants.

My memory played a trick on me here. I asked Shimoda-san about this
issue on Gen4. I thought I got an answer that it was fixed, so I left
the code Gen3 only. But he actually never got a reply and I forgot to
ping about it.

The latest documentation has now a "usage note for DMA mode" about it
implying that the issue is still present on Gen4 :(

> BTW, depending on the answers to my questions above, you may want to
> replace the rcar_i2c_type enum by a feature mask...

That might be an option. I need to reshuffle my I2C patches first,
though. I'll send some cleanups first to have them out of the way. Then,
I will respin Gen4 support and take care of the DMA RX issue and the new
reset handling there. Thank you for your input!

All the best,

   Wolfram

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Wolfram Sang 2 years, 5 months ago

> I was reluctant to add the reset too often because my measurements back
> then showed that it costs around 5us every time. Annoying. Maybe I
> should take it easy and follow the documentation. But then I am still
> not sure if a large transfer with way more than two messages are OK
> without reset? I will ask the HW team.

Stupid me, we are following the documentation. Except that we treat the
reset property as optional while it should be mandatory for >= Gen3.

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Andi Shyti 2 years, 5 months ago

Hi Wolfram,

> @@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
>  	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
>  	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
>  	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> +	/* S4 has no FM+ bit */
> +	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },

is this I2C_RCAR_GEN3 or I2C_RCAR_GEN4?

Rest looks good.

Andi

>  	{ .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
>  	{ .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
>  	{ .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
> -	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> +	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
>  	{},

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Wolfram Sang 2 years, 5 months ago

On Tue, Sep 05, 2023 at 01:36:24PM +0200, Andi Shyti wrote:
> Hi Wolfram,
> 
> > @@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
> >  	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
> >  	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
> >  	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> > +	/* S4 has no FM+ bit */
> > +	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
> 
> is this I2C_RCAR_GEN3 or I2C_RCAR_GEN4?

Technically, it is Gen4. But its I2C controller behaves like Gen3. This
is why it has a seperate entry to avoid the generic Gen4 fallback...

> > -	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> > +	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },

... here.

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Posted by Andi Shyti 2 years, 5 months ago

Hi Wolfram,

> > >  	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
> > >  	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
> > >  	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> > > +	/* S4 has no FM+ bit */
> > > +	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
> > 
> > is this I2C_RCAR_GEN3 or I2C_RCAR_GEN4?
> 
> Technically, it is Gen4. But its I2C controller behaves like Gen3. This
> is why it has a seperate entry to avoid the generic Gen4 fallback...
> 
> > > -	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> > > +	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
> 
> ... here.

got it... I just wanted to be sure... thanks!

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi