[v2] net: dsa: microchip: look for phy-mode in port nodes

Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
property should be specified on port nodes. However, the microchip
drivers read it from the switch node.

Let the driver use the per-port property and fall back to the old
location with a warning.

Fix in-tree users.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
 drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
 drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  3 ++-
 5 files changed, 45 insertions(+), 18 deletions(-)

Changes since v1:
 * Preserve the reverse christmas tree ordering of local variables.
   Reported by David Miller.

Reason for resending v1:
 * While Andrew Lunn agreed to the semantic change, he found the
   implementation unnecessarily complex. He suggested going without a
   per-port interface attribute, but that happened to not work out. The
   information of which port will become the cpu port is only realized
   in a later initialization step.

There were no further replies, so here goes a v2 with minimal changes.

Helmut

On 04/09/2020 10:14:42+0200, Helmut Grohne wrote:
> Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
> property should be specified on port nodes. However, the microchip
> drivers read it from the switch node.
> 
> Let the driver use the per-port property and fall back to the old
> location with a warning.
> 
> Fix in-tree users.
> 
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
> ---
>  arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
>  drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
>  drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
>  drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
>  drivers/net/dsa/microchip/ksz_common.h |  3 ++-
>  5 files changed, 45 insertions(+), 18 deletions(-)
> 
> Changes since v1:
>  * Preserve the reverse christmas tree ordering of local variables.
>    Reported by David Miller.
> 
> Reason for resending v1:
>  * While Andrew Lunn agreed to the semantic change, he found the
>    implementation unnecessarily complex. He suggested going without a
>    per-port interface attribute, but that happened to not work out. The
>    information of which port will become the cpu port is only realized
>    in a later initialization step.
> 
> There were no further replies, so here goes a v2 with minimal changes.
> 
> Helmut
> 
> diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
> index 8d19925fc09e..6783cf16ff81 100644
> --- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
> +++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
> @@ -116,7 +116,6 @@
>  		switch0: ksz8563@0 {
>  			compatible = "microchip,ksz8563";
>  			reg = <0>;
> -			phy-mode = "mii";
>  			reset-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_LOW>;
>  
>  			spi-max-frequency = <500000>;
> @@ -140,6 +139,7 @@
>  					reg = <2>;
>  					label = "cpu";
>  					ethernet = <&macb0>;
> +					phy-mode = "mii";
>  					fixed-link {
>  						speed = <100>;
>  						full-duplex;
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 8f1d15ea15d9..cae77eafd533 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -932,11 +932,18 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
>  
>  	if (cpu_port) {
> +		if (!p->interface && dev->compat_interface) {
> +			dev_warn(dev->dev,
> +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> +				 port);
> +			p->interface = dev->compat_interface;
> +		}
> +
>  		/* Configure MII interface for proper network communication. */
>  		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
>  		data8 &= ~PORT_INTERFACE_TYPE;
>  		data8 &= ~PORT_GMII_1GPS_MODE;
> -		switch (dev->interface) {
> +		switch (p->interface) {
>  		case PHY_INTERFACE_MODE_MII:
>  			p->phydev.speed = SPEED_100;
>  			break;
> @@ -952,11 +959,11 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		default:
>  			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
>  			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>  				data8 |= PORT_RGMII_ID_IN_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>  				data8 |= PORT_RGMII_ID_OUT_ENABLE;
>  			data8 |= PORT_GMII_1GPS_MODE;
>  			data8 |= PORT_INTERFACE_RGMII;
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 3cb22d149813..89e8934bc60b 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1208,7 +1208,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  
>  		/* configure MAC to 1G & RGMII mode */
>  		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
> -		switch (dev->interface) {
> +		switch (p->interface) {
>  		case PHY_INTERFACE_MODE_MII:
>  			ksz9477_set_xmii(dev, 0, &data8);
>  			ksz9477_set_gbit(dev, false, &data8);
> @@ -1229,11 +1229,11 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  			ksz9477_set_gbit(dev, true, &data8);
>  			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
>  			data8 &= ~PORT_RGMII_ID_EG_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>  				data8 |= PORT_RGMII_ID_IG_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>  				data8 |= PORT_RGMII_ID_EG_ENABLE;
>  			p->phydev.speed = SPEED_1000;
>  			break;
> @@ -1269,23 +1269,31 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  			dev->cpu_port = i;
>  			dev->host_mask = (1 << dev->cpu_port);
>  			dev->port_mask |= dev->host_mask;
> +			p = &dev->ports[i];
>  
>  			/* Read from XMII register to determine host port
>  			 * interface.  If set specifically in device tree
>  			 * note the difference to help debugging.
>  			 */
>  			interface = ksz9477_get_interface(dev, i);
> -			if (!dev->interface)
> -				dev->interface = interface;
> -			if (interface && interface != dev->interface)
> +			if (!p->interface) {
> +				if (dev->compat_interface) {
> +					dev_warn(dev->dev,
> +						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> +						 i);
> +					p->interface = dev->compat_interface;
> +				} else {
> +					p->interface = interface;
> +				}
> +			}
> +			if (interface && interface != p->interface)
>  				dev_info(dev->dev,
>  					 "use %s instead of %s\n",
> -					  phy_modes(dev->interface),
> +					  phy_modes(p->interface),
>  					  phy_modes(interface));
>  
>  			/* enable cpu port */
>  			ksz9477_port_setup(dev, i, true);
> -			p = &dev->ports[dev->cpu_port];
>  			p->vid_member = dev->port_mask;
>  			p->on = 1;
>  		}
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 8d53b12d40a8..8e755b50c9c1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -388,6 +388,8 @@ int ksz_switch_register(struct ksz_device *dev,
>  			const struct ksz_dev_ops *ops)
>  {
>  	phy_interface_t interface;
> +	struct device_node *port;
> +	unsigned int port_num;
>  	int ret;
>  
>  	if (dev->pdata)
> @@ -421,10 +423,19 @@ int ksz_switch_register(struct ksz_device *dev,
>  	/* Host port interface will be self detected, or specifically set in
>  	 * device tree.
>  	 */
> +	for (port_num = 0; port_num < dev->port_cnt; ++port_num)
> +		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
>  	if (dev->dev->of_node) {
>  		ret = of_get_phy_mode(dev->dev->of_node, &interface);
>  		if (ret == 0)
> -			dev->interface = interface;
> +			dev->compat_interface = interface;
> +		for_each_available_child_of_node(dev->dev->of_node, port) {
> +			if (of_property_read_u32(port, "reg", &port_num))
> +				continue;
> +			if (port_num >= dev->port_cnt)
> +				return -EINVAL;
> +			of_get_phy_mode(port, &dev->ports[port_num].interface);
> +		}
>  		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
>  							 "microchip,synclko-125");
>  	}
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 206838160f49..cf866e48ff66 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -39,6 +39,7 @@ struct ksz_port {
>  	u32 freeze:1;			/* MIB counter freeze is enabled */
>  
>  	struct ksz_port_mib mib;
> +	phy_interface_t interface;
>  };
>  
>  struct ksz_device {
> @@ -72,7 +73,7 @@ struct ksz_device {
>  	int mib_cnt;
>  	int mib_port_cnt;
>  	int last_port;			/* ports after that not used */
> -	phy_interface_t interface;
> +	phy_interface_t compat_interface;
>  	u32 regs_size;
>  	bool phy_errata_9477;
>  	bool synclko_125;
> -- 
> 2.20.1
>

> +			dev_warn(dev->dev,
> +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",

That message seems mangled.

> +			if (!p->interface) {
> +				if (dev->compat_interface) {
> +					dev_warn(dev->dev,
> +						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> +						 i);

Same warning again.

     Andrew

Hi Andrew,

On Fri, Sep 04, 2020 at 03:52:55PM +0200, Andrew Lunn wrote:
> > +			dev_warn(dev->dev,
> > +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",

This is inside ksz8795_port_setup.

> That message seems mangled.

I'm not sure that I understand what you are objecting to here.

> > +			if (!p->interface) {
> > +				if (dev->compat_interface) {
> > +					dev_warn(dev->dev,
> > +						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> > +						 i);

This is inside ksz9477_config_cpu_port.

> Same warning again.

I guess that you believe the warning should only be issued in one place.
The locations affect different chips driven by the same driver. I
considered moving them to a common function, but figured that it was not
worth tearing the code apart. In case a third chip would be supported by
the driver, it would not need the compatibility code. It would start out
using only the correct phy-mode property.

Does that address your concern?

Helmut

On Mon, Sep 07, 2020 at 08:15:33AM +0200, Helmut Grohne wrote:
> Hi Andrew,
> 
> On Fri, Sep 04, 2020 at 03:52:55PM +0200, Andrew Lunn wrote:
> > > +			dev_warn(dev->dev,
> > > +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> 
> This is inside ksz8795_port_setup.
> 
> > That message seems mangled.
> 
> I'm not sure that I understand what you are objecting to here.

Hi Helmut

The grammar seems wrong. 

"Using legacy switch \"phy-mode\" because \"phy-mode\" missing from port %d node. "
"Please update your device tree.\n"

	Andrew