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

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Sep 4, 2023 at 3:59 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxx

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