Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Chen-Yu Tsai]

Hi,

On Sat, Dec 7, 2013 at 6:33 PM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> Hi,
>
> On Sat, Dec 07, 2013 at 01:29:34AM +0800, Chen-Yu Tsai wrote:
>> Signed-off-by: Chen-Yu Tsai <wens@xxxxxxxx>
>> ---
>>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/