[5/8] net: dwc_eth_qos: Make clk_rx and clk_tx optional

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 at rock-chips.com>
---

 drivers/net/dwc_eth_qos.c | 65 +++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

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 at 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.