| Message ID | 20170918075814.27405-6-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Antoine, On Mon, Sep 18, 2017 at 09:58:10AM +0200, Antoine Tenart wrote: > This patch adds comphy phandles to the Ethernet ports in the mcbin > device tree. The comphy is used to configure the serdes PHYs used by > these ports. > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > index acf5c7d16d79..9a3b4a1fd445 100644 > --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > @@ -223,6 +223,7 @@ > &cpm_eth0 { > status = "okay"; > phy = <&phy0>; > + phys = <&cpm_comphy4 0>; > phy-mode = "10gbase-kr"; Since the 'phys' generic PHY property is unrelated to the 'phy*' Ethernet PHY properties that surround it, its location might be confusing. I think it should appear below those two other properties like you did below ... > }; > > @@ -258,6 +259,7 @@ > &cps_eth0 { > status = "okay"; > phy = <&phy8>; > + phys = <&cps_comphy4 0>; > phy-mode = "10gbase-kr"; ... not here ... > }; > > @@ -266,6 +268,7 @@ > status = "okay"; > phy = <&ge_phy>; > phy-mode = "sgmii"; > + phys = <&cps_comphy0 1>; ... but here. See also upstream commits 9a94b3a4bdfdb40 (dt-binding: phy: don't confuse with Ethernet phy properties) and c43593d81ab59b (dt-bindings: net: don't confuse with generic PHY property). > }; > > &cps_pinctrl { baruch
Hi Baruch, On Mon, Sep 18, 2017 at 11:50:12AM +0300, Baruch Siach wrote: > On Mon, Sep 18, 2017 at 09:58:10AM +0200, Antoine Tenart wrote: > > This patch adds comphy phandles to the Ethernet ports in the mcbin > > device tree. The comphy is used to configure the serdes PHYs used by > > these ports. > > > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > > --- > > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > index acf5c7d16d79..9a3b4a1fd445 100644 > > --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > @@ -223,6 +223,7 @@ > > &cpm_eth0 { > > status = "okay"; > > phy = <&phy0>; > > + phys = <&cpm_comphy4 0>; > > phy-mode = "10gbase-kr"; > > Since the 'phys' generic PHY property is unrelated to the 'phy*' Ethernet PHY > properties that surround it, its location might be confusing. I think it > should appear below those two other properties like you did below ... > > > }; > > > > @@ -258,6 +259,7 @@ > > &cps_eth0 { > > status = "okay"; > > phy = <&phy8>; > > + phys = <&cps_comphy4 0>; > > phy-mode = "10gbase-kr"; > > ... not here ... > > > }; > > > > @@ -266,6 +268,7 @@ > > status = "okay"; > > phy = <&ge_phy>; > > phy-mode = "sgmii"; > > + phys = <&cps_comphy0 1>; > > ... but here. OK, I'll update. Thanks, Antoine
On Mon, Sep 18, 2017 at 11:50:12AM +0300, Baruch Siach wrote: > Hi Antoine, > > On Mon, Sep 18, 2017 at 09:58:10AM +0200, Antoine Tenart wrote: > > This patch adds comphy phandles to the Ethernet ports in the mcbin > > device tree. The comphy is used to configure the serdes PHYs used by > > these ports. > > > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > > --- > > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > index acf5c7d16d79..9a3b4a1fd445 100644 > > --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > @@ -223,6 +223,7 @@ > > &cpm_eth0 { > > status = "okay"; > > phy = <&phy0>; > > + phys = <&cpm_comphy4 0>; > > phy-mode = "10gbase-kr"; > > Since the 'phys' generic PHY property is unrelated to the 'phy*' Ethernet PHY > properties that surround it, its location might be confusing. I think it > should appear below those two other properties like you did below ... Hi Baruch, Antoine Yes, i agree. It is confusing enough as it is, so we should not make it worse. I even wonder if it is worth doing some renaming? phy = <ðernet_phy0>; phy-mode = "10gbase-kr"; phys = <&cpm_generic_comphy4 0>; Andrew
Hi Andrew, On Mon, Sep 18, 2017 at 03:04:37PM +0200, Andrew Lunn wrote: > On Mon, Sep 18, 2017 at 11:50:12AM +0300, Baruch Siach wrote: > > On Mon, Sep 18, 2017 at 09:58:10AM +0200, Antoine Tenart wrote: > > > This patch adds comphy phandles to the Ethernet ports in the mcbin > > > device tree. The comphy is used to configure the serdes PHYs used by > > > these ports. > > > > > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > > > --- > > > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > > index acf5c7d16d79..9a3b4a1fd445 100644 > > > --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > > +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts > > > @@ -223,6 +223,7 @@ > > > &cpm_eth0 { > > > status = "okay"; > > > phy = <&phy0>; > > > + phys = <&cpm_comphy4 0>; > > > phy-mode = "10gbase-kr"; > > > > Since the 'phys' generic PHY property is unrelated to the 'phy*' Ethernet PHY > > properties that surround it, its location might be confusing. I think it > > should appear below those two other properties like you did below ... > > Yes, i agree. It is confusing enough as it is, so we should not make > it worse. > > I even wonder if it is worth doing some renaming? > > phy = <ðernet_phy0>; > phy-mode = "10gbase-kr"; > phys = <&cpm_generic_comphy4 0>; We could also add an extra comment without renaming the comphy node, something like: /* Network PHY */ phy = <ðernet_phy0>; phy-mode = "10gbase-kr"; /* Generic PHY, providing serdes lanes */ phys = <&cpm_comphy4 0>; Thanks, Antoine
> We could also add an extra comment without renaming the comphy node, something > like: > > /* Network PHY */ > phy = <ðernet_phy0>; > phy-mode = "10gbase-kr"; > /* Generic PHY, providing serdes lanes */ > phys = <&cpm_comphy4 0>; Yes, that is good. Thanks Andrew
diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts index acf5c7d16d79..9a3b4a1fd445 100644 --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts @@ -223,6 +223,7 @@ &cpm_eth0 { status = "okay"; phy = <&phy0>; + phys = <&cpm_comphy4 0>; phy-mode = "10gbase-kr"; }; @@ -258,6 +259,7 @@ &cps_eth0 { status = "okay"; phy = <&phy8>; + phys = <&cps_comphy4 0>; phy-mode = "10gbase-kr"; }; @@ -266,6 +268,7 @@ status = "okay"; phy = <&ge_phy>; phy-mode = "sgmii"; + phys = <&cps_comphy0 1>; }; &cps_pinctrl {
This patch adds comphy phandles to the Ethernet ports in the mcbin device tree. The comphy is used to configure the serdes PHYs used by these ports. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 3 +++ 1 file changed, 3 insertions(+)