Messages in this thread | | | Date | Thu, 30 Jan 2014 04:14:31 +0400 | Subject | Re: [PATCH v2 3/4] net: ethoc: set up MII management bus clock | From | Max Filippov <> |
| |
On Wed, Jan 29, 2014 at 11:01 AM, Florian Fainelli <f.fainelli@gmail.com> 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 <jcmvbkbc@gmail.com> >> --- >> 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 <linux/dma-mapping.h> >> #include <linux/etherdevice.h> >> +#include <linux/clk.h> >> #include <linux/crc32.h> >> #include <linux/interrupt.h> >> #include <linux/io.h> >> @@ -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/
| |