[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Marc Zyngier]

Enable the 1GB Ethernet interface that lives on the slave CP110,
with its corresponding phy (that oddly lives on the master CP110).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
index f7bb0cc03147..1eb3cd5332c1 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
@@ -100,6 +100,12 @@
 	status = "okay";
 };
 
+&cpm_mdio {
+	phy0: ethernet-phy at 0 {
+		reg = <0>;
+	};
+};
+
 &cpm_sata0 {
 	/* CPM Lane 0 - U29 */
 	status = "okay";
@@ -115,6 +121,16 @@
 	status = "okay";
 };
 
+&cps_ethernet {
+	status = "okay";
+};
+
+&cps_eth1 {
+	status = "okay";
+	phy = <&phy0>;
+	phy-mode = "sgmii";
+};
+
 &cps_sata0 {
 	/* CPS Lane 1 - U32 */
 	/* CPS Lane 3 - U31 */
-- 
2.11.0

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Andrew Lunn]

> +&cps_eth1 {
> +	status = "okay";
> +	phy = <&phy0>;
> +	phy-mode = "sgmii";
> +};

Hi Marc

Documentation/devicetree/bindings/net/ethernet.txt says:

- phy-handle: phandle, specifies a reference to a node representing a PHY
  device; this property is described in ePAPR and so preferred;
- phy: the same as "phy-handle" property, not recommended for new bindings.

  Andrew

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Marc Zyngier]

Hi Andrew,

On 05/06/17 22:16, Andrew Lunn wrote:
>> +&cps_eth1 {
>> +	status = "okay";
>> +	phy = <&phy0>;
>> +	phy-mode = "sgmii";
>> +};
> 
> Hi Marc
> 
> Documentation/devicetree/bindings/net/ethernet.txt says:
> 
> - phy-handle: phandle, specifies a reference to a node representing a PHY
>   device; this property is described in ePAPR and so preferred;
> - phy: the same as "phy-handle" property, not recommended for new bindings.

Happy to amend the patch, though armada-8040-db.dts is already using
"phy" (and not "phy-handle"). Should that one be fixed as well in the
name of consistency? Also,
Documentation/devicetree/bindings/net/marvell-pp2.txt only mentions the
"phy" property. If feels a bit odd not to at least recommend the new
property in the binding documentation.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Andrew Lunn]

On Tue, Jun 06, 2017 at 09:21:58AM +0100, Marc Zyngier wrote:
> Hi Andrew,
> 
> On 05/06/17 22:16, Andrew Lunn wrote:
> >> +&cps_eth1 {
> >> +	status = "okay";
> >> +	phy = <&phy0>;
> >> +	phy-mode = "sgmii";
> >> +};
> > 
> > Hi Marc
> > 
> > Documentation/devicetree/bindings/net/ethernet.txt says:
> > 
> > - phy-handle: phandle, specifies a reference to a node representing a PHY
> >   device; this property is described in ePAPR and so preferred;
> > - phy: the same as "phy-handle" property, not recommended for new bindings.
> 
> Happy to amend the patch, though armada-8040-db.dts is already using
> "phy" (and not "phy-handle"). Should that one be fixed as well in the
> name of consistency? Also,
> Documentation/devicetree/bindings/net/marvell-pp2.txt only mentions the
> "phy" property. If feels a bit odd not to at least recommend the new
> property in the binding documentation.

Ah. As Russell pointed out, parsing this property is done by the
driver, not a central helper. mvpp2 only supports phy, not phy-handle.
This is not a new binding, it has been around since the middle of
2014. So "phy" is O.K.

Sometime in the future, it would be nice to centralise the parsing of
this and phy-mode.

     Andrew

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Thomas Petazzoni]

Hello,

On Mon,  5 Jun 2017 21:01:07 +0100, Marc Zyngier wrote:
> Enable the 1GB Ethernet interface that lives on the slave CP110,
> with its corresponding phy (that oddly lives on the master CP110).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

This patch is somewhat redundant with
https://patchwork.kernel.org/patch/9728207/, which despite the flaming
discussion that followed, does the same thing (and more).

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Thomas Petazzoni]

Hello,

On Tue, 6 Jun 2017 11:02:34 +0200, Thomas Petazzoni wrote:

> This patch is somewhat redundant with
> https://patchwork.kernel.org/patch/9728207/, which despite the flaming
> discussion that followed, does the same thing (and more).

That being said, the patch from Marcin was also enabling SDHCI, which
has already been enabled in the mean time by a patch from Russell.

So perhaps we should indeed keep Marc's patch, enabling just the 1GB
Ethernet interface.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Marc Zyngier]

On Tue, Jun 06 2017 at 11:06:03 am BST, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Tue, 6 Jun 2017 11:02:34 +0200, Thomas Petazzoni wrote:
>
>> This patch is somewhat redundant with
>> https://patchwork.kernel.org/patch/9728207/, which despite the flaming
>> discussion that followed, does the same thing (and more).
>
> That being said, the patch from Marcin was also enabling SDHCI, which
> has already been enabled in the mean time by a patch from Russell.
>
> So perhaps we should indeed keep Marc's patch, enabling just the 1GB
> Ethernet interface.

Whichever patch we merge, Andrew's comment remains. Should we stick with
"phy", or switch to "phy-handle" everywhere?

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Thomas Petazzoni]

Hello,

On Tue, 06 Jun 2017 10:18:11 +0100, Marc Zyngier wrote:

