[PATCH 1/3] net: phy: Allow PHY devices to identify themselves as Ethernet switches, etc.

[PATCH 1/3] net: phy: Allow PHY devices to identify themselves as Ethernet switches, etc.

[Andrew Lunn]

From: Florian Fainelli <f.fainelli@gmail.com>

Some Ethernet MAC drivers using the PHY library require the hardcoding
of link parameters when interfaced to a switch device, SFP module,
switch to switch port, etc. This has typically lead to various ad-hoc
implementations looking like this:

- using a "fixed PHY" emulated device, which will provide link
  indication towards the Ethernet MAC driver and hardware

- pretend there is no PHY and hardcode link parameters, ala mv643x_eth

Based on that, it is desireable to have the PHY drivers advertise the
correct link parameters, just like regular Ethernet PHYs towards their
CPU Ethernet MAC drivers, however, Ethernet MAC drivers should be able
to tell whether this link should be monitored or not. In the context
of an Ethernet switch, SFP module, switch to switch link, we do not
need to monitor this link since it should be always up.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/linux/phy.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index a26c3f84b8dd..5c3b87c0773c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -330,6 +330,7 @@ struct phy_c45_device_ids {
  * c45_ids: 802.3-c45 Device Identifers if is_c45.
  * is_c45:  Set to true if this phy uses clause 45 addressing.
  * is_internal: Set to true if this phy is internal to a MAC.
+ * is_pseudo_fixed_link: Set to true if this phy is an Ethernet switch, etc.
  * has_fixups: Set to true if this phy has fixups/quirks.
  * suspended: Set to true if this phy has been suspended successfully.
  * state: state of the PHY for management purposes
@@ -368,6 +369,7 @@ struct phy_device {
 	struct phy_c45_device_ids c45_ids;
 	bool is_c45;
 	bool is_internal;
+	bool is_pseudo_fixed_link;
 	bool has_fixups;
 	bool suspended;
 
@@ -686,6 +688,16 @@ static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
 {
 	return phydev->interface >= PHY_INTERFACE_MODE_RGMII &&
 		phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
+};
+
+/*
+ * phy_is_pseudo_fixed_link - Convenience function for testing if this
+ * PHY is the CPU port facing side of an Ethernet switch, or similar.
+ * @phydev: the phy_device struct
+ */
+static inline bool phy_is_pseudo_fixed_link(struct phy_device *phydev)
+{
+	return phydev->is_pseudo_fixed_link;
 }
 
 /**
-- 
2.1.4

[PATCH 2/3] dsa: mv88e6xxx: Allow speed/duplex of port to be configured

[Andrew Lunn]

The current code sets user ports to perform auto negotiation using the
phy. CPU and DSA ports are configured to full duplex and maximum speed
the switch supports.

There are however use cases where the CPU has a slower port, and when
user ports have SFP modules with fixed speed. In these cases, allow
port settings to be read from a fixed_phy devices.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6123_61_65.c |  1 +
 drivers/net/dsa/mv88e6131.c       |  1 +
 drivers/net/dsa/mv88e6171.c       |  1 +
 drivers/net/dsa/mv88e6352.c       |  1 +
 drivers/net/dsa/mv88e6xxx.c       | 56 +++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h       |  2 ++
 net/dsa/slave.c                   |  4 ++-
 7 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index 71a29a7ce538..3de2a6d73fdc 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -129,6 +129,7 @@ struct dsa_switch_driver mv88e6123_61_65_switch_driver = {
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
+	.adjust_link		= mv88e6xxx_adjust_link,
 #ifdef CONFIG_NET_DSA_HWMON
 	.get_temp		= mv88e6xxx_get_temp,
 #endif
diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index 32f4a08e9bc9..3e8386529965 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -182,6 +182,7 @@ struct dsa_switch_driver mv88e6131_switch_driver = {
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
+	.adjust_link		= mv88e6xxx_adjust_link,
 };
 
 MODULE_ALIAS("platform:mv88e6085");
diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index 1c7808495a9d..8803e20ebc52 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -108,6 +108,7 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
+	.adjust_link		= mv88e6xxx_adjust_link,
 #ifdef CONFIG_NET_DSA_HWMON
 	.get_temp               = mv88e6xxx_get_temp,
 #endif
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 632815c10a40..7a2deddbe270 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -374,6 +374,7 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
+	.adjust_link		= mv88e6xxx_adjust_link,
 	.set_eee		= mv88e6xxx_set_eee,
 	.get_eee		= mv88e6xxx_get_eee,
 #ifdef CONFIG_NET_DSA_HWMON
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 7fba330ce702..3defccb59d42 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -10,6 +10,7 @@
 
 #include <linux/delay.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/if_bridge.h>
 #include <linux/jiffies.h>
 #include <linux/list.h>
@@ -543,6 +544,61 @@ static bool mv88e6xxx_6352_family(struct dsa_switch *ds)
 	return false;
 }
 
+/* We expect the switch to perform auto negotiation if there is a real
+ * phy. However, in the case of a fixed link phy, we force the port
+ * settings from the fixed link settings.
+ */
+void mv88e6xxx_adjust_link(struct dsa_switch *ds, int port,
+			   struct phy_device *phydev)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	u32 ret, reg;
+
+	if (!phy_is_pseudo_fixed_link(phydev))
+		return;
+
+	mutex_lock(&ps->smi_mutex);
+
+	ret = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_PCS_CTRL);
+	if (ret < 0)
+		goto out;
+
+	reg = ret & ~(PORT_PCS_CTRL_LINK_UP |
+		      PORT_PCS_CTRL_FORCE_LINK |
+		      PORT_PCS_CTRL_DUPLEX_FULL |
+		      PORT_PCS_CTRL_FORCE_DUPLEX |
+		      PORT_PCS_CTRL_UNFORCED);
+
+	reg |= PORT_PCS_CTRL_FORCE_LINK;
+	if (phydev->link)
+			reg |= PORT_PCS_CTRL_LINK_UP;
+
+	if (mv88e6xxx_6065_family(ds) && phydev->speed > SPEED_100)
+		goto out;
+
+	switch (phydev->speed) {
+	case SPEED_1000:
+		reg |= PORT_PCS_CTRL_1000;
+		break;
+	case SPEED_100:
+		reg |= PORT_PCS_CTRL_100;
+		break;
+	case SPEED_10:
+		reg |= PORT_PCS_CTRL_10;
+	default:
+		goto out;
+	}
+
+	reg |= PORT_PCS_CTRL_FORCE_DUPLEX;
+	if (phydev->duplex == DUPLEX_FULL)
+		reg |= PORT_PCS_CTRL_DUPLEX_FULL;
+
+	_mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_PCS_CTRL, reg);
+
+out:
+	mutex_unlock(&ps->smi_mutex);
+}
+
 /* Must be called with SMI mutex held */
 static int _mv88e6xxx_stats_wait(struct dsa_switch *ds)
 {
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index e10ccdb4ffbc..5273ecb700ed 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -373,6 +373,8 @@ void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 				 uint64_t *data);
 int mv88e6xxx_get_sset_count(struct dsa_switch *ds);
 int mv88e6xxx_get_sset_count_basic(struct dsa_switch *ds);
