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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/29/26 13:39, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 

The scope concern is valid. I will restrict the platform forced-speed 
flag to Northstar Plus; the BCMA path already selects the Northstar 
family explicitly.

I will also decouple link-status printing from MAC reconfiguration. 
Userspace will continue reporting the synthetic fixed PHY’s existing 
1Gbps speed, and I will document that limitation in the changelog.

pw-bot: cr
-- 
Florian