Re: [PATCH v2] 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:
- [High] Exporting the CR port regmap via debugfs without clock synchronization allows unclocked MMIO accesses, leading to a synchronous external abort (kernel panic).
- [Medium] Using a direct MMIO regmap for an indirect data/address parallel port does not correctly expose the internal PHY registers, defeating the diagnostic purpose of the patch.
--

commit ac06b61c8f793ff4f5693b009fbad15e95d82242
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
provided 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 b05d80e849a14..958d114b0c835 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -685,6 +689,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,
> +};
> +
>  static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
> @@ -713,6 +725,11 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(imx_phy->base))
>  		return PTR_ERR(imx_phy->base);
>  
> +	imx_phy->cr_regmap = devm_regmap_init_mmio(dev, imx_phy->base + PHY_CRCTL,
> +						   &imx_cr_regmap_config);

Will accessing the debugfs file when the PHY is suspended or not yet powered
on cause a kernel panic?

The driver dynamically manages its clocks (imx_phy->clk and imx_phy->alt_clk)
in imx8mq_phy_power_on() and imx8mq_phy_power_off() rather than using
pm_runtime.

Since devm_regmap_init_mmio() does not attach a clock to the regmap,
the regmap core won't enable clocks during debugfs register accesses. On
i.MX platforms, MMIO reads on an unclocked hardware block trigger a
synchronous external abort.

Could we delegate clock management to the regmap (e.g., by using
devm_regmap_init_mmio_clk()) or implement pm_runtime hooks?

Additionally, the commit message mentions the CR port is a 16-bit
data/address parallel port used to access internal PHY registers. Does
this direct MMIO regmap actually expose the internal PHY registers?

Since devm_regmap_init_mmio() creates a direct memory-mapped regmap over
the CR port's base MMIO registers, reading the debugfs file will simply
dump the CR port's Address and Data MMIO registers. It won't execute the
required address/data handshake protocol to read the internal PHY state,
which defeats the diagnostic purpose of the patch.

Would it be better to implement a custom regmap_bus to handle the atomic
address/data indirect read sequence?

Also, is max_register = 0x7 correct here? It seems misleading for a 32-bit
regmap with a stride of 4 if there are only a couple of MMIO registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512101212.1498223-1-xu.yang_2@nxp.com?part=1

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