[PATCH v3 0/4] Enable ethernet on qcs615

[PATCH v3 0/4] Enable ethernet on qcs615

[Yijie Yang]

Correct the definition and usage of phy-mode in both the DT binding and
driver code.
Add dts nodes and EMAC driver data to enable ethernet interface on
qcs615-ride platform.
The EMAC version currently in use on this platform is the same as that in
qcs404. The EPHY model is Micrel KSZ9031.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
Changes in v3:
- Correct the definition of 'rgmii' in ethernet-controller.yaml.
- Remove the redundant max-speed limit in the dts file.
- Update the definition of 'rgmii' to prevent any further misunderstandings.
- Update the phy-mode in the dts file from rgmii to rgmii-id.
- Mask the PHY mode passed to the driver to allow the MAC to add the delay.
- Update the low power mode exit interrupt from 662 to 661.
- Update the compatible string to fallback to qcs404 since they share the same hardware.
- Update base commit to next-20250120.
- Link to v2: https://lore.kernel.org/r/20241118-dts_qcs615-v2-0-e62b924a3cbd@quicinc.com

---
Yijie Yang (4):
      dt-bindings: net: ethernet-controller: Correct the definition of phy-mode
      net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id
      arm64: dts: qcom: qcs615: add ethernet node
      arm64: dts: qcom: qcs615-ride: Enable ethernet node

 .../bindings/net/ethernet-controller.yaml          |   2 +-
 arch/arm64/boot/dts/qcom/qcs615-ride.dts           | 104 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/qcs615.dtsi               |  34 +++++++
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |  18 +++-
 4 files changed, 152 insertions(+), 6 deletions(-)
---
base-commit: 9424d9acada6461344c71ac02f2f3fbcdd775498
change-id: 20241224-dts_qcs615-9612efea02cb
prerequisite-message-id: <20250120-schema_qcs615-v4-1-d9d122f89e64@quicinc.com>
prerequisite-patch-id: b97f36116c87036abe66e061db82588eb1bbaa9a

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

[PATCH v3 1/4] dt-bindings: net: ethernet-controller: Correct the definition of phy-mode

[Yijie Yang]

Correct the definition of 'phy-mode' to reflect that RX and TX delays are
added by the board, not the MAC, to prevent confusion and ensure accurate
documentation.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 Documentation/devicetree/bindings/net/ethernet-controller.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 45819b2358002bc75e876eddb4b2ca18017c04bd..d56f1a2e79c4656762c7397dc96408bdde19c52b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -74,7 +74,7 @@ properties:
       - rev-rmii
       - moca
 
-      # RX and TX delays are added by the MAC when required
+      # RX and TX delays are added by the board when required
       - rgmii
 
       # RGMII with internal RX and TX delays provided by the PHY,

-- 
2.34.1

[PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

The Qualcomm board always chooses the MAC to provide the delay instead of
the PHY, which is completely opposite to the suggestion of the Linux
kernel. The usage of phy-mode in legacy DTS was also incorrect. Change the
phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
the definition.
To address the ABI compatibility issue between the kernel and DTS caused by
this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
code, as it is the only legacy board that mistakenly uses the 'rgmii'
phy-mode.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
 {
 	struct device *dev = &ethqos->pdev->dev;
-	int phase_shift;
+	int phase_shift = 0;
 	int loopback;
 
 	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
-	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
-	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
-		phase_shift = 0;
-	else
+	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
 		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
 
 	/* Disable loopback mode */
@@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	ret = of_get_phy_mode(np, &ethqos->phy_mode);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
+
+	root = of_find_node_by_path("/");
+	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
+		ethqos->phy_mode = PHY_INTERFACE_MODE_RGMII_ID;
+	else if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII)
+		return dev_err_probe(dev, -EINVAL, "Invalid phy-mode rgmii\n");
+	of_node_put(root);
+
+	if (plat_dat->phy_interface == PHY_INTERFACE_MODE_RGMII_ID)
+		plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII;
+
 	switch (ethqos->phy_mode) {
 	case PHY_INTERFACE_MODE_RGMII:
 	case PHY_INTERFACE_MODE_RGMII_ID:

-- 
2.34.1

[PATCH v3 3/4] arm64: dts: qcom: qcs615: add ethernet node

[Yijie Yang]

Add an ethernet controller node for QCS615 SoC to enable ethernet
functionality.

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

diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
index f4abfad474ea62dea13d05eb874530947e1e8d3e..b93609a07a5554ab127c0c0540c58ebc781416a4 100644
--- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
@@ -432,6 +432,40 @@ soc: soc@0 {
 		#address-cells = <2>;
 		#size-cells = <2>;
 
+		ethernet: ethernet@20000 {
+			compatible = "qcom,qcs615-ethqos", "qcom,qcs404-ethqos";
+			reg = <0x0 0x00020000 0x0 0x10000>,
+			      <0x0 0x00036000 0x0 0x100>;
+			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 661 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 v3 4/4] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Yijie Yang]

Enable the ethernet node, add the phy node and pinctrl for ethernet. This
change is necessary to support ethernet functionality on this board.

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

diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
index 2b5aa3c66867676bda59ff82b902b6e4974126f8..2d201b5d09a79613364f0e514858dd30b25706a9 100644
--- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
+++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
@@ -211,6 +211,59 @@ vreg_l17a: ldo17 {
 	};
 };
 
+&ethernet {
+	pinctrl-0 = <&ethernet_defaults>;
+	pinctrl-names = "default";
+
+	phy-handle = <&rgmii_phy>;
+	phy-mode = "rgmii-id";
+
+	snps,mtl-rx-config = <&mtl_rx_setup>;
+	snps,mtl-tx-config = <&mtl_tx_setup>;
+
+	status = "okay";
+
+	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>,
@@ -278,6 +331,57 @@ &sdhc_2 {
 	status = "okay";
 };
 
+&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>;
+		};
+
+		phy-reset-pins {
+			pins = "gpio104";
+			function = "gpio";
+			bias-pull-up;
+			drive-strength = <16>;
+		};
+
+		pps-pins {
+			pins = "gpio91";
+			function = "rgmii";
+			bias-disable;
+			drive-strength = <8>;
+		};
+	};
+};
+
 &uart0 {
 	status = "okay";
 };