+void mv88e6xxx_adjust_link(struct dsa_switch *ds, int port,
+			   struct phy_device *phydev);
 int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port);
 void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
 			struct ethtool_regs *regs, void *_p);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 04ffad311704..c32e4050b9bd 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -816,8 +816,10 @@ static int dsa_slave_phy_setup(struct dsa_slave_priv *p,
 		}
 	}
 
-	if (p->phy && phy_is_fixed)
+	if (p->phy && phy_is_fixed) {
 		fixed_phy_set_link_update(p->phy, dsa_slave_fixed_link_update);
+		p->phy->is_pseudo_fixed_link = true;
+	}
 
 	/* We could not connect to a designated PHY, so use the switch internal
 	 * MDIO bus instead
-- 
2.1.4

[PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Andrew Lunn]

By default, DSA and CPU ports are configured to the maximum speed the
switch supports. However there can be use cases where the peer device
port is slower. Allow a fixed-link property to be used with the DSA
and CPU port in the device tree, and use this information to configure
the port.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h |  1 +
 net/dsa/dsa.c     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63ba8f73..24572f99224c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -160,6 +160,7 @@ struct dsa_switch {
 	 * Slave mii_bus and devices for the individual ports.
 	 */
 	u32			dsa_port_mask;
+	u32			cpu_port_mask;
 	u32			phys_port_mask;
 	u32			phys_mii_mask;
 	struct mii_bus		*slave_mii_bus;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 392e29a0227d..f9c8f4e7ebce 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -176,6 +176,36 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
 #endif /* CONFIG_NET_DSA_HWMON */
 
 /* basic switch operations **************************************************/
