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 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 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 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.
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 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.
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 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 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 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 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.
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.
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 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
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 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
[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
Signed-off-by: Chen-Yu Tsai
Signed-off-by: Emilio López
Signed-off-by: Hans de Goede
Signed-off-by: Henrik Nordstrom
Signed-off-by: Jens Kuske
Signed-off-by: Luke Leighton
Signed-off-by: Oliver Schinagl
Signed-off-by: Ian Campbell
---
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
+#
+# 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.
+ * Tom Cubie
+ *
+ * (C) Copyright 2013 Luke Kenneth Casson Leighton
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#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)
+{
+ struct sunxi_ccm_reg *const ccm =
+ (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+ if (port > 2)
+ return -1;
+
+ /* set the apb1 clock gate for twi */
+ sr32(&ccm->apb1_gate, 0 + port, 1, state);
+
+ return 0;
+}
