On Tue, Dec 10, 2024 at 01:06:29PM +0100, Christian Marangi wrote:
> On Tue, Dec 10, 2024 at 02:53:34AM +0100, Andrew Lunn wrote:
> > > +static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
> > > + int phy) __must_hold(&priv->bus->mdio_lock)
> > > +{
> > > + /* Check PHY page only for addr shared with switch */
> > > + if (phy != priv->switch_addr)
> > > + return 0;
> > > +
> > > + /* Don't restore page if it's not set to switch page */
> > > + if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
> > > + AN8855_PHY_PAGE_EXTENDED_4))
> > > + return 0;
> > > +
> > > + /* Restore page to 0, PHY might change page right after but that
> > > + * will be ignored as it won't be a switch page.
> > > + */
> > > + return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
> > > +}
> >
> > I don't really understand what is going on here. Maybe the commit
> > message needs expanding, or the function names changing.
> >
> > Generally, i would expect a save/restore action. Save the current
> > page, swap to the PHY page, do the PHY access, and then restore to the
> > saved page.
> >
>
> Idea is to save on extra read/write on subsequent write on the same
> page.
>
> Idea here is that PHY will receive most of the read (for status
> poll) hence in 90% of the time page will be 0.
>
> And switch will receive read/write only on setup or fdb/vlan access on
> configuration so it will receive subsequent write on the same page.
> (page 4)
>
> PHY might also need to write on page 1 on setup but never on page 4 as
> that is reserved for switch.
>
> Making the read/swap/write/restore adds 2 additional operation that can
> really be skipped 90% of the time.
>
> Also curret_page cache is indirectly protected by the mdio lock.
>
> So in short this function makes sure PHY for port 0 is configured on the
> right page based on the prev page set.
>
> An alternative way might be assume PHY is always on page 0 and any
> switch operation save and restore the page.
>
> Hope it's clear now why this is needed. Is this ok or you prefer the
> alternative way?
Please think about naming. I'm not sure _restore_ is the correct name
for the function. Maybe select. And i suggest adding some comments to
explain what is going on, since it is not obvious from the code in
this file, you probably need to cross reference it with code in the
DSA driver as well.
Andrew
