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?
--
Ansuel
