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

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(-)

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 },
>  	{},

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

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

> 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.