Re: [PATCH v2 11/24] phy: zte: Add ZTE ZX279128S PCIe PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] phy: missing error path cleanup in zx_pcie_phy_init()
- [Low] phy: incomplete hardware teardown on error in zx_pcie_phy_exit()
- [Low] kconfig: PHY_ZX279128S_PCIE selects MFD_SYSCON without HAS_IOMEM
--

commit 9afe9a48cb2bc29d0bfd7f5b0a437a5d1621c1b4
Author: Navid Ghahremani <ghahramani.navid@gmail.com>

phy: zte: Add ZTE ZX279128S PCIe PHY driver

Add a generic PHY driver for the PCIe SerDes PHY inside the Top CRM
block of the ZTE ZX279128S SoC. This driver encapsulates PLL power
and analog SerDes calibration for the two PCIe ports.

> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 19f3b7d12b7d4..3518d384be854 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
[ ... ]
> @@ -91,6 +91,18 @@ config PHY_ECONET_PCIE
>  	  callback for PCIe GEN1 and GEN2 ports. This PHY is found on
>  	  EcoNet SoCs including EN751221 and EN7528.
>  
> +config PHY_ZX279128S_PCIE
> +	tristate "ZTE ZX279128S PCIe PHY driver"
> +	depends on ARCH_ZTE || COMPILE_TEST
> +	depends on OF
> +	select GENERIC_PHY
> +	select MFD_SYSCON

[Severity: Low]
Does selecting MFD_SYSCON here without a HAS_IOMEM dependency cause an unmet
direct dependency warning? Because this driver supports COMPILE_TEST, it could
be built on architectures without HAS_IOMEM (like UML), where MFD_SYSCON
cannot be naturally enabled.

> +	help
> +	  Enable support for the PCIe SerDes PHY found inside the Top CRM
> +	  of ZTE ZX279128S SoCs. This driver configures PLL power and analog
[ ... ]
> diff --git a/drivers/phy/phy-zx279128s-pcie.c b/drivers/phy/phy-zx279128s-pcie.c
> new file mode 100644
> index 0000000000000..33bf0eaac8f74
> --- /dev/null
> +++ b/drivers/phy/phy-zx279128s-pcie.c
[ ... ]
> +static int zx_pcie_phy_init(struct phy *phy)
> +{
> +	struct zx_pcie_phy *zphy = phy_get_drvdata(phy);
> +	const struct zx_pcie_phy_port *port = zphy->port;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(port->phy_on); i++) {
> +		ret = regmap_write_bits(zphy->regmap, CRM1_PHY_CTRL,
> +					port->phy_on[i], port->phy_on[i]);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Can this leave the PHY blocks in an inconsistent state? If regmap_write_bits()
fails on a later iteration (for example, when i > 0), this loop returns
immediately and leaves the hardware resources from previous iterations powered
on.

Since the PHY framework's phy_init() does not invoke the phy_exit() callback
if initialization fails, the driver likely needs to manually unwind the power
bits before returning the error.

> +		fsleep(50);
> +
> +		if (i == 0 && port->has_cal) {
> +			ret = regmap_write(zphy->regmap, CRM1_PHY1_CAL1, 0x00202d5a);
> +			if (ret)
> +				return ret;

[Severity: Medium]
Similarly, if either of these calibration writes fail, we return without
clearing the CRM1_PHY_CTRL bits that were already successfully set. Should
there be an error path here that unwinds the state?

> +			ret = regmap_write(zphy->regmap, CRM1_PHY1_CAL0, 0x0046c24a);
> +			if (ret)
> +				return ret;
> +		}
> +	}
[ ... ]
> +static int zx_pcie_phy_exit(struct phy *phy)
> +{
> +	struct zx_pcie_phy *zphy = phy_get_drvdata(phy);
> +	const struct zx_pcie_phy_port *port = zphy->port;
> +	int i, ret;
> +
> +	for (i = ARRAY_SIZE(port->phy_on) - 1; i >= 0; i--) {
> +		ret = regmap_write_bits(zphy->regmap, CRM1_PHY_CTRL,
> +					port->phy_on[i], 0);
> +		if (ret)
> +			return ret;

[Severity: Low]
Does this early return leave the hardware partially active on teardown? If
regmap_write_bits() fails while clearing one of the bits, we abort the loop
early.

It might be safer to record the error but continue unwinding the remaining
bits to ensure the fullest possible teardown.

> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014072109.1759502-1-ghahramani.navid@gmail.com?part=11

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