Re: [PATCH net v3 1/2] net: ethernet: bgmac: preserve forced 2.5 Gbps link speed

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The steady-state 2.5G override also hits non-Northstar
  platform AMACs.
- [Low] On forced-2500 devices, the programmed MAC speed and the reported
  link speed now disagree.

--

net: ethernet: bgmac: preserve forced 2.5 Gbps link speed

This commit makes bgmac_adjust_link() use SPEED_2500 instead of the fixed
PHY's reported speed when BGMAC_FEAT_FORCE_SPEED_2500 is set. The goal is
to keep the AMAC at 2.5 Gbps on Northstar and Northstar Plus. Without it,
the first link change reprograms the MAC to 1 Gbps.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index b3a92ae95de30..ec879c203bc59 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1417,11 +1417,15 @@ void bgmac_adjust_link(struct net_device *net_dev)
>  {
>  	struct bgmac *bgmac = netdev_priv(net_dev);
>  	struct phy_device *phy_dev = net_dev->phydev;
> +	int speed = phy_dev->speed;
>  	bool update = false;
>  
> +	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500)
> +		speed = SPEED_2500;

[Severity: Medium]
Does this reach more than Northstar and Northstar Plus? bgmac_probe() in
bgmac-platform.c sets the flag for any platform AMAC that has no
phy-handle, whatever the SoC family:

drivers/net/ethernet/broadcom/bgmac-platform.c:bgmac_probe() {
    ...
	} else {
		bgmac->phy_connect = bgmac_phy_connect_direct;
		bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
	}
    ...
}

Before this patch, the 2.5G setting from bgmac_miiconfig() only lasted
until the first bgmac_adjust_link(). That call reprogrammed the MAC to the
fixed PHY's 1000. With this change, 2.5G becomes the permanent MAC speed.

The in-tree Cygnus board bcm911360_entphn.dts enables eth0 ("brcm,amac"
in bcm-cygnus.dtsi) without a phy-handle, so it gets the flag. Its switch
peer is "brcm,bcm11360-srab" (BCM583XX_DEVICE_ID), and CPU port@8 there is
a fixed-link with speed = <1000>.

The companion change "net: dsa: b53: restore the NSP IMP port 2 Gbps mode"
only forces the IMP port to 2000 for is5301x() or BCM58XX_DEVICE_ID:

drivers/net/dsa/b53/b53_common.c:b53_phylink_mac_link_up() {
    ...
		if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
		    dsa_is_cpu_port(ds, port)) {
    ...
}

Would that leave the Cygnus AMAC at 2.5G and its IMP port at 1G? That is
the same mismatch the series describes as the cause of the output queue
discards.

Hurricane2 ("brcm,nsp-amac") and Stingray ("brcm,amac") would be treated
the same way if their DT had no phy-handle. In-tree Stingray boards do set
phy-handle, and no in-tree HR2 board enables amac0.

On parts without idm_base, bgmac_miiconfig() skips the 2.5G IOCTL clock
select. The MAC speed field would still stay at CMD_SPEED_2500:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_miiconfig() {
	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
		if (!(bgmac->feature_flags & BGMAC_FEAT_IDM_MASK)) {
			bgmac_idm_write(bgmac, BCMA_IOCTL,
					bgmac_idm_read(bgmac, BCMA_IOCTL) |
					0x40 | BGMAC_BCMA_IOCTL_SW_CLKEN);
		}
    ...
}

Should the override be limited to the SoCs that really run this link at
2.5G?

>  	if (phy_dev->link) {
> -		if (phy_dev->speed != bgmac->mac_speed) {
> -			bgmac->mac_speed = phy_dev->speed;
> +		if (speed != bgmac->mac_speed) {
> +			bgmac->mac_speed = speed;
>  			update = true;
>  		}

[Severity: Low]
After this change, can update ever become true on forced 2.5G devices?

bgmac_chip_reset()->bgmac_miiconfig() caches these values before the PHY
state machine runs:

		bgmac->mac_speed = SPEED_2500;
		bgmac->mac_duplex = DUPLEX_FULL;
		bgmac_mac_speed(bgmac);

The fixed PHY reports DUPLEX_FULL, so both comparisons in
bgmac_adjust_link() come out false and this block is skipped:

	if (update) {
		bgmac_mac_speed(bgmac);
		phy_print_status(phy_dev);
	}

Before the patch, the first link-up saw 1000 != 2500, reprogrammed the MAC
and printed the "Link is Up" message. That message seems to be gone on
these boards now.

Also, phy_dev->speed stays at 1000. bgmac_phy_connect_direct() registers
the synthetic fixed PHY with .speed = SPEED_1000, the DT fixed-link uses
speed = <1000>, and swphy can't represent 2500 either way.

bgmac_ethtool_ops uses phy_ethtool_get_link_ksettings(). Would ethtool and
/sys/class/net/<if>/speed then report 1000 Mb/s while the MAC is
programmed for 2.5G?

Should the commit message mention these user-visible changes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026203742.3198673-1-florian.fainelli%40broadcom.com