[PATCH v2 0/2] Enable ethernet on qcs615

[PATCH v2 0/2] Enable ethernet on qcs615

[Yijie Yang]

Add dts nodes and EMAC driver data to enable ethernet interface on
qcs615-ride platforms.
The EMAC version currently in use on this platform is 2.3.1, and the EPHY
model is Micrel KSZ9031.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
This patch series depends on below patch series:
https://lore.kernel.org/all/20241104-add_initial_support_for_qcs615-v5-0-9dde8d7b80b0@quicinc.com/
https://lore.kernel.org/all/20241118-schema-v1-1-11b7c1583c0c@quicinc.com/
https://lore.kernel.org/all/20241118-schema-v1-2-11b7c1583c0c@quicinc.com/

Changes in v2:
- Pad the address to 8 hex digits with leading zeros.
- Make clock names a vertical list.
- Refresh the dependencies and update the base-commit.
- Link to v1: https://lore.kernel.org/r/20241010-dts_qcs615-v1-0-05f27f6ac4d3@quicinc.com

---
Yijie Yang (2):
      arm64: dts: qcom: qcs615: add ethernet node
      arm64: dts: qcom: qcs615-ride: Enable ethernet node

 arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/qcs615.dtsi     |  32 ++++++++++
 2 files changed, 138 insertions(+)
---
base-commit: ec29543c01b3dbfcb9a2daa4e0cd33afb3c30c39
change-id: 20241111-dts_qcs615-7300e18d52d8

Best regards,
-- 
Yijie Yang <quic_yijiyang@quicinc.com>

[PATCH v2 1/2] arm64: dts: qcom: qcs615: add ethernet node

[Yijie Yang]

