[RFT,1/2] i2c: rcar: reset controller is mandatory for Gen3+

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

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