[PATCH 7/9] ARM: zynq: clk: add pll type

2013-03-11 Thread Steffen Trumtrar 
Signed-off-by: Steffen Trumtrar 
---
 arch/arm/mach-zynq/clk-zynq7000.c | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-zynq/clk-zynq7000.c 
b/arch/arm/mach-zynq/clk-zynq7000.c
index 5a8a12a..0d3c3a8 100644
--- a/arch/arm/mach-zynq/clk-zynq7000.c
+++ b/arch/arm/mach-zynq/clk-zynq7000.c
@@ -33,10 +33,21 @@ enum zynq_clks {
cpu_clk, cpu_6x4x, cpu_3x2x, cpu_2x, cpu_1x, clks_max
 };
 
+enum zynq_pll_type {
+   ZYNQ_PLL_ARM,
+   ZYNQ_PLL_DDR,
+   ZYNQ_PLL_IO,
+};
+
+#define PLL_ARM_LOCK   (1 << 0)
+#define PLL_DDR_LOCK   (1 << 1)
+#define PLL_IO_LOCK(1 << 2)
+
 static struct clk *clks[clks_max];
 
 struct zynq_pll_clk {
struct clk  clk;
+   u32 pll_lock;
void __iomem*pll_ctrl;
 };
 
@@ -51,11 +62,19 @@ static unsigned long zynq_pll_recalc_rate(struct clk *clk,
return parent_rate * PLL_CTRL_FDIV(readl(pll->pll_ctrl));
 }
 
+static int zynq_pll_enable(struct clk *clk)
+{
+   return 0;
+}
+
 static struct clk_ops zynq_pll_clk_ops = {
.recalc_rate = zynq_pll_recalc_rate,
+   .enable = zynq_pll_enable,
 };
 
