Re: [PATCH v1 net-next 1/4] net: dsa: microchip: prepare PHY for proper advertisement

[Andrew Lunn <andrew@lunn.ch>]

> +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> +			      struct phy_device *phy)
> +{
> +	/* ETHTOOL_LINK_MODE_Pause_BIT and ETHTOOL_LINK_MODE_Asym_Pause_BIT
> +	 * can be removed to disable flow control when rate limiting is used.
> +	 */
> +	if (port < dev->phy_port_cnt) {
> +		/* The MAC actually cannot run in 1000 half-duplex mode. */
> +		phy_remove_link_mode(phy,
> +				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> +	}

Hi Tristram

The comment about pause does not seem to match the code.

> +}
> +
>  static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  {
>  	u8 data8;
> @@ -1151,6 +1164,7 @@ static int ksz9477_setup(struct dsa_switch *ds)
>  	.setup			= ksz9477_setup,
>  	.phy_read		= ksz9477_phy_read16,
>  	.phy_write		= ksz9477_phy_write16,
> +	.adjust_link		= ksz_adjust_link,
>  	.port_enable		= ksz_enable_port,
>  	.port_disable		= ksz_disable_port,
>  	.get_strings		= ksz9477_get_strings,
> @@ -1298,6 +1312,7 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
>  	.get_port_addr = ksz9477_get_port_addr,
>  	.cfg_port_member = ksz9477_cfg_port_member,
>  	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
> +	.phy_setup = ksz9477_phy_setup,
>  	.port_setup = ksz9477_port_setup,
>  	.shutdown = ksz9477_reset_switch,
>  	.detect = ksz9477_switch_detect,
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 8a5111f..a57bda7 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip switch driver main logic
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2019 Microchip Technology Inc.
>   */
>  
>  #include <linux/delay.h>
> @@ -61,6 +61,22 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
>  }
>  EXPORT_SYMBOL_GPL(ksz_phy_write16);
>  
> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (phydev->link) {
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	} else if (p->phydev.link) {
> +		p->link_just_down = 1;
> +		dev->live_ports &= ~(1 << port);
> +	}

It is not very easy to understand what on_ports, live_ports and
link_just_down are all about. Could you simplify this, and make the
code symmetric? ksz_enable_port does not touch any of these, but
ksz_disable_port does?

grep live_ports *
ksz9477.c:		dev->live_ports = dev->host_mask;
ksz9477.c:				  dev->live_ports |= (1 << port);
ksz_common.c:				  dev->live_ports &= ~(1 << port);
ksz_priv.h:				  u16 live_ports;

So live_ports is not actually used for anything.

> +	p->phydev = *phydev;

This last statement seems like it belongs in phy_setup, which gets
called as part of port_enable.