Re: [PATCH 2/6] usb: phy: mxs: keep USBPHY2's clk always on

[Xu Yang <xu.yang_2@nxp.com>]

On Thu, Jul 18, 2024 at 11:00:06AM -0400, Frank Li wrote:
> On Thu, Jul 18, 2024 at 06:26:33PM +0800, Xu Yang wrote:
> > Per IC engineer request, we need to keep USBPHY2's clk always on,
> 
> "IP require keep keep USBPHY2's clk always on."
> 
> Not personal request, even it is IC expert. It should base on the "fact"
> instead of personal's opinion.

Okay.

> 
> > in this way, the USBPHY2 (PLL7) power can be controlled by
> > hardware suspend signal totally. It is benefit of USB remote wakeup
> > case which needs the resume signal be sent out as soon as
> > possible (without software interfere). Without this, we may see usb
> > remote wakeup issue since the host does not send resume in time.
> 
> So USBPHY2 (PLL7) power can be controlled by suspend signal. USB remote
> wakeup needs resume signal be sent out as soon as possible to match
> 
> "spec requirement" or some other requirement.

Will change.

> 
> > 
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/phy/phy-mxs-usb.c | 36 ++++++++++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> > index 42fcc8ad9492..b6868cc22c1e 100644
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -150,6 +150,16 @@
> >  #define MXS_PHY_TX_D_CAL_MIN			79
> >  #define MXS_PHY_TX_D_CAL_MAX			119
> >  
> > +/*
> > + * At some versions, the PHY2's clock is controlled by hardware directly,
> 
> It better declear which version, for example, which chip use if no version
> info in IP.

Okay, will add.

> 
> > + * eg, according to PHY's suspend status. In these PHYs, we only need to
> > + * open the clock at the initialization and close it at its shutdown routine.
> > + * It will be benefit for remote wakeup case which needs to send resume
> > + * signal as soon as possible, and in this case, the resume signal can be sent
> > + * out without software interfere.
> 
> These PHYs can send resume signal without software interfere if not gate
> clock.

Will change.

> 
> > + */
> > +#define MXS_PHY_HARDWARE_CONTROL_PHY2_CLK	BIT(4)
> > +
> >  struct mxs_phy_data {
> >  	unsigned int flags;
> >  };
> > @@ -161,12 +171,14 @@ static const struct mxs_phy_data imx23_phy_data = {
> >  static const struct mxs_phy_data imx6q_phy_data = {
> >  	.flags = MXS_PHY_SENDING_SOF_TOO_FAST |
> >  		MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS |
> > -		MXS_PHY_NEED_IP_FIX,
> > +		MXS_PHY_NEED_IP_FIX |
> > +		MXS_PHY_HARDWARE_CONTROL_PHY2_CLK,
> >  };
> >  
> >  static const struct mxs_phy_data imx6sl_phy_data = {
> >  	.flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS |
> > -		MXS_PHY_NEED_IP_FIX,
> > +		MXS_PHY_NEED_IP_FIX |
> > +		MXS_PHY_HARDWARE_CONTROL_PHY2_CLK,
> >  };
> >  
> >  static const struct mxs_phy_data vf610_phy_data = {
> > @@ -175,7 +187,8 @@ static const struct mxs_phy_data vf610_phy_data = {
> >  };
> >  
> >  static const struct mxs_phy_data imx6sx_phy_data = {
> > -	.flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> > +	.flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS |
> > +		MXS_PHY_HARDWARE_CONTROL_PHY2_CLK,
> >  };
> >  
> >  static const struct mxs_phy_data imx6ul_phy_data = {
> > @@ -206,6 +219,7 @@ struct mxs_phy {
> >  	u32 tx_reg_set;
> >  	u32 tx_reg_mask;
> >  	struct regulator *phy_3p0;
> > +	bool hardware_control_phy2_clk;
> 
> Needn't it. just check MXS_PHY_HARDWARE_CONTROL_PHY2_CLK flag is enough.

Okay.

> 
> >  };
> >  
> >  static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy)
> > @@ -518,12 +532,17 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
> >  		}
> >  		writel(BM_USBPHY_CTRL_CLKGATE,
> >  		       x->io_priv + HW_USBPHY_CTRL_SET);
> > -		clk_disable_unprepare(mxs_phy->clk);
> > +		if (!(mxs_phy->port_id == 1 &&
> > +				mxs_phy->hardware_control_phy2_clk))
> > +			clk_disable_unprepare(mxs_phy->clk);
> >  	} else {
> >  		mxs_phy_clock_switch_delay();
> > -		ret = clk_prepare_enable(mxs_phy->clk);
> > -		if (ret)
> > -			return ret;
> > +		if (!(mxs_phy->port_id == 1 &&
> > +				mxs_phy->hardware_control_phy2_clk)) {
> > +			ret = clk_prepare_enable(mxs_phy->clk);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  		writel(BM_USBPHY_CTRL_CLKGATE,
> >  		       x->io_priv + HW_USBPHY_CTRL_CLR);
> >  		writel(0, x->io_priv + HW_USBPHY_PWD);
> > @@ -819,6 +838,9 @@ static int mxs_phy_probe(struct platform_device *pdev)
> >  	if (mxs_phy->phy_3p0)
> >  		regulator_set_voltage(mxs_phy->phy_3p0, 3200000, 3200000);
> >  
> > +	if (mxs_phy->data->flags & MXS_PHY_HARDWARE_CONTROL_PHY2_CLK)
> > +		mxs_phy->hardware_control_phy2_clk = true;
> > +
> 
> Needn't it.

Okay. Will remove this.

> 
> >  	platform_set_drvdata(pdev, mxs_phy);
> >  
> >  	device_set_wakeup_capable(&pdev->dev, true);
> > -- 
> > 2.34.1
> >