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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] phy: airoha: use of devm_phy_optional_get() for required qp_phy
- [Medium] phy: airoha: G3_ANA_PXP_JCPLL_SSC_EN not cleared on init or deinit
- [Low] phy: airoha: misuse of dev_err_probe() in an7583_pcie_phy_init()
- [Low] phy: airoha: unused FREQ_LOCK_MAX_ATTEMPT macro and MAINTAINERS order
--

commit 2954f920d212f21a88edba191990e99091d7cd13
Author: Christian Marangi <ansuelsmth@gmail.com>

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

This commit adds a new PHY driver for the Gen3 PCIe port on the Airoha
AN7583 SoC, implementing its register map and calibration sequence.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 93abbb315065d..fb88a25ce0381 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -788,6 +788,14 @@ F:	Documentation/devicetree/bindings/phy/airoha,en7581-pcie-phy.yaml
>  F:	drivers/phy/phy-airoha-pcie-regs.h
>  F:	drivers/phy/phy-airoha-pcie.c
>  
> +AIROHA AN7583 PCIE PHY DRIVER
> +M:	Christian Marangi <ansuelsmth@gmail.com>
> +L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/phy/airoha,an7583-pcie-gen3-phy.yaml
> +F:	drivers/phy/airoha/phy-an7583-pcie-regs.h
> +F:	drivers/phy/airoha/phy-an7583-pcie.c
> +
>  AIROHA SOUND DRIVER
>  M:	Christian Marangi <ansuelsmth@gmail.com>
>  L:	linux-sound@vger.kernel.org
[ ... ]
> diff --git a/drivers/phy/airoha/phy-an7583-pcie.c b/drivers/phy/airoha/phy-an7583-pcie.c
> new file mode 100644
> index 0000000000000..f3c42654895f8
> --- /dev/null
> +++ b/drivers/phy/airoha/phy-an7583-pcie.c
> @@ -0,0 +1,823 @@
[ ... ]
> +#include "phy-an7583-pcie-regs.h"
> +
> +#define FREQ_LOCK_MAX_ATTEMPT	50

[Severity: Low]
This isn't a bug, but FREQ_LOCK_MAX_ATTEMPT is defined in
drivers/phy/airoha/phy-an7583-pcie.c without being referenced anywhere in
the driver. Also, in MAINTAINERS, AIROHA AN7583 PCIE PHY DRIVER is placed
after AIROHA PCIE PHY DRIVER; should FREQ_LOCK_MAX_ATTEMPT be dropped and
the MAINTAINERS entry be placed in alphabetical order before AIROHA
ETHERNET DRIVER?

[ ... ]
> +static void an7583_pcie_phy_init_jcpll(struct an7583_pcie_phy *pcie_phy)
> +{
> +	regmap_set_bits(pcie_phy->g3_pma, REG_FORCE_DA_PXP_JCPLL_CKOUT_EN,
> +			G3_PMA_FORCE_SEL_DA_PXP_JCPLL_EN);
> +	regmap_clear_bits(pcie_phy->g3_pma, REG_FORCE_DA_PXP_JCPLL_CKOUT_EN,
> +			  G3_PMA_FORCE_DA_PXP_JCPLL_EN);
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SPARE_H,
> +			   G3_ANA_PXP_JCPLL_SPARE_L,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SPARE_L, 0x20));
> +	regmap_set_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_RST_DLY,
> +			G3_ANA_PXP_JCPLL_PLL_RSTB);
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_DELTA,
> +			   G3_ANA_PXP_JCPLL_SSC_DELTA,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SSC_DELTA, 0x0));
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_TRI_EN,
> +			   G3_ANA_PXP_JCPLL_SSC_DELTA1,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SSC_DELTA1, 0x0));
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_DELTA,
> +			   G3_ANA_PXP_JCPLL_SSC_PERIOD,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SSC_PERIOD, 0x0));
> +	regmap_clear_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_VCO_TCLVAR,
> +			  G3_ANA_PXP_JCPLL_SSC_PHASE_INI);
> +	regmap_clear_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_TRI_EN,
> +			  G3_ANA_PXP_JCPLL_SSC_TRI_EN);
[ ... ]
> +static void an7583_pcie_phy_init_ssc_jcpll(struct an7583_pcie_phy *pcie_phy)
> +{
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_DELTA,
> +			   G3_ANA_PXP_JCPLL_SSC_DELTA,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SSC_DELTA, 0x106));
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_TRI_EN,
> +			   G3_ANA_PXP_JCPLL_SSC_DELTA1,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SSC_DELTA1, 0x106));
> +	regmap_update_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_DELTA,
> +			   G3_ANA_PXP_JCPLL_SSC_PERIOD,
> +			   FIELD_PREP(G3_ANA_PXP_JCPLL_SSC_PERIOD, 0x31B));
> +	regmap_set_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_VCO_TCLVAR,
> +			G3_ANA_PXP_JCPLL_SSC_PHASE_INI);
> +	regmap_set_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_VCO_TCLVAR,
> +			G3_ANA_PXP_JCPLL_SSC_EN);
> +	regmap_set_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SDM_IFM,
> +			G3_ANA_PXP_JCPLL_SDM_IFM);
> +	regmap_set_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SDM_HREN,
> +			G3_ANA_PXP_JCPLL_SDM_HREN);
> +	regmap_clear_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_RST_DLY,
> +			  G3_ANA_PXP_JCPLL_SDM_DI_EN);
> +	regmap_set_bits(pcie_phy->g3_ana, REG_PXP_JCPLL_SSC_TRI_EN,
> +			G3_ANA_PXP_JCPLL_SSC_TRI_EN);
> +}
[ ... ]
> +static int an7583_pcie_phy_deinit(struct an7583_pcie_phy *pcie_phy)
> +{
> +	/* Disable RX CDR/PR */
> +	regmap_clear_bits(pcie_phy->g3_pma, REG_SS_DA_XPON_PWDB_0,
> +			  G3_PMA_DA_XPON_CDR_PR_PWDB);
> +
> +	/* Disable TX PLL output/enable */
> +	regmap_clear_bits(pcie_phy->g3_pma, REG_FORCE_DA_PXP_TXPLL_CKOUT_EN,
> +			  G3_PMA_FORCE_DA_PXP_TXPLL_CKOUT_EN |
> +			  G3_PMA_FORCE_SEL_DA_PXP_TXPLL_CKOUT_EN |
> +			  G3_PMA_FORCE_SEL_DA_PXP_TXPLL_EN |
> +			  G3_PMA_FORCE_DA_PXP_TXPLL_EN);
> +
> +	/* Disable JCPLL output/enable */
> +	regmap_clear_bits(pcie_phy->g3_pma, REG_FORCE_DA_PXP_JCPLL_CKOUT_EN,
> +			  G3_PMA_FORCE_DA_PXP_JCPLL_CKOUT_EN |
> +			  G3_PMA_FORCE_SEL_DA_PXP_JCPLL_CKOUT_EN |
> +			  G3_PMA_FORCE_SEL_DA_PXP_JCPLL_EN |
> +			  G3_PMA_FORCE_DA_PXP_JCPLL_EN);
> +
> +	/* Assert PHY resets */
> +	regmap_clear_bits(pcie_phy->g3_pma, REG_SW_RST_SET,
> +			  G3_PMA_SW_TX_FIFO_RST_N |
> +			  G3_PMA_SW_ALLPCS_RST_N |
> +			  G3_PMA_SW_PMA_RST_N |
> +			  G3_PMA_SW_TX_RST_N |
> +			  G3_PMA_SW_RX_FIFO_RST_N |
> +			  G3_PMA_SW_XFI_RXPCS_RST_N |
> +			  G3_PMA_SW_REF_RST_N |
> +			  G3_PMA_SW_RX_RST_N);
> +
> +	/* Disable TX analog blocks */
> +	regmap_clear_bits(pcie_phy->g3_ana, REG_PXP_TX_CKLDO_EN,
> +			  G3_ANA_PXP_TX_CKLDO_EN |
> +			  G3_ANA_PXP_TX_DMEDGEGEN_EN);
> +
> +	return 0;
> +}

