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

[Andrew Lunn]

> 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.

> +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.

> +	} 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?

You probably want to look at phylib, it figure out the correct way to
to implement ksetting set/get.


    Andrew

---
pw-bot: cr