[PATCH v3 2/2] phy: airoha: add support for AN7583 Gen3 PCIe PHY driver

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect teardown order and potential resource leak in an7583_pcie_phy_exit().
- [Medium] Calling phy_init() and phy_exit() on a child PHY from within the parent's init/exit callbacks triggers a lockdep recursive locking warning.
- [Low] Missing 'select REGMAP_MMIO' causes build failures.
--

commit d7a329b5d2d570ebb31f2d95a8c4c781915cf2bd
Author: Christian Marangi <ansuelsmth at gmail.com>

phy: airoha: add support for AN7583 Gen3 PCIe PHY driver

This commit adds support for the AN7583 PCIe PHY driver. It introduces the
initialization callback for the PCIe Gen3 port present on the Airoha AN7583
SoC, which requires a different calibration process and register setup
compared to the EN7581.

> diff --git a/drivers/phy/airoha/Kconfig b/drivers/phy/airoha/Kconfig
> new file mode 100644
> index 0000000000000..55fd20f29da65
> --- /dev/null
> +++ b/drivers/phy/airoha/Kconfig
> @@ -0,0 +1,13 @@
[ ... ]
> +config PHY_AIROHA_AN7583_PCIE
> +	tristate "Airoha AN7583 PCIe-PHY Driver"
> +	depends on ARCH_AIROHA || COMPILE_TEST
> +	depends on OF
> +	select GENERIC_PHY

[Severity: Low]
Since the driver uses devm_regmap_init_mmio(), should this Kconfig
entry also select REGMAP_MMIO to prevent build failures? If this driver
is built with COMPILE_TEST on architectures that do not select REGMAP_MMIO
by default, it will cause an undefined reference link error for
__devm_regmap_init_mmio_clk.

> diff --git a/drivers/phy/airoha/phy-an7583-pcie.c b/drivers/phy/airoha/phy-an7583-pcie.c
> new file mode 100644
> index 0000000000000..8aaca3f50713c
> --- /dev/null
> +++ b/drivers/phy/airoha/phy-an7583-pcie.c
[ ... ]
> +static int an7583_pcie_phy_init(struct phy *phy)
> +{
> +	struct an7583_pcie_phy *pcie_phy = phy_get_drvdata(phy);
> +	u32 val;
> +	int ret;
[ ... ]
> +	regmap_set_bits(pcie_phy->g3_pma, REG_SS_DA_XPON_PWDB_0,
> +			G3_PMA_DA_XPON_CDR_PR_PWDB);
> +
> +	usleep_range(100, 200);
> +
> +	ret = phy_init(pcie_phy->qp_phy);

[Severity: Medium]
Will calling phy_init() on the child PHY from within the parent's init
callback trigger a lockdep recursive locking warning? The PHY core already
holds the parent's phy->mutex when executing this callback. Since all PHY
mutexes are initialized to the same lock class in phy_create(), acquiring
the child's mutex here is seen by lockdep as recursive locking on the
same class, which will emit a warning splat and disable lockdep.

> +	if (ret) {
> +		an7583_pcie_phy_deinit(pcie_phy);
> +
> +		return dev_err_probe(pcie_phy->dev, ret, "failed to initialize QP phy\n");
> +	}
> +
> +	/* Wait for the PCIe PHY to complete initialization before returning */
> +	msleep(PHY_HW_INIT_TIME_MS);
> +
> +	return 0;
> +}
> +
> +static int an7583_pcie_phy_exit(struct phy *phy)
> +{
> +	struct an7583_pcie_phy *pcie_phy = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = an7583_pcie_phy_deinit(pcie_phy);
> +	if (ret)
> +		return ret;
> +
> +	return phy_exit(pcie_phy->qp_phy);

[Severity: High]
Does this teardown order safely handle the child PHY? This sequence shuts
down the parent PHY hardware and clocks before calling phy_exit() on the
child PHY. If the child PHY attempts to access parent-provided clocks or
registers during its exit routine, could this cause a bus hang or system
crash?

Additionally, if an7583_pcie_phy_deinit() returns an error, the child PHY
resource will be leaked due to the early return. Should phy_exit() be called
first to exactly reverse the initialization order?

[Severity: Medium]
Similar to the init path, will calling phy_exit() on the child PHY from
within the parent's exit callback trigger a lockdep recursive locking warning
on phy->mutex?

> +}
> +
> +static const struct phy_ops an7583_pcie_phy_ops = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901125819.20378-1-ansuelsmth@gmail.com?part=2