[PATCH 0/9] arm64: dts: qcom: enable EMAC1 on sa8775p

[PATCH 0/9] arm64: dts: qcom: enable EMAC1 on sa8775p

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This series contains changes required to enable EMAC1 on sa8775p-ride.

Bartosz Golaszewski (9):
  arm64: dts: qcom: sa8775p: add a node for the second serdes PHY
  arm64: dts: qcom: sa8775p: add a node for EMAC1
  arm64: dts: qcom: sa8775p-ride: enable the second SerDes PHY
  arm64: dts: qcom: sa8775p-ride: add pin functions for ethernet1
  arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the
    PHY
  arm64: dts: qcom: sa8775p-ride: index the first SGMII PHY
  arm64: dts: qcom: sa8775p-ride: add the second SGMII PHY
  arm64: dts: qcom: sa8775p-ride: label the mdio node
  arm64: dts: qcom: sa8775p-ride: enable EMAC1

 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 116 ++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/sa8775p.dtsi     |  43 ++++++++
 2 files changed, 152 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH 1/9] arm64: dts: qcom: sa8775p: add a node for the second serdes PHY

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 7b55cb701472..38d10af37ab0 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -1846,6 +1846,15 @@ serdes0: phy@8901000 {
 			status = "disabled";
 		};
 
