[PATCH 1/3] dt-bindings: net: Add bindings for Realtek PHYs

[PATCH 1/3] dt-bindings: net: Add bindings for Realtek PHYs

[Matthias Kaehlcke]

Add the 'realtek,enable-ssc' property to enable Spread Spectrum
Clocking (SSC) on Realtek PHYs that support it.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 .../devicetree/bindings/net/realtek.txt       | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek.txt

diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
new file mode 100644
index 000000000000..9fad97e7404f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek.txt
@@ -0,0 +1,21 @@
+Realtek PHY properties.
+
+This document describes properties of Realtek PHYs.
+
+Optional properties:
+- realtek,enable-ssc	Enable Spread Spectrum Clocking (SSC) on this port.
+			SSC is only available on some Realtek PHYs (e.g.
+			RTL8211E).
+
+Example:
+
+mdio0 {
+	compatible = "snps,dwmac-mdio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	ethphy: ethernet-phy@1 {
+		reg = <1>;
+		realtek,enable-ssc;
+	};
+};
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Matthias Kaehlcke]

The RTL8211E has extension pages, which can be accessed after
selecting a page through a custom method. Add a function to
modify bits in a register of an extension page and a few
helpers for dealing with ext pages.

rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
inspired by their counterparts phy_modify_paged() and
phy_restore_page().

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
This code might be applicable to other Realtek PHYs, but I don't
have access to the datasheets to confirm it, so for now it's just
for the RTL8211E.

 drivers/net/phy/realtek.c | 61 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945eb829..dfc2e20ef335 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -26,6 +26,9 @@
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
+#define RTL8211E_EXT_PAGE			7
+#define RTL8211E_EPAGSR				0x1e
+
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
@@ -53,6 +56,64 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl821e_select_ext_page(struct phy_device *phydev, int page)
+{
+	int rc;
+
+	rc = phy_write(phydev, RTL821x_PAGE_SELECT, RTL8211E_EXT_PAGE);
+	if (rc)
+		return rc;
+
+	return phy_write(phydev, RTL8211E_EPAGSR, page);
+}
+
+static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
+{
+	int r;
+
+	if (oldpage >= 0) {
+		r = phy_write(phydev, RTL821x_PAGE_SELECT, oldpage);
+
+		/* Propagate the operation return code if the page write
+		 * was successful.
+		 */
+		if (ret >= 0 && r < 0)
+			ret = r;
+	} else {
+		/* Propagate the page selection error code */
+		ret = oldpage;
+	}
+
+	return ret;
+}
+
+static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
+				    int page, u32 regnum, u16 mask, u16 set)
+{
+	int ret = 0;
+	int oldpage;
+	int new;
+
+	oldpage = phy_read(phydev, RTL821x_PAGE_SELECT);
+	if (oldpage < 0)
+		goto out;
+
+	ret = rtl821e_select_ext_page(phydev, page);
+	if (ret)
+		goto out;
+
+	ret = phy_read(phydev, regnum);
+	if (ret < 0)
+		goto out;
+
+	new = (ret & ~mask) | set;
+	if (new != ret)
+		ret = phy_write(phydev, regnum, new);
+
+out:
+	return rtl821e_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH 3/3] net: phy: realtek: Support SSC for the RTL8211E

[Matthias Kaehlcke]

By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
Enable it if the device tree property 'realtek,enable-ssc' exists.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/net/phy/realtek.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index dfc2e20ef335..b617169ccc8c 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,8 +9,10 @@
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
  */
 #include <linux/bitops.h>
-#include <linux/phy.h>
+#include <linux/device.h>
+#include <linux/of.h>
 #include <linux/module.h>
+#include <linux/phy.h>
 
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
@@ -28,6 +30,8 @@
 
 #define RTL8211E_EXT_PAGE			7
 #define RTL8211E_EPAGSR				0x1e
+#define RTL8211E_SCR				0x1a
+#define RTL8211E_SCR_DISABLE_RXC_SSC		BIT(2)
 
 #define RTL8211F_INSR				0x1d
 
@@ -87,8 +91,8 @@ static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
 	return ret;
 }
 
-static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
-				    int page, u32 regnum, u16 mask, u16 set)
+static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
+				     u32 regnum, u16 mask, u16 set)
 {
 	int ret = 0;
 	int oldpage;
@@ -114,6 +118,22 @@ static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
 	return rtl821e_restore_page(phydev, oldpage, ret);
 }
 
