| Message ID | 20210409100022.1.I93dea8b89ea632e7d8f2640a6eca6f6e69fed319@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
| Series | net: dwc_eth_qos: add support of device tree configuration for reset delay | expand |
On 4/9/21 10:00 AM, Patrick Delaunay wrote: > The gpio reset assert/deassert delay are today harcoded in U-Boot driver > but the value should be read from DT. > > STM32 use the generic binding defined in linux: > Documentation/devicetree/bindings/net/ethernet-phy.yaml > > reset-gpios: > maxItems: 1 > description: > The GPIO phandle and specifier for the PHY reset signal. > > reset-assert-us: > description: > Delay after the reset was asserted in microseconds. If this > property is missing the delay will be skipped. > > reset-deassert-us: > description: > Delay after the reset was deasserted in microseconds. If > this property is missing the delay will be skipped. > > See also U-Boot: doc/device-tree-bindings/net/phy.txt Since this is a PHY property, shouldn't that be handled in drivers/net/phy/ instead of MAC driver ?
Hi, On 4/9/21 2:22 PM, Marek Vasut wrote: > On 4/9/21 10:00 AM, Patrick Delaunay wrote: >> The gpio reset assert/deassert delay are today harcoded in U-Boot driver >> but the value should be read from DT. >> >> STM32 use the generic binding defined in linux: >> Documentation/devicetree/bindings/net/ethernet-phy.yaml >> >> reset-gpios: >> maxItems: 1 >> description: >> The GPIO phandle and specifier for the PHY reset signal. >> >> reset-assert-us: >> description: >> Delay after the reset was asserted in microseconds. If this >> property is missing the delay will be skipped. >> >> reset-deassert-us: >> description: >> Delay after the reset was deasserted in microseconds. If >> this property is missing the delay will be skipped. >> >> See also U-Boot: doc/device-tree-bindings/net/phy.txt > > Since this is a PHY property, shouldn't that be handled in > drivers/net/phy/ instead of MAC driver ? I was my first idea but I don't found found the correct location in phy (driver or uclass) to manage these generic property and the generic property "reset-gpios" was already managed in eth driver, so I continue to patch the driver. But I come back to this idea after your remark.... => in linux these property are managed in drivers/net/mdio/of_mdio.c::of_mdiobus_phy_device_register parse DT node and add info in mdio drivers/net/phy/mdio_device.c::mdio_device_reset called in mdio_probe / mdio_remove In my first search, I don't found the same level in the U-Boot drivers in drivers/net/phy/ but I miss the uclass defined in drivers/net/eth-phy-uclass.c Finally I think I need to manage the generic binding property (reset-gpios, reset-assert-us, reset-deassert-us) directly in => drivers/net/mdio-uclass The GPIO RESET will be managed in mdio ops: pre_probe/ post_remove as it is done in linux warning: today post_remove ops don't exit in u-class. Do you think it is the correct location ? Do you think it should be a new serie (migrate the eqos property in mdio) after this eqos is accepted or I shoudl sent a new serie to replace this serie. Regards Patrick
On 4/14/21 4:07 PM, Patrick DELAUNAY wrote: > Hi, Hi, > On 4/9/21 2:22 PM, Marek Vasut wrote: >> On 4/9/21 10:00 AM, Patrick Delaunay wrote: >>> The gpio reset assert/deassert delay are today harcoded in U-Boot driver >>> but the value should be read from DT. >>> >>> STM32 use the generic binding defined in linux: >>> Documentation/devicetree/bindings/net/ethernet-phy.yaml >>> >>> reset-gpios: >>> maxItems: 1 >>> description: >>> The GPIO phandle and specifier for the PHY reset signal. >>> >>> reset-assert-us: >>> description: >>> Delay after the reset was asserted in microseconds. If this >>> property is missing the delay will be skipped. >>> >>> reset-deassert-us: >>> description: >>> Delay after the reset was deasserted in microseconds. If >>> this property is missing the delay will be skipped. >>> >>> See also U-Boot: doc/device-tree-bindings/net/phy.txt >> >> Since this is a PHY property, shouldn't that be handled in >> drivers/net/phy/ instead of MAC driver ? > > > I was my first idea but I don't found found the correct location in phy > (driver or uclass) > > to manage these generic property and the generic property "reset-gpios" > was already > > managed in eth driver, so I continue to patch the driver. > > > But I come back to this idea after your remark.... > > => in linux these property are managed in > > drivers/net/mdio/of_mdio.c::of_mdiobus_phy_device_register > > parse DT node and add info in mdio > > drivers/net/phy/mdio_device.c::mdio_device_reset > > called in mdio_probe / mdio_remove > > > In my first search, I don't found the same level in the U-Boot drivers > in drivers/net/phy/ Note that this is MDIO-wide PHY reset (e.g. you can have single reset line connected to multiple PHYs on single MDIO bus), this is not PHY-specific reset. > but I miss the uclass defined in drivers/net/eth-phy-uclass.c > > > Finally I think I need to manage the generic binding property > > (reset-gpios, reset-assert-us, reset-deassert-us) directly in > > => drivers/net/mdio-uclass > > > The GPIO RESET will be managed in mdio ops: pre_probe/ post_remove > > as it is done in linux > > warning: today post_remove ops don't exit in u-class. > > > Do you think it is the correct location ? For single-PHY reset, the correct location is in drivers/net/phy/ somewhere. > Do you think it should be a new serie (migrate the eqos property in mdio) > > after this eqos is accepted > > or I shoudl sent a new serie to replace this serie. I'll leave that decision to Ramon/Joe.
On Wed, Apr 14, 2021 at 5:36 PM Marek Vasut <marex@denx.de> wrote: > > On 4/14/21 4:07 PM, Patrick DELAUNAY wrote: > > Hi, > > Hi, > > > On 4/9/21 2:22 PM, Marek Vasut wrote: > >> On 4/9/21 10:00 AM, Patrick Delaunay wrote: > >>> The gpio reset assert/deassert delay are today harcoded in U-Boot driver > >>> but the value should be read from DT. > >>> > >>> STM32 use the generic binding defined in linux: > >>> Documentation/devicetree/bindings/net/ethernet-phy.yaml > >>> > >>> reset-gpios: > >>> maxItems: 1 > >>> description: > >>> The GPIO phandle and specifier for the PHY reset signal. > >>> > >>> reset-assert-us: > >>> description: > >>> Delay after the reset was asserted in microseconds. If this > >>> property is missing the delay will be skipped. > >>> > >>> reset-deassert-us: > >>> description: > >>> Delay after the reset was deasserted in microseconds. If > >>> this property is missing the delay will be skipped. > >>> > >>> See also U-Boot: doc/device-tree-bindings/net/phy.txt > >> > >> Since this is a PHY property, shouldn't that be handled in > >> drivers/net/phy/ instead of MAC driver ? > > > > > > I was my first idea but I don't found found the correct location in phy > > (driver or uclass) > > > > to manage these generic property and the generic property "reset-gpios" > > was already > > > > managed in eth driver, so I continue to patch the driver. > > > > > > But I come back to this idea after your remark.... > > > > => in linux these property are managed in > > > > drivers/net/mdio/of_mdio.c::of_mdiobus_phy_device_register > > > > parse DT node and add info in mdio > > > > drivers/net/phy/mdio_device.c::mdio_device_reset > > > > called in mdio_probe / mdio_remove > > > > > > In my first search, I don't found the same level in the U-Boot drivers > > in drivers/net/phy/ > > Note that this is MDIO-wide PHY reset (e.g. you can have single reset > line connected to multiple PHYs on single MDIO bus), this is not > PHY-specific reset. > > > but I miss the uclass defined in drivers/net/eth-phy-uclass.c > > > > > > Finally I think I need to manage the generic binding property > > > > (reset-gpios, reset-assert-us, reset-deassert-us) directly in > > > > => drivers/net/mdio-uclass > > > > > > The GPIO RESET will be managed in mdio ops: pre_probe/ post_remove > > > > as it is done in linux > > > > warning: today post_remove ops don't exit in u-class. > > > > > > Do you think it is the correct location ? > > For single-PHY reset, the correct location is in drivers/net/phy/ somewhere. > > > Do you think it should be a new serie (migrate the eqos property in mdio) > > > > after this eqos is accepted > > > > or I shoudl sent a new serie to replace this serie. > > I'll leave that decision to Ramon/Joe. Joe, I'll leave this to you.
On Thu, Apr 15, 2021 at 4:41 AM Ramon Fried <rfried.dev@gmail.com> wrote: > > On Wed, Apr 14, 2021 at 5:36 PM Marek Vasut <marex@denx.de> wrote: > > > > On 4/14/21 4:07 PM, Patrick DELAUNAY wrote: > > > Hi, > > > > Hi, > > > > > On 4/9/21 2:22 PM, Marek Vasut wrote: > > >> On 4/9/21 10:00 AM, Patrick Delaunay wrote: > > >>> The gpio reset assert/deassert delay are today harcoded in U-Boot driver > > >>> but the value should be read from DT. > > >>> > > >>> STM32 use the generic binding defined in linux: > > >>> Documentation/devicetree/bindings/net/ethernet-phy.yaml > > >>> > > >>> reset-gpios: > > >>> maxItems: 1 > > >>> description: > > >>> The GPIO phandle and specifier for the PHY reset signal. > > >>> > > >>> reset-assert-us: > > >>> description: > > >>> Delay after the reset was asserted in microseconds. If this > > >>> property is missing the delay will be skipped. > > >>> > > >>> reset-deassert-us: > > >>> description: > > >>> Delay after the reset was deasserted in microseconds. If > > >>> this property is missing the delay will be skipped. > > >>> > > >>> See also U-Boot: doc/device-tree-bindings/net/phy.txt > > >> > > >> Since this is a PHY property, shouldn't that be handled in > > >> drivers/net/phy/ instead of MAC driver ? > > > > > > > > > I was my first idea but I don't found found the correct location in phy > > > (driver or uclass) > > > > > > to manage these generic property and the generic property "reset-gpios" > > > was already > > > > > > managed in eth driver, so I continue to patch the driver. > > > > > > > > > But I come back to this idea after your remark.... > > > > > > => in linux these property are managed in > > > > > > drivers/net/mdio/of_mdio.c::of_mdiobus_phy_device_register > > > > > > parse DT node and add info in mdio > > > > > > drivers/net/phy/mdio_device.c::mdio_device_reset > > > > > > called in mdio_probe / mdio_remove > > > > > > > > > In my first search, I don't found the same level in the U-Boot drivers > > > in drivers/net/phy/ > > > > Note that this is MDIO-wide PHY reset (e.g. you can have single reset > > line connected to multiple PHYs on single MDIO bus), this is not > > PHY-specific reset. > > > > > but I miss the uclass defined in drivers/net/eth-phy-uclass.c > > > > > > > > > Finally I think I need to manage the generic binding property > > > > > > (reset-gpios, reset-assert-us, reset-deassert-us) directly in > > > > > > => drivers/net/mdio-uclass > > > > > > > > > The GPIO RESET will be managed in mdio ops: pre_probe/ post_remove > > > > > > as it is done in linux > > > > > > warning: today post_remove ops don't exit in u-class. > > > > > > > > > Do you think it is the correct location ? > > > > For single-PHY reset, the correct location is in drivers/net/phy/ somewhere. > > > > > Do you think it should be a new serie (migrate the eqos property in mdio) > > > > > > after this eqos is accepted > > > > > > or I shoudl sent a new serie to replace this serie. > > > > I'll leave that decision to Ramon/Joe. > Joe, I'll leave this to you. You know what, let's go with the new series, please migrate it to the net/phy. Thanks, Ramon
Hi, On 4/15/21 8:08 AM, Ramon Fried wrote: > On Thu, Apr 15, 2021 at 4:41 AM Ramon Fried <rfried.dev@gmail.com> wrote: >> On Wed, Apr 14, 2021 at 5:36 PM Marek Vasut <marex@denx.de> wrote: >>> On 4/14/21 4:07 PM, Patrick DELAUNAY wrote: >>>> Hi, >>> Hi, >>> >>>> On 4/9/21 2:22 PM, Marek Vasut wrote: >>>>> On 4/9/21 10:00 AM, Patrick Delaunay wrote: >>>>>> The gpio reset assert/deassert delay are today harcoded in U-Boot driver >>>>>> but the value should be read from DT. >>>>>> >>>>>> STM32 use the generic binding defined in linux: >>>>>> Documentation/devicetree/bindings/net/ethernet-phy.yaml >>>>>> >>>>>> reset-gpios: >>>>>> maxItems: 1 >>>>>> description: >>>>>> The GPIO phandle and specifier for the PHY reset signal. >>>>>> >>>>>> reset-assert-us: >>>>>> description: >>>>>> Delay after the reset was asserted in microseconds. If this >>>>>> property is missing the delay will be skipped. >>>>>> >>>>>> reset-deassert-us: >>>>>> description: >>>>>> Delay after the reset was deasserted in microseconds. If >>>>>> this property is missing the delay will be skipped. >>>>>> >>>>>> See also U-Boot: doc/device-tree-bindings/net/phy.txt >>>>> Since this is a PHY property, shouldn't that be handled in >>>>> drivers/net/phy/ instead of MAC driver ? >>>> >>>> I was my first idea but I don't found found the correct location in phy >>>> (driver or uclass) >>>> >>>> to manage these generic property and the generic property "reset-gpios" >>>> was already >>>> >>>> managed in eth driver, so I continue to patch the driver. >>>> >>>> >>>> But I come back to this idea after your remark.... >>>> >>>> => in linux these property are managed in >>>> >>>> drivers/net/mdio/of_mdio.c::of_mdiobus_phy_device_register >>>> >>>> parse DT node and add info in mdio >>>> >>>> drivers/net/phy/mdio_device.c::mdio_device_reset >>>> >>>> called in mdio_probe / mdio_remove >>>> >>>> >>>> In my first search, I don't found the same level in the U-Boot drivers >>>> in drivers/net/phy/ >>> Note that this is MDIO-wide PHY reset (e.g. you can have single reset >>> line connected to multiple PHYs on single MDIO bus), this is not >>> PHY-specific reset. >>> >>>> but I miss the uclass defined in drivers/net/eth-phy-uclass.c >>>> >>>> >>>> Finally I think I need to manage the generic binding property >>>> >>>> (reset-gpios, reset-assert-us, reset-deassert-us) directly in >>>> >>>> => drivers/net/mdio-uclass >>>> >>>> >>>> The GPIO RESET will be managed in mdio ops: pre_probe/ post_remove >>>> >>>> as it is done in linux >>>> >>>> warning: today post_remove ops don't exit in u-class. >>>> >>>> >>>> Do you think it is the correct location ? >>> For single-PHY reset, the correct location is in drivers/net/phy/ somewhere. >>> >>>> Do you think it should be a new serie (migrate the eqos property in mdio) >>>> >>>> after this eqos is accepted >>>> >>>> or I shoudl sent a new serie to replace this serie. >>> I'll leave that decision to Ramon/Joe. >> Joe, I'll leave this to you. > You know what, let's go with the new series, please migrate it to the net/phy. > Thanks, > Ramon I am preparing V2 with DT parsing in drivers/net/eth-phy-uclass.c with CONFIG_DM_ETH_PHY enabled for stm32 variant of dwc_eth_qos driver. Regards Patrick
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index e8242ca4e1..6ef750a348 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -293,6 +293,13 @@ struct eqos_ops { ulong (*eqos_get_tick_clk_rate)(struct udevice *dev); }; +/* extend the u-class platdata (eth_pdata) with EQOS driver platdata */ +struct eqos_pdata { + struct eth_pdata eth_pdata; + u32 reset_assert_us; + u32 reset_deassert_us; +}; + struct eqos_priv { struct udevice *dev; const struct eqos_config *config; @@ -662,6 +669,7 @@ static void eqos_stop_clks_imx(struct udevice *dev) static int eqos_start_resets_tegra186(struct udevice *dev) { + struct eqos_pdata *eqos_plat = dev_get_plat(dev); struct eqos_priv *eqos = dev_get_priv(dev); int ret; @@ -672,14 +680,14 @@ static int eqos_start_resets_tegra186(struct udevice *dev) pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", ret); return ret; } - - udelay(2); + udelay(eqos_plat->reset_assert_us); ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0); if (ret < 0) { pr_err("dm_gpio_set_value(phy_reset, deassert) failed: %d", ret); return ret; } + udelay(eqos_plat->reset_deassert_us); ret = reset_assert(&eqos->reset_ctl); if (ret < 0) { @@ -701,6 +709,7 @@ static int eqos_start_resets_tegra186(struct udevice *dev) static int eqos_start_resets_stm32(struct udevice *dev) { + struct eqos_pdata *eqos_plat = dev_get_plat(dev); struct eqos_priv *eqos = dev_get_priv(dev); int ret; @@ -712,8 +721,7 @@ static int eqos_start_resets_stm32(struct udevice *dev) ret); return ret; } - - udelay(2); + udelay(eqos_plat->reset_assert_us); ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0); if (ret < 0) { @@ -721,6 +729,7 @@ static int eqos_start_resets_stm32(struct udevice *dev) ret); return ret; } + udelay(eqos_plat->reset_deassert_us); } debug("%s: OK\n", __func__); @@ -1070,7 +1079,8 @@ static int eqos_adjust_link(struct udevice *dev) static int eqos_write_hwaddr(struct udevice *dev) { - struct eth_pdata *plat = dev_get_plat(dev); + struct eqos_pdata *eqos_plat = dev_get_plat(dev); + struct eth_pdata *plat = &eqos_plat->eth_pdata; struct eqos_priv *eqos = dev_get_priv(dev); uint32_t val; @@ -1114,7 +1124,8 @@ static int eqos_write_hwaddr(struct udevice *dev) static int eqos_read_rom_hwaddr(struct udevice *dev) { - struct eth_pdata *pdata = dev_get_plat(dev); + struct eqos_pdata *eqos_plat = dev_get_plat(dev); + struct eth_pdata *pdata = &eqos_plat->eth_pdata; #ifdef CONFIG_ARCH_IMX8M imx_get_mac_from_fuse(dev_seq(dev), pdata->enetaddr); @@ -1704,6 +1715,7 @@ static int eqos_remove_resources_core(struct udevice *dev) static int eqos_probe_resources_tegra186(struct udevice *dev) { + struct eqos_pdata *eqos_plat = dev_get_plat(dev); struct eqos_priv *eqos = dev_get_priv(dev); int ret; @@ -1722,6 +1734,8 @@ static int eqos_probe_resources_tegra186(struct udevice *dev) pr_err("gpio_request_by_name(phy reset) failed: %d", ret); goto err_free_reset_eqos; } + eqos_plat->reset_assert_us = 2; + eqos_plat->reset_deassert_us = 1; ret = clk_get_by_name(dev, "slave_bus", &eqos->clk_slave_bus); if (ret) { @@ -1783,6 +1797,7 @@ __weak int board_interface_eth_init(struct udevice *dev, static int eqos_probe_resources_stm32(struct udevice *dev) { + struct eqos_pdata *eqos_plat = dev_get_plat(dev); struct eqos_priv *eqos = dev_get_priv(dev); int ret; phy_interface_t interface; @@ -1839,6 +1854,10 @@ static int eqos_probe_resources_stm32(struct udevice *dev) if (ret) pr_warn("gpio_request_by_name(phy reset) not provided %d", ret); + eqos_plat->reset_assert_us = ofnode_read_u32_default(phandle_args.node, + "reset-assert-us", 2); + eqos_plat->reset_deassert_us = ofnode_read_u32_default(phandle_args.node, + "reset-deassert-us", 1); eqos->phyaddr = ofnode_read_u32_default(phandle_args.node, "reg", -1); @@ -2167,5 +2186,5 @@ U_BOOT_DRIVER(eth_eqos) = { .remove = eqos_remove, .ops = &eqos_ops, .priv_auto = sizeof(struct eqos_priv), - .plat_auto = sizeof(struct eth_pdata), + .plat_auto = sizeof(struct eqos_pdata), };
The gpio reset assert/deassert delay are today harcoded in U-Boot driver but the value should be read from DT. STM32 use the generic binding defined in linux: Documentation/devicetree/bindings/net/ethernet-phy.yaml reset-gpios: maxItems: 1 description: The GPIO phandle and specifier for the PHY reset signal. reset-assert-us: description: Delay after the reset was asserted in microseconds. If this property is missing the delay will be skipped. reset-deassert-us: description: Delay after the reset was deasserted in microseconds. If this property is missing the delay will be skipped. See also U-Boot: doc/device-tree-bindings/net/phy.txt This patch moves these 2 delay configuration in the driver platdata and manages this ethernet phy binding for STM32 variant; when the properties are absent, the driver use the existing value = 2 and the minimal supported udelay = 1 to avoid issue with udelay(0) when the the property is absent. This patch also updates the tegra186 part to use the modified platdata, even if the GPIO reset delay value is hardcoded in probe function. Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> --- drivers/net/dwc_eth_qos.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)