> +
> +#define PIT_IRQ_SHIFT 12
> +#define PIT_PRIO_SHIFT 20
> +#define PIT_ENABLE_SHIFT 26
> +#define PIT_IRQ_MASK 0x3f
> +#define PIT_PRIO_MASK 0xf
Can you please align the numbers as nicely as you did below and please use
TABS not spaces to seperate them.
> +#define REG_PITEN 0x00
> +#define REG_THROT 0x10
> +#define REG_COUNT 0x14
> +#define REG_BUSPD 0x18
> +#define REG_SECHI 0x20
> +#define REG_SECLO 0x24
> +#define REG_NSEC 0x28
> +
> +struct jcore_clocksource {
> + struct clocksource cs;
> + __iomem void *base;
> +};
> +
> +struct jcore_pit {
> + struct clock_event_device ced;
> + __iomem void *base;
> + unsigned long periodic_delta;
> + unsigned cpu;
> + u32 enable_val;
> +};
Again. Please align the struct members as I asked in the other mail.
> +
> +struct jcore_pit_nb {
> + struct notifier_block nb;
> + struct jcore_pit __percpu *pit_percpu;
> +};
> +
> +static struct clocksource *jcore_cs;
> +
> +static cycle_t jcore_clocksource_read(struct clocksource *cs)
> +{
> + __iomem void *base =
> + container_of(cs, struct jcore_clocksource, cs)->base;
> + u32 sechi, seclo, nsec, sechi0, seclo0;
It would be way simpler to read if you avoid that line break by doing:
u32 sechi, seclo, nsec, sechi0, seclo0;
__iomem void *base;
base = container_of(cs, struct jcore_clocksource, cs)->base;
Hmm?
> +
> + sechi = __raw_readl(base + REG_SECHI);
> + seclo = __raw_readl(base + REG_SECLO);
> + do {
> + sechi0 = sechi;
> + seclo0 = seclo;
> + nsec = __raw_readl(base + REG_NSEC);
> + sechi = __raw_readl(base + REG_SECHI);
> + seclo = __raw_readl(base + REG_SECLO);
> + } while (sechi0 != sechi || seclo0 != seclo);
> +
> + return ((u64)sechi << 32 | seclo) * NSEC_PER_SEC + nsec;
Wow, that's an expensive thing for a hotpath operation. You really don't have
binary readout register for that clock thingy?
> +}
> +
> +static notrace u64 jcore_sched_clock_read(void)
> +{
> + return jcore_clocksource_read(jcore_cs);
Why don't you stuff the above code into this function?
> +}
> +
> +static int jcore_pit_disable(struct jcore_pit *pit)
> +{
> + __raw_writel(0, pit->base + REG_PITEN);
> + return 0;
> +}
> +
> +static int jcore_pit_set(unsigned long delta, struct jcore_pit *pit)
> +{
> + jcore_pit_disable(pit);
> + __raw_writel(delta, pit->base + REG_THROT);
> + __raw_writel(pit->enable_val, pit->base + REG_PITEN);
> + return 0;
> +}
> +
> +static int jcore_pit_set_state_shutdown(struct clock_event_device *ced)
> +{
> + struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);
Newline between declaration and code please....
> + return jcore_pit_disable(pit);
> +}
> +static int jcore_pit_cpu_notify(struct notifier_block *self,
> + unsigned long action, void *hcpu)
> +{
> + struct jcore_pit_nb *nb = container_of(self, struct jcore_pit_nb, nb);
> + switch (action & ~CPU_TASKS_FROZEN) {
> + case CPU_STARTING:
> + jcore_pit_local_init(this_cpu_ptr(nb->pit_percpu));
> + break;
> + }
> + return NOTIFY_OK;
Please convert this to the new state machine model of cpu
hotplug. CPU_STARTING will be gone very soon. Here is an example:
http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=smp/hotplug&id=7e86e8bd8dd67649d176e08d8dfb90039f0a1e98
> +static int __init jcore_pit_init(struct device_node *node)
> +{
> + int err;
> + __iomem void *pit_base;
> + unsigned pit_irq;
> + unsigned cpu;
Please put the same data types into a single line
> + struct jcore_pit_nb *nb = 0;
= NULL;
Please
> + struct jcore_clocksource *cs = 0;
> + struct jcore_pit __percpu *pit_percpu = 0;
> +
> + err = request_irq(pit_irq, jcore_timer_interrupt,
> + IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);
If you need line breaks, then please keep the arguments aligned:
err = request_irq(pit_irq, jcore_timer_interrupt,
IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);
> + /* The J-Core PIT is not hard-wired to a particular IRQ, but
> + * integrated with the interrupt controller such that the IRQ it
> + * generates is programmable. The programming interface has a
> + * legacy field which was an interrupt priority for AIC1, but
> + * which is OR'd onto bits 2-5 of the generated IRQ number when
> + * used with J-Core AIC2, so set it to match these bits. */
Proper multiline comments please
> + return 0;
> +
> +out:
> + pr_err("Could not initialize J-Core PIT driver\n");
So you have a pr_err in every error path and that goto just to pr_err the
obvious? Well, yes ...
Thanks,
tglx
