From: sashiko-bot@kernel.org
To: "Christian Marangi" <ansuelsmth@gmail.com>
Cc: conor+dt@kernel.org, devicetree@vger.kernel.org, robh@kernel.org
Subject: Re: [PATCH net-next v17 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC
Date: Fri, 25 Sep 2026 23:03:04 +0000 [thread overview]
Message-ID: <20260925230305.0BD571F0089A@smtp.kernel.org> (raw)
In-Reply-To: <20260924230211.11519-12-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: pcs: airoha: uncoordinated shared resource resets disrupt active multi-port links
- [High] net: pcs: airoha: logic bug in FIELD_PREP clobbers XTAL_TOP_RESERVE register
- [Medium] net: pcs: airoha: missing default switch case causes uninitialized variable usage
- [Low] net: pcs: airoha: mismatch between comment and macro for maximum calibration retries
--
commit 6e5693f88efad46bf7131d0ace5d0278a3c0f5ca
Author: Christian Marangi <ansuelsmth@gmail.com>
net: pcs: airoha: add PCS driver for Airoha AN7581 SoC
This commit adds a new PCS driver for the Airoha AN7581 SoC. It supports Ethernet, PON, PCIe, and USB SERDES configurations, with multiple interface modes including USXGMII and SGMII.
> diff --git a/drivers/net/pcs/airoha/pcs-airoha.h b/drivers/net/pcs/airoha/pcs-airoha.h
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha.h
> @@ -0,0 +1,1311 @@
[ ... ]
> +#define AIROHA_PCS_MAX_CALIBRATION_TRY 50
[Severity: Low]
Does this macro definition mismatch the logic described in the comments?
The comment in an7581_pcs_phya_bringup() indicates 10 retries are used, but the
macro evaluates to 50.
> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ -0,0 +1,1303 @@
[ ... ]
> +static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
> + const struct airoha_pcs_match_data *data = priv->data;
> + int ret;
> +
> + switch (data->port_type) {
> + case AIROHA_PCS_ETH:
> + airoha_pcs_setup_scu_eth(priv, interface);
> + break;
> + case AIROHA_PCS_PON:
> + airoha_pcs_setup_scu_pon(priv, interface);
> + break;
> + case AIROHA_PCS_PCIE:
> + airoha_pcs_setup_scu_pcie(priv, index, interface);
> + break;
> + case AIROHA_PCS_USB:
> + break;
> + }
> +
> + ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
> + priv->rsts);
[Severity: High]
Does this disrupt other active ports on the same chip?
It looks like bringing up a secondary port unconditionally asserts a device-wide
bulk reset here, which would destroy the link state of already active ports.
> + if (ret)
> + return ret;
> +
> + ret = reset_control_bulk_deassert(ARRAY_SIZE(priv->rsts),
> + priv->rsts);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
[ ... ]
> +static void airoha_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
> + phy_interface_t interface, int speed, int duplex)
> +{
[ ... ]
> + if (interface == PHY_INTERFACE_MODE_SGMII ||
> + interface == PHY_INTERFACE_MODE_2500BASEX) {
> + u32 ck_gen_mode;
> + u32 speed_reg;
> + u32 if_mode;
> +
> + switch (speed) {
> + case SPEED_2500:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_2_5G;
> + break;
> + case SPEED_1000:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_1G;
> + if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_1000;
> + ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_1000;
> + break;
> + case SPEED_100:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_100M;
> + if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_100;
> + ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_100;
> + break;
> + case SPEED_10:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_10M;
> + if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_10;
> + ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_10;
> + break;
> + }
> +
> + if (interface == PHY_INTERFACE_MODE_SGMII) {
> + regmap_update_bits(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_13,
> + AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE,
> + if_mode);
> +
> + regmap_update_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_PCS_AN_SGMII_MODE_FORCE,
> + AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE |
> + AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_SEL,
> + ck_gen_mode |
> + AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_SEL);
> + }
> +
> + regmap_update_bits(maps->multi_sgmii, AIROHA_PCS_MULTI_SGMII_SGMII_STS_CTRL_0,
> + AIROHA_PCS_LINK_MODE_P0 |
> + AIROHA_PCS_FORCE_SPD_MODE_P0,
> + speed_reg |
[Severity: Medium]
What happens if an unexpected speed is provided here?
Because there is no default switch case catching unsupported speeds (unlike other
switch statements in the same function), variables like speed_reg remain
uninitialized. If this occurs, uninitialized stack garbage will be written to
hardware registers.
> + AIROHA_PCS_FORCE_SPD_MODE_P0);
> + }
> + }
> +
> + if (data->link_up)
> + data->link_up(priv, port->index);
[ ... ]
> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
> @@ -0,0 +1,2100 @@
[ ... ]
> +static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
[ ... ]
> + /* 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];
> +
> + /* Setup LDO */
> + usleep_range(200, 300);
> +
> + regmap_field_set_bits(pcs_ana_fields[AN7581_PCS_JCPLL_SPARE_L],
> + AIROHA_PCS_ANA_JCPLL_SPARE_L_LDO);
> +
> + /* Setup RSTB */
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_JCPLL_RST_DLY],
> + AIROHA_PCS_ANA_JCPLL_RST_DLY_150_200);
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_JCPLL_PLL_RSTB], 0x1);
> +
> + /* Enable PLL force selection and Force Disable */
> + regmap_update_bits(pcs_pma, AIROHA_PCS_PMA_PXP_JCPLL_CKOUT_EN,
> + AIROHA_PCS_PMA_FORCE_SEL_DA_JCPLL_EN |
> + AIROHA_PCS_PMA_FORCE_DA_JCPLL_EN,
> + AIROHA_PCS_PMA_FORCE_SEL_DA_JCPLL_EN);
[Severity: High]
Could this reconfigure the shared JCPLL without locks and disrupt an already
active port 0?
Since pcs_pma[0] is modified directly without checking if another port is already
using it, configuring a secondary port may break the primary one.
[ ... ]
> +static void an7581_pcs_txpll_bringup(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
[ ... ]
> + /* 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 TXPLL.
> + */
> + pcs_pma = priv->pcs_pma[0];
> +
> + /* Setup VCO LDO Output */
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_LDO_VCO_OUT], 0x1);
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_LDO_OUT], 0x1);
> +
> + /* Setup RSTB */
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_PLL_RSTB], 0x1);
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_RST_DLY], 0x4);
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_REFIN_DIV],
> + AIROHA_PCS_ANA_TXPLL_REFIN_DIV_1);
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_REFIN_INTERNAL], 0x1);
> +
> + /* Enable PLL force selection and Force Disable */
> + regmap_update_bits(pcs_pma, AIROHA_PCS_PMA_PXP_TXPLL_CKOUT_EN,
> + AIROHA_PCS_PMA_FORCE_SEL_DA_TXPLL_EN |
> + AIROHA_PCS_PMA_FORCE_DA_TXPLL_EN,
> + AIROHA_PCS_PMA_FORCE_SEL_DA_TXPLL_EN);
[Severity: High]
Does this shared TXPLL modification have the same issue as the JCPLL above?
It appears that a secondary port bringup will unconditionally reconfigure
this shared resource without synchronization.
[ ... ]
> +static int an7581_pcs_phya_bringup(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
> + struct regmap *pcs_pma = priv->pcs_pma[index];
> + int calibration_try = 0;
> + u32 val = 0;
> +
> + an7581_pcs_tx_bringup(priv, index, interface);
> + an7581_pcs_rx_bringup(priv, index, interface);
> +
> + usleep_range(100, 200);
> +
> +retry_calibration:
> + an7581_pcs_cdr_reset(priv, index, interface, priv->manual_rx_calib);
> +
> + /* Global reset clear */
> + regmap_update_bits(pcs_pma, AIROHA_PCS_PMA_SW_RST_SET,
> + AIROHA_PCS_PMA_SW_HSG_RXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_HSG_TXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_XFI_RXPCS_BIST_RST_N |
> + AIROHA_PCS_PMA_SW_XFI_RXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_XFI_TXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_TX_FIFO_RST_N |
> + AIROHA_PCS_PMA_SW_REF_RST_N |
> + AIROHA_PCS_PMA_SW_ALLPCS_RST_N |
> + AIROHA_PCS_PMA_SW_PMA_RST_N |
> + AIROHA_PCS_PMA_SW_TX_RST_N |
> + AIROHA_PCS_PMA_SW_RX_RST_N |
> + AIROHA_PCS_PMA_SW_RX_FIFO_RST_N,
> + AIROHA_PCS_PMA_SW_REF_RST_N);
> +
> + usleep_range(100, 200);
> +
> + /* Global reset */
> + regmap_set_bits(pcs_pma, AIROHA_PCS_PMA_SW_RST_SET,
> + AIROHA_PCS_PMA_SW_HSG_RXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_HSG_TXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_XFI_RXPCS_BIST_RST_N |
> + AIROHA_PCS_PMA_SW_XFI_RXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_XFI_TXPCS_RST_N |
> + AIROHA_PCS_PMA_SW_TX_FIFO_RST_N |
> + AIROHA_PCS_PMA_SW_REF_RST_N |
> + AIROHA_PCS_PMA_SW_ALLPCS_RST_N |
[Severity: High]
Is it safe to trigger a hardware-level ALLPCS_RST here?
Applying this reset from an individual port's bring-up path seems like it
would destroy the state across all active PCS ports on the chip.
> + AIROHA_PCS_PMA_SW_PMA_RST_N |
> + AIROHA_PCS_PMA_SW_TX_RST_N |
> + AIROHA_PCS_PMA_SW_RX_RST_N |
> + AIROHA_PCS_PMA_SW_RX_FIFO_RST_N);
> +
> + usleep_range(5000, 7000);
> +
> + an7581_pcs_cdr_reset(priv, index, interface, false);
[ ... ]
> + /* It was discovered that after a global reset and auto mode gets
> + * actually enabled, the fl_out from calibration might change and
> + * might deviates a lot from the expected value it was calibrated for.
> + * To correctly work, the PCS FreqDet module needs to Lock to the fl_out
> + * (frequency level output) or no signal can correctly be transmitted.
> + * This is detected by checking the FreqDet module Lock bit.
> + *
> + * If it's detected that the FreqDet module is not locked, retry
> + * calibration. From observation on real hardware with a 10g SFP module,
> + * it required a maximum of an additional calibration to actually make
> + * the FreqDet module to lock. Try 10 times before failing to handle
> + * really strange case.
> + */
> + regmap_read(pcs_pma, AIROHA_PCS_PMA_RX_FREQDET, &val);
> + if (!(val & AIROHA_PCS_PMA_FBCK_LOCK)) {
> + if (calibration_try > AIROHA_PCS_MAX_CALIBRATION_TRY) {
[Severity: Low]
Does this macro evaluation mismatch the logic described in the comments?
The comment above indicates 10 retries are used, but the macro evaluates to 50.
> + dev_err(priv->dev, "No FBCK Lock from FreqDet module after %d calibration try. PCS won't work.\n",
> + AIROHA_PCS_MAX_CALIBRATION_TRY);
> + return -EIO;
> + }
> +
> + calibration_try++;
[ ... ]
> +int an7581_pcs_usb_bringup(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
> + int ret;
[ ... ]
> + regmap_update_bits(priv->pcs_ana, AIROHA_PCS_HSGMII_ANA_SGMII_PHYA_18,
> + AIROHA_PCS_HSGMII_ANA_SSUSB_BG_DIV,
> + FIELD_PREP(AIROHA_PCS_HSGMII_ANA_SSUSB_BG_DIV, 0x1));
> +
> + regmap_update_bits(priv->pcs_ana, AIROHA_PCS_HSGMII_ANA_SGMII_PHYA_19,
> + AIROHA_PCS_HSGMII_ANA_SSUSB_XTAL_TOP_RESERVE,
> + FIELD_PREP(AIROHA_PCS_HSGMII_ANA_SSUSB_XTAL_TOP_RESERVE,
> + FIELD_PREP(AIROHA_PCS_HSGMII_ANA_SSUSB_XTAL_TOP_RESERVE_HV,
> + AIROHA_PCS_HSGMII_ANA_SSUSB_XTAL_TOP_RESERVE_NS_MONPLL_CK)));
[Severity: High]
Does this logic bug clobber the XTAL_TOP_RESERVE register?
The AIROHA_PCS_HSGMII_ANA_SSUSB_XTAL_TOP_RESERVE_NS_MONPLL_CK constant is nested
inside a FIELD_PREP for the HV subfield rather than the MONCKBG subfield. This
causes the bit pattern to be placed at the wrong offset (bit 19 instead of
within 7:0) and the outer FIELD_PREP zeroes out everything else, clearing the LV
subfield.
> +
> + if (interface == PHY_INTERFACE_MODE_2500BASEX)
> + regmap_update_bits(priv->pcs_ana, AIROHA_PCS_HSGMII_ANA_SGMII_PHYA_11,
> + AIROHA_PCS_HSGMII_ANA_TPHY_SPEED,
> + AIROHA_PCS_HSGMII_ANA_TPHY_SPEED_HSGMII);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260924230211.11519-1-ansuelsmth@gmail.com?part=11
next prev parent reply other threads:[~2026-09-25 23:03 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-24 23:01 [PATCH net-next v17 00/12] net: pcs: Introduce support for fwnode PCS Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 01/12] net: phylink: keep and use MAC supported_interfaces in phylink struct Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 02/12] net: phylink: introduce internal phylink PCS handling Christian Marangi
2026-09-25 23:03 ` sashiko-bot
2026-09-24 23:01 ` [PATCH net-next v17 03/12] net: pcs: implement Firmware node support for PCS driver Christian Marangi
2026-09-25 23:03 ` sashiko-bot
2026-09-24 23:01 ` [PATCH net-next v17 04/12] net: phylink: save phylink instance fwnode on phylink_create Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 05/12] net: phylink: support PCS provider release Christian Marangi
2026-09-25 23:03 ` sashiko-bot
2026-09-24 23:01 ` [PATCH net-next v17 06/12] net: phylink: support late PCS provider attach Christian Marangi
2026-09-25 23:03 ` sashiko-bot
2026-09-24 23:01 ` [PATCH net-next v17 07/12] net: Document PCS subsystem Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 08/12] MAINTAINERS: add myself as PCS subsystem maintainer Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 09/12] net: phylink: add .pcs_link_down PCS OP Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 10/12] dt-bindings: net: pcs: Document support for Airoha Ethernet PCS Christian Marangi
2026-09-24 23:01 ` [PATCH net-next v17 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC Christian Marangi
2026-09-25 23:03 ` sashiko-bot [this message]
2026-09-24 23:01 ` [PATCH net-next v17 12/12] net: airoha: add phylink support Christian Marangi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260925230305.0BD571F0089A@smtp.kernel.org \
--to=sashiko-bot@kernel.org \
--cc=ansuelsmth@gmail.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=robh@kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.