Re: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver

[Rich Felker]

On Thu, Jul 28, 2016 at 04:44:05PM +0200, Thomas Gleixner wrote:
> > +
> > +#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.

OK.

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

OK.

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

Yes, I agree.

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

Unforunately the clock is in sec64.nsec32 format instead of a flat
nanoseconds count. Daniel Lezcano also suggested just using
nanoseconds, which would still need some retry and arithmetic for
adding seclo*NSEC_PER_SEC (otherwise it's problematic because it wraps
at NSEC_PER_SEC rather than at a power of two) but that does put a
hard upper bound on tickless sleep time of 4 sec. In practice it
probably doesn't matter. Should I try that instead?

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

I was trying to avoid code duplication, but I could if you think it
really matters for performance.

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

OK.

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

I don't think that's the commit you wanted to link -- it doesn't show
a usage example.

I've asked about this before in another context but didn't get an
answer -- I'm a bit concerned that, from what I can tell, the new
framework is a big singleton does assumes singletons in all the
drivers that use it. In practice it doesn't matter as long as there's
only one instance of the pit driver, but this seems architecturally
really bad and like it's a time bomb waiting to blow up on us in the
future. Am I missing something?

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

OK.

> > +	struct jcore_pit_nb *nb = 0;
> 
>   = NULL;
> 
> Please

OK.

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

OK.

> 	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

OK.

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

I can remove it if you prefer. I'd have to look back at how this
happened but I suspect some of the individual pr_errs did not exist
before.

Rich