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
