Re: [PATCH 6/8] [POWERPC] sysdev,qe_lib: implement FSL GTM support

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Tue, Mar 18, 2008 at 02:55:14PM -0500, Scott Wood wrote:
> Anton Vorontsov wrote:
> >>>+arch_initcall(qe_init_gtm);
> >>If this happens before the gtm_init_gtm(),
> >
> >"If" isn't possible, order is guaranteed.
> 
> You use arch_initcall for both, so you're relying on link order.  I 
> think this at least merits a comment.
> >>then np->data will not be set. 
> >
> >It's a bug in the device tree or in the Linux code then.
> 
> Hmm?  It's set by gtm_init_gtm().  If this code runs before 
> gtm_init_gtm(), what are you expecting to initialize np->data?

What code exactly?

> >>If this happens after gtm_init_gtm(), then gtm_init_gtm() will fail in
> >>gtm_get_clock(), if there's no clock-frequency -- and if there is, then 
> >>why
> >>do we need qe_init_gtm() at all?
> >
> >Because for the QE clock-frequency != brg-frequency.
> 
> Sorry, I missed that you were getting clock-frequency from the parent, 
> rather than the gtm node.  If you do the latter, then you can just stick 
> the relevant frequency in the gtm node and not worry about where it 
> comes from.  This would be analogous to how UART clocks are specified.

Ok.

> Also, what if some arch_initcall runs between gtm_init_gtm and 
> qe_init_gtm, that registers itself as a client of the gtm driver, and 
> uses the wrong clock value?

Again, what code exactly? If it is a driver (for what this API is
created for), it hardly will run earlier than arch/ code. If this is
platform code (arch/powerpc/platform/), then it is hardly will run
earlier than arch/sysdev/. Inside the arch/sysdev/ fsl_gtm.c is
guaranteed to run earlier than qe_lib/gtm.c. So, where is the problem?

Since I'll implement clock-frequency inside the timer node, this
isn't relevant anymore...

> >>>+extern struct gtm_timer *gtm_get_timer(int width);
> >>To support using the GTM as a wakeup from deep sleep on 831x (which I've 
> >>had
> >>a patch pending for quite a while now), we'll need some way of reserving a
> >>specific timer (only GTM1, timer 4 is supported).
> >
> >You can add reserve function either in the PM driver (if any), or
> 
> What I meant was that there needs to be some way of telling this driver 
> not to hand the reserved timer out to some other client.
> 
> >you can do something in the device tree (wakeup-timer = <..>). I don't
> >see any problems if you want to implement it.
> 
> How about simply having optional arguments to gtm_get_timer() to specify 
> the GTM device and timer number, which will fail if it's already in use? 
>  Then, the PM driver simply needs to run early enough to grab the timer 
> it needs.

Ah. You need specific timer. No problem. I don't like idea of new arguments
to the gtm_get_timer() function (complicates things), but we can just
implement another one. gtm_get_timer_<name>, choice the name please.
_specific, _2, _for, __gtm_get_timer, ...

-- 
Anton Vorontsov
email: cboumailru@gmail.com
irc://irc.freenode.net/bd2