| Message ID | 20200512095603.29126-3-david.wu@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Joe Hershberger |
| Headers | show |
| Series | Add dwc_eth_qos support for rockchip | expand |
Hi David On 5/12/20 11:56 AM, David Wu wrote: > It can be seen that most of the Socs using STM mac, "snps,reset-gpio" > gpio is used, adding this option makes reset function more general. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > > Changes in v2: > - Remove the code is not related (Patrice) > > drivers/net/dwc_eth_qos.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > index 66a02aa80b..92dab678c7 100644 > --- a/drivers/net/dwc_eth_qos.c > +++ b/drivers/net/dwc_eth_qos.c > @@ -314,6 +314,7 @@ struct eqos_priv { > struct eqos_tegra186_regs *tegra186_regs; > struct reset_ctl reset_ctl; > struct gpio_desc phy_reset_gpio; > + u32 reset_delays[3]; > struct clk clk_master_bus; > struct clk clk_rx; > struct clk clk_ptp_ref; > @@ -739,6 +740,15 @@ static int eqos_start_resets_stm32(struct udevice *dev) > > debug("%s(dev=%p):\n", __func__, dev); > if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) { > + 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->reset_delays[0]); > + > ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1); > if (ret < 0) { > pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", > @@ -746,7 +756,7 @@ static int eqos_start_resets_stm32(struct udevice *dev) > return ret; > } > > - udelay(2); > + udelay(eqos->reset_delays[1]); > > ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0); > if (ret < 0) { > @@ -754,6 +764,8 @@ static int eqos_start_resets_stm32(struct udevice *dev) > ret); > return ret; > } > + > + udelay(eqos->reset_delays[2]); > } > debug("%s: OK\n", __func__); > > @@ -1864,11 +1876,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev) > if (ret) > pr_warn("gpio_request_by_name(phy reset) not provided %d", > ret); > + else > + eqos->reset_delays[1] = 2; > > eqos->phyaddr = ofnode_read_u32_default(phandle_args.node, > "reg", -1); > } > > + if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) { > + int reset_flags = GPIOD_IS_OUT; > + > + if (dev_read_bool(dev, "snps,reset-active-low")) > + reset_flags |= GPIOD_ACTIVE_LOW; > + > + ret = gpio_request_by_name(dev, "snps,reset-gpio", 0, > + &eqos->phy_reset_gpio, reset_flags); > + if (ret == 0) > + ret = dev_read_u32_array(dev, "snps,reset-delays-us", > + eqos->reset_delays, 3); > + else > + pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d", > + ret); > + } > + > debug("%s: OK\n", __func__); > return 0; > Reviewed-by: Patrice Chotard <patrice.chotard@st.com> Thanks
Hi David > From: David Wu <david.wu@rock-chips.com> > Sent: mardi 12 mai 2020 11:56 > > It can be seen that most of the Socs using STM mac, "snps,reset-gpio" > gpio is used, adding this option makes reset function more general. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > > Changes in v2: > - Remove the code is not related (Patrice) > > drivers/net/dwc_eth_qos.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index > 66a02aa80b..92dab678c7 100644 > --- a/drivers/net/dwc_eth_qos.c > +++ b/drivers/net/dwc_eth_qos.c > @@ -314,6 +314,7 @@ struct eqos_priv { > struct eqos_tegra186_regs *tegra186_regs; > struct reset_ctl reset_ctl; > struct gpio_desc phy_reset_gpio; > + u32 reset_delays[3]; > struct clk clk_master_bus; > struct clk clk_rx; > struct clk clk_ptp_ref; > @@ -739,6 +740,15 @@ static int eqos_start_resets_stm32(struct udevice *dev) > > debug("%s(dev=%p):\n", __func__, dev); > if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) { > + 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->reset_delays[0]); > + > ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1); > if (ret < 0) { > pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", > @@ -746,7 +756,7 @@ static int eqos_start_resets_stm32(struct udevice *dev) > return ret; > } > > - udelay(2); > + udelay(eqos->reset_delays[1]); > > ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0); > if (ret < 0) { > @@ -754,6 +764,8 @@ static int eqos_start_resets_stm32(struct udevice *dev) > ret); > return ret; > } > + > + udelay(eqos->reset_delays[2]); > } > debug("%s: OK\n", __func__); > > @@ -1864,11 +1876,29 @@ static int eqos_probe_resources_stm32(struct > udevice *dev) > if (ret) > pr_warn("gpio_request_by_name(phy reset) not provided > %d", > ret); > + else > + eqos->reset_delays[1] = 2; > > eqos->phyaddr = ofnode_read_u32_default(phandle_args.node, > "reg", -1); > } > > + if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) { > + int reset_flags = GPIOD_IS_OUT; > + > + if (dev_read_bool(dev, "snps,reset-active-low")) > + reset_flags |= GPIOD_ACTIVE_LOW; > + > + ret = gpio_request_by_name(dev, "snps,reset-gpio", 0, > + &eqos->phy_reset_gpio, reset_flags); > + if (ret == 0) > + ret = dev_read_u32_array(dev, "snps,reset-delays-us", > + eqos->reset_delays, 3); > + else > + pr_warn("gpio_request_by_name(snps,reset-gpio) failed: > %d", > + ret); > + } > + > debug("%s: OK\n", __func__); > return 0; > > -- > 2.19.1 > > This obsolete binding isn't expected to be supported in stm32 glue for dwmac (and it tis the purpose of eqos_stm32_config) Reference in linux binding ./Documentation/devicetree/bindings/net/stm32-dwmac.txt (the glue) ./Documentation/devicetree/bindings/net/snps,dwmac.yaml snps,reset-gpio: deprecated: true snps,reset-active-low: deprecated: true snps,reset-delays-us: deprecated: true I expected that gpio reset in future device tree should be managed by only by PHY generic binding (upstream in progress on Linux side for STM32MP15x), as described in: 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 alsoU-Boot: doc/device-tree-bindings/net/phy.txt Something as &mac { status = "okay"; pinctrl-0 = <ðernet_mii>; pinctrl-names = "default"; phy-mode = "mii"; phy-handle = <&phy1>; mdio0 { #address-cells = <1>; #size-cells = <0>; compatible = "snps,dwmac-mdio"; phy1: ethernet-phy@1 { reg = <1>; + reset-assert-us = <10000>; + reset-deassert-us = <30000>; + reset-gpios = <&gpioh 15 GPIO_ACTIVE_LOW>; }; }; }; And this binding "reset-gpios" is supported since the commit 5177b31ba6aa ("net: dwc_eth_qos: implement reset-gpios for stm32") "reset-assert-us" and "reset-deassert-us" can be added if needed. For witch product / compatible do you want add the support of this obsolete binding ? Regards Patrick
On Tue, May 12, 2020 at 05:56:01PM +0800, David Wu wrote: > It can be seen that most of the Socs using STM mac, "snps,reset-gpio" > gpio is used, adding this option makes reset function more general. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > Reviewed-by: Patrice Chotard <patrice.chotard@st.com> > --- > > Changes in v2: > - Remove the code is not related (Patrice) Please note that based on Patrick's feedback I am expecting a v3 of this patch or further answers about the obsolete binding being used. Thanks!
Hi Patrick, Yes, this is the case, it should be add at PHY node, and I also used the original writing "snps,reset*" at MAC node. Anyway, I will try to put the reset gpio in the PHY node. 在 2020/5/13 下午8:55, Patrick DELAUNAY 写道: > Hi David > >> From: David Wu <david.wu@rock-chips.com> >> Sent: mardi 12 mai 2020 11:56 >> >> It can be seen that most of the Socs using STM mac, "snps,reset-gpio" >> gpio is used, adding this option makes reset function more general. >> >> Signed-off-by: David Wu <david.wu@rock-chips.com> >> --- >> >> Changes in v2: >> - Remove the code is not related (Patrice) >> >> drivers/net/dwc_eth_qos.c | 32 +++++++++++++++++++++++++++++++- >> 1 file changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index >> 66a02aa80b..92dab678c7 100644 >> --- a/drivers/net/dwc_eth_qos.c >> +++ b/drivers/net/dwc_eth_qos.c >> @@ -314,6 +314,7 @@ struct eqos_priv { >> struct eqos_tegra186_regs *tegra186_regs; >> struct reset_ctl reset_ctl; >> struct gpio_desc phy_reset_gpio; >> + u32 reset_delays[3]; >> struct clk clk_master_bus; >> struct clk clk_rx; >> struct clk clk_ptp_ref; >> @@ -739,6 +740,15 @@ static int eqos_start_resets_stm32(struct udevice *dev) >> >> debug("%s(dev=%p):\n", __func__, dev); >> if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) { >> + 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->reset_delays[0]); >> + >> ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1); >> if (ret < 0) { >> pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", >> @@ -746,7 +756,7 @@ static int eqos_start_resets_stm32(struct udevice *dev) >> return ret; >> } >> >> - udelay(2); >> + udelay(eqos->reset_delays[1]); >> >> ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0); >> if (ret < 0) { >> @@ -754,6 +764,8 @@ static int eqos_start_resets_stm32(struct udevice *dev) >> ret); >> return ret; >> } >> + >> + udelay(eqos->reset_delays[2]); >> } >> debug("%s: OK\n", __func__); >> >> @@ -1864,11 +1876,29 @@ static int eqos_probe_resources_stm32(struct >> udevice *dev) >> if (ret) >> pr_warn("gpio_request_by_name(phy reset) not provided >> %d", >> ret); >> + else >> + eqos->reset_delays[1] = 2; >> >> eqos->phyaddr = ofnode_read_u32_default(phandle_args.node, >> "reg", -1); >> } >> >> + if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) { >> + int reset_flags = GPIOD_IS_OUT; >> + >> + if (dev_read_bool(dev, "snps,reset-active-low")) >> + reset_flags |= GPIOD_ACTIVE_LOW; >> + >> + ret = gpio_request_by_name(dev, "snps,reset-gpio", 0, >> + &eqos->phy_reset_gpio, reset_flags); >> + if (ret == 0) >> + ret = dev_read_u32_array(dev, "snps,reset-delays-us", >> + eqos->reset_delays, 3); >> + else >> + pr_warn("gpio_request_by_name(snps,reset-gpio) failed: >> %d", >> + ret); >> + } >> + >> debug("%s: OK\n", __func__); >> return 0; >> >> -- >> 2.19.1 >> >> > > This obsolete binding isn't expected to be supported in stm32 glue for dwmac > (and it tis the purpose of eqos_stm32_config) > > Reference in linux binding > ./Documentation/devicetree/bindings/net/stm32-dwmac.txt (the glue) > ./Documentation/devicetree/bindings/net/snps,dwmac.yaml > > snps,reset-gpio: > deprecated: true > > snps,reset-active-low: > deprecated: true > > snps,reset-delays-us: > deprecated: true > > I expected that gpio reset in future device tree should be managed by only by PHY generic binding > (upstream in progress on Linux side for STM32MP15x), as described in: > > 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 alsoU-Boot: doc/device-tree-bindings/net/phy.txt > > Something as > > &mac { > status = "okay"; > pinctrl-0 = <ðernet_mii>; > pinctrl-names = "default"; > phy-mode = "mii"; > phy-handle = <&phy1>; > mdio0 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "snps,dwmac-mdio"; > phy1: ethernet-phy@1 { > reg = <1>; > + reset-assert-us = <10000>; > + reset-deassert-us = <30000>; > + reset-gpios = <&gpioh 15 GPIO_ACTIVE_LOW>; > }; > }; > }; > > And this binding "reset-gpios" is supported since the commit 5177b31ba6aa > ("net: dwc_eth_qos: implement reset-gpios for stm32") > > "reset-assert-us" and "reset-deassert-us" can be added if needed. > > For witch product / compatible do you want add the support of this obsolete binding ? > > Regards > > Patrick > > >
Hi Tom, 在 2020/6/12 下午10:48, Tom Rini 写道: > On Tue, May 12, 2020 at 05:56:01PM +0800, David Wu wrote: > >> It can be seen that most of the Socs using STM mac, "snps,reset-gpio" >> gpio is used, adding this option makes reset function more general. >> >> Signed-off-by: David Wu <david.wu@rock-chips.com> >> Reviewed-by: Patrice Chotard <patrice.chotard@st.com> >> --- >> >> Changes in v2: >> - Remove the code is not related (Patrice) > > Please note that based on Patrick's feedback I am expecting a v3 of this > patch or further answers about the obsolete binding being used. Thanks! > I think I will submit v3 after the following two patches, it looks like I need to add a rockchip config. [1] net: dwc_eth_qos: update the compatible supported for STM32 http://patchwork.ozlabs.org/project/uboot/patch/20200514130023.15030-1-patrick.delaunay@st.com/ [2] net: dwc_eth_qos: add Kconfig option to select supported configuration http://patchwork.ozlabs.org/project/uboot/list/?series=181931
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index 66a02aa80b..92dab678c7 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -314,6 +314,7 @@ struct eqos_priv { struct eqos_tegra186_regs *tegra186_regs; struct reset_ctl reset_ctl; struct gpio_desc phy_reset_gpio; + u32 reset_delays[3]; struct clk clk_master_bus; struct clk clk_rx; struct clk clk_ptp_ref; @@ -739,6 +740,15 @@ static int eqos_start_resets_stm32(struct udevice *dev) debug("%s(dev=%p):\n", __func__, dev); if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) { + 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->reset_delays[0]); + ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1); if (ret < 0) { pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", @@ -746,7 +756,7 @@ static int eqos_start_resets_stm32(struct udevice *dev) return ret; } - udelay(2); + udelay(eqos->reset_delays[1]); ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0); if (ret < 0) { @@ -754,6 +764,8 @@ static int eqos_start_resets_stm32(struct udevice *dev) ret); return ret; } + + udelay(eqos->reset_delays[2]); } debug("%s: OK\n", __func__); @@ -1864,11 +1876,29 @@ static int eqos_probe_resources_stm32(struct udevice *dev) if (ret) pr_warn("gpio_request_by_name(phy reset) not provided %d", ret); + else + eqos->reset_delays[1] = 2; eqos->phyaddr = ofnode_read_u32_default(phandle_args.node, "reg", -1); } + if (!dm_gpio_is_valid(&eqos->phy_reset_gpio)) { + int reset_flags = GPIOD_IS_OUT; + + if (dev_read_bool(dev, "snps,reset-active-low")) + reset_flags |= GPIOD_ACTIVE_LOW; + + ret = gpio_request_by_name(dev, "snps,reset-gpio", 0, + &eqos->phy_reset_gpio, reset_flags); + if (ret == 0) + ret = dev_read_u32_array(dev, "snps,reset-delays-us", + eqos->reset_delays, 3); + else + pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d", + ret); + } + debug("%s: OK\n", __func__); return 0;
It can be seen that most of the Socs using STM mac, "snps,reset-gpio" gpio is used, adding this option makes reset function more general. Signed-off-by: David Wu <david.wu@rock-chips.com> --- Changes in v2: - Remove the code is not related (Patrice) drivers/net/dwc_eth_qos.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)