-- 
2.34.1

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Krzysztof Kozlowski]

On 21/01/2025 08:54, Yijie Yang wrote:
> The Qualcomm board always chooses the MAC to provide the delay instead of
> the PHY, which is completely opposite to the suggestion of the Linux
> kernel.


How does the Linux kernel suggest it?

> The usage of phy-mode in legacy DTS was also incorrect. Change the
> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
> the definition.
> To address the ABI compatibility issue between the kernel and DTS caused by
> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
> code, as it is the only legacy board that mistakenly uses the 'rgmii'
> phy-mode.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>  static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>  {
>  	struct device *dev = &ethqos->pdev->dev;
> -	int phase_shift;
> +	int phase_shift = 0;
>  	int loopback;
>  
>  	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
> -		phase_shift = 0;
> -	else
> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>  		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>  
>  	/* Disable loopback mode */
> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
> +
> +	root = of_find_node_by_path("/");
> +	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))


First, just check if machine is compatible, don't open code it.

Second, drivers should really, really not rely on the machine. I don't
think how this resolves ABI break for other users at all.

You need to check what the ABI is here and do not break it.


Best regards,
Krzysztof

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Russell King (Oracle)]

On Tue, Jan 21, 2025 at 01:47:55PM +0100, Krzysztof Kozlowski wrote:
> On 21/01/2025 08:54, Yijie Yang wrote:
> > The Qualcomm board always chooses the MAC to provide the delay instead of
> > the PHY, which is completely opposite to the suggestion of the Linux
> > kernel.

You still need to explain why it's preferable to match this in the mainline
kernel. Does it not work when you use the phylib maintainers suggestion
(if that is so, you need to state as much.)

> How does the Linux kernel suggest it?

It's what phylib maintainers prefer, as documented in many emails from
Andrew Lunn and in Documentation/networking/phy.rst:

"Whenever possible, use the PHY side RGMII delay for these reasons:

* PHY devices may offer sub-nanosecond granularity in how they allow a
  receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such
  precision may be required to account for differences in PCB trace lengths

* PHY devices are typically qualified for a large range of applications
  (industrial, medical, automotive...), and they provide a constant and
  reliable delay across temperature/pressure/voltage ranges

* PHY device drivers in PHYLIB being reusable by nature, being able to
  configure correctly a specified delay enables more designs with similar delay
  requirements to be operated correctly
"

> > The usage of phy-mode in legacy DTS was also incorrect. Change the
> > phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
> > to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
> > the definition.

If the delays dependent on the phy-mode are going to be implemented at
the MAC end, then changing the PHY mode indicated to phylink and phylib
to PHY_INTERFACE_MODE_RGMII is the right thing to be doing. However,
as mentioned in the documentation and by Andrew, this is discouraged.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 1/4] dt-bindings: net: ethernet-controller: Correct the definition of phy-mode

[Maxime Chevallier]

On Tue, 21 Jan 2025 15:54:53 +0800
Yijie Yang <quic_yijiyang@quicinc.com> wrote:

> Correct the definition of 'phy-mode' to reflect that RX and TX delays are
> added by the board, not the MAC, to prevent confusion and ensure accurate
> documentation.

