| Message ID | 20230913062950.4968-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: rcar: improve Gen3 support | expand |
Hi Wolfram, Thanks for your patch! On Wed, Sep 13, 2023 at 8:41 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Initially, we only needed a reset controller to make sure RXDMA works at > least once per transfer. Meanwhile, documentation has been updated. It > now says that a reset has to be performed prior every transaction, also > if it is non-DMA. So, make the reset controller a requirement instead of > being optional. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -838,12 +838,10 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, > > /* Gen3 needs a reset before allowing RXDMA once */ > if (priv->devtype == I2C_RCAR_GEN3) { > - priv->flags |= ID_P_NO_RXDMA; > - if (!IS_ERR(priv->rstc)) { > - ret = rcar_i2c_do_reset(priv); > - if (ret == 0) > - priv->flags &= ~ID_P_NO_RXDMA; > - } > + priv->flags &= ~ID_P_NO_RXDMA; > + ret = rcar_i2c_do_reset(priv); > + if (ret) > + priv->flags |= ID_P_NO_RXDMA; This is pre-existing, but if rcar_i2c_do_reset() returns an error, that means the I2C block couldn't get out of reset. Are we sure we can still do PIO transfers in that case, or should this be considered a fatal error? > } > > rcar_i2c_init(priv); > @@ -1096,11 +1094,13 @@ static int rcar_i2c_probe(struct platform_device *pdev) > > if (priv->devtype == I2C_RCAR_GEN3) { > priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > - if (!IS_ERR(priv->rstc)) { > - ret = reset_control_status(priv->rstc); > - if (ret < 0) > - priv->rstc = ERR_PTR(-ENOTSUPP); > - } > + if (IS_ERR(priv->rstc)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), > + "couldn't get reset"); > + > + ret = reset_control_status(priv->rstc); > + if (ret < 0) > + return ret; This is a pre-existing check, but do you really need it? This condition will be true if the reset is still asserted, which could happen due to some glitch, or force-booting into a new kernel using kexec. And AFAIUI, that should be resolved by the call to rcar_i2c_do_reset() above. > } > > /* Stay always active when multi-master to keep arbitration working */ 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, > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks! > > + priv->flags &= ~ID_P_NO_RXDMA; > > + ret = rcar_i2c_do_reset(priv); > > + if (ret) > > + priv->flags |= ID_P_NO_RXDMA; > > This is pre-existing, but if rcar_i2c_do_reset() returns an error, > that means the I2C block couldn't get out of reset. Are we sure we > can still do PIO transfers in that case, or should this be considered > a fatal error? Makes sense. I will double check what to do here. > > + ret = reset_control_status(priv->rstc); > > + if (ret < 0) > > + return ret; > > This is a pre-existing check, but do you really need it? > This condition will be true if the reset is still asserted, which > could happen due to some glitch, or force-booting into a new kernel > using kexec. And AFAIUI, that should be resolved by the call to > rcar_i2c_do_reset() above. This check is needed to ensure reset_control_status() really works because we need it in rcar_i2c_do_reset(). From the docs: "reset_control_status - returns a negative errno if not supported,..." The code only checks for that, not for the status of the reset line. All the best, Wolfram
Hi Wolfram, On Tue, Sep 19, 2023 at 11:19 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > + ret = reset_control_status(priv->rstc); > > > + if (ret < 0) > > > + return ret; > > > > This is a pre-existing check, but do you really need it? > > This condition will be true if the reset is still asserted, which > > could happen due to some glitch, or force-booting into a new kernel > > using kexec. And AFAIUI, that should be resolved by the call to > > rcar_i2c_do_reset() above. > > This check is needed to ensure reset_control_status() really works > because we need it in rcar_i2c_do_reset(). From the docs: > > "reset_control_status - returns a negative errno if not supported,..." > > The code only checks for that, not for the status of the reset line. Right, I missed the negative. I don't think this can fail on R-Car Gen2+ (using the CPG/MSSR driver) if devm_reset_control_get_exclusive() succeeded, but it's prudent, in case the block is every reused on a different SoC family. Gr{oetje,eeting}s, Geert
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 5e97635faf78..342c3747f415 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -838,12 +838,10 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, /* Gen3 needs a reset before allowing RXDMA once */ if (priv->devtype == I2C_RCAR_GEN3) { - priv->flags |= ID_P_NO_RXDMA; - if (!IS_ERR(priv->rstc)) { - ret = rcar_i2c_do_reset(priv); - if (ret == 0) - priv->flags &= ~ID_P_NO_RXDMA; - } + priv->flags &= ~ID_P_NO_RXDMA; + ret = rcar_i2c_do_reset(priv); + if (ret) + priv->flags |= ID_P_NO_RXDMA; } rcar_i2c_init(priv); @@ -1096,11 +1094,13 @@ static int rcar_i2c_probe(struct platform_device *pdev) if (priv->devtype == I2C_RCAR_GEN3) { priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); - if (!IS_ERR(priv->rstc)) { - ret = reset_control_status(priv->rstc); - if (ret < 0) - priv->rstc = ERR_PTR(-ENOTSUPP); - } + if (IS_ERR(priv->rstc)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), + "couldn't get reset"); + + ret = reset_control_status(priv->rstc); + if (ret < 0) + return ret; } /* Stay always active when multi-master to keep arbitration working */
Initially, we only needed a reset controller to make sure RXDMA works at least once per transfer. Meanwhile, documentation has been updated. It now says that a reset has to be performed prior every transaction, also if it is non-DMA. So, make the reset controller a requirement instead of being optional. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rcar.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)