subject:"Re\: \[PATCH 1\/5\] clk\: berlin\: add support for berlin plls"

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Alexandre Belloni

On 21/03/2014 at 17:03:52 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 04:45 PM, Alexandre Belloni wrote:
> >[all commentis I agree on are snipped]
> 
> :)
> 
> >On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> >>On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> >>obj-y += pll.o
> >>obj-$(CONFIG_MACH_BERLIN_BG2)   += pll-berlin2.o
> >>obj-$(CONFIG_MACH_BERLIN_BG2CD) += pll-berlin2.o
> >>obj-$(CONFIG_MACH_BERLIN_BG2Q)  += pll-berlin2q.o
> >>
> >>Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
> >>arch/arm/mach-berlin/Kconfig. Can you spin a patch?
> >>
> >
> >I will do that.
> >
> >>>+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> >>>+
> >>>+static struct berlin_pllmap berlin_pll_map = {
> >>>+  .vcodiv = vcodiv_berlin2,
> >>>+  .fbdiv_mask = 0x1FF,
> >>>+  .rfdiv_mask = 0x1F,
> >>>+  .divsel_mask = 0xF,
> >>
> >>divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
> >>9, and common pll driver below does not know how many valid vcodiv
> >>values are passed. That can become dangerous..
> >>
> >>I'd rather extend vcodiv_berlin2 to full divsel range and provide
> >>safe (=1) divisiors. This way wrong/new register values will only
> >>break clock frequency derived.
> >>
> >
> >Good catch ! Then, what about simply shrinking the mask so that we don't
> >overflow the table. We'll put it back to its supposed real value whant
> >we know what are the remaining divisors (my guess is that they are already
> >all listed here). I would say that we are getting the divisor wrong if
> >divsel > 8 anyway.
> 
> Hmm, maybe I should look up valid vcodiv myself, but your vcodiv_berlin2
> has 9 values and I guess they are all valid, aren't they?
> 
> The next possible, larger mask where 0-8 fits in, is 0xf. You used that
> above and that reveals 16 possible indices.
> 
> The only option for shrinking the table that I see, would be min/max
> allowed indices, but that is as useful as having a slightly larger
> table.
> 

You are absolutely right :) I definitely need to take a break, right now !

> >>>+  .fbdiv_shift = 6,
> >>>+  .rfdiv_shift = 1,
> >>>+  .divsel_shift = 7,
> >>
> >>Have .foo_mask and .foo_shift together?
> >>
> >
> >This will make the struct larger but I don't really have an opinion.
> 
> Maybe, I wasn't clear enough. Just assign .foo_mask and .foo_shift in
> subsequent lines of code, i.e.
> 
> static struct berlin_pllmap berlin_pll_map = {
>   .vcodiv = vcodiv_berlin2,
>   .fbdiv_mask = 0x1FF,
>   .fbdiv_shift = 6,
>   .rfdiv_mask = 0x1F,
>   .rfdiv_shift = 1,
>   .divsel_mask = 0xF,
>   .divsel_shift = 7,
> };
> 

yeah, I figured that out a few minutes ago...

Thanks again for your reviews and patience !

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Sebastian Hesselbarth


On 03/21/2014 04:56 PM, Alexandre Belloni wrote:

On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a367a9831717..4a2602737c27 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
  obj-$(CONFIG_COMMON_CLK_WM831X)   += clk-wm831x.o
  obj-$(CONFIG_COMMON_CLK_XGENE)+= clk-xgene.o
  obj-$(CONFIG_COMMON_CLK_AT91) += at91/
+obj-$(CONFIG_ARCH_BERLIN)  += berlin/
  obj-$(CONFIG_ARCH_HI3xxx) += hisilicon/
  obj-$(CONFIG_COMMON_CLK_KEYSTONE) += keystone/


I'll have a look at clk/Makefile later myself, but alphabetic ordering
seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
add another violator.



BTW,  don't think there is any issue here, the Makefile states:
# please keep this section sorted lexicographically by file/directory path name


Ok, thanks for the clarification. As I said, I should have
looked it up myself in the first place.

Sebastian

--
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 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Alexandre Belloni

On 21/03/2014 at 16:45:39 +0100, Alexandre Belloni wrote :
> > >+  .fbdiv_shift = 6,
> > >+  .rfdiv_shift = 1,
> > >+  .divsel_shift = 7,
> > 
> > Have .foo_mask and .foo_shift together?
> > 
> 
> This will make the struct larger but I don't really have an opinion.
> 

