| Message ID | E1oCNlE-006e3z-3T@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: always use phylink | expand |
On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote: > Create and use a swnode fixed-link specification for phylink if no > parameters are given in DT for a fixed-link. This allows phylink to > be used for "default" cases for DSA and CPU ports. Enable the use > of phylink in all cases for DSA and CPU ports. > Co-developed by Vladimir Oltean and myself. Why not to use Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com> ? > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Marek Behún <kabel@kernel.org> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> ... > +static struct { > + unsigned long mask; > + int speed; > + int duplex; > +} phylink_caps_params[] = { > + { MAC_400000FD, SPEED_400000, DUPLEX_FULL }, > + { MAC_200000FD, SPEED_200000, DUPLEX_FULL }, > + { MAC_100000FD, SPEED_100000, DUPLEX_FULL }, > + { MAC_56000FD, SPEED_56000, DUPLEX_FULL }, > + { MAC_50000FD, SPEED_50000, DUPLEX_FULL }, > + { MAC_40000FD, SPEED_40000, DUPLEX_FULL }, > + { MAC_25000FD, SPEED_25000, DUPLEX_FULL }, > + { MAC_20000FD, SPEED_20000, DUPLEX_FULL }, > + { MAC_10000FD, SPEED_10000, DUPLEX_FULL }, > + { MAC_5000FD, SPEED_5000, DUPLEX_FULL }, > + { MAC_2500FD, SPEED_2500, DUPLEX_FULL }, > + { MAC_1000FD, SPEED_1000, DUPLEX_FULL }, > + { MAC_100FD, SPEED_100, DUPLEX_FULL }, > + { MAC_10FD, SPEED_10, DUPLEX_FULL }, > + { MAC_1000HD, SPEED_1000, DUPLEX_HALF }, > + { MAC_100HD, SPEED_100, DUPLEX_HALF }, > + { MAC_10HD, SPEED_10, DUPLEX_HALF }, > +}; > + > +static int dsa_port_find_max_speed(unsigned long caps, int *speed, int *duplex) > +{ > + int i; > + > + *speed = SPEED_UNKNOWN; > + *duplex = DUPLEX_UNKNOWN; > + > + for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) { > + if (caps & phylink_caps_params[i].mask) { > + *speed = phylink_caps_params[i].speed; > + *duplex = phylink_caps_params[i].duplex; > + break; With the below check it's way too protective programming. return 0; > + } > + } > + > + return *speed == SPEED_UNKNOWN ? -EINVAL : 0; return -EINVAL; > +} ... > +static struct fwnode_handle *dsa_port_get_fwnode(struct dsa_port *dp, > + phy_interface_t mode) > +{ > + struct property_entry fixed_link_props[3] = { }; > + struct property_entry port_props[3] = {}; A bit of consistency in the assignments? Also it seems you are using up to 2 for the first one and only 1 in the second one. IIUC it requires a terminator entry, so it means 3 and 2. Do we really need 3 in the second case? > + struct fwnode_handle *fixed_link_fwnode; > + struct fwnode_handle *new_port_fwnode; > + struct device_node *dn = dp->dn; > + struct device_node *phy_node; > + int err, speed, duplex; > + unsigned long caps; > + > + phy_node = of_parse_phandle(dn, "phy-handle", 0); fwnode in the name, why not to use fwnode APIs? fwnode_find_reference(); > + of_node_put(phy_node); > + if (phy_node || 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 of_fwnode_handle(dn); > + > + if (mode == PHY_INTERFACE_MODE_NA) > + dsa_port_find_max_caps(dp, &mode, &caps); > + else > + caps = dp->pl_config.mac_capabilities & > + phylink_interface_to_caps(mode); > + > + err = dsa_port_find_max_speed(caps, &speed, &duplex); > + if (err) > + return ERR_PTR(err); > + > + fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed); > + if (duplex == DUPLEX_FULL) > + fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex"); > + > + port_props[0] = 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 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 fixed_link_fwnode; > + } > + > + return new_port_fwnode; > +}
On Fri, Jul 15, 2022 at 11:11:18PM +0300, Andy Shevchenko wrote: > On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote: > > Create and use a swnode fixed-link specification for phylink if no > > parameters are given in DT for a fixed-link. This allows phylink to > > be used for "default" cases for DSA and CPU ports. Enable the use > > of phylink in all cases for DSA and CPU ports. > > > Co-developed by Vladimir Oltean and myself. > > Why not to use > > Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Ah, that's an official thing. Thanks. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Reviewed-by: Marek Behún <kabel@kernel.org> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > ... > > > +static struct { > > + unsigned long mask; > > + int speed; > > + int duplex; > > +} phylink_caps_params[] = { > > + { MAC_400000FD, SPEED_400000, DUPLEX_FULL }, > > + { MAC_200000FD, SPEED_200000, DUPLEX_FULL }, > > + { MAC_100000FD, SPEED_100000, DUPLEX_FULL }, > > + { MAC_56000FD, SPEED_56000, DUPLEX_FULL }, > > + { MAC_50000FD, SPEED_50000, DUPLEX_FULL }, > > + { MAC_40000FD, SPEED_40000, DUPLEX_FULL }, > > + { MAC_25000FD, SPEED_25000, DUPLEX_FULL }, > > + { MAC_20000FD, SPEED_20000, DUPLEX_FULL }, > > + { MAC_10000FD, SPEED_10000, DUPLEX_FULL }, > > + { MAC_5000FD, SPEED_5000, DUPLEX_FULL }, > > + { MAC_2500FD, SPEED_2500, DUPLEX_FULL }, > > + { MAC_1000FD, SPEED_1000, DUPLEX_FULL }, > > + { MAC_100FD, SPEED_100, DUPLEX_FULL }, > > + { MAC_10FD, SPEED_10, DUPLEX_FULL }, > > + { MAC_1000HD, SPEED_1000, DUPLEX_HALF }, > > + { MAC_100HD, SPEED_100, DUPLEX_HALF }, > > + { MAC_10HD, SPEED_10, DUPLEX_HALF }, > > +}; > > + > > +static int dsa_port_find_max_speed(unsigned long caps, int *speed, int *duplex) > > +{ > > + int i; > > + > > + *speed = SPEED_UNKNOWN; > > + *duplex = DUPLEX_UNKNOWN; > > + > > + for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) { > > + if (caps & phylink_caps_params[i].mask) { > > + *speed = phylink_caps_params[i].speed; > > + *duplex = phylink_caps_params[i].duplex; > > > + break; > > With the below check it's way too protective programming. > > return 0; > > > + } > > + } > > + > > + return *speed == SPEED_UNKNOWN ? -EINVAL : 0; > > return -EINVAL; Ok. > > +static struct fwnode_handle *dsa_port_get_fwnode(struct dsa_port *dp, > > + phy_interface_t mode) > > +{ > > > + struct property_entry fixed_link_props[3] = { }; > > + struct property_entry port_props[3] = {}; > > A bit of consistency in the assignments? > > Also it seems you are using up to 2 for the first one and only 1 in the second > one. IIUC it requires a terminator entry, so it means 3 and 2. Do we really > need 3 in the second case? Probably not - that came from Vladimir's patch, and I removed the "reg" property without fixing this up. Thanks for spotting. > > + struct fwnode_handle *fixed_link_fwnode; > > + struct fwnode_handle *new_port_fwnode; > > + struct device_node *dn = dp->dn; > > + struct device_node *phy_node; > > + int err, speed, duplex; > > + unsigned long caps; > > + > > + phy_node = of_parse_phandle(dn, "phy-handle", 0); > > fwnode in the name, why not to use fwnode APIs? > > fwnode_find_reference(); Marcin has a series converting DSA to use fwnode things - currently DSA does not support ACPI, so converting it to fwnode doesn't make that much sese until the proper ACPI patches get merged, which have now been rebased on this series by Marcin in the expectation that these patches would be merged... so I don't want to tred on Marcin's feet on that.
On Fri, Jul 15, 2022 at 10:36:29PM +0100, Russell King (Oracle) wrote: > On Fri, Jul 15, 2022 at 11:11:18PM +0300, Andy Shevchenko wrote: > > On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote: ... > > > Co-developed by Vladimir Oltean and myself. > > > > Why not to use > > > > Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Ah, that's an official thing. Thanks. Yep, it's even documented in Submitting Patches. ... > > > + phy_node = of_parse_phandle(dn, "phy-handle", 0); > > > > fwnode in the name, why not to use fwnode APIs? > > > > fwnode_find_reference(); > > Marcin has a series converting DSA to use fwnode things - currently DSA > does not support ACPI, so converting it to fwnode doesn't make that much > sese until the proper ACPI patches get merged, which have now been > rebased on this series by Marcin in the expectation that these patches > would be merged... so I don't want to tred on Marcin's feet on that. But it's normal development process... Anyway, it seems to me that you are using fwnode out of that (with the exception of one call). To me it looks that you add a work to him, rather than making his life easier, since you know ahead that this is going to be converted.
On Mon, Jul 18, 2022 at 09:59:25PM +0300, Andy Shevchenko wrote: > On Fri, Jul 15, 2022 at 10:36:29PM +0100, Russell King (Oracle) wrote: > > On Fri, Jul 15, 2022 at 11:11:18PM +0300, Andy Shevchenko wrote: > > > On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote: > > ... > > > > > Co-developed by Vladimir Oltean and myself. > > > > > > Why not to use > > > > > > Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Ah, that's an official thing. Thanks. > > Yep, it's even documented in Submitting Patches. > > ... > > > > > + phy_node = of_parse_phandle(dn, "phy-handle", 0); > > > > > > fwnode in the name, why not to use fwnode APIs? > > > > > > fwnode_find_reference(); > > > > Marcin has a series converting DSA to use fwnode things - currently DSA > > does not support ACPI, so converting it to fwnode doesn't make that much > > sese until the proper ACPI patches get merged, which have now been > > rebased on this series by Marcin in the expectation that these patches > > would be merged... so I don't want to tred on Marcin's feet on that. > > But it's normal development process... > > Anyway, it seems to me that you are using fwnode out of that (with the > exception of one call). To me it looks that you add a work to him, rather > than making his life easier, since you know ahead that this is going to be > converted. No, I didn't know ahead of time until Marcin piped up about it, and then we discussed how to resolve the conflict between the two patch sets.
On Mon, Jul 18, 2022 at 08:13:19PM +0100, Russell King (Oracle) wrote: > On Mon, Jul 18, 2022 at 09:59:25PM +0300, Andy Shevchenko wrote: > > On Fri, Jul 15, 2022 at 10:36:29PM +0100, Russell King (Oracle) wrote: > > > On Fri, Jul 15, 2022 at 11:11:18PM +0300, Andy Shevchenko wrote: > > > > On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote: > > > > ... > > > > > > > Co-developed by Vladimir Oltean and myself. > > > > > > > > Why not to use > > > > > > > > Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > > > Ah, that's an official thing. Thanks. > > > > Yep, it's even documented in Submitting Patches. > > > > ... > > > > > > > + phy_node = of_parse_phandle(dn, "phy-handle", 0); > > > > > > > > fwnode in the name, why not to use fwnode APIs? > > > > > > > > fwnode_find_reference(); > > > > > > Marcin has a series converting DSA to use fwnode things - currently DSA > > > does not support ACPI, so converting it to fwnode doesn't make that much > > > sese until the proper ACPI patches get merged, which have now been > > > rebased on this series by Marcin in the expectation that these patches > > > would be merged... so I don't want to tred on Marcin's feet on that. > > > > But it's normal development process... > > > > Anyway, it seems to me that you are using fwnode out of that (with the > > exception of one call). To me it looks that you add a work to him, rather > > than making his life easier, since you know ahead that this is going to be > > converted. > > No, I didn't know ahead of time until Marcin piped up about it, and > then we discussed how to resolve the conflict between the two patch > sets. But now you know that and since your series is not yet in, back to phase 1, i.e. "normal development process (with additional coordination required)".
diff --git a/net/dsa/port.c b/net/dsa/port.c index 35b4e1f8dc05..abcf7899abf8 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1521,10 +1521,131 @@ static const struct phylink_mac_ops dsa_port_phylink_mac_ops = { .mac_link_up = dsa_port_phylink_mac_link_up, }; +static struct { + unsigned long mask; + int speed; + int duplex; +} phylink_caps_params[] = { + { MAC_400000FD, SPEED_400000, DUPLEX_FULL }, + { MAC_200000FD, SPEED_200000, DUPLEX_FULL }, + { MAC_100000FD, SPEED_100000, DUPLEX_FULL }, + { MAC_56000FD, SPEED_56000, DUPLEX_FULL }, + { MAC_50000FD, SPEED_50000, DUPLEX_FULL }, + { MAC_40000FD, SPEED_40000, DUPLEX_FULL }, + { MAC_25000FD, SPEED_25000, DUPLEX_FULL }, + { MAC_20000FD, SPEED_20000, DUPLEX_FULL }, + { MAC_10000FD, SPEED_10000, DUPLEX_FULL }, + { MAC_5000FD, SPEED_5000, DUPLEX_FULL }, + { MAC_2500FD, SPEED_2500, DUPLEX_FULL }, + { MAC_1000FD, SPEED_1000, DUPLEX_FULL }, + { MAC_100FD, SPEED_100, DUPLEX_FULL }, + { MAC_10FD, SPEED_10, DUPLEX_FULL }, + { MAC_1000HD, SPEED_1000, DUPLEX_HALF }, + { MAC_100HD, SPEED_100, DUPLEX_HALF }, + { MAC_10HD, SPEED_10, DUPLEX_HALF }, +}; + +static int dsa_port_find_max_speed(unsigned long caps, int *speed, int *duplex) +{ + int i; + + *speed = SPEED_UNKNOWN; + *duplex = DUPLEX_UNKNOWN; + + for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) { + if (caps & phylink_caps_params[i].mask) { + *speed = phylink_caps_params[i].speed; + *duplex = phylink_caps_params[i].duplex; + break; + } + } + + return *speed == SPEED_UNKNOWN ? -EINVAL : 0; +} + +static void dsa_port_find_max_caps(struct dsa_port *dp, + phy_interface_t *max_interface, + unsigned long *max_caps) +{ + struct phylink_config *config = &dp->pl_config; + phy_interface_t interface; + unsigned long caps; + + *max_interface = PHY_INTERFACE_MODE_NA; + *max_caps = 0; + + for_each_set_bit(interface, config->supported_interfaces, + PHY_INTERFACE_MODE_MAX) { + caps = config->mac_capabilities & + phylink_interface_to_caps(interface); + if (caps > *max_caps) { + *max_caps = caps; + *max_interface = interface; + } + } +} + +static struct fwnode_handle *dsa_port_get_fwnode(struct dsa_port *dp, + phy_interface_t mode) +{ + struct property_entry fixed_link_props[3] = { }; + struct property_entry port_props[3] = {}; + struct fwnode_handle *fixed_link_fwnode; + struct fwnode_handle *new_port_fwnode; + struct device_node *dn = dp->dn; + struct device_node *phy_node; + int err, speed, duplex; + unsigned long caps; + + phy_node = of_parse_phandle(dn, "phy-handle", 0); + of_node_put(phy_node); + if (phy_node || 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 of_fwnode_handle(dn); + + if (mode == PHY_INTERFACE_MODE_NA) + dsa_port_find_max_caps(dp, &mode, &caps); + else + caps = dp->pl_config.mac_capabilities & + phylink_interface_to_caps(mode); + + err = dsa_port_find_max_speed(caps, &speed, &duplex); + if (err) + return ERR_PTR(err); + + fixed_link_props[0] = PROPERTY_ENTRY_U32("speed", speed); + if (duplex == DUPLEX_FULL) + fixed_link_props[1] = PROPERTY_ENTRY_BOOL("full-duplex"); + + port_props[0] = 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 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 fixed_link_fwnode; + } + + return new_port_fwnode; +} + int dsa_port_phylink_create(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; phy_interface_t mode, def_mode; + struct fwnode_handle *fwnode; int err; /* Presence of phylink_mac_link_state or phylink_mac_an_restart is @@ -1552,8 +1673,19 @@ int dsa_port_phylink_create(struct dsa_port *dp) mode = PHY_INTERFACE_MODE_NA; } - dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), - mode, &dsa_port_phylink_mac_ops); + fwnode = dsa_port_get_fwnode(dp, mode); + if (IS_ERR(fwnode)) { + dev_err(ds->dev, + "Failed to get fwnode for port %d: %pe\n", + dp->index, fwnode); + return PTR_ERR(fwnode); + } + + dp->pl = phylink_create(&dp->pl_config, fwnode, mode, + &dsa_port_phylink_mac_ops); + + fwnode_remove_software_node(fwnode); + if (IS_ERR(dp->pl)) { pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl)); return PTR_ERR(dp->pl); @@ -1663,20 +1795,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,