Re: [PATCH v5 3/5] phy: fsl-imx8mq-usb: add runtime PM support

[Xu Yang]

On Tue, Jun 30, 2026 at 02:06:21PM -0500, Frank Li wrote:
> On Tue, Jun 30, 2026 at 06:11:30PM +0800, Xu Yang wrote:
> > From: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > Add runtime PM to ensure the PHY is properly powered and clocked during
> > register access, preventing potential system hangs.
> >
> > It guards register access in the following scenarios:
> > - PHY operations: init() and power_on/off() callbacks are guarded by
> >   phy core
> > - Type-C orientation switching when PHY/Controller are suspended which
> >   needs explicitly care
> > - Future PHY control port register regmap debugfs access
> >
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > ---
> > Changes in v5:
> >  - use non-devm PM runtime callback to correctly enable/disable clocks
> >    when unbind the device
> > Changes in v4:
> >  - replace guard() with PM_RUNTIME_ACQUIRE()
> > Changes in v3:
> >  - new patch
> > ---
> >  drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 64 +++++++++++++++++++++---------
> >  1 file changed, 45 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> > index 3a5788c609e1..9d1dd0e7352e 100644
> > --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> > +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/of.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/usb/typec_mux.h>
> >
> > @@ -136,17 +137,15 @@ static int tca_blk_typec_switch_set(struct typec_switch_dev *sw,
> >  {
> >  	struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
> >  	struct tca_blk *tca = imx_phy->tca;
> > -	int ret;
> >
> >  	if (tca->orientation == orientation)
> >  		return 0;
> >
> > -	ret = clk_prepare_enable(imx_phy->clk);
> > -	if (ret)
> > -		return ret;
> > +	PM_RUNTIME_ACQUIRE(&imx_phy->phy->dev, pm);
> > +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> > +		return -ENXIO;
> >
> >  	tca_blk_orientation_set(tca, orientation);
> > -	clk_disable_unprepare(imx_phy->clk);
> >
> >  	return 0;
> >  }
> > @@ -620,16 +619,6 @@ static int imx8mq_phy_power_on(struct phy *phy)
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = clk_prepare_enable(imx_phy->clk);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = clk_prepare_enable(imx_phy->alt_clk);
> > -	if (ret) {
> > -		clk_disable_unprepare(imx_phy->clk);
> > -		return ret;
> > -	}
> > -
> >  	/* Disable rx term override */
> >  	value = readl(imx_phy->base + PHY_CTRL6);
> >  	value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL;
> > @@ -648,8 +637,6 @@ static int imx8mq_phy_power_off(struct phy *phy)
> >  	value |= PHY_CTRL6_RXTERM_OVERRIDE_SEL;
> >  	writel(value, imx_phy->base + PHY_CTRL6);
> >
> > -	clk_disable_unprepare(imx_phy->alt_clk);
> > -	clk_disable_unprepare(imx_phy->clk);
> >  	regulator_disable(imx_phy->vbus);
> >
> >  	return 0;
> > @@ -693,13 +680,13 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, imx_phy);
> >
> > -	imx_phy->clk = devm_clk_get(dev, "phy");
> > +	imx_phy->clk = devm_clk_get_enabled(dev, "phy");
> >  	if (IS_ERR(imx_phy->clk)) {
> >  		dev_err(dev, "failed to get imx8mq usb phy clock\n");
> >  		return PTR_ERR(imx_phy->clk);
> >  	}
> >
> > -	imx_phy->alt_clk = devm_clk_get_optional(dev, "alt");
> > +	imx_phy->alt_clk = devm_clk_get_optional_enabled(dev, "alt");
> >  	if (IS_ERR(imx_phy->alt_clk))
> >  		return dev_err_probe(dev, PTR_ERR(imx_phy->alt_clk),
> >  				    "Failed to get alt clk\n");
> > @@ -708,6 +695,9 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> >  	if (IS_ERR(imx_phy->base))
> >  		return PTR_ERR(imx_phy->base);
> >
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +
> 
> devm_pm_runtime_enable();

OK.

> 
> runtime pm will be always on active status, why suspend it?

First, at the end of __driver_probe_device(), it will call pm_request_idle()
for the device. So runtime_suspend() will run.
Second, this platform device is the parent of created phy device, the runtime status
of phy device is managed by phy core, so this platform device will be managed by phy
core indirectly.

> 
> >  	phy_ops = of_device_get_match_data(dev);
> >  	if (!phy_ops)
> >  		return -EINVAL;
> > @@ -737,15 +727,51 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> >
> >  static void imx8mq_usb_phy_remove(struct platform_device *pdev)
> >  {
> > +	struct device *dev = &pdev->dev;
> > +
> > +	pm_runtime_get_sync(dev);
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_put_noidle(dev);
> > +}
> > +
> > +static int imx8mq_usb_phy_runtime_suspend(struct device *dev)
> > +{
> > +	struct imx8mq_usb_phy *imx_phy = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(imx_phy->alt_clk);
> > +	clk_disable_unprepare(imx_phy->clk);
> 
> can you switch to use bulk clk api.

imx_phy->clk is required but imx_phy->alt_clk is optional. So it seems not
convenient to use clk_bulk_get()/clk_bulk_get_optional() APIs. Maybe we can
switch to use bulk clk API when more clocks are required in the future.

Thanks,
Xu Yang