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

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

On 11/2/26 13:50, 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] After this patch the speed seen by userspace and phylib no
>    longer matches the MAC.
> - [Low] The patch depends on a switch-side change that is not named.
> 
> --
> 
> net: ethernet: bgmac: preserve forced 2.5 Gbps link speed
> 
> When BGMAC_FEAT_FORCE_SPEED_2500 is set, this commit makes
> bgmac_adjust_link() keep the MAC at SPEED_2500 instead of following the
> fixed PHY's 1 Gbps speed. It also restricts the platform forced-speed
> flag to Northstar Plus via of_machine_is_compatible("brcm,nsp").
> 
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 669eb7ce7d40b..9ef7307edd41e 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -1430,11 +1430,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: Low]
> Does this patch depend on the b53 change that puts the IMP port in its
> 2 Gbps mode? The commit message says:
> 
>    Together with configuring the switch IMP port for its 2 Gbps
>    mode, this eliminates the output queue discards ...
> 
> but it does not name the commit that makes that change.
> 
> With only this patch applied, the AMAC stays at SPEED_2500. Meanwhile
> b53_force_port_config() still programs the IMP port from the DT
> fixed-link (speed = <1000> in bcm-ns.dtsi) and clears the 2G bit:
> 
> drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
>      ...
> 	if (is5301x(dev) || is58xx(dev))
> 		reg &= ~PORT_OVERRIDE_SPEED_2000M;
>      ...
> }
> 
> The next patch in this series, "net: dsa: b53: restore the NSP IMP port
> 2 Gbps mode", fixes this. It sets speed = 2000 for BCM5301x/BCM58XX CPU
> ports in b53_phylink_mac_link_up(), so both ends match once the whole
> series is applied.
> 
> The two patches have different Fixes: tags, though (f6a95a24957a and
> b6a8a5477fe9). If a stable backport or a bisect picks up only this
> patch, would the MAC run at 2500 while the switch IMP port runs at
> 1000? Could the b53 commit be named here as a dependency so both are
> backported together?
> 
>>   	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: Medium]
> Can phy_print_status() still be reached on these interfaces?
> 

No it does not reach on those interfaces.

> bgmac_open()->bgmac_chip_reset()->bgmac_miiconfig() sets the cached
> speed and duplex before phy_start():
> 
> drivers/net/ethernet/broadcom/bgmac.c:bgmac_miiconfig() {
> 	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
> 		...
> 		bgmac->mac_speed = SPEED_2500;
> 		bgmac->mac_duplex = DUPLEX_FULL;
> 		bgmac_mac_speed(bgmac);
>      ...
> }
> 
> When the fixed PHY reports link up, bgmac_adjust_link() now computes
> speed == SPEED_2500, which equals mac_speed, and the duplex also equals
> mac_duplex. So update stays false and the link-up message is never
> printed. Before this patch, the change from 2500 to 1000 always set
> update.
> 
> Also, bgmac_ethtool_ops uses phy_ethtool_get_link_ksettings, which
> reports phydev->speed. That is still SPEED_1000, either from the fixed
> PHY in bgmac_phy_connect_direct() or from the DT fixed-link:
> 
> 	struct fixed_phy_status fphy_status = {
> 		.link = 1,
> 		.speed = SPEED_1000,
> 		.duplex = DUPLEX_FULL,
> 	};
> 
> The UniMAC, however, is now programmed for 2500.
> 
> Will ethtool and netlink report 1000 Mb/s for a link that is actually
> running at 2.5 Gbps?

Yes, that will happen, teaching phylink, phylib and ethtool about 
Broadcom's proprietary 2Gbits/sec is beyond the scope of this patch.
-- 
Florian