That's not entirely correct though. The purpose of the RGMII variants
(TXID, RXID, ID) are mostly to know whether or not the PHY must add
internal delays. That would be when the MAC can't AND there's no PCB
delay traces. Some MAC can insert delays.

There's documentation here as well on that point :

https://elixir.bootlin.com/linux/v6.13-rc3/source/Documentation/networking/phy.rst#L82

So, MACs may insert delays. A modification for the doc, if needed,
would rather be :

-      # RX and TX delays are added by the MAC when required
+      # RX and TX delays are added by the MAC or PCB traces when required

Thanks,

Maxime

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Maxime Chevallier]

Hi,

On Tue, 21 Jan 2025 15:54:54 +0800
Yijie Yang <quic_yijiyang@quicinc.com> wrote:

> The Qualcomm board always chooses the MAC to provide the delay instead of
> the PHY, which is completely opposite to the suggestion of the Linux
> kernel. The usage of phy-mode in legacy DTS was also incorrect. Change the
> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
> the definition.
> To address the ABI compatibility issue between the kernel and DTS caused by
> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
> code, as it is the only legacy board that mistakenly uses the 'rgmii'
> phy-mode.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>  static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>  {
>  	struct device *dev = &ethqos->pdev->dev;
> -	int phase_shift;
> +	int phase_shift = 0;
>  	int loopback;
>  
>  	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
> -		phase_shift = 0;
> -	else
> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>  		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;

So this looks like a driver modification to deal with errors in
devicetree, and these modifications don't seem to be correct.

You should set RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN (i.e. adding a delay
n the TX line) when the PHY does not add internal delays on that line
(so, when the mode is rgmii or rgmii-rxid. The previous logic looks
correct in that regard.

Can you elaborate a bit more on the issue you are seeing ? On what
hardware is this happening ? What's the RGMII setup used (i.e. which
PHY, which mode, is there any delay lines on the PCB ?)

Thanks,

Maxime

Re: [PATCH v3 1/4] dt-bindings: net: ethernet-controller: Correct the definition of phy-mode

[Andrew Lunn]

On Tue, Jan 21, 2025 at 02:08:40PM +0100, Maxime Chevallier wrote:
> On Tue, 21 Jan 2025 15:54:53 +0800
> Yijie Yang <quic_yijiyang@quicinc.com> wrote:
> 
> > Correct the definition of 'phy-mode' to reflect that RX and TX delays are
> > added by the board, not the MAC, to prevent confusion and ensure accurate
> > documentation.
> 
> That's not entirely correct though. The purpose of the RGMII variants
> (TXID, RXID, ID) are mostly to know whether or not the PHY must add
> internal delays. That would be when the MAC can't AND there's no PCB
> delay traces. Some MAC can insert delays.
> 
> There's documentation here as well on that point :
> 
> https://elixir.bootlin.com/linux/v6.13-rc3/source/Documentation/networking/phy.rst#L82

This is part of the problem. This describes
PHY_INTERFACE_MODE_RGMII_*, and the value passed to phylib. The
documentation even says:

   The values of phy_interface_t must be understood from the
   perspective of the PHY device itself,

But the value in DT is about the board as a whole, it describes the
hardware. Software gets to decide if the MAC or the PHY adds the
delays, if the board does not provide the delay.

> So, MACs may insert delays. A modification for the doc, if needed,
> would rather be :
> 
> -      # RX and TX delays are added by the MAC when required
> +      # RX and TX delays are added by the MAC or PCB traces when required

From the perspective of the board, this is wrong. 'rgmii' means the
board provides the delays.

There is a parallel discussion going on, about how aspeed have also
got there implementation wrong. See:

https://lore.kernel.org/netdev/0ee94fd3-d099-4d82-9ba8-eb1939450cc3@lunn.ch/

and the test of that thread.

	Andrew

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Andrew Lunn]

> To address the ABI compatibility issue between the kernel and DTS caused by
> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
> code, as it is the only legacy board that mistakenly uses the 'rgmii'
> phy-mode.

Are you saying every other board DT got this correct? How do you know
that? Was this SoC never shipped to anybody, so the only possible
board is the QCOM RDK which never left the lab?

I doubt this is true, so you are probably breaking out of tree
boards. We care less about out of tree boards, but we also don't
needlessly break them.

	Andrew

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Andrew Lunn]

