Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

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

On Fri, Aug 16, 2024 at 11:08:59PM +0530, Raju Lakkaraju wrote:
> Hi Russell King,
> 
> Thank you for quick response.
> 
> The 08/08/2024 22:07, Russell King (Oracle) wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, Aug 08, 2024 at 08:23:38PM +0000, Ronnie.Kunin@microchip.com wrote:
> > > We looked into an alternate way to migrate our lan743x driver from phylib to phylink continuing to support our existing hardware out in the field, without using the phylib's fixed-phy approach that you opposed to, but without modifying the phylib framework either.
> > > While investigating how to implement it we came across this which Raju borrowed ideas from: https://lore.kernel.org/linux-arm-kernel/YtGPO5SkMZfN8b%2Fs@shell.armlinux.org.uk/ . He is in the process of testing/cleaning it up and expects to submit it early next week.
> > 
> > That series died a death because it wasn't acceptable to the swnode
> > folk. In any case, that's clearly an over-complex solution for what is
> > a simple problem here.
> > 
> > The simplest solution would be for phylink to provide a new function,
> > e.g.
> > 
> > int phylink_set_fixed_link(struct phylink *pl,
> >                            const struct phylink_state *state)
> > {
> >         const struct phy_setting *s;
> >         unsigned long *adv;
> > 
> >         if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
> >             !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
> >                 return -EINVAL;
> > 
> >         s = phy_lookup_setting(state->speed, state->duplex,
> >                                pl->supported, true);
> >         if (!s)
> >                 return -EINVAL;
> > 
> >         adv = pl->link_config.advertising;
> >         linkmode_zero(adv);
> >         linkmode_set_bit(s->bit, adv);
> >         linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);
> > 
> >         pl->link_config.speed = state->speed;
> >         pl->link_config.duplex = state->duplex;
> >         pl->link_config.link = 1;
> >         pl->link_config.an_complete = 1;
> > 
> >         pl->cfg_link_an_mode = MLO_AN_FIXED;
> >         pl->cur_link_an_mode = pl->cfg_link_an_mode;
> > 
> >         return 0;
> > }
> > 
> > You can then call this _instead_ of attaching a PHY to switch phylink
> > into fixed-link mode with the specified speed and duplex (assuming
> > they are supported by the MAC.)
> > 
> > Isn't this going to be simpler than trying to use swnodes that need
> > to be setup before phylink_create() gets called?
> > 
> 
> Your suggestion seems to be working well for us. I'm currently testing it on
> different boards and checking for corner cases.
> I plan to submit it for code review next week.
> 
> Quick question: Should I submit your suggested code along with our patches, or
> will you be submitting it separately?

Note the point in my signature, which means I won't be doing very much
likely for through the rest of August and - given the timeline I expect,
nothing at all through much of September.

So, please include it as a separate patch with my authorship. You'll
need to add a prototype to linux/phylink.h for it as well. I'm giving
you explicit permission to add my sign-off for such a patch. Thanks.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

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