Hi Geert-san,
> From: Geert Uytterhoeven, Sent: Tuesday, June 26, 2018 5:58 PM
>
> Hi Shimoda-san,
>
> On Tue, Jun 26, 2018 at 10:38 AM Yoshihiro Shimoda
> <[email protected]> wrote:
> > > From: Wolfram Sang <[email protected]>, Sent: Tuesday, June 26, 2018
> > > 12:05 PM
> > > > > I got information about this topic.
> > > > >
> > > > > < In CPG / MSSR point of view >
> > > > > - This needs 35 usec wait while asserting.
> > > > > - After deasserted a reset, no wait needs.
> > > > > - In other words, there is each hardware IP dependence.
> > > >
> > > > Let's call the above procedure A.
> > > >
> > > > > < In I2C point of view >
> > > > > - After deasserted the reset, this nees SRCR register polling.
> > > >
> > > > Let's call the above procedure B.
> > > >
> > > > > So, I don't think cpg_mssr_reset() checks the status bit after
> > > > > deasserted a reset.
> > > > > But, what do you think?
> > > >
> > > > cpg_mssr_reset() indeed does not check the status bit after deasserting
> > > > the reset, as it follows procedure A.
> > > >
> > > > Such a check could be added, though. Then it'll become
> > > > (let's call this procedure C):
> > > >
> > > > /* Reset module */
> > > > spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > value = readl(priv->base + SRCR(reg));
> > > > value |= bitmask;
> > > > writel(value, priv->base + SRCR(reg));
> > > > spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > >
> > > > /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz)
> > > > */
> > > > udelay(35);
> > > >
> > > > /* Release module from reset state */
> > > > writel(bitmask, priv->base + SRSTCLR(reg));
> > > >
> > > > + /* Wait until deassertion has completed */
> > > > + while (readl(priv->base + SRCR(reg)) & bitmask)
> > > > + cpu_relax();
> > > >
> > > > Probably we need an upper limit on the number of loops, and call
> > > > udelay(1)
> > > > after each iteration?
> > > >
> > > > for (i 0; i < 35; i++) {
> > > > if (!(readl(priv->base + SRCR(reg)) & bitmask))
> > > > return 0;
> > > > udelay(1);
> > > > }
> > > > return -EIO;
> > > >
> > > > Any advice from the hardware team about that?
> >
> > The hardware team said:
> > - In CPG point of view:
> > - such polling doesn't need (because the reset pulse is generated
> > correctly).
> > - About the interval after deassert the reset, this is depend on each
> > hardware module.
> > (In other words, if each hardware module has a special handling about
> > after the deassert interval,
> > we should follow the special handling.)
> > - The I2C controller needs an interval of reading SRCR at least (this is a
> > special handling).
> >
> > So, I think adding this code is not good fit in CPG point of view.
> >
> > > > But according to procedure A, the check is not needed?
> >
> > As hardware team said, the check (that means procedure C) is not needed.
> >
> > > > Probably because 35µs is an upper limit satisfying all individual
> > > > hardware
> > > > modules?
> > > >
> > > > I'm wondering whether we could use procedure B in the general case,
> > > > as it explicitly checks for completion?
> > > >
> > > > Procedure C is safest, though, so probably the way to go.
> > >
> > > Any news about this topic?
> > >
> > > And how to upstream all this? My I2C patch is clearly a bugfix, but the
> > > necessary CPG update technically isn't? Not sure about this one...
> >
> > I think we have to add reset_control_status() calling into the i2c-rcar.c
> > to follow procedure B.
> > But, Geert-san, what do you think?
>
> Calling reset_control_status() from i2c-rcar is fine for me.
Thank you for your comment!
> Note that reset controller support is optional, so we want to add
>
> select RESET_CONTROLLER if ARCH_RENESAS && ARM64
>
> to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail
> silently.
Thanks for this information. It's helpful to me because I'll add reset
controller support on renesas_usbhs driver later.
> This hardware bug is restricted to R-Car Gen3?
Yes, this is hardware "behaviour" on R-Car Gen3 :)
And older SoCs' I2C doesn't support DMA transfer.
> If it applies to R-Car Gen2, too, the "&& ARM64" has to be dropped.
> If it applies to R-Car Gen1, too, we have to write an R-Car Gen1 reset
> controller driver (R-Car Gen1 uses the legacy CPG and MSTP clock drivers,
> and cannot be migrated to the CPG/MSSR driver, as its MSTP and RESET
> functionalities are in separate modules).
So, they are not needed, I think.
Best regards,
Yoshihiro Shimoda
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [email protected]
>
> 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
