| Message ID | 20220508185313.2222956-16-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for VSC7512 control over SPI | expand |
On Sun, May 08, 2022 at 11:53:12AM -0700, Colin Foster wrote: > Add the ability for felix users to announce their capabilities to DSA > switches by way of phylink_get_caps. This will allow those users the > ability to use phylink_generic_validate, which otherwise wouldn't be > possible. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > drivers/net/dsa/ocelot/felix.c | 22 +++++++++++++++------- > drivers/net/dsa/ocelot/felix.h | 2 ++ > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > index d09408baaab7..32ed093f47c6 100644 > --- a/drivers/net/dsa/ocelot/felix.c > +++ b/drivers/net/dsa/ocelot/felix.c > @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > struct phylink_config *config) > { > struct ocelot *ocelot = ds->priv; > + struct felix *felix; > > - /* This driver does not make use of the speed, duplex, pause or the > - * advertisement in its mac_config, so it is safe to mark this driver > - * as non-legacy. > - */ > - config->legacy_pre_march2020 = false; > + felix = ocelot_to_felix(ocelot); > + > + if (felix->info->phylink_get_caps) { > + felix->info->phylink_get_caps(ocelot, port, config); > + } else { > > - __set_bit(ocelot->ports[port]->phy_mode, > - config->supported_interfaces); > + /* This driver does not make use of the speed, duplex, pause or > + * the advertisement in its mac_config, so it is safe to mark > + * this driver as non-legacy. > + */ > + config->legacy_pre_march2020 = false; I don't think you mean to set legacy_pre_march2020 to true only felix->info->phylink_get_caps is absent, do you? Also, I'm thinking maybe we could provide an implementation of this function for all switches, not just for vsc7512. > + > + __set_bit(ocelot->ports[port]->phy_mode, > + config->supported_interfaces); > + } > } > > static void felix_phylink_validate(struct dsa_switch *ds, int port, > diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h > index 3ecac79bbf09..33281370f415 100644 > --- a/drivers/net/dsa/ocelot/felix.h > +++ b/drivers/net/dsa/ocelot/felix.h > @@ -57,6 +57,8 @@ struct felix_info { > u32 speed); > struct regmap *(*init_regmap)(struct ocelot *ocelot, > struct resource *res); > + void (*phylink_get_caps)(struct ocelot *ocelot, int port, > + struct phylink_config *pl_config); > }; > > extern const struct dsa_switch_ops felix_switch_ops; > -- > 2.25.1 >
On Mon, May 09, 2022 at 05:23:32PM -0700, Colin Foster wrote: > > > @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > > > struct phylink_config *config) > > > { > > > struct ocelot *ocelot = ds->priv; > > > + struct felix *felix; > > > > > > - /* This driver does not make use of the speed, duplex, pause or the > > > - * advertisement in its mac_config, so it is safe to mark this driver > > > - * as non-legacy. > > > - */ > > > - config->legacy_pre_march2020 = false; > > > + felix = ocelot_to_felix(ocelot); > > > + > > > + if (felix->info->phylink_get_caps) { > > > + felix->info->phylink_get_caps(ocelot, port, config); > > > + } else { > > > > > > - __set_bit(ocelot->ports[port]->phy_mode, > > > - config->supported_interfaces); > > > + /* This driver does not make use of the speed, duplex, pause or > > > + * the advertisement in its mac_config, so it is safe to mark > > > + * this driver as non-legacy. > > > + */ > > > + config->legacy_pre_march2020 = false; > > > > I don't think you mean to set legacy_pre_march2020 to true only > > felix->info->phylink_get_caps is absent, do you? > > > > Also, I'm thinking maybe we could provide an implementation of this > > function for all switches, not just for vsc7512. > > I had assumed these last two patches might spark more discussion, which > is why I kept them separate (specifically the last patch). > > With this, are you simply suggesting to take everything that is > currently in felix_phylink_get_caps and doing it in the felix / seville > implementations? This is because the default condition is no longer the > "only" condition. Sounds easy enough. No, not everything, just the way in which config->supported_interfaces is populated. We have different PCS implementations, so it's likely that the procedures to retrieve the valid SERDES protocols (when changing them will be supported) are different. But in fact I seriously doubt that the current way in which supported_interfaces gets populated is limiting you from doing anything right now, precisely because you don't have any code that supports changing the phy-mode. Also, it's unlikely (I'd say impossible) for one driver to be unconverted to the post-March-2020 requirements and the other not to be. The simple reason is that they share the same mac_config implementation. So it's perfectly ok to keep "config->legacy_pre_march2020 = false" right where it is. So I'd say it's up to you, but I'd drop this patch right now.
On Mon, May 09, 2022 at 05:55:37PM -0700, Colin Foster wrote: > Hmm... So the main reason I needed get_caps was because > phylink_generic_validate looks at mac_capabilities. I know I'll have to > deal with supported_interfaces once I finally get the other four ports > working, but for now the main purpose of this patch is to allow me to > populate the mac_capabilities entry for phylink_generic_validate. > > Aside from ensuring legacy_pre_march2020 = false, I feel like the rest > of the patch makes sense. But still. Just populate mac_capabilities for everybody in the common felix_phylink_get_caps(), and use the generic phylink validation only for your driver.
On Mon, May 09, 2022 at 10:34:45AM +0000, Vladimir Oltean wrote: > On Sun, May 08, 2022 at 11:53:12AM -0700, Colin Foster wrote: > > Add the ability for felix users to announce their capabilities to DSA > > switches by way of phylink_get_caps. This will allow those users the > > ability to use phylink_generic_validate, which otherwise wouldn't be > > possible. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > drivers/net/dsa/ocelot/felix.c | 22 +++++++++++++++------- > > drivers/net/dsa/ocelot/felix.h | 2 ++ > > 2 files changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > > index d09408baaab7..32ed093f47c6 100644 > > --- a/drivers/net/dsa/ocelot/felix.c > > +++ b/drivers/net/dsa/ocelot/felix.c > > @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > > struct phylink_config *config) > > { > > struct ocelot *ocelot = ds->priv; > > + struct felix *felix; > > > > - /* This driver does not make use of the speed, duplex, pause or the > > - * advertisement in its mac_config, so it is safe to mark this driver > > - * as non-legacy. > > - */ > > - config->legacy_pre_march2020 = false; > > + felix = ocelot_to_felix(ocelot); > > + > > + if (felix->info->phylink_get_caps) { > > + felix->info->phylink_get_caps(ocelot, port, config); > > + } else { > > > > - __set_bit(ocelot->ports[port]->phy_mode, > > - config->supported_interfaces); > > + /* This driver does not make use of the speed, duplex, pause or > > + * the advertisement in its mac_config, so it is safe to mark > > + * this driver as non-legacy. > > + */ > > + config->legacy_pre_march2020 = false; > > I don't think you mean to set legacy_pre_march2020 to true only > felix->info->phylink_get_caps is absent, do you? > > Also, I'm thinking maybe we could provide an implementation of this > function for all switches, not just for vsc7512. I had assumed these last two patches might spark more discussion, which is why I kept them separate (specifically the last patch). With this, are you simply suggesting to take everything that is currently in felix_phylink_get_caps and doing it in the felix / seville implementations? This is because the default condition is no longer the "only" condition. Sounds easy enough. > > > + > > + __set_bit(ocelot->ports[port]->phy_mode, > > + config->supported_interfaces); > > + } > > } > > > > static void felix_phylink_validate(struct dsa_switch *ds, int port, > > diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h > > index 3ecac79bbf09..33281370f415 100644 > > --- a/drivers/net/dsa/ocelot/felix.h > > +++ b/drivers/net/dsa/ocelot/felix.h > > @@ -57,6 +57,8 @@ struct felix_info { > > u32 speed); > > struct regmap *(*init_regmap)(struct ocelot *ocelot, > > struct resource *res); > > + void (*phylink_get_caps)(struct ocelot *ocelot, int port, > > + struct phylink_config *pl_config); > > }; > > > > extern const struct dsa_switch_ops felix_switch_ops; > > -- > > 2.25.1 > >
On Mon, May 09, 2022 at 05:30:30PM +0000, Vladimir Oltean wrote: > On Mon, May 09, 2022 at 05:23:32PM -0700, Colin Foster wrote: > > > > @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > > > > struct phylink_config *config) > > > > { > > > > struct ocelot *ocelot = ds->priv; > > > > + struct felix *felix; > > > > > > > > - /* This driver does not make use of the speed, duplex, pause or the > > > > - * advertisement in its mac_config, so it is safe to mark this driver > > > > - * as non-legacy. > > > > - */ > > > > - config->legacy_pre_march2020 = false; > > > > + felix = ocelot_to_felix(ocelot); > > > > + > > > > + if (felix->info->phylink_get_caps) { > > > > + felix->info->phylink_get_caps(ocelot, port, config); > > > > + } else { > > > > > > > > - __set_bit(ocelot->ports[port]->phy_mode, > > > > - config->supported_interfaces); > > > > + /* This driver does not make use of the speed, duplex, pause or > > > > + * the advertisement in its mac_config, so it is safe to mark > > > > + * this driver as non-legacy. > > > > + */ > > > > + config->legacy_pre_march2020 = false; > > > > > > I don't think you mean to set legacy_pre_march2020 to true only > > > felix->info->phylink_get_caps is absent, do you? > > > > > > Also, I'm thinking maybe we could provide an implementation of this > > > function for all switches, not just for vsc7512. > > > > I had assumed these last two patches might spark more discussion, which > > is why I kept them separate (specifically the last patch). > > > > With this, are you simply suggesting to take everything that is > > currently in felix_phylink_get_caps and doing it in the felix / seville > > implementations? This is because the default condition is no longer the > > "only" condition. Sounds easy enough. > > No, not everything, just the way in which config->supported_interfaces > is populated. We have different PCS implementations, so it's likely that > the procedures to retrieve the valid SERDES protocols (when changing > them will be supported) are different. > > But in fact I seriously doubt that the current way in which supported_interfaces > gets populated is limiting you from doing anything right now, precisely > because you don't have any code that supports changing the phy-mode. Hmm... So the main reason I needed get_caps was because phylink_generic_validate looks at mac_capabilities. I know I'll have to deal with supported_interfaces once I finally get the other four ports working, but for now the main purpose of this patch is to allow me to populate the mac_capabilities entry for phylink_generic_validate. Aside from ensuring legacy_pre_march2020 = false, I feel like the rest of the patch makes sense. > > Also, it's unlikely (I'd say impossible) for one driver to be > unconverted to the post-March-2020 requirements and the other not to be. > The simple reason is that they share the same mac_config implementation. > So it's perfectly ok to keep "config->legacy_pre_march2020 = false" > right where it is. > > So I'd say it's up to you, but I'd drop this patch right now.
On Mon, May 09, 2022 at 05:58:07PM +0000, Vladimir Oltean wrote: > On Mon, May 09, 2022 at 05:55:37PM -0700, Colin Foster wrote: > > Hmm... So the main reason I needed get_caps was because > > phylink_generic_validate looks at mac_capabilities. I know I'll have to > > deal with supported_interfaces once I finally get the other four ports > > working, but for now the main purpose of this patch is to allow me to > > populate the mac_capabilities entry for phylink_generic_validate. > > > > Aside from ensuring legacy_pre_march2020 = false, I feel like the rest > > of the patch makes sense. > > But still. Just populate mac_capabilities for everybody in the common > felix_phylink_get_caps(), and use the generic phylink validation only > for your driver. Resurrecting this thread, now that I'm back into the networking stuff. I removed some people from the CC chain who I think were MFD / GPIO specific. v1 of my upcoming "add dsa capabilities to the vsc7512" will build off these suggestions, but these DSA patches were dropped in the MFD set. Restoring context: my oritinal patch was: +++ b/drivers/net/dsa/ocelot/felix.c @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, struct phylink_config *config) { struct ocelot *ocelot = ds->priv; + struct felix *felix; - /* This driver does not make use of the speed, duplex, pause or the - * advertisement in its mac_config, so it is safe to mark this driver - * as non-legacy. - */ - config->legacy_pre_march2020 = false; + felix = ocelot_to_felix(ocelot); + + if (felix->info->phylink_get_caps) { + felix->info->phylink_get_caps(ocelot, port, config); + } else { - __set_bit(ocelot->ports[port]->phy_mode, - config->supported_interfaces); + /* This driver does not make use of the speed, duplex, pause or + * the advertisement in its mac_config, so it is safe to mark + * this driver as non-legacy. + */ + config->legacy_pre_march2020 = false; + + __set_bit(ocelot->ports[port]->phy_mode, + config->supported_interfaces); + } } Regarding felix_phylink_get_caps() - does it make sense that mac_capabilities would be the same for all ports? This is where I've currently landed, and I want to make sure it doesn't have adverse effects on vsc9959 or seville: static void felix_phylink_get_caps(struct dsa_switch *ds, int port, struct phylink_config *config) { struct ocelot *ocelot = ds->priv; struct felix *felix; u32 modes; int i; felix = ocelot_to_felix(ocelot); modes = felix->info->port_modes[port]; /* This driver does not make use of the speed, duplex, pause or * the advertisement in its mac_config, so it is safe to mark * this driver as non-legacy. */ config->legacy_pre_march2020 = false; for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) if (modes & felix_phy_match_table[i]) __set_bit(i, config->supported_interfaces); config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD; } (this might be two patches - one for the match table and one for the mac_capabilities) Seemingly because net/dsa/port.c checks for phylink_validate before it checks for mac_capabilties, it won't make a difference there, but this seems ... wrong? Or maybe it isn't wrong until I implement the QSGMII port that supports 2500FD (as in drivers/net/ethernet/mscc/ocelot_net.c ocelot_port_phylink_create())
On Fri, Sep 09, 2022 at 11:33:59AM -0700, Colin Foster wrote: > Seemingly because net/dsa/port.c checks for phylink_validate before it > checks for mac_capabilties, it won't make a difference there, but this > seems ... wrong? Or maybe it isn't wrong until I implement the QSGMII > port that supports 2500FD (as in drivers/net/ethernet/mscc/ocelot_net.c > ocelot_port_phylink_create()) No, the code in dsa_port_phylink_validate() is exactly what I intend. If there's a phylink_validate() function, then that gets used to cater for Ocelot's rate adaption in the PCS (where the link modes are not limited by the interface mode.) If there isn't a phylink_validate() function, then we require that mac_capabilities() is filled in, and if it is, we use the generic validator - essentially I want to see everyone filling in both the supported interface masks and the MAC capabilities no matter what. The Ocelot rate adaption is something that needs to be tidied up, but until that has been done, Ocelot needs to have the phylink_validate() hook. Ocelot is currently the sole user of this hook. I have some experimental patches to address this, but nothing that I felt happy to send out yet.
On Fri, Sep 09, 2022 at 08:20:03PM +0100, Russell King (Oracle) wrote: > On Fri, Sep 09, 2022 at 11:33:59AM -0700, Colin Foster wrote: > > Seemingly because net/dsa/port.c checks for phylink_validate before it > > checks for mac_capabilties, it won't make a difference there, but this > > seems ... wrong? Or maybe it isn't wrong until I implement the QSGMII > > port that supports 2500FD (as in drivers/net/ethernet/mscc/ocelot_net.c > > ocelot_port_phylink_create()) > > No, the code in dsa_port_phylink_validate() is exactly what I intend. My apologies... I meant "it seems wrong for me to blindly assign values to mac_capabilities" not that there are any issues in the logic of dsa_port_phylink_validate(). My phrasing was misleading. > > If there's a phylink_validate() function, then that gets used to cater > for Ocelot's rate adaption in the PCS (where the link modes are not > limited by the interface mode.) > > If there isn't a phylink_validate() function, then we require that > mac_capabilities() is filled in, and if it is, we use the generic > validator - essentially I want to see everyone filling in both the > supported interface masks and the MAC capabilities no matter what. > > The Ocelot rate adaption is something that needs to be tidied up, but > until that has been done, Ocelot needs to have the phylink_validate() > hook. Ocelot is currently the sole user of this hook. > > I have some experimental patches to address this, but nothing that I > felt happy to send out yet. I think we're on the same page. My version of Ocelot phylink_validate utilizes phylink_generic_validate while other Felix devices don't, as intended. Hopefully I'll be able to tidy up this patch set and send it out for full review soon. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Sep 09, 2022 at 11:33:59AM -0700, Colin Foster wrote: > Regarding felix_phylink_get_caps() - does it make sense that > mac_capabilities would be the same for all ports? This is where I've > currently landed, and I want to make sure it doesn't have adverse > effects on vsc9959 or seville: > > static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > struct phylink_config *config) > { > struct ocelot *ocelot = ds->priv; > struct felix *felix; > u32 modes; > int i; > > felix = ocelot_to_felix(ocelot); > modes = felix->info->port_modes[port]; > > /* This driver does not make use of the speed, duplex, pause or > * the advertisement in its mac_config, so it is safe to mark > * this driver as non-legacy. > */ > config->legacy_pre_march2020 = false; > > for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) > if (modes & felix_phy_match_table[i]) > __set_bit(i, config->supported_interfaces); The current shape of the SERDES driver used with VSC9959 and VSC9953 is such that dynamic changes to the SERDES protocol are not supported. So at least for these 2 switches, please keep setting just the current ocelot->ports[port]->phy_mode (i.e. what was set in the device tree). > > config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 | > MAC_100 | MAC_1000FD; > } > > (this might be two patches - one for the match table and one for the > mac_capabilities) > > Seemingly because net/dsa/port.c checks for phylink_validate before it > checks for mac_capabilties, it won't make a difference there, but this > seems ... wrong? Or maybe it isn't wrong until I implement the QSGMII > port that supports 2500FD (as in drivers/net/ethernet/mscc/ocelot_net.c > ocelot_port_phylink_create()) I don't think there is any QSGMII port that supports 2500FD in VSC7514. In general, the frequency at which QSGMII operates would not be able to support that data rate. That must be an artefact of me copying and pasting code from Felix to Ocelot in commit e6e12df625f2 ("net: mscc: ocelot: convert to phylink"), later transformed by Russell in commit 7258aa5094db ("net: ocelot_net: use phylink_generic_validate()"). How about you do the other way around: populate config->mac_capabilities such that it unconditionally also includes MAC_2500FD. You may have noticed that phylink_generic_validate() calls phylink_get_linkmodes(), which contains a logical AND between the capabilities reported by the MAC, and the plausible capabilities given by state->interface. So it would be quite within the expectations of this API for phylink to exclude MAC_2500FD from mac_capabilities if PHY_INTERFACE_MODE_QSGMII is used. On the other hand, VSC9959 and VSC9953 do support actual 2.5G on internal interfaces (and on USXGMII, in the case of VSC9959). They don't use the generic phylink validation method right now, but it would be good, in case they get mechanically converted or something, to keep reporting MAC_2500FD whatever code you add right now.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index d09408baaab7..32ed093f47c6 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, struct phylink_config *config) { struct ocelot *ocelot = ds->priv; + struct felix *felix; - /* This driver does not make use of the speed, duplex, pause or the - * advertisement in its mac_config, so it is safe to mark this driver - * as non-legacy. - */ - config->legacy_pre_march2020 = false; + felix = ocelot_to_felix(ocelot); + + if (felix->info->phylink_get_caps) { + felix->info->phylink_get_caps(ocelot, port, config); + } else { - __set_bit(ocelot->ports[port]->phy_mode, - config->supported_interfaces); + /* This driver does not make use of the speed, duplex, pause or + * the advertisement in its mac_config, so it is safe to mark + * this driver as non-legacy. + */ + config->legacy_pre_march2020 = false; + + __set_bit(ocelot->ports[port]->phy_mode, + config->supported_interfaces); + } } static void felix_phylink_validate(struct dsa_switch *ds, int port, diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index 3ecac79bbf09..33281370f415 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -57,6 +57,8 @@ struct felix_info { u32 speed); struct regmap *(*init_regmap)(struct ocelot *ocelot, struct resource *res); + void (*phylink_get_caps)(struct ocelot *ocelot, int port, + struct phylink_config *pl_config); }; extern const struct dsa_switch_ops felix_switch_ops;
Add the ability for felix users to announce their capabilities to DSA switches by way of phylink_get_caps. This will allow those users the ability to use phylink_generic_validate, which otherwise wouldn't be possible. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- drivers/net/dsa/ocelot/felix.c | 22 +++++++++++++++------- drivers/net/dsa/ocelot/felix.h | 2 ++ 2 files changed, 17 insertions(+), 7 deletions(-)