+		serdes1: phy@8902000 {
+			compatible = "qcom,sa8775p-dwmac-sgmii-phy";
+			reg = <0x0 0x08902000 0x0 0xe10>;
+			clocks = <&gcc GCC_SGMI_CLKREF_EN>;
+			clock-names = "sgmi_ref";
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
 		pdc: interrupt-controller@b220000 {
 			compatible = "qcom,sa8775p-pdc", "qcom,pdc";
 			reg = <0x0 0x0b220000 0x0 0x30000>,
-- 
2.39.2

[PATCH 2/9] arm64: dts: qcom: sa8775p: add a node for EMAC1

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a node for the second MAC on sa8775p platforms.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 34 +++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 38d10af37ab0..82af2e6cbda4 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -2325,6 +2325,40 @@ cpufreq_hw: cpufreq@18591000 {
 			#freq-domain-cells = <1>;
 		};
 
+		ethernet1: ethernet@23000000 {
+			compatible = "qcom,sa8775p-ethqos";
+			reg = <0x0 0x23000000 0x0 0x10000>,
+			      <0x0 0x23016000 0x0 0x100>;
+			reg-names = "stmmaceth", "rgmii";
+
+			interrupts = <GIC_SPI 929 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq";
+
+			clocks = <&gcc GCC_EMAC1_AXI_CLK>,
+				 <&gcc GCC_EMAC1_SLV_AHB_CLK>,
+				 <&gcc GCC_EMAC1_PTP_CLK>,
+				 <&gcc GCC_EMAC1_PHY_AUX_CLK>;
+
+			clock-names = "stmmaceth",
+				      "pclk",
+				      "ptp_ref",
+				      "phyaux";
+
+			power-domains = <&gcc EMAC1_GDSC>;
+
+			phys = <&serdes1>;
+			phy-names = "serdes";
+
+			iommus = <&apps_smmu 0x140 0xf>;
+
+			snps,tso;
+			snps,pbl = <32>;
+			rx-fifo-depth = <16384>;
+			tx-fifo-depth = <16384>;
+
+			status = "disabled";
+		};
+
 		ethernet0: ethernet@23040000 {
 			compatible = "qcom,sa8775p-ethqos";
 			reg = <0x0 0x23040000 0x0 0x10000>,
-- 
2.39.2

[PATCH 3/9] arm64: dts: qcom: sa8775p-ride: enable the second SerDes PHY

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Enable the second SerDes PHY on sa8775p-ride development board.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index ed76680410b4..09ae6e153282 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -448,6 +448,11 @@ &serdes0 {
 	status = "okay";
 };
 
+&serdes1 {
+	phy-supply = <&vreg_l5a>;
+	status = "okay";
+};
+
 &sleep_clk {
 	clock-frequency = <32764>;
 };
-- 
2.39.2

[PATCH 4/9] arm64: dts: qcom: sa8775p-ride: add pin functions for ethernet1

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add the MDC and MDIO pin functions for ethernet1 on sa8775p-ride.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index 09ae6e153282..38327aff18b0 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -480,6 +480,22 @@ ethernet0_mdio: ethernet0-mdio-pins {
 		};
 	};
 
+	ethernet1_default: ethernet1-default-state {
+		ethernet1_mdc: ethernet1-mdc-pins {
+			pins = "gpio20";
+			function = "emac1_mdc";
+			drive-strength = <16>;
+			bias-pull-up;
+		};
+
+		ethernet1_mdio: ethernet1-mdio-pins {
+			pins = "gpio21";
+			function = "emac1_mdio";
+			drive-strength = <16>;
+			bias-pull-up;
+		};
+	};
+
 	qup_uart10_default: qup-uart10-state {
 		pins = "gpio46", "gpio47";
 		function = "qup1_se3";
-- 
2.39.2

[PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Device-tree bindings for MDIO define per-PHY reset-gpios as well as a
global reset-gpios property at the MDIO node level which controlls all
devices on the bus. The latter is most likely a workaround for the
chicken-and-egg problem where we cannot read the ID of the PHY before
bringing it out of reset but we cannot bring it out of reset until we've
read its ID.

I have proposed a solution for this problem in 2020 but it never got
upstream. Now we have a workaround in place which allows us to hard-code
the PHY id in the compatible property, thus skipping the ID scanning).

Let's make the device-tree for sa8775p-ride slightly more correct by
moving the reset-gpios property to the PHY node with its ID put into the
PHY node's compatible.

Link: https://lore.kernel.org/all/20200622093744.13685-1-brgl@bgdev.pl/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index 38327aff18b0..1c471278d441 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -279,13 +279,12 @@ mdio {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
-		reset-delay-us = <11000>;
-		reset-post-delay-us = <70000>;
-
 		sgmii_phy: phy@8 {
+			compatible = "ethernet-phy-id0141.0dd4";
 			reg = <0x8>;
 			device_type = "ethernet-phy";
+			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
+			reset-deassert-us = <70000>;
 		};
 	};
 
-- 
2.39.2

[PATCH 6/9] arm64: dts: qcom: sa8775p-ride: index the first SGMII PHY

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We'll be adding a second SGMII PHY on the same MDIO bus, so let's index
the first one for better readability.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index 1c471278d441..55feaac7fa1b 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -263,7 +263,7 @@ vreg_l8e: ldo8 {
 
 &ethernet0 {
 	phy-mode = "sgmii";
-	phy-handle = <&sgmii_phy>;
+	phy-handle = <&sgmii_phy0>;
 
 	pinctrl-0 = <&ethernet0_default>;
 	pinctrl-names = "default";
@@ -279,7 +279,7 @@ mdio {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		sgmii_phy: phy@8 {
+		sgmii_phy0: phy@8 {
 			compatible = "ethernet-phy-id0141.0dd4";
 			reg = <0x8>;
 			device_type = "ethernet-phy";
-- 
2.39.2

[PATCH 7/9] arm64: dts: qcom: sa8775p-ride: add the second SGMII PHY

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a second SGMII PHY that will be used by EMAC1 on sa8775p-ride.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index 55feaac7fa1b..5b48066f312a 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -286,6 +286,14 @@ sgmii_phy0: phy@8 {
 			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
 			reset-deassert-us = <70000>;
 		};
+
+		sgmii_phy1: phy@a {
+			compatible = "ethernet-phy-id0141.0dd4";
+			reg = <0xa>;
+			device_type = "ethernet-phy";
+			reset-gpios = <&pmm8654au_2_gpios 9 GPIO_ACTIVE_LOW>;
+			reset-deassert-us = <70000>;
+		};
 	};
 
 	mtl_rx_setup: rx-queues-config {
-- 
2.39.2

[PATCH 8/9] arm64: dts: qcom: sa8775p-ride: label the mdio node

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a label to the MDIO node on ethernet0 so that we can reference it
from the ethernet1's node.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index 5b48066f312a..af50aa2d9b10 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -274,7 +274,7 @@ &ethernet0 {
 
 	status = "okay";
 
-	mdio {
+	mdio0: mdio {
 		compatible = "snps,dwmac-mdio";
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.39.2

[PATCH 9/9] arm64: dts: qcom: sa8775p-ride: enable EMAC1

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Enable the second MAC on sa8775p-ride.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 74 +++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index af50aa2d9b10..0862bfb4c580 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -356,6 +356,80 @@ queue3 {
 	};
 };
 
+&ethernet1 {
+	phy-mode = "sgmii";
+	phy-handle = <&sgmii_phy1>;
+
+	pinctrl-0 = <&ethernet1_default>;
+	pinctrl-names = "default";
+
+	snps,mtl-rx-config = <&mtl_rx_setup1>;
+	snps,mtl-tx-config = <&mtl_tx_setup1>;
+	snps,ps-speed = <1000>;
+	snps,shared-mdio = <&mdio0>;
+
+	status = "okay";
+
+	mtl_rx_setup1: rx-queues-config {
+		snps,rx-queues-to-use = <4>;
+		snps,rx-sched-sp;
+
+		queue0 {
+			snps,dcb-algorithm;
+			snps,map-to-dma-channel = <0x0>;
+			snps,route-up;
+			snps,priority = <0x1>;
+		};
+
+		queue1 {
+			snps,dcb-algorithm;
+			snps,map-to-dma-channel = <0x1>;
+			snps,route-ptp;
+		};
+
+		queue2 {
+			snps,avb-algorithm;
+			snps,map-to-dma-channel = <0x2>;
+			snps,route-avcp;
+		};
+
+		queue3 {
+			snps,avb-algorithm;
+			snps,map-to-dma-channel = <0x3>;
+			snps,priority = <0xc>;
+		};
+	};
+
+	mtl_tx_setup1: tx-queues-config {
+		snps,tx-queues-to-use = <4>;
+		snps,tx-sched-sp;
+
+		queue0 {
+			snps,dcb-algorithm;
+		};
+
+		queue1 {
+			snps,dcb-algorithm;
+		};
+
+		queue2 {
+			snps,avb-algorithm;
+			snps,send_slope = <0x1000>;
+			snps,idle_slope = <0x1000>;
+			snps,high_credit = <0x3e800>;
+			snps,low_credit = <0xffc18000>;
+		};
+
+		queue3 {
+			snps,avb-algorithm;
+			snps,send_slope = <0x1000>;
+			snps,idle_slope = <0x1000>;
+			snps,high_credit = <0x3e800>;
+			snps,low_credit = <0xffc18000>;
+		};
+	};
+};
+
 &i2c11 {
 	clock-frequency = <400000>;
 	pinctrl-0 = <&qup_i2c11_default>;
-- 
2.39.2

Re: [PATCH 9/9] arm64: dts: qcom: sa8775p-ride: enable EMAC1

[Andrew Lunn]

On Mon, Aug 07, 2023 at 09:35:07PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Enable the second MAC on sa8775p-ride.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 74 +++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index af50aa2d9b10..0862bfb4c580 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -356,6 +356,80 @@ queue3 {
>  	};
>  };
>  
> +&ethernet1 {
> +	phy-mode = "sgmii";
> +	phy-handle = <&sgmii_phy1>;

This should be all you need.

> +	snps,shared-mdio = <&mdio0>;

This is not needed, since it is implicit in the phy-handle.

You need to debug why the existing -EPROBE_DEFER is not working.

    Andrew

Re: [PATCH 1/9] arm64: dts: qcom: sa8775p: add a node for the second serdes PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

FWIW this seems to match downstream sources.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 7b55cb701472..38d10af37ab0 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -1846,6 +1846,15 @@ serdes0: phy@8901000 {
>  			status = "disabled";
>  		};
>  
> +		serdes1: phy@8902000 {
> +			compatible = "qcom,sa8775p-dwmac-sgmii-phy";
> +			reg = <0x0 0x08902000 0x0 0xe10>;
> +			clocks = <&gcc GCC_SGMI_CLKREF_EN>;
> +			clock-names = "sgmi_ref";
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +
>  		pdc: interrupt-controller@b220000 {
>  			compatible = "qcom,sa8775p-pdc", "qcom,pdc";
>  			reg = <0x0 0x0b220000 0x0 0x30000>,
> -- 
> 2.39.2
> 

Re: [PATCH 2/9] arm64: dts: qcom: sa8775p: add a node for EMAC1

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:00PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a node for the second MAC on sa8775p platforms.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 34 +++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 38d10af37ab0..82af2e6cbda4 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -2325,6 +2325,40 @@ cpufreq_hw: cpufreq@18591000 {
>  			#freq-domain-cells = <1>;
>  		};
>  
> +		ethernet1: ethernet@23000000 {
> +			compatible = "qcom,sa8775p-ethqos";
> +			reg = <0x0 0x23000000 0x0 0x10000>,
> +			      <0x0 0x23016000 0x0 0x100>;
> +			reg-names = "stmmaceth", "rgmii";
> +
> +			interrupts = <GIC_SPI 929 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "macirq";
> +
> +			clocks = <&gcc GCC_EMAC1_AXI_CLK>,
> +				 <&gcc GCC_EMAC1_SLV_AHB_CLK>,
> +				 <&gcc GCC_EMAC1_PTP_CLK>,
> +				 <&gcc GCC_EMAC1_PHY_AUX_CLK>;
> +
> +			clock-names = "stmmaceth",
> +				      "pclk",
> +				      "ptp_ref",
> +				      "phyaux";
> +
> +			power-domains = <&gcc EMAC1_GDSC>;
> +
> +			phys = <&serdes1>;
> +			phy-names = "serdes";
> +
> +			iommus = <&apps_smmu 0x140 0xf>;
> +
> +			snps,tso;
> +			snps,pbl = <32>;
> +			rx-fifo-depth = <16384>;
> +			tx-fifo-depth = <16384>;
> +
> +			status = "disabled";
> +		};
> +
>  		ethernet0: ethernet@23040000 {
>  			compatible = "qcom,sa8775p-ethqos";
>  			reg = <0x0 0x23040000 0x0 0x10000>,
> -- 
> 2.39.2
> 

Re: [PATCH 3/9] arm64: dts: qcom: sa8775p-ride: enable the second SerDes PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:01PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Enable the second SerDes PHY on sa8775p-ride development board.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Matches what I see downstream wrt the supply, so:

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index ed76680410b4..09ae6e153282 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -448,6 +448,11 @@ &serdes0 {
>  	status = "okay";
>  };
>  
> +&serdes1 {
> +	phy-supply = <&vreg_l5a>;
> +	status = "okay";
> +};
> +
>  &sleep_clk {
>  	clock-frequency = <32764>;
>  };
> -- 
> 2.39.2
> 

Re: [PATCH 4/9] arm64: dts: qcom: sa8775p-ride: add pin functions for ethernet1

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:02PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add the MDC and MDIO pin functions for ethernet1 on sa8775p-ride.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index 09ae6e153282..38327aff18b0 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -480,6 +480,22 @@ ethernet0_mdio: ethernet0-mdio-pins {
>  		};
>  	};
>  
> +	ethernet1_default: ethernet1-default-state {
> +		ethernet1_mdc: ethernet1-mdc-pins {
> +			pins = "gpio20";
> +			function = "emac1_mdc";
> +			drive-strength = <16>;
> +			bias-pull-up;
> +		};
> +
> +		ethernet1_mdio: ethernet1-mdio-pins {
> +			pins = "gpio21";
> +			function = "emac1_mdio";
> +			drive-strength = <16>;
> +			bias-pull-up;
> +		};
> +	};
> +

With the whole "EMAC0 MDIO handles the ethernet phy for EMAC1", this
doesn't seem to make sense.

I don't have all the schematics, but these pins are not connected from
what I see.

Re: [PATCH 1/9] arm64: dts: qcom: sa8775p: add a node for the second serdes PHY

[Andrew Lunn]

On Mon, Aug 07, 2023 at 04:10:34PM -0500, Andrew Halaney wrote:
> On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
> > 
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> FWIW this seems to match downstream sources.
> 
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

Why does matching downstream make it correct and deserve a
Reviewed-by?

Did you actually review the change?

    Andrew

Re: [PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:03PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Device-tree bindings for MDIO define per-PHY reset-gpios as well as a
> global reset-gpios property at the MDIO node level which controlls all

s/controlls/controls/

> devices on the bus. The latter is most likely a workaround for the
> chicken-and-egg problem where we cannot read the ID of the PHY before
> bringing it out of reset but we cannot bring it out of reset until we've
> read its ID.
> 
> I have proposed a solution for this problem in 2020 but it never got
> upstream. Now we have a workaround in place which allows us to hard-code
> the PHY id in the compatible property, thus skipping the ID scanning).

nitpicky, but I think that already existed at that time :D

> 
> Let's make the device-tree for sa8775p-ride slightly more correct by
> moving the reset-gpios property to the PHY node with its ID put into the
> PHY node's compatible.
> 
> Link: https://lore.kernel.org/all/20200622093744.13685-1-brgl@bgdev.pl/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index 38327aff18b0..1c471278d441 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -279,13 +279,12 @@ mdio {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> -		reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> -		reset-delay-us = <11000>;
> -		reset-post-delay-us = <70000>;
> -
>  		sgmii_phy: phy@8 {
> +			compatible = "ethernet-phy-id0141.0dd4";
>  			reg = <0x8>;
>  			device_type = "ethernet-phy";
> +			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> +			reset-deassert-us = <70000>;

Doesn't this need reset-assert-us?

Re: [PATCH 1/9] arm64: dts: qcom: sa8775p: add a node for the second serdes PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 11:28:15PM +0200, Andrew Lunn wrote:
> On Mon, Aug 07, 2023 at 04:10:34PM -0500, Andrew Halaney wrote:
> > On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > 
> > > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
> > > 
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > FWIW this seems to match downstream sources.
> > 
> > Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> 
> Why does matching downstream make it correct and deserve a
> Reviewed-by?
> 
> Did you actually review the change?
> 
>     Andrew
> 

That wording of "match downstream sources" is amiguous, sorry.

What I meant was that:

    1. This looks like a properly formatted dtsi node, follows
       conventions in the file for ordering, etc
    2. The downstream devicetree from Qualcomm uses the same MMIO region
       same dependencies for clocks, etc. I do not have documentation
       to further review past that

I didn't suspect anyone else would cross check this information, so I
did and thought it deserves a RB after that. Please let me know if you'd
rather I just leave a comment saying such without any formal tags (but I
thought since the whole patch looks proper and that information looks
correct compared to the "documentation" I have access to it deserved
such).

In the rest of this series review I may have used similar phrasing with
respect to downstream, but that expanded description is what I meant in
those cases as well.

Thanks,
Andrew

Re: [PATCH 4/9] arm64: dts: qcom: sa8775p-ride: add pin functions for ethernet1

[Andrew Lunn]

On Mon, Aug 07, 2023 at 04:18:21PM -0500, Andrew Halaney wrote:
> On Mon, Aug 07, 2023 at 09:35:02PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > Add the MDC and MDIO pin functions for ethernet1 on sa8775p-ride.
> > 
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> > index 09ae6e153282..38327aff18b0 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> > +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> > @@ -480,6 +480,22 @@ ethernet0_mdio: ethernet0-mdio-pins {
> >  		};
> >  	};
> >  
> > +	ethernet1_default: ethernet1-default-state {
> > +		ethernet1_mdc: ethernet1-mdc-pins {
> > +			pins = "gpio20";
> > +			function = "emac1_mdc";
> > +			drive-strength = <16>;
> > +			bias-pull-up;
> > +		};
> > +
> > +		ethernet1_mdio: ethernet1-mdio-pins {
> > +			pins = "gpio21";
> > +			function = "emac1_mdio";
> > +			drive-strength = <16>;
> > +			bias-pull-up;
> > +		};
> > +	};
> > +
> 
> With the whole "EMAC0 MDIO handles the ethernet phy for EMAC1", this
> doesn't seem to make sense.
> 
> I don't have all the schematics, but these pins are not connected from
> what I see.

I kind of agree. I've seen different ways of describing pinmux. What
i've done for Kirkwood was to put all the common pinmux configurations
into the SoC .dtsi file. The .dts file can then reference it if
needed.

In this case, since the bus is unused, it seems odd to mux it. And
later versions of the board could actually use the pins for something
else, GPIOs etc.

      Andrew

Re: [PATCH 6/9] arm64: dts: qcom: sa8775p-ride: index the first SGMII PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:04PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We'll be adding a second SGMII PHY on the same MDIO bus, so let's index
> the first one for better readability.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index 1c471278d441..55feaac7fa1b 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -263,7 +263,7 @@ vreg_l8e: ldo8 {
>  
>  &ethernet0 {
>  	phy-mode = "sgmii";
> -	phy-handle = <&sgmii_phy>;
> +	phy-handle = <&sgmii_phy0>;
>  
>  	pinctrl-0 = <&ethernet0_default>;
>  	pinctrl-names = "default";
> @@ -279,7 +279,7 @@ mdio {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> -		sgmii_phy: phy@8 {
> +		sgmii_phy0: phy@8 {
>  			compatible = "ethernet-phy-id0141.0dd4";
>  			reg = <0x8>;
>  			device_type = "ethernet-phy";
> -- 
> 2.39.2
> 

Re: [PATCH 7/9] arm64: dts: qcom: sa8775p-ride: add the second SGMII PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:05PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a second SGMII PHY that will be used by EMAC1 on sa8775p-ride.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index 55feaac7fa1b..5b48066f312a 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -286,6 +286,14 @@ sgmii_phy0: phy@8 {
>  			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
>  			reset-deassert-us = <70000>;
>  		};
> +
> +		sgmii_phy1: phy@a {
> +			compatible = "ethernet-phy-id0141.0dd4";
> +			reg = <0xa>;
> +			device_type = "ethernet-phy";
> +			reset-gpios = <&pmm8654au_2_gpios 9 GPIO_ACTIVE_LOW>;
> +			reset-deassert-us = <70000>;
> +		};

This is connected to the (another) Marvell 88EA1512. I mentioned in the
earlier patch for sgmii_phy0 that you dropped the reset-assert-us.

Unless there was a reason for that, I suspect you want to add it here
too.

Otherwise the description matches the bit of schematic I have access to.

Thanks,
Andrew

Re: [PATCH 1/9] arm64: dts: qcom: sa8775p: add a node for the second serdes PHY

[Andrew Lunn]

> That wording of "match downstream sources" is amiguous, sorry.
> 
> What I meant was that:
> 
>     1. This looks like a properly formatted dtsi node, follows
>        conventions in the file for ordering, etc
>     2. The downstream devicetree from Qualcomm uses the same MMIO region
>        same dependencies for clocks, etc. I do not have documentation
>        to further review past that

O.K. This does make your reviews worthwhile.

Vendor crap gets that name for a reason. So just saying it is the same
as the vendor code is not really helpful. So i would avoid this
ambiguous statement. And your later comment on a patch which points
out real problems adds to my confidence you did a real review.

Thanks
	  Andrew

Re: [PATCH 9/9] arm64: dts: qcom: sa8775p-ride: enable EMAC1

[Andrew Halaney]

On Mon, Aug 07, 2023 at 09:35:07PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Enable the second MAC on sa8775p-ride.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 74 +++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index af50aa2d9b10..0862bfb4c580 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -356,6 +356,80 @@ queue3 {
>  	};
>  };
>  
> +&ethernet1 {
> +	phy-mode = "sgmii";
> +	phy-handle = <&sgmii_phy1>;
> +
> +	pinctrl-0 = <&ethernet1_default>;
> +	pinctrl-names = "default";

As I stated in the earlier patch that added ethernet1_default, I don't
think it makes sense. All the MDIO is happening via the pins described
via ethernet0_default.

> +
> +	snps,mtl-rx-config = <&mtl_rx_setup1>;
> +	snps,mtl-tx-config = <&mtl_tx_setup1>;
> +	snps,ps-speed = <1000>;
> +	snps,shared-mdio = <&mdio0>;

same question as Andrew Lunn, but I'll let you respond to one of his
threads.

> +
> +	status = "okay";
> +
> +	mtl_rx_setup1: rx-queues-config {
> +		snps,rx-queues-to-use = <4>;
> +		snps,rx-sched-sp;
> +
> +		queue0 {
> +			snps,dcb-algorithm;
> +			snps,map-to-dma-channel = <0x0>;
> +			snps,route-up;
> +			snps,priority = <0x1>;
> +		};
> +
> +		queue1 {
> +			snps,dcb-algorithm;
> +			snps,map-to-dma-channel = <0x1>;
> +			snps,route-ptp;
> +		};
> +
> +		queue2 {
> +			snps,avb-algorithm;
> +			snps,map-to-dma-channel = <0x2>;
> +			snps,route-avcp;
> +		};
> +
> +		queue3 {
> +			snps,avb-algorithm;
> +			snps,map-to-dma-channel = <0x3>;
> +			snps,priority = <0xc>;
> +		};
> +	};
> +
> +	mtl_tx_setup1: tx-queues-config {
> +		snps,tx-queues-to-use = <4>;
> +		snps,tx-sched-sp;
> +
> +		queue0 {
> +			snps,dcb-algorithm;
> +		};
> +
> +		queue1 {
> +			snps,dcb-algorithm;
> +		};
> +
> +		queue2 {
> +			snps,avb-algorithm;
> +			snps,send_slope = <0x1000>;
> +			snps,idle_slope = <0x1000>;
> +			snps,high_credit = <0x3e800>;
> +			snps,low_credit = <0xffc18000>;
> +		};
> +
> +		queue3 {
> +			snps,avb-algorithm;
> +			snps,send_slope = <0x1000>;
> +			snps,idle_slope = <0x1000>;
> +			snps,high_credit = <0x3e800>;
> +			snps,low_credit = <0xffc18000>;
> +		};
> +	};
> +};
> +
>  &i2c11 {
>  	clock-frequency = <400000>;
>  	pinctrl-0 = <&qup_i2c11_default>;
> -- 
> 2.39.2
> 

Re: [PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Andrew Lunn]

> > I have proposed a solution for this problem in 2020 but it never got
> > upstream. Now we have a workaround in place which allows us to hard-code
> > the PHY id in the compatible property, thus skipping the ID scanning).
> 
> nitpicky, but I think that already existed at that time :D

Yes, it has been there are long long time. It is however only in the
last 5 years of so has it been seen as a solution to the chicken egg
problem.

> >  		sgmii_phy: phy@8 {
> > +			compatible = "ethernet-phy-id0141.0dd4";
> >  			reg = <0x8>;
> >  			device_type = "ethernet-phy";
> > +			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> > +			reset-deassert-us = <70000>;
> 
> Doesn't this need reset-assert-us?

If i remember correctly, there is a default value if DT does not
provide one.

	Andrew

Re: [PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Andrew Halaney]

On Mon, Aug 07, 2023 at 11:51:40PM +0200, Andrew Lunn wrote:
> > > I have proposed a solution for this problem in 2020 but it never got
> > > upstream. Now we have a workaround in place which allows us to hard-code
> > > the PHY id in the compatible property, thus skipping the ID scanning).
> > 
> > nitpicky, but I think that already existed at that time :D
> 
> Yes, it has been there are long long time. It is however only in the
> last 5 years of so has it been seen as a solution to the chicken egg
> problem.
> 
> > >  		sgmii_phy: phy@8 {
> > > +			compatible = "ethernet-phy-id0141.0dd4";
> > >  			reg = <0x8>;
> > >  			device_type = "ethernet-phy";
> > > +			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> > > +			reset-deassert-us = <70000>;
> > 
> > Doesn't this need reset-assert-us?
> 
> If i remember correctly, there is a default value if DT does not
> provide one.
> 

I've been trying to make sure I view devicetree properties as an OS
agnostic ABI lately, with that in mind...

The dt-binding says this for ethernet-phy:

  reset-assert-us:
    description:
      Delay after the reset was asserted in microseconds. If this
      property is missing the delay will be skipped.

If the hardware needs a delay I think we should encode it based on that
description, else we risk it starting to look like a unit impulse!

Re: [PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Bartosz Golaszewski]

On Tue, Aug 8, 2023 at 12:27 AM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Mon, Aug 07, 2023 at 11:51:40PM +0200, Andrew Lunn wrote:
> > > > I have proposed a solution for this problem in 2020 but it never got
> > > > upstream. Now we have a workaround in place which allows us to hard-code
> > > > the PHY id in the compatible property, thus skipping the ID scanning).
> > >
> > > nitpicky, but I think that already existed at that time :D
> >
> > Yes, it has been there are long long time. It is however only in the
> > last 5 years of so has it been seen as a solution to the chicken egg
> > problem.
> >
> > > >           sgmii_phy: phy@8 {
> > > > +                 compatible = "ethernet-phy-id0141.0dd4";
> > > >                   reg = <0x8>;
> > > >                   device_type = "ethernet-phy";
> > > > +                 reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> > > > +                 reset-deassert-us = <70000>;
> > >
> > > Doesn't this need reset-assert-us?
> >
> > If i remember correctly, there is a default value if DT does not
> > provide one.
> >
>
> I've been trying to make sure I view devicetree properties as an OS
> agnostic ABI lately, with that in mind...
>
> The dt-binding says this for ethernet-phy:
>
>   reset-assert-us:
>     description:
>       Delay after the reset was asserted in microseconds. If this
>       property is missing the delay will be skipped.
>
> If the hardware needs a delay I think we should encode it based on that
> description, else we risk it starting to look like a unit impulse!
>

Please note that the mdio-level delay properties are not the same as
the ones on the PHY levels.

reset-delay-us - this is the delay BEFORE *DEASSERTING* the reset line
reset-post-delay-us - this is the delay AFTER *DEASSERTING* the reset line

On PHY level we have:

reset-assert-us - AFTER *ASSERTING*
reset-deassert-us - AFTER *DEASSERTING*

There never has been any reset-assert delay on that line before. It
doesn't look like we need a delay BEFORE deasserting the line, do we?

Bart

Re: [PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Andrew Lunn]

> I've been trying to make sure I view devicetree properties as an OS
> agnostic ABI lately, with that in mind...
> 
> The dt-binding says this for ethernet-phy:
> 
>   reset-assert-us:
>     description:
>       Delay after the reset was asserted in microseconds. If this
>       property is missing the delay will be skipped.
> 
> If the hardware needs a delay I think we should encode it based on that
> description, else we risk it starting to look like a unit impulse!
 
I checked, and the documentation does appear correct, there is no
default value. So yes, it does seem prudent to specify a value,
otherwise it could be a short pulse.

	  Andrew

Re: [PATCH 5/9] arm64: dts: qcom: sa8775p-ride: move the reset-gpios property of the PHY

[Andrew Halaney]

On Tue, Aug 08, 2023 at 02:16:50PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 8, 2023 at 12:27 AM Andrew Halaney <ahalaney@redhat.com> wrote:
> >
> > On Mon, Aug 07, 2023 at 11:51:40PM +0200, Andrew Lunn wrote:
> > > > > I have proposed a solution for this problem in 2020 but it never got
> > > > > upstream. Now we have a workaround in place which allows us to hard-code
> > > > > the PHY id in the compatible property, thus skipping the ID scanning).
> > > >
> > > > nitpicky, but I think that already existed at that time :D
> > >
> > > Yes, it has been there are long long time. It is however only in the
> > > last 5 years of so has it been seen as a solution to the chicken egg
> > > problem.
> > >
> > > > >           sgmii_phy: phy@8 {
> > > > > +                 compatible = "ethernet-phy-id0141.0dd4";
> > > > >                   reg = <0x8>;
> > > > >                   device_type = "ethernet-phy";
> > > > > +                 reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> > > > > +                 reset-deassert-us = <70000>;
> > > >
> > > > Doesn't this need reset-assert-us?
> > >
> > > If i remember correctly, there is a default value if DT does not
> > > provide one.
> > >
> >
> > I've been trying to make sure I view devicetree properties as an OS
> > agnostic ABI lately, with that in mind...
> >
> > The dt-binding says this for ethernet-phy:
> >
> >   reset-assert-us:
> >     description:
> >       Delay after the reset was asserted in microseconds. If this
> >       property is missing the delay will be skipped.
> >
> > If the hardware needs a delay I think we should encode it based on that
> > description, else we risk it starting to look like a unit impulse!
> >
> 
> Please note that the mdio-level delay properties are not the same as
> the ones on the PHY levels.
> 
> reset-delay-us - this is the delay BEFORE *DEASSERTING* the reset line
> reset-post-delay-us - this is the delay AFTER *DEASSERTING* the reset line
> 
> On PHY level we have:
> 
> reset-assert-us - AFTER *ASSERTING*
> reset-deassert-us - AFTER *DEASSERTING*
> 
> There never has been any reset-assert delay on that line before. It
> doesn't look like we need a delay BEFORE deasserting the line, do we?
> 

The MDIO reset-delay-us happens "before deassert"/"after assert", so to
make things a proper move here I think it needs a resrt-assert, otherwise
behavior with respect to reset timing is definitely changed from this
patch!

Here's a trimmed version of the reset handling in mdio_bus.c to back
that up:

	/* assert bus level PHY GPIO reset */
	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_HIGH);
    ...
	} else	if (gpiod) {
		bus->reset_gpiod = gpiod;
		fsleep(bus->reset_delay_us);
		gpiod_set_value_cansleep(gpiod, 0);
		if (bus->reset_post_delay_us > 0)
			fsleep(bus->reset_post_delay_us);
	}

so its assert reset, sleep reset_delay_us, deassert, sleep
reset_post_delay_us for the MDIO case.

Thanks,
Andrew