[PATCH 1/1] designware: add clock support

[PATCH 1/1] designware: add clock support

[Jean-Christophe PLAGNIOL-VILLARD]

allow the driver to request it's clock and enable it

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/net/designware.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index ec51825..1cbcd5a 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -32,8 +32,10 @@
 #include <asm/mmu.h>
 #include <net/designware.h>
 #include <linux/phy.h>
-#include "designware.h"
+#include <linux/clk.h>
+#include <linux/err.h>
 
+#include "designware.h"
 
 struct dw_eth_dev {
 	struct eth_device netdev;
@@ -390,6 +392,8 @@ static int dwc_ether_probe(struct device_d *dev)
 	struct mii_bus *miibus;
 	void __iomem *base;
 	struct dwc_ether_platform_data *pdata = dev->platform_data;
+	struct clk *clk;
+	int ret;
 
 	if (!pdata) {
 		printf("dwc_ether: no platform_data\n");
@@ -427,6 +431,20 @@ static int dwc_ether_probe(struct device_d *dev)
 	miibus->write = dwc_ether_mii_write;
 	miibus->priv = priv;
 
+	clk = clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		dev_err(dev, "no clk ret = %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_enable(clk);
+	if (ret) {
+		dev_err(dev, "can not enable clk ret = %d\n", ret);
+		clk_put(clk);
+		return ret;
+	}
+
 	mdiobus_register(miibus);
 	eth_register(edev);
 	return 0;
-- 
1.7.10.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] designware: add clock support

[Johannes Stezenbach]

On Sun, Oct 07, 2012 at 03:03:03PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> allow the driver to request it's clock and enable it
...
> +	clk = clk_get(dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		dev_err(dev, "no clk ret = %d\n", ret);
> +		return ret;
> +	}

Just a minor nit: s/allow/require/.  Users of the driver may
now have to define a dummy clock.  Since the actual clock freqeuency
is not used by the driver you could also make it optional?

And while at it, it's "its", not "it's". :-)

Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] designware: add clock support

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:03 Mon 08 Oct     , Johannes Stezenbach wrote:
> On Sun, Oct 07, 2012 at 03:03:03PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > allow the driver to request it's clock and enable it
> ...
> > +	clk = clk_get(dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		ret = PTR_ERR(clk);
> > +		dev_err(dev, "no clk ret = %d\n", ret);
> > +		return ret;
> > +	}
> 
> Just a minor nit: s/allow/require/.  Users of the driver may
> now have to define a dummy clock.  Since the actual clock freqeuency
> is not used by the driver you could also make it optional?
> 
> And while at it, it's "its", not "it's". :-)
srry but there is no user at hte mainline and I'm preparing the ST support
where I nned to enabld the clock and this IP require 2 clock one for the gmac
and one for phy

so no please provide a clock on generic drivers I do want to use the clk
framework

btw which SoC do you use because I see no code for it

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] designware: add clock support

[Johannes Stezenbach]

On Mon, Oct 08, 2012 at 11:27:06AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:03 Mon 08 Oct     , Johannes Stezenbach wrote:
> > On Sun, Oct 07, 2012 at 03:03:03PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > allow the driver to request it's clock and enable it
> > ...
> > > +	clk = clk_get(dev, NULL);
> > > +	if (IS_ERR(clk)) {
> > > +		ret = PTR_ERR(clk);
> > > +		dev_err(dev, "no clk ret = %d\n", ret);
> > > +		return ret;
> > > +	}
> > 
> > Just a minor nit: s/allow/require/.  Users of the driver may
> > now have to define a dummy clock.  Since the actual clock freqeuency
> > is not used by the driver you could also make it optional?
> > 
> > And while at it, it's "its", not "it's". :-)
> srry but there is no user at hte mainline and I'm preparing the ST support
> where I nned to enabld the clock and this IP require 2 clock one for the gmac
> and one for phy
> 
> so no please provide a clock on generic drivers I do want to use the clk
> framework

