Re: [PATCH net-next] net: phy: bcm84881: add BCM84891/BCM84892 support

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Mar 24, 2026 at 03:25:04PM +0000, Daniel Wagner wrote:
> +static bool bcm84881_is_bcm8489x(struct phy_device *phydev)
> +{
> +	/* For C45 PHYs, phydev->phy_id is 0; match via the driver entry's
> +	 * declared ID, which is what phy_bus_match() used to bind us.
> +	 */
> +	u32 id = phydev->drv->phy_id;
> +
> +	return (id & 0xfffffff0) == 0x35905080 ||
> +	       (id & 0xfffffff0) == 0x359050a0;

With the .phy_id's fixed (dropping the revision field) you can simply
test phydev->drv->phy_id here. Alternatively, consider using
phy_id_compare_model().

> +}
> +
>  static void bcm84881_fill_possible_interfaces(struct phy_device *phydev)
>  {
>  	unsigned long *possible = phydev->possible_interfaces;
> @@ -42,10 +57,22 @@ static int bcm84881_config_init(struct phy_device *phydev)
>  {
>  	bcm84881_fill_possible_interfaces(phydev);
>  
> +	if (bcm84881_is_bcm8489x(phydev)) {
> +		__set_bit(PHY_INTERFACE_MODE_USXGMII,
> +			  phydev->possible_interfaces);
> +		/* Bootloader may have left the speed LED on, and
> +		 * link_change_notify won't fire for ports with no cable.
> +		 * Clear both color bits (green ignores the gate register).
> +		 */
> +		phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD,
> +				   BCM8489X_LED_COLOR, 0x0012);
> +	}
> +
>  	switch (phydev->interface) {
>  	case PHY_INTERFACE_MODE_SGMII:
>  	case PHY_INTERFACE_MODE_2500BASEX:
>  	case PHY_INTERFACE_MODE_10GBASER:
> +	case PHY_INTERFACE_MODE_USXGMII:

This also allows USXGMII for BCM84881, which it doesn't support. This is
wrong. Does the bcm8489x support these other modes? If not, this is more
wrong - it should be a separate function - bcm8489x_config_init().

>  		break;
>  	default:
>  		return -ENODEV;
> @@ -96,6 +123,17 @@ static int bcm84881_config_aneg(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> +	/* BCM84891/92 firmware has been observed setting MDIO_CTRL1_LPOWER
> +	 * autonomously (e.g. during SerDes reconfiguration). Clear it so AN
> +	 * can proceed.
> +	 */
> +	if (bcm84881_is_bcm8489x(phydev)) {
> +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
> +					 MDIO_CTRL1_LPOWER);
> +		if (ret < 0)
> +			return ret;
> +	}
> +

This function only gets called when the PHY is being started, resumed,
or when the user changes the advertisement. If firmware switches to low
power when e.g. the results of the media negotiation change, then this
isn't the correct place to do this. When exactly does firmware set the
PHY to low power mode?

>  	/* We don't support manual MDI control */
>  	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>  
> @@ -201,6 +239,15 @@ static int bcm84881_read_status(struct phy_device *phydev)
>  		return 0;
>  	}
>  
> +	/* For BCM84891/92 in USXGMII mode, the host-side register 0x4011
> +	 * reports 10GBASER (the USXGMII SerDes line rate) regardless of the
> +	 * negotiated copper speed. Skip it; phy_resolve_aneg_linkmode()
> +	 * above already set the correct speed from standard AN resolution.
> +	 */

Is the PHY really using 10GBASE-R or is it using USXGMII. The two are
different. They don't just define the SerDes rate, but the protocol
that operates over it. 10GBASE-R has no inband signalling whereas
USXGMII does and supports symbol replication for slower speeds.

If the PHY is actually using 10GBASE-R, but with rate adaption (so
converting between the media speed and 10GBASE-R which only runs at 10G
rate) then you need more in this driver (see rate adaption stuff.)

If the PHY is really using USXGMII, where it will do symbol replication,
for the slower speeds over the 10G host-side link then the rate
adaption stuff is unnecessary.

Is there a code in the 0x4011 register which indicates if it uses
USXGMII as opposed to 10GBASE-R ? Please clarify.

> +	if (bcm84881_is_bcm8489x(phydev) &&
> +	    phydev->interface == PHY_INTERFACE_MODE_USXGMII)
> +		return genphy_c45_read_mdix(phydev);
> +
>  	/* Set the host link mode - we set the phy interface mode and
>  	 * the speed according to this register so that downshift works.
>  	 * We leave the duplex setting as per the resolution from the
> @@ -235,6 +282,26 @@ static int bcm84881_read_status(struct phy_device *phydev)
>  	return genphy_c45_read_mdix(phydev);
>  }
>  
> +/* BCM8489x speed LED control.
> + * Dev1:a83b bit 1 (0x0002) = green, bit 4 (0x0010) = amber.
> + * Dev1:a82f = 0x0020 gates amber on; green ignores the gate.
> + * Writes are best-effort (LED is cosmetic; this callback is void).
> + */
> +static void bcm84881_link_change_notify(struct phy_device *phydev)

While the driver is called bcm84881, I'd prefer if we didn't name
functions with the same prefix when they have nothing to do with the
bcm84881 PHY. bcm8489x_link_change_notify() would be more suitable.

> +{
> +	if (phydev->link) {
> +		/* 10G = green, else amber; matches vendor firmware */
> +		u16 color = (phydev->speed == SPEED_10000) ? 0x0002 : 0x0010;
> +
> +		phy_write_mmd(phydev, MDIO_MMD_PMAPMD, BCM8489X_LED_GATE, 0x0020);
> +		phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, BCM8489X_LED_COLOR,
> +			       0x0012, color);
> +	} else {
> +		phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD,
> +				   BCM8489X_LED_COLOR, 0x0012);
> +	}

... however, this is not how we support LEDs. Please look at the PHY
LEDs support using the generic LEDs support.

> +}
> +
>  /* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
>   * or 802.3z control word, so inband will not work.
>   */
> @@ -257,6 +324,32 @@ static struct phy_driver bcm84881_drivers[] = {
>  		.aneg_done	= bcm84881_aneg_done,
>  		.read_status	= bcm84881_read_status,
>  	},
> +	{
> +		.phy_id		= 0x35905081,
> +		.phy_id_mask	= 0xfffffff0,

Why set the revision in .phy_id when you mask it off? Please use
PHY_ID_MATCH_MODEL().

> +		.name		= "Broadcom BCM84891",
> +		.inband_caps	= bcm84881_inband_caps,
> +		.config_init	= bcm84881_config_init,
> +		.probe		= bcm84881_probe,
> +		.get_features	= bcm84881_get_features,
> +		.config_aneg	= bcm84881_config_aneg,
> +		.aneg_done	= bcm84881_aneg_done,
> +		.read_status	= bcm84881_read_status,
> +		.link_change_notify = bcm84881_link_change_notify,
> +	},
> +	{
> +		.phy_id		= 0x359050a1,
> +		.phy_id_mask	= 0xfffffff0,

Same here.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!