Hi Wolfram-san, Geert-san,
I'm sorry for delayed response. I completely overlooked this email...
> From: Wolfram Sang <wsa@xxxxxxxxxxxxx>, 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?
Best regards,
Yoshihiro Shimoda
