[1/4,RFCv2] net: phy: realtek: Support RTL8366RB variant

Message ID 20180528174752.6806-2-linus.walleij@linaro.org
State New
Headers show
Series
  • Realtek SMI RTL836x DSA driver
Related show

Commit Message

Linus Walleij May 28, 2018, 5:47 p.m.
The RTL8366RB is an ASIC with five internal PHYs for
LAN0..LAN3 and WAN. The PHYs are spawn off the main
device so they can be handled in a distributed manner
by the Realtek PHY driver. All that is really needed
is the power save feature enablement and letting the
PHY driver core pick up the IRQ from the switch chip.

Cc: Antti Seppälä <a.seppala@gmail.com>
Cc: Roman Yeryomin <roman@advem.lv>
Cc: Colin Leitner <colin.leitner@googlemail.com>
Cc: Gabor Juhos <juhosg@openwrt.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog RFCv1->RFCv2:
- No real changes.
---
 drivers/net/phy/realtek.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

-- 
2.17.0

Comments

Andrew Lunn May 29, 2018, 12:34 p.m. | #1
On Mon, May 28, 2018 at 07:47:49PM +0200, Linus Walleij wrote:
> The RTL8366RB is an ASIC with five internal PHYs for

> LAN0..LAN3 and WAN. The PHYs are spawn off the main

> device so they can be handled in a distributed manner

> by the Realtek PHY driver. All that is really needed

> is the power save feature enablement and letting the

> PHY driver core pick up the IRQ from the switch chip.

> 

> Cc: Antti Seppälä <a.seppala@gmail.com>

> Cc: Roman Yeryomin <roman@advem.lv>

> Cc: Colin Leitner <colin.leitner@googlemail.com>

> Cc: Gabor Juhos <juhosg@openwrt.org>

> Cc: Florian Fainelli <f.fainelli@gmail.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog RFCv1->RFCv2:

> - No real changes.

> ---

>  drivers/net/phy/realtek.c | 32 ++++++++++++++++++++++++++++++++

>  1 file changed, 32 insertions(+)

> 

> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c

> index 9f48ecf9c627..21624d1c9d38 100644

> --- a/drivers/net/phy/realtek.c

> +++ b/drivers/net/phy/realtek.c

> @@ -37,6 +37,9 @@

>  #define RTL8201F_ISR				0x1e

>  #define RTL8201F_IER				0x13

>  

> +#define RTL8366RB_POWER_SAVE	0x21

> +#define RTL8366RB_POWER_SAVE_ON 0x1000

> +

>  MODULE_DESCRIPTION("Realtek PHY driver");

>  MODULE_AUTHOR("Johnson Leung");

>  MODULE_LICENSE("GPL");

> @@ -145,6 +148,22 @@ static int rtl8211f_config_init(struct phy_device *phydev)

>  	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);

>  }

>  

> +static int rtl8366rb_config_init(struct phy_device *phydev)

> +{

> +	int ret;

> +	u16 reg;

> +

> +	ret = genphy_config_init(phydev);

> +	if (ret < 0)

> +		return ret;

> +

> +	reg = phy_read(phydev, RTL8366RB_POWER_SAVE);

> +	reg |= RTL8366RB_POWER_SAVE_ON;

> +	phy_write(phydev, RTL8366RB_POWER_SAVE, reg);

> +

> +	return 0;

> +}

> +

>  static struct phy_driver realtek_drvs[] = {

>  	{

>  		.phy_id         = 0x00008201,

> @@ -207,6 +226,18 @@ static struct phy_driver realtek_drvs[] = {

>  		.resume		= genphy_resume,

>  		.read_page	= rtl821x_read_page,

>  		.write_page	= rtl821x_write_page,

> +	}, {

> +		/* The main part of this DSA is in drivers/net/dsa */


Hi Linus

I would not bother with this comment. We don't say, The main part of
this driver is in drivers/net/ethernet/... PHY drivers should be
completely separate to MAC drivers.

Otherwise this looks good.

	Andrew



> +		.phy_id		= 0x001cc961,

> +		.name		= "RTL8366RB Gigabit Ethernet",

> +		.phy_id_mask	= 0x001fffff,

> +		.features	= PHY_GBIT_FEATURES,

> +		.flags		= PHY_HAS_INTERRUPT,

> +		.config_aneg	= &genphy_config_aneg,

> +		.config_init	= &rtl8366rb_config_init,

> +		.read_status	= &genphy_read_status,

> +		.suspend	= genphy_suspend,

> +		.resume		= genphy_resume,

>  	},

>  };

>  

> @@ -218,6 +249,7 @@ static struct mdio_device_id __maybe_unused realtek_tbl[] = {

>  	{ 0x001cc914, 0x001fffff },

>  	{ 0x001cc915, 0x001fffff },

>  	{ 0x001cc916, 0x001fffff },

> +	{ 0x001cc961, 0x001fffff },

>  	{ }

>  };

>  

> -- 

> 2.17.0

>
Heiner Kallweit May 29, 2018, 6:51 p.m. | #2
Am 28.05.2018 um 19:47 schrieb Linus Walleij:
> The RTL8366RB is an ASIC with five internal PHYs for

> LAN0..LAN3 and WAN. The PHYs are spawn off the main

> device so they can be handled in a distributed manner

> by the Realtek PHY driver. All that is really needed

> is the power save feature enablement and letting the

> PHY driver core pick up the IRQ from the switch chip.

> 

> Cc: Antti Seppälä <a.seppala@gmail.com>

> Cc: Roman Yeryomin <roman@advem.lv>

> Cc: Colin Leitner <colin.leitner@googlemail.com>

> Cc: Gabor Juhos <juhosg@openwrt.org>

> Cc: Florian Fainelli <f.fainelli@gmail.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog RFCv1->RFCv2:

> - No real changes.

> ---

>  drivers/net/phy/realtek.c | 32 ++++++++++++++++++++++++++++++++

>  1 file changed, 32 insertions(+)

> 

> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c

> index 9f48ecf9c627..21624d1c9d38 100644

> --- a/drivers/net/phy/realtek.c

> +++ b/drivers/net/phy/realtek.c

> @@ -37,6 +37,9 @@

>  #define RTL8201F_ISR				0x1e

>  #define RTL8201F_IER				0x13

>  

> +#define RTL8366RB_POWER_SAVE	0x21

Typically PHY register addresses are 5 bits wide, is 0x21 correct
and I miss something?

> +#define RTL8366RB_POWER_SAVE_ON 0x1000

Use BIT(12) instead?

> +

>  MODULE_DESCRIPTION("Realtek PHY driver");

>  MODULE_AUTHOR("Johnson Leung");

>  MODULE_LICENSE("GPL");

> @@ -145,6 +148,22 @@ static int rtl8211f_config_init(struct phy_device *phydev)

>  	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);

>  }

>  

> +static int rtl8366rb_config_init(struct phy_device *phydev)

> +{

> +	int ret;

> +	u16 reg;

> +

> +	ret = genphy_config_init(phydev);

> +	if (ret < 0)

> +		return ret;

> +

> +	reg = phy_read(phydev, RTL8366RB_POWER_SAVE);

> +	reg |= RTL8366RB_POWER_SAVE_ON;

> +	phy_write(phydev, RTL8366RB_POWER_SAVE, reg);

return phy_set_bits(phydev, RTL8366RB_POWER_SAVE, RTL8366RB_POWER_SAVE_ON);
would be easier.

> +

> +	return 0;

> +}

> +

>  static struct phy_driver realtek_drvs[] = {

>  	{

>  		.phy_id         = 0x00008201,

> @@ -207,6 +226,18 @@ static struct phy_driver realtek_drvs[] = {

>  		.resume		= genphy_resume,

>  		.read_page	= rtl821x_read_page,

>  		.write_page	= rtl821x_write_page,

> +	}, {

> +		/* The main part of this DSA is in drivers/net/dsa */

> +		.phy_id		= 0x001cc961,

> +		.name		= "RTL8366RB Gigabit Ethernet",

> +		.phy_id_mask	= 0x001fffff,

> +		.features	= PHY_GBIT_FEATURES,

> +		.flags		= PHY_HAS_INTERRUPT,

> +		.config_aneg	= &genphy_config_aneg,

Can be omitted, genphy_config_aneg is the default since 00fde79532d6
"net: phy: core: use genphy version of callbacks read_status and
config_aneg per default".

> +		.config_init	= &rtl8366rb_config_init,

> +		.read_status	= &genphy_read_status,

Can be omitted, genphy_read_status is the default.

> +		.suspend	= genphy_suspend,

> +		.resume		= genphy_resume,

>  	},

>  };

>  

> @@ -218,6 +249,7 @@ static struct mdio_device_id __maybe_unused realtek_tbl[] = {

>  	{ 0x001cc914, 0x001fffff },

>  	{ 0x001cc915, 0x001fffff },

>  	{ 0x001cc916, 0x001fffff },

> +	{ 0x001cc961, 0x001fffff },

>  	{ }

>  };

>  

>
Linus Walleij May 29, 2018, 8:01 p.m. | #3
On Tue, May 29, 2018 at 8:51 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:

>> +#define RTL8366RB_POWER_SAVE 0x21


> Typically PHY register addresses are 5 bits wide, is 0x21 correct

> and I miss something?


If it is correct I don't know, but it appears in the vendor
code:

/*Power Saving*/
#define RTL8368S_POWER_SAVING_PAGE                              0
#define RTL8368S_POWER_SAVING_REG                                       21
#define RTL8368S_POWER_SAVING_BIT_MSK                   0x1000

Then:

        phySmiAddr = 0x8000 | (1<<(phyNo +RTL8368S_PHY_NO_OFFSET)) |

((RTL8368S_POWER_SAVING_PAGE<<RTL8368S_PHY_PAGE_OFFSET)&RTL8368S_PHY_PAGE_MASK)
|
                        (RTL8368S_POWER_SAVING_REG&RTL8368S_PHY_REG_MASK);

        retVal = rtl8368s_setAsicReg(phySmiAddr, 0);
        if (retVal !=  SUCCESS)
                return retVal;

The PHYs are accessed here in memory area 0x8000.

Fixed the rest, thanks!

Yours,
Linus Walleij
Andrew Lunn May 29, 2018, 8:17 p.m. | #4
On Tue, May 29, 2018 at 10:01:14PM +0200, Linus Walleij wrote:
> On Tue, May 29, 2018 at 8:51 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> 

> >> +#define RTL8366RB_POWER_SAVE 0x21

> 

> > Typically PHY register addresses are 5 bits wide, is 0x21 correct

> > and I miss something?


Heiner is correct, MDIO only supports 32 register, when using clause
22. However, your device is not clause 22, it is its own thing. So one
danger you have is that we put some checks in the generic code testing
for values > 31, and return -EINVAL.

I think you have two choices:

1) A comment explaining what is going on here, how 0x21 is valid in
this context. And check the return value and give out a good warning
which will point somebody in the right direction to notice this 0x21.

2) Move this into the DSA driver, which does not have this
restriction.

	Andrew
Heiner Kallweit May 29, 2018, 9:15 p.m. | #5
Am 29.05.2018 um 22:01 schrieb Linus Walleij:
> On Tue, May 29, 2018 at 8:51 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> 

>>> +#define RTL8366RB_POWER_SAVE 0x21

> 

>> Typically PHY register addresses are 5 bits wide, is 0x21 correct

>> and I miss something?

> 

> If it is correct I don't know, but it appears in the vendor

> code:

> 

> /*Power Saving*/

> #define RTL8368S_POWER_SAVING_PAGE                              0

> #define RTL8368S_POWER_SAVING_REG                                       21

This is a decimal number .. You use 0x21.


> #define RTL8368S_POWER_SAVING_BIT_MSK                   0x1000

> 

> Then:

> 

>         phySmiAddr = 0x8000 | (1<<(phyNo +RTL8368S_PHY_NO_OFFSET)) |

> 

> ((RTL8368S_POWER_SAVING_PAGE<<RTL8368S_PHY_PAGE_OFFSET)&RTL8368S_PHY_PAGE_MASK)

> |

>                         (RTL8368S_POWER_SAVING_REG&RTL8368S_PHY_REG_MASK);

> 

>         retVal = rtl8368s_setAsicReg(phySmiAddr, 0);

>         if (retVal !=  SUCCESS)

>                 return retVal;

> 

> The PHYs are accessed here in memory area 0x8000.

> 

> Fixed the rest, thanks!

> 

> Yours,

> Linus Walleij

>

Patch

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9f48ecf9c627..21624d1c9d38 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -37,6 +37,9 @@ 
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_IER				0x13
 
+#define RTL8366RB_POWER_SAVE	0x21
+#define RTL8366RB_POWER_SAVE_ON 0x1000
+
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
@@ -145,6 +148,22 @@  static int rtl8211f_config_init(struct phy_device *phydev)
 	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
 }
 
+static int rtl8366rb_config_init(struct phy_device *phydev)
+{
+	int ret;
+	u16 reg;
+
+	ret = genphy_config_init(phydev);
+	if (ret < 0)
+		return ret;
+
+	reg = phy_read(phydev, RTL8366RB_POWER_SAVE);
+	reg |= RTL8366RB_POWER_SAVE_ON;
+	phy_write(phydev, RTL8366RB_POWER_SAVE, reg);
+
+	return 0;
+}
+
 static struct phy_driver realtek_drvs[] = {
 	{
 		.phy_id         = 0x00008201,
@@ -207,6 +226,18 @@  static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+	}, {
+		/* The main part of this DSA is in drivers/net/dsa */
+		.phy_id		= 0x001cc961,
+		.name		= "RTL8366RB Gigabit Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.config_init	= &rtl8366rb_config_init,
+		.read_status	= &genphy_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
 	},
 };
 
@@ -218,6 +249,7 @@  static struct mdio_device_id __maybe_unused realtek_tbl[] = {
 	{ 0x001cc914, 0x001fffff },
 	{ 0x001cc915, 0x001fffff },
 	{ 0x001cc916, 0x001fffff },
+	{ 0x001cc961, 0x001fffff },
 	{ }
 };