+static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
+{
+	struct dsa_chip_data *cd = ds->pd;
+	struct device_node *port_dn;
+	struct phy_device *phydev;
+	int ret, port;
+
+	for (port = 0; port < DSA_MAX_PORTS; port++) {
+		if (!((ds->cpu_port_mask | ds->dsa_port_mask) & (1 << port)))
+			continue;
+
+		port_dn = cd->port_dn[port];
+		if (of_phy_is_fixed_link(port_dn)) {
+			ret = of_phy_register_fixed_link(port_dn);
+			if (ret) {
+				netdev_err(master,
+					   "failed to register fixed PHY\n");
+				return ret;
+			}
+			phydev = of_phy_find_device(port_dn);
+			phydev->is_pseudo_fixed_link = true;
+			genphy_config_init(phydev);
+			genphy_read_status(phydev);
+			if (ds->drv->adjust_link)
+				ds->drv->adjust_link(ds, port, phydev);
+		}
+	}
+	return 0;
+}
+
 static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 {
 	struct dsa_switch_driver *drv = ds->drv;
@@ -204,6 +234,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 			}
 			dst->cpu_switch = index;
 			dst->cpu_port = i;
+			ds->cpu_port_mask |= 1 << i;
 		} else if (!strcmp(name, "dsa")) {
 			ds->dsa_port_mask |= 1 << i;
 		} else {
@@ -297,6 +328,14 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 		}
 	}
 
+	/* Perform configuration of the CPU and DSA ports */
+	ret = dsa_cpu_dsa_setup(ds, dst->master_netdev);
+	if (ret < 0) {
+		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
+			   index);
+		ret = 0;
+	}
+
 #ifdef CONFIG_NET_DSA_HWMON
 	/* If the switch provides a temperature sensor,
 	 * register with hardware monitoring subsystem.
-- 
2.1.4

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Guenter Roeck]

Hi Florian,

On 06/12/2015 10:18 AM, Andrew Lunn wrote:
> By default, DSA and CPU ports are configured to the maximum speed the
> switch supports. However there can be use cases where the peer device
> port is slower. Allow a fixed-link property to be used with the DSA
> and CPU port in the device tree, and use this information to configure
> the port.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   include/net/dsa.h |  1 +
>   net/dsa/dsa.c     | 39 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63ba8f73..24572f99224c 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -160,6 +160,7 @@ struct dsa_switch {
>   	 * Slave mii_bus and devices for the individual ports.
>   	 */
>   	u32			dsa_port_mask;
> +	u32			cpu_port_mask;
>   	u32			phys_port_mask;
>   	u32			phys_mii_mask;
>   	struct mii_bus		*slave_mii_bus;
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 392e29a0227d..f9c8f4e7ebce 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -176,6 +176,36 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
>   #endif /* CONFIG_NET_DSA_HWMON */
>
>   /* basic switch operations **************************************************/
> +static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
> +{
> +	struct dsa_chip_data *cd = ds->pd;
> +	struct device_node *port_dn;
> +	struct phy_device *phydev;
> +	int ret, port;
> +
> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
> +		if (!((ds->cpu_port_mask | ds->dsa_port_mask) & (1 << port)))
> +			continue;
> +
How does cpu_port_mask interact / interfer / coexist with dst->cpu_port
and dsa_is_cpu_port() ?

Elsewhere we have
	if (dsa_is_cpu_port(ds, p) || ds->dsa_port_mask & (1 << p))
so I don't entirely see why we need to add cpu_port_mask at this time.
Shouldn't that be a separate patch, maybe with a new macro / function
to check if the port is a cpu port or an external switch port ?

Thanks,
Guenter

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Florian Fainelli]