+static int rtl8211e_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	int err;
+
+	if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
+		err = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
+						RTL8211E_SCR_DISABLE_RXC_SSC,
+						0);
+		if (err)
+			dev_err(dev, "failed to enable SSC on RXC: %d\n", err);
+	}
+
+	return 0;
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -372,6 +392,7 @@ static struct phy_driver realtek_drvs[] = {
 		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
+		.probe          = rtl8211e_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Andrew Lunn]

On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> The RTL8211E has extension pages, which can be accessed after
> selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a few
> helpers for dealing with ext pages.
> 
> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> inspired by their counterparts phy_modify_paged() and
> phy_restore_page().

Hi Matthias

While an extended page is selected, what happens to the normal
registers in the range 0-0x1c? Are they still accessible?

	  Andrew

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Heiner Kallweit]

On 01.07.2019 22:02, Andrew Lunn wrote:
> On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
>> The RTL8211E has extension pages, which can be accessed after
>> selecting a page through a custom method. Add a function to
>> modify bits in a register of an extension page and a few
>> helpers for dealing with ext pages.
>>
>> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
>> inspired by their counterparts phy_modify_paged() and
>> phy_restore_page().
> 
> Hi Matthias
> 
> While an extended page is selected, what happens to the normal
> registers in the range 0-0x1c? Are they still accessible?
> 
AFAIK: no

> 	  Andrew
> 
Heiner

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Heiner Kallweit]

On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> The RTL8211E has extension pages, which can be accessed after
> selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a few
> helpers for dealing with ext pages.
> 
> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> inspired by their counterparts phy_modify_paged() and
> phy_restore_page().
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> This code might be applicable to other Realtek PHYs, but I don't
> have access to the datasheets to confirm it, so for now it's just
> for the RTL8211E.
> 
This extended page mechanism exists on a number of older Realtek
PHY's. For most extended pages however Realtek releases no public
documentation.
Considering that we use these helpers in one place only,  I don't
really see a need for them.

>  drivers/net/phy/realtek.c | 61 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a669945eb829..dfc2e20ef335 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -26,6 +26,9 @@
>  #define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> +#define RTL8211E_EXT_PAGE			7
> +#define RTL8211E_EPAGSR				0x1e
> +
>  #define RTL8211F_INSR				0x1d
>  
>  #define RTL8211F_TX_DELAY			BIT(8)
> @@ -53,6 +56,64 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
>  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
>  }
>  
> +static int rtl821e_select_ext_page(struct phy_device *phydev, int page)
> +{
> +	int rc;
> +
> +	rc = phy_write(phydev, RTL821x_PAGE_SELECT, RTL8211E_EXT_PAGE);
> +	if (rc)
> +		return rc;
> +
> +	return phy_write(phydev, RTL8211E_EPAGSR, page);
> +}
> +
> +static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> +{
> +	int r;
> +
> +	if (oldpage >= 0) {
> +		r = phy_write(phydev, RTL821x_PAGE_SELECT, oldpage);
> +
> +		/* Propagate the operation return code if the page write
> +		 * was successful.
> +		 */
> +		if (ret >= 0 && r < 0)
> +			ret = r;
> +	} else {
> +		/* Propagate the page selection error code */
> +		ret = oldpage;
> +	}
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> +				    int page, u32 regnum, u16 mask, u16 set)
> +{
> +	int ret = 0;
> +	int oldpage;
> +	int new;
> +
> +	oldpage = phy_read(phydev, RTL821x_PAGE_SELECT);
> +	if (oldpage < 0)
> +		goto out;
> +
> +	ret = rtl821e_select_ext_page(phydev, page);
> +	if (ret)
> +		goto out;
> +
> +	ret = phy_read(phydev, regnum);
> +	if (ret < 0)
> +		goto out;
> +
> +	new = (ret & ~mask) | set;
> +	if (new != ret)
> +		ret = phy_write(phydev, regnum, new);
> +
> +out:
> +	return rtl821e_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl8201_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> 

Re: [PATCH 3/3] net: phy: realtek: Support SSC for the RTL8211E

[Heiner Kallweit]

On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
> Enable it if the device tree property 'realtek,enable-ssc' exists.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/net/phy/realtek.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index dfc2e20ef335..b617169ccc8c 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,8 +9,10 @@
>   * Copyright (c) 2004 Freescale Semiconductor, Inc.
>   */
>  #include <linux/bitops.h>
> -#include <linux/phy.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
>  #include <linux/module.h>
> +#include <linux/phy.h>
>  
>  #define RTL821x_PHYSR				0x11
>  #define RTL821x_PHYSR_DUPLEX			BIT(13)
> @@ -28,6 +30,8 @@
>  
>  #define RTL8211E_EXT_PAGE			7
>  #define RTL8211E_EPAGSR				0x1e
> +#define RTL8211E_SCR				0x1a
> +#define RTL8211E_SCR_DISABLE_RXC_SSC		BIT(2)
>  
>  #define RTL8211F_INSR				0x1d
>  
> @@ -87,8 +91,8 @@ static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
>  	return ret;
>  }
>  
> -static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> -				    int page, u32 regnum, u16 mask, u16 set)
> +static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
> +				     u32 regnum, u16 mask, u16 set)
>  {
>  	int ret = 0;
>  	int oldpage;
> @@ -114,6 +118,22 @@ static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
>  	return rtl821e_restore_page(phydev, oldpage, ret);
>  }
>  
> +static int rtl8211e_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	int err;
> +
> +	if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
> +		err = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
> +						RTL8211E_SCR_DISABLE_RXC_SSC,
> +						0);
> +		if (err)
> +			dev_err(dev, "failed to enable SSC on RXC: %d\n", err);
> +	}
> +
> +	return 0;
> +}
> +
>  static int rtl8201_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> @@ -372,6 +392,7 @@ static struct phy_driver realtek_drvs[] = {
>  		.config_init	= &rtl8211e_config_init,
>  		.ack_interrupt	= &rtl821x_ack_interrupt,
>  		.config_intr	= &rtl8211e_config_intr,
> +		.probe          = rtl8211e_probe,

I'm not sure whether this setting survives soft reset and power-down.
Maybe it should be better applied in the config_init callback.

>  		.suspend	= genphy_suspend,
>  		.resume		= genphy_resume,
>  		.read_page	= rtl821x_read_page,
> 

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Andrew Lunn]

