Re: [PATCH v2 3/4] net: ethoc: set up MII management bus clock

From: Max Filippov
Date: Wed Jan 29 2014 - 19:14:39 EST

On Wed, Jan 29, 2014 at 11:01 AM, Florian Fainelli <f.fainelli@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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", &eth_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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/