[01/10] net: stmmac: Enable stmmac main clock when probing hardware
diff mbox

Message ID 1386350983-13281-2-git-send-email-wens@csie.org
State New, archived
Headers show

Commit Message

Chen-Yu Tsai Dec. 6, 2013, 5:29 p.m. UTC
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---

Guiseppe previously stated that the "stmmaceth" clock is the
main clock that drives the IP. The stmmac driver does not
enable this clock during the probe phase. When the driver is
built in to the kernel, this is fine because the clock maybe
on by default, or the boot loader had enabled it.

If stmmac is built as a module, when the module is loaded,
the clock may be found unused and disabled by the kernel.

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

Comments

Maxime Ripard Dec. 7, 2013, 10:33 a.m. UTC | #1
Hi,

On Sat, Dec 07, 2013 at 01:29:34AM +0800, Chen-Yu Tsai wrote:
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> 
> Guiseppe previously stated that the "stmmaceth" clock is the
> main clock that drives the IP. The stmmac driver does not
> enable this clock during the probe phase. When the driver is
> built in to the kernel, this is fine because the clock maybe
> on by default, or the boot loader had enabled it.
> 
> If stmmac is built as a module, when the module is loaded,
> the clock may be found unused and disabled by the kernel.

This should be your commit log.

And this is actually not related to the fact that we are building it
as a module or not. You'd face the same issue with a statically built
kernel if the bootloader didn't enable it.

> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8d4ccd3..7da71ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  	if ((phyaddr >= 0) && (phyaddr <= 31))
>  		priv->plat->phy_addr = phyaddr;
>  
> +	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);

You can probably use devm_clk_get to make the exit path smaller.

