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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox