| Message ID | E1o8fA7-0059aO-K8@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: always use phylink | expand |
Hello, On Tue, Jul 05, 2022 at 10:48:07AM +0100, Russell King (Oracle) wrote: > diff --git a/net/dsa/port.c b/net/dsa/port.c > index 35b4e1f8dc05..34487e62eb03 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp) > { > struct dsa_switch *ds = dp->ds; > phy_interface_t mode, def_mode; > + struct device_node *phy_np; > int err; > > /* Presence of phylink_mac_link_state or phylink_mac_an_restart is > @@ -1559,6 +1560,13 @@ int dsa_port_phylink_create(struct dsa_port *dp) > return PTR_ERR(dp->pl); > } > > + if (dp->type == DSA_PORT_TYPE_CPU || dp->type == DSA_PORT_TYPE_DSA) { > + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); > + of_node_put(phy_np); > + if (!phy_np) > + err = phylink_set_max_fixed_link(dp->pl); Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding. > + } > + > return 0; > } > > @@ -1663,20 +1671,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp) > int dsa_port_link_register_of(struct dsa_port *dp) > { > struct dsa_switch *ds = dp->ds; > - struct device_node *phy_np; > int port = dp->index; > > if (!ds->ops->adjust_link) { > - phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); > - if (of_phy_is_fixed_link(dp->dn) || phy_np) { > - if (ds->ops->phylink_mac_link_down) > - ds->ops->phylink_mac_link_down(ds, port, > - MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); > - of_node_put(phy_np); > - return dsa_port_phylink_register(dp); > - } > - of_node_put(phy_np); > - return 0; > + if (ds->ops->phylink_mac_link_down) > + ds->ops->phylink_mac_link_down(ds, port, > + MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); Can you please align these arguments to the open bracket? > + > + return dsa_port_phylink_register(dp); > } > > dev_warn(ds->dev, > -- > 2.30.2 >
On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > Hello, > > On Tue, Jul 05, 2022 at 10:48:07AM +0100, Russell King (Oracle) wrote: > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > index 35b4e1f8dc05..34487e62eb03 100644 > > --- a/net/dsa/port.c > > +++ b/net/dsa/port.c > > @@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp) > > { > > struct dsa_switch *ds = dp->ds; > > phy_interface_t mode, def_mode; > > + struct device_node *phy_np; > > int err; > > > > /* Presence of phylink_mac_link_state or phylink_mac_an_restart is > > @@ -1559,6 +1560,13 @@ int dsa_port_phylink_create(struct dsa_port *dp) > > return PTR_ERR(dp->pl); > > } > > > > + if (dp->type == DSA_PORT_TYPE_CPU || dp->type == DSA_PORT_TYPE_DSA) { > > + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); > > + of_node_put(phy_np); > > + if (!phy_np) > > + err = phylink_set_max_fixed_link(dp->pl); > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > where a fixed-link property is also missing, not just a phy-handle? > Although to be entirely correct, we can also have MLO_AN_INBAND, which > wouldn't be covered by these 2 checks and would still represent a valid > DT binding. phylink_set_max_fixed_link() already excludes itself: if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) return -EBUSY; intentionally so that if there is anything specified for the port, be that a fixed link or in-band, then phylink_set_max_fixed_link() errors out with -EBUSY. The only case that it can't detect is if there is a PHY that may be added to phylink at a later time, and that is what the check above is for.
On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote: > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > > where a fixed-link property is also missing, not just a phy-handle? > > Although to be entirely correct, we can also have MLO_AN_INBAND, which > > wouldn't be covered by these 2 checks and would still represent a valid > > DT binding. > > phylink_set_max_fixed_link() already excludes itself: > > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) > return -EBUSY; > > intentionally so that if there is anything specified for the port, be > that a fixed link or in-band, then phylink_set_max_fixed_link() errors > out with -EBUSY. > > The only case that it can't detect is if there is a PHY that may be > added to phylink at a later time, and that is what the check above > is for. I've updated the function description to mention this detail: +/** + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink + * @pl: a pointer to a &struct phylink returned from phylink_create() + * + * Set a maximum speed fixed-link configuration for the chosen interface mode + * and MAC capabilities for the phylink instance if the instance has not + * already been configured with a SFP, fixed link, or in-band AN mode. If the + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported + * interfaces bitmap for the first interface that gives the fastest supported + * speed. Does this address your concern? Thanks.
On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > Hello, > > On Tue, Jul 05, 2022 at 10:48:07AM +0100, Russell King (Oracle) wrote: > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > index 35b4e1f8dc05..34487e62eb03 100644 > > --- a/net/dsa/port.c > > +++ b/net/dsa/port.c > > @@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp) > > { > > struct dsa_switch *ds = dp->ds; > > phy_interface_t mode, def_mode; > > + struct device_node *phy_np; > > int err; > > > > /* Presence of phylink_mac_link_state or phylink_mac_an_restart is > > @@ -1559,6 +1560,13 @@ int dsa_port_phylink_create(struct dsa_port *dp) > > return PTR_ERR(dp->pl); > > } > > > > + if (dp->type == DSA_PORT_TYPE_CPU || dp->type == DSA_PORT_TYPE_DSA) { > > + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); > > + of_node_put(phy_np); > > + if (!phy_np) > > + err = phylink_set_max_fixed_link(dp->pl); > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > where a fixed-link property is also missing, not just a phy-handle? > Although to be entirely correct, we can also have MLO_AN_INBAND, which > wouldn't be covered by these 2 checks and would still represent a valid > DT binding. More importantly, we need your input on Ocelot, which you are listed as a maintainer for, and Ocelot is the only DSA driver that does stuff differently (due to the rate adapting PCS). It doesn't set mac_capabilities, and therefore phylink_set_max_fixed_link() will not work here. Has Ocelot ever made use of this DSA feature where, when nothing is specified for a CPU or DSA port, we use an effective fixed-link setup with an interface mode that gives the highest speed? Or does this not apply to this DSA driver? Thanks.
On Thu, Jul 07, 2022 at 11:09:43AM +0100, Russell King (Oracle) wrote: > On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote: > > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > > > where a fixed-link property is also missing, not just a phy-handle? > > > Although to be entirely correct, we can also have MLO_AN_INBAND, which > > > wouldn't be covered by these 2 checks and would still represent a valid > > > DT binding. > > > > phylink_set_max_fixed_link() already excludes itself: > > > > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) ~~~~~~~~~~ If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() that were initiated by the phylink user between phylink_create() and phylink_set_max_fixed_link(), correct? Those are specified as invalid in the kerneldoc and that's about it - that's not what the checking is for, correct? But if so, why even check for pl->phydev, if you check for pl->sfp_bus? > > return -EBUSY; > > > > intentionally so that if there is anything specified for the port, be > > that a fixed link or in-band, then phylink_set_max_fixed_link() errors > > out with -EBUSY. > > > > The only case that it can't detect is if there is a PHY that may be > > added to phylink at a later time, and that is what the check above > > is for. Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy() after phylink_set_max_fixed_link(), right? So this is what I don't understand. If we've called phylink_set_max_fixed_link() we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA predicts if it's going to call either of those connect_phy() functions, and calls phylink_set_max_fixed_link() only if it won't. Right? You've structured the checks in this "distributed" way because phylink can't really predict whether phylink_{,fwnode_}connect_phy() will be called after phylink_set_max_fixed_link(), right? I mean, it can probably predict the fwnode_ variant, but not phylink_connect_phy, and this is why it is up to the caller to decide when to call and when not to. > I've updated the function description to mention this detail: > > +/** > + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink > + * @pl: a pointer to a &struct phylink returned from phylink_create() > + * > + * Set a maximum speed fixed-link configuration for the chosen interface mode > + * and MAC capabilities for the phylink instance if the instance has not > + * already been configured with a SFP, fixed link, or in-band AN mode. If the > + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported > + * interfaces bitmap for the first interface that gives the fastest supported > + * speed. > > Does this address your concern? > > Thanks. Not really, no, sorry, it just confuses me more. It should maybe also say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later. Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() based on the following? (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error condition from phylink_fwnode_phy_connect(): phy_fwnode = fwnode_get_phy_node(fwnode); if (IS_ERR(phy_fwnode)) { if (pl->cfg_link_an_mode == MLO_AN_PHY) return -ENODEV; <- here return 0; }
On Thu, Jul 07, 2022 at 12:00:54PM +0100, Russell King (Oracle) wrote: > More importantly, we need your input on Ocelot, which you are listed as > a maintainer for, and Ocelot is the only DSA driver that does stuff > differently (due to the rate adapting PCS). It doesn't set > mac_capabilities, and therefore phylink_set_max_fixed_link() will not > work here. > > Has Ocelot ever made use of this DSA feature where, when nothing is > specified for a CPU or DSA port, we use an effective fixed-link setup > with an interface mode that gives the highest speed? Or does this not > apply to this DSA driver? > > Thanks. I'm fine with both the ocelot and sja1105 drivers. The ocelot driver has 3 users: - felix_vsc9959 (arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi) on NXP LS1028A, where the CPU ports have and have always had a fixed-link node in the SoC dtsi. LS1028A based boards should include the SoC dtsi. If other board DT writers don't do that or if they delete the fixed-link node from the CPU ports, that's not my problem and I don't really want to help them. - seville_vsc9953 (arch/powerpc/boot/dts/fsl/t1040si-post.dtsi) on NXP T1040. Same thing, embedded switch, not my fault if the fixed-link disappears from the SoC dtsi. - Colin Foster's SPI-controlled VSC7512 (still downstream). He has an Ethernet cable connecting the CPU port to a Beaglebone Black, so he has a phy-handle on the CPU port, so definitely not nothing. I believe his work hasn't made it to production in any case, so enforcing validation now shouldn't bother him too much if at all. As for sja1105, there is DT validation that checks for the presence of all required properties in sja1105_parse_ports_node(). There is some DT validation in felix_parse_ports_node() too, but it doesn't check that all specifiers that phylink might use are there. I'd really like to add some validation before I gain any involuntary users, but all open-coded constructs I can come up with are clumsy. What would you suggest, if I explicitly don't want to rely on context-specific phylink interpretation of empty OF nodes, and rather error out?
On Thu, Jul 07, 2022 at 06:27:27PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 11:09:43AM +0100, Russell King (Oracle) wrote: > > On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote: > > > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > > > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > > > > where a fixed-link property is also missing, not just a phy-handle? > > > > Although to be entirely correct, we can also have MLO_AN_INBAND, which > > > > wouldn't be covered by these 2 checks and would still represent a valid > > > > DT binding. > > > > > > phylink_set_max_fixed_link() already excludes itself: > > > > > > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) > ~~~~~~~~~~ > > If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from > phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> > phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() > that were initiated by the phylink user between phylink_create() and > phylink_set_max_fixed_link(), correct? Those are specified as invalid in the > kerneldoc and that's about it - that's not what the checking is for, correct? No, it's not to do with sfps at all, but to do with enforcing the pre-conditions for the function - that entire line is checking that (a) we are in a sane state to be called, and (b) there is no configuration initialisation beyond the default done by phylink_create() - in other words, there is no in-band or fixed-link specified. Let's go through this step by step. 1. pl->cfg_link_an_mode != MLO_AN_PHY The default value for cfg_link_an_mode is MLO_AN_PHY. If it's anything other than that, then a fixed-link or in-band mode has been specified, and we don't want to override that. So this call needs to fail. 2. pl->phydev If a PHY has been attached, then the pre-condition for calling this function immediately after phylink_create() has been violated, because the only way it can be non-NULL is if someone's called one of the phylink functions that connects a PHY. Note: SFPs will not set their PHY here, because, for them to discover that there's a PHY, the network interface needs to be up, and it will never be up here... but in any case... 3. pl->sfp_bus If we have a SFP bus, then we definitely are not going to be operating in this default fixed-link mode - because the SFP is going to want to set its own parameters. > > > return -EBUSY; > > > > > > intentionally so that if there is anything specified for the port, be > > > that a fixed link or in-band, then phylink_set_max_fixed_link() errors > > > out with -EBUSY. > > > > > > The only case that it can't detect is if there is a PHY that may be > > > added to phylink at a later time, and that is what the check above > > > is for. > > Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy() > after phylink_set_max_fixed_link(), right? Correct. > So this is what I don't understand. If we've called phylink_set_max_fixed_link() > we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will > silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA > predicts if it's going to call either of those connect_phy() functions, > and calls phylink_set_max_fixed_link() only if it won't. Right? > > You've structured the checks in this "distributed" way because phylink > can't really predict whether phylink_{,fwnode_}connect_phy() will be > called after phylink_set_max_fixed_link(), right? I mean, it can > probably predict the fwnode_ variant, but not phylink_connect_phy, and > this is why it is up to the caller to decide when to call and when not to. phylink has no idea whether phylink_fwnode_connect_phy() will be called with the same fwnode as phylink_create(), so it really can't make any assumptions about whether there will be a PHY or not. > > > I've updated the function description to mention this detail: > > > > +/** > > + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink > > + * @pl: a pointer to a &struct phylink returned from phylink_create() > > + * > > + * Set a maximum speed fixed-link configuration for the chosen interface mode > > + * and MAC capabilities for the phylink instance if the instance has not > > + * already been configured with a SFP, fixed link, or in-band AN mode. If the > > + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported > > + * interfaces bitmap for the first interface that gives the fastest supported > > + * speed. > > > > Does this address your concern? > > > > Thanks. > > Not really, no, sorry, it just confuses me more. I find that happens a lot when I try to add more documentation to clarify things. I sometimes get to the point of deciding its better _not_ to write any documentation, because documentation just ends up being confusing and people want more and more detail. I've got to the point in some case where I've had to spell out which keys to press on the keyboard for people to formulate the "[PATCH ...]" thing correctly, because if you put it in quotes, you get people who will include the quotes even if you tell them not to. I hate documentation, I seem incapable of writing it in a way people can understand. > It should maybe also > say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() > is going to be called later. It's already a precondition that phylink_{,fwnode_}connect_phy() fail if we're in fixed-link mode (because PHYs have never been supported when in fixed-link mode - if one remembers, the old fixed-link code used to provide its own emulation of a PHY to make fixed-links work.) So PHYs and fixed-links have always been mutually exclusive before phylink, and continue to be so with phylink. > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > based on the following? > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > condition from phylink_fwnode_phy_connect(): > > phy_fwnode = fwnode_get_phy_node(fwnode); > if (IS_ERR(phy_fwnode)) { > if (pl->cfg_link_an_mode == MLO_AN_PHY) > return -ENODEV; <- here > return 0; > } My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this?
On Thu, Jul 07, 2022 at 06:43:03PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 12:00:54PM +0100, Russell King (Oracle) wrote: > > More importantly, we need your input on Ocelot, which you are listed as > > a maintainer for, and Ocelot is the only DSA driver that does stuff > > differently (due to the rate adapting PCS). It doesn't set > > mac_capabilities, and therefore phylink_set_max_fixed_link() will not > > work here. > > > > Has Ocelot ever made use of this DSA feature where, when nothing is > > specified for a CPU or DSA port, we use an effective fixed-link setup > > with an interface mode that gives the highest speed? Or does this not > > apply to this DSA driver? > > > > Thanks. > > I'm fine with both the ocelot and sja1105 drivers. > > The ocelot driver has 3 users: > > - felix_vsc9959 (arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi) on NXP > LS1028A, where the CPU ports have and have always had a fixed-link > node in the SoC dtsi. LS1028A based boards should include the SoC > dtsi. If other board DT writers don't do that or if they delete the > fixed-link node from the CPU ports, that's not my problem and I don't > really want to help them. > > - seville_vsc9953 (arch/powerpc/boot/dts/fsl/t1040si-post.dtsi) on NXP > T1040. Same thing, embedded switch, not my fault if the fixed-link > disappears from the SoC dtsi. Great, so I'll mark ocelot is safe. > - Colin Foster's SPI-controlled VSC7512 (still downstream). He has an > Ethernet cable connecting the CPU port to a Beaglebone Black, so he > has a phy-handle on the CPU port, so definitely not nothing. I believe > his work hasn't made it to production in any case, so enforcing > validation now shouldn't bother him too much if at all. Ok, thanks. > As for sja1105, there is DT validation that checks for the presence of > all required properties in sja1105_parse_ports_node(). Looking at those, it requires all of: - a phy mode to be specified (as determined by of_get_phy_mode()) - a phy-handle or of_phy_is_fixed_link() to return true otherwise it errors out. > There is some DT validation in felix_parse_ports_node() too, but it > doesn't check that all specifiers that phylink might use are there. Phylink (correction, fwnode_get_phy_node() which is not part of phylink anymore) will look for phy-handle, phy, or phy-device. This is I don't see that there's any incompatibility between what the driver is doing and what phylink does. If there's a fixed-link property, then sja1105_parse_ports_node() is happy, and so will phylink. If there's a phy-handle, the same is true. If there's a "phy" or "phy-device" then sja1105_parse_ports_node() errors out. That's completely fine. "phy" and "phy-device" are the backwards compatibility for DT - I believe one of them is the ePAPR specified property that we in Linux have decided to only fall back on if there's not our more modern "phy-handle" property. It seems We have a lot of users of "phy" in DT today, so we can't drop that from generic code such as phylink, but I haven't found any users of "phy-device". > I'd really like to add some validation before I gain any involuntary > users, but all open-coded constructs I can come up with are clumsy. > What would you suggest, if I explicitly don't want to rely on > context-specific phylink interpretation of empty OF nodes, and rather > error out? So I also don't see a problem - sja1105 rejects DTs that fail to describe a port using at least one of a phy-handle, a fixed-link, or a managed in-band link, and I don't think it needs to do further validation, certainly not for the phy describing properties that the kernel has chosen to deprecate for new implementations.
On Thu, Jul 07, 2022 at 04:48:12PM +0100, Russell King (Oracle) wrote: > On Thu, Jul 07, 2022 at 06:27:27PM +0300, Vladimir Oltean wrote: > > On Thu, Jul 07, 2022 at 11:09:43AM +0100, Russell King (Oracle) wrote: > > > On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote: > > > > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > > > > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > > > > > where a fixed-link property is also missing, not just a phy-handle? > > > > > Although to be entirely correct, we can also have MLO_AN_INBAND, which > > > > > wouldn't be covered by these 2 checks and would still represent a valid > > > > > DT binding. > > > > > > > > phylink_set_max_fixed_link() already excludes itself: > > > > > > > > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) > > ~~~~~~~~~~ > > > > If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from > > phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> > > phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() > > that were initiated by the phylink user between phylink_create() and > > phylink_set_max_fixed_link(), correct? Those are specified as invalid in the > > kerneldoc and that's about it - that's not what the checking is for, correct? > > No, it's not to do with sfps at all, but to do with enforcing the > pre-conditions for the function - that entire line is checking that > (a) we are in a sane state to be called, and (b) there is no > configuration initialisation beyond the default done by > phylink_create() - in other words, there is no in-band or fixed-link > specified. > > Let's go through this step by step. > > 1. pl->cfg_link_an_mode != MLO_AN_PHY > The default value for cfg_link_an_mode is MLO_AN_PHY. If it's > anything other than that, then a fixed-link or in-band mode has > been specified, and we don't want to override that. So this call > needs to fail. Thanks for the explanation. Yes, I noticed that phylink_set_max_fixed_link() relies on the fact that pl->cfg_link_an_mode has the unset value of 0, which coincidentally is MLO_AN_PHY. > 2. pl->phydev > If a PHY has been attached, then the pre-condition for calling this > function immediately after phylink_create() has been violated, > because the only way it can be non-NULL is if someone's called one of > the phylink functions that connects a PHY. Note: SFPs will not set > their PHY here, because, for them to discover that there's a PHY, the > network interface needs to be up, and it will never be up here... but > in any case... Ok, so this does check for a precondition that the caller did something correctly. But it doesn't (and can't) check that all preconditions and postconditions are satisfied. That's one of my irks, why bother checking the easy to satisfy precondition (which depends on the code organization, static information, easy to check), and give up on the hard one (which depends on the device tree blob, dynamic information, not so easy). > > So this is what I don't understand. If we've called phylink_set_max_fixed_link() > > we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will > > silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA > > predicts if it's going to call either of those connect_phy() functions, > > and calls phylink_set_max_fixed_link() only if it won't. Right? > > > > You've structured the checks in this "distributed" way because phylink > > can't really predict whether phylink_{,fwnode_}connect_phy() will be > > called after phylink_set_max_fixed_link(), right? I mean, it can > > probably predict the fwnode_ variant, but not phylink_connect_phy, and > > this is why it is up to the caller to decide when to call and when not to. > > phylink has no idea whether phylink_fwnode_connect_phy() will be called > with the same fwnode as phylink_create(), so it really can't make any > assumptions about whether there will be a PHY or not. This is interesting. Is there a use case for passing a different fwnode_handle to the 2 functions? > > It should maybe also > > say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() > > is going to be called later. > > It's already a precondition that phylink_{,fwnode_}connect_phy() fail if > we're in fixed-link mode (because PHYs have never been supported when in > fixed-link mode - if one remembers, the old fixed-link code used to > provide its own emulation of a PHY to make fixed-links work.) So PHYs > and fixed-links have always been mutually exclusive before phylink, and > continue to be so with phylink. Define "fail" exactly, because if I look in phylink_fwnode_phy_connect(), I see: /* Fixed links and 802.3z are handled without needing a PHY */ if (pl->cfg_link_an_mode == MLO_AN_FIXED || (pl->cfg_link_an_mode == MLO_AN_INBAND && phy_interface_mode_is_8023z(pl->link_interface))) return 0; <- does this count as failure? This is why dsa_port_phylink_register() calls phylink_of_phy_connect() without checking whether it has a fixed-link or a PHY, because it doesn't fail even if it doesn't do anything. In fact I've wanted to make a correction to my previous phrasing that "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later". The correction is "... with a phy-handle". > > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > > based on the following? > > > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > > condition from phylink_fwnode_phy_connect(): > > > > phy_fwnode = fwnode_get_phy_node(fwnode); > > if (IS_ERR(phy_fwnode)) { > > if (pl->cfg_link_an_mode == MLO_AN_PHY) > > return -ENODEV; <- here > > return 0; > > } > > My question in response would be - why should this DSA specific behaviour > be handled completely internally within phylink, when it's a DSA > specific behaviour? Why do we need boolean flags for this? Because the end result will be simpler if we respect the separation of concerns that continues to exist, and it's still phylink's business to say what is and isn't valid. DSA still isn't aware of the bindings required by phylink, it just passes its fwnode to it. Practically speaking, I wouldn't be scratching my head as to why we're checking for half the prerequisites of phylink_set_max_fixed_link() in one place and for the other half in another. True, through this patch set DSA is creating its own context specific extension of phylink bindings, but arguably those existed before DSA was even integrated with phylink, and we're just fixing something now we didn't realize at the time we'd need to do. I can reverse the question, why would phylink even want to be involved in how the max fixed link parameters are deduced, and it doesn't just require that a fixed-link software node is constructed somehow (irrelevant to phylink how), and phylink is just modified to find and work with that if it exists? Isn't it for the exact same reason, separation of concerns, that it's easiest for phylink to figure out what is the most appropriate maximum fixed-link configuration?
On Thu, Jul 07, 2022 at 05:32:57PM +0100, Russell King (Oracle) wrote: > Great, so I'll mark ocelot is safe. Yes, please. > > As for sja1105, there is DT validation that checks for the presence of > > all required properties in sja1105_parse_ports_node(). > > Looking at those, it requires all of: > > - a phy mode to be specified (as determined by of_get_phy_mode()) > - a phy-handle or of_phy_is_fixed_link() to return true > > otherwise it errors out. I know. The problem with this ad-hoc validation is that it doesn't cover the pure MLO_AN_INBAND: managed = "in-band-status"; so it is more restrictive than it needs to be. Also it doesn't recognize the presence of an SFP bus in MLO_AN_PHY mode. That is part 1 of my problem. I want to have validation that I'm providing phylink with all the right things it may need, but I don't want to make the driver code super clunky. By checking just the presence of either phy-handle or fixed-link I am rejecting valid phylink configurations. What I need is a validation function that is actually in sync with phylink, not just ad-hoc. > > There is some DT validation in felix_parse_ports_node() too, but it > > doesn't check that all specifiers that phylink might use are there. > > Phylink (correction, fwnode_get_phy_node() which is not part of phylink > anymore) will look for phy-handle, phy, or phy-device. This is I don't > see that there's any incompatibility between what the driver is doing > and what phylink does. > > If there's a fixed-link property, then sja1105_parse_ports_node() is > happy, and so will phylink. If there's a phy-handle, the same is true. > If there's a "phy" or "phy-device" then sja1105_parse_ports_node() > errors out. That's completely fine. > > "phy" and "phy-device" are the backwards compatibility for DT - I > believe one of them is the ePAPR specified property that we in Linux > have decided to only fall back on if there's not our more modern > "phy-handle" property. > > It seems We have a lot of users of "phy" in DT today, so we can't drop > that from generic code such as phylink, but I haven't found any users > of "phy-device". > > > I'd really like to add some validation before I gain any involuntary > > users, but all open-coded constructs I can come up with are clumsy. > > What would you suggest, if I explicitly don't want to rely on > > context-specific phylink interpretation of empty OF nodes, and rather > > error out? > > So I also don't see a problem - sja1105 rejects DTs that fail to > describe a port using at least one of a phy-handle, a fixed-link, or > a managed in-band link, and I don't think it needs to do further > validation, certainly not for the phy describing properties that > the kernel has chosen to deprecate for new implementations. And this is part 2 of my problem, ocelot/felix doesn't have validation at all except for phy-mode, because if it were to simply copy the phy-handle/fixed-link either/or logic from sja1105, it would break some customer boards with SFP cages. But without that validation, I am exposing this driver to configurations I don't want it to support (CPU ports with empty OF nodes, i.o.w. what this patch set is about).
On Thu, Jul 07, 2022 at 07:38:31PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 04:48:12PM +0100, Russell King (Oracle) wrote: > > Let's go through this step by step. > > > > 1. pl->cfg_link_an_mode != MLO_AN_PHY > > The default value for cfg_link_an_mode is MLO_AN_PHY. If it's > > anything other than that, then a fixed-link or in-band mode has > > been specified, and we don't want to override that. So this call > > needs to fail. > > Thanks for the explanation. > > Yes, I noticed that phylink_set_max_fixed_link() relies on the fact that > pl->cfg_link_an_mode has the unset value of 0, which coincidentally is > MLO_AN_PHY. > > > 2. pl->phydev > > If a PHY has been attached, then the pre-condition for calling this > > function immediately after phylink_create() has been violated, > > because the only way it can be non-NULL is if someone's called one of > > the phylink functions that connects a PHY. Note: SFPs will not set > > their PHY here, because, for them to discover that there's a PHY, the > > network interface needs to be up, and it will never be up here... but > > in any case... > > Ok, so this does check for a precondition that the caller did something > correctly. But it doesn't (and can't) check that all preconditions and > postconditions are satisfied. That's one of my irks, why bother checking > the easy to satisfy precondition (which depends on the code organization, > static information, easy to check), and give up on the hard one (which > depends on the device tree blob, dynamic information, not so easy). So what you're asking is: why bother doing any checks if you can't do all of them? My response would be: isn't best effort better than doing nothing? In my mind, it is best effort, because: a) if you've called it when the preconditions (with the exception of a future PHY) are not satisfied, then it fails with -EBUSY. b) if this call succeeds, then it locks out the future ability to bind a PHY. So, if one forgets to check whether there'll be a future PHY, and call this anyway, then a future attempt to bind a PHY to phylink fails and you get a failure. Considering that we are only talking about DSA making use of this (no other network driver has this behaviour) and this is being handled by core DSA code, we could label this up as "this is for DSA use only." > > > So this is what I don't understand. If we've called phylink_set_max_fixed_link() > > > we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will > > > silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA > > > predicts if it's going to call either of those connect_phy() functions, > > > and calls phylink_set_max_fixed_link() only if it won't. Right? > > > > > > You've structured the checks in this "distributed" way because phylink > > > can't really predict whether phylink_{,fwnode_}connect_phy() will be > > > called after phylink_set_max_fixed_link(), right? I mean, it can > > > probably predict the fwnode_ variant, but not phylink_connect_phy, and > > > this is why it is up to the caller to decide when to call and when not to. > > > > phylink has no idea whether phylink_fwnode_connect_phy() will be called > > with the same fwnode as phylink_create(), so it really can't make any > > assumptions about whether there will be a PHY or not. > > This is interesting. Is there a use case for passing a different > fwnode_handle to the 2 functions? That depends on the driver. It looks like drivers/net/ethernet/ti/am65-cpsw-nuss.c may well pass in different nodes in the firmware tree. > > > It should maybe also > > > say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() > > > is going to be called later. > > > > It's already a precondition that phylink_{,fwnode_}connect_phy() fail if > > we're in fixed-link mode (because PHYs have never been supported when in > > fixed-link mode - if one remembers, the old fixed-link code used to > > provide its own emulation of a PHY to make fixed-links work.) So PHYs > > and fixed-links have always been mutually exclusive before phylink, and > > continue to be so with phylink. > > Define "fail" exactly, because if I look in phylink_fwnode_phy_connect(), I see: > > /* Fixed links and 802.3z are handled without needing a PHY */ > if (pl->cfg_link_an_mode == MLO_AN_FIXED || > (pl->cfg_link_an_mode == MLO_AN_INBAND && > phy_interface_mode_is_8023z(pl->link_interface))) > return 0; <- does this count as failure? Sorry, yes, that is correct - it ignores the attempt (so drivers don't have to conditionalise the call for this everywhere.) > This is why dsa_port_phylink_register() calls phylink_of_phy_connect() > without checking whether it has a fixed-link or a PHY, because it > doesn't fail even if it doesn't do anything. > > In fact I've wanted to make a correction to my previous phrasing that > "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is > going to be called later". The correction is "... with a phy-handle". I'm not sure that clarification makes sense when talking about phylink_connect_phy(), so I think if you're clarifying it with a firmware property, you're only talking about phylink_fwnode_connect_phy() now? > > > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > > > based on the following? > > > > > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > > > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > > > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > > > condition from phylink_fwnode_phy_connect(): > > > > > > phy_fwnode = fwnode_get_phy_node(fwnode); > > > if (IS_ERR(phy_fwnode)) { > > > if (pl->cfg_link_an_mode == MLO_AN_PHY) > > > return -ENODEV; <- here > > > return 0; > > > } > > > > My question in response would be - why should this DSA specific behaviour > > be handled completely internally within phylink, when it's a DSA > > specific behaviour? Why do we need boolean flags for this? > > Because the end result will be simpler if we respect the separation of > concerns that continues to exist, and it's still phylink's business to > say what is and isn't valid. DSA still isn't aware of the bindings > required by phylink, it just passes its fwnode to it. Practically > speaking, I wouldn't be scratching my head as to why we're checking for > half the prerequisites of phylink_set_max_fixed_link() in one place and > for the other half in another. > > True, through this patch set DSA is creating its own context specific > extension of phylink bindings, but arguably those existed before DSA was > even integrated with phylink, and we're just fixing something now we > didn't realize at the time we'd need to do. > > I can reverse the question, why would phylink even want to be involved > in how the max fixed link parameters are deduced, and it doesn't just > require that a fixed-link software node is constructed somehow > (irrelevant to phylink how), and phylink is just modified to find and > work with that if it exists? Isn't it for the exact same reason, > separation of concerns, that it's easiest for phylink to figure out what > is the most appropriate maximum fixed-link configuration? If that could be done, I'd love it, because then we don't have this in phylink at all, and it can all be a DSA problem to solve. It also means that others won't be tempted to use the interface incorrectly. I'm not sure how practical that is when we have both DT and ACPI to deal with, and ACPI is certainly out of my knowledge area to be able to construct a software node to specify a fixed-link. Maybe it can be done at the fwnode layer? I don't know. Do you have a handy example of what you're suggesting?
On Thu, Jul 07, 2022 at 06:15:46PM +0100, Russell King (Oracle) wrote: > > This is why dsa_port_phylink_register() calls phylink_of_phy_connect() > > without checking whether it has a fixed-link or a PHY, because it > > doesn't fail even if it doesn't do anything. > > > > In fact I've wanted to make a correction to my previous phrasing that > > "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is > > going to be called later". The correction is "... with a phy-handle". > > I'm not sure that clarification makes sense when talking about > phylink_connect_phy(), so I think if you're clarifying it with a > firmware property, you're only talking about > phylink_fwnode_connect_phy() now? Yes, it's super hard to verbalize, and this is the reason why I didn't add "... with a phy-handle" in the first place. I wanted to say: phylink_connect_phy(), OR phylink_fwnode_connect_phy() WITH a phy-handle. I shouldn't have conflated them in the first place. > > > > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > > > > based on the following? > > > > > > > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > > > > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > > > > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > > > > condition from phylink_fwnode_phy_connect(): > > > > > > > > phy_fwnode = fwnode_get_phy_node(fwnode); > > > > if (IS_ERR(phy_fwnode)) { > > > > if (pl->cfg_link_an_mode == MLO_AN_PHY) > > > > return -ENODEV; <- here > > > > return 0; > > > > } > > > > > > My question in response would be - why should this DSA specific behaviour > > > be handled completely internally within phylink, when it's a DSA > > > specific behaviour? Why do we need boolean flags for this? > > > > Because the end result will be simpler if we respect the separation of > > concerns that continues to exist, and it's still phylink's business to > > say what is and isn't valid. DSA still isn't aware of the bindings > > required by phylink, it just passes its fwnode to it. Practically > > speaking, I wouldn't be scratching my head as to why we're checking for > > half the prerequisites of phylink_set_max_fixed_link() in one place and > > for the other half in another. > > > > True, through this patch set DSA is creating its own context specific > > extension of phylink bindings, but arguably those existed before DSA was > > even integrated with phylink, and we're just fixing something now we > > didn't realize at the time we'd need to do. > > > > I can reverse the question, why would phylink even want to be involved > > in how the max fixed link parameters are deduced, and it doesn't just > > require that a fixed-link software node is constructed somehow > > (irrelevant to phylink how), and phylink is just modified to find and > > work with that if it exists? Isn't it for the exact same reason, > > separation of concerns, that it's easiest for phylink to figure out what > > is the most appropriate maximum fixed-link configuration? > > If that could be done, I'd love it, because then we don't have this in > phylink at all, and it can all be a DSA problem to solve. It also means > that others won't be tempted to use the interface incorrectly. > > I'm not sure how practical that is when we have both DT and ACPI to deal > with, and ACPI is certainly out of my knowledge area to be able to > construct a software node to specify a fixed-link. Maybe it can be done > at the fwnode layer? I don't know. I don't want to be misunderstood. I brought up software nodes because I'm sure you must have thought about this too, before proposing what you did here. And unless there's a technical reason against software nodes (which there doesn't appear to be, but I don't want to get ahead of myself), I figured you must be OK with phylink absorbing the logic, case in which I just don't understand why you are pushing back on a proposal how to make phylink absorb the logic completely. > Do you have a handy example of what you're suggesting? No, I didn't, but I thought, how hard can it be, and here's a hacked up attempt on one of my boards: From 5d002f03c91b357be304d41f7422969d0dd89f5b Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: Thu, 7 Jul 2022 21:04:32 +0300 Subject: [PATCH] dsa_port_fixup_broken_dt Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- arch/arm/boot/dts/ls1021a-tsn.dts | 5 -- drivers/base/swnode.c | 14 +++- drivers/net/dsa/sja1105/sja1105_main.c | 4 +- include/linux/property.h | 4 ++ net/dsa/dsa_priv.h | 2 +- net/dsa/port.c | 99 +++++++++++++++++++++----- net/dsa/slave.c | 2 +- 7 files changed, 100 insertions(+), 30 deletions(-) diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts b/arch/arm/boot/dts/ls1021a-tsn.dts index 1ea32fff4120..c577f90057c7 100644 --- a/arch/arm/boot/dts/ls1021a-tsn.dts +++ b/arch/arm/boot/dts/ls1021a-tsn.dts @@ -94,11 +94,6 @@ port@4 { rx-internal-delay-ps = <0>; tx-internal-delay-ps = <0>; reg = <4>; - - fixed-link { - speed = <1000>; - full-duplex; - }; }; }; }; diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 0a482212c7e8..b2ea08f0e898 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -972,8 +972,9 @@ void software_node_unregister(const struct software_node *node) EXPORT_SYMBOL_GPL(software_node_unregister); struct fwnode_handle * -fwnode_create_software_node(const struct property_entry *properties, - const struct fwnode_handle *parent) +fwnode_create_named_software_node(const struct property_entry *properties, + const struct fwnode_handle *parent, + const char *name) { struct fwnode_handle *fwnode; struct software_node *node; @@ -991,6 +992,7 @@ fwnode_create_software_node(const struct property_entry *properties, return ERR_CAST(node); node->parent = p ? p->node : NULL; + node->name = name; fwnode = swnode_register(node, p, 1); if (IS_ERR(fwnode)) @@ -998,6 +1000,14 @@ fwnode_create_software_node(const struct property_entry *properties, return fwnode; } +EXPORT_SYMBOL_GPL(fwnode_create_named_software_node); + +struct fwnode_handle * +fwnode_create_software_node(const struct property_entry *properties, + const struct fwnode_handle *parent) +{ + return fwnode_create_named_software_node(properties, parent, NULL); +} EXPORT_SYMBOL_GPL(fwnode_create_software_node); void fwnode_remove_software_node(struct fwnode_handle *fwnode) diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index b253e27bcfb4..6c7deebc8d76 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -1218,8 +1218,8 @@ static int sja1105_parse_ports_node(struct sja1105_private *priv, if (!of_phy_is_fixed_link(child)) { dev_err(dev, "phy-handle or fixed-link " "properties missing!\n"); - of_node_put(child); - return -ENODEV; +// of_node_put(child); +// return -ENODEV; } /* phy-handle is missing, but fixed-link isn't. * So it's a fixed link. Default to PHY role. diff --git a/include/linux/property.h b/include/linux/property.h index a5b429d623f6..23330ae2b1fa 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -492,6 +492,10 @@ void software_node_unregister(const struct software_node *node); struct fwnode_handle * fwnode_create_software_node(const struct property_entry *properties, const struct fwnode_handle *parent); +struct fwnode_handle * +fwnode_create_named_software_node(const struct property_entry *properties, + const struct fwnode_handle *parent, + const char *name); void fwnode_remove_software_node(struct fwnode_handle *fwnode); int device_add_software_node(struct device *dev, const struct software_node *node); diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index d9722e49864b..95c4e13e2dbf 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -284,7 +284,7 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp, const struct switchdev_obj_ring_role_mrp *mrp); int dsa_port_mrp_del_ring_role(const struct dsa_port *dp, const struct switchdev_obj_ring_role_mrp *mrp); -int dsa_port_phylink_create(struct dsa_port *dp); +int dsa_port_phylink_create(struct dsa_port *dp, struct fwnode_handle *fwnode); int dsa_port_link_register_of(struct dsa_port *dp); void dsa_port_link_unregister_of(struct dsa_port *dp); int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr); diff --git a/net/dsa/port.c b/net/dsa/port.c index 3738f2d40a0b..4ed9075468f4 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1521,15 +1521,14 @@ static const struct phylink_mac_ops dsa_port_phylink_mac_ops = { .mac_link_up = dsa_port_phylink_mac_link_up, }; -int dsa_port_phylink_create(struct dsa_port *dp) +int dsa_port_phylink_create(struct dsa_port *dp, struct fwnode_handle *fwnode) { struct dsa_switch *ds = dp->ds; phy_interface_t mode; int err; - err = of_get_phy_mode(dp->dn, &mode); - if (err) - mode = PHY_INTERFACE_MODE_NA; + err = fwnode_get_phy_mode(fwnode); + mode = err < 0 ? PHY_INTERFACE_MODE_NA : err; /* Presence of phylink_mac_link_state or phylink_mac_an_restart is * an indicator of a legacy phylink driver. @@ -1541,8 +1540,8 @@ int dsa_port_phylink_create(struct dsa_port *dp) if (ds->ops->phylink_get_caps) ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config); - dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), - mode, &dsa_port_phylink_mac_ops); + dp->pl = phylink_create(&dp->pl_config, fwnode, mode, + &dsa_port_phylink_mac_ops); if (IS_ERR(dp->pl)) { pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl)); return PTR_ERR(dp->pl); @@ -1627,16 +1626,19 @@ static int dsa_port_phylink_register(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; struct device_node *port_dn = dp->dn; + struct fwnode_handle *fwnode; int err; dp->pl_config.dev = ds->dev; dp->pl_config.type = PHYLINK_DEV; - err = dsa_port_phylink_create(dp); + fwnode = port_dn->fwnode.secondary ? : of_fwnode_handle(port_dn); + + err = dsa_port_phylink_create(dp, fwnode); if (err) return err; - err = phylink_of_phy_connect(dp->pl, port_dn, 0); + err = phylink_fwnode_phy_connect(dp->pl, fwnode, 0); if (err && err != -ENODEV) { pr_err("could not attach to PHY: %d\n", err); goto err_phy_connect; @@ -1649,23 +1651,82 @@ static int dsa_port_phylink_register(struct dsa_port *dp) return err; } +static int dsa_port_fixup_broken_dt(struct dsa_port *dp) +{ + struct property_entry fixed_link_props[] = { + PROPERTY_ENTRY_BOOL("full-duplex"), + PROPERTY_ENTRY_U32("speed", 1000), /* TODO determine actual speed */ + {}, + }; + struct property_entry port_props[3] = {}; + struct fwnode_handle *fixed_link_fwnode; + struct fwnode_handle *new_port_fwnode; + struct device_node *dn = dp->dn; + phy_interface_t mode; + int err; + + if (of_parse_phandle(dn, "phy-handle", 0) || + of_phy_is_fixed_link(dn)) + /* Nothing broken, nothing to fix. + * TODO: As discussed with Russell, maybe phylink could provide + * a more comprehensive helper to determine what constitutes a + * valid fwnode binding than this guerilla kludge. + */ + return 0; + + err = of_get_phy_mode(dn, &mode); + if (err) + /* TODO this may be missing too, ask the driver for the + * max-speed interface mode for this port + */ + mode = PHY_INTERFACE_MODE_NA; + + port_props[0] = PROPERTY_ENTRY_U32("reg", dp->index); + port_props[1] = PROPERTY_ENTRY_STRING("phy-mode", phy_modes(mode)); + + new_port_fwnode = fwnode_create_software_node(port_props, NULL); + if (IS_ERR(new_port_fwnode)) + return PTR_ERR(new_port_fwnode); + + /* Node needs to be named so that phylink's call to + * fwnode_get_named_child_node() finds it. + */ + fixed_link_fwnode = fwnode_create_named_software_node(fixed_link_props, + new_port_fwnode, + "fixed-link"); + if (IS_ERR(fixed_link_fwnode)) { + fwnode_remove_software_node(new_port_fwnode); + return PTR_ERR(fixed_link_fwnode); + } + + /* set_secondary_fwnode() takes a struct device as argument, + * and we have none for a port. TODO we need to free this. + */ + dn->fwnode.secondary = new_port_fwnode; + + return 0; +} + int dsa_port_link_register_of(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; - struct device_node *phy_np; int port = dp->index; + int err; + + err = dsa_port_fixup_broken_dt(dp); + if (err) { + dev_err(ds->dev, + "Failed to fix up broken DT for shared port %d: %pe\n", + dp->index, ERR_PTR(err)); + return err; + } if (!ds->ops->adjust_link) { - phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); - if (of_phy_is_fixed_link(dp->dn) || phy_np) { - if (ds->ops->phylink_mac_link_down) - ds->ops->phylink_mac_link_down(ds, port, - MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); - of_node_put(phy_np); - return dsa_port_phylink_register(dp); - } - of_node_put(phy_np); - return 0; + if (ds->ops->phylink_mac_link_down) + ds->ops->phylink_mac_link_down(ds, port, MLO_AN_FIXED, + PHY_INTERFACE_MODE_NA); + + return dsa_port_phylink_register(dp); } dev_warn(ds->dev, diff --git a/net/dsa/slave.c b/net/dsa/slave.c index ad6a6663feeb..f243e73cb522 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2245,7 +2245,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) dp->pl_config.poll_fixed_state = true; } - ret = dsa_port_phylink_create(dp); + ret = dsa_port_phylink_create(dp, of_fwnode_handle(dp->dn)); if (ret) return ret;
On Thu, Jul 07, 2022 at 10:37:53PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 06:15:46PM +0100, Russell King (Oracle) wrote: > > > This is why dsa_port_phylink_register() calls phylink_of_phy_connect() > > > without checking whether it has a fixed-link or a PHY, because it > > > doesn't fail even if it doesn't do anything. > > > > > > In fact I've wanted to make a correction to my previous phrasing that > > > "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is > > > going to be called later". The correction is "... with a phy-handle". > > > > I'm not sure that clarification makes sense when talking about > > phylink_connect_phy(), so I think if you're clarifying it with a > > firmware property, you're only talking about > > phylink_fwnode_connect_phy() now? > > Yes, it's super hard to verbalize, and this is the reason why I didn't > add "... with a phy-handle" in the first place. > > I wanted to say: phylink_connect_phy(), OR phylink_fwnode_connect_phy() > WITH a phy-handle. I shouldn't have conflated them in the first place. Ah, right, because I interpreted it quite differently! > > > > > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > > > > > based on the following? > > > > > > > > > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > > > > > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > > > > > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > > > > > condition from phylink_fwnode_phy_connect(): > > > > > > > > > > phy_fwnode = fwnode_get_phy_node(fwnode); > > > > > if (IS_ERR(phy_fwnode)) { > > > > > if (pl->cfg_link_an_mode == MLO_AN_PHY) > > > > > return -ENODEV; <- here > > > > > return 0; > > > > > } > > > > > > > > My question in response would be - why should this DSA specific behaviour > > > > be handled completely internally within phylink, when it's a DSA > > > > specific behaviour? Why do we need boolean flags for this? > > > > > > Because the end result will be simpler if we respect the separation of > > > concerns that continues to exist, and it's still phylink's business to > > > say what is and isn't valid. DSA still isn't aware of the bindings > > > required by phylink, it just passes its fwnode to it. Practically > > > speaking, I wouldn't be scratching my head as to why we're checking for > > > half the prerequisites of phylink_set_max_fixed_link() in one place and > > > for the other half in another. > > > > > > True, through this patch set DSA is creating its own context specific > > > extension of phylink bindings, but arguably those existed before DSA was > > > even integrated with phylink, and we're just fixing something now we > > > didn't realize at the time we'd need to do. > > > > > > I can reverse the question, why would phylink even want to be involved > > > in how the max fixed link parameters are deduced, and it doesn't just > > > require that a fixed-link software node is constructed somehow > > > (irrelevant to phylink how), and phylink is just modified to find and > > > work with that if it exists? Isn't it for the exact same reason, > > > separation of concerns, that it's easiest for phylink to figure out what > > > is the most appropriate maximum fixed-link configuration? > > > > If that could be done, I'd love it, because then we don't have this in > > phylink at all, and it can all be a DSA problem to solve. It also means > > that others won't be tempted to use the interface incorrectly. > > > > I'm not sure how practical that is when we have both DT and ACPI to deal > > with, and ACPI is certainly out of my knowledge area to be able to > > construct a software node to specify a fixed-link. Maybe it can be done > > at the fwnode layer? I don't know. > > I don't want to be misunderstood. I brought up software nodes because > I'm sure you must have thought about this too, before proposing what you > did here. And unless there's a technical reason against software nodes > (which there doesn't appear to be, but I don't want to get ahead of > myself), I figured you must be OK with phylink absorbing the logic, case > in which I just don't understand why you are pushing back on a proposal > how to make phylink absorb the logic completely. The reason I hadn't is because switching DSA to fwnode brings with it issues for ACPI, and Andrew wants to be very careful about ACPI in networking - and I think quite rightly. As soon as one switches from DT APIs to fwnode APIs, you basically permit people an easy path to re-use DT properties in ACPI-land without the ACPI issues being first considered. So, I think if we did go this route, we need Andrew's input. > > Do you have a handy example of what you're suggesting? > > No, I didn't, but I thought, how hard can it be, and here's a hacked up > attempt on one of my boards: Thanks - that looks like something that should be possible to do, and way better than trying to shoe-horn this into phylink. My only comment would be that Andrew would disagree with you about this being "fixing up broken DT" - he has actively encouraged some drivers to adopt this "default" mode, which means it's anything but "broken" but it really is part of the DSA firmware description. > [ 4.315754] sja1105 spi0.1: configuring for fixed/rgmii link mode > [ 4.322653] sja1105 spi0.1 swp5 (uninitialized): PHY [mdio@2d24000:06] driver [Broadcom BCM5464] (irq=POLL) > [ 4.334796] sja1105 spi0.1 swp2 (uninitialized): PHY [mdio@2d24000:03] driver [Broadcom BCM5464] (irq=POLL) > [ 4.345853] sja1105 spi0.1 swp3 (uninitialized): PHY [mdio@2d24000:04] driver [Broadcom BCM5464] (irq=POLL) > [ 4.356859] sja1105 spi0.1 swp4 (uninitialized): PHY [mdio@2d24000:05] driver [Broadcom BCM5464] (irq=POLL) > [ 4.367245] device eth2 entered promiscuous mode > [ 4.371864] DSA: tree 0 setup > [ 4.376971] sja1105 spi0.1: Link is Up - 1Gbps/Full - flow control off > (...) > root@black:~# ip link set swp2 up && dhclient -i swp2 && ip addr show swp2 > [ 64.762756] fsl-gianfar soc:ethernet@2d90000 eth2: Link is Up - 1Gbps/Full - flow control off > [ 64.771530] sja1105 spi0.1 swp2: configuring for phy/rgmii-id link mode > [ 68.955048] sja1105 spi0.1 swp2: Link is Up - 1Gbps/Full - flow control off > 12: swp2@eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000 > link/ether 00:1f:7b:63:02:48 brd ff:ff:ff:ff:ff:ff > inet 10.0.0.68/24 brd 10.0.0.255 scope global dynamic swp2 > valid_lft 600sec preferred_lft 600sec > > It's by far the messiest patch I've posted to the list (in the interest > of responding quickly), but if you study the code you can obviously see > what's missing, basically I've hardcoded the speed to 1000 and I'm > copying the phy-mode from the real DT node. Yep - there's at least one other property we need to carry over from the DT node, which is the "ethernet" property. > Unfortunately I don't have the time (and most importantly the interest) > in pushing this any further than that. If you want to take this from > here and integrate it with phylink_get_caps() I'd be glad to review > the result. Otherwise, feel free to continue with phylink_set_max_fixed_link(). I think this could be a much better solution to this problem, quite simply because we then don't end up with phylink_set_max_fixed_link() which could be abused - and this keeps the complexity where it should be, in the DSA code. As I say, though, I think we need Andrew's input on this. Andrew? I'll look at turning this into a proper solution tomorrow if Andrew is okay with the fwnode change.
On Thu, Jul 07, 2022 at 09:23:46PM +0100, Russell King (Oracle) wrote: > > > I'm not sure how practical that is when we have both DT and ACPI to deal > > > with, and ACPI is certainly out of my knowledge area to be able to > > > construct a software node to specify a fixed-link. Maybe it can be done > > > at the fwnode layer? I don't know. > > > > I don't want to be misunderstood. I brought up software nodes because > > I'm sure you must have thought about this too, before proposing what you > > did here. And unless there's a technical reason against software nodes > > (which there doesn't appear to be, but I don't want to get ahead of > > myself), I figured you must be OK with phylink absorbing the logic, case > > in which I just don't understand why you are pushing back on a proposal > > how to make phylink absorb the logic completely. > > The reason I hadn't is because switching DSA to fwnode brings with it > issues for ACPI, and Andrew wants to be very careful about ACPI in > networking - and I think quite rightly. As soon as one switches from > DT APIs to fwnode APIs, you basically permit people an easy path to > re-use DT properties in ACPI-land without the ACPI issues being first > considered. > Yep - there's at least one other property we need to carry over from the > DT node, which is the "ethernet" property. I've cropped only these segments because there is something I apparently need to highlight. What my patch does is _not_ at the device fwnode level (in fact, DSA ports do not have a struct device, only the switch does), but indeed at the most crude fwnode_handle level. And the fwnode_handles I'm creating using the software_node API are not connected in any way with the device. I'm not replacing the device's fwnodes with prosthetic ones. The fact that I wrote "dn->fwnode.secondary = new_port_fwnode;" is just a cheap hack to minimize the patch delta so I wouldn't have to transport the software fwnode_handle from one place to another within dsa_port_link_register_of(). This should definitely dissapear in the final solution. In fact, as long as phylink doesn't keep a reference on the fwnode after phylink_create() or phylink_fwnode_phy_connect(), I'm pretty sure that the software node can even be deallocated during the probing stage, no need to keep it for the entire lifetime of the device. Therefore, no, we don't need the "ethernet" phandle in the software node we create, because we just use that to pass it to phylink. We still keep our original OF node for the rest of the activities. We don't even need the "reg" u32 property, I just added that for no reason (I wasn't completely sure what the API offers, then I didn't remove it). So the concern that this software node can be abused for a transition to ACPI is quite overestimated. Nothing in DSA is "switched to fwnode" per se, and the creation of a fwnode is just part of "speaking the software node language", which phylink already happily understands and so, needs no change. Just my 2 cents.
Hi, On Thu, Jul 07, 2022 at 10:37:53PM +0300, Vladimir Oltean wrote: > +static int dsa_port_fixup_broken_dt(struct dsa_port *dp) As I mentioned, I doubt that Andrew considers this "broken DT" as he's been promoting this as a standard DSA feature. > +{ > + struct property_entry fixed_link_props[] = { > + PROPERTY_ENTRY_BOOL("full-duplex"), > + PROPERTY_ENTRY_U32("speed", 1000), /* TODO determine actual speed */ > + {}, > + }; > + struct property_entry port_props[3] = {}; > + struct fwnode_handle *fixed_link_fwnode; > + struct fwnode_handle *new_port_fwnode; > + struct device_node *dn = dp->dn; > + phy_interface_t mode; > + int err; > + > + if (of_parse_phandle(dn, "phy-handle", 0) || > + of_phy_is_fixed_link(dn)) > + /* Nothing broken, nothing to fix. > + * TODO: As discussed with Russell, maybe phylink could provide > + * a more comprehensive helper to determine what constitutes a > + * valid fwnode binding than this guerilla kludge. > + */ > + return 0; I think this is sufficient. Yes, phylink accepts "phy" and "phy-device" because it has to for compatibility with other drivers, but the binding document for DSA quite clearly states that "phy-handle" is what DSA accepts, so DT in the kernel will be validated against the yaml file and enforce correctness here. We do need to check for "sfp" being present as well. > + > + err = of_get_phy_mode(dn, &mode); > + if (err) > + /* TODO this may be missing too, ask the driver for the > + * max-speed interface mode for this port > + */ > + mode = PHY_INTERFACE_MODE_NA; I think it would be easier to omit the phy-mode property in the swnode if it isn't present in DT, because then we can handle that in dsa_port_phylink_create() as I've done in my patch series via the ds->ops->phylink_get_caps() method. > + > + port_props[0] = PROPERTY_ENTRY_U32("reg", dp->index); You said in one of your other replies that this node we're constructing is only for phylink, do we need the "reg" property? phylink doesn't care about it.
On Fri, 8 Jul 2022 16:25:03 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > Hi, > > On Thu, Jul 07, 2022 at 10:37:53PM +0300, Vladimir Oltean wrote: > > +static int dsa_port_fixup_broken_dt(struct dsa_port *dp) > > As I mentioned, I doubt that Andrew considers this "broken DT" as he's > been promoting this as a standard DSA feature. > > > +{ > > + struct property_entry fixed_link_props[] = { > > + PROPERTY_ENTRY_BOOL("full-duplex"), > > + PROPERTY_ENTRY_U32("speed", 1000), /* TODO determine actual speed */ > > + {}, > > + }; > > + struct property_entry port_props[3] = {}; > > + struct fwnode_handle *fixed_link_fwnode; > > + struct fwnode_handle *new_port_fwnode; > > + struct device_node *dn = dp->dn; > > + phy_interface_t mode; > > + int err; > > + > > + if (of_parse_phandle(dn, "phy-handle", 0) || > > + of_phy_is_fixed_link(dn)) > > + /* Nothing broken, nothing to fix. > > + * TODO: As discussed with Russell, maybe phylink could provide > > + * a more comprehensive helper to determine what constitutes a > > + * valid fwnode binding than this guerilla kludge. > > + */ > > + return 0; > > I think this is sufficient. Yes, phylink accepts "phy" and "phy-device" > because it has to for compatibility with other drivers, but the binding > document for DSA quite clearly states that "phy-handle" is what DSA > accepts, so DT in the kernel will be validated against the yaml file > and enforce correctness here. > > We do need to check for "sfp" being present as well. > > > + > > + err = of_get_phy_mode(dn, &mode); > > + if (err) > > + /* TODO this may be missing too, ask the driver for the > > + * max-speed interface mode for this port > > + */ > > + mode = PHY_INTERFACE_MODE_NA; > > I think it would be easier to omit the phy-mode property in the swnode > if it isn't present in DT, because then we can handle that in > dsa_port_phylink_create() as I've done in my patch series via the > ds->ops->phylink_get_caps() method. > > > + > > + port_props[0] = PROPERTY_ENTRY_U32("reg", dp->index); > > You said in one of your other replies that this node we're constructing > is only for phylink, do we need the "reg" property? phylink doesn't care > about it. We don't. Vladimir wrote: "We don't even need the "reg" u32 property, I just added that for no reason (I wasn't completely sure what the API offers, then I didn't remove it)." Marek
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 877407bc09de..7fd89239a7a7 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3315,9 +3315,8 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) { struct device_node *phy_handle = NULL; struct dsa_switch *ds = chip->ds; - phy_interface_t mode; struct dsa_port *dp; - int tx_amp, speed; + int tx_amp; int err; u16 reg; @@ -3326,40 +3325,10 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) dp = dsa_to_port(ds, port); - /* MAC Forcing register: don't force link, speed, duplex or flow control - * state to any particular values on physical ports, but force the CPU - * port and all DSA ports to their maximum bandwidth and full duplex. - */ - if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) { - unsigned long caps = dp->pl_config.mac_capabilities; - - if (chip->info->ops->port_max_speed_mode) - mode = chip->info->ops->port_max_speed_mode(port); - else - mode = PHY_INTERFACE_MODE_NA; - - if (caps & MAC_10000FD) - speed = SPEED_10000; - else if (caps & MAC_5000FD) - speed = SPEED_5000; - else if (caps & MAC_2500FD) - speed = SPEED_2500; - else if (caps & MAC_1000) - speed = SPEED_1000; - else if (caps & MAC_100) - speed = SPEED_100; - else - speed = SPEED_10; - - err = mv88e6xxx_port_setup_mac(chip, port, LINK_FORCED_UP, - speed, DUPLEX_FULL, - PAUSE_OFF, mode); - } else { - err = mv88e6xxx_port_setup_mac(chip, port, LINK_UNFORCED, - SPEED_UNFORCED, DUPLEX_UNFORCED, - PAUSE_ON, - PHY_INTERFACE_MODE_NA); - } + err = mv88e6xxx_port_setup_mac(chip, port, LINK_UNFORCED, + SPEED_UNFORCED, DUPLEX_UNFORCED, + PAUSE_ON, + PHY_INTERFACE_MODE_NA); if (err) return err; @@ -4307,7 +4276,6 @@ static const struct mv88e6xxx_ops mv88e6141_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6341_port_set_speed_duplex, - .port_max_speed_mode = mv88e6341_port_max_speed_mode, .port_tag_remap = mv88e6095_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -4700,7 +4668,6 @@ static const struct mv88e6xxx_ops mv88e6190_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6390_port_set_speed_duplex, - .port_max_speed_mode = mv88e6390_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -4763,7 +4730,6 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6390x_port_set_speed_duplex, - .port_max_speed_mode = mv88e6390x_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -4826,7 +4792,6 @@ static const struct mv88e6xxx_ops mv88e6191_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6390_port_set_speed_duplex, - .port_max_speed_mode = mv88e6390_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_frame_mode = mv88e6351_port_set_frame_mode, .port_set_ucast_flood = mv88e6352_port_set_ucast_flood, @@ -4991,7 +4956,6 @@ static const struct mv88e6xxx_ops mv88e6290_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6390_port_set_speed_duplex, - .port_max_speed_mode = mv88e6390_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -5142,7 +5106,6 @@ static const struct mv88e6xxx_ops mv88e6341_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6341_port_set_speed_duplex, - .port_max_speed_mode = mv88e6341_port_max_speed_mode, .port_tag_remap = mv88e6095_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -5365,7 +5328,6 @@ static const struct mv88e6xxx_ops mv88e6390_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6390_port_set_speed_duplex, - .port_max_speed_mode = mv88e6390_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -5432,7 +5394,6 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6390x_port_set_speed_duplex, - .port_max_speed_mode = mv88e6390x_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_policy = mv88e6352_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, @@ -5498,7 +5459,6 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = { .port_sync_link = mv88e6xxx_port_sync_link, .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay, .port_set_speed_duplex = mv88e6393x_port_set_speed_duplex, - .port_max_speed_mode = mv88e6393x_port_max_speed_mode, .port_tag_remap = mv88e6390_port_tag_remap, .port_set_policy = mv88e6393x_port_set_policy, .port_set_frame_mode = mv88e6351_port_set_frame_mode, diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 4518c17c1b9b..a3b7cfe3eb23 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -502,9 +502,6 @@ struct mv88e6xxx_ops { int (*port_set_speed_duplex)(struct mv88e6xxx_chip *chip, int port, int speed, int duplex); - /* What interface mode should be used for maximum speed? */ - phy_interface_t (*port_max_speed_mode)(int port); - int (*port_tag_remap)(struct mv88e6xxx_chip *chip, int port); int (*port_set_policy)(struct mv88e6xxx_chip *chip, int port, diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index 90c55f23b7c9..47e21f3c437a 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -333,14 +333,6 @@ int mv88e6341_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, duplex); } -phy_interface_t mv88e6341_port_max_speed_mode(int port) -{ - if (port == 5) - return PHY_INTERFACE_MODE_2500BASEX; - - return PHY_INTERFACE_MODE_NA; -} - /* Support 10, 100, 200, 1000 Mbps (e.g. 88E6352 family) */ int mv88e6352_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, int speed, int duplex) @@ -372,14 +364,6 @@ int mv88e6390_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, duplex); } -phy_interface_t mv88e6390_port_max_speed_mode(int port) -{ - if (port == 9 || port == 10) - return PHY_INTERFACE_MODE_2500BASEX; - - return PHY_INTERFACE_MODE_NA; -} - /* Support 10, 100, 200, 1000, 2500, 10000 Mbps (e.g. 88E6190X) */ int mv88e6390x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, int speed, int duplex) @@ -394,14 +378,6 @@ int mv88e6390x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, duplex); } -phy_interface_t mv88e6390x_port_max_speed_mode(int port) -{ - if (port == 9 || port == 10) - return PHY_INTERFACE_MODE_XAUI; - - return PHY_INTERFACE_MODE_NA; -} - /* Support 10, 100, 200, 1000, 2500, 5000, 10000 Mbps (e.g. 88E6393X) * Function mv88e6xxx_port_set_speed_duplex() can't be used as the register * values for speeds 2500 & 5000 conflict. @@ -491,14 +467,6 @@ int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, return 0; } -phy_interface_t mv88e6393x_port_max_speed_mode(int port) -{ - if (port == 0 || port == 9 || port == 10) - return PHY_INTERFACE_MODE_10GBASER; - - return PHY_INTERFACE_MODE_NA; -} - static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, phy_interface_t mode, bool force) { diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index cb04243f37c1..2a5741a44e97 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -357,11 +357,6 @@ int mv88e6390x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, int speed, int duplex); -phy_interface_t mv88e6341_port_max_speed_mode(int port); -phy_interface_t mv88e6390_port_max_speed_mode(int port); -phy_interface_t mv88e6390x_port_max_speed_mode(int port); -phy_interface_t mv88e6393x_port_max_speed_mode(int port); - int mv88e6xxx_port_set_state(struct mv88e6xxx_chip *chip, int port, u8 state); int mv88e6xxx_port_set_vlan_map(struct mv88e6xxx_chip *chip, int port, u16 map); diff --git a/net/dsa/port.c b/net/dsa/port.c index 35b4e1f8dc05..34487e62eb03 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; phy_interface_t mode, def_mode; + struct device_node *phy_np; int err; /* Presence of phylink_mac_link_state or phylink_mac_an_restart is @@ -1559,6 +1560,13 @@ int dsa_port_phylink_create(struct dsa_port *dp) return PTR_ERR(dp->pl); } + if (dp->type == DSA_PORT_TYPE_CPU || dp->type == DSA_PORT_TYPE_DSA) { + phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); + of_node_put(phy_np); + if (!phy_np) + err = phylink_set_max_fixed_link(dp->pl); + } + return 0; } @@ -1663,20 +1671,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp) int dsa_port_link_register_of(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; - struct device_node *phy_np; int port = dp->index; if (!ds->ops->adjust_link) { - phy_np = of_parse_phandle(dp->dn, "phy-handle", 0); - if (of_phy_is_fixed_link(dp->dn) || phy_np) { - if (ds->ops->phylink_mac_link_down) - ds->ops->phylink_mac_link_down(ds, port, - MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); - of_node_put(phy_np); - return dsa_port_phylink_register(dp); - } - of_node_put(phy_np); - return 0; + if (ds->ops->phylink_mac_link_down) + ds->ops->phylink_mac_link_down(ds, port, + MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); + + return dsa_port_phylink_register(dp); } dev_warn(ds->dev,
Currently, we only use phylink for CPU and DSA ports if there is a fixed-link specification, or a PHY specified. The reason for this behaviour is that when neither is specified, there was no way for phylink to know the link parameters. Now that we have phylink_set_max_link_speed() (which has become possible through the addition of mac_capabilities) we now have the ability to know the maximum link speed for a specific link, and can use phylink for this case as well. However, we need all DSA drivers to provide mac_capabilities for this to work, and either report the default interface to be used for a port or have filled in supported_interfaces, so that we can select a maximum speed appropriate for the interface mode that hardware may have configured for the port. Any drivers that do not meet these requirements are likely to break. This is especially important with the conversion of DSA drivers to phylink_pcs, as the PCS code only gets called if we are using phylink for the port. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/mv88e6xxx/chip.c | 50 ++++---------------------------- drivers/net/dsa/mv88e6xxx/chip.h | 3 -- drivers/net/dsa/mv88e6xxx/port.c | 32 -------------------- drivers/net/dsa/mv88e6xxx/port.h | 5 ---- net/dsa/port.c | 24 ++++++++------- 5 files changed, 18 insertions(+), 96 deletions(-)