| Message ID | 20230807193507.6488-6-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: enable EMAC1 on sa8775p | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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?
> > 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 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 <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
> 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 <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
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>; }; };