Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1728800 > unrolled thread

[PATCH net-next 0/3] add UniPhier AVE ethernet support

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 0/3] add UniPhier AVE ethernet support Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-08 15:10 +0200
    [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-08 15:10 +0200
      Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and  functions Andrew Lunn <andrew@lunn.ch> - 2017-09-08 16:00 +0200
      Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and  functions Florian Fainelli <f.fainelli@gmail.com> - 2017-09-08 21:00 +0200
        Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions Jassi Brar <jaswinder.singh@linaro.org> - 2017-09-09 05:40 +0200
          Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and  functions Andrew Lunn <andrew@lunn.ch> - 2017-09-09 18:00 +0200
            Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-11 09:50 +0200
    [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-08 15:10 +0200
      Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for  Socionext UniPhier AVE Andrew Lunn <andrew@lunn.ch> - 2017-09-08 16:10 +0200
        Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-11 09:20 +0200
      Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for  Socionext UniPhier AVE Florian Fainelli <f.fainelli@gmail.com> - 2017-09-08 21:00 +0200
        Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-11 09:20 +0200
    Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet  driver Andrew Lunn <andrew@lunn.ch> - 2017-09-08 16:00 +0200
      Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-11 09:00 +0200
        Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet  driver Andrew Lunn <andrew@lunn.ch> - 2017-09-11 14:40 +0200
          Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-12 11:30 +0200
    Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-09-08 16:50 +0200
      Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-11 09:00 +0200
    Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet  driver Florian Fainelli <f.fainelli@gmail.com> - 2017-09-09 18:40 +0200
      Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver Kunihiko Hayashi <hayashi.kunihiko@socionext.com> - 2017-09-11 09:00 +0200

#1728800 — [PATCH net-next 0/3] add UniPhier AVE ethernet support

This series adds support for Socionext AVE ethernet controller implemented
on UniPhier SoCs. This driver supports RGMII/RMII modes.

Furthermore, this series includes support for realtek RTL8201F PHY to be
implemented on some supported boards.

Jassi Brar (1):
  net: phy: realtek: add RTL8201F phy-id and functions

Kunihiko Hayashi (2):
  dt-bindings: net: add DT bindings for Socionext UniPhier AVE
  net: ethernet: socionext: add AVE ethernet driver

 .../bindings/net/socionext,uniphier-ave4.txt       |   44 +
 drivers/net/ethernet/Kconfig                       |    1 +
 drivers/net/ethernet/Makefile                      |    1 +
 drivers/net/ethernet/socionext/Kconfig             |   22 +
 drivers/net/ethernet/socionext/Makefile            |    4 +
 drivers/net/ethernet/socionext/sni_ave.c           | 1618 ++++++++++++++++++++
 drivers/net/phy/realtek.c                          |   45 +
 7 files changed, 1735 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
 create mode 100644 drivers/net/ethernet/socionext/Kconfig
 create mode 100644 drivers/net/ethernet/socionext/Makefile
 create mode 100644 drivers/net/ethernet/socionext/sni_ave.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1728802 — [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

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
+#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 },
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1728939 — Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

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

[toc] | [prev] | [next] | [standalone]

#1729205 — Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

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 },
> 


-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1729374 — Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

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.

[toc] | [prev] | [next] | [standalone]

#1729476 — Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

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

[toc] | [prev] | [next] | [standalone]

#1730195 — Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions

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

[toc] | [prev] | [next] | [standalone]

#1728805 — [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE

DT bindings for the AVE ethernet controller found on Socionext's
UniPhier platforms.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 .../bindings/net/socionext,uniphier-ave4.txt       | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt

diff --git a/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
new file mode 100644
index 0000000..57ae96d
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
@@ -0,0 +1,44 @@
+* Socionext AVE ethernet controller
+
+This describes the devicetree bindings for AVE ethernet controller
+implemented on Socionext UniPhier SoCs.
+
+Required properties:
+ - compatible:  Should be "socionext,uniphier-ave4"
+ - reg: Address where registers are mapped and size of region.
+ - interrupts: IRQ common for mac and phy interrupts.
+ - phy-mode: See ethernet.txt in the same directory.
+ - #address-cells: Must be <1>.
+ - #size-cells: Must be <0>.
+ - Node, with 'reg' property, for each PHY on the MDIO bus.
+
+Optional properties:
+ - socionext,desc-bits: 32/64 descriptor size. Default 32.
+ - local-mac-address: See ethernet.txt in the same directory.
+ - pinctrl-names: List of assigned state names, see pinctrl
+	binding documentation.
+ - pinctrl-0: List of phandles to configure the GPIO pin used
+	as interrupt line, see also generic and your platform
+	specific pinctrl binding documentation.
+ - socionext,internal-phy-interrupt: Boolean to denote if the
+	PHY interrupt is internally handled by the MAC.
+
+
+Example:
+
+	eth: ethernet@65000000 {
+		compatible = "socionext,uniphier-ave4";
+		reg = <0x65000000 0x8500>;
+		interrupts = <0 66 4>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ether_rgmii>;
+		phy-mode = "rgmii";
+		socionext,desc-bits = <64>;
+		local-mac-address = [00 00 00 00 00 00];
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		ethphy: ethphy@1 {
+			reg = <1>;
+		};
+	};
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1728953 — Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE

> +	eth: ethernet@65000000 {
> +		compatible = "socionext,uniphier-ave4";
> +		reg = <0x65000000 0x8500>;
> +		interrupts = <0 66 4>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ether_rgmii>;
> +		phy-mode = "rgmii";
> +		socionext,desc-bits = <64>;
> +		local-mac-address = [00 00 00 00 00 00];
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;

> +		ethphy: ethphy@1 {
> +			reg = <1>;
> +		};


So you normally have an mdio node, and the phy as a children of that
node.

       mdio {
                ethphy: ethernet-phy@6 {
                        reg = <6>;
                };
        };

	Andrew

[toc] | [prev] | [next] | [standalone]

#1730183 — Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE

Hi Andrew,

On Fri, 8 Sep 2017 16:03:22 +0200 <andrew@lunn.ch> wrote:

> > +	eth: ethernet@65000000 {
> > +		compatible = "socionext,uniphier-ave4";
> > +		reg = <0x65000000 0x8500>;
> > +		interrupts = <0 66 4>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_ether_rgmii>;
> > +		phy-mode = "rgmii";
> > +		socionext,desc-bits = <64>;
> > +		local-mac-address = [00 00 00 00 00 00];
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> 
> > +		ethphy: ethphy@1 {
> > +			reg = <1>;
> > +		};
> 
> 
> So you normally have an mdio node, and the phy as a children of that
> node.
> 
>        mdio {
>                 ethphy: ethernet-phy@6 {
>                         reg = <6>;
>                 };
>         };
> 
> 	Andrew

I see. I'll rewrite the binding example using mdio node.

---
Best Regards,
Kunihiko Hayashi

[toc] | [prev] | [next] | [standalone]

#1729203 — Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE

On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> DT bindings for the AVE ethernet controller found on Socionext's
> UniPhier platforms.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  .../bindings/net/socionext,uniphier-ave4.txt       | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> new file mode 100644
> index 0000000..57ae96d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> @@ -0,0 +1,44 @@
> +* Socionext AVE ethernet controller
> +
> +This describes the devicetree bindings for AVE ethernet controller
> +implemented on Socionext UniPhier SoCs.
> +
> +Required properties:
> + - compatible:  Should be "socionext,uniphier-ave4"
> + - reg: Address where registers are mapped and size of region.
> + - interrupts: IRQ common for mac and phy interrupts.
> + - phy-mode: See ethernet.txt in the same directory.
> + - #address-cells: Must be <1>.
> + - #size-cells: Must be <0>.
> + - Node, with 'reg' property, for each PHY on the MDIO bus.
> +
> +Optional properties:
> + - socionext,desc-bits: 32/64 descriptor size. Default 32.

32 bits, this has an unit. Why is this configurable through Device Tree
and if this is describing the HW, as it should, why is not that inferred
from the compatible string?

> + - local-mac-address: See ethernet.txt in the same directory.
> + - pinctrl-names: List of assigned state names, see pinctrl
> +	binding documentation.
> + - pinctrl-0: List of phandles to configure the GPIO pin used
> +	as interrupt line, see also generic and your platform
> +	specific pinctrl binding documentation.
> + - socionext,internal-phy-interrupt: Boolean to denote if the
> +	PHY interrupt is internally handled by the MAC.

Interesting, why do you need to declare this as opposed to also
determining this entirely from the compatible string of the Ethernet MAC?

> +
> +
> +Example:
> +
> +	eth: ethernet@65000000 {
> +		compatible = "socionext,uniphier-ave4";
> +		reg = <0x65000000 0x8500>;
> +		interrupts = <0 66 4>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ether_rgmii>;
> +		phy-mode = "rgmii";
> +		socionext,desc-bits = <64>;
> +		local-mac-address = [00 00 00 00 00 00];
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;

There is no phy-handle property, yet there clearly seems to be one
attached, and I think Andrew now also responded to that in the driver
patch, this needs fixing.

> +		ethphy: ethphy@1 {
> +			reg = <1>;
> +		};
> +	};
> 


-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1730180 — Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE

Hi Florian,

On Fri, 8 Sep 2017 11:54:31 -0700 <f.fainelli@gmail.com> wrote:

> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> > DT bindings for the AVE ethernet controller found on Socionext's
> > UniPhier platforms.
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >  .../bindings/net/socionext,uniphier-ave4.txt       | 44 ++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> > new file mode 100644
> > index 0000000..57ae96d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> > @@ -0,0 +1,44 @@
> > +* Socionext AVE ethernet controller
> > +
> > +This describes the devicetree bindings for AVE ethernet controller
> > +implemented on Socionext UniPhier SoCs.
> > +
> > +Required properties:
> > + - compatible:  Should be "socionext,uniphier-ave4"
> > + - reg: Address where registers are mapped and size of region.
> > + - interrupts: IRQ common for mac and phy interrupts.
> > + - phy-mode: See ethernet.txt in the same directory.
> > + - #address-cells: Must be <1>.
> > + - #size-cells: Must be <0>.
> > + - Node, with 'reg' property, for each PHY on the MDIO bus.
> > +
> > +Optional properties:
> > + - socionext,desc-bits: 32/64 descriptor size. Default 32.
> 
> 32 bits, this has an unit. Why is this configurable through Device Tree
> and if this is describing the HW, as it should, why is not that inferred
> from the compatible string?

I've thought the common compatible string was reasonable for all UniPhier
SoCs. However, since there are some different features among SoCs
like "desc-bits" etc. and such feature would increase from now on,
it's convenient to add compatible strings for each SoCs.

> > + - local-mac-address: See ethernet.txt in the same directory.
> > + - pinctrl-names: List of assigned state names, see pinctrl
> > +	binding documentation.
> > + - pinctrl-0: List of phandles to configure the GPIO pin used
> > +	as interrupt line, see also generic and your platform
> > +	specific pinctrl binding documentation.
> > + - socionext,internal-phy-interrupt: Boolean to denote if the
> > +	PHY interrupt is internally handled by the MAC.
> 
> Interesting, why do you need to declare this as opposed to also
> determining this entirely from the compatible string of the Ethernet MAC?

This property was added after determining to use common compatible string.
I'll re-consider to use compatible strings for each SoCs.

> 
> > +
> > +
> > +Example:
> > +
> > +	eth: ethernet@65000000 {
> > +		compatible = "socionext,uniphier-ave4";
> > +		reg = <0x65000000 0x8500>;
> > +		interrupts = <0 66 4>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_ether_rgmii>;
> > +		phy-mode = "rgmii";
> > +		socionext,desc-bits = <64>;
> > +		local-mac-address = [00 00 00 00 00 00];
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> 
> There is no phy-handle property, yet there clearly seems to be one
> attached, and I think Andrew now also responded to that in the driver
> patch, this needs fixing.

Surely phy-handle is missing in this. I'll fix it with mdio node.

> > +		ethphy: ethphy@1 {
> > +			reg = <1>;
> > +		};
> > +	};
> > 
> 
> 
> -- 
> Florian

---
Best Regards,
Kunihiko Hayashi

[toc] | [prev] | [next] | [standalone]

#1728947 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

> +static int ave_mdio_busywait(struct net_device *ndev)
> +{
> +	int ret = 1, loop = 100;
> +	u32 mdiosr;
> +
> +	/* wait until completion */
> +	while (1) {
> +		mdiosr = ave_r32(ndev, AVE_MDIOSR);
> +		if (!(mdiosr & AVE_MDIOSR_STS))
> +			break;
> +
> +		usleep_range(10, 20);
> +		if (!loop--) {
> +			netdev_err(ndev,
> +				   "failed to read from MDIO (status:0x%08x)\n",
> +				   mdiosr);
> +			ret = 0;

ETIMEDOUT would be better.

> +			break;
> +		}
> +	}
> +
> +	return ret;

and then return 0 on success. That is the normal convention for return
values. An error code, and 0.

> +static int ave_mdiobus_write(struct mii_bus *bus,
> +			     int phyid, int regnum, u16 val)
> +{
> +	struct net_device *ndev = bus->priv;
> +	u32 mdioctl;
> +
> +	/* write address */
> +	ave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);
> +
> +	/* write data */
> +	ave_w32(ndev, AVE_MDIOWDR, val);
> +
> +	/* write request */
> +	mdioctl = ave_r32(ndev, AVE_MDIOCTR);
> +	ave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);
> +
> +	if (!ave_mdio_busywait(ndev)) {
> +		netdev_err(ndev, "phy-%d reg-%x write failed\n",
> +			   phyid, regnum);
> +		return -1;

If ave_mdio_busywait() returns ETIMEDOUT, you can just return
it. Returning -1 is not good.

> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ave_interrupt(int irq, void *netdev)
> +{
> +	struct net_device *ndev = (struct net_device *)netdev;
> +	struct ave_private *priv = netdev_priv(ndev);
> +	u32 gimr_val, gisr_val;
> +
> +	gimr_val = ave_irq_disable_all(ndev);
> +
> +	/* get interrupt status */
> +	gisr_val = ave_r32(ndev, AVE_GISR);
> +
> +	/* PHY */
> +	if (gisr_val & AVE_GI_PHY) {
> +		ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> +		if (priv->internal_phy_interrupt)
> +			phy_mac_interrupt(ndev->phydev, ndev->phydev->link);

Humm. I don't think this is correct. You are supposed to give it the
new link state, not the old.

What does a PHY interrupt mean here? 

> +static void ave_adjust_link(struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +	struct phy_device *phydev = ndev->phydev;
> +	u32 val, txcr, rxcr, rxcr_org;
> +
> +	/* set RGMII speed */
> +	val = ave_r32(ndev, AVE_TXCR);
> +	val &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);
> +
> +	if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&
> +	    phydev->speed == SPEED_1000)

phy_interface_mode_is_rgmii(), so that you handle all the RGMII modes.

> +		val |= AVE_TXCR_TXSPD_1G;
> +	else if (phydev->speed == SPEED_100)
> +		val |= AVE_TXCR_TXSPD_100;
> +
> +	ave_w32(ndev, AVE_TXCR, val);
> +
> +	/* set RMII speed (100M/10M only) */
> +	if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {

Not so safe. It would be better to check for the modes you actually
support.

> +	if (phydev->link)
> +		netif_carrier_on(ndev);
> +	else
> +		netif_carrier_off(ndev);

I don't think you need this. The phylib should do it for you.

> +
> +	phy_print_status(phydev);
> +}
> +
> +static int ave_init(struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +	struct device *dev = ndev->dev.parent;
> +	struct device_node *phy_node, *np = dev->of_node;
> +	struct phy_device *phydev;
> +	const void *mac_addr;
> +	u32 supported;
> +
> +	/* get mac address */
> +	mac_addr = of_get_mac_address(np);
> +	if (mac_addr)
> +		ether_addr_copy(ndev->dev_addr, mac_addr);
> +
> +	/* if the mac address is invalid, use random mac address */
> +	if (!is_valid_ether_addr(ndev->dev_addr)) {
> +		eth_hw_addr_random(ndev);
> +		dev_warn(dev, "Using random MAC address: %pM\n",
> +			 ndev->dev_addr);
> +	}
> +
> +	/* attach PHY with MAC */
> +	phy_node =  of_get_next_available_child(np, NULL);

???

Should this not be looking for a phy-handle property?
Documentation/devicetree/binds/net/ethernet.txt:

- phy-handle: phandle, specifies a reference to a node representing a PHY
  device; this property is described in the Devicetree Specification and so
  preferred;


> +	phydev = of_phy_connect(ndev, phy_node,
> +				ave_adjust_link, 0, priv->phy_mode);
> +	if (!phydev) {
> +		dev_err(dev, "could not attach to PHY\n");
> +		return -ENODEV;
> +	}
> +	of_node_put(phy_node);
> +
> +	priv->phydev = phydev;
> +	phydev->autoneg = AUTONEG_ENABLE;
> +	phydev->speed = 0;
> +	phydev->duplex = 0;

And this should not be needed.

> +
> +	dev_info(dev, "connected to %s phy with id 0x%x\n",
> +		 phydev->drv->name, phydev->phy_id);

phy_attached_info()

> +
> +	if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {

Same comment as above.

> +		supported = phydev->supported;
> +		phydev->supported &= ~PHY_GBIT_FEATURES;
> +		phydev->supported |= supported & PHY_BASIC_FEATURES;
> +	}
> +
> +	/* PHY interrupt stop instruction is needed because the interrupt
> +	 * continues to assert.
> +	 */
> +	phy_stop_interrupts(phydev);

Could you explain this some more? It sounds like your interrupt
controller is broken.

> +
> +	/* When PHY driver can't handle its interrupt directly,
> +	 * interrupt request always fails and polling method is used
> +	 * alternatively. In this case, the libphy says
> +	 * "libphy: uniphier-mdio: Can't get IRQ -1 (PHY)".
> +	 */
> +	phy_start_interrupts(phydev);

-1 is PHY_POLL. So calling phy_start_interrupts() is wrong. In fact,
you should not be calling phy_start_interrupts() at all. No other
Ethernet driver does.

> +
> +	phy_start_aneg(phydev);
> +
> +	return 0;
> +}
> +
> +static void ave_uninit(struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +
> +	phy_stop_interrupts(priv->phydev);

And no other Ethernet driver calls phy_stop_interrupts either.
Please take a look at this.

> +	phy_disconnect(priv->phydev);
> +}
> +

  Andrew

[toc] | [prev] | [next] | [standalone]

#1730175 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

Hi Andrew,
Thank you for your comments.

On Fri, 8 Sep 2017 15:50:30 +0200 <andrew@lunn.ch> wrote:

> > +static int ave_mdio_busywait(struct net_device *ndev)
> > +{
> > +	int ret = 1, loop = 100;
> > +	u32 mdiosr;
> > +
> > +	/* wait until completion */
> > +	while (1) {
> > +		mdiosr = ave_r32(ndev, AVE_MDIOSR);
> > +		if (!(mdiosr & AVE_MDIOSR_STS))
> > +			break;
> > +
> > +		usleep_range(10, 20);
> > +		if (!loop--) {
> > +			netdev_err(ndev,
> > +				   "failed to read from MDIO (status:0x%08x)\n",
> > +				   mdiosr);
> > +			ret = 0;
> 
> ETIMEDOUT would be better.

I see. I'll use this.

> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret;
> 
> and then return 0 on success. That is the normal convention for return
> values. An error code, and 0.

Surely, it's desiable to return zero and error code.

> > +static int ave_mdiobus_write(struct mii_bus *bus,
> > +			     int phyid, int regnum, u16 val)
> > +{
> > +	struct net_device *ndev = bus->priv;
> > +	u32 mdioctl;
> > +
> > +	/* write address */
> > +	ave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);
> > +
> > +	/* write data */
> > +	ave_w32(ndev, AVE_MDIOWDR, val);
> > +
> > +	/* write request */
> > +	mdioctl = ave_r32(ndev, AVE_MDIOCTR);
> > +	ave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);
> > +
> > +	if (!ave_mdio_busywait(ndev)) {
> > +		netdev_err(ndev, "phy-%d reg-%x write failed\n",
> > +			   phyid, regnum);
> > +		return -1;
> 
> If ave_mdio_busywait() returns ETIMEDOUT, you can just return
> it. Returning -1 is not good.

Indeed. I'll re-consider handling return value.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t ave_interrupt(int irq, void *netdev)
> > +{
> > +	struct net_device *ndev = (struct net_device *)netdev;
> > +	struct ave_private *priv = netdev_priv(ndev);
> > +	u32 gimr_val, gisr_val;
> > +
> > +	gimr_val = ave_irq_disable_all(ndev);
> > +
> > +	/* get interrupt status */
> > +	gisr_val = ave_r32(ndev, AVE_GISR);
> > +
> > +	/* PHY */
> > +	if (gisr_val & AVE_GI_PHY) {
> > +		ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> > +		if (priv->internal_phy_interrupt)
> > +			phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
> 
> Humm. I don't think this is correct. You are supposed to give it the
> new link state, not the old.
> 
> What does a PHY interrupt mean here? 

In the general case, I think PHY events like changing link state are transmitted
to CPU as interrupt via interrupt controller, then PHY driver itself can handle
the interrupt.

And in this case, PHY events are transmitted to MAC as one of its interrupt factor,
then I thought that MAC driver had to tell the events to PHY.

> > +static void ave_adjust_link(struct net_device *ndev)
> > +{
> > +	struct ave_private *priv = netdev_priv(ndev);
> > +	struct phy_device *phydev = ndev->phydev;
> > +	u32 val, txcr, rxcr, rxcr_org;
> > +
> > +	/* set RGMII speed */
> > +	val = ave_r32(ndev, AVE_TXCR);
> > +	val &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);
> > +
> > +	if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&
> > +	    phydev->speed == SPEED_1000)
> 
> phy_interface_mode_is_rgmii(), so that you handle all the RGMII modes.

It's convenient. I'll replace it.

> > +		val |= AVE_TXCR_TXSPD_1G;
> > +	else if (phydev->speed == SPEED_100)
> > +		val |= AVE_TXCR_TXSPD_100;
> > +
> > +	ave_w32(ndev, AVE_TXCR, val);
> > +
> > +	/* set RMII speed (100M/10M only) */
> > +	if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
> 
> Not so safe. It would be better to check for the modes you actually
> support.

Indeed. This part has to check if the specified mode is supported accurately.

> > +	if (phydev->link)
> > +		netif_carrier_on(ndev);
> > +	else
> > +		netif_carrier_off(ndev);
> 
> I don't think you need this. The phylib should do it for you.

Okay. I'll remove it.

> > +
> > +	phy_print_status(phydev);
> > +}
> > +
> > +static int ave_init(struct net_device *ndev)
> > +{
> > +	struct ave_private *priv = netdev_priv(ndev);
> > +	struct device *dev = ndev->dev.parent;
> > +	struct device_node *phy_node, *np = dev->of_node;
> > +	struct phy_device *phydev;
> > +	const void *mac_addr;
> > +	u32 supported;
> > +
> > +	/* get mac address */
> > +	mac_addr = of_get_mac_address(np);
> > +	if (mac_addr)
> > +		ether_addr_copy(ndev->dev_addr, mac_addr);
> > +
> > +	/* if the mac address is invalid, use random mac address */
> > +	if (!is_valid_ether_addr(ndev->dev_addr)) {
> > +		eth_hw_addr_random(ndev);
> > +		dev_warn(dev, "Using random MAC address: %pM\n",
> > +			 ndev->dev_addr);
> > +	}
> > +
> > +	/* attach PHY with MAC */
> > +	phy_node =  of_get_next_available_child(np, NULL);
> 
> ???
> 
> Should this not be looking for a phy-handle property?
> Documentation/devicetree/binds/net/ethernet.txt:
> 
> - phy-handle: phandle, specifies a reference to a node representing a PHY
>   device; this property is described in the Devicetree Specification and so
>   preferred;

Yes, I found it was wrong.
The device node has not a child node but phy-handle to specify PHY.

> > +	phydev = of_phy_connect(ndev, phy_node,
> > +				ave_adjust_link, 0, priv->phy_mode);
> > +	if (!phydev) {
> > +		dev_err(dev, "could not attach to PHY\n");
> > +		return -ENODEV;
> > +	}
> > +	of_node_put(phy_node);
> > +
> > +	priv->phydev = phydev;
> > +	phydev->autoneg = AUTONEG_ENABLE;
> > +	phydev->speed = 0;
> > +	phydev->duplex = 0;
> 
> And this should not be needed.

I understand that these parameters have been initialized.

> > +
> > +	dev_info(dev, "connected to %s phy with id 0x%x\n",
> > +		 phydev->drv->name, phydev->phy_id);
> 
> phy_attached_info()

It's convernient.

> > +
> > +	if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
> 
> Same comment as above.

Ditto.

> > +		supported = phydev->supported;
> > +		phydev->supported &= ~PHY_GBIT_FEATURES;
> > +		phydev->supported |= supported & PHY_BASIC_FEATURES;
> > +	}
> > +
> > +	/* PHY interrupt stop instruction is needed because the interrupt
> > +	 * continues to assert.
> > +	 */
> > +	phy_stop_interrupts(phydev);
> 
> Could you explain this some more? It sounds like your interrupt
> controller is broken.

I thought that the driver had to stop the own interrupt of PHY device
because it might be already enabled.
But surely we don't think of such a case.

> > +
> > +	/* When PHY driver can't handle its interrupt directly,
> > +	 * interrupt request always fails and polling method is used
> > +	 * alternatively. In this case, the libphy says
> > +	 * "libphy: uniphier-mdio: Can't get IRQ -1 (PHY)".
> > +	 */
> > +	phy_start_interrupts(phydev);
> 
> -1 is PHY_POLL. So calling phy_start_interrupts() is wrong. In fact,
> you should not be calling phy_start_interrupts() at all. No other
> Ethernet driver does.

The phy_start_interrupts() calls request_threaded_irq(), and it wll succeed
when PHY node has own 'interrupts' property.
In this case, it will fail and phydev->irq sets PHY_POLL.

However, phydev->irq is initialized PHY_POLL in phy_probe(),
surely it isn't necessary.

> > +
> > +	phy_start_aneg(phydev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void ave_uninit(struct net_device *ndev)
> > +{
> > +	struct ave_private *priv = netdev_priv(ndev);
> > +
> > +	phy_stop_interrupts(priv->phydev);
> 
> And no other Ethernet driver calls phy_stop_interrupts either.
> Please take a look at this.

I see. I'll check it.

> 
> > +	phy_disconnect(priv->phydev);
> > +}
> > +
> 
>   Andrew

---
Best Regards,
Kunihiko Hayashi

[toc] | [prev] | [next] | [standalone]

#1730325 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

> > > +static irqreturn_t ave_interrupt(int irq, void *netdev)
> > > +{
> > > +	struct net_device *ndev = (struct net_device *)netdev;
> > > +	struct ave_private *priv = netdev_priv(ndev);
> > > +	u32 gimr_val, gisr_val;
> > > +
> > > +	gimr_val = ave_irq_disable_all(ndev);
> > > +
> > > +	/* get interrupt status */
> > > +	gisr_val = ave_r32(ndev, AVE_GISR);
> > > +
> > > +	/* PHY */
> > > +	if (gisr_val & AVE_GI_PHY) {
> > > +		ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> > > +		if (priv->internal_phy_interrupt)
> > > +			phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
> > 
> > Humm. I don't think this is correct. You are supposed to give it the
> > new link state, not the old.
> > 
> > What does a PHY interrupt mean here? 
> 
> In the general case, I think PHY events like changing link state are transmitted
> to CPU as interrupt via interrupt controller, then PHY driver itself can handle
> the interrupt.
> 
> And in this case, PHY events are transmitted to MAC as one of its interrupt factor,
> then I thought that MAC driver had to tell the events to PHY.

Could this be in-band SGMI signalling from the PHY to the MAC? Does
the documentation give examples of when this interrupt will happen?

    Andrew

[toc] | [prev] | [next] | [standalone]

#1730772 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

Hi Andrew,

On Mon, 11 Sep 2017 14:00:09 +0200 Andrew Lunn <andrew@lunn.ch> wrote:

> > > > +static irqreturn_t ave_interrupt(int irq, void *netdev)
> > > > +{
> > > > +	struct net_device *ndev = (struct net_device *)netdev;
> > > > +	struct ave_private *priv = netdev_priv(ndev);
> > > > +	u32 gimr_val, gisr_val;
> > > > +
> > > > +	gimr_val = ave_irq_disable_all(ndev);
> > > > +
> > > > +	/* get interrupt status */
> > > > +	gisr_val = ave_r32(ndev, AVE_GISR);
> > > > +
> > > > +	/* PHY */
> > > > +	if (gisr_val & AVE_GI_PHY) {
> > > > +		ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> > > > +		if (priv->internal_phy_interrupt)
> > > > +			phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
> > > 
> > > Humm. I don't think this is correct. You are supposed to give it the
> > > new link state, not the old.
> > > 
> > > What does a PHY interrupt mean here? 
> > 
> > In the general case, I think PHY events like changing link state are transmitted
> > to CPU as interrupt via interrupt controller, then PHY driver itself can handle
> > the interrupt.
> > 
> > And in this case, PHY events are transmitted to MAC as one of its interrupt factor,
> > then I thought that MAC driver had to tell the events to PHY.
> 
> Could this be in-band SGMI signalling from the PHY to the MAC? Does
> the documentation give examples of when this interrupt will happen?
> 
>     Andrew

Unfortunately this MAC doesn't support SGMII.
And there aren't any examples of when this interrupt will happen.
This interrupt happens after ave_open() is called and link is established.

However, I found that auto negotiation didn't start when this interrupt wasn't handled.

Although ave_init() calls phy_start_aneg(), it doesn't make sense
because phy driver doesn't start yet.

And according to Florian's comment in ave_init(),

> +	phy_start_interrupts(phydev);
> +
> +	phy_start_aneg(phydev);
>
> No, no, phy_start() would take care of all of that correctly for you,
> you don't have have to do it just there because ave_open() eventually
> calls phy_start() for you, so just drop these two calls.

When moving phy_start_aneg() to ave_open(), the handler doesn't need to call
phy_mac_interrupt() and I can remove it from the handler.

---
Best Regards,
Kunihiko Hayashi

[toc] | [prev] | [next] | [standalone]

#1729003 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:

> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
> new file mode 100644
> index 0000000..788f26f
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/Kconfig
> @@ -0,0 +1,22 @@
> +config NET_VENDOR_SOCIONEXT
> +       bool "Socionext ethernet drivers"
> +       default y
> +       ---help---
> +         Option to select ethernet drivers for Socionext platforms.
> +
> +         Note that the answer to this question doesn't directly affect the
> +         kernel: saying N will just cause the configurator to skip all
> +         the questions about Agere devices. If you say Y, you will be asked
> +         for your specific card in the following questions.


Agere?



> +
> +       dev_info(dev, "Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\n",
> +                (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,
> +                (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,
> +                buf, ndev->irq, phy_modes(phy_mode));
> +
> +       return 0;
> +err_netdev_register:

Maybe, a bad label name.
for ex. out_del_napi or whatever.

Documentation/process/coding-style.rst says
"Choose label names which say what the goto does ..."


> +       netif_napi_del(&priv->napi_rx);
> +       netif_napi_del(&priv->napi_tx);
> +       mdiobus_unregister(priv->mdio);
> +err_mdiobus_register:
> +err_mdiobus_alloc:
> +err_req_irq:

These three should be merged, for ex.
out_free_device.



I ran sparse for you.

Please take a look if it is worthwhile.
Seems endianess around mac_addr.


drivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to
restricted __le16
drivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to
restricted __le32
drivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to
restricted __le16





-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1730170 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

Hi Yamada-san,
Thank you for your comments,

On Fri, 8 Sep 2017 23:44:13 +0900 <yamada.masahiro@socionext.com> wrote:

> 2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:
> 
> > diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
> > new file mode 100644
> > index 0000000..788f26f
> > --- /dev/null
> > +++ b/drivers/net/ethernet/socionext/Kconfig
> > @@ -0,0 +1,22 @@
> > +config NET_VENDOR_SOCIONEXT
> > +       bool "Socionext ethernet drivers"
> > +       default y
> > +       ---help---
> > +         Option to select ethernet drivers for Socionext platforms.
> > +
> > +         Note that the answer to this question doesn't directly affect the
> > +         kernel: saying N will just cause the configurator to skip all
> > +         the questions about Agere devices. If you say Y, you will be asked
> > +         for your specific card in the following questions.
> 
> 
> Agere?

It's wrong. I have template strings left.
I'll fix it.

> > +
> > +       dev_info(dev, "Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\n",
> > +                (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,
> > +                (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,
> > +                buf, ndev->irq, phy_modes(phy_mode));
> > +
> > +       return 0;
> > +err_netdev_register:
> 
> Maybe, a bad label name.
> for ex. out_del_napi or whatever.
> 
> Documentation/process/coding-style.rst says
> "Choose label names which say what the goto does ..."

Yes, I'll rename it according to the document.

> > +       netif_napi_del(&priv->napi_rx);
> > +       netif_napi_del(&priv->napi_tx);
> > +       mdiobus_unregister(priv->mdio);
> > +err_mdiobus_register:
> > +err_mdiobus_alloc:
> > +err_req_irq:
> 
> These three should be merged, for ex.
> out_free_device.

I see. These will be merged.

> I ran sparse for you.
> 
> Please take a look if it is worthwhile.
> Seems endianess around mac_addr.

Okay, I'll check the suspicious code.

> drivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to
> restricted __le32
> drivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to
> restricted __le16
> drivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to
> restricted __le32
> drivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to
> restricted __le16
> drivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to
> restricted __le32
> drivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to
> restricted __le16
> 
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

---
Best Regards,
Kunihiko Hayashi

[toc] | [prev] | [next] | [standalone]

#1729482 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> The UniPhier platform from Socionext provides the AVE ethernet
> controller that includes MAC and MDIO bus supporting RGMII/RMII
> modes. The controller is named AVE.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---

[snip]

> +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +	u32 proc_idx, done_idx, ndesc, cmdsts;
> +	int freepkt;
> +	unsigned char *buffptr = NULL; /* buffptr for descriptor */
> +	unsigned int len;
> +	dma_addr_t paddr;
> +
> +	proc_idx = priv->tx.proc_idx;
> +	done_idx = priv->tx.done_idx;
> +	ndesc = priv->tx.ndesc;
> +	freepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;
> +
> +	/* not enough entry, then we stop queue */
> +	if (unlikely(freepkt < 2)) {
> +		netif_stop_queue(ndev);
> +		if (unlikely(freepkt < 1))
> +			return NETDEV_TX_BUSY;

This looks wrong, why are you checking first for less than 2
descriptors, and if there is none, NETDEV_TX_BUSY? If you need 2 slots
to complete a transmision, stop the transmit queue and return
NETDEV_TX_BUSY.

> +	}
> +
> +	priv->tx.desc[proc_idx].skbs = skb;
> +
> +	/* add padding for short packet */
> +	if (skb_padto(skb, ETH_ZLEN)) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}

skb_padto() frees the SKB in case of error, that would lead to a double
free here.

> +
> +	buffptr = skb->data - NET_IP_ALIGN;
> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);

If you use skb_put_padto() if padding was necessary skb->len will be at
least ETH_ZLEN, so you can remove this.

> +
> +	paddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,
> +			    len + NET_IP_ALIGN, DMA_TO_DEVICE);

As mentioned before you can't assume this will never fail.

> +	paddr += NET_IP_ALIGN;
> +
> +	/* set buffer address to descriptor */
> +	ave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);

Also mentioned in the other email, make this 4 a constant so we know
it's an offset and not a length.

> +
> +	/* set flag and length to send */
> +	cmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST
> +		| (len & AVE_STS_PKTLEN_TX);

AVE_STS_PKTLEN_TX would be better named with a _MASK suffix.

> +
> +	/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */
> +	if (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))
> +		cmdsts |= AVE_STS_INTR;
> +
> +	/* disable checksum calculation when skb doesn't calurate checksum */
> +	if (skb->ip_summed == CHECKSUM_NONE ||
> +	    skb->ip_summed == CHECKSUM_UNNECESSARY)
> +		cmdsts |= AVE_STS_NOCSUM;
> +
> +	/* set cmdsts */
> +	ave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);
> +
> +	priv->tx.proc_idx = (proc_idx + 1) % ndesc;

You should also check the ring space after transmission and assert flow
control on the transmit queue if needed.

> +
> +	return NETDEV_TX_OK;
> +}

[snip]

> +static struct net_device_stats *ave_stats(struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +	u32 drop_num = 0;
> +
> +	priv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);
> +
> +	drop_num += ave_r32(ndev, AVE_RX0OVFFC);
> +	drop_num += ave_r32(ndev, AVE_SN5FC);
> +	drop_num += ave_r32(ndev, AVE_SN6FC);
> +	drop_num += ave_r32(ndev, AVE_SN7FC);
> +	priv->stats.rx_dropped = drop_num;
> +

You should consider switching to 64-bit statistics, this requires a
little bit more work for 32-bit hosts (see
include/linux/u64_stats_sync.h) but this allows you to keep statistics
around above 4GB.

> +	return &priv->stats;
> +}
> +-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1730174 — Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

Hi Florian,

On Sat, 9 Sep 2017 09:30:58 -0700 <f.fainelli@gmail.com> wrote:

> 
> 
> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> > The UniPhier platform from Socionext provides the AVE ethernet
> > controller that includes MAC and MDIO bus supporting RGMII/RMII
> > modes. The controller is named AVE.
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> 
> [snip]
> 
> > +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > +	struct ave_private *priv = netdev_priv(ndev);
> > +	u32 proc_idx, done_idx, ndesc, cmdsts;
> > +	int freepkt;
> > +	unsigned char *buffptr = NULL; /* buffptr for descriptor */
> > +	unsigned int len;
> > +	dma_addr_t paddr;
> > +
> > +	proc_idx = priv->tx.proc_idx;
> > +	done_idx = priv->tx.done_idx;
> > +	ndesc = priv->tx.ndesc;
> > +	freepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;
> > +
> > +	/* not enough entry, then we stop queue */
> > +	if (unlikely(freepkt < 2)) {
> > +		netif_stop_queue(ndev);
> > +		if (unlikely(freepkt < 1))
> > +			return NETDEV_TX_BUSY;
> 
> This looks wrong, why are you checking first for less than 2
> descriptors, and if there is none, NETDEV_TX_BUSY? If you need 2 slots
> to complete a transmision, stop the transmit queue and return
> NETDEV_TX_BUSY.

This code is misleading and I have to fix this.
The device needs a slot to complete a transmission.

> > +	}
> > +
> > +	priv->tx.desc[proc_idx].skbs = skb;
> > +
> > +	/* add padding for short packet */
> > +	if (skb_padto(skb, ETH_ZLEN)) {
> > +		dev_kfree_skb_any(skb);
> > +		return NETDEV_TX_OK;
> > +	}
> 
> skb_padto() frees the SKB in case of error, that would lead to a double
> free here.

Ah, it occurs double free. I'll fix it.

> > +
> > +	buffptr = skb->data - NET_IP_ALIGN;
> > +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
> 
> If you use skb_put_padto() if padding was necessary skb->len will be at
> least ETH_ZLEN, so you can remove this.

I see. It's reasonable.

> > +
> > +	paddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,
> > +			    len + NET_IP_ALIGN, DMA_TO_DEVICE);
> 
> As mentioned before you can't assume this will never fail.

Okay, I'll rewrite it in consideration of error case.

> > +	paddr += NET_IP_ALIGN;
> > +
> > +	/* set buffer address to descriptor */
> > +	ave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);
> 
> Also mentioned in the other email, make this 4 a constant so we know
> it's an offset and not a length.

I see.

> > +
> > +	/* set flag and length to send */
> > +	cmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST
> > +		| (len & AVE_STS_PKTLEN_TX);
> 
> AVE_STS_PKTLEN_TX would be better named with a _MASK suffix.

Yes.

> > +
> > +	/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */
> > +	if (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))
> > +		cmdsts |= AVE_STS_INTR;
> > +
> > +	/* disable checksum calculation when skb doesn't calurate checksum */
> > +	if (skb->ip_summed == CHECKSUM_NONE ||
> > +	    skb->ip_summed == CHECKSUM_UNNECESSARY)
> > +		cmdsts |= AVE_STS_NOCSUM;
> > +
> > +	/* set cmdsts */
> > +	ave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);
> > +
> > +	priv->tx.proc_idx = (proc_idx + 1) % ndesc;
> 
> You should also check the ring space after transmission and assert flow
> control on the transmit queue if needed.

Okay, I'll add this.

> > +
> > +	return NETDEV_TX_OK;
> > +}
> 
> [snip]
> 
> > +static struct net_device_stats *ave_stats(struct net_device *ndev)
> > +{
> > +	struct ave_private *priv = netdev_priv(ndev);
> > +	u32 drop_num = 0;
> > +
> > +	priv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);
> > +
> > +	drop_num += ave_r32(ndev, AVE_RX0OVFFC);
> > +	drop_num += ave_r32(ndev, AVE_SN5FC);
> > +	drop_num += ave_r32(ndev, AVE_SN6FC);
> > +	drop_num += ave_r32(ndev, AVE_SN7FC);
> > +	priv->stats.rx_dropped = drop_num;
> > +
> 
> You should consider switching to 64-bit statistics, this requires a
> little bit more work for 32-bit hosts (see
> include/linux/u64_stats_sync.h) but this allows you to keep statistics
> around above 4GB.

I see.
I'll refer to this header and its examples, and rewrite it to be suitable
for 32-bit and 64-bit hosts.

> 
> > +	return &priv->stats;
> > +}
> > +-- 
> Florian

---
Best Regards,
Kunihiko Hayashi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web