On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 22:02, Andrew Lunn wrote:
> > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> >> The RTL8211E has extension pages, which can be accessed after
> >> selecting a page through a custom method. Add a function to
> >> modify bits in a register of an extension page and a few
> >> helpers for dealing with ext pages.
> >>
> >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> >> inspired by their counterparts phy_modify_paged() and
> >> phy_restore_page().
> > 
> > Hi Matthias
> > 
> > While an extended page is selected, what happens to the normal
> > registers in the range 0-0x1c? Are they still accessible?
> > 
> AFAIK: no

This it would be better to make use of the core paged access support,
so that locking is done correctly.

   Andrew

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Matthias Kaehlcke]

On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 22:02, Andrew Lunn wrote:
> > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> >> The RTL8211E has extension pages, which can be accessed after
> >> selecting a page through a custom method. Add a function to
> >> modify bits in a register of an extension page and a few
> >> helpers for dealing with ext pages.
> >>
> >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> >> inspired by their counterparts phy_modify_paged() and
> >> phy_restore_page().
> > 
> > Hi Matthias
> > 
> > While an extended page is selected, what happens to the normal
> > registers in the range 0-0x1c? Are they still accessible?
> > 
> AFAIK: no

From my observations it looks like registers 0x00 to 0x0f are still
accessible, but not the ones above. IIUC 0x00-0x0f are standard
registers, the others are vendor specific.

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Matthias Kaehlcke]

On Mon, Jul 01, 2019 at 10:43:12PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> > The RTL8211E has extension pages, which can be accessed after
> > selecting a page through a custom method. Add a function to
> > modify bits in a register of an extension page and a few
> > helpers for dealing with ext pages.
> > 
> > rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> > inspired by their counterparts phy_modify_paged() and
> > phy_restore_page().
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > This code might be applicable to other Realtek PHYs, but I don't
> > have access to the datasheets to confirm it, so for now it's just
> > for the RTL8211E.
> > 
> This extended page mechanism exists on a number of older Realtek
> PHY's. For most extended pages however Realtek releases no public
> documentation.
> Considering that we use these helpers in one place only,  I don't
> really see a need for them.

I see it as self-documenting code, that may be reused, rather than
inline code with comments.

In any case I'm looking into another patch that would write registers
on extented pages rather than doing a modify, if that materializes I
think we would want the helpers.

Re: [PATCH 3/3] net: phy: realtek: Support SSC for the RTL8211E

[Matthias Kaehlcke]

