| Message ID | 1440323220-20438-5-git-send-email-andrew@lunn.ch |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
> > + 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
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>
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 >
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.
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(+)