On Tue, Jan 21, 2025 at 03:54:54PM +0800, Yijie Yang wrote:
> The Qualcomm board always chooses the MAC to provide the delay instead of
> the PHY, which is completely opposite to the suggestion of the Linux
> kernel. The usage of phy-mode in legacy DTS was also incorrect. Change the
> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
> the definition.
> To address the ABI compatibility issue between the kernel and DTS caused by
> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
> code, as it is the only legacy board that mistakenly uses the 'rgmii'
> phy-mode.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>  static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>  {
>  	struct device *dev = &ethqos->pdev->dev;
> -	int phase_shift;
> +	int phase_shift = 0;
>  	int loopback;
>  
>  	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */

Please think about this comment.

       Andrew

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
> On 21/01/2025 08:54, Yijie Yang wrote:
>> The Qualcomm board always chooses the MAC to provide the delay instead of
>> the PHY, which is completely opposite to the suggestion of the Linux
>> kernel.
> 
> 
> How does the Linux kernel suggest it?
> 
>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>> the definition.
>> To address the ABI compatibility issue between the kernel and DTS caused by
>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>> phy-mode.
>>
>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>> ---
>>   .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>   static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>   {
>>   	struct device *dev = &ethqos->pdev->dev;
>> -	int phase_shift;
>> +	int phase_shift = 0;
>>   	int loopback;
>>   
>>   	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>> -		phase_shift = 0;
>> -	else
>> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>   		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>   
>>   	/* Disable loopback mode */
>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>   	if (ret)
>>   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>> +
>> +	root = of_find_node_by_path("/");
>> +	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
> 
> 
> First, just check if machine is compatible, don't open code it.
> 
> Second, drivers should really, really not rely on the machine. I don't
> think how this resolves ABI break for other users at all.

As detailed in the commit description, some boards mistakenly use the 
'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and 
added the delay for it. This code aims to prevent breaking these boards 
while correcting the erroneous parsing. This issue is similar to the one 
discussed in another thread:
https://lore.kernel.org/all/20241225-support_10m100m-v1-2-4b52ef48b488@quicinc.com/

> 
> You need to check what the ABI is here and do not break it.

If board compatible string matching is not recommended, can I address 
this historical issue by adding the rx-internal-delay-ps and 
tx-internal-delay-ps properties, as Andrew suggested in the thread 
mentioned above? Boards without this property are considered legacy 
boards and will follow the legacy routine, while others will apply the 
new routine. Similar examples include ksz_parse_rgmii_delay and 
sja1105_parse_rgmii_delays.

> 
> 
> Best regards,
> Krzysztof

-- 
Best Regards,
Yijie

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-01-21 20:57, Russell King (Oracle) wrote:
> On Tue, Jan 21, 2025 at 01:47:55PM +0100, Krzysztof Kozlowski wrote:
>> On 21/01/2025 08:54, Yijie Yang wrote:
>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>> the PHY, which is completely opposite to the suggestion of the Linux
>>> kernel.
> 
> You still need to explain why it's preferable to match this in the mainline
> kernel. Does it not work when you use the phylib maintainers suggestion
> (if that is so, you need to state as much.)

Okay, I will include that explanation in the next version.

> 
>> How does the Linux kernel suggest it?
> 
> It's what phylib maintainers prefer, as documented in many emails from
> Andrew Lunn and in Documentation/networking/phy.rst:
> 
> "Whenever possible, use the PHY side RGMII delay for these reasons:
> 
> * PHY devices may offer sub-nanosecond granularity in how they allow a
>    receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such
>    precision may be required to account for differences in PCB trace lengths
> 
> * PHY devices are typically qualified for a large range of applications
>    (industrial, medical, automotive...), and they provide a constant and
>    reliable delay across temperature/pressure/voltage ranges
> 
> * PHY device drivers in PHYLIB being reusable by nature, being able to
>    configure correctly a specified delay enables more designs with similar delay
>    requirements to be operated correctly
> "
> 
>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>> the definition.
> 
> If the delays dependent on the phy-mode are going to be implemented at
> the MAC end, then changing the PHY mode indicated to phylink and phylib
> to PHY_INTERFACE_MODE_RGMII is the right thing to be doing. However,
> as mentioned in the documentation and by Andrew, this is discouraged.
> 
Adding delay by the MAC side is generally discouraged, but the current 
driver configuration sequence was designed to do so. We need to follow 
this approach until we can rewrite it, correct?

-- 
Best Regards,
Yijie

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-01-21 21:17, Maxime Chevallier wrote:
> Hi,
> 
> On Tue, 21 Jan 2025 15:54:54 +0800
> Yijie Yang <quic_yijiyang@quicinc.com> wrote:
> 
>> The Qualcomm board always chooses the MAC to provide the delay instead of
>> the PHY, which is completely opposite to the suggestion of the Linux
>> kernel. The usage of phy-mode in legacy DTS was also incorrect. Change the
>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>> the definition.
>> To address the ABI compatibility issue between the kernel and DTS caused by
>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>> phy-mode.
>>
>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>> ---
>>   .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>   static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>   {
>>   	struct device *dev = &ethqos->pdev->dev;
>> -	int phase_shift;
>> +	int phase_shift = 0;
>>   	int loopback;
>>   
>>   	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>> -		phase_shift = 0;
>> -	else
>> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>   		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
> 
> So this looks like a driver modification to deal with errors in
> devicetree, and these modifications don't seem to be correct.
> 
> You should set RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN (i.e. adding a delay
> n the TX line) when the PHY does not add internal delays on that line
> (so, when the mode is rgmii or rgmii-rxid. The previous logic looks
> correct in that regard.
> 
> Can you elaborate a bit more on the issue you are seeing ? On what
> hardware is this happening ? What's the RGMII setup used (i.e. which
> PHY, which mode, is there any delay lines on the PCB ?)

As discussed following the first patch, the previous method of using 
'rgmii' in DTS while adding delay via the MAC was incorrect. We need to 
correct this misuse in both the DTS and the driver. For new boards, the 
phy-mode should be 'rgmii-id', while legacy boards will remain 'rgmii'. 
Both configurations will still enable 
RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN and allow the MAC to add the delay, 
ensuring the behavior remains consistent before and after the change.

> 
> Thanks,
> 
> Maxime

-- 
Best Regards,
Yijie

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Krzysztof Kozlowski]

On 22/01/2025 09:56, Yijie Yang wrote:
> 
> 
> On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
>> On 21/01/2025 08:54, Yijie Yang wrote:
>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>> the PHY, which is completely opposite to the suggestion of the Linux
>>> kernel.
>>
>>
>> How does the Linux kernel suggest it?
>>
>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>> the definition.
>>> To address the ABI compatibility issue between the kernel and DTS caused by
>>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>>> phy-mode.
>>>
>>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>>> ---
>>>   .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>>   static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>>   {
>>>   	struct device *dev = &ethqos->pdev->dev;
>>> -	int phase_shift;
>>> +	int phase_shift = 0;
>>>   	int loopback;
>>>   
>>>   	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>>> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>>> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>>> -		phase_shift = 0;
>>> -	else
>>> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>>   		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>>   
>>>   	/* Disable loopback mode */
>>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>   	if (ret)
>>>   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>> +
>>> +	root = of_find_node_by_path("/");
>>> +	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
>>
>>
>> First, just check if machine is compatible, don't open code it.
>>
>> Second, drivers should really, really not rely on the machine. I don't
>> think how this resolves ABI break for other users at all.
> 
> As detailed in the commit description, some boards mistakenly use the 
> 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and 

That's a kind of an ABI now, assuming it worked fine.

> added the delay for it. This code aims to prevent breaking these boards 
> while correcting the erroneous parsing. This issue is similar to the one 
> discussed in another thread:
> https://lore.kernel.org/all/20241225-support_10m100m-v1-2-4b52ef48b488@quicinc.com/
> 
>>
>> You need to check what the ABI is here and do not break it.
> 
> If board compatible string matching is not recommended, can I address 

And what if affected board does not match the compatible, because you
did not include it here? I said this code does not prevent breaking ABI
and you did not address it. To my comment you need to consider ABI, you
repeat the same, so kind of something irrelevant. I have feelings you
want to push your code, just like in the past several of your
colleagues. Learning from past mistakes I will not engage in such
discussions, so please really rethink the concept and comments you
received here.

That's a NAK.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-01-22 01:10, Andrew Lunn wrote:
>> To address the ABI compatibility issue between the kernel and DTS caused by
>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>> phy-mode.
> 
> Are you saying every other board DT got this correct? How do you know
> that? Was this SoC never shipped to anybody, so the only possible
> board is the QCOM RDK which never left the lab?
> 
> I doubt this is true, so you are probably breaking out of tree
> boards. We care less about out of tree boards, but we also don't
> needlessly break them.

You're right. My conclusion was based solely on browsing all RGMII-type 
Qualcomm boards under the source tree, and I didn't take the scenario 
you mentioned into account. I will work on coming up with a new 
solution. If you have any suggestions, I will gladly take them as a 
reference.

> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Konrad Dybcio]

On 22.01.2025 10:48 AM, Krzysztof Kozlowski wrote:
> On 22/01/2025 09:56, Yijie Yang wrote:
>>
>>
>> On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
>>> On 21/01/2025 08:54, Yijie Yang wrote:
>>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>>> the PHY, which is completely opposite to the suggestion of the Linux
>>>> kernel.
>>>
>>>
>>> How does the Linux kernel suggest it?
>>>
>>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>>> the definition.
>>>> To address the ABI compatibility issue between the kernel and DTS caused by
>>>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>>>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>>>> phy-mode.
>>>>
>>>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>>>> ---
>>>>   .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>>>   static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>>>   {
>>>>   	struct device *dev = &ethqos->pdev->dev;
>>>> -	int phase_shift;
>>>> +	int phase_shift = 0;
>>>>   	int loopback;
>>>>   
>>>>   	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>>>> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>>>> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>>>> -		phase_shift = 0;
>>>> -	else
>>>> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>>>   		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>>>   
>>>>   	/* Disable loopback mode */
>>>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>   	if (ret)
>>>>   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>> +
>>>> +	root = of_find_node_by_path("/");
>>>> +	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
>>>
>>>
>>> First, just check if machine is compatible, don't open code it.
>>>
>>> Second, drivers should really, really not rely on the machine. I don't
>>> think how this resolves ABI break for other users at all.
>>
>> As detailed in the commit description, some boards mistakenly use the 
>> 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and 
> 
> That's a kind of an ABI now, assuming it worked fine.

I'm inclined to think it's better to drop compatibility given we're talking
about rather obscure boards here.

$ rg 'mode.*=.*"rgmii"' arch/arm64/boot/dts/qcom -l

arch/arm64/boot/dts/qcom/sa8155p-adp.dts
arch/arm64/boot/dts/qcom/qcs404-evb-4000.dts

QCS404 seems to have zero interest from anyone (and has been considered
for removal upstream..).

The ADP doesn't see much traction either, last time around someone found
a boot breaking issue months after it was committed.

Konrad

Re: [PATCH v3 3/4] arm64: dts: qcom: qcs615: add ethernet node

[Konrad Dybcio]

On 21.01.2025 8:54 AM, Yijie Yang wrote:
> Add an ethernet controller node for QCS615 SoC to enable ethernet
> functionality.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v3 4/4] arm64: dts: qcom: qcs615-ride: Enable ethernet node

[Konrad Dybcio]

On 21.01.2025 8:54 AM, Yijie Yang wrote:
> Enable the ethernet node, add the phy node and pinctrl for ethernet. This
> change is necessary to support ethernet functionality on this board.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v3 3/4] arm64: dts: qcom: qcs615: add ethernet node

[Konrad Dybcio]

On 1.02.2025 4:48 PM, Konrad Dybcio wrote:
> On 21.01.2025 8:54 AM, Yijie Yang wrote:
>> Add an ethernet controller node for QCS615 SoC to enable ethernet
>> functionality.
>>
>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>> ---
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Wrong copypasta, please use this one instead:

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-01-27 18:49, Konrad Dybcio wrote:
> On 22.01.2025 10:48 AM, Krzysztof Kozlowski wrote:
>> On 22/01/2025 09:56, Yijie Yang wrote:
>>>
>>>
>>> On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
>>>> On 21/01/2025 08:54, Yijie Yang wrote:
>>>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>>>> the PHY, which is completely opposite to the suggestion of the Linux
>>>>> kernel.
>>>>
>>>>
>>>> How does the Linux kernel suggest it?
>>>>
>>>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>>>> the definition.
>>>>> To address the ABI compatibility issue between the kernel and DTS caused by
>>>>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>>>>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>>>>> phy-mode.
>>>>>
>>>>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>>>>> ---
>>>>>    .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>>>>    static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>>>>    {
>>>>>    	struct device *dev = &ethqos->pdev->dev;
>>>>> -	int phase_shift;
>>>>> +	int phase_shift = 0;
>>>>>    	int loopback;
>>>>>    
>>>>>    	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>>>>> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>> -		phase_shift = 0;
>>>>> -	else
>>>>> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>>>>    		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>>>>    
>>>>>    	/* Disable loopback mode */
>>>>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>    	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>>    	if (ret)
>>>>>    		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>> +
>>>>> +	root = of_find_node_by_path("/");
>>>>> +	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
>>>>
>>>>
>>>> First, just check if machine is compatible, don't open code it.
>>>>
>>>> Second, drivers should really, really not rely on the machine. I don't
>>>> think how this resolves ABI break for other users at all.
>>>
>>> As detailed in the commit description, some boards mistakenly use the
>>> 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and
>>
>> That's a kind of an ABI now, assuming it worked fine.
> 
> I'm inclined to think it's better to drop compatibility given we're talking
> about rather obscure boards here.
> 
> $ rg 'mode.*=.*"rgmii"' arch/arm64/boot/dts/qcom -l
> 
> arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> arch/arm64/boot/dts/qcom/qcs404-evb-4000.dts
> 
> QCS404 seems to have zero interest from anyone (and has been considered
> for removal upstream..).
> 
> The ADP doesn't see much traction either, last time around someone found
> a boot breaking issue months after it was committed.

But what about the out-of-tree boards that Andrew mentioned? We need to 
ensure we don't break them, right?

> 
> Konrad

-- 
Best Regards,
Yijie

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Konrad Dybcio]

On 10.02.2025 4:09 AM, Yijie Yang wrote:
> 
> 
> On 2025-01-27 18:49, Konrad Dybcio wrote:
>> On 22.01.2025 10:48 AM, Krzysztof Kozlowski wrote:
>>> On 22/01/2025 09:56, Yijie Yang wrote:
>>>>
>>>>
>>>> On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
>>>>> On 21/01/2025 08:54, Yijie Yang wrote:
>>>>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>>>>> the PHY, which is completely opposite to the suggestion of the Linux
>>>>>> kernel.
>>>>>
>>>>>
>>>>> How does the Linux kernel suggest it?
>>>>>
>>>>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>>>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>>>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>>>>> the definition.
>>>>>> To address the ABI compatibility issue between the kernel and DTS caused by
>>>>>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>>>>>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>>>>>> phy-mode.
>>>>>>
>>>>>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>>>>>> ---
>>>>>>    .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>>>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>>>>>    static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>>>>>    {
>>>>>>        struct device *dev = &ethqos->pdev->dev;
>>>>>> -    int phase_shift;
>>>>>> +    int phase_shift = 0;
>>>>>>        int loopback;
>>>>>>           /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>>>>>> -    if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>>> -        ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>>> -        phase_shift = 0;
>>>>>> -    else
>>>>>> +    if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>>>>>            phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>>>>>           /* Disable loopback mode */
>>>>>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>>        ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>>>        if (ret)
>>>>>>            return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>>> +
>>>>>> +    root = of_find_node_by_path("/");
>>>>>> +    if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
>>>>>
>>>>>
>>>>> First, just check if machine is compatible, don't open code it.
>>>>>
>>>>> Second, drivers should really, really not rely on the machine. I don't
>>>>> think how this resolves ABI break for other users at all.
>>>>
>>>> As detailed in the commit description, some boards mistakenly use the
>>>> 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and
>>>
>>> That's a kind of an ABI now, assuming it worked fine.
>>
>> I'm inclined to think it's better to drop compatibility given we're talking
>> about rather obscure boards here.
>>
>> $ rg 'mode.*=.*"rgmii"' arch/arm64/boot/dts/qcom -l
>>
>> arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> arch/arm64/boot/dts/qcom/qcs404-evb-4000.dts
>>
>> QCS404 seems to have zero interest from anyone (and has been considered
>> for removal upstream..).
>>
>> The ADP doesn't see much traction either, last time around someone found
>> a boot breaking issue months after it was committed.
> 
> But what about the out-of-tree boards that Andrew mentioned? We need to ensure we don't break them, right?

No. What's not on the list, doesn't exist

Konrad

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Andrew Lunn]

> > But what about the out-of-tree boards that Andrew mentioned? We need to ensure we don't break them, right?
> 
> No. What's not on the list, doesn't exist

How i worded it was:

> We care less about out of tree boards, but we also don't needlessly
> break them.

I guess if Qualcomm wants to break all its customers boards, that is
up to Qualcomm. But we can also make it easier for Qualcomm customers
to get off the vendor crap kernel and to mainline if we at least give
them an easier migration path.

     Andrew

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-02-11 02:01, Konrad Dybcio wrote:
> On 10.02.2025 4:09 AM, Yijie Yang wrote:
>>
>>
>> On 2025-01-27 18:49, Konrad Dybcio wrote:
>>> On 22.01.2025 10:48 AM, Krzysztof Kozlowski wrote:
>>>> On 22/01/2025 09:56, Yijie Yang wrote:
>>>>>
>>>>>
>>>>> On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
>>>>>> On 21/01/2025 08:54, Yijie Yang wrote:
>>>>>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>>>>>> the PHY, which is completely opposite to the suggestion of the Linux
>>>>>>> kernel.
>>>>>>
>>>>>>
>>>>>> How does the Linux kernel suggest it?
>>>>>>
>>>>>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>>>>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>>>>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>>>>>> the definition.
>>>>>>> To address the ABI compatibility issue between the kernel and DTS caused by
>>>>>>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>>>>>>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>>>>>>> phy-mode.
>>>>>>>
>>>>>>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>>>>>>> ---
>>>>>>>     .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>>>>>>     1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>>>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>>>>>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>>>>>>     static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>>>>>>     {
>>>>>>>         struct device *dev = &ethqos->pdev->dev;
>>>>>>> -    int phase_shift;
>>>>>>> +    int phase_shift = 0;
>>>>>>>         int loopback;
>>>>>>>            /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>>>>>>> -    if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>>>> -        ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>>>> -        phase_shift = 0;
>>>>>>> -    else
>>>>>>> +    if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>>>>>>             phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>>>>>>            /* Disable loopback mode */
>>>>>>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>>>         ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>>>>         if (ret)
>>>>>>>             return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>>>> +
>>>>>>> +    root = of_find_node_by_path("/");
>>>>>>> +    if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
>>>>>>
>>>>>>
>>>>>> First, just check if machine is compatible, don't open code it.
>>>>>>
>>>>>> Second, drivers should really, really not rely on the machine. I don't
>>>>>> think how this resolves ABI break for other users at all.
>>>>>
>>>>> As detailed in the commit description, some boards mistakenly use the
>>>>> 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and
>>>>
>>>> That's a kind of an ABI now, assuming it worked fine.
>>>
>>> I'm inclined to think it's better to drop compatibility given we're talking
>>> about rather obscure boards here.
>>>
>>> $ rg 'mode.*=.*"rgmii"' arch/arm64/boot/dts/qcom -l
>>>
>>> arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>>> arch/arm64/boot/dts/qcom/qcs404-evb-4000.dts
>>>
>>> QCS404 seems to have zero interest from anyone (and has been considered
>>> for removal upstream..).
>>>
>>> The ADP doesn't see much traction either, last time around someone found
>>> a boot breaking issue months after it was committed.
>>
>> But what about the out-of-tree boards that Andrew mentioned? We need to ensure we don't break them, right?
> 
> No. What's not on the list, doesn't exist

Okay, I understand.

> 
> Konrad

-- 
Best Regards,
Yijie

Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

[Yijie Yang]

On 2025-02-11 05:28, Andrew Lunn wrote:
>>> But what about the out-of-tree boards that Andrew mentioned? We need to ensure we don't break them, right?
>>
>> No. What's not on the list, doesn't exist
> 
> How i worded it was:
> 
>> We care less about out of tree boards, but we also don't needlessly
>> break them.
> 
> I guess if Qualcomm wants to break all its customers boards, that is
> up to Qualcomm. But we can also make it easier for Qualcomm customers
> to get off the vendor crap kernel and to mainline if we at least give
> them an easier migration path.

I understand the importance of not breaking customers' boards and will 
work on finding a better solution than the current one.
However, if no better solutions are found, we will consider dropping the 
compatibility.

> 
>       Andrew

-- 
Best Regards,
Yijie

Re: [PATCH v3 0/4] Enable ethernet on qcs615

[Yijie Yang]

On 2025-01-21 15:54, Yijie Yang wrote:
> Correct the definition and usage of phy-mode in both the DT binding and
> driver code.
> Add dts nodes and EMAC driver data to enable ethernet interface on
> qcs615-ride platform.
> The EMAC version currently in use on this platform is the same as that in
> qcs404. The EPHY model is Micrel KSZ9031.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
> Changes in v3:
> - Correct the definition of 'rgmii' in ethernet-controller.yaml.
> - Remove the redundant max-speed limit in the dts file.
> - Update the definition of 'rgmii' to prevent any further misunderstandings.
> - Update the phy-mode in the dts file from rgmii to rgmii-id.
> - Mask the PHY mode passed to the driver to allow the MAC to add the delay.
> - Update the low power mode exit interrupt from 662 to 661.
> - Update the compatible string to fallback to qcs404 since they share the same hardware.
> - Update base commit to next-20250120.
> - Link to v2: https://lore.kernel.org/r/20241118-dts_qcs615-v2-0-e62b924a3cbd@quicinc.com
> 
> ---
> Yijie Yang (4):
>        dt-bindings: net: ethernet-controller: Correct the definition of phy-mode
>        net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id
>        arm64: dts: qcom: qcs615: add ethernet node
>        arm64: dts: qcom: qcs615-ride: Enable ethernet node
> 
>   .../bindings/net/ethernet-controller.yaml          |   2 +-
>   arch/arm64/boot/dts/qcom/qcs615-ride.dts           | 104 +++++++++++++++++++++
>   arch/arm64/boot/dts/qcom/qcs615.dtsi               |  34 +++++++
>   .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |  18 +++-
>   4 files changed, 152 insertions(+), 6 deletions(-)
> ---
> base-commit: 9424d9acada6461344c71ac02f2f3fbcdd775498
> change-id: 20241224-dts_qcs615-9612efea02cb
> prerequisite-message-id: <20250120-schema_qcs615-v4-1-d9d122f89e64@quicinc.com>
> prerequisite-patch-id: b97f36116c87036abe66e061db82588eb1bbaa9a
> 
> Best regards,


Since my last submission, I’ve been working on an updated version of the 
patch that incorporates the feedback received and improves the overall 
implementation. I plan to submit the revised version shortly.

Please let me know if there are any additional concerns or changes in 
direction I should be aware of. I’d really appreciate any guidance to 
ensure the patch aligns well with the current goals of the subsystem.


-- 
Best Regards,
Yijie