Forget that one, I'll take a break...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Sebastian Hesselbarth


On 03/21/2014 04:45 PM, Alexandre Belloni wrote:

[all commentis I agree on are snipped]


:)


On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :

On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
obj-y += pll.o
obj-$(CONFIG_MACH_BERLIN_BG2)   += pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2CD) += pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2Q)  += pll-berlin2q.o

Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
arch/arm/mach-berlin/Kconfig. Can you spin a patch?



I will do that.


+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
+
+static struct berlin_pllmap berlin_pll_map = {
+   .vcodiv = vcodiv_berlin2,
+   .fbdiv_mask = 0x1FF,
+   .rfdiv_mask = 0x1F,
+   .divsel_mask = 0xF,


divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
9, and common pll driver below does not know how many valid vcodiv
values are passed. That can become dangerous..

I'd rather extend vcodiv_berlin2 to full divsel range and provide
safe (=1) divisiors. This way wrong/new register values will only
break clock frequency derived.



Good catch ! Then, what about simply shrinking the mask so that we don't
overflow the table. We'll put it back to its supposed real value whant
we know what are the remaining divisors (my guess is that they are already
all listed here). I would say that we are getting the divisor wrong if
divsel > 8 anyway.


Hmm, maybe I should look up valid vcodiv myself, but your vcodiv_berlin2
has 9 values and I guess they are all valid, aren't they?

The next possible, larger mask where 0-8 fits in, is 0xf. You used that
above and that reveals 16 possible indices.

The only option for shrinking the table that I see, would be min/max
allowed indices, but that is as useful as having a slightly larger
table.


+   .fbdiv_shift = 6,
+   .rfdiv_shift = 1,
+   .divsel_shift = 7,


Have .foo_mask and .foo_shift together?



This will make the struct larger but I don't really have an opinion.


Maybe, I wasn't clear enough. Just assign .foo_mask and .foo_shift in
subsequent lines of code, i.e.

static struct berlin_pllmap berlin_pll_map = {
.vcodiv = vcodiv_berlin2,
.fbdiv_mask = 0x1FF,
.fbdiv_shift = 6,
.rfdiv_mask = 0x1F,
.rfdiv_shift = 1,
.divsel_mask = 0xF,
.divsel_shift = 7,
};

Sebastian
--
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 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Alexandre Belloni

On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> >diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> >index a367a9831717..4a2602737c27 100644
> >--- a/drivers/clk/Makefile
> >+++ b/drivers/clk/Makefile
> >@@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500)  += clk-vt8500.o
> >  obj-$(CONFIG_COMMON_CLK_WM831X)+= clk-wm831x.o
> >  obj-$(CONFIG_COMMON_CLK_XGENE) += clk-xgene.o
> >  obj-$(CONFIG_COMMON_CLK_AT91)  += at91/
> >+obj-$(CONFIG_ARCH_BERLIN)   += berlin/
> >  obj-$(CONFIG_ARCH_HI3xxx)  += hisilicon/
> >  obj-$(CONFIG_COMMON_CLK_KEYSTONE)  += keystone/
> 
> I'll have a look at clk/Makefile later myself, but alphabetic ordering
> seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
> add another violator.
> 

BTW,  don't think there is any issue here, the Makefile states:
# please keep this section sorted lexicographically by file/directory path name



-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Alexandre Belloni

[all commentis I agree on are snipped]

On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> obj-y += pll.o
> obj-$(CONFIG_MACH_BERLIN_BG2) += pll-berlin2.o
> obj-$(CONFIG_MACH_BERLIN_BG2CD)   += pll-berlin2.o
> obj-$(CONFIG_MACH_BERLIN_BG2Q)+= pll-berlin2q.o
> 
> Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
> arch/arm/mach-berlin/Kconfig. Can you spin a patch?
> 

I will do that.

