Re: [PATCH v14 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unprotected re-initialization of shared resources (PLLs and global resets) during per-port bringup disrupts active ports.
- [Medium] State retention across dynamic interface switching due to asymmetric register configuration.
- [Medium] Read-modify-write on write-1-to-clear (W1C) interrupt status registers inadvertently clears unintended pending interrupts.
- [Medium] Missing delay between asserting and deasserting reset controls.
- [Medium] Tight hardware polling loop for RX signal detection lacks a delay, risking false negatives.
- [Low] Unresolved FIXME comments and typographical errors indicate incomplete validation.
--

--- Patch [11]: [PATCH v14 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC ---
commit 3d0ad7799a7613a55a29a2070766d339f6c58307
Author: Christian Marangi <ansuelsmth@gmail.com>

    net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

This commit adds a PCS driver for the Airoha AN7581 SoC for Ethernet, PON,
PCIe, and USB SERDES. It registers the needed registers and acts as a PCS
provider for phylink usage.

> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> new file mode 100644
> index 0000000000000..d9ec0e97b746a
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ -131,10 +131,10 @@ static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
>  		break;
>  	}
>  
>  	ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
>  					priv->rsts);
>  	if (ret)
>  		return ret;
>  
>  	ret = reset_control_bulk_deassert(ARRAY_SIZE(priv->rsts),
>  					  priv->rsts);

[Severity: High]
Since this function is called per-port during airoha_pcs_pre_config(), will
asserting these global bulk resets disrupt an already active port (like the
other port on the 2-port PCIe SerDes)?

[Severity: Medium]
Does this sequence need a delay between asserting and deasserting the reset
controls?

Hardware blocks often require a minimum reset hold time to cleanly reset their
state machines.

[ ... ]

> @@ -272,4 +272,4 @@ static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
>  	/* Interrupt saddly seems to be not weel supported for Link Down.
>  	 * PCS Poll is a must to correctly read and react on Cable Deatch

[Severity: Low]
There are a few typos in this comment ("saddly", "weel", "Deatch"). Could
they be fixed for clarity?

[ ... ]

> @@ -278,9 +278,9 @@ static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
>  	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_4,
>  			  AIROHA_PCS_USXGMII_LINK_DOWN_ST_INT_EN);
>  
>  	/* Clear any pending interrupt */
>  	regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_2,
>  			AIROHA_PCS_USXGMII_RPCS_FSM_DEC_ERR_INT |
>  			AIROHA_PCS_USXGMII_R_TYPE_E_INT |
>  			AIROHA_PCS_USXGMII_R_TYPE_T_INT |
>  			AIROHA_PCS_USXGMII_R_TYPE_D_INT);

[Severity: Medium]
Are these interrupt status registers write-1-to-clear (W1C)?

If so, using regmap_set_bits() performs a read-modify-write. Any currently
pending interrupts read as 1s will be written back as 1s, which could
unintentionally clear all pending interrupts rather than just the ones
specified in the mask.

[ ... ]

> @@ -568,6 +568,6 @@ static int airoha_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
>  	    neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
>  		regmap_set_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_PCS_CTROL_1,
>  				AIROHA_PCS_SGMII_SEND_AN_ERR_EN);
>  
>  		regmap_set_bits(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37,
>  				AIROHA_PCS_HSGMII_AN_FORCE_AN_DONE);
>  	}

[Severity: Medium]
Will these bits remain set if the port is later dynamically reconfigured to
a different interface mode (e.g. switching to SGMII or 1000BASEX with
inband negotiation enabled)?

It appears there is no logic to clear these bits if they were previously
set, which could break the link upon changing modes.

> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> new file mode 100644
> index 0000000000000..bba4a5164a4e2
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
> @@ -255,5 +255,5 @@ static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
>  	/* This comment only apply to Serdes PCIe that expose
>  	 * 2 PCS.
>  	 *
>  	 * The Serdes PCIe expose 2 PCS but always require
>  	 * the PMA for the first PCS to be configured
>  	 * for correct functionality for JCPLL.
>  	 */
>  	pcs_pma = priv->pcs_pma[0];

[Severity: High]
Since pcs_pma[0] is hardcoded here and the JCPLL is shared, will bringing
up a secondary port dynamically disable and restart the shared JCPLL,
causing a disruption on the active first port?

[ ... ]

> @@ -643,2 +643,2 @@ static void an7581_pcs_rx_bringup(struct airoha_pcs_priv *priv,
>  
>  	/* FIXME: Warn and Ask Airoha about typo in air_eth_xsgmii.c line 1391 */
>  	/* AIROHA_PCS_ANA_REV_1_FE_BUF1_BIAS_CTRL is set 0x0 in SDK but seems a typo */

[Severity: Low]
Is this FIXME intended to be resolved before merging?

It might be best to verify this hardware assumption with the vendor to
ensure it won't hide any bugs.

[ ... ]

> @@ -2072,6 +2072,6 @@ static bool an7581_pcs_have_rx_signal(struct airoha_pcs_priv *priv, int index)
>  	regmap_write(pcs_pma, AIROHA_PCS_PMA_DIG_RESERVE_0,
>  		     AIROHA_PCS_TRIGGER_RX_SIDGET_SCAN);
>  
>  	/* Scan 6 times for RX sigdet module to detect RX signal */
>  	for (i = 0; i < AIROHA_PCS_MAX_RX_SIGDET_TRY; i++) {
>  		regmap_read(pcs_pma, AIROHA_PCS_PMA_DIG_RO_RESERVE_2,
>  			    &val);
>  		if (val & AIROHA_PCS_RX_SIGDET)
>  			count++;
>  	}

[Severity: Medium]
Does this tight polling loop need a delay (such as usleep_range())?

Currently, the CPU will execute these reads across the MMIO bus very rapidly.
Since analog circuitry typically requires settling time to assert signal
detection, might this lead to false negatives?

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