> > So perhaps we should indeed keep Marc's patch, enabling just the 1GB
> > Ethernet interface.  
> 
> Whichever patch we merge, Andrew's comment remains. Should we stick with
> "phy", or switch to "phy-handle" everywhere?

Since the binding says to use phy-handle, we should use phy-handle for
your patch. Of course, progressively, we should migrate all our
bindings to this new property name, but I don't think this should be a
prerequisite for your patch to go in.

Side note: I don't understand this change from "phy" to "phy-handle".
It really doesn't make sense to me to encode the type of the property
in the name of the property. Are we going to use:

  phy-mode-string = "sgmii";
  pinctrl-0-handle = <&foo>;
  reg-u32s = <0xdeadbeef 0xbadcafe>;

is there a pointer to the reasoning for this change?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Russell King - ARM Linux]

On Tue, Jun 06, 2017 at 11:36:26AM +0200, Thomas Petazzoni wrote:
> Side note: I don't understand this change from "phy" to "phy-handle".
> It really doesn't make sense to me to encode the type of the property
> in the name of the property. Are we going to use:

ePAPR 1.1 has specified phy-handle, which as the binding document says
is the preferred version (probably because it's in ePAPR as the official
name of the property.)

We've ended up with "phy" and "phy-handle" because there was never a
central binding document for ethernet devices until 2014, so drivers
randomly picked between the two property names.  Which one should be
used is driver dependent, since drivers parse this property.  Many
drivers parse just one of the names and not the other, so you have to
use the right one for the driver.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Marc Zyngier]

On Tue, Jun 06 2017 at 11:02:34 am BST, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Mon,  5 Jun 2017 21:01:07 +0100, Marc Zyngier wrote:
>> Enable the 1GB Ethernet interface that lives on the slave CP110,
>> with its corresponding phy (that oddly lives on the master CP110).
>> 
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>
> This patch is somewhat redundant with
> https://patchwork.kernel.org/patch/9728207/, which despite the flaming
> discussion that followed, does the same thing (and more).

It's great that there is a patch for that already. How far is that from
being merged (given how much controversy seem to surround this)?

Thanks,

	
-- 
Jazz is not dead, it just smell funny.

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Thomas Petazzoni]

Hello,

On Tue, 06 Jun 2017 10:16:08 +0100, Marc Zyngier wrote:

> > This patch is somewhat redundant with
> > https://patchwork.kernel.org/patch/9728207/, which despite the flaming
> > discussion that followed, does the same thing (and more).  
> 
> It's great that there is a patch for that already. How far is that from
> being merged (given how much controversy seem to surround this)?

As I said in my second reply, I believe a v2 of your patch would in
fact be better.

Marcin's patch is no longer applicable, and was doing too many things
at once. So let's merge your patch instead.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Thomas Petazzoni]

Hello,

On Mon,  5 Jun 2017 21:01:07 +0100, Marc Zyngier wrote:
> Enable the 1GB Ethernet interface that lives on the slave CP110,
> with its corresponding phy (that oddly lives on the master CP110).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Russell King - ARM Linux]

On Mon, Jun 05, 2017 at 09:01:07PM +0100, Marc Zyngier wrote:
> Enable the 1GB Ethernet interface that lives on the slave CP110,
> with its corresponding phy (that oddly lives on the master CP110).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> index f7bb0cc03147..1eb3cd5332c1 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> @@ -100,6 +100,12 @@
>  	status = "okay";
>  };
>  
> +&cpm_mdio {
> +	phy0: ethernet-phy at 0 {

As the board has multiple phys, can we use a more descriptive label
here, such as "ge_phy" (for gigabit ethernet phy) to distinugish it
from the 10G PHYs?

> +&cps_ethernet {
> +	status = "okay";
> +};
> +
> +&cps_eth1 {

Labelling up the connector as is done elsewhere in the file would be
nice:

	/* CPS Lane 0 - J5 (Gigabit RJ45) */


> +	status = "okay";
> +	phy = <&phy0>;
> +	phy-mode = "sgmii";

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: dts: marvell: 8040-mcbin: Enable 1GB Ethernet

[Marc Zyngier]

On 06/06/17 14:34, Russell King - ARM Linux wrote:
> On Mon, Jun 05, 2017 at 09:01:07PM +0100, Marc Zyngier wrote:
>> Enable the 1GB Ethernet interface that lives on the slave CP110,
>> with its corresponding phy (that oddly lives on the master CP110).
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
>> index f7bb0cc03147..1eb3cd5332c1 100644
>> --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
>> +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
>> @@ -100,6 +100,12 @@
>>  	status = "okay";
>>  };
>>  
>> +&cpm_mdio {
>> +	phy0: ethernet-phy at 0 {
> 
> As the board has multiple phys, can we use a more descriptive label
> here, such as "ge_phy" (for gigabit ethernet phy) to distinugish it
> from the 10G PHYs?
> 
>> +&cps_ethernet {
>> +	status = "okay";
>> +};
>> +
>> +&cps_eth1 {
> 
> Labelling up the connector as is done elsewhere in the file would be
> nice:
> 
> 	/* CPS Lane 0 - J5 (Gigabit RJ45) */
> 
> 
>> +	status = "okay";
>> +	phy = <&phy0>;
>> +	phy-mode = "sgmii";
> 

Yup, agreed on both counts. I'll update the patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...