powerpc/kmcent2: update the ethernet devices' phy properties
diff mbox series

Message ID 20190714200501.1276-1-valentin@longchamp.me
State New
Delegated to: Scott Wood
Headers show
Series
  • powerpc/kmcent2: update the ethernet devices' phy properties
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 31 lines checked
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/apply_patch success Successfully applied on branch next (f5c20693d8edcd665f1159dc941b9e7f87c17647)

Commit Message

Valentin Longchamp July 14, 2019, 8:05 p.m. UTC
Change all phy-connection-type properties to phy-mode that are better
supported by the fman driver.

Use the more readable fixed-link node for the 2 sgmii links.

Change the RGMII link to rgmii-id as the clock delays are added by the
phy.

Signed-off-by: Valentin Longchamp <valentin@longchamp.me>
---
 arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Valentin Longchamp July 28, 2019, 4:01 p.m. UTC | #1
Hi Scott, Kumar,

Looking at this patch I have realised that I had already submitted it
to the mailing list nearly 2 years ago:
https://patchwork.ozlabs.org/patch/842944/

Could you please make sure that this one gets merged in the next
window, so that I avoid forgetting such a patch a 2nd time ?

Thanks a lot

Le dim. 14 juil. 2019 à 22:05, Valentin Longchamp
<valentin@longchamp.me> a écrit :
>
> Change all phy-connection-type properties to phy-mode that are better
> supported by the fman driver.
>
> Use the more readable fixed-link node for the 2 sgmii links.
>
> Change the RGMII link to rgmii-id as the clock delays are added by the
> phy.
>
> Signed-off-by: Valentin Longchamp <valentin@longchamp.me>
> ---
>  arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> index 48b7f9797124..c3e0741cafb1 100644
> --- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> +++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> @@ -210,13 +210,19 @@
>
>                 fman@400000 {
>                         ethernet@e0000 {
> -                               fixed-link = <0 1 1000 0 0>;
> -                               phy-connection-type = "sgmii";
> +                               phy-mode = "sgmii";
> +                               fixed-link {
> +                                       speed = <1000>;
> +                                       full-duplex;
> +                               };
>                         };
>
>                         ethernet@e2000 {
> -                               fixed-link = <1 1 1000 0 0>;
> -                               phy-connection-type = "sgmii";
> +                               phy-mode = "sgmii";
> +                               fixed-link {
> +                                       speed = <1000>;
> +                                       full-duplex;
> +                               };
>                         };
>
>                         ethernet@e4000 {
> @@ -229,7 +235,7 @@
>
>                         ethernet@e8000 {
>                                 phy-handle = <&front_phy>;
> -                               phy-connection-type = "rgmii";
> +                               phy-mode = "rgmii-id";
>                         };
>
>                         mdio0: mdio@fc000 {
> --
> 2.17.1
>
Scott Wood July 28, 2019, 7:26 p.m. UTC | #2
On Sun, 2019-07-28 at 18:01 +0200, Valentin Longchamp wrote:
> Hi Scott, Kumar,
> 
> Looking at this patch I have realised that I had already submitted it
> to the mailing list nearly 2 years ago:
> https://patchwork.ozlabs.org/patch/842944/
> 
> Could you please make sure that this one gets merged in the next
> window, so that I avoid forgetting such a patch a 2nd time ?
> 
> Thanks a lot

I added it to my patchwork todo list; thanks for the reminder.

> Le dim. 14 juil. 2019 à 22:05, Valentin Longchamp
> <valentin@longchamp.me> a écrit :
> > 
> > Change all phy-connection-type properties to phy-mode that are better
> > supported by the fman driver.
> > 
> > Use the more readable fixed-link node for the 2 sgmii links.
> > 
> > Change the RGMII link to rgmii-id as the clock delays are added by the
> > phy.
> > 
> > Signed-off-by: Valentin Longchamp <valentin@longchamp.me>

I don't see any other uses of phy-mode in arch/powerpc/boot/dts/fsl, and I see
lots of phy-connection-type with fman.  Madalin, does this patch look OK?

-Scott

> > ---
> >  arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > index 48b7f9797124..c3e0741cafb1 100644
> > --- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > +++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > @@ -210,13 +210,19 @@
> > 
> >                 fman@400000 {
> >                         ethernet@e0000 {
> > -                               fixed-link = <0 1 1000 0 0>;
> > -                               phy-connection-type = "sgmii";
> > +                               phy-mode = "sgmii";
> > +                               fixed-link {
> > +                                       speed = <1000>;
> > +                                       full-duplex;
> > +                               };
> >                         };
> > 
> >                         ethernet@e2000 {
> > -                               fixed-link = <1 1 1000 0 0>;
> > -                               phy-connection-type = "sgmii";
> > +                               phy-mode = "sgmii";
> > +                               fixed-link {
> > +                                       speed = <1000>;
> > +                                       full-duplex;
> > +                               };
> >                         };
> > 
> >                         ethernet@e4000 {
> > @@ -229,7 +235,7 @@
> > 
> >                         ethernet@e8000 {
> >                                 phy-handle = <&front_phy>;
> > -                               phy-connection-type = "rgmii";
> > +                               phy-mode = "rgmii-id";
> >                         };
> > 
> >                         mdio0: mdio@fc000 {
> > --
> > 2.17.1
> > 
> 
>
Valentin Longchamp July 28, 2019, 8:26 p.m. UTC | #3
Le dim. 28 juil. 2019 à 21:26, Scott Wood <oss@buserror.net> a écrit :
> On Sun, 2019-07-28 at 18:01 +0200, Valentin Longchamp wrote:
> > Le dim. 14 juil. 2019 à 22:05, Valentin Longchamp
> > <valentin@longchamp.me> a écrit :
> > >
> > > Change all phy-connection-type properties to phy-mode that are better
> > > supported by the fman driver.
> > >
> > > Use the more readable fixed-link node for the 2 sgmii links.
> > >
> > > Change the RGMII link to rgmii-id as the clock delays are added by the
> > > phy.
> > >
> > > Signed-off-by: Valentin Longchamp <valentin@longchamp.me>
>
> I don't see any other uses of phy-mode in arch/powerpc/boot/dts/fsl, and I see
> lots of phy-connection-type with fman.  Madalin, does this patch look OK?

The fman driver (mac_probe()) calls of_get_phy_mode() which first
looks for phy-mode, and then phy-connection-type. Both should be the
same according to the device tree binding.

With some older kernels I remember we had issues with
phy-connection-type but not phy-mode, but this is more than 2 years
ago, I don't remember the details. phy-mode works well (tested ~2
weeks ago) with 4.14, 4.19 and 5.2, for sure.

Valentin

>
> -Scott
>
> > > ---
> > >  arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > index 48b7f9797124..c3e0741cafb1 100644
> > > --- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > +++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > @@ -210,13 +210,19 @@
> > >
> > >                 fman@400000 {
> > >                         ethernet@e0000 {
> > > -                               fixed-link = <0 1 1000 0 0>;
> > > -                               phy-connection-type = "sgmii";
> > > +                               phy-mode = "sgmii";
> > > +                               fixed-link {
> > > +                                       speed = <1000>;
> > > +                                       full-duplex;
> > > +                               };
> > >                         };
> > >
> > >                         ethernet@e2000 {
> > > -                               fixed-link = <1 1 1000 0 0>;
> > > -                               phy-connection-type = "sgmii";
> > > +                               phy-mode = "sgmii";
> > > +                               fixed-link {
> > > +                                       speed = <1000>;
> > > +                                       full-duplex;
> > > +                               };
> > >                         };
> > >
> > >                         ethernet@e4000 {
> > > @@ -229,7 +235,7 @@
> > >
> > >                         ethernet@e8000 {
> > >                                 phy-handle = <&front_phy>;
> > > -                               phy-connection-type = "rgmii";
> > > +                               phy-mode = "rgmii-id";
> > >                         };
> > >
> > >                         mdio0: mdio@fc000 {
> > > --
> > > 2.17.1
> > >
> >
> >
>
Madalin-cristian Bucur July 30, 2019, 9:44 a.m. UTC | #4
> -----Original Message-----
> From: Scott Wood <oss@buserror.net>
> Sent: Sunday, July 28, 2019 10:27 PM
> To: Valentin Longchamp <valentin@longchamp.me>; linuxppc-
> dev@lists.ozlabs.org; galak@kernel.crashing.org
> Cc: Madalin-cristian Bucur <madalin.bucur@nxp.com>
> Subject: Re: [PATCH] powerpc/kmcent2: update the ethernet devices' phy
> properties
> 
> On Sun, 2019-07-28 at 18:01 +0200, Valentin Longchamp wrote:
> > Hi Scott, Kumar,
> >
> > Looking at this patch I have realised that I had already submitted it
> > to the mailing list nearly 2 years ago:
> >
> > https://patchwork.ozlabs.org/patch/842944/
> >
> > Could you please make sure that this one gets merged in the next
> > window, so that I avoid forgetting such a patch a 2nd time ?
> >
> > Thanks a lot
> 
> I added it to my patchwork todo list; thanks for the reminder.
> 
> > Le dim. 14 juil. 2019 à 22:05, Valentin Longchamp
> > <valentin@longchamp.me> a écrit :
> > >
> > > Change all phy-connection-type properties to phy-mode that are better
> > > supported by the fman driver.
> > >
> > > Use the more readable fixed-link node for the 2 sgmii links.
> > >
> > > Change the RGMII link to rgmii-id as the clock delays are added by the
> > > phy.
> > >
> > > Signed-off-by: Valentin Longchamp <valentin@longchamp.me>
> 
> I don't see any other uses of phy-mode in arch/powerpc/boot/dts/fsl, and I see
> lots of phy-connection-type with fman.  Madalin, does this patch look OK?
> 
> -Scott

Hi,

we are using "phy-connection-type" not "phy-mode" for the NXP (former Freescale)
DPAA platforms. While the two seem to be interchangeable ("phy-mode" seems to be
more recent, looking at the device tree bindings), the driver code in Linux seems
to use one or the other, not both so one should stick with the variant the driver
is using. To make things more complex, there may be dependencies in bootloaders,
I see code in u-boot using only "phy-connection-type" or only "phy-mode".

I'd leave "phy-connection-type" as is.

Madalin

> > > ---
> > >  arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > index 48b7f9797124..c3e0741cafb1 100644
> > > --- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > +++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > @@ -210,13 +210,19 @@
> > >
> > >                 fman@400000 {
> > >                         ethernet@e0000 {
> > > -                               fixed-link = <0 1 1000 0 0>;
> > > -                               phy-connection-type = "sgmii";
> > > +                               phy-mode = "sgmii";
> > > +                               fixed-link {
> > > +                                       speed = <1000>;
> > > +                                       full-duplex;
> > > +                               };
> > >                         };
> > >
> > >                         ethernet@e2000 {
> > > -                               fixed-link = <1 1 1000 0 0>;
> > > -                               phy-connection-type = "sgmii";
> > > +                               phy-mode = "sgmii";
> > > +                               fixed-link {
> > > +                                       speed = <1000>;
> > > +                                       full-duplex;
> > > +                               };
> > >                         };
> > >
> > >                         ethernet@e4000 {
> > > @@ -229,7 +235,7 @@
> > >
> > >                         ethernet@e8000 {
> > >                                 phy-handle = <&front_phy>;
> > > -                               phy-connection-type = "rgmii";
> > > +                               phy-mode = "rgmii-id";
> > >                         };
> > >
> > >                         mdio0: mdio@fc000 {
> > > --
> > > 2.17.1
> > >
> >
> >
Valentin Longchamp Aug. 8, 2019, 9:09 p.m. UTC | #5
Le mar. 30 juil. 2019 à 11:44, Madalin-cristian Bucur
<madalin.bucur@nxp.com> a écrit :
>
> > -----Original Message-----
> >
> > > Le dim. 14 juil. 2019 à 22:05, Valentin Longchamp
> > > <valentin@longchamp.me> a écrit :
> > > >
> > > > Change all phy-connection-type properties to phy-mode that are better
> > > > supported by the fman driver.
> > > >
> > > > Use the more readable fixed-link node for the 2 sgmii links.
> > > >
> > > > Change the RGMII link to rgmii-id as the clock delays are added by the
> > > > phy.
> > > >
> > > > Signed-off-by: Valentin Longchamp <valentin@longchamp.me>
> >
> > I don't see any other uses of phy-mode in arch/powerpc/boot/dts/fsl, and I see
> > lots of phy-connection-type with fman.  Madalin, does this patch look OK?
> >
> > -Scott
>
> Hi,
>
> we are using "phy-connection-type" not "phy-mode" for the NXP (former Freescale)
> DPAA platforms. While the two seem to be interchangeable ("phy-mode" seems to be
> more recent, looking at the device tree bindings), the driver code in Linux seems
> to use one or the other, not both so one should stick with the variant the driver
> is using. To make things more complex, there may be dependencies in bootloaders,
> I see code in u-boot using only "phy-connection-type" or only "phy-mode".
>
> I'd leave "phy-connection-type" as is.

So I have finally had time to have a look and now I understand what
happens. You are right, there are bootloader dependencies: u-boot
calls fdt_fixup_phy_connection() that somehow in our case adds (or
changes if already in the device tree) the phy-connection-type
property to a wrong value ! By having a phy-mode in the device tree,
that is not changed by u-boot and by chance picked up by the kernel
fman driver (of_get_phy_mode() ) over phy-connection-mode, the below
patch fixes it for us.

I agree with you, it's not correct to have both phy-connection-type
and phy-mode. Ideally, u-boot on the board should be reworked so that
it does not perform the above wrong fixup. However, in an "unfixed"
.dtb (I have disabled fdt_fixup_phy_connection), the device tree in
the end only has either phy-connection-type or phy-mode, according to
what was chosen in the .dts file. And the fman driver works well with
both (thanks to the call to of_get_phy_mode() ). I would therefore
argue that even if all other DPAA platforms use phy-connection-type,
phy-mode is valid as well. (Furthermore we already have hundreds of
such boards in the field and we don't really support "remote" u-boot
update, so the u-boot fix is going to be difficult for us to pull).

Valentin

>
> Madalin
>
> > > > ---
> > > >  arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
> > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > > b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > > index 48b7f9797124..c3e0741cafb1 100644
> > > > --- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > > +++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > > @@ -210,13 +210,19 @@
> > > >
> > > >                 fman@400000 {
> > > >                         ethernet@e0000 {
> > > > -                               fixed-link = <0 1 1000 0 0>;
> > > > -                               phy-connection-type = "sgmii";
> > > > +                               phy-mode = "sgmii";
> > > > +                               fixed-link {
> > > > +                                       speed = <1000>;
> > > > +                                       full-duplex;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ethernet@e2000 {
> > > > -                               fixed-link = <1 1 1000 0 0>;
> > > > -                               phy-connection-type = "sgmii";
> > > > +                               phy-mode = "sgmii";
> > > > +                               fixed-link {
> > > > +                                       speed = <1000>;
> > > > +                                       full-duplex;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ethernet@e4000 {
> > > > @@ -229,7 +235,7 @@
> > > >
> > > >                         ethernet@e8000 {
> > > >                                 phy-handle = <&front_phy>;
> > > > -                               phy-connection-type = "rgmii";
> > > > +                               phy-mode = "rgmii-id";
> > > >                         };
> > > >
> > > >                         mdio0: mdio@fc000 {
> > > > --
> > > > 2.17.1
> > > >
> > >
> > >
>

Patch
diff mbox series

diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts b/arch/powerpc/boot/dts/fsl/kmcent2.dts
index 48b7f9797124..c3e0741cafb1 100644
--- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
+++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
@@ -210,13 +210,19 @@ 
 
 		fman@400000 {
 			ethernet@e0000 {
-				fixed-link = <0 1 1000 0 0>;
-				phy-connection-type = "sgmii";
+				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			ethernet@e2000 {
-				fixed-link = <1 1 1000 0 0>;
-				phy-connection-type = "sgmii";
+				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			ethernet@e4000 {
@@ -229,7 +235,7 @@ 
 
 			ethernet@e8000 {
 				phy-handle = <&front_phy>;
-				phy-connection-type = "rgmii";
+				phy-mode = "rgmii-id";
 			};
 
 			mdio0: mdio@fc000 {