Message ID | 20180115193722.10241-2-sebastian.reichel@collabora.co.uk |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Series | GEHC Bx50 Switch Support | expand |
On 01/15/2018 11:37 AM, Sebastian Reichel wrote: > This adds support for enabling the internal PHY for a 'cpu' port. > It has been tested on GE B850v3, B650v3 and B450v3, which have a > built-in MV88E6240 switch connected to a PCIe based network card. > Without this patch the link does not come up and no traffic can be > routed through the switch. > > The PHY interface, that is being used on the above test systems is > part of the MV88E6240 and since mv88e6xxx driver resets the chip > during probe, it is definitely disabled without this patch. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > net/dsa/port.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/net/dsa/port.c b/net/dsa/port.c > index bb4be2679904..011ecd9b1be7 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -273,6 +273,55 @@ int dsa_port_vlan_del(struct dsa_port *dp, > return 0; > } > > +static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable) > +{ > + struct device_node *port_dn = dp->dn; > + struct device_node *phy_dn; > + struct dsa_switch *ds = dp->ds; > + struct phy_device *phydev; > + int port = dp->index; > + int err = 0; I don't have a better solution to offer yet, but this is really making the number of special cases within DSA much worse, thus impending the on-going conversion to PHYLINK... Having PHYs with no network devices is clearly a hack so maybe we should re-think this whole design paradigm in DSA for which we don't have net_device being created for CPU and DSA links, and maybe come up with an actual net_device and just not expose it to the default namespace. Anyway... > + > + phy_dn = of_parse_phandle(port_dn, "phy-handle", 0); > + if (!phy_dn) > + return 0; > + > + phydev = of_phy_find_device(phy_dn); > + if (!phydev) { > + err = -EPROBE_DEFER; > + goto err_put_of; > + } > + > + if (enable) { > + err = genphy_config_init(phydev); > + if (err < 0) > + goto err_put_dev; > + > + err = genphy_resume(phydev); > + if (err < 0) > + goto err_put_dev; > + > + err = genphy_read_status(phydev); > + if (err < 0) > + goto err_put_dev; > + } else { > + err = genphy_suspend(phydev); > + if (err < 0) > + goto err_put_dev; > + } > + > + if (ds->ops->adjust_link) > + ds->ops->adjust_link(ds, port, phydev); > + > + dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev)); > + > +err_put_dev: > + put_device(&phydev->mdio.dev); > +err_put_of: > + of_node_put(phy_dn); > + return err; > +} > + > int dsa_port_fixed_link_register_of(struct dsa_port *dp) > { > struct device_node *dn = dp->dn; > @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp) > ds->ops->adjust_link(ds, port, phydev); > > put_device(&phydev->mdio.dev); > + } else { > + err = dsa_port_setup_phy_of(dp, true); > + if (err) > + return err; > } > > return 0; > @@ -316,4 +369,6 @@ void dsa_port_fixed_link_unregister_of(struct dsa_port *dp) > > if (of_phy_is_fixed_link(dn)) > of_phy_deregister_fixed_link(dn); > + else > + dsa_port_setup_phy_of(dp, false); > } >
> int dsa_port_fixed_link_register_of(struct dsa_port *dp) > { > struct device_node *dn = dp->dn; > @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp) > ds->ops->adjust_link(ds, port, phydev); > > put_device(&phydev->mdio.dev); > + } else { > + err = dsa_port_setup_phy_of(dp, true); > + if (err) > + return err; Hi Sebastian First off, i tend to agree with Florian. I'm not sure how reliable this is. There is normally a state machine moving the PHY between different states. But in order to do that, i think you need a netdev. Have you tried multiple down/up of the other MAC/PHY? Does it always work? But, at the moment, we don't have much better. What i don't like is having this code inside dsa_port_fixed_link_register_of(). This has nothing to do with a fixed link. Please export functions from port.c and call them directly from dsa_port_setup() and dsa_port_teardown(). Andrew
Hi, On Mon, Jan 15, 2018 at 11:57:18PM +0100, Andrew Lunn wrote: > > int dsa_port_fixed_link_register_of(struct dsa_port *dp) > > { > > struct device_node *dn = dp->dn; > > @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp) > > ds->ops->adjust_link(ds, port, phydev); > > > > put_device(&phydev->mdio.dev); > > + } else { > > + err = dsa_port_setup_phy_of(dp, true); > > + if (err) > > + return err; > > Hi Sebastian > > First off, i tend to agree with Florian. I'm not sure how reliable > this is. There is normally a state machine moving the PHY between > different states. But in order to do that, i think you need a netdev. > Have you tried multiple down/up of the other MAC/PHY? Does it always > work? I tested multiple down/up transitions and everything works as expected. > But, at the moment, we don't have much better. > > What i don't like is having this code inside > dsa_port_fixed_link_register_of(). This has nothing to do with a fixed > link. Please export functions from port.c and call them directly from > dsa_port_setup() and dsa_port_teardown(). I just sent PATCHv4 implementing this change. -- Sebastian
diff --git a/net/dsa/port.c b/net/dsa/port.c index bb4be2679904..011ecd9b1be7 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -273,6 +273,55 @@ int dsa_port_vlan_del(struct dsa_port *dp, return 0; } +static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable) +{ + struct device_node *port_dn = dp->dn; + struct device_node *phy_dn; + struct dsa_switch *ds = dp->ds; + struct phy_device *phydev; + int port = dp->index; + int err = 0; + + phy_dn = of_parse_phandle(port_dn, "phy-handle", 0); + if (!phy_dn) + return 0; + + phydev = of_phy_find_device(phy_dn); + if (!phydev) { + err = -EPROBE_DEFER; + goto err_put_of; + } + + if (enable) { + err = genphy_config_init(phydev); + if (err < 0) + goto err_put_dev; + + err = genphy_resume(phydev); + if (err < 0) + goto err_put_dev; + + err = genphy_read_status(phydev); + if (err < 0) + goto err_put_dev; + } else { + err = genphy_suspend(phydev); + if (err < 0) + goto err_put_dev; + } + + if (ds->ops->adjust_link) + ds->ops->adjust_link(ds, port, phydev); + + dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev)); + +err_put_dev: + put_device(&phydev->mdio.dev); +err_put_of: + of_node_put(phy_dn); + return err; +} + int dsa_port_fixed_link_register_of(struct dsa_port *dp) { struct device_node *dn = dp->dn; @@ -305,6 +354,10 @@ int dsa_port_fixed_link_register_of(struct dsa_port *dp) ds->ops->adjust_link(ds, port, phydev); put_device(&phydev->mdio.dev); + } else { + err = dsa_port_setup_phy_of(dp, true); + if (err) + return err; } return 0; @@ -316,4 +369,6 @@ void dsa_port_fixed_link_unregister_of(struct dsa_port *dp) if (of_phy_is_fixed_link(dn)) of_phy_deregister_fixed_link(dn); + else + dsa_port_setup_phy_of(dp, false); }
This adds support for enabling the internal PHY for a 'cpu' port. It has been tested on GE B850v3, B650v3 and B450v3, which have a built-in MV88E6240 switch connected to a PCIe based network card. Without this patch the link does not come up and no traffic can be routed through the switch. The PHY interface, that is being used on the above test systems is part of the MV88E6240 and since mv88e6xxx driver resets the chip during probe, it is definitely disabled without this patch. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> --- net/dsa/port.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)