Re: [PATCH net-next 3/6] net: dsa: add support for retrieving the interface mode

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Jul 15, 2022 at 08:24:44PM +0300, Vladimir Oltean wrote:
> On Fri, Jul 15, 2022 at 05:01:37PM +0100, Russell King (Oracle) wrote:
> > DSA port bindings allow for an optional phy interface mode. When an
> > interface mode is not specified, DSA uses the NA interface mode type.
> > 
> > However, phylink needs to know the parameters of the link, and this
> > will become especially important when using phylink for ports that
> > are devoid of all properties except the required "reg" property, so
> > that phylink can select the maximum supported link settings. Without
> > knowing the interface mode, phylink can't truely know the maximum
> > link speed.
> > 
> > Update the prototype for the phylink_get_caps method to allow drivers
> > to report this information back to DSA, and update all DSA
> > implementations function declarations to cater for this change. No
> > code is added to the implementations.
> > 
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> (...)
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index b902b31bebce..7c6870d2c607 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -852,7 +852,8 @@ struct dsa_switch_ops {
> >  	 * PHYLINK integration
> >  	 */
> >  	void	(*phylink_get_caps)(struct dsa_switch *ds, int port,
> > -				    struct phylink_config *config);
> > +				    struct phylink_config *config,
> > +				    phy_interface_t *default_interface);
> 
> I would prefer having a dedicated void (*port_max_speed_interface),
> because the post-phylink DSA drivers (which are not few) will generally
> not need to concern themselves with implementing this, and I don't want
> driver writers to think they need to populate every parameter they see
> in phylink_get_caps. So the new function needs to be documented
> appropriately (specify who needs and who does not need to implement it,
> on which ports it will be called, etc).
> 
> In addition, if we have a dedicated ds->ops->port_max_speed_interface(),
> we can do a better job of avoiding breakage with this patch set, since
> if DSA cannot find a valid phylink fwnode, AND there is no
> port_max_speed_interface() callback for this driver, DSA can still
> preserve the current logic of not putting the port down, and not
> registering it with phylink. That can be accompanied by a dev_warn() to
> state that the CPU/DSA port isn't registered with phylink, please
> implement port_max_speed_interface() to address that.

To continue my previous email...

This is a great illustration why posting RFC series is a waste of time.
This patch was posted as RFC on:

24th June
29th June
5th July
13th July

Only when it's been posted today has there been a concern raised about
the approach. So, what's the use of asking for comments if comments only
come when patches are posted for merging. None what so ever. So, we've
lost the last three weeks because I decided to "be kind" and post RFC.
Total waste of effort.

Now, on your point... the series posted on the 24th June was using
the mv88e6xxx port_max_speed_interface() but discussion off the mailing
list:

20:19 < rmk> kabel: hmm, is mv88e6393x_port_max_speed_mode() correct?
20:20 < rmk> it seems to be suggesting to use PHY_INTERFACE_MODE_10GBASER for
             port 9
09:50 < kabel> rmk: yes, 10gbase-r is correct for 6393x. But we need to add
               exception for 6191x, as is done in chip.c function
               mv88e6393x_phylink_get_caps()
09:51 < kabel> rmk: on 6191x only port 10 supports >1g speeds
11:51 < rmk> kabel: moving it into the get_caps function makes it easier to set
             the default_interfaces for 6193x
14:20 < kabel> rmk: yes, get_caps doing it would be better

The problem is this - we call get_caps(), and we have to read registers
to work out what the port supports. If we have a separate callback, then
we need to re-read those registers to get the same information to report
what the default interface should be.

Since almost all of the Marvell implementations the values for both the
list of supported interfaces and the default interface both require
reading a register and translating it to a phy_interface_t, and then
setting the support mask, it seems logical to combine these two
functioalities into one function.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel