diff mbox series

[net-next,3/3] net: phy: realtek: add RTL8201F phy-id and functions

Message ID 1504875731-3680-4-git-send-email-hayashi.kunihiko@socionext.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series add UniPhier AVE ethernet support | expand

Commit Message

Kunihiko Hayashi Sept. 8, 2017, 1:02 p.m. UTC 
From: Jassi Brar <jaswinder.singh@linaro.org>

Add RTL8201F phy-id and the related functions to the driver.

The original patch is as follows:
https://patchwork.kernel.org/patch/2538341/

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Andrew Lunn Sept. 8, 2017, 1:57 p.m. UTC | #1 
On Fri, Sep 08, 2017 at 10:02:11PM +0900, Kunihiko Hayashi wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Add RTL8201F phy-id and the related functions to the driver.
> 
> The original patch is as follows:
> https://patchwork.kernel.org/patch/2538341/
> 
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Florian Fainelli Sept. 8, 2017, 6:51 p.m. UTC | #2 
On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Add RTL8201F phy-id and the related functions to the driver.
> 
> The original patch is as follows:
> https://patchwork.kernel.org/patch/2538341/
> 
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 9cbe645..d9974ce 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -29,10 +29,23 @@
>  #define RTL8211F_PAGE_SELECT	0x1f
>  #define RTL8211F_TX_DELAY	0x100
>  
> +#define RTL8201F_ISR		0x1e
> +#define RTL8201F_PAGE_SELECT	0x1f

We have a page select register define for the RTL8211F right above, so
surely we can make that a common definition?

> +#define RTL8201F_IER		0x13
> +
>  MODULE_DESCRIPTION("Realtek PHY driver");
>  MODULE_AUTHOR("Johnson Leung");
>  MODULE_LICENSE("GPL");
>  
> +static int rtl8201_ack_interrupt(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	err = phy_read(phydev, RTL8201F_ISR);
> +
> +	return (err < 0) ? err : 0;
> +}
> +
>  static int rtl821x_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> @@ -54,6 +67,25 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
>  	return (err < 0) ? err : 0;
>  }
>  
> +static int rtl8201_config_intr(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	/* switch to page 7 */
> +	phy_write(phydev, RTL8201F_PAGE_SELECT, 0x7);
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +		err = phy_write(phydev, RTL8201F_IER,
> +				BIT(13) | BIT(12) | BIT(11));

Can you detail what bits 11, 12 and 13 do? Do they correspond to link,
duplex and pause changes by any chance?

> +	else
> +		err = phy_write(phydev, RTL8201F_IER, 0);
> +
> +	/* restore to default page 0 */
> +	phy_write(phydev, RTL8201F_PAGE_SELECT, 0x0);
> +
> +	return err;
> +}
> +

Other than that, LGTM:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

>  static int rtl8211b_config_intr(struct phy_device *phydev)
>  {
>  	int err;
> @@ -129,6 +161,18 @@ static struct phy_driver realtek_drvs[] = {
>  		.config_aneg    = &genphy_config_aneg,
>  		.read_status    = &genphy_read_status,
>  	}, {
> +		.phy_id		= 0x001cc816,
> +		.name		= "RTL8201F 10/100Mbps Ethernet",
> +		.phy_id_mask	= 0x001fffff,
> +		.features	= PHY_BASIC_FEATURES,
> +		.flags		= PHY_HAS_INTERRUPT,
> +		.config_aneg	= &genphy_config_aneg,
> +		.read_status	= &genphy_read_status,
> +		.ack_interrupt	= &rtl8201_ack_interrupt,
> +		.config_intr	= &rtl8201_config_intr,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +	}, {
>  		.phy_id		= 0x001cc912,
>  		.name		= "RTL8211B Gigabit Ethernet",
>  		.phy_id_mask	= 0x001fffff,
> @@ -181,6 +225,7 @@ static struct phy_driver realtek_drvs[] = {
>  module_phy_driver(realtek_drvs);
>  
>  static struct mdio_device_id __maybe_unused realtek_tbl[] = {
> +	{ 0x001cc816, 0x001fffff },
>  	{ 0x001cc912, 0x001fffff },
>  	{ 0x001cc914, 0x001fffff },
>  	{ 0x001cc915, 0x001fffff },
>
Jassi Brar Sept. 9, 2017, 3:33 a.m. UTC | #3 
On 9 September 2017 at 00:21, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> Add RTL8201F phy-id and the related functions to the driver.
>>
>> The original patch is as follows:
>> https://patchwork.kernel.org/patch/2538341/
>>
>> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> ---
>>  drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>> index 9cbe645..d9974ce 100644
>> --- a/drivers/net/phy/realtek.c
>> +++ b/drivers/net/phy/realtek.c
>> @@ -29,10 +29,23 @@
>>  #define RTL8211F_PAGE_SELECT 0x1f
>>  #define RTL8211F_TX_DELAY    0x100
>>
>> +#define RTL8201F_ISR         0x1e
>> +#define RTL8201F_PAGE_SELECT 0x1f
>
> We have a page select register define for the RTL8211F right above, so
> surely we can make that a common definition?
>
That is just for the sake of consistency.
I mean RTL8211 wouldn't look neat among everything else RTL8201.

Also the page-select offsets just _happen_ to be same value...
RTL8211E_INER_LINK_STATUS and RTL8211F_INER_LINK_STATUS are very
different.

>> +#define RTL8201F_IER         0x13
>> +
>>  MODULE_DESCRIPTION("Realtek PHY driver");
>>  MODULE_AUTHOR("Johnson Leung");
>>  MODULE_LICENSE("GPL");
>>
>> +static int rtl8201_ack_interrupt(struct phy_device *phydev)
>> +{
>> +     int err;
>> +
>> +     err = phy_read(phydev, RTL8201F_ISR);
>> +
>> +     return (err < 0) ? err : 0;
>> +}
>> +
>>  static int rtl821x_ack_interrupt(struct phy_device *phydev)
>>  {
>>       int err;
>> @@ -54,6 +67,25 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
>>       return (err < 0) ? err : 0;
>>  }
>>
>> +static int rtl8201_config_intr(struct phy_device *phydev)
>> +{
>> +     int err;
>> +
>> +     /* switch to page 7 */
>> +     phy_write(phydev, RTL8201F_PAGE_SELECT, 0x7);
>> +
>> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>> +             err = phy_write(phydev, RTL8201F_IER,
>> +                             BIT(13) | BIT(12) | BIT(11));
>
> Can you detail what bits 11, 12 and 13 do? Do they correspond to link,
> duplex and pause changes by any chance?
>
Sorry no idea. The datasheet would say, and other functions too use
such magic values.

>> +     else
>> +             err = phy_write(phydev, RTL8201F_IER, 0);
>> +
>> +     /* restore to default page 0 */
>> +     phy_write(phydev, RTL8201F_PAGE_SELECT, 0x0);
>> +
>> +     return err;
>> +}
>> +
>
> Other than that, LGTM:
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
Thank you.
Andrew Lunn Sept. 9, 2017, 3:55 p.m. UTC | #4 
On Sat, Sep 09, 2017 at 09:03:05AM +0530, Jassi Brar wrote:
> On 9 September 2017 at 00:21, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> >> From: Jassi Brar <jaswinder.singh@linaro.org>
> >>
> >> Add RTL8201F phy-id and the related functions to the driver.
> >>
> >> The original patch is as follows:
> >> https://patchwork.kernel.org/patch/2538341/
> >>
> >> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> >> ---
> >>  drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 45 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> >> index 9cbe645..d9974ce 100644
> >> --- a/drivers/net/phy/realtek.c
> >> +++ b/drivers/net/phy/realtek.c
> >> @@ -29,10 +29,23 @@
> >>  #define RTL8211F_PAGE_SELECT 0x1f
> >>  #define RTL8211F_TX_DELAY    0x100
> >>
> >> +#define RTL8201F_ISR         0x1e
> >> +#define RTL8201F_PAGE_SELECT 0x1f
> >
> > We have a page select register define for the RTL8211F right above, so
> > surely we can make that a common definition?
> >
> That is just for the sake of consistency.
> I mean RTL8211 wouldn't look neat among everything else RTL8201.
> 
> Also the page-select offsets just _happen_ to be same value...

If you look at all the other supported PHYs, they all consistently use
the same page register across models. Marvell is always 22, mscc is
always 31, vitesse is always 31.

I would say it is a safe bet that all realtek PHYs will use 0x1f for
page select. So please add a patch which renames RTL8211F_PAGE_SELECT
to RTL821x_PAGE_SELECT.

It is best to do this now. I spent a while cleaning up the mess the
Marvell driver had got into with its page select code. Lots of
duplicate code and defines doing the same thing.

	  Andrew
Kunihiko Hayashi Sept. 11, 2017, 7:48 a.m. UTC | #5 
Hi Andrew,
Thank your for reviewing.

On Sat, 9 Sep 2017 17:55:17 +0200 <andrew@lunn.ch> wrote:

> On Sat, Sep 09, 2017 at 09:03:05AM +0530, Jassi Brar wrote:
> > On 9 September 2017 at 00:21, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> > >> From: Jassi Brar <jaswinder.singh@linaro.org>
> > >>
> > >> Add RTL8201F phy-id and the related functions to the driver.
> > >>
> > >> The original patch is as follows:
> > >> https://patchwork.kernel.org/patch/2538341/
> > >>
> > >> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> > >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > >> ---
> > >>  drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 45 insertions(+)
> > >>
> > >> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > >> index 9cbe645..d9974ce 100644
> > >> --- a/drivers/net/phy/realtek.c
> > >> +++ b/drivers/net/phy/realtek.c
> > >> @@ -29,10 +29,23 @@
> > >>  #define RTL8211F_PAGE_SELECT 0x1f
> > >>  #define RTL8211F_TX_DELAY    0x100
> > >>
> > >> +#define RTL8201F_ISR         0x1e
> > >> +#define RTL8201F_PAGE_SELECT 0x1f
> > >
> > > We have a page select register define for the RTL8211F right above, so
> > > surely we can make that a common definition?
> > >
> > That is just for the sake of consistency.
> > I mean RTL8211 wouldn't look neat among everything else RTL8201.
> > 
> > Also the page-select offsets just _happen_ to be same value...
> 
> If you look at all the other supported PHYs, they all consistently use
> the same page register across models. Marvell is always 22, mscc is
> always 31, vitesse is always 31.
> 
> I would say it is a safe bet that all realtek PHYs will use 0x1f for
> page select. So please add a patch which renames RTL8211F_PAGE_SELECT
> to RTL821x_PAGE_SELECT.
> 
> It is best to do this now. I spent a while cleaning up the mess the
> Marvell driver had got into with its page select code. Lots of
> duplicate code and defines doing the same thing.
> 
> 	  Andrew

I see. In case of renaming to RTL821x_PAGE_SELECT, 
I think that I'll make a patch series as realtek PHY series including this
patch independent from the series of MAC driver.

---
Best Regards,
Kunihiko Hayashi
diff mbox series

Patch

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9cbe645..d9974ce 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -29,10 +29,23 @@ 
 #define RTL8211F_PAGE_SELECT	0x1f
 #define RTL8211F_TX_DELAY	0x100
 
+#define RTL8201F_ISR		0x1e
+#define RTL8201F_PAGE_SELECT	0x1f
+#define RTL8201F_IER		0x13
+
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+static int rtl8201_ack_interrupt(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_read(phydev, RTL8201F_ISR);
+
+	return (err < 0) ? err : 0;
+}
+
 static int rtl821x_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -54,6 +67,25 @@  static int rtl8211f_ack_interrupt(struct phy_device *phydev)
 	return (err < 0) ? err : 0;
 }
 
+static int rtl8201_config_intr(struct phy_device *phydev)
+{
+	int err;
+
+	/* switch to page 7 */
+	phy_write(phydev, RTL8201F_PAGE_SELECT, 0x7);
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		err = phy_write(phydev, RTL8201F_IER,
+				BIT(13) | BIT(12) | BIT(11));
+	else
+		err = phy_write(phydev, RTL8201F_IER, 0);
+
+	/* restore to default page 0 */
+	phy_write(phydev, RTL8201F_PAGE_SELECT, 0x0);
+
+	return err;
+}
+
 static int rtl8211b_config_intr(struct phy_device *phydev)
 {
 	int err;
@@ -129,6 +161,18 @@  static struct phy_driver realtek_drvs[] = {
 		.config_aneg    = &genphy_config_aneg,
 		.read_status    = &genphy_read_status,
 	}, {
+		.phy_id		= 0x001cc816,
+		.name		= "RTL8201F 10/100Mbps Ethernet",
+		.phy_id_mask	= 0x001fffff,
+		.features	= PHY_BASIC_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
+		.config_aneg	= &genphy_config_aneg,
+		.read_status	= &genphy_read_status,
+		.ack_interrupt	= &rtl8201_ack_interrupt,
+		.config_intr	= &rtl8201_config_intr,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+	}, {
 		.phy_id		= 0x001cc912,
 		.name		= "RTL8211B Gigabit Ethernet",
 		.phy_id_mask	= 0x001fffff,
@@ -181,6 +225,7 @@  static struct phy_driver realtek_drvs[] = {
 module_phy_driver(realtek_drvs);
 
 static struct mdio_device_id __maybe_unused realtek_tbl[] = {
+	{ 0x001cc816, 0x001fffff },
 	{ 0x001cc912, 0x001fffff },
 	{ 0x001cc914, 0x001fffff },
 	{ 0x001cc915, 0x001fffff },