| Message ID | 20230904135852.12146-3-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: rcar: add FastMode+ support | expand |
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 }, > {},
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.
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
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.
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)) {
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(-)