Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752362AbaA3AOd (ORCPT ); Wed, 29 Jan 2014 19:14:33 -0500 Received: from mail-ob0-f173.google.com ([209.85.214.173]:63103 "EHLO mail-ob0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751116AbaA3AOc convert rfc822-to-8bit (ORCPT ); Wed, 29 Jan 2014 19:14:32 -0500 MIME-Version: 1.0 In-Reply-To: <52E8A74F.9060603@gmail.com> References: <1390975218-13863-1-git-send-email-jcmvbkbc@gmail.com> <1390975218-13863-4-git-send-email-jcmvbkbc@gmail.com> <52E8A74F.9060603@gmail.com> Date: Thu, 30 Jan 2014 04:14:31 +0400 Message-ID: Subject: Re: [PATCH v2 3/4] net: ethoc: set up MII management bus clock From: Max Filippov To: Florian Fainelli Cc: "linux-xtensa@linux-xtensa.org" , netdev , LKML , Chris Zankel , Marc Gauthier , "David S. Miller" , Ben Hutchings Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 29, 2014 at 11:01 AM, Florian Fainelli wrote: > Le 28/01/2014 22:00, Max Filippov a écrit : > >> MII management bus clock is derived from the MAC clock by dividing it by >> MIIMODER register CLKDIV field value. This value may need to be set up >> in case it is undefined or its default value is too high (and >> communication with PHY is too slow) or too low (and communication with >> PHY is impossible). The value of CLKDIV is not specified directly, but >> is derived from the MAC clock for the default MII management bus frequency >> of 2.5MHz. The MAC clock may be specified in the platform data, or as >> either 'clock-frequency' or 'clocks' device tree attribute. >> >> Signed-off-by: Max Filippov >> --- >> Changes v1->v2: >> - drop MDIO bus frequency configurability, always configure for standard >> 2.5MHz; >> - allow using common clock framework to provide ethoc clock. >> >> drivers/net/ethernet/ethoc.c | 37 +++++++++++++++++++++++++++++++++++-- >> include/net/ethoc.h | 1 + >> 2 files changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c >> index 5643b2d..5854d41 100644 >> --- a/drivers/net/ethernet/ethoc.c >> +++ b/drivers/net/ethernet/ethoc.c >> @@ -13,6 +13,7 @@ >> >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -216,6 +217,7 @@ struct ethoc { >> >> struct phy_device *phy; >> struct mii_bus *mdio; >> + struct clk *clk; >> s8 phy_id; >> }; >> >> @@ -925,6 +927,8 @@ static int ethoc_probe(struct platform_device *pdev) >> int num_bd; >> int ret = 0; >> bool random_mac = false; >> + u32 eth_clkfreq = 0; >> + struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev); >> >> /* allocate networking device */ >> netdev = alloc_etherdev(sizeof(struct ethoc)); >> @@ -1038,8 +1042,7 @@ static int ethoc_probe(struct platform_device *pdev) >> } >> >> /* Allow the platform setup code to pass in a MAC address. */ >> - if (dev_get_platdata(&pdev->dev)) { >> - struct ethoc_platform_data *pdata = >> dev_get_platdata(&pdev->dev); >> + if (pdata) { >> memcpy(netdev->dev_addr, pdata->hwaddr, IFHWADDRLEN); >> priv->phy_id = pdata->phy_id; >> } else { >> @@ -1077,6 +1080,32 @@ static int ethoc_probe(struct platform_device >> *pdev) >> if (random_mac) >> netdev->addr_assign_type = NET_ADDR_RANDOM; >> >> + /* Allow the platform setup code to adjust MII management bus >> clock. */ >> + if (pdata) >> + eth_clkfreq = pdata->eth_clkfreq; > > Since this is a new member, why not make it a struct clk pointer directly so > you could simplify the code path? Basically this is to provide flexibility for the user: it may be more appropriate to specify frequency if it's known and fixed, otherwise clk_get below would find the clock registered for this device/generic clock. >> + else >> + of_property_read_u32(pdev->dev.of_node, >> + "clock-frequency", ð_clkfreq); >> + if (!eth_clkfreq) { >> + struct clk *clk = clk_get(&pdev->dev, NULL); This should have been devm_clk_get. >> + >> + if (!IS_ERR(clk)) { >> + priv->clk = clk; >> + clk_prepare_enable(clk); >> + eth_clkfreq = clk_get_rate(clk); >> + } >> + } >> + if (eth_clkfreq) { >> + u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / 2500000 + 1); >> + >> + if (!clkdiv) >> + clkdiv = 2; >> + dev_dbg(&pdev->dev, "setting MII clkdiv to %u\n", clkdiv); >> + ethoc_write(priv, MIIMODER, >> + (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE) >> | >> + clkdiv); >> + } > > > This does look a bit convoluted, and it looks like the clk_get() or getting > the clock-frequency property should boil down to being the same thing with > of_clk_get() as it should resolve all clocks phandles and fetch their > frequencies appropriately. I can drop clock-frequency property checking to encourage usage of common clock framework. I don't quite understand the rest of the objection, could you please rephrase it? clk_get calls of_clk_get internally. >> + >> /* register MII bus */ >> priv->mdio = mdiobus_alloc(); >> if (!priv->mdio) { >> @@ -1141,6 +1170,8 @@ free_mdio: >> kfree(priv->mdio->irq); >> mdiobus_free(priv->mdio); >> free: >> + if (priv->clk) >> + clk_disable_unprepare(priv->clk); > > > This should probbaly be if (!IS_ERR(priv-clk)). No, I haven't changed priv->clk when get_clk returned error. >> free_netdev(netdev); >> out: >> return ret; >> @@ -1165,6 +1196,8 @@ static int ethoc_remove(struct platform_device >> *pdev) >> kfree(priv->mdio->irq); >> mdiobus_free(priv->mdio); >> } >> + if (priv->clk) >> + clk_disable_unprepare(priv->clk); > > > Same here Ditto. >> unregister_netdev(netdev); >> free_netdev(netdev); >> } >> diff --git a/include/net/ethoc.h b/include/net/ethoc.h >> index 96f3789..2a2d6bb 100644 >> --- a/include/net/ethoc.h >> +++ b/include/net/ethoc.h >> @@ -16,6 +16,7 @@ >> struct ethoc_platform_data { >> u8 hwaddr[IFHWADDRLEN]; >> s8 phy_id; >> + u32 eth_clkfreq; >> }; >> >> #endif /* !LINUX_NET_ETHOC_H */ >> -- Thanks. -- Max -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/