Re: [patch v2 18/35] Hexagon: Add time and timer functions

Arnd Bergmann
2011-08-31T14:05:02Z

On Tuesday 30 August 2011, Richard Kuo wrote:
> Signed-off-by: Richard Kuo <rkuo@codeaurora.org>

> +#ifdef CONFIG_SMP
> +void setup_percpu_clockdev(void);
> +void ipi_timer(void);
> +#endif

You should never need to wrap declarations with #ifdef like this.

> +
> +#ifndef _ASM_TIMER_REGS_H
> +#define _ASM_TIMER_REGS_H
> +
> +/* This stuff should go into a platform specific file */
> +#define TCX0_CLK_RATE 19200
> +#define TIMER_ENABLE 0
> +#define TIMER_CLR_ON_MATCH 1

Same comment as for NR_IRQS. If this is platform specific, it should
not be a compile-time constant.

>+/*
>+ * For now, hard wire the simulated CPU/pcycle frequency.
>+ * XXX TODO fish it out of device tree!
>+ */
>+#define CPU_MHZ 600
>+
>+static struct resource rtos_timer_resources[] = {
>+ {
>+ .start = RTOS_TIMER_REGS_ADDR,
>+ .end = RTOS_TIMER_REGS_ADDR+PAGE_SIZE-1,
>+ .flags = IORESOURCE_MEM,
>+ },
>+};

Yes, please do get it out of the device tree before merging the driver
upstream.

> +/* A lot of this stuff should move into a platform specific section. */
> +struct adsp_hw_timer_struct {
> + unsigned int match; /* Match value */
> + unsigned int count;
> + unsigned int enable; /* [1] - CLR_ON_MATCH_EN, [0] - EN */
> + unsigned int clear; /* one-shot register that clears the count */
> +};
> +
> +/* Look for "TCX0" for related constants. */
> +static volatile struct adsp_hw_timer_struct *rtos_timer = (void *) 0x0;

There are multiple things wrong with this:

* No need to initialize static variables to zero

* This is an MMIO register, so the pointer must be "__iomem", not "volatile",
see Documentation/volatile-considered-harmful.txt

* You should have found this when building with 'make C=1' using sparse.
I would hope that all your code was compiled this way to find common
errors. If not, please do so now.

* Use u32 instead of unsigned int when describing hardware structures.
They are always identical, but u32 is more descriptive. Alternatively,
use no structure at all but constant register numbers.

> +static struct clock_event_device hexagon_clockevent_dev = {
> + .name = "clockevent",
> + .features = CLOCK_EVT_FEAT_ONESHOT,
> + .rating = 400,
> + .shift = 32,
> + .irq = RTOS_TIMER_INT,
> + .set_next_event = set_next_event,
> + .set_mode = set_mode,
> +#ifdef CONFIG_SMP
> + .broadcast = broadcast
> +#endif
> +};

Always put a comma at the last entry, to allow adding more members
as necessary.

> +/*
> + * time_init_deferred - called by start_kernel to set up timer/clock source
> + *
> + * Install the IRQ handler for the clock, setup timers.
> + * This is done late, as that way, we can use ioremap().
> + *
> + * This runs just before the delay loop is calibrated, and
> + * is used for delay calibration.
> + */
> +void __init time_init_deferred(void)
> +{
> + struct resource *resource = NULL;
> + struct clock_event_device *ce_dev = &hexagon_clockevent_dev;
> + struct device_node *dn;
> + struct resource r;
> + int err;
> +
> +#ifdef CONFIG_SMP
> + ce_dev->cpumask = cpu_all_mask;
> +#else
> + ce_dev->cpumask = cpumask_of(0);
> +#endif

No need for the #ifdef AFAICT, because the two cases are the same.

> + if (!resource)
> + resource = rtos_timer_device.resource;
> +
> + /* ioremap here means this has to run later, after paging init */
> + rtos_timer = ioremap(resource->start, resource->end
> + - resource->start + 1);

When the device is in the device tree, you can simplify this a lot
by using of_iomap().

> +void __udelay(unsigned long usecs)
> +{
> + unsigned long long start = __vmgettime();
> + unsigned long long finish = (CPU_MHZ * usecs) - fudgefactor;
> +
> + while ((__vmgettime() - start) < finish)
> + ; /* not sure how this improves readability */
> +}

Better use cpu_relax() or a direct hypervisor yield call here instead of
the ';'. It should improve both readability and power consumption during
long delays.

Arnd