diff mbox series

[RESEND,v2,04/11] net: dwc_eth_qos: Make clk_rx and clk_tx optional

Message ID 20200512095603.29126-5-david.wu@rock-chips.com
State Deferred
Delegated to: Joe Hershberger
Headers show
Series Add dwc_eth_qos support for rockchip | expand

Commit Message

David Wu May 12, 2020, 9:56 a.m. UTC
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>
---

Changes in v2:
- Don't change the Rx and Tx clock names. (Patrice, Stephen)

 drivers/net/dwc_eth_qos.c | 61 +++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

Comments

Patrice CHOTARD May 13, 2020, 8:34 a.m. UTC | #1
Hi David

On 5/12/20 11:56 AM, 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>
> ---
>
> Changes in v2:
> - Don't change the Rx and Tx clock names. (Patrice, Stephen)
>
>  drivers/net/dwc_eth_qos.c | 61 +++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index ae2167637f..bec9bf556b 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -613,16 +613,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)) {
> @@ -639,9 +643,11 @@ static int eqos_start_clks_stm32(struct udevice *dev)
>  
>  #ifdef CONFIG_CLK
>  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:
> @@ -679,8 +685,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);
> @@ -1843,20 +1851,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;
> -	}
> +	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;
> -	}
> +	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);
> @@ -1901,15 +1905,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)
> @@ -1991,8 +1986,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);

Reviewed-by: Patrice Chotard <patrice.chotard@st.com>

Thanks
Patrick DELAUNAY May 13, 2020, 1:17 p.m. UTC | #2
Hi David,

> From: David Wu <david.wu@rock-chips.com>
> Sent: mardi 12 mai 2020 11:56
> 
> 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>
> ---
> 
> Changes in v2:
> - Don't change the Rx and Tx clock names. (Patrice, Stephen)
> 
>  drivers/net/dwc_eth_qos.c | 61 +++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index
> ae2167637f..bec9bf556b 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -613,16 +613,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)) {
> @@ -639,9 +643,11 @@ static int eqos_start_clks_stm32(struct udevice *dev)
> 
>  #ifdef CONFIG_CLK
>  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:
> @@ -679,8 +685,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);
> @@ -1843,20 +1851,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;
> -	}
> +	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;
> -	}
> +	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); @@ -1901,15
> +1905,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) @@ -1991,8
> +1986,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);
> --
> 2.19.1
> 
> 

These clock are mandatory for STM32 glue as explain in Linux binding
Documentation/devicetree/bindings/net/stm32-dwmac.txt

But I fact when when I check the code, I see perhaps an issue in the current U-Boot glue: 
we don't select the STM32 glue for the correct compatible, I think I will push 

static const struct udevice_id eqos_ids[] = {
	{
		.compatible = "nvidia,tegra186-eqos",
		.data = (ulong)&eqos_tegra186_config
	},
	{
-		.compatible = "snps,dwmac-4.20a",
+		.compatible = "st,stm32mp1-dwmac",
		.data = (ulong)&eqos_stm32_config
	},
	{
		.compatible = "fsl,imx-eqos",
		.data = (ulong)&eqos_imx_config
	},

	{ }
};

Then you can manage your own glue for rockchip ETH for your compatible.

Regards