-static inline struct clk *zynq_pll_clk(const char *name, void __iomem 
*pll_ctrl)
+static inline struct clk *zynq_pll_clk(enum zynq_pll_type type,
+  const char *name,
+  void __iomem *pll_ctrl)
 {
static const char *pll_parent = "ps_clk";
struct zynq_pll_clk *pll;
@@ -68,6 +87,18 @@ static inline struct clk *zynq_pll_clk(const char *name, 
void __iomem *pll_ctrl)
pll->clk.parent_names   = &pll_parent;
pll->clk.num_parents= 1;
 
+   switch(type) {
+   case ZYNQ_PLL_ARM:
+   pll->pll_lock = PLL_ARM_LOCK;
+   break;
+   case ZYNQ_PLL_DDR:
+   pll->pll_lock = PLL_DDR_LOCK;
+   break;
+   case ZYNQ_PLL_IO:
+   pll->pll_lock = PLL_IO_LOCK;
+   break;
+   }
+
ret = clk_register(&pll->clk);
if (ret) {
free(pll);
-- 
1.8.2.rc2


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

Re: [PATCH 7/9] ARM: zynq: clk: add pll type

2013-03-11 Thread Josh Cartwright 
On Mon, Mar 11, 2013 at 10:15:04AM +0100, Steffen Trumtrar wrote:
> Signed-off-by: Steffen Trumtrar 
> ---
>  arch/arm/mach-zynq/clk-zynq7000.c | 33 -
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-zynq/clk-zynq7000.c 
> b/arch/arm/mach-zynq/clk-zynq7000.c
> index 5a8a12a..0d3c3a8 100644
> --- a/arch/arm/mach-zynq/clk-zynq7000.c
> +++ b/arch/arm/mach-zynq/clk-zynq7000.c
> @@ -33,10 +33,21 @@ enum zynq_clks {
>   cpu_clk, cpu_6x4x, cpu_3x2x, cpu_2x, cpu_1x, clks_max
>  };
>  
> +enum zynq_pll_type {
> + ZYNQ_PLL_ARM,
> + ZYNQ_PLL_DDR,
> + ZYNQ_PLL_IO,
> +};
> +
> +#define PLL_ARM_LOCK (1 << 0)
> +#define PLL_DDR_LOCK (1 << 1)
> +#define PLL_IO_LOCK  (1 << 2)

Having both an enum and the #define's seem like an unnecessary
indirection.  I'd suggest just:

enum zynq_pll_lockbit {
PLL_ARM_LOCK= (1 << 0),
PLL_DDR_LOCK= (1 << 1),
PLL_IO_LOCK = (1 << 2),
};

struct zynq_pll_clk {
/* ... */
enum zynq_pll_lockbit lockbit;
};

static inline struct clk *zynq_pll_clk(enum zynq_pll_lockbit lockbit,
   const char *name,
   void __iomem *pll_ctrl)
{
/* ... */
pll->lockbit = lockbit; 
/* ... */
}

> +
>  static struct clk *clks[clks_max];
>  
>  struct zynq_pll_clk {
>   struct clk  clk;
> + u32 pll_lock;
>   void __iomem*pll_ctrl;
>  };
>  
> @@ -51,11 +62,19 @@ static unsigned long zynq_pll_recalc_rate(struct clk *clk,
>   return parent_rate * PLL_CTRL_FDIV(readl(pll->pll_ctrl));
>  }
>  
> +static int zynq_pll_enable(struct clk *clk)
> +{
> + return 0;
> +}
> +
>  static struct clk_ops zynq_pll_clk_ops = {
>   .recalc_rate = zynq_pll_recalc_rate,
> + .enable = zynq_pll_enable,
>  };
>  
> -static inline struct clk *zynq_pll_clk(const char *name, void __iomem 
> *pll_ctrl)
> +static inline struct clk *zynq_pll_clk(enum zynq_pll_type type,
> +const char *name,
> +void __iomem *pll_ctrl)
>  {
>   static const char *pll_parent = "ps_clk";
>   struct zynq_pll_clk *pll;
> @@ -68,6 +87,18 @@ static inline struct clk *zynq_pll_clk(const char *name, 
> void __iomem *pll_ctrl)
>   pll->clk.parent_names   = &pll_parent;
>   pll->clk.num_parents= 1;
>  
> + switch(type) {
> + case ZYNQ_PLL_ARM:
> + pll->pll_lock = PLL_ARM_LOCK;
> + break;
> + case ZYNQ_PLL_DDR:
> + pll->pll_lock = PLL_DDR_LOCK;
> + break;
> + case ZYNQ_PLL_IO:
> + pll->pll_lock = PLL_IO_LOCK;

Actually, maybe I've gotten a little ahead of myself...you add bits for
the lock, but you never use it!  So, what's the point!  (If it's to be
used in the future, it'd be nice to see that in the commit description).

Josh

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

Re: [PATCH 7/9] ARM: zynq: clk: add pll type

2013-03-11 Thread Steffen Trumtrar 
On Mon, Mar 11, 2013 at 01:28:09PM -0500, Josh Cartwright wrote:
> On Mon, Mar 11, 2013 at 10:15:04AM +0100, Steffen Trumtrar wrote:
> > Signed-off-by: Steffen Trumtrar 
> > ---
> >  arch/arm/mach-zynq/clk-zynq7000.c | 33 -
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-zynq/clk-zynq7000.c 
> > b/arch/arm/mach-zynq/clk-zynq7000.c
> > index 5a8a12a..0d3c3a8 100644
> > --- a/arch/arm/mach-zynq/clk-zynq7000.c
> > +++ b/arch/arm/mach-zynq/clk-zynq7000.c
> > @@ -33,10 +33,21 @@ enum zynq_clks {
> > cpu_clk, cpu_6x4x, cpu_3x2x, cpu_2x, cpu_1x, clks_max
> >  };
> >  
> > +enum zynq_pll_type {
> > +   ZYNQ_PLL_ARM,
> > +   ZYNQ_PLL_DDR,
> > +   ZYNQ_PLL_IO,
> > +};
> > +
> > +#define PLL_ARM_LOCK   (1 << 0)
> > +#define PLL_DDR_LOCK   (1 << 1)
> > +#define PLL_IO_LOCK(1 << 2)
> 
> Having both an enum and the #define's seem like an unnecessary
> indirection.  I'd suggest just:
> 
> enum zynq_pll_lockbit {
>   PLL_ARM_LOCK= (1 << 0),
>   PLL_DDR_LOCK= (1 << 1),
>   PLL_IO_LOCK = (1 << 2),
> };
> 
> struct zynq_pll_clk {
>   /* ... */
>   enum zynq_pll_lockbit lockbit;
> };
> 
> static inline struct clk *zynq_pll_clk(enum zynq_pll_lockbit lockbit,
>  const char *name,
>  void __iomem *pll_ctrl)
> {
>   /* ... */
>   pll->lockbit = lockbit; 
>   /* ... */
> }
> 
> > +
> >  static struct clk *clks[clks_max];
> >  
> >  struct zynq_pll_clk {
> > struct clk  clk;
> > +   u32 pll_lock;
> > void __iomem*pll_ctrl;
> >  };
> >  
> > @@ -51,11 +62,19 @@ static unsigned long zynq_pll_recalc_rate(struct clk 
> > *clk,
> > return parent_rate * PLL_CTRL_FDIV(readl(pll->pll_ctrl));
> >  }
> >  
> > +static int zynq_pll_enable(struct clk *clk)
> > +{
> > +   return 0;
> > +}
> > +
> >  static struct clk_ops zynq_pll_clk_ops = {
> > .recalc_rate = zynq_pll_recalc_rate,
> > +   .enable = zynq_pll_enable,
> >  };
> >  
> > -static inline struct clk *zynq_pll_clk(const char *name, void __iomem 
> > *pll_ctrl)
> > +static inline struct clk *zynq_pll_clk(enum zynq_pll_type type,
> > +  const char *name,
> > +  void __iomem *pll_ctrl)
> >  {
> > static const char *pll_parent = "ps_clk";
> > struct zynq_pll_clk *pll;
> > @@ -68,6 +87,18 @@ static inline struct clk *zynq_pll_clk(const char *name, 
> > void __iomem *pll_ctrl)
> > pll->clk.parent_names   = &pll_parent;
> > pll->clk.num_parents= 1;
> >  
> > +   switch(type) {
> > +   case ZYNQ_PLL_ARM:
> > +   pll->pll_lock = PLL_ARM_LOCK;
> > +   break;
> > +   case ZYNQ_PLL_DDR:
> > +   pll->pll_lock = PLL_DDR_LOCK;
> > +   break;
> > +   case ZYNQ_PLL_IO:
> > +   pll->pll_lock = PLL_IO_LOCK;
> 
> Actually, maybe I've gotten a little ahead of myself...you add bits for
> the lock, but you never use it!  So, what's the point!  (If it's to be
> used in the future, it'd be nice to see that in the commit description).

I will remove this from this series. This only makes sense, when
the enable function is filled.

str

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

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