Re: [PATCH 1/6] usb: phy: mxs: Using regulator phy-3p0

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

On Thu, Jul 18, 2024 at 10:42:33AM -0400, Frank Li wrote:
> On Thu, Jul 18, 2024 at 06:26:32PM +0800, Xu Yang wrote:
> > It is one of PHY's power, and we need to enable it to keep signal
> > quality good, and pass eye diagram test.
> 
> "Enable regulator 'phy-3p0' to pass eye diagram test since it improve signal
> qualilty."
> 
> My question is why it just improve signal quality instead of make it work.
> It should not work if no power supply. 

I'm cleaning up these old local patch and I'm not very clear about the history.

But I think this 3p0 is not the main power of phy, it's just one of phy's power.
I can only get below info about this reg-3p0:

37.6.2 Regulator 3P0 Register (PMU_REG_3P0)
  "This register defines the control and status bits for the 3.0V regulator
powered by the host USB VBUS pin."

I've just tested it works well without this phy-3p0. I'll rewrite the commit
message to avoid confusion.

> 
> > 
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/phy/phy-mxs-usb.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> > index 920a32cd094d..42fcc8ad9492 100644
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/regmap.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/regulator/consumer.h>
> >  
> >  #define DRIVER_NAME "mxs_phy"
> >  
> > @@ -204,6 +205,7 @@ struct mxs_phy {
> >  	int port_id;
> >  	u32 tx_reg_set;
> >  	u32 tx_reg_mask;
> > +	struct regulator *phy_3p0;
> >  };
> >  
> >  static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy)
> > @@ -288,6 +290,16 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
> >  	if (ret)
> >  		goto disable_pll;
> >  
> > +	if (mxs_phy->phy_3p0) {
> > +		ret = regulator_enable(mxs_phy->phy_3p0);
> > +		if (ret) {
> > +			dev_err(mxs_phy->phy.dev,
> > +				"Failed to enable 3p0 regulator, ret=%d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	/* Power up the PHY */
> >  	writel(0, base + HW_USBPHY_PWD);
> >  
> > @@ -448,6 +460,9 @@ static void mxs_phy_shutdown(struct usb_phy *phy)
> >  	if (is_imx7ulp_phy(mxs_phy))
> >  		mxs_phy_pll_enable(phy->io_priv, false);
> >  
> > +	if (mxs_phy->phy_3p0)
> > +		regulator_disable(mxs_phy->phy_3p0);
> > +
> >  	clk_disable_unprepare(mxs_phy->clk);
> >  }
> >  
> > @@ -789,6 +804,21 @@ static int mxs_phy_probe(struct platform_device *pdev)
> >  	mxs_phy->clk = clk;
> >  	mxs_phy->data = of_device_get_match_data(&pdev->dev);
> >  
> > +	mxs_phy->phy_3p0 = devm_regulator_get(&pdev->dev, "phy-3p0");
> 
> Does binding doc update?

Yes.

> 
> > +	if (PTR_ERR(mxs_phy->phy_3p0) == -EPROBE_DEFER) {
> > +		return -EPROBE_DEFER;
> > +	} else if (PTR_ERR(mxs_phy->phy_3p0) == -ENODEV) {
> > +		/* not exist */
> > +		mxs_phy->phy_3p0 = NULL;
> > +	} else if (IS_ERR(mxs_phy->phy_3p0)) {
> > +		dev_err(&pdev->dev, "Getting regulator error: %ld\n",
> > +			PTR_ERR(mxs_phy->phy_3p0));
> > +		return PTR_ERR(mxs_phy->phy_3p0);
> > +	}
> 
> just need call dev_err_probe()

Ok. Will change it.

> 
> > +
> > +	if (mxs_phy->phy_3p0)
> > +		regulator_set_voltage(mxs_phy->phy_3p0, 3200000, 3200000);
> > +
> >  	platform_set_drvdata(pdev, mxs_phy);
> >  
> >  	device_set_wakeup_capable(&pdev->dev, true);
> > -- 
> > 2.34.1
> >