Patrick
Marek Vasut May 13, 2020, 1:53 p.m. UTC | #3
On 5/13/20 3:17 PM, Patrick DELAUNAY wrote:
> Hi David,
> 
>> From: David Wu <david.wu@rock-chips.com>
>> Sent: mardi 12 mai 2020 11:56
>>
>> 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>
>> ---
>>
>> Changes in v2:
>> - Don't change the Rx and Tx clock names. (Patrice, Stephen)
>>
>>  drivers/net/dwc_eth_qos.c | 61 +++++++++++++++++++--------------------
>>  1 file changed, 29 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index
>> ae2167637f..bec9bf556b 100644
>> --- a/drivers/net/dwc_eth_qos.c
>> +++ b/drivers/net/dwc_eth_qos.c
>> @@ -613,16 +613,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)) {
>> @@ -639,9 +643,11 @@ static int eqos_start_clks_stm32(struct udevice *dev)
>>
>>  #ifdef CONFIG_CLK
>>  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:
>> @@ -679,8 +685,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);
>> @@ -1843,20 +1851,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;
>> -	}
>> +	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;
>> -	}
>> +	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); @@ -1901,15
>> +1905,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) @@ -1991,8
>> +1986,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);
>> --
>> 2.19.1
>>
>>
> 
> These clock are mandatory for STM32 glue as explain in Linux binding
> Documentation/devicetree/bindings/net/stm32-dwmac.txt
> 
> But I fact when when I check the code, I see perhaps an issue in the current U-Boot glue: 
> we don't select the STM32 glue for the correct compatible, I think I will push 
> 
> static const struct udevice_id eqos_ids[] = {
> 	{
> 		.compatible = "nvidia,tegra186-eqos",
> 		.data = (ulong)&eqos_tegra186_config
> 	},
> 	{
> -		.compatible = "snps,dwmac-4.20a",
> +		.compatible = "st,stm32mp1-dwmac",
> 		.data = (ulong)&eqos_stm32_config
> 	},
> 	{
> 		.compatible = "fsl,imx-eqos",
> 		.data = (ulong)&eqos_imx_config
> 	},
> 
> 	{ }
> };
> 
> Then you can manage your own glue for rockchip ETH for your compatible.

You might even want to drop the tegra support on ARM32 , thus save space
by dropping useless code.
Patrick DELAUNAY June 8, 2020, 9:29 a.m. UTC | #4
Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mercredi 13 mai 2020 15:54
> 
> On 5/13/20 3:17 PM, Patrick DELAUNAY wrote:
> > Hi David,
> >
> >> From: David Wu <david.wu@rock-chips.com>
> >> Sent: mardi 12 mai 2020 11:56
> >>
> >> 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>
> >> ---
> >>
> >> Changes in v2:
> >> - Don't change the Rx and Tx clock names. (Patrice, Stephen)
> >>
> >>  drivers/net/dwc_eth_qos.c | 61
> >> +++++++++++++++++++--------------------
> >>  1 file changed, 29 insertions(+), 32 deletions(-)
> >>
[...]
> >
> > These clock are mandatory for STM32 glue as explain in Linux binding
> > Documentation/devicetree/bindings/net/stm32-dwmac.txt
> >
> > But I fact when when I check the code, I see perhaps an issue in the current U-
> Boot glue:
> > we don't select the STM32 glue for the correct compatible, I think I
> > will push
> >
> > static const struct udevice_id eqos_ids[] = {
> > 	{
> > 		.compatible = "nvidia,tegra186-eqos",
> > 		.data = (ulong)&eqos_tegra186_config
> > 	},
> > 	{
> > -		.compatible = "snps,dwmac-4.20a",
> > +		.compatible = "st,stm32mp1-dwmac",
> > 		.data = (ulong)&eqos_stm32_config
> > 	},
> > 	{
> > 		.compatible = "fsl,imx-eqos",
> > 		.data = (ulong)&eqos_imx_config
> > 	},
> >
> > 	{ }
> > };
> >
> > Then you can manage your own glue for rockchip ETH for your compatible.
> 
> You might even want to drop the tegra support on ARM32 , thus save space by
> dropping useless code.

For information I push 2 patches after this remark:

