| Message ID | 20200430103656.29728-4-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:36 PM, 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> > --- > > drivers/net/dwc_eth_qos.c | 40 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 35 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > index 16988f6bdc..06a8d924a7 100644 > --- a/drivers/net/dwc_eth_qos.c > +++ b/drivers/net/dwc_eth_qos.c > @@ -298,6 +298,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; > @@ -701,6 +702,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]); > + not related to this patch subject > ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1); > if (ret < 0) { > pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", > @@ -708,7 +718,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) { > @@ -716,6 +726,8 @@ static int eqos_start_resets_stm32(struct udevice *dev) > ret); > return ret; > } > + > + udelay(eqos->reset_delays[2]); > } > debug("%s: OK\n", __func__); > > @@ -1065,16 +1077,16 @@ static int eqos_start(struct udevice *dev) > val |= EQOS_DMA_MODE_SWR; > writel(val, &eqos->dma_regs->mode); > limit = eqos->config->swr_wait / 10; > - while (limit--) { > + do { > if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR)) > break; > mdelay(10000); > - } > + } while (limit--); why are you updating this ? it's not related to the purpose of this patch > > if (limit < 0) { > pr_err("EQOS_DMA_MODE_SWR stuck"); > - goto err_stop_clks; > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto err_stop_resets; ditto > } > > ret = eqos->config->ops->eqos_calibrate_pads(dev); > @@ -1712,11 +1724,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; this is not the correct place to set default value. It must be set in case we can't get value from DT below > > 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); in case "snps,reset-delays-us" is not in present DT, all resets-delays are set to 0, see my remark above > + else > + pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d", > + ret); > + } > + > debug("%s: OK\n", __func__); > return 0; >
On 4/30/20 4:36 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. > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > @@ -1712,11 +1724,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); The kernel's bindings/net/snps,dwmac.yaml does not mention any reset-gpios property (which is what the existing code parses just above the portion that is quoted by this patch as context). I suspect that this patch should simply change the name of the property that this function parses to align with the binding, and fix any DTs in U-Boot that also don't match the binding?
On 4/30/20 4:36 PM, Stephen Warren wrote: > On 4/30/20 4:36 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. > >> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > >> @@ -1712,11 +1724,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); > > > The kernel's bindings/net/snps,dwmac.yaml does not mention any > reset-gpios property (which is what the existing code parses just above > the portion that is quoted by this patch as context). I suspect that > this patch should simply change the name of the property that this > function parses to align with the binding, and fix any DTs in U-Boot > that also don't match the binding? Oops, the relevant YAML file is probably ./bindings/net/rockchip-dwmac.txt, although this makes no difference to my statement luckily.
Hi Stephen, 在 2020/5/1 上午6:36, Stephen Warren 写道: > The kernel's bindings/net/snps,dwmac.yaml does not mention any > reset-gpios property (which is what the existing code parses just above > the portion that is quoted by this patch as context). I suspect that > this patch should simply change the name of the property that this > function parses to align with the binding, and fix any DTs in U-Boot > that also don't match the binding? The kernel's ./Documentation/devicetree/bindings/net/stmmac.txt mentions that Required properties: - phy-mode: See ethernet.txt file in the same directory. - snps,reset-gpio gpio number for phy reset. - snps,reset-active-low boolean flag to indicate if phy reset is active low. - snps,reset-delays-us is triplet of delays The 1st cell is reset pre-delay in micro seconds. The 2nd cell is reset pulse in micro seconds. The 3rd cell is reset post-delay in micro seconds.
Hi Patrice, 在 2020/4/30 下午11:47, Patrice CHOTARD 写道: >> @@ -701,6 +702,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]); >> + > not related to this patch subject >> ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1); >> if (ret < 0) { >> pr_err("dm_gpio_set_value(phy_reset, assert) failed: %d", >> @@ -708,7 +718,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) { >> @@ -1712,11 +1724,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; > this is not the correct place to set default value. It must be set in case we can't get value from DT below No, three cases below, it is second case, and we can see udelay(2) in eqos_start_resets_stm32(), here we are to be compatible with the original. - If there is not phy rst, reset_delays is 0; - If "reset-gpios exists in phy node, reset_delays [1] = 2; - "snps, reset-gpio" exists in DT, reset_delays is obtained from DT >> >> 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); > in case "snps,reset-delays-us" is not in present DT, all resets-delays are set to 0, see my remark above >> + else >> + pr_warn("gpio_request_by_name(snps,reset-gpio) failed: %d", >> + ret); >> + } >> + >> debug("%s: OK\n", __func__); >> return 0; >>
Hi Stephen, 在 2020/5/9 上午10:41, David Wu 写道: > > The kernel's ./Documentation/devicetree/bindings/net/stmmac.txt mentions > that Required properties: > > - phy-mode: See ethernet.txt file in the same directory. > - snps,reset-gpio gpio number for phy reset. > - snps,reset-active-low boolean flag to indicate if phy reset is active > low. > - snps,reset-delays-us is triplet of delays > The 1st cell is reset pre-delay in micro seconds. > The 2nd cell is reset pulse in micro seconds. > The 3rd cell is reset post-delay in micro seconds. Sorry, I just saw you replying again before, stmmac.txt was found, this reply email please discard.
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index 16988f6bdc..06a8d924a7 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -298,6 +298,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; @@ -701,6 +702,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", @@ -708,7 +718,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) { @@ -716,6 +726,8 @@ static int eqos_start_resets_stm32(struct udevice *dev) ret); return ret; } + + udelay(eqos->reset_delays[2]); } debug("%s: OK\n", __func__); @@ -1065,16 +1077,16 @@ static int eqos_start(struct udevice *dev) val |= EQOS_DMA_MODE_SWR; writel(val, &eqos->dma_regs->mode); limit = eqos->config->swr_wait / 10; - while (limit--) { + do { if (!(readl(&eqos->dma_regs->mode) & EQOS_DMA_MODE_SWR)) break; mdelay(10000); - } + } while (limit--); if (limit < 0) { pr_err("EQOS_DMA_MODE_SWR stuck"); - goto err_stop_clks; - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto err_stop_resets; } ret = eqos->config->ops->eqos_calibrate_pads(dev); @@ -1712,11 +1724,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> --- drivers/net/dwc_eth_qos.c | 40 ++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-)