On Mon, Jul 01, 2019 at 10:49:45PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> > By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
> > Enable it if the device tree property 'realtek,enable-ssc' exists.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/net/phy/realtek.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index dfc2e20ef335..b617169ccc8c 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -9,8 +9,10 @@
> >   * Copyright (c) 2004 Freescale Semiconductor, Inc.
> >   */
> >  #include <linux/bitops.h>
> > -#include <linux/phy.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> >  #include <linux/module.h>
> > +#include <linux/phy.h>
> >  
> >  #define RTL821x_PHYSR				0x11
> >  #define RTL821x_PHYSR_DUPLEX			BIT(13)
> > @@ -28,6 +30,8 @@
> >  
> >  #define RTL8211E_EXT_PAGE			7
> >  #define RTL8211E_EPAGSR				0x1e
> > +#define RTL8211E_SCR				0x1a
> > +#define RTL8211E_SCR_DISABLE_RXC_SSC		BIT(2)
> >  
> >  #define RTL8211F_INSR				0x1d
> >  
> > @@ -87,8 +91,8 @@ static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> >  	return ret;
> >  }
> >  
> > -static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> > -				    int page, u32 regnum, u16 mask, u16 set)
> > +static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
> > +				     u32 regnum, u16 mask, u16 set)
> >  {
> >  	int ret = 0;
> >  	int oldpage;
> > @@ -114,6 +118,22 @@ static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> >  	return rtl821e_restore_page(phydev, oldpage, ret);
> >  }
> >  
> > +static int rtl8211e_probe(struct phy_device *phydev)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	int err;
> > +
> > +	if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
> > +		err = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
> > +						RTL8211E_SCR_DISABLE_RXC_SSC,
> > +						0);
> > +		if (err)
> > +			dev_err(dev, "failed to enable SSC on RXC: %d\n", err);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int rtl8201_ack_interrupt(struct phy_device *phydev)
> >  {
> >  	int err;
> > @@ -372,6 +392,7 @@ static struct phy_driver realtek_drvs[] = {
> >  		.config_init	= &rtl8211e_config_init,
> >  		.ack_interrupt	= &rtl821x_ack_interrupt,
> >  		.config_intr	= &rtl8211e_config_intr,
> > +		.probe          = rtl8211e_probe,
> 
> I'm not sure whether this setting survives soft reset and power-down.
> Maybe it should be better applied in the config_init callback.

Sounds reasonable, I'll change it in the next revision, thanks!

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Matthias Kaehlcke]

On Mon, Jul 01, 2019 at 11:09:02PM +0200, Andrew Lunn wrote:
> On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> > On 01.07.2019 22:02, Andrew Lunn wrote:
> > > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> > >> The RTL8211E has extension pages, which can be accessed after
> > >> selecting a page through a custom method. Add a function to
> > >> modify bits in a register of an extension page and a few
> > >> helpers for dealing with ext pages.
> > >>
> > >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> > >> inspired by their counterparts phy_modify_paged() and
> > >> phy_restore_page().
> > > 
> > > Hi Matthias
> > > 
> > > While an extended page is selected, what happens to the normal
> > > registers in the range 0-0x1c? Are they still accessible?
> > > 
> > AFAIK: no
> 
> This it would be better to make use of the core paged access support,
> so that locking is done correctly.

Do I understand correctly that this would involve assigning
.read/write_page and use phy_select_page() and phy_restore_page()?

Besides the benefit of locking this would also result in less code and
we could get rid of the custom _restore_page().

Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages

[Heiner Kallweit]

On 02.07.2019 02:09, Matthias Kaehlcke wrote:
> On Mon, Jul 01, 2019 at 11:09:02PM +0200, Andrew Lunn wrote:
>> On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
>>> On 01.07.2019 22:02, Andrew Lunn wrote:
>>>> On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
>>>>> The RTL8211E has extension pages, which can be accessed after
>>>>> selecting a page through a custom method. Add a function to
>>>>> modify bits in a register of an extension page and a few
>>>>> helpers for dealing with ext pages.
>>>>>
>>>>> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
>>>>> inspired by their counterparts phy_modify_paged() and
>>>>> phy_restore_page().
>>>>
>>>> Hi Matthias
>>>>
>>>> While an extended page is selected, what happens to the normal
>>>> registers in the range 0-0x1c? Are they still accessible?
>>>>
>>> AFAIK: no
>>
>> This it would be better to make use of the core paged access support,
>> so that locking is done correctly.
> 
> Do I understand correctly that this would involve assigning
> .read/write_page and use phy_select_page() and phy_restore_page()?
> 
> Besides the benefit of locking this would also result in less code and
> we could get rid of the custom _restore_page().
> 
Interestingly certain Realtek PHY's (incl. RTL8211E) support two paging
mechanisms.

1. Normal paging (set reg 0x1f to page number) - set by core paging
2. Extended pages (set reg 0x1f to 7, and reg 0x1e to ext. page number)

Newer Realtek PHY's use normal paging only.