[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
Marek Vasut June 8, 2020, 9:45 a.m. UTC | #5
On 6/8/20 11:29 AM, Patrick DELAUNAY wrote:
[...]
>>> we don't select the STM32 glue for the correct compatible, I think I
>>> will push
>>>
>>> static const struct udevice_id eqos_ids[] = {
>>> 	{
>>> 		.compatible = "nvidia,tegra186-eqos",
>>> 		.data = (ulong)&eqos_tegra186_config
>>> 	},
>>> 	{
>>> -		.compatible = "snps,dwmac-4.20a",
>>> +		.compatible = "st,stm32mp1-dwmac",
>>> 		.data = (ulong)&eqos_stm32_config
>>> 	},
>>> 	{
>>> 		.compatible = "fsl,imx-eqos",
>>> 		.data = (ulong)&eqos_imx_config
>>> 	},
>>>
>>> 	{ }
>>> };
>>>
>>> Then you can manage your own glue for rockchip ETH for your compatible.
>>
>> You might even want to drop the tegra support on ARM32 , thus save space by
>> dropping useless code.
> 
> For information I push 2 patches after this remark:
> 
> [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

That's for -next, right ?
Patrick DELAUNAY June 8, 2020, 5:05 p.m. UTC | #6
Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: lundi 8 juin 2020 11:45
> 
> On 6/8/20 11:29 AM, Patrick DELAUNAY wrote:
> [...]
> >>> we don't select the STM32 glue for the correct compatible, I think I
> >>> will push
> >>>
> >>> static const struct udevice_id eqos_ids[] = {
> >>> 	{
> >>> 		.compatible = "nvidia,tegra186-eqos",
> >>> 		.data = (ulong)&eqos_tegra186_config
> >>> 	},
> >>> 	{
> >>> -		.compatible = "snps,dwmac-4.20a",
> >>> +		.compatible = "st,stm32mp1-dwmac",
> >>> 		.data = (ulong)&eqos_stm32_config
> >>> 	},
> >>> 	{
> >>> 		.compatible = "fsl,imx-eqos",
> >>> 		.data = (ulong)&eqos_imx_config
> >>> 	},
> >>>
> >>> 	{ }
> >>> };
> >>>
> >>> Then you can manage your own glue for rockchip ETH for your compatible.
> >>
> >> You might even want to drop the tegra support on ARM32 , thus save
> >> space by dropping useless code.
> >
> > For information I push 2 patches after this remark:
> >
> > [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
> 
> That's for -next, right ?

Yes both are for -next.

It is driver improvement (=code size reduction and cleanup compatible) and no bugfix.

Patrick.
Marek Vasut June 8, 2020, 5:14 p.m. UTC | #7
On 6/8/20 7:05 PM, Patrick DELAUNAY wrote:
> Dear Marek,

Hi,

>> From: Marek Vasut <marex@denx.de>
>> Sent: lundi 8 juin 2020 11:45
>>
>> On 6/8/20 11:29 AM, Patrick DELAUNAY wrote:
>> [...]
>>>>> we don't select the STM32 glue for the correct compatible, I think I
>>>>> will push
>>>>>
>>>>> static const struct udevice_id eqos_ids[] = {
>>>>> 	{
>>>>> 		.compatible = "nvidia,tegra186-eqos",
>>>>> 		.data = (ulong)&eqos_tegra186_config
>>>>> 	},
>>>>> 	{
>>>>> -		.compatible = "snps,dwmac-4.20a",
>>>>> +		.compatible = "st,stm32mp1-dwmac",
>>>>> 		.data = (ulong)&eqos_stm32_config
>>>>> 	},
>>>>> 	{
>>>>> 		.compatible = "fsl,imx-eqos",
>>>>> 		.data = (ulong)&eqos_imx_config
>>>>> 	},
>>>>>
>>>>> 	{ }
>>>>> };
>>>>>
>>>>> Then you can manage your own glue for rockchip ETH for your compatible.
>>>>
>>>> You might even want to drop the tegra support on ARM32 , thus save
>>>> space by dropping useless code.
>>>
>>> For information I push 2 patches after this remark:
>>>
>>> [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
>>
>> That's for -next, right ?
> 
> Yes both are for -next.
> 
> It is driver improvement (=code size reduction and cleanup compatible) and no bugfix.

I am still not entirely sure whether the ifdeffery is the way to go.
I wonder whether we can't rather somehow use the linker-lists to
generate a list of compatible strings at runtime (like we do for
commands) and then reduce that list to only the compatible strings
present in the DT (that's a bit tricky).
diff mbox series

Patch

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index ae2167637f..bec9bf556b 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -613,16 +613,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)) {
@@ -639,9 +643,11 @@  static int eqos_start_clks_stm32(struct udevice *dev)
 
 #ifdef CONFIG_CLK
 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:
@@ -679,8 +685,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);
@@ -1843,20 +1851,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;
-	}
+	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;
-	}
+	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);
@@ -1901,15 +1905,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)
@@ -1991,8 +1986,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);