[PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[René van Dorst]

This patch the removes the recently added mediatek,physpeed property.
Use the fixed-link property speed = <2500> to set the phy in 2.5Gbit.
See mt7622-bananapi-bpi-r64.dts for a working example.

Signed-off-by: René van Dorst <opensource@vdorst.com>
Tested-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../arm/mediatek/mediatek,sgmiisys.txt        |  2 --
 .../dts/mediatek/mt7622-bananapi-bpi-r64.dts  | 28 +++++++++++++------
 arch/arm64/boot/dts/mediatek/mt7622.dtsi      |  1 -
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
index f5518f26a914..30cb645c0e54 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt
@@ -9,8 +9,6 @@ Required Properties:
 	- "mediatek,mt7622-sgmiisys", "syscon"
 	- "mediatek,mt7629-sgmiisys", "syscon"
 - #clock-cells: Must be 1
-- mediatek,physpeed: Should be one of "auto", "1000" or "2500" to match up
-		     the capability of the target PHY.
 
 The SGMIISYS controller uses the common clk binding from
 Documentation/devicetree/bindings/clock/clock-bindings.txt
diff --git a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
index 710c5c3d87d3..2605ab3bc7ff 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
+++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
@@ -115,24 +115,34 @@
 };
 
 &eth {
-	pinctrl-names = "default";
-	pinctrl-0 = <&eth_pins>;
 	status = "okay";
+	gmac0: mac@0 {
+		compatible = "mediatek,eth-mac";
+		reg = <0>;
+		phy-mode = "sgmii";
+
+		fixed-link {
+			speed = <2500>;
+			full-duplex;
+			pause;
+		};
+	};
 
 	gmac1: mac@1 {
 		compatible = "mediatek,eth-mac";
 		reg = <1>;
-		phy-handle = <&phy5>;
+		phy-mode = "rgmii";
+
+		fixed-link {
+			speed = <1000>;
+			full-duplex;
+			pause;
+		};
 	};
 
-	mdio-bus {
+	mdio: mdio-bus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-
-		phy5: ethernet-phy@5 {
-			reg = <5>;
-			phy-mode = "sgmii";
-		};
 	};
 };
 
diff --git a/arch/arm64/boot/dts/mediatek/mt7622.dtsi b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
index d1e13d340e26..dac51e98204c 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
@@ -931,6 +931,5 @@
 			     "syscon";
 		reg = <0 0x1b128000 0 0x3000>;
 		#clock-cells = <1>;
-		mediatek,physpeed = "2500";
 	};
 };
-- 
2.20.1