On 12/06/15 10:18, Andrew Lunn wrote:
> By default, DSA and CPU ports are configured to the maximum speed the
> switch supports. However there can be use cases where the peer device
> port is slower. Allow a fixed-link property to be used with the DSA
> and CPU port in the device tree, and use this information to configure
> the port.

Humm, I suppose this means that we might end-up with two fixed PHY
devices, one for the Ethernet MAC, and another one for the switch? That
might duplicate the same information, though I cannot think of a better
solution than using phandles to resolve that.

> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/net/dsa.h |  1 +
>  net/dsa/dsa.c     | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63ba8f73..24572f99224c 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -160,6 +160,7 @@ struct dsa_switch {
>  	 * Slave mii_bus and devices for the individual ports.
>  	 */
>  	u32			dsa_port_mask;
> +	u32			cpu_port_mask;
>  	u32			phys_port_mask;
>  	u32			phys_mii_mask;
>  	struct mii_bus		*slave_mii_bus;
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 392e29a0227d..f9c8f4e7ebce 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -176,6 +176,36 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
>  #endif /* CONFIG_NET_DSA_HWMON */
>  
>  /* basic switch operations **************************************************/
> +static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
> +{
> +	struct dsa_chip_data *cd = ds->pd;
> +	struct device_node *port_dn;
> +	struct phy_device *phydev;
> +	int ret, port;
> +
> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
> +		if (!((ds->cpu_port_mask | ds->dsa_port_mask) & (1 << port)))
> +			continue;
> +
> +		port_dn = cd->port_dn[port];
> +		if (of_phy_is_fixed_link(port_dn)) {
> +			ret = of_phy_register_fixed_link(port_dn);
> +			if (ret) {
> +				netdev_err(master,
> +					   "failed to register fixed PHY\n");
> +				return ret;
> +			}
> +			phydev = of_phy_find_device(port_dn);
> +			phydev->is_pseudo_fixed_link = true;
> +			genphy_config_init(phydev);
> +			genphy_read_status(phydev);

I was curious as to why you were doing this at first, but I guess this
is because the PHY state machine is not started for this fixed PHY that
you just created, right?

> +			if (ds->drv->adjust_link)
> +				ds->drv->adjust_link(ds, port, phydev);
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  {
>  	struct dsa_switch_driver *drv = ds->drv;
> @@ -204,6 +234,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  			}
>  			dst->cpu_switch = index;
>  			dst->cpu_port = i;
> +			ds->cpu_port_mask |= 1 << i;

Same question as Guenter here, I assume this is because you plan on
having multiple CPU ports connected to the switch and this makes it
easier to deal with, is that right?

>  		} else if (!strcmp(name, "dsa")) {
>  			ds->dsa_port_mask |= 1 << i;
>  		} else {
> @@ -297,6 +328,14 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  		}
>  	}
>  
> +	/* Perform configuration of the CPU and DSA ports */
> +	ret = dsa_cpu_dsa_setup(ds, dst->master_netdev);
> +	if (ret < 0) {
> +		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
> +			   index);
> +		ret = 0;
> +	}
> +
>  #ifdef CONFIG_NET_DSA_HWMON
>  	/* If the switch provides a temperature sensor,
>  	 * register with hardware monitoring subsystem.
> 


-- 
Florian

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Guenter Roeck]

On 06/12/2015 11:14 AM, Florian Fainelli wrote:
> On 12/06/15 10:18, Andrew Lunn wrote:
>> By default, DSA and CPU ports are configured to the maximum speed the
>> switch supports. However there can be use cases where the peer device
>> port is slower. Allow a fixed-link property to be used with the DSA
>> and CPU port in the device tree, and use this information to configure
>> the port.
>
> Humm, I suppose this means that we might end-up with two fixed PHY
> devices, one for the Ethernet MAC, and another one for the switch? That
> might duplicate the same information, though I cannot think of a better
> solution than using phandles to resolve that.
>
>>
>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> ---
>>   include/net/dsa.h |  1 +
>>   net/dsa/dsa.c     | 39 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index fbca63ba8f73..24572f99224c 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -160,6 +160,7 @@ struct dsa_switch {
>>   	 * Slave mii_bus and devices for the individual ports.
>>   	 */
>>   	u32			dsa_port_mask;
>> +	u32			cpu_port_mask;
>>   	u32			phys_port_mask;
>>   	u32			phys_mii_mask;
>>   	struct mii_bus		*slave_mii_bus;
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 392e29a0227d..f9c8f4e7ebce 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -176,6 +176,36 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
>>   #endif /* CONFIG_NET_DSA_HWMON */
>>
>>   /* basic switch operations **************************************************/
>> +static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
>> +{
>> +	struct dsa_chip_data *cd = ds->pd;
>> +	struct device_node *port_dn;
>> +	struct phy_device *phydev;
>> +	int ret, port;
>> +
>> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
>> +		if (!((ds->cpu_port_mask | ds->dsa_port_mask) & (1 << port)))
>> +			continue;
>> +
>> +		port_dn = cd->port_dn[port];
>> +		if (of_phy_is_fixed_link(port_dn)) {
>> +			ret = of_phy_register_fixed_link(port_dn);
>> +			if (ret) {
>> +				netdev_err(master,
>> +					   "failed to register fixed PHY\n");
>> +				return ret;
>> +			}
>> +			phydev = of_phy_find_device(port_dn);
>> +			phydev->is_pseudo_fixed_link = true;
>> +			genphy_config_init(phydev);
>> +			genphy_read_status(phydev);
>
> I was curious as to why you were doing this at first, but I guess this
> is because the PHY state machine is not started for this fixed PHY that
> you just created, right?
>
>> +			if (ds->drv->adjust_link)
>> +				ds->drv->adjust_link(ds, port, phydev);
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>>   static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>>   {
>>   	struct dsa_switch_driver *drv = ds->drv;
>> @@ -204,6 +234,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>>   			}
>>   			dst->cpu_switch = index;
>>   			dst->cpu_port = i;
>> +			ds->cpu_port_mask |= 1 << i;
>
> Same question as Guenter here, I assume this is because you plan on
> having multiple CPU ports connected to the switch and this makes it
> easier to deal with, is that right?
>

If so, should that be done in a separate patch set ?

Guenter

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Florian Fainelli]

On 12/06/15 11:29, Guenter Roeck wrote:
[snip]

static int dsa_switch_setup_one(struct dsa_switch *ds, struct
>>> device *parent)
>>>   {
>>>       struct dsa_switch_driver *drv = ds->drv;
>>> @@ -204,6 +234,7 @@ static int dsa_switch_setup_one(struct dsa_switch
>>> *ds, struct device *parent)
>>>               }
>>>               dst->cpu_switch = index;
>>>               dst->cpu_port = i;
>>> +            ds->cpu_port_mask |= 1 << i;
>>
>> Same question as Guenter here, I assume this is because you plan on
>> having multiple CPU ports connected to the switch and this makes it
>> easier to deal with, is that right?
>>
> 
> If so, should that be done in a separate patch set ?

I think it would be clearer that way, yes.
-- 
Florian

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Andrew Lunn]