[Severity: Medium]
When an7583_pcie_phy_init() runs, an7583_pcie_phy_init_ssc_jcpll() enables
JCPLL spread-spectrum clocking by setting G3_ANA_PXP_JCPLL_SSC_PHASE_INI
and G3_ANA_PXP_JCPLL_SSC_EN in REG_PXP_JCPLL_VCO_TCLVAR, and
G3_ANA_PXP_JCPLL_SSC_TRI_EN in REG_PXP_JCPLL_SSC_TRI_EN.

However, an7583_pcie_phy_deinit() does not clear these JCPLL SSC bits (in
contrast to airoha_pcie_phy_exit() in drivers/phy/phy-airoha-pcie.c, which
clears CSR_2L_PXP_JCPLL_SSC_PHASE_INI | CSR_2L_PXP_JCPLL_SSC_TRI_EN |
CSR_2L_PXP_JCPLL_SSC_EN), and Phase 1 in an7583_pcie_phy_init_jcpll() only
clears G3_ANA_PXP_JCPLL_SSC_PHASE_INI and G3_ANA_PXP_JCPLL_SSC_TRI_EN
without clearing G3_ANA_PXP_JCPLL_SSC_EN.

If the PHY is re-initialized after phy_exit() or after a failed phy_init()
attempt (or if the bootloader left JCPLL SSC enabled), will
G3_ANA_PXP_JCPLL_SSC_EN remain set during Phase 1 JCPLL and TXPLL
configuration when SSC is supposed to be disabled?

> +
> +static int an7583_pcie_phy_init(struct phy *phy)
> +{
[ ... ]
> +	ret = phy_init(pcie_phy->qp_phy);
> +	if (ret) {
> +		an7583_pcie_phy_deinit(pcie_phy);
> +
> +		return dev_err_probe(pcie_phy->dev, ret, "failed to initialize QP phy\n");
> +	}

[Severity: Low]
Is dev_err_probe() appropriate inside the phy_ops .init callback
an7583_pcie_phy_init()?

Because an7583_pcie_phy_init() runs when a consumer PCIe controller driver
calls phy_init() after an7583_pcie_phy_probe() has already bound
pcie_phy->dev, calling dev_err_probe(pcie_phy->dev, ret, ...) will suppress
error messages completely if ret is -ENOMEM, or downgrade the message to
dev_dbg() and allocate pcie_phy->dev->p->deferred_probe_reason on an
already-bound device if ret is -EPROBE_DEFER. Should this use dev_err()
instead?

[ ... ]
> +static int an7583_pcie_phy_probe(struct platform_device *pdev)
> +{
> +	struct an7583_pcie_phy *pcie_phy;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *provider;
> +
> +	pcie_phy = devm_kzalloc(dev, sizeof(*pcie_phy), GFP_KERNEL);
> +	if (!pcie_phy)
> +		return -ENOMEM;
> +
> +	pcie_phy->g3_ana = an7583_pcie_phy_regmap_init(pdev, "g3-ana");
> +	if (IS_ERR(pcie_phy->g3_ana))
> +		return dev_err_probe(dev, PTR_ERR(pcie_phy->g3_ana),
> +				     "Failed to initialize g3 ANA regmap\n");
> +
> +	pcie_phy->qp_phy = devm_phy_optional_get(dev, NULL);
> +	if (IS_ERR(pcie_phy->qp_phy))
> +		return dev_err_probe(dev, PTR_ERR(pcie_phy->qp_phy),
> +				     "Failed to get QP PCIe phy\n");

[Severity: Medium]
Should an7583_pcie_phy_probe() use devm_phy_get() instead of
devm_phy_optional_get() for pcie_phy->qp_phy?

In Documentation/devicetree/bindings/phy/airoha,an7583-pcie-gen3-phy.yaml,
phys is a required property because QPHY configuration is needed during the
PCIe PHY setup and calibration phase. With devm_phy_optional_get(dev, NULL),
a missing phys property or a disabled QPHY node in DeviceTree causes
_of_phy_get() to return ERR_PTR(-ENODEV), which devm_phy_optional_get()
turns into NULL. Both an7583_pcie_phy_probe() and phy_init(pcie_phy->qp_phy)
in an7583_pcie_phy_init() will then succeed silently without initializing
the required QPHY.

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