That is not the issue I tried to point out.  The problem is that
your commit description is inaccurate.

> btw which SoC do you use because I see no code for it

SoC that doesn't exist yet (in development).

Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] designware: add clock support

[Sascha Hauer]

On Sun, Oct 07, 2012 at 03:03:03PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> allow the driver to request it's clock and enable it
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  drivers/net/designware.c |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index ec51825..1cbcd5a 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -32,8 +32,10 @@
>  #include <asm/mmu.h>
>  #include <net/designware.h>
>  #include <linux/phy.h>
> -#include "designware.h"
> +#include <linux/clk.h>
> +#include <linux/err.h>
>  
> +#include "designware.h"
>  
>  struct dw_eth_dev {
>  	struct eth_device netdev;
> @@ -390,6 +392,8 @@ static int dwc_ether_probe(struct device_d *dev)
>  	struct mii_bus *miibus;
>  	void __iomem *base;
>  	struct dwc_ether_platform_data *pdata = dev->platform_data;
> +	struct clk *clk;
> +	int ret;
>  
>  	if (!pdata) {
>  		printf("dwc_ether: no platform_data\n");
> @@ -427,6 +431,20 @@ static int dwc_ether_probe(struct device_d *dev)
>  	miibus->write = dwc_ether_mii_write;
>  	miibus->priv = priv;
>  
> +	clk = clk_get(dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		dev_err(dev, "no clk ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_enable(clk);
> +	if (ret) {
> +		dev_err(dev, "can not enable clk ret = %d\n", ret);
> +		clk_put(clk);
> +		return ret;
> +	}

I really doubt it makes sense to request clks in drivers just to enable
it in barebox. The clock ends up being enabled everytime anyway since
the clk enable is done in the probe function and is never disabled
again. This forces every user of this driver to provide the clk API for
absolutely no gain.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] designware: add clock support

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:03 Wed 10 Oct     , Sascha Hauer wrote:
> On Sun, Oct 07, 2012 at 03:03:03PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > allow the driver to request it's clock and enable it
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  drivers/net/designware.c |   20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> > index ec51825..1cbcd5a 100644
> > --- a/drivers/net/designware.c
> > +++ b/drivers/net/designware.c
> > @@ -32,8 +32,10 @@
> >  #include <asm/mmu.h>
> >  #include <net/designware.h>
> >  #include <linux/phy.h>
> > -#include "designware.h"
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> >  
> > +#include "designware.h"
> >  
> >  struct dw_eth_dev {
> >  	struct eth_device netdev;
> > @@ -390,6 +392,8 @@ static int dwc_ether_probe(struct device_d *dev)
> >  	struct mii_bus *miibus;
> >  	void __iomem *base;
> >  	struct dwc_ether_platform_data *pdata = dev->platform_data;
> > +	struct clk *clk;
> > +	int ret;
> >  
> >  	if (!pdata) {
> >  		printf("dwc_ether: no platform_data\n");
> > @@ -427,6 +431,20 @@ static int dwc_ether_probe(struct device_d *dev)
> >  	miibus->write = dwc_ether_mii_write;
> >  	miibus->priv = priv;
> >  
> > +	clk = clk_get(dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		ret = PTR_ERR(clk);
> > +		dev_err(dev, "no clk ret = %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_enable(clk);
> > +	if (ret) {
> > +		dev_err(dev, "can not enable clk ret = %d\n", ret);
> > +		clk_put(clk);
> > +		return ret;
> > +	}
> 
> I really doubt it makes sense to request clks in drivers just to enable
> it in barebox. The clock ends up being enabled everytime anyway since
> the clk enable is done in the probe function and is never disabled
> again. This forces every user of this driver to provide the clk API for
> absolutely no gain.
I'm working on the phy and on specific part no ready yet but I net the rate
too as this IP support specific rate on SH4

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox