| Message ID | 20220508185313.2222956-16-colin.foster@in-advantage.com |
|---|---|
| State | New |
| 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.
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(-)