| Message ID | 20200430104343.30843-1-david.wu@rock-chips.com |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
| Series | Add dwc_eth_qos support for rockchip | expand |
Hi David On 4/30/20 12:43 PM, David Wu wrote: > For others using, clk_rx and clk_tx may not be necessary, > and their clock names are different. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > > drivers/net/dwc_eth_qos.c | 65 +++++++++++++++++++-------------------- > 1 file changed, 31 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > index fbd6caf85b..b5d5156292 100644 > --- a/drivers/net/dwc_eth_qos.c > +++ b/drivers/net/dwc_eth_qos.c > @@ -592,16 +592,20 @@ static int eqos_start_clks_stm32(struct udevice *dev) > goto err; > } > > - ret = clk_enable(&eqos->clk_rx); > - if (ret < 0) { > - pr_err("clk_enable(clk_rx) failed: %d", ret); > - goto err_disable_clk_master_bus; > + if (clk_valid(&eqos->clk_rx)) { > + ret = clk_enable(&eqos->clk_rx); > + if (ret < 0) { > + pr_err("clk_enable(clk_rx) failed: %d", ret); > + goto err_disable_clk_master_bus; > + } > } > > - ret = clk_enable(&eqos->clk_tx); > - if (ret < 0) { > - pr_err("clk_enable(clk_tx) failed: %d", ret); > - goto err_disable_clk_rx; > + if (clk_valid(&eqos->clk_tx)) { > + ret = clk_enable(&eqos->clk_tx); > + if (ret < 0) { > + pr_err("clk_enable(clk_tx) failed: %d", ret); > + goto err_disable_clk_rx; > + } > } > > if (clk_valid(&eqos->clk_ck)) { > @@ -616,9 +620,11 @@ static int eqos_start_clks_stm32(struct udevice *dev) > return 0; > > err_disable_clk_tx: > - clk_disable(&eqos->clk_tx); > + if (clk_valid(&eqos->clk_tx)) > + clk_disable(&eqos->clk_tx); > err_disable_clk_rx: > - clk_disable(&eqos->clk_rx); > + if (clk_valid(&eqos->clk_rx)) > + clk_disable(&eqos->clk_rx); > err_disable_clk_master_bus: > clk_disable(&eqos->clk_master_bus); > err: > @@ -647,8 +653,10 @@ static void eqos_stop_clks_stm32(struct udevice *dev) > > debug("%s(dev=%p):\n", __func__, dev); > > - clk_disable(&eqos->clk_tx); > - clk_disable(&eqos->clk_rx); > + if (clk_valid(&eqos->clk_tx)) > + clk_disable(&eqos->clk_tx); > + if (clk_valid(&eqos->clk_rx)) > + clk_disable(&eqos->clk_rx); > clk_disable(&eqos->clk_master_bus); > if (clk_valid(&eqos->clk_ck)) > clk_disable(&eqos->clk_ck); > @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev) > ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus); > if (ret) { > pr_err("clk_get_by_name(master_bus) failed: %d", ret); > - goto err_probe; > + return ret; > } why are you changing the error path here ? > > - ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx); > - if (ret) { > - pr_err("clk_get_by_name(rx) failed: %d", ret); > - goto err_free_clk_master_bus; > - } > + ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx); > + if (ret) > + pr_warn("clk_get_by_name(rx) failed: %d", ret); > > - ret = clk_get_by_name(dev, "mac-clk-tx", &eqos->clk_tx); > - if (ret) { > - pr_err("clk_get_by_name(tx) failed: %d", ret); > - goto err_free_clk_rx; > - } > + ret = clk_get_by_name(dev, "mac_clk_tx", &eqos->clk_tx); > + if (ret) > + pr_warn("clk_get_by_name(tx) failed: %d", ret); Nak Why are you changing the Rx and Tx clock names ? for information, check with the kernel dt bindings regarding this driver: Documentation/devicetree/bindings/net/stm32-dwmac.txt This patch is breaking ethernet on STM32MP1 boards > > /* Get ETH_CLK clocks (optional) */ > ret = clk_get_by_name(dev, "eth-ck", &eqos->clk_ck); > @@ -1749,15 +1753,6 @@ static int eqos_probe_resources_stm32(struct udevice *dev) > > debug("%s: OK\n", __func__); > return 0; > - > -err_free_clk_rx: > - clk_free(&eqos->clk_rx); > -err_free_clk_master_bus: > - clk_free(&eqos->clk_master_bus); > -err_probe: > - > - debug("%s: returns %d\n", __func__, ret); > - return ret; > } > > static phy_interface_t eqos_get_interface_stm32(struct udevice *dev) > @@ -1803,8 +1798,10 @@ static int eqos_remove_resources_stm32(struct udevice *dev) > > debug("%s(dev=%p):\n", __func__, dev); > > - clk_free(&eqos->clk_tx); > - clk_free(&eqos->clk_rx); > + if (clk_valid(&eqos->clk_tx)) > + clk_free(&eqos->clk_tx); > + if (clk_valid(&eqos->clk_rx)) > + clk_free(&eqos->clk_rx); > clk_free(&eqos->clk_master_bus); > if (clk_valid(&eqos->clk_ck)) > clk_free(&eqos->clk_ck);
On 4/30/20 4:43 AM, David Wu wrote: > For others using, clk_rx and clk_tx may not be necessary, > and their clock names are different. > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev) > ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus); > if (ret) { > pr_err("clk_get_by_name(master_bus) failed: %d", ret); > - goto err_probe; > + return ret; > } > > - ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx); > - if (ret) { > - pr_err("clk_get_by_name(rx) failed: %d", ret); > - goto err_free_clk_master_bus; > - } > + ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx); > + if (ret) > + pr_warn("clk_get_by_name(rx) failed: %d", ret); Oh... Judging by your email, you're trying to make this driver work on a Rockchip system. However, you're editing an STM32-specific probe function. You should introduce a new probe function for Rockchip if it needs to work differently to the existing STM32 code. Also, mac_clk_rx isn't a valid DT property name; they aren't supposed to have _ in them. I don't see mac_clk_rx or mac-clk-rx in the DT binding file in Documentation/bindings/net/rockchip-dwmac.txt the kernel. That should probably be submitted/reviewed/applied before using the binding...
Hi Patrice, 在 2020/4/30 下午10:00, Patrice CHOTARD 写道: >> @@ -647,8 +653,10 @@ static void eqos_stop_clks_stm32(struct udevice *dev) >> >> debug("%s(dev=%p):\n", __func__, dev); >> >> - clk_disable(&eqos->clk_tx); >> - clk_disable(&eqos->clk_rx); >> + if (clk_valid(&eqos->clk_tx)) >> + clk_disable(&eqos->clk_tx); >> + if (clk_valid(&eqos->clk_rx)) >> + clk_disable(&eqos->clk_rx); >> clk_disable(&eqos->clk_master_bus); >> if (clk_valid(&eqos->clk_ck)) >> clk_disable(&eqos->clk_ck); >> @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev) >> ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus); >> if (ret) { >> pr_err("clk_get_by_name(master_bus) failed: %d", ret); >> - goto err_probe; >> + return ret; >> } > why are you changing the error path here ? The following code has not returned, so for the sake of simpler code, return directly. >> >> - ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx); >> - if (ret) { >> - pr_err("clk_get_by_name(rx) failed: %d", ret); >> - goto err_free_clk_master_bus; >> - } >> + ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx); >> + if (ret) >> + pr_warn("clk_get_by_name(rx) failed: %d", ret); >> >> - ret = clk_get_by_name(dev, "mac-clk-tx", &eqos->clk_tx); >> - if (ret) { >> - pr_err("clk_get_by_name(tx) failed: %d", ret); >> - goto err_free_clk_rx; >> - } >> + ret = clk_get_by_name(dev, "mac_clk_tx", &eqos->clk_tx); >> + if (ret) >> + pr_warn("clk_get_by_name(tx) failed: %d", ret); > Nak > > Why are you changing the Rx and Tx clock names ? > > for information, check with the kernel dt bindings regarding this driver: > > Documentation/devicetree/bindings/net/stm32-dwmac.txt > > This patch is breaking ethernet on STM32MP1 boards I should have made a mistake here. In fact, for Rockchip, there is no need to obtain this two clock. My intention is to make these two clocks optional. >
Hi Stephen, 在 2020/5/1 上午6:45, Stephen Warren 写道: > Oh... Judging by your email, you're trying to make this driver work on a > Rockchip system. However, you're editing an STM32-specific probe > function. You should introduce a new probe function for Rockchip if it > needs to work differently to the existing STM32 code. > > Also, mac_clk_rx isn't a valid DT property name; they aren't supposed to > have _ in them. I don't see mac_clk_rx or mac-clk-rx in the DT binding > file in Documentation/bindings/net/rockchip-dwmac.txt the kernel. That > should probably be submitted/reviewed/applied before using the binding... If necessary, I can rewrite a function to obtain resources, but I think the function of rockchip and stm should be very similar, and can be shared.
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index fbd6caf85b..b5d5156292 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -592,16 +592,20 @@ static int eqos_start_clks_stm32(struct udevice *dev) goto err; } - ret = clk_enable(&eqos->clk_rx); - if (ret < 0) { - pr_err("clk_enable(clk_rx) failed: %d", ret); - goto err_disable_clk_master_bus; + if (clk_valid(&eqos->clk_rx)) { + ret = clk_enable(&eqos->clk_rx); + if (ret < 0) { + pr_err("clk_enable(clk_rx) failed: %d", ret); + goto err_disable_clk_master_bus; + } } - ret = clk_enable(&eqos->clk_tx); - if (ret < 0) { - pr_err("clk_enable(clk_tx) failed: %d", ret); - goto err_disable_clk_rx; + if (clk_valid(&eqos->clk_tx)) { + ret = clk_enable(&eqos->clk_tx); + if (ret < 0) { + pr_err("clk_enable(clk_tx) failed: %d", ret); + goto err_disable_clk_rx; + } } if (clk_valid(&eqos->clk_ck)) { @@ -616,9 +620,11 @@ static int eqos_start_clks_stm32(struct udevice *dev) return 0; err_disable_clk_tx: - clk_disable(&eqos->clk_tx); + if (clk_valid(&eqos->clk_tx)) + clk_disable(&eqos->clk_tx); err_disable_clk_rx: - clk_disable(&eqos->clk_rx); + if (clk_valid(&eqos->clk_rx)) + clk_disable(&eqos->clk_rx); err_disable_clk_master_bus: clk_disable(&eqos->clk_master_bus); err: @@ -647,8 +653,10 @@ static void eqos_stop_clks_stm32(struct udevice *dev) debug("%s(dev=%p):\n", __func__, dev); - clk_disable(&eqos->clk_tx); - clk_disable(&eqos->clk_rx); + if (clk_valid(&eqos->clk_tx)) + clk_disable(&eqos->clk_tx); + if (clk_valid(&eqos->clk_rx)) + clk_disable(&eqos->clk_rx); clk_disable(&eqos->clk_master_bus); if (clk_valid(&eqos->clk_ck)) clk_disable(&eqos->clk_ck); @@ -1691,20 +1699,16 @@ static int eqos_probe_resources_stm32(struct udevice *dev) ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus); if (ret) { pr_err("clk_get_by_name(master_bus) failed: %d", ret); - goto err_probe; + return ret; } - ret = clk_get_by_name(dev, "mac-clk-rx", &eqos->clk_rx); - if (ret) { - pr_err("clk_get_by_name(rx) failed: %d", ret); - goto err_free_clk_master_bus; - } + ret = clk_get_by_name(dev, "mac_clk_rx", &eqos->clk_rx); + if (ret) + pr_warn("clk_get_by_name(rx) failed: %d", ret); - ret = clk_get_by_name(dev, "mac-clk-tx", &eqos->clk_tx); - if (ret) { - pr_err("clk_get_by_name(tx) failed: %d", ret); - goto err_free_clk_rx; - } + ret = clk_get_by_name(dev, "mac_clk_tx", &eqos->clk_tx); + if (ret) + pr_warn("clk_get_by_name(tx) failed: %d", ret); /* Get ETH_CLK clocks (optional) */ ret = clk_get_by_name(dev, "eth-ck", &eqos->clk_ck); @@ -1749,15 +1753,6 @@ static int eqos_probe_resources_stm32(struct udevice *dev) debug("%s: OK\n", __func__); return 0; - -err_free_clk_rx: - clk_free(&eqos->clk_rx); -err_free_clk_master_bus: - clk_free(&eqos->clk_master_bus); -err_probe: - - debug("%s: returns %d\n", __func__, ret); - return ret; } static phy_interface_t eqos_get_interface_stm32(struct udevice *dev) @@ -1803,8 +1798,10 @@ static int eqos_remove_resources_stm32(struct udevice *dev) debug("%s(dev=%p):\n", __func__, dev); - clk_free(&eqos->clk_tx); - clk_free(&eqos->clk_rx); + if (clk_valid(&eqos->clk_tx)) + clk_free(&eqos->clk_tx); + if (clk_valid(&eqos->clk_rx)) + clk_free(&eqos->clk_rx); clk_free(&eqos->clk_master_bus); if (clk_valid(&eqos->clk_ck)) clk_free(&eqos->clk_ck);
For others using, clk_rx and clk_tx may not be necessary, and their clock names are different. Signed-off-by: David Wu <david.wu@rock-chips.com> --- drivers/net/dwc_eth_qos.c | 65 +++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 34 deletions(-)