lkml.org 
[lkml]   [2013]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware
    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.


    \
     
     \ /
      Last update: 2013-12-09 04:21    [W:3.628 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site