On Fri, Jun 12, 2015 at 11:14:25AM -0700, Florian Fainelli wrote:
> On 12/06/15 10:18, Andrew Lunn wrote:
> > By default, DSA and CPU ports are configured to the maximum speed the
> > switch supports. However there can be use cases where the peer device
> > port is slower. Allow a fixed-link property to be used with the DSA
> > and CPU port in the device tree, and use this information to configure
> > the port.
> 
> Humm, I suppose this means that we might end-up with two fixed PHY
> devices, one for the Ethernet MAC, and another one for the switch?

Yes. This is exactly what i have for the board i'm working on. The
concept also applies for DSA ports, so you could have two switches and
two fixed phys for one inter-switch link.

> That might duplicate the same information, though I cannot think of
> a better solution than using phandles to resolve that.

This seems the simplest solution. It would be possible to create a
"dual port" fixed phy, meaning it exposes two phy_device structures,
one for each side. But that seems overkill.

> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  include/net/dsa.h |  1 +
> >  net/dsa/dsa.c     | 39 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index fbca63ba8f73..24572f99224c 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -160,6 +160,7 @@ struct dsa_switch {
> >  	 * Slave mii_bus and devices for the individual ports.
> >  	 */
> >  	u32			dsa_port_mask;
> > +	u32			cpu_port_mask;
> >  	u32			phys_port_mask;
> >  	u32			phys_mii_mask;
> >  	struct mii_bus		*slave_mii_bus;
> > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> > index 392e29a0227d..f9c8f4e7ebce 100644
> > --- a/net/dsa/dsa.c
> > +++ b/net/dsa/dsa.c
> > @@ -176,6 +176,36 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
> >  #endif /* CONFIG_NET_DSA_HWMON */
> >  
> >  /* basic switch operations **************************************************/
> > +static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
> > +{
> > +	struct dsa_chip_data *cd = ds->pd;
> > +	struct device_node *port_dn;
> > +	struct phy_device *phydev;
> > +	int ret, port;
> > +
> > +	for (port = 0; port < DSA_MAX_PORTS; port++) {
> > +		if (!((ds->cpu_port_mask | ds->dsa_port_mask) & (1 << port)))
> > +			continue;
> > +
> > +		port_dn = cd->port_dn[port];
> > +		if (of_phy_is_fixed_link(port_dn)) {
> > +			ret = of_phy_register_fixed_link(port_dn);
> > +			if (ret) {
> > +				netdev_err(master,
> > +					   "failed to register fixed PHY\n");
> > +				return ret;
> > +			}
> > +			phydev = of_phy_find_device(port_dn);
> > +			phydev->is_pseudo_fixed_link = true;
> > +			genphy_config_init(phydev);
> > +			genphy_read_status(phydev);
> 
> I was curious as to why you were doing this at first, but I guess this
> is because the PHY state machine is not started for this fixed PHY that
> you just created, right?

For the fixed phy to be of any use in adjust_link(), it needs to set
phydev->link, phydev->speed and phydev->duplex. That only happens when
genphy_read_status() is called. And you don't get sensible values
unless genphy_config_init() is called first. We don't have a netdev we
can attach this phydev to, so the core has no chance to do these
genphy_XXX calls.

> > +			if (ds->drv->adjust_link)
> > +				ds->drv->adjust_link(ds, port, phydev);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
> >  {
> >  	struct dsa_switch_driver *drv = ds->drv;
> > @@ -204,6 +234,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
> >  			}
> >  			dst->cpu_switch = index;
> >  			dst->cpu_port = i;
> > +			ds->cpu_port_mask |= 1 << i;
> 
> Same question as Guenter here, I assume this is because you plan on
> having multiple CPU ports connected to the switch and this makes it
> easier to deal with, is that right?

Yes, sort of. At the time i wrote this code, i already had multiple
CPU ports working. But the order i'm submitting the patches has been
reversed. This could be simplified for a single CPU port.

The multiple CPU ports is turning out to be messy, but not because of
the code. It works on my DIR665, but the second ethernet does not have
a MAC address, which is causing issues i need to track down. For
testing i've set one in device tree. And my WRT1900AC has something
funny going on with its second interface resulting in it never
sending/receiving packets, but works fine with OpenWRT swconfig
drivers. Until i have one platform in a state i can mainline, i'm
holding off with the multi-cpu patches. I do want to work on them next
though, but i guess i'm going to miss this merge window.

	Andrew

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Florian Fainelli]

On 12/06/15 10:18, Andrew Lunn wrote:
> By default, DSA and CPU ports are configured to the maximum speed the
> switch supports. However there can be use cases where the peer device
> port is slower. Allow a fixed-link property to be used with the DSA
> and CPU port in the device tree, and use this information to configure
> the port.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/net/dsa.h |  1 +
>  net/dsa/dsa.c     | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63ba8f73..24572f99224c 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -160,6 +160,7 @@ struct dsa_switch {
>  	 * Slave mii_bus and devices for the individual ports.
>  	 */
>  	u32			dsa_port_mask;
> +	u32			cpu_port_mask;
>  	u32			phys_port_mask;
>  	u32			phys_mii_mask;
>  	struct mii_bus		*slave_mii_bus;
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 392e29a0227d..f9c8f4e7ebce 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -176,6 +176,36 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
>  #endif /* CONFIG_NET_DSA_HWMON */
>  
>  /* basic switch operations **************************************************/
> +static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
> +{
> +	struct dsa_chip_data *cd = ds->pd;
> +	struct device_node *port_dn;
> +	struct phy_device *phydev;
> +	int ret, port;
> +
> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
> +		if (!((ds->cpu_port_mask | ds->dsa_port_mask) & (1 << port)))
> +			continue;
> +
> +		port_dn = cd->port_dn[port];
> +		if (of_phy_is_fixed_link(port_dn)) {
> +			ret = of_phy_register_fixed_link(port_dn);
> +			if (ret) {
> +				netdev_err(master,
> +					   "failed to register fixed PHY\n");
> +				return ret;
> +			}
> +			phydev = of_phy_find_device(port_dn);
> +			phydev->is_pseudo_fixed_link = true;

I suppose this could be automatically set by the Generic PHY driver once
we bind it to the fixed MDIO bus implementation instead of having driver
have to do this, what do you think?
-- 
Florian

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Vivien Didelot]

Hi Andrew, All,

On 12/06/15 10:18, Andrew Lunn wrote:
> By default, DSA and CPU ports are configured to the maximum speed the
> switch supports. However there can be use cases where the peer device
> port is slower. Allow a fixed-link property to be used with the DSA
> and CPU port in the device tree, and use this information to configure
> the port.

Would it be a good idea for DSA to expose the "cpu" port to userspace as well?
That way, it'd be possible to use ethtool to set the port speed and duplex
mode, or dump registers (this would have saved me quite some time in dev).

Also, in my RFC for 802.1Q support [1], I assume the CPU port to be a tagged
member of each VLAN. But someone may want to add a VLAN with swp3 and swp4
only, and another VLAN with swp0, swp1 and the CPU port. Am I correct? This is
currently not possible, but with an exposed "cpu" interface, the user could
explicitly add the CPU port to a VLAN.

Sorry if this is a bit off-topic.

[1] https://lkml.org/lkml/2015/6/1/752

Thanks,
-v

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Andrew Lunn]

On Wed, Jun 17, 2015 at 02:09:52PM -0400, Vivien Didelot wrote:
> Hi Andrew, All,
> 
> On 12/06/15 10:18, Andrew Lunn wrote:
> > By default, DSA and CPU ports are configured to the maximum speed the
> > switch supports. However there can be use cases where the peer device
> > port is slower. Allow a fixed-link property to be used with the DSA
> > and CPU port in the device tree, and use this information to configure
> > the port.
> 
> Would it be a good idea for DSA to expose the "cpu" port to userspace as well?
> That way, it'd be possible to use ethtool to set the port speed and duplex
> mode, or dump registers (this would have saved me quite some time in dev).

I have code which expose these via debugfs. So far, i have all
registers, stats, ATU, and the scratch registers. For the patches to
apply cleanly, they depend on these patches, so i've not posted them
yet.

I'm not strongly against having a CPU port, but i don't particularly
like having the CPU port as an interface. And when you get to cascaded
switches, the DSA ports are also interesting, so should we also have a
netdev for them? But they are equally useless for transferring frames
from the host as the CPU port. This is why i went for debugfs.

> 
> Also, in my RFC for 802.1Q support [1], I assume the CPU port to be a tagged
> member of each VLAN. But someone may want to add a VLAN with swp3 and swp4
> only, and another VLAN with swp0, swp1 and the CPU port. Am I correct?

The DSA concept is that switch ports are separate interfaces. So
adding a VLAN to two ports does to automatically bridge those ports
together. You need to add them to a bridge as well before VLAN tagged
frames are bridged between ports.

       Andrew

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Vivien Didelot]

Hi Andrew,

On Jun 17, 2015, at 9:11 PM, Andrew Lunn andrew@lunn.ch wrote:
> On Wed, Jun 17, 2015 at 02:09:52PM -0400, Vivien Didelot wrote:
>> Hi Andrew, All,
>> 
>> On 12/06/15 10:18, Andrew Lunn wrote:
>> > By default, DSA and CPU ports are configured to the maximum speed the
>> > switch supports. However there can be use cases where the peer device
>> > port is slower. Allow a fixed-link property to be used with the DSA
>> > and CPU port in the device tree, and use this information to configure
>> > the port.
>> 
>> Would it be a good idea for DSA to expose the "cpu" port to userspace as well?
>> That way, it'd be possible to use ethtool to set the port speed and duplex
>> mode, or dump registers (this would have saved me quite some time in dev).
> 
> I have code which expose these via debugfs. So far, i have all
> registers, stats, ATU, and the scratch registers. For the patches to
> apply cleanly, they depend on these patches, so i've not posted them
> yet.

Yes I do have debug too, but via sysfs (with eventually write access) for:
GLOBAL1, GLOBAL2, cpu port registers, SerDes registers, PVIDs, and VTU.
Not really standard though.

> I'm not strongly against having a CPU port, but i don't particularly
> like having the CPU port as an interface. And when you get to cascaded
> switches, the DSA ports are also interesting, so should we also have a
> netdev for them? But they are equally useless for transferring frames
> from the host as the CPU port. This is why i went for debugfs.
> 
>> Also, in my RFC for 802.1Q support [1], I assume the CPU port to be a tagged
>> member of each VLAN. But someone may want to add a VLAN with swp3 and swp4
>> only, and another VLAN with swp0, swp1 and the CPU port. Am I correct?
> 
> The DSA concept is that switch ports are separate interfaces. So
> adding a VLAN to two ports does to automatically bridge those ports
> together. You need to add them to a bridge as well before VLAN tagged
> frames are bridged between ports.

My point was to expose all configurable ports with the same standard interface
(netdev, like any other virtual switch port). But indeed, their uselessness for
data transfer can be a good reason not to do it.

Thanks,
-v

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Andrew Lunn]

> Yes I do have debug too, but via sysfs (with eventually write access) for:
> GLOBAL1, GLOBAL2, cpu port registers, SerDes registers, PVIDs, and VTU.
> Not really standard though.

We should really get an implementation into mainline. There is no
point us all implementing our own.

You say your code is not really standard. Do you think it would get
rejected if it was submitted? The rules for debugfs are much more
relaxed, so what i have should be acceptable.

	 Andrew

Re: [PATCH 3/3] net: dsa: Allow configuration of CPU & DSA port speeds/duplex

[Vivien Didelot]

Hi Andrew,

On Jun 19, 2015, at 11:05 AM, Andrew Lunn andrew@lunn.ch wrote:
>> Yes I do have debug too, but via sysfs (with eventually write access) for:
>> GLOBAL1, GLOBAL2, cpu port registers, SerDes registers, PVIDs, and VTU.
>> Not really standard though.
> 
> We should really get an implementation into mainline. There is no
> point us all implementing our own.

I couldn't agree more. It's important for development to have read/write
access to the switch registers, bypassing the userspace tools.

> You say your code is not really standard. Do you think it would get
> rejected if it was submitted? The rules for debugfs are much more
> relaxed, so what i have should be acceptable.

Yes my code will definitely be rejected. I have a net/dsa/debug.c file
that creates a debug directory under /sys/devices/platform/dsa.0/ with
global1, global2, cpu_port, serdes, pvid, and vtu sysfs attributes.

Read and write access to debugfs sounds better IMO.

Thanks,
-v