[net-next,4/9] net: dsa: Allow configuration of CPU & DSA port speeds/duplex
diff mbox

Message ID 1440323220-20438-5-git-send-email-andrew@lunn.ch
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Andrew Lunn Aug. 23, 2015, 9:46 a.m. UTC
By default, DSA and CPU ports are configured to the maximum speed the
switch supports. However there can be use cases where the peer devices
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>
---
 net/dsa/dsa.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Florian Fainelli Aug. 23, 2015, 6:38 p.m. UTC | #1
Le 08/23/15 02:46, Andrew Lunn a écrit :
> By default, DSA and CPU ports are configured to the maximum speed the
> switch supports. However there can be use cases where the peer devices
> 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>
> ---
>  net/dsa/dsa.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 053eb2b8e682..afff17341b73 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -176,6 +176,35 @@ __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 (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, 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);
> +			genphy_config_init(phydev);
> +			genphy_read_status(phydev);
> +			if (ds->drv->adjust_link)
> +				ds->drv->adjust_link(ds, port, phydev);

This kind of hack here because what you really need is just the link
parameters, but you cannot obtain such information without first
configuring the PHY up to a certain point in genphy_config_init(), and
then have genphy_read_status() copy these values in your phydev structure.

Maybe we should really consider something like this after all:

https://lkml.org/lkml/2015/8/5/490

Or maybe, we should really introduce this "cpu" network device after all
with a dropping xmit function, such that we get ethtool counters to work
on it, and we can also attach it to a PHY device to configure link
parameters?
Andrew Lunn Aug. 23, 2015, 9:24 p.m. UTC | #2
> > +		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);
> > +			genphy_config_init(phydev);
> > +			genphy_read_status(phydev);
> > +			if (ds->drv->adjust_link)
> > +				ds->drv->adjust_link(ds, port, phydev);
> 
> This kind of hack here because what you really need is just the link
> parameters, but you cannot obtain such information without first
> configuring the PHY up to a certain point in genphy_config_init(), and
> then have genphy_read_status() copy these values in your phydev structure.
> 
> Maybe we should really consider something like this after all:
> 
> https://lkml.org/lkml/2015/8/5/490

Hi Florian

This half solves the problem. The nice thing about using the
fixed_link, is that i can just call the adjust_link function with it.
The fixed_phy_status cannot be passed directly to adjust_link. Some
code refactoring or duplication would be needed.
 
> Or maybe, we should really introduce this "cpu" network device after all
> with a dropping xmit function, such that we get ethtool counters to work
> on it, and we can also attach it to a PHY device to configure link
> parameters?

I keep humming and harring about this. I don't really like the idea of
having an interface which you cannot send/receive packets. Yet it
solves a number of problems like this, and gives you access to
statistics and registers in the usual way. If we do it for the CPU
port, we should also do it for the DSA ports. And we probably want the
call for up to return -ENOSUP, just to make it clear it cannot be used
for anything.

      Andrew
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli Aug. 24, 2015, 5:41 p.m. UTC | #3
On 23/08/15 14:24, Andrew Lunn wrote:
>>> +		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);
>>> +			genphy_config_init(phydev);
>>> +			genphy_read_status(phydev);
>>> +			if (ds->drv->adjust_link)
>>> +				ds->drv->adjust_link(ds, port, phydev);
>>
>> This kind of hack here because what you really need is just the link
>> parameters, but you cannot obtain such information without first
>> configuring the PHY up to a certain point in genphy_config_init(), and
>> then have genphy_read_status() copy these values in your phydev structure.
>>
>> Maybe we should really consider something like this after all:
>>
>> https://lkml.org/lkml/2015/8/5/490
> 
> Hi Florian
> 
> This half solves the problem. The nice thing about using the
> fixed_link, is that i can just call the adjust_link function with it.
> The fixed_phy_status cannot be passed directly to adjust_link. Some
> code refactoring or duplication would be needed.

Right, and using an adjust_link callback seems a little cleaner anyway
since you get an abstracted PHY device to work with.

>  
>> Or maybe, we should really introduce this "cpu" network device after all
>> with a dropping xmit function, such that we get ethtool counters to work
>> on it, and we can also attach it to a PHY device to configure link
>> parameters?
> 
> I keep humming and harring about this. I don't really like the idea of
> having an interface which you cannot send/receive packets. Yet it
> solves a number of problems like this, and gives you access to
> statistics and registers in the usual way.

Right that would be my primary motivation and use case as well.

> If we do it for the CPU
> port, we should also do it for the DSA ports. And we probably want the
> call for up to return -ENOSUP, just to make it clear it cannot be used
> for anything.

We should definitively start a separate thread for this, as there might
be real uses cases that are not yet covered that would need a network
device.

Let's go ahead with your patch for now:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli Aug. 26, 2015, 1:45 a.m. UTC | #4
Le 08/23/15 14:24, Andrew Lunn a écrit :
>>> +		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);
>>> +			genphy_config_init(phydev);
>>> +			genphy_read_status(phydev);
>>> +			if (ds->drv->adjust_link)
>>> +				ds->drv->adjust_link(ds, port, phydev);
>>
>> This kind of hack here because what you really need is just the link
>> parameters, but you cannot obtain such information without first
>> configuring the PHY up to a certain point in genphy_config_init(), and
>> then have genphy_read_status() copy these values in your phydev structure.
>>
>> Maybe we should really consider something like this after all:
>>
>> https://lkml.org/lkml/2015/8/5/490
> 
> Hi Florian
> 
> This half solves the problem. The nice thing about using the
> fixed_link, is that i can just call the adjust_link function with it.
> The fixed_phy_status cannot be passed directly to adjust_link. Some
> code refactoring or duplication would be needed.

BTW, this is really the reason why I was trying to push for having MDIO
connected switches as PHY devices, because then you get the PHY library
to calculate the advertised/supported intersection for you, and you
could even imagine re-negotiating the CPU port link with the Ethernet MAC.

Maybe I should repost these patches some day once I can get that working
with multiple switches in a tree ;)

>  
>> Or maybe, we should really introduce this "cpu" network device after all
>> with a dropping xmit function, such that we get ethtool counters to work
>> on it, and we can also attach it to a PHY device to configure link
>> parameters?
> 
> I keep humming and harring about this. I don't really like the idea of
> having an interface which you cannot send/receive packets. Yet it
> solves a number of problems like this, and gives you access to
> statistics and registers in the usual way. If we do it for the CPU
> port, we should also do it for the DSA ports. And we probably want the
> call for up to return -ENOSUP, just to make it clear it cannot be used
> for anything.
> 
>       Andrew
>

Patch
diff mbox

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 053eb2b8e682..afff17341b73 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -176,6 +176,35 @@  __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 (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, 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);
+			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;
@@ -297,6 +326,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.