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

2014-01-29 Thread Max Filippov
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(>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(>dev)) {
>> -   struct ethoc_platform_data *pdata =
>> dev_get_platdata(>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(>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 / 250 + 1);
>> +
>> +   if (!clkdiv)
>> +   clkdiv = 2;
>> +   dev_dbg(>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 

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

2014-01-29 Thread Max Filippov
On Wed, Jan 29, 2014 at 11:01 AM, Florian Fainelli f.faine...@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 jcmvb...@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, 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 / 250 + 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);
 

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

2014-01-28 Thread Florian Fainelli

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(>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(>dev)) {
-   struct ethoc_platform_data *pdata = 
dev_get_platdata(>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?



+   else
+   of_property_read_u32(pdev->dev.of_node,
+"clock-frequency", _clkfreq);
+   if (!eth_clkfreq) {
+   struct clk *clk = clk_get(>dev, NULL);
+
+   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 / 250 + 1);
+
+   if (!clkdiv)
+   clkdiv = 2;
+   dev_dbg(>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.



+
/* 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)).


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


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 */



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-01-28 Thread Max Filippov
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(>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(>dev)) {
-   struct ethoc_platform_data *pdata = 
dev_get_platdata(>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;
+   else
+   of_property_read_u32(pdev->dev.of_node,
+"clock-frequency", _clkfreq);
+   if (!eth_clkfreq) {
+   struct clk *clk = clk_get(>dev, NULL);
+
+   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 / 250 + 1);
+
+   if (!clkdiv)
+   clkdiv = 2;
+   dev_dbg(>dev, "setting MII clkdiv to %u\n", clkdiv);
+   ethoc_write(priv, MIIMODER,
+   (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE) |
+   clkdiv);
+   }
+
/* 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);
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);
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 */
-- 
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-01-28 Thread Max Filippov
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 jcmvb...@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;
+   else
+   of_property_read_u32(pdev-dev.of_node,
+clock-frequency, eth_clkfreq);
+   if (!eth_clkfreq) {
+   struct clk *clk = clk_get(pdev-dev, NULL);
+
+   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 / 250 + 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);
+   }
+
/* 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);
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);
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 */
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-01-28 Thread Florian Fainelli

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 jcmvb...@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?



+   else
+   of_property_read_u32(pdev-dev.of_node,
+clock-frequency, eth_clkfreq);
+   if (!eth_clkfreq) {
+   struct clk *clk = clk_get(pdev-dev, NULL);
+
+   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 / 250 + 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.



+
/* 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)).


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


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 */



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at