> >+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> >+
> >+static struct berlin_pllmap berlin_pll_map = {
> >+.vcodiv = vcodiv_berlin2,
> >+.fbdiv_mask = 0x1FF,
> >+.rfdiv_mask = 0x1F,
> >+.divsel_mask = 0xF,
> 
> divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
> 9, and common pll driver below does not know how many valid vcodiv
> values are passed. That can become dangerous..
> 
> I'd rather extend vcodiv_berlin2 to full divsel range and provide
> safe (=1) divisiors. This way wrong/new register values will only
> break clock frequency derived.
> 

Good catch ! Then, what about simply shrinking the mask so that we don't
overflow the table. We'll put it back to its supposed real value whant
we know what are the remaining divisors (my guess is that they are already
all listed here). I would say that we are getting the divisor wrong if
divsel > 8 anyway.

> >+.fbdiv_shift = 6,
> >+.rfdiv_shift = 1,
> >+.divsel_shift = 7,
> 
> Have .foo_mask and .foo_shift together?
> 

This will make the struct larger but I don't really have an opinion.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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 1/5] clk: berlin: add support for berlin plls

2014-03-21 Thread Sebastian Hesselbarth


On 03/21/2014 12:43 PM, Alexandre Belloni wrote:

This drivers allows to provide DT clocks for the cpu and system PLLs found on
Marvell Berlin SoCs.

Signed-off-by: Alexandre Belloni 
---
  drivers/clk/Makefile  |   1 +
  drivers/clk/berlin/Makefile   |   1 +
  drivers/clk/berlin/clk.h  |  35 +
  drivers/clk/berlin/pll-berlin2.c  |  41 
  drivers/clk/berlin/pll-berlin2q.c |  41 
  drivers/clk/berlin/pll.c  | 100 ++
  6 files changed, 219 insertions(+)
  create mode 100644 drivers/clk/berlin/Makefile
  create mode 100644 drivers/clk/berlin/clk.h
  create mode 100644 drivers/clk/berlin/pll-berlin2.c
  create mode 100644 drivers/clk/berlin/pll-berlin2q.c
  create mode 100644 drivers/clk/berlin/pll.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a367a9831717..4a2602737c27 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
  obj-$(CONFIG_COMMON_CLK_WM831X)   += clk-wm831x.o
  obj-$(CONFIG_COMMON_CLK_XGENE)+= clk-xgene.o
  obj-$(CONFIG_COMMON_CLK_AT91) += at91/
+obj-$(CONFIG_ARCH_BERLIN)  += berlin/
  obj-$(CONFIG_ARCH_HI3xxx) += hisilicon/
  obj-$(CONFIG_COMMON_CLK_KEYSTONE) += keystone/


I'll have a look at clk/Makefile later myself, but alphabetic ordering
seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
add another violator.


  ifeq ($(CONFIG_COMMON_CLK), y)
diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
new file mode 100644
index ..4f6580e2a3ee
--- /dev/null
+++ b/drivers/clk/berlin/Makefile
@@ -0,0 +1 @@
+obj-y += pll.o pll-berlin2.o pll-berlin2q.o


obj-y += pll.o
obj-$(CONFIG_MACH_BERLIN_BG2)   += pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2CD) += pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2Q)  += pll-berlin2q.o

Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
arch/arm/mach-berlin/Kconfig. Can you spin a patch?


diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h


I prefer to have this include named "common.h" as I guess we will
dump all common declarations in here.


new file mode 100644
index ..41459db31a02
--- /dev/null
+++ b/drivers/clk/berlin/clk.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2013 Marvell Technology Group Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+#ifndef __BERLIN_CLK_H
+#define __BERLIN_CLK_H
+
+#include 


What is this include good for in here?

You will need struct device_node declared, but a simple

struct device_node;

should be sufficient.


+struct berlin_pllmap {
+   const u8*vcodiv;
+   u32 fbdiv_mask;
+   u32 rfdiv_mask;
+   u32 divsel_mask;
+   u8  fbdiv_shift;
+   u8  rfdiv_shift;
+   u8  divsel_shift;
+   u8  mult;
+};
+
+extern void __init berlin_pll_setup(struct device_node *np,
+   struct berlin_pllmap *map);
+
+#endif /* BERLIN_CLK_H */
diff --git a/drivers/clk/berlin/pll-berlin2.c b/drivers/clk/berlin/pll-berlin2.c
new file mode 100644
index ..5f5b4674f811
--- /dev/null
+++ b/drivers/clk/berlin/pll-berlin2.c
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2014 Marvell Technology Group Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+#include 
+#include 
+#include 
+#include 


Please sort includes alphabetically, it will save us some pain
in the future.


+#include "clk.h"


#include "common.h"


+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
+
+static struct berlin_pllmap berlin_pll_map = {
+   .vcodiv = vcodiv_berlin2,
+   .fbdiv_mask = 0x1FF,
+   .rfdiv_mask = 0x1F,
+   .divsel_mask = 0x

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

Re: [PATCH 1/5] clk: berlin: add support for berlin plls

7 matches

Site Navigation

Mail list logo

Footer information