Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Mon, 2014-03-24 at 23:42 +0100, Olliver Schinagl wrote: On 03/24/2014 09:52 PM, Marek Vasut wrote: + /* Switch to 24MHz clock while changing PLL1 */ + writel(AXI_DIV_1 AXI_DIV_SHIFT | + AHB_DIV_2 AHB_DIV_SHIFT | + APB0_DIV_1 APB0_DIV_SHIFT | + CPU_CLK_SRC_OSC24M CPU_CLK_SRC_SHIFT, + ccm-cpu_ahb_apb0_cfg); + sdelay(20); What is sdelay() function all about ? It also is from arch/arm/include/asm/arch-sunxi/sys_proto.h and I thought all where replaced with udelays Since these sdelays() are used while we are frobbing around with the clocks I'm not sure that switching to udelay is possible/wise. sdelay is documented as: * sdelay() - simple spin loop. Will be constant time as * its generally used in bypass conditions only. This * is necessary until timers are accessible. IOW it sounds like it is designed to be used in exactly these circumstances. Given the lack of documentation for what is actually required by the hardware when changing these clocks I'm a bit reluctant to go changing things. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Sunday, April 13, 2014 at 09:55:57 PM, Ian Campbell wrote: On Mon, 2014-03-24 at 23:42 +0100, Olliver Schinagl wrote: On 03/24/2014 09:52 PM, Marek Vasut wrote: +/* Switch to 24MHz clock while changing PLL1 */ +writel(AXI_DIV_1 AXI_DIV_SHIFT | + AHB_DIV_2 AHB_DIV_SHIFT | + APB0_DIV_1 APB0_DIV_SHIFT | + CPU_CLK_SRC_OSC24M CPU_CLK_SRC_SHIFT, + ccm-cpu_ahb_apb0_cfg); +sdelay(20); What is sdelay() function all about ? It also is from arch/arm/include/asm/arch-sunxi/sys_proto.h and I thought all where replaced with udelays Since these sdelays() are used while we are frobbing around with the clocks I'm not sure that switching to udelay is possible/wise. sdelay is documented as: * sdelay() - simple spin loop. Will be constant time as * its generally used in bypass conditions only. This * is necessary until timers are accessible. IOW it sounds like it is designed to be used in exactly these circumstances. Given the lack of documentation for what is actually required by the hardware when changing these clocks I'm a bit reluctant to go changing things. Don't you have an clock-agnostic timer in the chip ? If not, well, can't be helped. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Thu, 2014-03-27 at 23:36 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg
*)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; +
+/* macro to read the 32 bit timer: since it decrements, we invert
read value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do
you use that negation in ~readl() anyway ?
The comment right above it explains why: the timer counts backwards and
inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in the
3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this
READ_TIMER() call and getting hit by timer_base undefined . That
macro hides the dependency on this symbol, while if you expanded it
in-place, the dependency would be explicit. I really do want to see that
macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a
body:
{
struct sunxi_timer *timers =
(struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
return timers[TIMER_NUM]-val;
}
With something similar in timer_init then both the macro and the static
global timer_base can be dropped.
That's just wrapping a readl() into another function, which seems unnecessary
really.
Sorry, but I think inlining the readl (and along with it the
interesting/subtle) inverting functionality is a terrible idea, it
should be wrapped in some sort of accessor precisely because it is not a
raw readl.
I'm going to make it a function as I suggested.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
I'm not sure which implementers down the line you were worried about
using it in some other context where it breaks.
People plumbing in the timer.c file who are not aware the macro has a
dependency
which is not passed as it's parameter.
What people? What plumbing? I've no idea what you are concerned about
here.
Ian.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
Hi,
On 03/28/2014 09:20 AM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:36 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg
*)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; +
+/* macro to read the 32 bit timer: since it decrements, we invert
read value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do
you use that negation in ~readl() anyway ?
The comment right above it explains why: the timer counts backwards and
inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in the
3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this
READ_TIMER() call and getting hit by timer_base undefined . That
macro hides the dependency on this symbol, while if you expanded it
in-place, the dependency would be explicit. I really do want to see that
macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a
body:
{
struct sunxi_timer *timers =
(struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
return timers[TIMER_NUM]-val;
}
With something similar in timer_init then both the macro and the static
global timer_base can be dropped.
That's just wrapping a readl() into another function, which seems
unnecessary
really.
Sorry, but I think inlining the readl (and along with it the
interesting/subtle) inverting functionality is a terrible idea, it
should be wrapped in some sort of accessor precisely because it is not a
raw readl.
I'm going to make it a function as I suggested.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
I'm not sure which implementers down the line you were worried about
using it in some other context where it breaks.
People plumbing in the timer.c file who are not aware the macro has a
dependency
which is not passed as it's parameter.
What people? What plumbing? I've no idea what you are concerned about
here.
I think what Marek is concerned about is people making some global u-boot change
which for some reason requires fixing up a bunch of platform specific files,
and they end up touching our timer.c without ever test-compiling it.
These kind of things happen in qemu / the kernel too. In this case they could
move
a READ_TIMER() somewhere where the timer_base is not defined. Your suggestion of
making it a proper function will fix that though, and I think is the best
solution.
Regards,
Hans
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Friday, March 28, 2014 at 09:20:17 AM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:36 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg
*)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; +
+/* macro to read the 32 bit timer: since it decrements, we
invert read value */ +#define READ_TIMER()
(~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why
do you use that negation in ~readl() anyway ?
The comment right above it explains why: the timer counts backwards
and inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in
the 3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this
READ_TIMER() call and getting hit by timer_base undefined . That
macro hides the dependency on this symbol, while if you expanded it
in-place, the dependency would be explicit. I really do want to see
that macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a
body:
{
struct sunxi_timer *timers =
(struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
return timers[TIMER_NUM]-val;
}
With something similar in timer_init then both the macro and the static
global timer_base can be dropped.
That's just wrapping a readl() into another function, which seems
unnecessary really.
Sorry, but I think inlining the readl (and along with it the
interesting/subtle) inverting functionality is a terrible idea, it
should be wrapped in some sort of accessor precisely because it is not a
raw readl.
I'm going to make it a function as I suggested.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
I'm not sure which implementers down the line you were worried about
using it in some other context where it breaks.
People plumbing in the timer.c file who are not aware the macro has a
dependency which is not passed as it's parameter.
What people? What plumbing? I've no idea what you are concerned about
here.
OK, I will wait for V3 of the patch since this discussion have gone awry .
Let's
talk about V3 , ok ? :)
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Friday, March 28, 2014 at 09:25:40 AM, Hans de Goede wrote:
Hi,
On 03/28/2014 09:20 AM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:36 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg
*)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; +
+/* macro to read the 32 bit timer: since it decrements, we invert
read value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do
you use that negation in ~readl() anyway ?
The comment right above it explains why: the timer counts backwards
and inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in
the 3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this
READ_TIMER() call and getting hit by timer_base undefined . That
macro hides the dependency on this symbol, while if you expanded it
in-place, the dependency would be explicit. I really do want to see
that macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a
body:
{
struct sunxi_timer *timers =
(struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
return timers[TIMER_NUM]-val;
}
With something similar in timer_init then both the macro and the static
global timer_base can be dropped.
That's just wrapping a readl() into another function, which seems
unnecessary really.
Sorry, but I think inlining the readl (and along with it the
interesting/subtle) inverting functionality is a terrible idea, it
should be wrapped in some sort of accessor precisely because it is not a
raw readl.
I'm going to make it a function as I suggested.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
I'm not sure which implementers down the line you were worried about
using it in some other context where it breaks.
People plumbing in the timer.c file who are not aware the macro has a
dependency which is not passed as it's parameter.
What people? What plumbing? I've no idea what you are concerned about
here.
I think what Marek is concerned about is people making some global u-boot
change which for some reason requires fixing up a bunch of platform
specific files, and they end up touching our timer.c without ever
test-compiling it.
We don't do such changes without test-compiling those (most of us don't and
those who do ... well ... tsk tsk tsk ! ;-) ).
These kind of things happen in qemu / the kernel too. In this case they
could move a READ_TIMER() somewhere where the timer_base is not defined.
Yeah.
Your suggestion of making it a proper function will fix that though, and I
think is the best solution.
I think wrapping a plain readl() into a function is not necessary, but I will
wait for V3 to see it in action and then I will decide .
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote: +static struct sunxi_timer *timer_base = + ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; + +/* macro to read the 32 bit timer: since it decrements, we invert read value */ +#define READ_TIMER() (~readl(timer_base-val)) This macro has to go, just use ~readl() in place. But still, why do you use that negation in ~readl() anyway ? The comment right above it explains why: the timer counts backwards and inverting it accounts for that. This is subtle enough that I don't think using ~readl() in place in the 3 callers would be an improvement. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote: On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote: +static struct sunxi_timer *timer_base = + ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; + +/* macro to read the 32 bit timer: since it decrements, we invert read value */ +#define READ_TIMER() (~readl(timer_base-val)) This macro has to go, just use ~readl() in place. But still, why do you use that negation in ~readl() anyway ? The comment right above it explains why: the timer counts backwards and inverting it accounts for that. This is subtle enough that I don't think using ~readl() in place in the 3 callers would be an improvement. Please do it, we don't want any implementers down the line using this READ_TIMER() call and getting hit by timer_base undefined . That macro hides the dependency on this symbol, while if you expanded it in-place, the dependency would be explicit. I really do want to see that macro gone, sorry. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)-timer[TIMER_NUM];
+
+/* macro to read the 32 bit timer: since it decrements, we invert read
value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do you
use that negation in ~readl() anyway ?
The comment right above it explains why: the timer counts backwards and
inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in the
3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this
READ_TIMER() call and getting hit by timer_base undefined . That macro
hides
the dependency on this symbol, while if you expanded it in-place, the
dependency
would be explicit. I really do want to see that macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a
body:
{
struct sunxi_timer *timers =
(struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
return timers[TIMER_NUM]-val;
}
With something similar in timer_init then both the macro and the static
global timer_base can be dropped.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
I'm not sure which implementers down the line you were worried about
using it in some other context where it breaks.
Ian.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg
*)SUNXI_TIMER_BASE)-timer[TIMER_NUM]; +
+/* macro to read the 32 bit timer: since it decrements, we invert
read value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do
you use that negation in ~readl() anyway ?
The comment right above it explains why: the timer counts backwards and
inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in the
3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this
READ_TIMER() call and getting hit by timer_base undefined . That
macro hides the dependency on this symbol, while if you expanded it
in-place, the dependency would be explicit. I really do want to see that
macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a
body:
{
struct sunxi_timer *timers =
(struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
return timers[TIMER_NUM]-val;
}
With something similar in timer_init then both the macro and the static
global timer_base can be dropped.
That's just wrapping a readl() into another function, which seems unnecessary
really.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so
I'm not sure which implementers down the line you were worried about
using it in some other context where it breaks.
People plumbing in the timer.c file who are not aware the macro has a
dependency
which is not passed as it's parameter.
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Mon, 2014-03-24 at 23:42 +0100, Olliver Schinagl wrote: [...] I've got a local cleanup patch set where I fixed this already to clrsetbits_le32 [...] Same here, got that in my local tree too Could you post what you've got please? +#ifdef CONFIG_SPL_BUILD +#define PLL1_CFG(N, K, M, P) (1 31 | 0 30 | 8 26 | 0 25 | \ + 16 20 | (P) 16 | 2 13 | (N) 8 | \ + (K) 4 | 0 3 | 0 2 | (M) 0) Here is well. dito :) +#define RDIV(a, b)((a + (b) - 1) / (b)) This is some kind of DIV_ROUND_UP() from include/common.h ? [...] That one i didn't have; Ian, I guess you can verify that generic macro works for here? Yeah, I'll look into that and all the other feedback from Marek. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote: On Friday, March 21, 2014 at 10:54:18 PM, Ian Campbell wrote: This has been stripped back for mainlining and supports only sun7i. These changes are not useful by themselves but are split out to make the patch sizes more manageable. [...] Thanks for all your feedback here and on the other patches, I'll sort it all out. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
Dear Olliver Schinagl, In message 5330b4c9.10...@schinagl.nl you wrote: sr32() is not defined anywhere. it should be defined in arch/arm/include/asm/arch-sunxi/sys_proto.h and comes from arch/arm/cpu/armv7/syslib.c Please avoid using sr32() alltogether. It is basically only used in a single file (arch/arm/cpu/armv7/omap3/clock.c) [plus a single call in board/ti/panda/panda.c] and should be replaced by standard I/O accessor functions. I will prepare a patch to remove it from mainline soon, so better don't start using it. Thanks. Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A metaphor is like a simile. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Friday, March 21, 2014 at 10:54:18 PM, Ian Campbell wrote:
This has been stripped back for mainlining and supports only sun7i. These
changes are not useful by themselves but are split out to make the patch
sizes more manageable.
[...]
+int clock_init(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_SPL_BUILD
+ clock_init_safe();
+#endif
+
+ /* uart clock source is apb1 */
+ sr32(ccm-apb1_clk_div_cfg, 24, 2, APB1_CLK_SRC_OSC24M);
+ sr32(ccm-apb1_clk_div_cfg, 16, 2, APB1_FACTOR_N);
+ sr32(ccm-apb1_clk_div_cfg, 0, 5, APB1_FACTOR_M);
sr32() is not defined anywhere.
+ /* open the clock for uart */
+ sr32(ccm-apb1_gate, 16 + CONFIG_CONS_INDEX - 1, 1, CLK_GATE_OPEN);
+
+ return 0;
+}
+
+/* Return PLL5 frequency in Hz
+ * Note: Assumes PLL5 reference is 24MHz clock
+ */
+unsigned int clock_get_pll5(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+ uint32_t rval = readl(ccm-pll5_cfg);
+ int n = (rval 8) 0x1f;
+ int k = ((rval 4) 3) + 1;
+ int p = 1 ((rval 16) 3);
+ return 2400 * n * k / p;
Please fix the magic values here.
[...]
+#ifdef CONFIG_SPL_BUILD
+#define PLL1_CFG(N, K, M, P) (1 31 | 0 30 | 8 26 | 0 25 |
\
+ 16 20 | (P) 16 | 2 13 | (N) 8 | \
+ (K) 4 | 0 3 | 0 2 | (M) 0)
Here is well.
+#define RDIV(a, b) ((a + (b) - 1) / (b))
This is some kind of DIV_ROUND_UP() from include/common.h ?
[...]
+ /* Map divisors to register values */
+ axi = axi - 1;
+ if (ahb 4)
+ ahb = 3;
+ else if (ahb 2)
+ ahb = 2;
+ else if (ahb 1)
+ ahb = 1;
+ else
+ ahb = 0;
+
+ apb0 = apb0 - 1;
+
+ /* Switch to 24MHz clock while changing PLL1 */
+ writel(AXI_DIV_1 AXI_DIV_SHIFT |
+AHB_DIV_2 AHB_DIV_SHIFT |
+APB0_DIV_1 APB0_DIV_SHIFT |
+CPU_CLK_SRC_OSC24M CPU_CLK_SRC_SHIFT,
+ccm-cpu_ahb_apb0_cfg);
+ sdelay(20);
What is sdelay() function all about ?
[...]
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)-timer[TIMER_NUM];
+
+/* macro to read the 32 bit timer: since it decrements, we invert read
value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do you use
that
negation in ~readl() anyway ?
[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On Monday, March 24, 2014 at 11:42:17 PM, Olliver Schinagl wrote:
On 03/24/2014 09:52 PM, Marek Vasut wrote:
On Friday, March 21, 2014 at 10:54:18 PM, Ian Campbell wrote:
This has been stripped back for mainlining and supports only sun7i.
These changes are not useful by themselves but are split out to make
the patch sizes more manageable.
[...]
+int clock_init(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_SPL_BUILD
+ clock_init_safe();
+#endif
+
+ /* uart clock source is apb1 */
+ sr32(ccm-apb1_clk_div_cfg, 24, 2, APB1_CLK_SRC_OSC24M);
+ sr32(ccm-apb1_clk_div_cfg, 16, 2, APB1_FACTOR_N);
+ sr32(ccm-apb1_clk_div_cfg, 0, 5, APB1_FACTOR_M);
sr32() is not defined anywhere.
it should be defined in
arch/arm/include/asm/arch-sunxi/sys_proto.h
and comes from
arch/arm/cpu/armv7/syslib.c
it was added for the ti omap's
I've got a local cleanup patch set where I fixed this already to
clrsetbits_le32
It's not part of this patch, but then, use clrsetbits_le32() instead of course.
+ /* open the clock for uart */
+ sr32(ccm-apb1_gate, 16 + CONFIG_CONS_INDEX - 1, 1, CLK_GATE_OPEN);
+
+ return 0;
+}
+
+/* Return PLL5 frequency in Hz
+ * Note: Assumes PLL5 reference is 24MHz clock
+ */
+unsigned int clock_get_pll5(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+ uint32_t rval = readl(ccm-pll5_cfg);
+ int n = (rval 8) 0x1f;
+ int k = ((rval 4) 3) + 1;
+ int p = 1 ((rval 16) 3);
+ return 2400 * n * k / p;
Please fix the magic values here.
[...]
Same here, got that in my local tree too
Then make it part of the V3 please.
[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
On 03/24/2014 09:52 PM, Marek Vasut wrote:
On Friday, March 21, 2014 at 10:54:18 PM, Ian Campbell wrote:
This has been stripped back for mainlining and supports only sun7i. These
changes are not useful by themselves but are split out to make the patch
sizes more manageable.
[...]
+int clock_init(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_SPL_BUILD
+ clock_init_safe();
+#endif
+
+ /* uart clock source is apb1 */
+ sr32(ccm-apb1_clk_div_cfg, 24, 2, APB1_CLK_SRC_OSC24M);
+ sr32(ccm-apb1_clk_div_cfg, 16, 2, APB1_FACTOR_N);
+ sr32(ccm-apb1_clk_div_cfg, 0, 5, APB1_FACTOR_M);
sr32() is not defined anywhere.
it should be defined in
arch/arm/include/asm/arch-sunxi/sys_proto.h
and comes from
arch/arm/cpu/armv7/syslib.c
it was added for the ti omap's
I've got a local cleanup patch set where I fixed this already to
clrsetbits_le32
+ /* open the clock for uart */
+ sr32(ccm-apb1_gate, 16 + CONFIG_CONS_INDEX - 1, 1, CLK_GATE_OPEN);
+
+ return 0;
+}
+
+/* Return PLL5 frequency in Hz
+ * Note: Assumes PLL5 reference is 24MHz clock
+ */
+unsigned int clock_get_pll5(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+ uint32_t rval = readl(ccm-pll5_cfg);
+ int n = (rval 8) 0x1f;
+ int k = ((rval 4) 3) + 1;
+ int p = 1 ((rval 16) 3);
+ return 2400 * n * k / p;
Please fix the magic values here.
[...]
Same here, got that in my local tree too
+#ifdef CONFIG_SPL_BUILD
+#define PLL1_CFG(N, K, M, P) (1 31 | 0 30 | 8 26 | 0 25 |
\
+16 20 | (P) 16 | 2 13 | (N) 8 | \
+(K) 4 | 0 3 | 0 2 | (M) 0)
Here is well.
dito :)
+#define RDIV(a, b) ((a + (b) - 1) / (b))
This is some kind of DIV_ROUND_UP() from include/common.h ?
[...]
That one i didn't have;
Ian, I guess you can verify that generic macro works for here?
+ /* Map divisors to register values */
+ axi = axi - 1;
+ if (ahb 4)
+ ahb = 3;
+ else if (ahb 2)
+ ahb = 2;
+ else if (ahb 1)
+ ahb = 1;
+ else
+ ahb = 0;
+
+ apb0 = apb0 - 1;
+
+ /* Switch to 24MHz clock while changing PLL1 */
+ writel(AXI_DIV_1 AXI_DIV_SHIFT |
+ AHB_DIV_2 AHB_DIV_SHIFT |
+ APB0_DIV_1 APB0_DIV_SHIFT |
+ CPU_CLK_SRC_OSC24M CPU_CLK_SRC_SHIFT,
+ ccm-cpu_ahb_apb0_cfg);
+ sdelay(20);
What is sdelay() function all about ?
It also is from
arch/arm/include/asm/arch-sunxi/sys_proto.h
and I thought all where replaced with udelays
With the sr32 and sdelays gone everywhere, sunxi/sys_proto.h can even go!
[...]
+static struct sunxi_timer *timer_base =
+ ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)-timer[TIMER_NUM];
+
+/* macro to read the 32 bit timer: since it decrements, we invert read
value */ +#define READ_TIMER() (~readl(timer_base-val))
This macro has to go, just use ~readl() in place. But still, why do you use that
negation in ~readl() anyway ?
[...]
Olliver
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.
This has been stripped back for mainlining and supports only sun7i. These
changes are not useful by themselves but are split out to make the patch sizes
more manageable.
As well as the following signed-off-by the sunxi branch shows commits to these
files authored by the following:
Alejandro Mery
Carl van Schaik
Stefan Roese
Tom Cubie
yemao
Signed-off-by: Alexandru Gagniuc mr.nuke...@gmail.com
Signed-off-by: Chen-Yu Tsai w...@csie.org
Signed-off-by: Emilio López emi...@elopez.com.ar
Signed-off-by: Hans de Goede hdego...@redhat.com
Signed-off-by: Henrik Nordstrom hen...@henriknordstrom.net
Signed-off-by: Jens Kuske jensku...@gmail.com
Signed-off-by: Luke Leighton l...@lkcl.net
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
Signed-off-by: Ian Campbell i...@hellion.org.uk
---
v2: Based on u-boot-sunxi.git#sunxi d9aa5dd3d15c sunxi: mmc:
checkpatch whitespace fixes with v2014.04-rc2 merged in:
- define magic numbers
- simplify get_tbclk
- correct clock_set_pll1 prototype
- add CONFIG_SUN7I to simplify future SUN?I support.
- defines for MMC AHB clocks
v1: Based on u-boot-sunxi.git#sunxi commit d854c4de2f57 arm: Handle .gnu.hash
section in ldscripts vs v2014.01.
---
arch/arm/cpu/armv7/sunxi/Makefile | 11 ++
arch/arm/cpu/armv7/sunxi/clock.c| 180 +
arch/arm/cpu/armv7/sunxi/timer.c| 102
arch/arm/include/asm/arch-sunxi/clock.h | 237
arch/arm/include/asm/arch-sunxi/sys_proto.h | 17 ++
arch/arm/include/asm/arch-sunxi/timer.h | 88 +++
6 files changed, 635 insertions(+)
create mode 100644 arch/arm/cpu/armv7/sunxi/Makefile
create mode 100644 arch/arm/cpu/armv7/sunxi/clock.c
create mode 100644 arch/arm/cpu/armv7/sunxi/timer.c
create mode 100644 arch/arm/include/asm/arch-sunxi/clock.h
create mode 100644 arch/arm/include/asm/arch-sunxi/sys_proto.h
create mode 100644 arch/arm/include/asm/arch-sunxi/timer.h
diff --git a/arch/arm/cpu/armv7/sunxi/Makefile
b/arch/arm/cpu/armv7/sunxi/Makefile
new file mode 100644
index 000..787a127
--- /dev/null
+++ b/arch/arm/cpu/armv7/sunxi/Makefile
@@ -0,0 +1,11 @@
+#
+# (C) Copyright 2012 Henrik Nordstrom hen...@henriknordstrom.net
+#
+# Based on some other Makefile
+# (C) Copyright 2000-2003
+# Wolfgang Denk, DENX Software Engineering, w...@denx.de.
+#
+# SPDX-License-Identifier: GPL-2.0+
+#
+obj-y += timer.o
+obj-y += clock.o
diff --git a/arch/arm/cpu/armv7/sunxi/clock.c b/arch/arm/cpu/armv7/sunxi/clock.c
new file mode 100644
index 000..dd01be6
--- /dev/null
+++ b/arch/arm/cpu/armv7/sunxi/clock.c
@@ -0,0 +1,180 @@
+/*
+ * (C) Copyright 2007-2012
+ * Allwinner Technology Co., Ltd. www.allwinnertech.com
+ * Tom Cubie tangli...@allwinnertech.com
+ *
+ * (C) Copyright 2013 Luke Kenneth Casson Leighton l...@lkcl.net
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include common.h
+#include asm/io.h
+#include asm/arch/clock.h
+#include asm/arch/gpio.h
+#include asm/arch/sys_proto.h
+
+#ifdef CONFIG_SPL_BUILD
+static void clock_init_safe(void)
+{
+ struct sunxi_ccm_reg * const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+ /* Set safe defaults until PMU is configured */
+ writel(AXI_DIV_1 AXI_DIV_SHIFT |
+ AHB_DIV_2 AHB_DIV_SHIFT |
+ APB0_DIV_1 APB0_DIV_SHIFT |
+ CPU_CLK_SRC_OSC24M CPU_CLK_SRC_SHIFT,
+ ccm-cpu_ahb_apb0_cfg);
+ writel(PLL1_CFG_DEFAULT, ccm-pll1_cfg);
+ sdelay(200);
+ writel(AXI_DIV_1 AXI_DIV_SHIFT |
+ AHB_DIV_2 AHB_DIV_SHIFT |
+ APB0_DIV_1 APB0_DIV_SHIFT |
+ CPU_CLK_SRC_PLL1 CPU_CLK_SRC_SHIFT,
+ ccm-cpu_ahb_apb0_cfg);
+#ifdef CONFIG_SUN7I
+ writel(0x1 AHB_GATE_OFFSET_DMA | readl(ccm-ahb_gate0),
+ ccm-ahb_gate0);
+ writel(0x1 PLL6_ENABLE_OFFSET | readl(ccm-pll6_cfg),
+ ccm-pll6_cfg);
+#endif
+}
+#endif
+
+int clock_init(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_SPL_BUILD
+ clock_init_safe();
+#endif
+
+ /* uart clock source is apb1 */
+ sr32(ccm-apb1_clk_div_cfg, 24, 2, APB1_CLK_SRC_OSC24M);
+ sr32(ccm-apb1_clk_div_cfg, 16, 2, APB1_FACTOR_N);
+ sr32(ccm-apb1_clk_div_cfg, 0, 5, APB1_FACTOR_M);
+
+ /* open the clock for uart */
+ sr32(ccm-apb1_gate, 16 + CONFIG_CONS_INDEX - 1, 1, CLK_GATE_OPEN);
+
+ return 0;
+}
+
+/* Return PLL5 frequency in Hz
+ * Note: Assumes PLL5 reference is 24MHz clock
+ */
+unsigned int clock_get_pll5(void)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+ uint32_t rval = readl(ccm-pll5_cfg);
+ int n = (rval 8) 0x1f;
+ int k = ((rval 4) 3) + 1;
+ int p = 1 ((rval 16) 3);
+ return 2400 * n * k / p;
+}
+
+int clock_twi_onoff(int port, int state)
+{
+
