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

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v3 05/11] reset: renesas: Add RZ/G2L usbphy control driver
Date: Fri, 02 Jul 2021 10:52:04 +0200	[thread overview]
Message-ID: <634d5d9203b633ffdcc05a9b388cdab12f383d8d.camel@pengutronix.de> (raw)
In-Reply-To: <TYCPR01MB593358CF332F7C5CC1A852DD86019@TYCPR01MB5933.jpnprd01.prod.outlook.com>

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.

regards
Philipp