On Tue, May 20, 2008 at 08:15:15AM -0500, Kumar Gala wrote:
[...]
>>>> + for_each_compatible_node(np, NULL, "fsl,gtm") {
>>>> + int i;
>>>> + struct gtm *gtm;
>>>> + const u32 *clock;
>>>> + int size;
>>>> +
>>>> + gtm = kzalloc(sizeof(*gtm), GFP_KERNEL);
>>>> + if (!gtm) {
>>>> + pr_err("%s: unable to allocate memory\n",
>>>> + np->full_name);
>>>> + continue;
>>>> + }
>>>
>>> why bother with making this a dynamic alloc?
>>
>> Because different platforms have different number of GTMs
>> blocks. For QE machines this could be up to three GTMs, and QE-less
>> usually implement two GTMs. Not sure about CPM2.
>
> ok, that makes sense.
>
>>>> +
>>>> + spin_lock_init(>m->lock);
>>>> +
>>>> + clock = of_get_property(np, "clock-frequency", &size);
>>>> + if (!clock || size != sizeof(*clock)) {
>>>> + pr_err("%s: no clock-frequency\n", np->full_name);
>>>> + goto err;
>>>> + }
>>>> + gtm->clock = *clock;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(gtm->timers); i++) {
>>>> + int ret;
>>>> + struct resource irq;
>>>> +
>>>> + ret = of_irq_to_resource(np, i, &irq);
>>>> + if (ret == NO_IRQ) {
>>>> + pr_err("%s: not enough interrupts specified\n",
>>>> + np->full_name);
>>>> + goto err;
>>>> + }
>>>> + gtm->timers[i].irq = irq.start;
>>>> + gtm->timers[i].gtm = gtm;
>>>> + }
>>>> +
>>>> + gtm->regs = of_iomap(np, 0);
>>>> + if (!gtm->regs) {
>>>> + pr_err("%s: unable to iomap registers\n",
>>>> + np->full_name);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + gtm_set_shortcuts(np, gtm->timers, gtm->regs);
>>>> + list_add(>m->list_node, >ms);
>>>> +
>>>> + /* We don't want to lose the node and its ->data */
>>>> + np->data = gtm;
>>>> + of_node_get(np);
>>>> +
>>>> + continue;
>>>> +err:
>>>> + kfree(gtm);
>>>> + }
>>>> +}
>>>
>>> Shouldn't we have an arch_initcall(fsl_gtm_init);
>>
>> There (and in the QE GPIO) was an arch_initcall, but based on
>> Grant Likely's review it was removed in favour of platform-specific
>> machine_initcalls.
>>
>> See http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg16469.html
>> There I was trying to argue, but quickly gave up. ;-) I don't have any
>> strong preference for this anyway. I can do either way, just tell
>> which
>> you prefer.
>
> I'd prefer the arch_initcall(). If its the board that is going to do
> the Kconfig select on this that seems sufficient to say do "init" for me
> w/o an explicit call to it.
IIRC, the argument was that we don't need unnecessary initcalls for the
multi-platform kernels. With arch_initcall() GTM/QE GPIOs will be probed
regardless of a board the kernel currently running at. With
machine_initcalls we only probe the GTMs/QE GPIOs on the boards which
actually use it.
Once again, I see pros and cons of both ways, and I don't have preference,
so.. ok, I will revert the arch_initcall() for GTM and QE GPIO.
>>>> diff --git a/include/asm-powerpc/fsl_gtm.h b/include/asm-powerpc/
>>>> fsl_gtm.h
>>>> new file mode 100644
>>>> index 0000000..49f1240
>>>> --- /dev/null
>>>> +++ b/include/asm-powerpc/fsl_gtm.h
>>>> @@ -0,0 +1,47 @@
>>>> +/*
>>>> + * Freescale General-purpose Timers Module
>>>> + *
>>>> + * Copyright (c) Freescale Semicondutor, Inc. 2006.
>>>> + * Shlomi Gridish <[EMAIL PROTECTED]>
>>>> + * Jerry Huang <[EMAIL PROTECTED]>
>>>> + * Copyright (c) MontaVista Software, Inc. 2008.
>>>> + * Anton Vorontsov <[EMAIL PROTECTED]>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify it
>>>> + * under the terms of the GNU General Public License as
>>>> published
>>>> by the
>>>> + * Free Software Foundation; either version 2 of the License, or
>>>> (at your
>>>> + * option) any later version.
>>>> + */
>>>> +
>>>> +#ifndef __ASM_FSL_GTM_H
>>>> +#define __ASM_FSL_GTM_H
>>>> +
>>>> +#include <linux/types.h>
>>>> +
>>>> +struct gtm;
>>>> +
>>>> +struct gtm_timer {
>>>> + unsigned int irq;
>>>> +
>>>> + struct gtm *gtm;
>>>> + bool requested;
>>>> + u8 __iomem *gtcfr;
>>>> + __be16 __iomem *gtmdr;
>>>> + __be16 __iomem *gtpsr;
>>>> + __be16 __iomem *gtcnr;
>>>> + __be16 __iomem *gtrfr;
>>>> + __be16 __iomem *gtevr;
>>>> +};
>>>> +
>>>> +extern void __init fsl_gtm_init(void);
>>>> +extern struct gtm_timer *gtm_get_timer(int width);
>>>> +extern struct gtm_timer *gtm_get_specific_timer(struct gtm *gtm,
>>>> int
>>>> timer,
>>>> + int width);
>>>> +extern void gtm_put_timer(struct gtm_timer *tmr);
>>>> +extern int gtm_reset_timer16(struct gtm_timer *tmr, unsigned long
>>>> usec,
>>>> + bool reload);
>>>> +extern int gtm_reset_utimer16(struct gtm_timer *tmr, u16 usec, bool
>>>> reload);
>>>
>>> can you explain the difference between these two. I'm not sure I
>>> understand the difference.
>>
>> This is explained in the .c file with a kernel doc. Basically the
>> difference is that timer16 could silently crop the precision, while
>> utimer16 could not thus explicitly accepts u16 argument (max. timer
>> interval with usec precision fits in u16).
>
> Maybe I'm confused what the utility is of cropping the precision in this
> way is. I'd also say that _timer16 is poorly named to convey the
> behavior. I'm not sure what to call it because I still dont get exactly
> why you'd want the precision cropped.
Precision matters for FHCI-like drivers, when driver, for example,
schedule transactions via the GTM timers, and there timings matters
a lot.
Though, timer16 crops the precision _only_ if usecs > 65535, so FHCI
_can_ still use the _timer16 (because FHCI does not request intervals
> 65535). But I implemented two function because:
1. I think we don't need unnecessary stuff in the ISRs (this is weak
argument since I didn't measure the impact).
2. I wanted to make the API clear (seem to fail this undertaking :-),
which functions will behave exactly the way you asked it (utimer16),
and which functions will _silently_ crop the precision (timer16)
(if asked for 1001000 usecs, it will give you ~~1001000, depending
on the GTM frequency).
--
Anton Vorontsov
email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