Add ethqos ethernet controller node for QCS615 SoC.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615.dtsi | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
index 868808918fd2cdf3f23fcb43ead61b2abfc776f7..e429a012428701b1240556c919c630382b3ee8ce 100644
--- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
@@ -375,6 +375,38 @@ soc: soc@0 {
 		#address-cells = <2>;
 		#size-cells = <2>;
 
+		ethernet: ethernet@20000 {
+			compatible = "qcom,qcs615-ethqos";
+			reg = <0x0 0x00020000 0x0 0x00010000>,
+			      <0x0 0x00036000 0x0 0x00000100>;
+			reg-names = "stmmaceth", "rgmii";
+
+			clocks = <&gcc GCC_EMAC_AXI_CLK>,
+				 <&gcc GCC_EMAC_SLV_AHB_CLK>,
+				 <&gcc GCC_EMAC_PTP_CLK>,
+				 <&gcc GCC_EMAC_RGMII_CLK>;
+			clock-names = "stmmaceth",
+				      "pclk",
+				      "ptp_ref",
+				      "rgmii";
+
+			interrupts = <GIC_SPI 660 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 662 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq", "eth_lpi";
+
+			power-domains = <&gcc EMAC_GDSC>;
+			resets = <&gcc GCC_EMAC_BCR>;
+
+			iommus = <&apps_smmu 0x1c0 0x0>;
+
+			snps,tso;
+			snps,pbl = <32>;
+			rx-fifo-depth = <16384>;
+			tx-fifo-depth = <20480>;
+
+			status = "disabled";
+		};
+
 		gcc: clock-controller@100000 {
 			compatible = "qcom,qcs615-gcc";
 			reg = <0 0x00100000 0 0x1f0000>;

-- 
2.34.1

[PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

Enable the ethernet node, add the phy node and pinctrl for ethernet.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644
--- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
+++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
@@ -5,6 +5,7 @@
 /dts-v1/;
 
 #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+#include <dt-bindings/gpio/gpio.h>
 #include "qcs615.dtsi"
 / {
 	model = "Qualcomm Technologies, Inc. QCS615 Ride";
@@ -196,6 +197,60 @@ vreg_l17a: ldo17 {
 	};
 };
 
+&ethernet {
+	status = "okay";
+
+	pinctrl-0 = <&ethernet_defaults>;
+	pinctrl-names = "default";
+
+	phy-handle = <&rgmii_phy>;
+	phy-mode = "rgmii";
+	max-speed = <1000>;
+
+	snps,mtl-rx-config = <&mtl_rx_setup>;
+	snps,mtl-tx-config = <&mtl_tx_setup>;
+
+	mdio: mdio {
+		compatible = "snps,dwmac-mdio";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		rgmii_phy: phy@7 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			reg = <0x7>;
+
+			interrupts-extended = <&tlmm 121 IRQ_TYPE_EDGE_FALLING>;
+			device_type = "ethernet-phy";
+			reset-gpios = <&tlmm 104 GPIO_ACTIVE_LOW>;
+			reset-assert-us = <11000>;
+			reset-deassert-us = <70000>;
+		};
+	};
+
+	mtl_rx_setup: rx-queues-config {
+		snps,rx-queues-to-use = <1>;
+		snps,rx-sched-sp;
+
+		queue0 {
+			snps,dcb-algorithm;
+			snps,map-to-dma-channel = <0x0>;
+			snps,route-up;
+			snps,priority = <0x1>;
+		};
+	};
+
+	mtl_tx_setup: tx-queues-config {
+		snps,tx-queues-to-use = <1>;
+		snps,tx-sched-wrr;
+
+		queue0 {
+			snps,weight = <0x10>;
+			snps,dcb-algorithm;
+			snps,priority = <0x0>;
+		};
+	};
+};
+
 &gcc {
 	clocks = <&rpmhcc RPMH_CXO_CLK>,
 		 <&rpmhcc RPMH_CXO_CLK_A>,
@@ -210,6 +265,57 @@ &rpmhcc {
 	clocks = <&xo_board_clk>;
 };
 
+&tlmm {
+	ethernet_defaults: ethernet-defaults-state {
+		mdc-pins {
+			pins = "gpio113";
+			function = "rgmii";
+			bias-pull-up;
+		};
+
+		mdio-pins {
+			pins = "gpio114";
+			function = "rgmii";
+			bias-pull-up;
+		};
+
+		rgmii-rx-pins {
+			pins = "gpio81", "gpio82", "gpio83", "gpio102", "gpio103", "gpio112";
+			function = "rgmii";
+			bias-disable;
+			drive-strength = <2>;
+		};
+
+		rgmii-tx-pins {
+			pins = "gpio92", "gpio93", "gpio94", "gpio95", "gpio96", "gpio97";
+			function = "rgmii";
+			bias-pull-up;
+			drive-strength = <16>;
+		};
+
+		phy-intr-pins {
+			pins = "gpio121";
+			function = "gpio";
+			bias-disable;
+			drive-strength = <8>;
+		};
+
+		pps-pins {
+			pins = "gpio91";
+			function = "rgmii";
+			bias-disable;
+			drive-strength = <8>;
+		};
+
+		phy-reset-pins {
+			pins = "gpio104";
+			function = "gpio";
+			bias-pull-up;
+			drive-strength = <16>;
+		};
+	};
+};
+
 &uart0 {
 	status = "okay";
 };

-- 
2.34.1

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

On Mon, Nov 18, 2024 at 02:44:02PM +0800, Yijie Yang wrote:
> Enable the ethernet node, add the phy node and pinctrl for ethernet.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> @@ -5,6 +5,7 @@
>  /dts-v1/;
>  
>  #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +#include <dt-bindings/gpio/gpio.h>
>  #include "qcs615.dtsi"
>  / {
>  	model = "Qualcomm Technologies, Inc. QCS615 Ride";
> @@ -196,6 +197,60 @@ vreg_l17a: ldo17 {
>  	};
>  };
>  
> +&ethernet {
> +	status = "okay";
> +
> +	pinctrl-0 = <&ethernet_defaults>;
> +	pinctrl-names = "default";
> +
> +	phy-handle = <&rgmii_phy>;
> +	phy-mode = "rgmii";

That is unusual. Does the board have extra long clock lines?

> +	max-speed = <1000>;

Why do you have this property? It is normally used to slow the MAC
down because of issues at higher speeds.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-11-19 09:27, Andrew Lunn wrote:
> On Mon, Nov 18, 2024 at 02:44:02PM +0800, Yijie Yang wrote:
>> Enable the ethernet node, add the phy node and pinctrl for ethernet.
>>
>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++
>>   1 file changed, 106 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> @@ -5,6 +5,7 @@
>>   /dts-v1/;
>>   
>>   #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>> +#include <dt-bindings/gpio/gpio.h>
>>   #include "qcs615.dtsi"
>>   / {
>>   	model = "Qualcomm Technologies, Inc. QCS615 Ride";
>> @@ -196,6 +197,60 @@ vreg_l17a: ldo17 {
>>   	};
>>   };
>>   
>> +&ethernet {
>> +	status = "okay";
>> +
>> +	pinctrl-0 = <&ethernet_defaults>;
>> +	pinctrl-names = "default";
>> +
>> +	phy-handle = <&rgmii_phy>;
>> +	phy-mode = "rgmii";
> 
> That is unusual. Does the board have extra long clock lines?

Do you mean to imply that using RGMII mode is unusual? While the EMAC 
controller supports various modes, due to hardware design limitations, 
only RGMII mode can be effectively implemented.

> 
>> +	max-speed = <1000>;
> 
> Why do you have this property? It is normally used to slow the MAC
> down because of issues at higher speeds.

According to the databoot, the EMAC in RGMII mode can support speeds of 
up to 1Gbps.

> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Konrad Dybcio]

On 19.11.2024 11:09 AM, Yijie Yang wrote:
> 
> 
> On 2024-11-19 09:27, Andrew Lunn wrote:
>> On Mon, Nov 18, 2024 at 02:44:02PM +0800, Yijie Yang wrote:
>>> Enable the ethernet node, add the phy node and pinctrl for ethernet.
>>>
>>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++
>>>   1 file changed, 106 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>>> index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644
>>> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>>> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>>> @@ -5,6 +5,7 @@
>>>   /dts-v1/;
>>>     #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>>> +#include <dt-bindings/gpio/gpio.h>
>>>   #include "qcs615.dtsi"
>>>   / {
>>>       model = "Qualcomm Technologies, Inc. QCS615 Ride";
>>> @@ -196,6 +197,60 @@ vreg_l17a: ldo17 {
>>>       };
>>>   };
>>>   +&ethernet {
>>> +    status = "okay";
>>> +
>>> +    pinctrl-0 = <&ethernet_defaults>;
>>> +    pinctrl-names = "default";
>>> +
>>> +    phy-handle = <&rgmii_phy>;
>>> +    phy-mode = "rgmii";
>>
>> That is unusual. Does the board have extra long clock lines?
> 
> Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented.

Is that a board-specific issue, or something that concerns the SoC itself?

> 
>>
>>> +    max-speed = <1000>;
>>
>> Why do you have this property? It is normally used to slow the MAC
>> down because of issues at higher speeds.
> 
> According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps.

I believe the question Andrew is asking is "how is that effectively
different from the default setting (omitting the property)?"

Konrad

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

> >>>   +&ethernet {
> >>> +    status = "okay";
> >>> +
> >>> +    pinctrl-0 = <&ethernet_defaults>;
> >>> +    pinctrl-names = "default";
> >>> +
> >>> +    phy-handle = <&rgmii_phy>;
> >>> +    phy-mode = "rgmii";
> >>
> >> That is unusual. Does the board have extra long clock lines?
> > 
> > Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented.
> 
> Is that a board-specific issue, or something that concerns the SoC itself?

Lots of developers gets this wrong.... Searching the mainline list for
patchs getting it wrong and the explanation i have given in the past
might help.

The usual setting here is 'rgmmii-id', which means something needs to
insert a 2ns delay on the clock lines. This is not always true, a very
small number of boards use extra long clock likes on the PCB to add
the needed 2ns delay.

Now, if 'rgmii' does work, it means something else is broken
somewhere. I will let you find out what.

> >>> +    max-speed = <1000>;
> >>
> >> Why do you have this property? It is normally used to slow the MAC
> >> down because of issues at higher speeds.
> > 
> > According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps.
> 
> I believe the question Andrew is asking is "how is that effectively
> different from the default setting (omitting the property)?"

Correct. If there are no issues at higher speeds, you don't need
this. phylib will ask the PHY what it is capable of, and limit the
negotiated speeds to its capabilities. Occasionally you do see an
RGMII PHY connected to a MII MAC, because a RGMII PHY is cheaper...

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-11-22 21:19, Andrew Lunn wrote:
>>>>>    +&ethernet {
>>>>> +    status = "okay";
>>>>> +
>>>>> +    pinctrl-0 = <&ethernet_defaults>;
>>>>> +    pinctrl-names = "default";
>>>>> +
>>>>> +    phy-handle = <&rgmii_phy>;
>>>>> +    phy-mode = "rgmii";
>>>>
>>>> That is unusual. Does the board have extra long clock lines?
>>>
>>> Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented.
>>
>> Is that a board-specific issue, or something that concerns the SoC itself?
> 
> Lots of developers gets this wrong.... Searching the mainline list for
> patchs getting it wrong and the explanation i have given in the past
> might help.
> 
> The usual setting here is 'rgmmii-id', which means something needs to
> insert a 2ns delay on the clock lines. This is not always true, a very
> small number of boards use extra long clock likes on the PCB to add
> the needed 2ns delay.
> 
> Now, if 'rgmii' does work, it means something else is broken
> somewhere. I will let you find out what.

The 'rgmii' does function correctly, but it does not necessarily mean 
that a time delay is required at the board level. The EPHY can also 
compensate for the time skew.

> 
>>>>> +    max-speed = <1000>;
>>>>
>>>> Why do you have this property? It is normally used to slow the MAC
>>>> down because of issues at higher speeds.
>>>
>>> According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps.
>>
>> I believe the question Andrew is asking is "how is that effectively
>> different from the default setting (omitting the property)?"
> 
> Correct. If there are no issues at higher speeds, you don't need
> this. phylib will ask the PHY what it is capable of, and limit the
> negotiated speeds to its capabilities. Occasionally you do see an
> RGMII PHY connected to a MII MAC, because a RGMII PHY is cheaper...
> 
> 	Andrew

It does unnecessary, I will remove it.

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-11-27 14:17, Yijie Yang wrote:
> 
> 
> On 2024-11-22 21:19, Andrew Lunn wrote:
>>>>>>    +&ethernet {
>>>>>> +    status = "okay";
>>>>>> +
>>>>>> +    pinctrl-0 = <&ethernet_defaults>;
>>>>>> +    pinctrl-names = "default";
>>>>>> +
>>>>>> +    phy-handle = <&rgmii_phy>;
>>>>>> +    phy-mode = "rgmii";
>>>>>
>>>>> That is unusual. Does the board have extra long clock lines?
>>>>
>>>> Do you mean to imply that using RGMII mode is unusual? While the 
>>>> EMAC controller supports various modes, due to hardware design 
>>>> limitations, only RGMII mode can be effectively implemented.
>>>
>>> Is that a board-specific issue, or something that concerns the SoC 
>>> itself?
>>
>> Lots of developers gets this wrong.... Searching the mainline list for
>> patchs getting it wrong and the explanation i have given in the past
>> might help.
>>
>> The usual setting here is 'rgmmii-id', which means something needs to
>> insert a 2ns delay on the clock lines. This is not always true, a very
>> small number of boards use extra long clock likes on the PCB to add
>> the needed 2ns delay.
>>
>> Now, if 'rgmii' does work, it means something else is broken
>> somewhere. I will let you find out what.
> 
> The 'rgmii' does function correctly, but it does not necessarily mean 
> that a time delay is required at the board level. The EPHY can also 
> compensate for the time skew.

I was mistaken earlier; it is actually the EMAC that will introduce a 
time skew by shifting the phase of the clock in 'rgmii' mode.

> 
>>
>>>>>> +    max-speed = <1000>;
>>>>>
>>>>> Why do you have this property? It is normally used to slow the MAC
>>>>> down because of issues at higher speeds.
>>>>
>>>> According to the databoot, the EMAC in RGMII mode can support speeds 
>>>> of up to 1Gbps.
>>>
>>> I believe the question Andrew is asking is "how is that effectively
>>> different from the default setting (omitting the property)?"
>>
>> Correct. If there are no issues at higher speeds, you don't need
>> this. phylib will ask the PHY what it is capable of, and limit the
>> negotiated speeds to its capabilities. Occasionally you do see an
>> RGMII PHY connected to a MII MAC, because a RGMII PHY is cheaper...
>>
>>     Andrew
> 
> It does unnecessary, I will remove it.
> 

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

> > The usual setting here is 'rgmmii-id', which means something needs to
> > insert a 2ns delay on the clock lines. This is not always true, a very
> > small number of boards use extra long clock likes on the PCB to add
> > the needed 2ns delay.
> > 
> > Now, if 'rgmii' does work, it means something else is broken
> > somewhere. I will let you find out what.
> 
> The 'rgmii' does function correctly, but it does not necessarily mean that a
> time delay is required at the board level. The EPHY can also compensate for
> the time skew.

Basic definitions for phy-mode:

rgmii: Indicates the board provides the delays, normally via extra
long clock lines.

rgmii-id: The board does not provide the delay, the software need to
arrange that either the MAC or the PHY adds the delays.

We then have the values passed between the MAC and the PHY driver:
PHY_INTERFACE_MODE_RGMII: The PHY should not add delays
PHY_INTERFACE_MODE_RGMII_ID: The PHY should add delays.

A typical MAC/PHY combination, phy-mode is passed to the PHY, and the
PHY adds the delays, if needed.

This is why i said there are probably two bugs:

1) phy-mode rgmii should probably be rgmii-id

2) The PHY is adding delays when it should not be, because it is being
   passed PHY_INTERFACE_MODE_RGMII not PHY_INTERFACE_MODE_RGMII_ID.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

> I was mistaken earlier; it is actually the EMAC that will introduce a time
> skew by shifting the phase of the clock in 'rgmii' mode.

This is fine, but not the normal way we do this. The Linux preference
is that the PHY adds the delays. There are a few exceptions, boards
which have PHYs which cannot add delays. In that case the MAC adds the
delays. But this is pretty unusual.

If you decided you want to be unusual and have the MAC add the delays,
it should not be hard coded. You need to look at phy-mode. Only add
delays for rgmii-id. And you then need to mask the value passed to the
PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID,
so the PHY does not add delays as well.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-11-29 23:29, Andrew Lunn wrote:
>> I was mistaken earlier; it is actually the EMAC that will introduce a time
>> skew by shifting the phase of the clock in 'rgmii' mode.
> 
> This is fine, but not the normal way we do this. The Linux preference
> is that the PHY adds the delays. There are a few exceptions, boards
> which have PHYs which cannot add delays. In that case the MAC adds the
> delays. But this is pretty unusual.

After testing, it has been observed that modes other than 'rgmii' do not 
function properly due to the current configuration sequence in the 
driver code.

> 
> If you decided you want to be unusual and have the MAC add the delays,
> it should not be hard coded. You need to look at phy-mode. Only add

Are you suggesting that 'rgmii' indicates the delay is introduced by the 
board rather than the EMAC? But according to the 
Documentation/devicetree/bindings/net/ethernet-controller.yaml, this 
mode explicitly states that 'RX and TX delays are added by the MAC when 
required'. That is indeed my preference.

> delays for rgmii-id. And you then need to mask the value passed to the
> PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID,
> so the PHY does not add delays as well.
> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

On Mon, Dec 09, 2024 at 10:11:23AM +0800, Yijie Yang wrote:
> 
> 
> On 2024-11-29 23:29, Andrew Lunn wrote:
> > > I was mistaken earlier; it is actually the EMAC that will introduce a time
> > > skew by shifting the phase of the clock in 'rgmii' mode.
> > 
> > This is fine, but not the normal way we do this. The Linux preference
> > is that the PHY adds the delays. There are a few exceptions, boards
> > which have PHYs which cannot add delays. In that case the MAC adds the
> > delays. But this is pretty unusual.
> 
> After testing, it has been observed that modes other than 'rgmii' do not
> function properly due to the current configuration sequence in the driver
> code.

O.K, so now you need to find out why.

It not working probably suggests you are adding double delays, both in
the MAC and the PHY. Where the PHY driver add delays is generally easy
to see in the code. Just search for PHY_INTERFACE_MODE_RGMII_ID. For
the MAC driver you probably need to read the datasheet and find
registers which control the delay.

> > If you decided you want to be unusual and have the MAC add the delays,
> > it should not be hard coded. You need to look at phy-mode. Only add
> 
> Are you suggesting that 'rgmii' indicates the delay is introduced by the
> board rather than the EMAC?

Yes.

> But according to the
> Documentation/devicetree/bindings/net/ethernet-controller.yaml, this mode
> explicitly states that 'RX and TX delays are added by the MAC when
> required'. That is indeed my preference.

You need to be careful with context. If the board is not adding
delays, and you pass PHY_INTERFACE_MODE_RGMII to the PHY, the MAC must
be adding the delays, otherwise there will not be any delays, and it
will not work.

> > delays for rgmii-id. And you then need to mask the value passed to the
> > PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID,
> > so the PHY does not add delays as well.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-12-09 11:13, Andrew Lunn wrote:
> On Mon, Dec 09, 2024 at 10:11:23AM +0800, Yijie Yang wrote:
>>
>>
>> On 2024-11-29 23:29, Andrew Lunn wrote:
>>>> I was mistaken earlier; it is actually the EMAC that will introduce a time
>>>> skew by shifting the phase of the clock in 'rgmii' mode.
>>>
>>> This is fine, but not the normal way we do this. The Linux preference
>>> is that the PHY adds the delays. There are a few exceptions, boards
>>> which have PHYs which cannot add delays. In that case the MAC adds the
>>> delays. But this is pretty unusual.
>>
>> After testing, it has been observed that modes other than 'rgmii' do not
>> function properly due to the current configuration sequence in the driver
>> code.
> 
> O.K, so now you need to find out why.
> 
> It not working probably suggests you are adding double delays, both in
> the MAC and the PHY. Where the PHY driver add delays is generally easy
> to see in the code. Just search for PHY_INTERFACE_MODE_RGMII_ID. For
> the MAC driver you probably need to read the datasheet and find
> registers which control the delay.

As previously mentioned, using 'rgmii' will enable EMAC to provide the 
delay while disabling the delay for EPHY. So there's won't be double delay.

Additionally, the current implementation of the QCOM driver code 
exclusively supports this mode, with the entire initialization sequence 
of EMAC designed and fixed for this specific mode.

Therefore, no other options are available until changes are made to the 
driver.

> 
>>> If you decided you want to be unusual and have the MAC add the delays,
>>> it should not be hard coded. You need to look at phy-mode. Only add
>>
>> Are you suggesting that 'rgmii' indicates the delay is introduced by the
>> board rather than the EMAC?
> 
> Yes.
> 
>> But according to the
>> Documentation/devicetree/bindings/net/ethernet-controller.yaml, this mode
>> explicitly states that 'RX and TX delays are added by the MAC when
>> required'. That is indeed my preference.
> 
> You need to be careful with context. If the board is not adding
> delays, and you pass PHY_INTERFACE_MODE_RGMII to the PHY, the MAC must
> be adding the delays, otherwise there will not be any delays, and it
> will not work.
> 

I'm not sure if there's a disagreement about the definition or a 
misunderstanding with other vendors. From my understanding, 'rgmii' 
should not imply that the delay must be provided by the board, based on 
both the definition in the dt-binding file and the implementations by 
other EMAC vendors. Most EMAC drivers provide the delay in this mode.

I confirmed that there is no delay on the qcs615-ride board., and the 
QCOM EMAC driver will adds the delay by shifting the clock after 
receiving PHY_INTERFACE_MODE_RGMII.

>>> delays for rgmii-id. And you then need to mask the value passed to the
>>> PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID,
>>> so the PHY does not add delays as well.
> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

> As previously mentioned, using 'rgmii' will enable EMAC to provide the delay
> while disabling the delay for EPHY. So there's won't be double delay.
> 
> Additionally, the current implementation of the QCOM driver code exclusively
> supports this mode, with the entire initialization sequence of EMAC designed
> and fixed for this specific mode.

OK. If it is impossible to disable these delays, you need to validate
phy-mode. Only rgmii-id is allowed. Anybody trying to build a board
using extra long clock lines is out of luck. It does not happen very
often, but there are a small number of boards which do this, and the
definitions of phy-mode are designed to support them.

> I'm not sure if there's a disagreement about the definition or a
> misunderstanding with other vendors. From my understanding, 'rgmii' should
> not imply that the delay must be provided by the board, based on both the
> definition in the dt-binding file and the implementations by other EMAC
> vendors. Most EMAC drivers provide the delay in this mode.

Nope. You are wrong. I've been enforcing this meaning for maybe the
last 10 years. You can go search the email archive for netdev. Before
that, we had a bit of a mess, developers were getting it wrong, and
reviewing was not as good. And i don't review everything, so some bad
code does get passed me every so often, e.g. if found out today that
TI AM62 got this wrong, they hard code TX delays in the MAC, and DT
developers have been using rgmii-rxid, not rgmii-id, and the MAC
driver is missing the mask operation before calling phy_connect.

> I confirmed that there is no delay on the qcs615-ride board., and the QCOM
> EMAC driver will adds the delay by shifting the clock after receiving
> PHY_INTERFACE_MODE_RGMII.

Which is wrong. Because you cannot disable the delay,
PHY_INTERFACE_MODE_RGMII should return in EINVAL, or maybe
EOPNOTSUPP. Your hardware only supports PHY_INTERFACE_MODE_RGMII_ID,
and you need to mask what you pass to phylib/phylink to make it clear
the MAC has added the delays.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-12-10 12:09, Andrew Lunn wrote:
>> As previously mentioned, using 'rgmii' will enable EMAC to provide the delay
>> while disabling the delay for EPHY. So there's won't be double delay.
>>
>> Additionally, the current implementation of the QCOM driver code exclusively
>> supports this mode, with the entire initialization sequence of EMAC designed
>> and fixed for this specific mode.
> 
> OK. If it is impossible to disable these delays, you need to validate
> phy-mode. Only rgmii-id is allowed. Anybody trying to build a board
> using extra long clock lines is out of luck. It does not happen very
> often, but there are a small number of boards which do this, and the
> definitions of phy-mode are designed to support them.
> 
>> I'm not sure if there's a disagreement about the definition or a
>> misunderstanding with other vendors. From my understanding, 'rgmii' should
>> not imply that the delay must be provided by the board, based on both the
>> definition in the dt-binding file and the implementations by other EMAC
>> vendors. Most EMAC drivers provide the delay in this mode.
> 
> Nope. You are wrong. I've been enforcing this meaning for maybe the
> last 10 years. You can go search the email archive for netdev. Before
> that, we had a bit of a mess, developers were getting it wrong, and
> reviewing was not as good. And i don't review everything, so some bad
> code does get passed me every so often, e.g. if found out today that
> TI AM62 got this wrong, they hard code TX delays in the MAC, and DT
> developers have been using rgmii-rxid, not rgmii-id, and the MAC
> driver is missing the mask operation before calling phy_connect.
> 
>> I confirmed that there is no delay on the qcs615-ride board., and the QCOM
>> EMAC driver will adds the delay by shifting the clock after receiving
>> PHY_INTERFACE_MODE_RGMII.
> 
> Which is wrong. Because you cannot disable the delay,
> PHY_INTERFACE_MODE_RGMII should return in EINVAL, or maybe
> EOPNOTSUPP. Your hardware only supports PHY_INTERFACE_MODE_RGMII_ID,
> and you need to mask what you pass to phylib/phylink to make it clear
> the MAC has added the delays.
> 
> 	Andrew
I intend to follow these steps. Could you please check if they are correct?
1. Add a new flag in DTS to inform the MAC driver to include the delay 
when configured with 'rgmii-id'. Without this flag, the MAC driver will 
not be aware of the need for the delay.
2. In the driver, if this flag is set to true, change the phy_mode to 
PHY_INTERFACE_MODE_RGMII to instruct the PHY not to add the delay.

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

> I intend to follow these steps. Could you please check if they are correct?
> 1. Add a new flag in DTS to inform the MAC driver to include the delay when
> configured with 'rgmii-id'. Without this flag, the MAC driver will not be
> aware of the need for the delay.

Why do you need this flag?

If the phy-mode is rgmii-id, either the MAC or the PHY needs to add
the delay.

The MAC driver gets to see phy-mode first. If it wants to add the
delay, it can, but it needs to mask out the delays before passing
phy-mode to the PHY. If the MAC driver does not want to add the
delays, pass phy-mode as is the PHY, and it will add the delays.

There is nothing special here, every MAC/PHY pair does this, without
needing additional properties.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-12-16 17:18, Andrew Lunn wrote:
>> I intend to follow these steps. Could you please check if they are correct?
>> 1. Add a new flag in DTS to inform the MAC driver to include the delay when
>> configured with 'rgmii-id'. Without this flag, the MAC driver will not be
>> aware of the need for the delay.
> 
> Why do you need this flag?
> 
> If the phy-mode is rgmii-id, either the MAC or the PHY needs to add
> the delay.
> 
> The MAC driver gets to see phy-mode first. If it wants to add the
> delay, it can, but it needs to mask out the delays before passing
> phy-mode to the PHY. If the MAC driver does not want to add the
> delays, pass phy-mode as is the PHY, and it will add the delays.

In this scenario, the delay in 'rgmii-id' mode is currently introduced 
by the MAC as it is fixed in the driver code. How can we enable the PHY 
to add the delay in this mode in the future (If we intend to revert to 
the most common approach of the Linux kernel)? After all, the MAC driver 
is unsure when to add the delay.

> 
> There is nothing special here, every MAC/PHY pair does this, without
> needing additional properties.
> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

On Tue, Dec 17, 2024 at 10:26:15AM +0800, Yijie Yang wrote:
> 
> 
> On 2024-12-16 17:18, Andrew Lunn wrote:
> > > I intend to follow these steps. Could you please check if they are correct?
> > > 1. Add a new flag in DTS to inform the MAC driver to include the delay when
> > > configured with 'rgmii-id'. Without this flag, the MAC driver will not be
> > > aware of the need for the delay.
> > 
> > Why do you need this flag?
> > 
> > If the phy-mode is rgmii-id, either the MAC or the PHY needs to add
> > the delay.
> > 
> > The MAC driver gets to see phy-mode first. If it wants to add the
> > delay, it can, but it needs to mask out the delays before passing
> > phy-mode to the PHY. If the MAC driver does not want to add the
> > delays, pass phy-mode as is the PHY, and it will add the delays.
> 
> In this scenario, the delay in 'rgmii-id' mode is currently introduced by
> the MAC as it is fixed in the driver code. How can we enable the PHY to add
> the delay in this mode in the future (If we intend to revert to the most
> common approach of the Linux kernel)? After all, the MAC driver is unsure
> when to add the delay.

You just take out the code in the MAC driver which adds the delay and
masks the phy-mode. 2ns should be 2ns delay, independent of who
inserts it. The only danger is, there might be some board uses a PHY
which is incapable of adding the 2ns delay, and such a change breaks
that board.

But i assume Qualcomm RDKs always make use of a Qualcomm PHY, there is
special pricing if you use the combination, so there is probably
little incentive to use somebody elses PHY. And i assume you can
quickly check all Qualcomm PHYs support RGMII delays. PHYs which don't
support RGMII delays are very rare, it just happened that one vendors
RDK happened to use one, so they ended up with delays in the MAC being
standard for their boards.

	Andrew

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

On 2024-12-17 18:18, Andrew Lunn wrote:
> On Tue, Dec 17, 2024 at 10:26:15AM +0800, Yijie Yang wrote:
>>
>>
>> On 2024-12-16 17:18, Andrew Lunn wrote:
>>>> I intend to follow these steps. Could you please check if they are correct?
>>>> 1. Add a new flag in DTS to inform the MAC driver to include the delay when
>>>> configured with 'rgmii-id'. Without this flag, the MAC driver will not be
>>>> aware of the need for the delay.
>>>
>>> Why do you need this flag?
>>>
>>> If the phy-mode is rgmii-id, either the MAC or the PHY needs to add
>>> the delay.
>>>
>>> The MAC driver gets to see phy-mode first. If it wants to add the
>>> delay, it can, but it needs to mask out the delays before passing
>>> phy-mode to the PHY. If the MAC driver does not want to add the
>>> delays, pass phy-mode as is the PHY, and it will add the delays.
>>
>> In this scenario, the delay in 'rgmii-id' mode is currently introduced by
>> the MAC as it is fixed in the driver code. How can we enable the PHY to add
>> the delay in this mode in the future (If we intend to revert to the most
>> common approach of the Linux kernel)? After all, the MAC driver is unsure
>> when to add the delay.
> 
> You just take out the code in the MAC driver which adds the delay and
> masks the phy-mode. 2ns should be 2ns delay, independent of who
> inserts it. The only danger is, there might be some board uses a PHY
> which is incapable of adding the 2ns delay, and such a change breaks
> that board.

Okay, I will follow your instructions:
1. Change the phy-mode to 'rgmii-id'.
2. Add the delay in the MAC driver.
3. Mask the phy-mode before passing it to the PHY.

> 
> But i assume Qualcomm RDKs always make use of a Qualcomm PHY, there is
> special pricing if you use the combination, so there is probably
> little incentive to use somebody elses PHY. And i assume you can
> quickly check all Qualcomm PHYs support RGMII delays. PHYs which don't
> support RGMII delays are very rare, it just happened that one vendors
> RDK happened to use one, so they ended up with delays in the MAC being
> standard for their boards.
> 

Most Qualcomm MAC applications are actually paired with a third-party 
PHY. I agree that the original author's PHY might not support adding the 
delay. However, this assumption cannot be verified since the initial 
code was uploaded from the internal code base.

> 	Andrew
> 

-- 
Best Regards,
Yijie

Re: [PATCH v2 2/2] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Andrew Lunn]

> Okay, I will follow your instructions:
> 1. Change the phy-mode to 'rgmii-id'.
> 2. Add the delay in the MAC driver.
> 3. Mask the phy-mode before passing it to the PHY.

Good.

> 
> > 
> > But i assume Qualcomm RDKs always make use of a Qualcomm PHY, there is
> > special pricing if you use the combination, so there is probably
> > little incentive to use somebody elses PHY. And i assume you can
> > quickly check all Qualcomm PHYs support RGMII delays. PHYs which don't
> > support RGMII delays are very rare, it just happened that one vendors
> > RDK happened to use one, so they ended up with delays in the MAC being
> > standard for their boards.
> > 
> 
> Most Qualcomm MAC applications are actually paired with a third-party PHY.

I'm not sure the QCA8K, IPC and old Atheros people would agree with
you.

But since you don't have any behaviour change, you are not asking the
PHY to insert the delays, you should be safe in your part of the
Qualcomm world.

	Andrew