Re: [PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[Andrew Lunn]

> +	gmac0: mac@0 {
> +		compatible = "mediatek,eth-mac";
> +		reg = <0>;
> +		phy-mode = "sgmii";
> +
> +		fixed-link {
> +			speed = <2500>;
> +			full-duplex;
> +			pause;
> +		};
> +	};

Hi René

SGMII and fixed-link is rather odd. Why do you need this combination?

      Andrew

Re: [PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[René van Dorst]

Quoting Andrew Lunn <andrew@lunn.ch>:

>> +	gmac0: mac@0 {
>> +		compatible = "mediatek,eth-mac";
>> +		reg = <0>;
>> +		phy-mode = "sgmii";
>> +
>> +		fixed-link {
>> +			speed = <2500>;
>> +			full-duplex;
>> +			pause;
>> +		};
>> +	};
>
> Hi René
>

Hi Andrew,

> SGMII and fixed-link is rather odd. Why do you need this combination?

BananaPi R64 has a RTL8367S 5+2-port switch, switch interfaces with  
the SOC by a
(H)SGMII and/or RGMII interface. SGMII is mainly used for the LAN ports and
RGMII for the WAN port.

I mimic the SDK software which puts SGMII interface in 2.5GBit  
fixed-link mode.
The RTL8367S switch code also put switch mac in forge 2.5GBit mode.

So this is the reason why I put a fixed-link mode here.

Greats,

René

>       Andrew

Re: [PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[Andrew Lunn]

On Fri, Jul 26, 2019 at 07:19:56AM +0000, René van Dorst wrote:
> Quoting Andrew Lunn <andrew@lunn.ch>:
> 
> >>+	gmac0: mac@0 {
> >>+		compatible = "mediatek,eth-mac";
> >>+		reg = <0>;
> >>+		phy-mode = "sgmii";
> >>+
> >>+		fixed-link {
> >>+			speed = <2500>;
> >>+			full-duplex;
> >>+			pause;
> >>+		};
> >>+	};
> >
> >Hi René
> >
> 
> Hi Andrew,
> 
> >SGMII and fixed-link is rather odd. Why do you need this combination?
> 
> BananaPi R64 has a RTL8367S 5+2-port switch, switch interfaces with the SOC
> by a
> (H)SGMII and/or RGMII interface. SGMII is mainly used for the LAN ports and
> RGMII for the WAN port.
> 
> I mimic the SDK software which puts SGMII interface in 2.5GBit fixed-link
> mode.
> The RTL8367S switch code also put switch mac in forge 2.5GBit mode.
> 
> So this is the reason why I put a fixed-link mode here.

Are you sure it is using SGMII and not 2500BaseX? Can you get access
to the signalling word? SGMII is supposed to indicate to the MAC what
speed it is using, via inband signalling. So there should not be any
need for a fixed-link. 2500BaseX however does not have such
signalling, so there would need to be a fixed link.

Maybe we should really consider what phy-mode = "sgmii"; means. Should
this include the overclocked 2.5G speed, or should we add a 2500sgmii
link mode?

     Andrew

Re: [PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[Russell King - ARM Linux admin]

On Fri, Jul 26, 2019 at 03:16:04PM +0200, Andrew Lunn wrote:
> Are you sure it is using SGMII and not 2500BaseX? Can you get access
> to the signalling word? SGMII is supposed to indicate to the MAC what
> speed it is using, via inband signalling. So there should not be any
> need for a fixed-link. 2500BaseX however does not have such
> signalling, so there would need to be a fixed link.
> 
> Maybe we should really consider what phy-mode = "sgmii"; means. Should
> this include the overclocked 2.5G speed, or should we add a 2500sgmii
> link mode?

Note that Documentation/networking/phy.rst now contains definitions
for SGMII, 1000BASE-X and 2500BASE-X.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[René van Dorst]

Quoting Andrew Lunn <andrew@lunn.ch>:

> On Fri, Jul 26, 2019 at 07:19:56AM +0000, René van Dorst wrote:
>> Quoting Andrew Lunn <andrew@lunn.ch>:
>>
>> >>+	gmac0: mac@0 {
>> >>+		compatible = "mediatek,eth-mac";
>> >>+		reg = <0>;
>> >>+		phy-mode = "sgmii";
>> >>+
>> >>+		fixed-link {
>> >>+			speed = <2500>;
>> >>+			full-duplex;
>> >>+			pause;
>> >>+		};
>> >>+	};
>> >
>> >Hi René
>> >
>>
>> Hi Andrew,
>>
>> >SGMII and fixed-link is rather odd. Why do you need this combination?
>>
>> BananaPi R64 has a RTL8367S 5+2-port switch, switch interfaces with the SOC
>> by a
>> (H)SGMII and/or RGMII interface. SGMII is mainly used for the LAN ports and
>> RGMII for the WAN port.
>>
>> I mimic the SDK software which puts SGMII interface in 2.5GBit fixed-link
>> mode.
>> The RTL8367S switch code also put switch mac in forge 2.5GBit mode.
>>
>> So this is the reason why I put a fixed-link mode here.
>
> Are you sure it is using SGMII and not 2500BaseX? Can you get access
> to the signalling word? SGMII is supposed to indicate to the MAC what
> speed it is using, via inband signalling. So there should not be any
> need for a fixed-link. 2500BaseX however does not have such
> signalling, so there would need to be a fixed link.

I am not sure.

I just converted the current mainline code to support phylink and  
mimic the DTS
of the SDK. But the SDK seems to be incorrect.

Realtek[0] calls these modes:
* SGMII (1.25GHz) Interface
* High SGMII (3.125GHz) Interface
Also the datasheet that I have doesn't talk about base-x modes.

But MT7622 Reference manual[1] page 1960 says:
  The core leverages the 1000Base-X PCS and Auto-Negotiation from IEEE 802.3
  specification (clause 36/37). This IP can support up to 3.125G baud  
for 2.5Gbps
  (proprietary 2500Base-X) data rate of MAC by overclocking.

So I think it phy-mode should be 2500Base-X in this case.

SGMII part is a bit hard for me to support, I don't have the hardware,
MediaTek datasheets are mostly incomplete and also I am a not familiar  
with it.

But I think I know what I have to change.
Based on your explanation above.

I think this more correct implementation:

* 1000base-x and 2500base-x always force the link.
* SGMII is always inband but I need in phylink_mac_link_status() to readout
   "PCS_SPEED_ABILITY Clause 45 3.5" register to see the inband status?
   Or is it just the GMAC PSMR register? For me it is a bit confusing.
   SGMII block has a register to set the link speed and etc. But tests on the
   bananapi R64 board shows that I also need to set the GMAC register else it
   didn't work. Also it is not easy to debug if you don't have the board.

> Maybe we should really consider what phy-mode = "sgmii"; means. Should
> this include the overclocked 2.5G speed, or should we add a 2500sgmii
> link mode?

No.

>
>      Andrew

Greats,

René

[0]:  
https://www.realtek.com/en/products/communications-network-ics/item/rtl8367s-cg
[1]:  
https://drive.google.com/file/d/1cW8KQmmVpwDGmBd48KNQes9CRn7FEgBb/view?usp=sharing

Re: [PATCH net-next 3/3] dt-bindings: net: ethernet: Update mt7622 docs and dts to reflect the new phylink API

[Russell King - ARM Linux admin]

On Fri, Jul 26, 2019 at 03:16:22PM +0000, René van Dorst wrote:
> Quoting Andrew Lunn <andrew@lunn.ch>:
> > On Fri, Jul 26, 2019 at 07:19:56AM +0000, René van Dorst wrote:
> > > Quoting Andrew Lunn <andrew@lunn.ch>:
> > > 
> > > >>+	gmac0: mac@0 {
> > > >>+		compatible = "mediatek,eth-mac";
> > > >>+		reg = <0>;
> > > >>+		phy-mode = "sgmii";
> > > >>+
> > > >>+		fixed-link {
> > > >>+			speed = <2500>;
> > > >>+			full-duplex;
> > > >>+			pause;
> > > >>+		};
> > > >>+	};
> > > >
> > > >Hi René
> > > >
> > > 
> > > Hi Andrew,
> > > 
> > > >SGMII and fixed-link is rather odd. Why do you need this combination?
> > > 
> > > BananaPi R64 has a RTL8367S 5+2-port switch, switch interfaces with the SOC
> > > by a
> > > (H)SGMII and/or RGMII interface. SGMII is mainly used for the LAN ports and
> > > RGMII for the WAN port.
> > > 
> > > I mimic the SDK software which puts SGMII interface in 2.5GBit fixed-link
> > > mode.
> > > The RTL8367S switch code also put switch mac in forge 2.5GBit mode.
> > > 
> > > So this is the reason why I put a fixed-link mode here.
> > 
> > Are you sure it is using SGMII and not 2500BaseX? Can you get access
> > to the signalling word? SGMII is supposed to indicate to the MAC what
> > speed it is using, via inband signalling. So there should not be any
> > need for a fixed-link. 2500BaseX however does not have such
> > signalling, so there would need to be a fixed link.
> 
> I am not sure.
> 
> I just converted the current mainline code to support phylink and mimic the
> DTS
> of the SDK. But the SDK seems to be incorrect.
> 
> Realtek[0] calls these modes:
> * SGMII (1.25GHz) Interface
> * High SGMII (3.125GHz) Interface
> Also the datasheet that I have doesn't talk about base-x modes.

So this is RTL8367S-CG, which is a switch.  It's entirely possible that
it really does support what it says it does.

> But MT7622 Reference manual[1] page 1960 says:
>  The core leverages the 1000Base-X PCS and Auto-Negotiation from IEEE 802.3
>  specification (clause 36/37). This IP can support up to 3.125G baud for
> 2.5Gbps
>  (proprietary 2500Base-X) data rate of MAC by overclocking.
> 
> So I think it phy-mode should be 2500Base-X in this case.

Right, so this suggests that it only supports 1000BASE-X and 2500BASE-X
via the normal method of "over-clocking" 1000BASE-X.

1000BASE-X and SGMII are compatible _if_ and only if you ignore the
contents of the 16-bit control word which is used for auto-negotiation
in the case of 1000BASE-X, or for communicating the negotiation results
in the case of SGMII.  Apart from that 16-bit control word, and the
semantics of it, at the data link level the two are the same.

> SGMII part is a bit hard for me to support, I don't have the hardware,
> MediaTek datasheets are mostly incomplete and also I am a not familiar with
> it.
> 
> But I think I know what I have to change.
> Based on your explanation above.
> 
> I think this more correct implementation:
> 
> * 1000base-x and 2500base-x always force the link.

I think the above is why you have to force the link: a link consisting
of one end configured for SGMII and the other end configured for
1000BASE-X is not a good idea at the best of times, but if you ignore
the 16-bit control word and force it, it will work.

What this means is that you _should_ be forcing it in DT to be a
fixed link, and not trying to do auto-neg.

> * SGMII is always inband but I need in phylink_mac_link_status() to readout
>   "PCS_SPEED_ABILITY Clause 45 3.5" register to see the inband status?
>   Or is it just the GMAC PSMR register? For me it is a bit confusing.
>   SGMII block has a register to set the link speed and etc. But tests on the
>   bananapi R64 board shows that I also need to set the GMAC register else it
>   didn't work. Also it is not easy to debug if you don't have the board.

phylink_mac_link_status() is expected to read the results of SGMII
or 1000BASE-X negotiation at the MAC side of the link.  To see why,
consider a fiber link:

MAC-PCS --- SFP ----- fiber ----- SFP --- MAC-PCS

The fiber is passive, the SFP merely converts between light and
electrical signals - there's nothing apart from the MAC's own PCS
that can report what the negotiation state of the link is.  So,
you need to read from whatever bit of hardware on the MAC side
which will report that - basically, the results of the 1000BASE-X
negotiation.

phylink currently expects results from the PCS to be automatically
propagated to the MAC through hardware, since that's what happens
on Marvell setups - however, that can be changed if there are
setups which need manual propagation.

If we do need to do that, I'd suggest we rename
phylink_mac_link_status() to be phylink_macpcs_link_status() to
clarify which bit of hardware it's supposed to be reading from.

> 
> > Maybe we should really consider what phy-mode = "sgmii"; means. Should
> > this include the overclocked 2.5G speed, or should we add a 2500sgmii
> > link mode?
> 
> No.

I'm really not in favour of "sgmii" being used to also describe the
over-clocked SGMII variants.  It's a different protocol - many data
sheets (e.g. for PHYs that support it) explicitly state that the
speed bits in the SGMII 16-bit control word are not valid, and
over-clocked vs normal SGMII can not be auto-negotiated.  Both ends
must be running at the same speed in order to successfully transfer
even the 16-bit control word that dictates the link speed.

So, SGMII at 3.125Gbps is really a different interface mode from
SGMII at 1.25Gbps.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up