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

[Scott Wood <scottwood@freescale.com>]

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?

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

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?

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

-Scott