Re: [PATCH net-next 2/5] net: rnpgbe: Report link settings through ethtool

[Yibo Dong]

On Sat, Nov 07, 2026 at 06:57:45PM +0100, Andrew Lunn wrote:
> > These devices support autonegotiation only on TP ports. Configure TP
> > ports with autonegotiation enabled and fibre and backplane ports with
> > fixed 1 Gb/s full duplex.
> >  int rnpgbe_setup_default_link(struct mucse_hw *hw)
> >  {
> > -	return mucse_mbx_set_link(hw, RNPGBE_DEFAULT_ADV, true, 0, 0,
> > -				  ETH_TP_MDI_AUTO);
> > +	u32 advertising = hw->link_config.supported & RNPGBE_DEFAULT_ADV;
> > +	bool autoneg = hw->link_config.status & MUCSE_LINK_TP;
> > +	u8 duplex = DUPLEX_UNKNOWN;
> > +	u32 speed = SPEED_UNKNOWN;
> > +	int err;
> > +
> > +	if (!autoneg) {
> > +		advertising &= RNPGBE_FW_ADV_1000_FULL;
> > +		speed = SPEED_1000;
> > +		duplex = DUPLEX_FULL;
> > +	}
> > +
> > +	err = mucse_mbx_set_link(hw, advertising, autoneg,
> > +				 autoneg ? 0 : speed, autoneg ? 0 : duplex,
> > +				 ETH_TP_MDI_AUTO);
> 
> This looks odd. If autoneg is off, you expect user space to indicate
> how the MAC should be configured. 10, 100, 1G, full or half.
> 
This function sets the initial configuration during probe. It does not
handle an ethtool configuration request.
For the supported fibre and backplane configurations, the link operates
at fixed 1 Gb/s full duplex without autonegotiation. TP ports default to
autonegotiation, and the subsequent patch adds support for user-selected
forced 10/100 Mb/s speed and duplex settings.
The port type is fixed by the device firmware. TP, fibre and backplane
are mutually exclusive configurations, and the driver cannot switch a
port between them.
> > +static int rnpgbe_get_link_ksettings(struct net_device *netdev,
> > +				     struct ethtool_link_ksettings *cmd)
> > +{
> > +	struct mucse *mucse = netdev_priv(netdev);
> > +	struct mucse_hw *hw = &mucse->hw;
> > +	u32 supported, advertising;
> > +	unsigned long flags;
> > +	u16 status;
> > +	int mode;
> > +
> > +	status = hw->link_config.status;
> > +	supported = hw->link_config.supported;
> > +	advertising = hw->link_config.advertising;
> > +	ethtool_link_ksettings_zero_link_mode(cmd, supported);
> > +	ethtool_link_ksettings_zero_link_mode(cmd, advertising);
> > +	ethtool_link_ksettings_zero_link_mode(cmd, lp_advertising);
> 
> Do you have access to the LP advertisement value? It is useful for debugging.
> 
Firmware manages PHY configuration, autonegotiation and reading the link
partner advertisement. However, the mailbox response currently used by
this driver only exposes the local capabilities and advertisement.
> > +	} else {
> > +		if (status & MUCSE_LINK_BACKPLANE) {
> > +			mode = ETHTOOL_LINK_MODE_1000baseKX_Full_BIT;
> > +			ethtool_link_ksettings_add_link_mode(cmd, supported,
> > +							     Backplane);
> > +			ethtool_link_ksettings_add_link_mode(cmd, advertising,
> > +							     Backplane);
> 
> Does it actually advertise Backplane?
> 
> 
> > +			cmd->base.port = PORT_NONE;
> > +		} else {
> > +			mode = ETHTOOL_LINK_MODE_1000baseX_Full_BIT;
> > +			ethtool_link_ksettings_add_link_mode(cmd, supported,
> > +							     FIBRE);
> > +			ethtool_link_ksettings_add_link_mode(cmd, advertising,
> > +							     FIBRE);
> 
> Again, does it advertise fibre? What happens if the LP wants to do TP?
> 
These bits were added based on the fixed port type, rather than an
advertisement reported by firmware. I agree that this does not establish
that they are actually advertised.
A fibre port cannot switch to TP based on the link partner's capabilities.
The firmware-selected port type remains fixed.
Maybe I should revise the reporting for the fixed non-TP configurations?
retain the port type and supported link mode in supported/base.port, Andrew
leave advertising empty since these configurations do not use autonegotiation.
> You probably want to look at phylib, it figure out the correct way to
> to implement ksetting set/get.
> 
The current design leaves PHY management with firmware, with the driver
using mailbox commands to configure and query the link.
I will use phylib as a reference for validating user settings
and reporting link capabilities and configuration, while keeping PHY
management in firmware.
I will correct the advertisement reporting for fixed non-TP ports.
> 
>     Andrew
> 
> ---
Thanks for your feedback.

> pw-bot: cr
>