From: Bartosz Golaszewski <[email protected]>
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
From: Bartosz Golaszewski <[email protected]>
Enable the second SerDes PHY on sa8775p-ride development board.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
Add a second SGMII PHY that will be used by EMAC1 on sa8775p-ride.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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 @@ ðernet0 {
status = "okay";
- mdio {
+ mdio0: mdio {
compatible = "snps,dwmac-mdio";
#address-cells = <1>;
#size-cells = <0>;
--
2.39.2
From: Bartosz Golaszewski <[email protected]>
Add a node for the second MAC on sa8775p platforms.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
FWIW this seems to match downstream sources.
Reviewed-by: Andrew Halaney <[email protected]>
> ---
> 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
>
On Mon, Aug 07, 2023 at 09:35:00PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a node for the second MAC on sa8775p platforms.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
> ---
> 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
>
From: Bartosz Golaszewski <[email protected]>
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/[email protected]/
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
On Mon, Aug 07, 2023 at 09:35:03PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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/[email protected]/
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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?
> > 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
On Mon, Aug 07, 2023 at 09:35:01PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Enable the second SerDes PHY on sa8775p-ride development board.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Matches what I see downstream wrt the supply, so:
Reviewed-by: Andrew Halaney <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > >
> > > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[email protected]>
> >
> > FWIW this seems to match downstream sources.
> >
> > Reviewed-by: Andrew Halaney <[email protected]>
>
> 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
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 <[email protected]>
> >
> > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> FWIW this seems to match downstream sources.
>
> Reviewed-by: Andrew Halaney <[email protected]>
Why does matching downstream make it correct and deserve a
Reviewed-by?
Did you actually review the change?
Andrew
On Mon, Aug 07, 2023 at 09:35:05PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a second SGMII PHY that will be used by EMAC1 on sa8775p-ride.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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
> 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
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!
On Tue, Aug 8, 2023 at 12:27 AM Andrew Halaney <[email protected]> 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
> 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
On Tue, Aug 08, 2023 at 02:16:50PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 8, 2023 at 12:27 AM Andrew Halaney <[email protected]> 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