> +	if (IS_ERR(priv->stmmac_clk)) {
> +		pr_warn("%s: warning: cannot get CSR clock\n", __func__);

And dev_warn here.

> +		goto error_clk_get;
> +	}
> +	clk_prepare_enable(priv->stmmac_clk);
> +
>  	/* Init MAC and get the capabilities */
>  	ret = stmmac_hw_init(priv);
>  	if (ret)
> -		goto error_free_netdev;
> +		goto error_hw_init;
>  	ndev->netdev_ops = &stmmac_netdev_ops;
>  
> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  		goto error_netdev_register;
>  	}
>  
> -	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
> -	if (IS_ERR(priv->stmmac_clk)) {
> -		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
> -		goto error_clk_get;
> -	}
> -
>  	/* If a specific clk_csr value is passed from the platform
>  	 * this means that the CSR Clock Range selection cannot be
>  	 * changed at run-time and it is fixed. Viceversa the driver'll try to
> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  		}
>  	}
>  
> +	clk_disable_unprepare(priv->stmmac_clk);
> +

Hu? Why do you disable the clock? don't you need it afterwards?

Maxime
Chen-Yu Tsai Dec. 9, 2013, 2:43 a.m. UTC | #2
Hi,

On Sat, Dec 7, 2013 at 6:33 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Sat, Dec 07, 2013 at 01:29:34AM +0800, Chen-Yu Tsai wrote:
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>
>> Guiseppe previously stated that the "stmmaceth" clock is the
>> main clock that drives the IP. The stmmac driver does not
>> enable this clock during the probe phase. When the driver is
>> built in to the kernel, this is fine because the clock maybe
>> on by default, or the boot loader had enabled it.
>>
>> If stmmac is built as a module, when the module is loaded,
>> the clock may be found unused and disabled by the kernel.
>
> This should be your commit log.
>
> And this is actually not related to the fact that we are building it
> as a module or not. You'd face the same issue with a statically built
> kernel if the bootloader didn't enable it.

Noted. Will revise commit log.

>
>>
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 8d4ccd3..7da71ed 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>       if ((phyaddr >= 0) && (phyaddr <= 31))
>>               priv->plat->phy_addr = phyaddr;
>>
>> +     priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>
> You can probably use devm_clk_get to make the exit path smaller.

As it stands, the driver does not call clk_put in the exit path.
Using devm_clk_get here would be a good fix.

>
>> +     if (IS_ERR(priv->stmmac_clk)) {
>> +             pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>
> And dev_warn here.
>
>> +             goto error_clk_get;
>> +     }
>> +     clk_prepare_enable(priv->stmmac_clk);
>> +
>>       /* Init MAC and get the capabilities */
>>       ret = stmmac_hw_init(priv);
>>       if (ret)
>> -             goto error_free_netdev;
>> +             goto error_hw_init;
>>       ndev->netdev_ops = &stmmac_netdev_ops;
>>
>> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>               goto error_netdev_register;
>>       }
>>
>> -     priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>> -     if (IS_ERR(priv->stmmac_clk)) {
>> -             pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>> -             goto error_clk_get;
>> -     }
>> -
>>       /* If a specific clk_csr value is passed from the platform
>>        * this means that the CSR Clock Range selection cannot be
>>        * changed at run-time and it is fixed. Viceversa the driver'll try to
>> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>               }
>>       }
>>
>> +     clk_disable_unprepare(priv->stmmac_clk);
>> +
>
> Hu? Why do you disable the clock? don't you need it afterwards?

The clock is enabled in *_open (when the network interface is used),
and disabled in *_close.
Giuseppe CAVALLARO Dec. 9, 2013, 7:14 a.m. UTC | #3
Hello Chen-Yu

On 12/6/2013 6:29 PM, Chen-Yu Tsai wrote:
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>
> Guiseppe previously stated that the "stmmaceth" clock is the
> main clock that drives the IP. The stmmac driver does not
> enable this clock during the probe phase. When the driver is
> built in to the kernel, this is fine because the clock maybe
> on by default, or the boot loader had enabled it.
>
> If stmmac is built as a module, when the module is loaded,
> the clock may be found unused and disabled by the kernel.

the clk_prepare_enable is then called in the open.
Is it not working for you?
Do you mean that the stmmac_hw_init fails if you do not move
the clk_get and clk_prepare_enable on top of the stmmac_dvr_probe?

Peppe

>
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8d4ccd3..7da71ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>   	if ((phyaddr >= 0) && (phyaddr <= 31))
>   		priv->plat->phy_addr = phyaddr;
>
> +	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
> +	if (IS_ERR(priv->stmmac_clk)) {
> +		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
> +		goto error_clk_get;
> +	}
> +	clk_prepare_enable(priv->stmmac_clk);
> +
>   	/* Init MAC and get the capabilities */
>   	ret = stmmac_hw_init(priv);
>   	if (ret)
> -		goto error_free_netdev;
> +		goto error_hw_init;
>
>   	ndev->netdev_ops = &stmmac_netdev_ops;
>
> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>   		goto error_netdev_register;
>   	}
>
> -	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
> -	if (IS_ERR(priv->stmmac_clk)) {
> -		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
> -		goto error_clk_get;
> -	}
> -
>   	/* If a specific clk_csr value is passed from the platform
>   	 * this means that the CSR Clock Range selection cannot be
>   	 * changed at run-time and it is fixed. Viceversa the driver'll try to
> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>   		}
>   	}
>
> +	clk_disable_unprepare(priv->stmmac_clk);
> +
>   	return priv;
>
>   error_mdio_register:
> -	clk_put(priv->stmmac_clk);
> -error_clk_get:
>   	unregister_netdev(ndev);
>   error_netdev_register:
>   	netif_napi_del(&priv->napi);
> -error_free_netdev:
> +error_hw_init:
> +	clk_disable_unprepare(priv->stmmac_clk);
> +	clk_put(priv->stmmac_clk);
> +error_clk_get:
>   	free_netdev(ndev);
>
>   	return NULL;
>
Chen-Yu Tsai Dec. 9, 2013, 7:26 a.m. UTC | #4
Hi Peppe,

On Mon, Dec 9, 2013 at 3:14 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> Hello Chen-Yu
>
>
> On 12/6/2013 6:29 PM, Chen-Yu Tsai wrote:
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>
>> Guiseppe previously stated that the "stmmaceth" clock is the
>> main clock that drives the IP. The stmmac driver does not
>> enable this clock during the probe phase. When the driver is
>> built in to the kernel, this is fine because the clock maybe
>> on by default, or the boot loader had enabled it.
>>
>> If stmmac is built as a module, when the module is loaded,
>> the clock may be found unused and disabled by the kernel.
>
>
> the clk_prepare_enable is then called in the open.
> Is it not working for you?
> Do you mean that the stmmac_hw_init fails if you do not move
> the clk_get and clk_prepare_enable on top of the stmmac_dvr_probe?
>

Exactly. The clock needs to be enabled prior to stmmac_dvr_probe.
Otherwise, stmmac_mdio_register will fail to find a usable PHY.

The DWMAC core I am working with does not support chip ID or
HW capability flags. I suspect those would fail, too.

Disabling the clock at the end of stmmac_dvr_probe is to make
sure it plays nice with existing power management callbacks.

Chen-Yu

>>
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24
>> +++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 8d4ccd3..7da71ed 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device
>> *device,
>>         if ((phyaddr >= 0) && (phyaddr <= 31))
>>                 priv->plat->phy_addr = phyaddr;
>>
>> +       priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>> +       if (IS_ERR(priv->stmmac_clk)) {
>> +               pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>> +               goto error_clk_get;
>> +       }
>> +       clk_prepare_enable(priv->stmmac_clk);
>> +
>>         /* Init MAC and get the capabilities */
>>         ret = stmmac_hw_init(priv);
>>         if (ret)
>> -               goto error_free_netdev;
>> +               goto error_hw_init;
>>
>>         ndev->netdev_ops = &stmmac_netdev_ops;
>>
>> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device
>> *device,
>>                 goto error_netdev_register;
>>         }
>>
>> -       priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>> -       if (IS_ERR(priv->stmmac_clk)) {
>> -               pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>> -               goto error_clk_get;
>> -       }
>> -
>>         /* If a specific clk_csr value is passed from the platform
>>          * this means that the CSR Clock Range selection cannot be
>>          * changed at run-time and it is fixed. Viceversa the driver'll
>> try to
>> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device
>> *device,
>>                 }
>>         }
>>
>> +       clk_disable_unprepare(priv->stmmac_clk);
>> +
>>         return priv;
>>
>>   error_mdio_register:
>> -       clk_put(priv->stmmac_clk);
>> -error_clk_get:
>>         unregister_netdev(ndev);
>>   error_netdev_register:
>>         netif_napi_del(&priv->napi);
>> -error_free_netdev:
>> +error_hw_init:
>> +       clk_disable_unprepare(priv->stmmac_clk);
>> +       clk_put(priv->stmmac_clk);
>> +error_clk_get:
>>         free_netdev(ndev);
>>
>>         return NULL;
>>
>
Hans de Goede Dec. 9, 2013, 10:09 a.m. UTC | #5
Hi Chen-Yu,

It seems this patch is not enough, gmac won't work for me
with this patchset (*), unless I boot with an uboot which
also has the gmac patches.

Regards,

Hans


*) see my sunxi-test tree where I've dropped your
old version and added this version.
Maxime Ripard Dec. 10, 2013, 8:05 p.m. UTC | #6
Hi,

On Mon, Dec 09, 2013 at 10:43:29AM +0800, Chen-Yu Tsai wrote:
> >> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
> >>               }
> >>       }
> >>
> >> +     clk_disable_unprepare(priv->stmmac_clk);
> >> +
> >
> > Hu? Why do you disable the clock? don't you need it afterwards?
> 
> The clock is enabled in *_open (when the network interface is used),
> and disabled in *_close.

Maybe it is the real issue then.

Why don't you move the clk_disable to _remove then?

Maxime
Chen-Yu Tsai Dec. 12, 2013, 4:31 a.m. UTC | #7
Hi,

On Wed, Dec 11, 2013 at 4:05 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Mon, Dec 09, 2013 at 10:43:29AM +0800, Chen-Yu Tsai wrote:
>> >> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>> >>               }
>> >>       }
>> >>
>> >> +     clk_disable_unprepare(priv->stmmac_clk);
>> >> +
>> >
>> > Hu? Why do you disable the clock? don't you need it afterwards?
>>
>> The clock is enabled in *_open (when the network interface is used),
>> and disabled in *_close.
>
> Maybe it is the real issue then.
>
> Why don't you move the clk_disable to _remove then?

I wasn't sure this was the proper way. However, looking around, it
seems other drivers enable the clock in, _probe, and disable it in
_remove. I will modify stmmac to do so as well.


ChenYu

Patch
diff mbox

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8d4ccd3..7da71ed 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2688,10 +2688,17 @@  struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 	if ((phyaddr >= 0) && (phyaddr <= 31))
 		priv->plat->phy_addr = phyaddr;
 
+	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
+	if (IS_ERR(priv->stmmac_clk)) {
+		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
+		goto error_clk_get;
+	}
+	clk_prepare_enable(priv->stmmac_clk);
+
 	/* Init MAC and get the capabilities */
 	ret = stmmac_hw_init(priv);
 	if (ret)
-		goto error_free_netdev;
+		goto error_hw_init;
 
 	ndev->netdev_ops = &stmmac_netdev_ops;
 
@@ -2729,12 +2736,6 @@  struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 		goto error_netdev_register;
 	}
 
-	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
-	if (IS_ERR(priv->stmmac_clk)) {
-		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
-		goto error_clk_get;
-	}
-
 	/* If a specific clk_csr value is passed from the platform
 	 * this means that the CSR Clock Range selection cannot be
 	 * changed at run-time and it is fixed. Viceversa the driver'll try to
@@ -2759,15 +2760,18 @@  struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 		}
 	}
 
+	clk_disable_unprepare(priv->stmmac_clk);
+
 	return priv;
 
 error_mdio_register:
-	clk_put(priv->stmmac_clk);
-error_clk_get:
 	unregister_netdev(ndev);
 error_netdev_register:
 	netif_napi_del(&priv->napi);
-error_free_netdev:
+error_hw_init:
+	clk_disable_unprepare(priv->stmmac_clk);
+	clk_put(priv->stmmac_clk);
+error_clk_get:
 	free_netdev(ndev);
 
 	return NULL;