Re: [PATCH net-next 13/15] net: phy: realtek: drop .read_page and .write_page for rtl822x series

["Marek Behún" <kabel@kernel.org>]

On Wed, 20 Dec 2023 18:23:21 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 20.12.2023 16:55, Marek Behún wrote:
> > Drop the .read_page() and .write_page() methods for rtl822x series.
> > 
> > The rtl822x driver methods are now reimplemented to only access clause
> > 45 registers and these are the last methods that explicitly access
> > clause 22 registers.
> > 
> > If the underlying MDIO bus is clause 22, the paging mechanism is still
> > used internally in the .read_mmd() and .write_mmd() methods when
> > accessing registers in MMD 31.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/net/phy/realtek.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index cf608d390aa5..e2f68ac4b005 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -963,8 +963,6 @@ static struct phy_driver realtek_drvs[] = {
> >  		.read_status	= rtl822x_read_status,
> >  		.suspend	= genphy_c45_pma_suspend,
> >  		.resume		= rtl822x_resume,
> > -		.read_page	= rtl821x_read_page,
> > -		.write_page	= rtl821x_write_page,
> >  		.read_mmd	= rtlgen_read_mmd,
> >  		.write_mmd	= rtlgen_write_mmd,
> >  	}, {
> > @@ -975,8 +973,6 @@ static struct phy_driver realtek_drvs[] = {
> >  		.read_status	= rtl822x_read_status,
> >  		.suspend	= genphy_c45_pma_suspend,
> >  		.resume		= rtl822x_resume,
> > -		.read_page	= rtl821x_read_page,
> > -		.write_page	= rtl821x_write_page,
> >  		.read_mmd	= rtlgen_read_mmd,
> >  		.write_mmd	= rtlgen_write_mmd,
> >  	}, {
> > @@ -987,8 +983,6 @@ static struct phy_driver realtek_drvs[] = {
> >  		.read_status    = rtl822x_read_status,
> >  		.suspend	= genphy_c45_pma_suspend,
> >  		.resume		= rtl822x_resume,
> > -		.read_page      = rtl821x_read_page,
> > -		.write_page     = rtl821x_write_page,
> >  		.read_mmd	= rtlgen_read_mmd,
> >  		.write_mmd	= rtlgen_write_mmd,
> >  	}, {
> > @@ -999,8 +993,6 @@ static struct phy_driver realtek_drvs[] = {
> >  		.read_status    = rtl822x_read_status,
> >  		.suspend	= genphy_c45_pma_suspend,
> >  		.resume		= rtl822x_resume,
> > -		.read_page      = rtl821x_read_page,
> > -		.write_page     = rtl821x_write_page,
> >  		.read_mmd	= rtlgen_read_mmd,
> >  		.write_mmd	= rtlgen_write_mmd,
> >  	}, {
> > @@ -1011,8 +1003,6 @@ static struct phy_driver realtek_drvs[] = {
> >  		.read_status    = rtl822x_read_status,
> >  		.suspend	= genphy_c45_pma_suspend,
> >  		.resume		= rtl822x_resume,
> > -		.read_page      = rtl821x_read_page,
> > -		.write_page     = rtl821x_write_page,
> >  		.read_mmd	= rtlgen_read_mmd,
> >  		.write_mmd	= rtlgen_write_mmd,
> >  	}, {
> > @@ -1023,8 +1013,6 @@ static struct phy_driver realtek_drvs[] = {
> >  		.read_status    = rtl822x_read_status,
> >  		.suspend	= genphy_c45_pma_suspend,
> >  		.resume		= rtl822x_resume,
> > -		.read_page      = rtl821x_read_page,
> > -		.write_page     = rtl821x_write_page,
> >  		.read_mmd	= rtlgen_read_mmd,
> >  		.write_mmd	= rtlgen_write_mmd,
> >  	}, {  
> 
> Dropping the read_page/write_page hooks will be problematic,
> because they are used by the PHY initialization in e.g.
> rtl8125a_2_hw_phy_config().

I see. 

Maybe it would be simpler to just remove it from this series.

Looking at all instances of paged access in r8169, most of them seem to
access the vendor 2 MMD registers. Also the person from Realtek says
that MMD registers are available also on 1gbps PHYs.

Looking at PHY specs for RTL8211 series, all of them (as old as 2009)
seem to document MMD access.

So I think we can safely add .read_mmd() and .write_mmd() methods to
all the PHYs in realtek.c that may be used by r8169, and then we can
change the relevant phy_read/write/modify_paged calls into
phy_read/write/modify_mmd in r8169 according to the formula.

(The relevant accesses being those where page is set to value >= 0xa00.)

What do you think?

Marek