Re: [PATCH v4 4/5] phy: fsl-imx8mq-usb: add control register regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Writes to the CR debugfs control registers are silently discarded without error if the PHY is runtime suspended.

Pre-existing issues:
- [High] Clock reference count underflow on driver unbind due to devm_clk_get_enabled() and Runtime PM.
--

commit 3bce0d41a4e2282036c6fa41e62f47cfe0788d65
Author: Xu Yang <xu.yang_2@nxp.com>

phy: fsl-imx8mq-usb: add control register regmap
    
The CR port is a simple 16-bit data/address parallel port that is
accessed through 32-bit MMIO registers for on-chip access to the
control registers inside the USB 3.0 femtoPHY. Add control register
regmap and export these registers by debugfs to help PHY's diagnostic.

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 27aa696f5dd40..e24f46d7924b6 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -667,6 +671,14 @@ static const struct of_device_id imx8mq_usb_phy_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, imx8mq_usb_phy_of_match);
>  
> +static const struct regmap_config imx_cr_regmap_config = {
> +	.name = "cr",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x7,
> +};

[Severity: Medium]
Does this regmap configuration need a .cache_type?

Without one, it defaults to REGCACHE_NONE. Later in the patch,
imx8mq_usb_phy_runtime_suspend() puts the regmap into cache-only mode.
With REGCACHE_NONE, any debugfs writes while suspended will return success,
but the data is silently discarded because there is no cache backing it.

[ ... ]
> @@ -731,6 +750,9 @@ static int imx8mq_usb_phy_runtime_suspend(struct device *dev)
>  {
>  	struct imx8mq_usb_phy *imx_phy = dev_get_drvdata(dev);
>  
> +	if (imx_phy->cr_regmap)
> +		regcache_cache_only(imx_phy->cr_regmap, true);
> +
>  	clk_disable_unprepare(imx_phy->alt_clk);
>  	clk_disable_unprepare(imx_phy->clk);

[Severity: High]
This is a pre-existing issue, but can this lead to a clock enable_count
underflow on driver unbind?

The driver acquires the clock using devm_clk_get_enabled() in
imx8mq_usb_phy_probe(), meaning devm manages the enable state. However, it
manually disables the clock here during runtime suspend.

If the driver is unbound while the device is in the runtime suspended state,
the devm cleanup function will call clk_disable_unprepare() a second time on
the already disabled clock, since there is no .remove callback to resume the
device first.

> @@ -752,6 +774,9 @@ static int imx8mq_usb_phy_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	if (imx_phy->cr_regmap)
> +		regcache_cache_only(imx_phy->cr_regmap, false);

[Severity: Medium]
Should there be a regcache_sync() call here after disabling cache-only mode?

If a valid cache type is added to imx_cr_regmap_config, the cache will need
to be synchronized here to flush any pending diagnostic writes to the hardware
that occurred while the device was suspended.

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-imx8mp-usb-phy-improvement-v4-0-b2ddf2f3862c@nxp.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy