RE: [PATCH v3 05/11] reset: renesas: Add RZ/G2L usbphy control driver

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Philipp,.

Thanks for the feedback


> Subject: Re: [PATCH v3 05/11] reset: renesas: Add RZ/G2L usbphy control
> driver
> 
> On Wed, 2021-06-30 at 13:25 +0000, Biju Das wrote:
> [...]
> > > What else does it control? Are we missing any functionality that
> > > would have to be added later?
> >
> > It has other controls like direct power down, clock control and
> > connection control to handle the cases, when USB interface is not used
> > permanently(like when port1 and port2 unused permanently)
> >
> > In future, if there is a case like below(for eg:- )
> > 1) when port1 and port2 unused permanently ( This case recommends HW
> > mod as well)
> > 2) when either port1 or port2 unused permanently( This case
> > recommends, from HW point not to supply the power to unused port)
> >
> > May be we could expose these properties in dt and probe time set the
> > required control, if there is a requirement to support this cases in
> future.
> 
> Ok, thanks. If that's board design specific static configuration, I see no
> issue.
> 
> [...]
> > > > +	if ((val & 0xff) == (PHY_RESET_PORT1 | PHY_RESET_PORT2))
> > >                    ^^^^
> > > What is the significance of the magic 0xff?
> >
> >  We should use (PHY_RESET_PORT1 | PHY_RESET_PORT2) instead.
> >
> > Basically it is checking both ports are in reset state or not?
> 
> That would be better. Right now it's checking that bits 2, 3, 6, and 7 are
> cleared, and I couldn't tell whether that was by accident or on purpose.
>
> [...]
> > > > +static void rzg2l_usbphy_ctrl_release_reset(struct
> > > > +reset_controller_dev
> > > *rcdev,
> > > > +					    unsigned long id)
> > > > +{
> > > > +	struct rzg2l_usbphy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > > > +	void __iomem *base = priv->base;
> > > > +	u32 val = readl(base + RESET);
> > > > +
> > > > +	val |= SEL_PLLRESET;
> > > > +	val &= ~(PLL_RESET | (id ? PHY_RESET_PORT2 :
> PHY_RESET_PORT1));
> > > > +	writel(val, base + RESET);
> 
> It would be good to protect the RESET register read-modify-writes with a
> spinlock, same in the _set_reset() function.

OK. Will do.

Regards,
Biju

> 
> regards
> Philipp