[PATCH v5 0/2] J-Core timer support

[PATCH v5 0/2] J-Core timer support

[Rich Felker]

The only difference since v4 should be merging of Arnd Bergmann's
patch that addresses the API change whereby init functions can return
an error.

Rich



Rich Felker (2):
  of: add J-Core timer bindings
  clocksource: add J-Core timer/clocksource driver

 .../devicetree/bindings/timer/jcore,pit.txt        |  25 ++
 drivers/clocksource/Kconfig                        |   8 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/jcore-pit.c                    | 283 +++++++++++++++++++++
 4 files changed, 317 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
 create mode 100644 drivers/clocksource/jcore-pit.c

-- 
2.8.1

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

[Rich Felker]

At the hardware level, the J-Core PIT is integrated with the interrupt
controller, but it is represented as its own device and has an
independent programming interface. It provides a 12-bit countdown
timer, which is not presently used, and a periodic timer. The interval
length for the latter is programmable via a 32-bit throttle register
whose units are determined by a bus-period register. The periodic
timer is used to implement both periodic and oneshot clock event
modes; in oneshot mode the interrupt handler simply disables the timer
as soon as it fires.

Despite its device tree node representing an interrupt for the PIT,
the actual irq generated is programmable, not hard-wired. The driver
is responsible for programming the PIT to generate the hardware irq
number that the DT assigns to it.

On SMP configurations, J-Core provides cpu-local instances of the PIT;
no broadcast timer is needed. This driver supports the creation of the
necessary per-cpu clock_event_device instances. The code has been
tested and works on SMP, but will not be usable without additional
J-Core SMP-support patches and appropriate hardware capable of running
SMP.

A nanosecond-resolution clocksource is provided using the J-Core "RTC"
registers, which give a 64-bit seconds count and 32-bit nanoseconds.
The driver converts these to a 64-bit nanoseconds count.

Signed-off-by: Rich Felker <dalias@libc.org>
---
 drivers/clocksource/Kconfig     |   8 ++
 drivers/clocksource/Makefile    |   1 +
 drivers/clocksource/jcore-pit.c | 283 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+)
 create mode 100644 drivers/clocksource/jcore-pit.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 47352d2..b6d4d43 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -313,6 +313,14 @@ config SYS_SUPPORTS_SH_TMU
 config SYS_SUPPORTS_EM_STI
         bool
 
+config CLKSRC_JCORE_PIT
+	bool "J-Core PIT timer driver"
+	depends on GENERIC_CLOCKEVENTS
+	depends on HAS_IOMEM
+	help
+	  This enables build of clocksource and clockevent driver for
+	  the integrated PIT in the J-Core synthesizable, open source SoC.
+
 config SH_TIMER_CMT
 	bool "Renesas CMT timer driver" if COMPILE_TEST
 	depends on GENERIC_CLOCKEVENTS
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 473974f..b41a834 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_ATMEL_TCB_CLKSRC)	+= tcb_clksrc.o
 obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
 obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
 obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
+obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
 obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
 obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
 obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
diff --git a/drivers/clocksource/jcore-pit.c b/drivers/clocksource/jcore-pit.c
new file mode 100644
index 0000000..4a2f780
--- /dev/null
+++ b/drivers/clocksource/jcore-pit.c
@@ -0,0 +1,283 @@
+/*
+ * J-Core SoC PIT/clocksource driver
+ *
+ * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
+#include <linux/cpu.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#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
+
+#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;
+};
+
+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;
+
+	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;
+}
+
+static notrace u64 jcore_sched_clock_read(void)
+{
+	return jcore_clocksource_read(jcore_cs);
+}
+
+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);
+	return jcore_pit_disable(pit);
+}
+
+static int jcore_pit_set_state_oneshot(struct clock_event_device *ced)
+{
+	struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);
+	return jcore_pit_disable(pit);
+}
+
+static int jcore_pit_set_state_periodic(struct clock_event_device *ced)
+{
+	struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);
+	return jcore_pit_set(pit->periodic_delta, pit);
+}
+
+static int jcore_pit_set_next_event(unsigned long delta,
+				    struct clock_event_device *ced)
+{
+	struct jcore_pit *pit = container_of(ced, struct jcore_pit, ced);
+	return jcore_pit_set(delta, pit);
+}
+
+static int jcore_pit_local_init(struct jcore_pit *pit)
+{
+	unsigned buspd, freq;
+
+	pr_info("Local J-Core PIT init on cpu %u\n", pit->cpu);
+
+	buspd = __raw_readl(pit->base + REG_BUSPD);
+	freq = DIV_ROUND_CLOSEST(NSEC_PER_SEC, buspd);
+	pit->periodic_delta = DIV_ROUND_CLOSEST(NSEC_PER_SEC, HZ*buspd);
+
+	clockevents_config_and_register(&pit->ced, freq, 1, ULONG_MAX);
+
+	return 0;
+}
+
+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;
+}
+
+static irqreturn_t jcore_timer_interrupt(int irq, void *dev_id)
+{
+	struct jcore_pit *pit = this_cpu_ptr(dev_id);
+
+	if (clockevent_state_oneshot(&pit->ced))
+		jcore_pit_disable(pit);
+
+	pit->ced.event_handler(&pit->ced);
+
+	return IRQ_HANDLED;
+}
+
+static int __init jcore_pit_init(struct device_node *node)
+{
+	int err;
+	__iomem void *pit_base;
+	unsigned pit_irq;
+	unsigned cpu;
+	struct jcore_pit_nb *nb = 0;
+	struct jcore_clocksource *cs = 0;
+	struct jcore_pit __percpu *pit_percpu = 0;
+	unsigned long hwirq;
+	u32 irqprio, enable_val;
+
+	pit_base = of_iomap(node, 0);
+	if (!pit_base) {
+		pr_err("Error: Cannot map base address for J-Core PIT\n");
+		err = -ENXIO;
+		goto out;
+	}
+
+	pit_irq = irq_of_parse_and_map(node, 0);
+	if (!pit_irq) {
+		pr_err("Error: J-Core PIT has no IRQ\n");
+		err = -ENXIO;
+		goto out;
+	}
+
+	pr_info("Initializing J-Core PIT at %p IRQ %d\n", pit_base, pit_irq);
+
+	cs = kzalloc(sizeof *cs, GFP_KERNEL);
+	if (!cs) {
+		pr_err("Failed to allocate memory for clocksource\n");
+		err = ENOMEM;
+		goto out;
+	}
+	jcore_cs = &cs->cs;
+
+	cs->base = pit_base;
+	cs->cs.name = "jcore_pit_cs";
+	cs->cs.rating = 400;
+	cs->cs.read = jcore_clocksource_read;
+	cs->cs.mult = 1;
+	cs->cs.shift = 0;
+	cs->cs.mask = CLOCKSOURCE_MASK(64);
+	cs->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+	err = clocksource_register_hz(&cs->cs, NSEC_PER_SEC);
+	if (err) {
+		pr_err("Error registering clocksource device: %d\n", err);
+		goto out;
+	}
+
+	sched_clock_register(jcore_sched_clock_read, 64, NSEC_PER_SEC);
+
+	pit_percpu = alloc_percpu(struct jcore_pit);
+	if (!pit_percpu) {
+		pr_err("Failed to allocate memory for clock event device\n");
+		err = ENOMEM;
+		goto out;
+	}
+
+	nb = kzalloc(sizeof *nb, GFP_KERNEL);
+	if (!nb) {
+		pr_err("Failed to allocate memory for J-Core PIT notifier\n");
+		err = ENOMEM;
+		goto out;
+	}
+
+	nb->pit_percpu = pit_percpu;
+	nb->nb.notifier_call = jcore_pit_cpu_notify;
+	err = register_cpu_notifier(&nb->nb);
+	if (err) {
+		pr_err("Error registering J-Core PIT notifier: %d\n", err);
+		goto out;
+	}
+
+	err = request_irq(pit_irq, jcore_timer_interrupt,
+		IRQF_TIMER | IRQF_PERCPU, "jcore_pit", pit_percpu);
+	if (err) {
+		pr_err("pit irq request failed: %d\n", err);
+		goto out;
+	}
+
+	/* 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. */
+	hwirq = irq_get_irq_data(pit_irq)->hwirq;
+	irqprio = (hwirq >> 2) & PIT_PRIO_MASK;
+	enable_val = (1U << PIT_ENABLE_SHIFT)
+		   | (hwirq << PIT_IRQ_SHIFT)
+		   | (irqprio << PIT_PRIO_SHIFT);
+
+	for_each_possible_cpu(cpu) {
+		struct jcore_pit *pit = per_cpu_ptr(pit_percpu, cpu);
+
+		pit->base = of_iomap(node, cpu);
+		if (!pit->base)
+			continue;
+
+		pit->ced.name = "jcore_pit";
+		pit->ced.features = CLOCK_EVT_FEAT_PERIODIC
+				  | CLOCK_EVT_FEAT_ONESHOT
+				  | CLOCK_EVT_FEAT_PERCPU;
+		pit->ced.cpumask = cpumask_of(cpu);
+		pit->ced.rating = 400;
+		pit->ced.irq = pit_irq;
+		pit->ced.set_state_shutdown = jcore_pit_set_state_shutdown;
+		pit->ced.set_state_periodic = jcore_pit_set_state_periodic;
+		pit->ced.set_state_oneshot = jcore_pit_set_state_oneshot;
+		pit->ced.set_next_event = jcore_pit_set_next_event;
+
+		pit->cpu = cpu;
+		pit->enable_val = enable_val;
+	}
+
+	jcore_pit_local_init(this_cpu_ptr(pit_percpu));
+
+	return 0;
+
+out:
+	pr_err("Could not initialize J-Core PIT driver\n");
+	return err;
+}
+
+CLOCKSOURCE_OF_DECLARE(jcore_pit, "jcore,pit", jcore_pit_init);
-- 
2.8.1

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

[Thomas Gleixner]

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

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

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

[Thomas Gleixner]

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

Up to you. I was just wondering about the MUL.
 
> > > +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.

Did not see where the other usage site was. Must have missed that. So keep it
as is and please add notrace to jcore_clocksource_read().
 
> > 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.
 
It's the conversion of a driver from the old to the new style, So it's an
example how to move your stuff to the new interface or am I missing something
here?

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

Most of the users are single instance. We have a dynamic interface for the
online callbacks and we might get one for the prep stage as well.

Now for the real core stuff like starting/dying we want explicit states and if
a driver really is multi instance then having a private list there is not
rocket science. Most of them have an instance list anyway.
 
Thanks,

	tglx

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

[Rich Felker]

On Thu, Jul 28, 2016 at 06:44:15PM +0200, Thomas Gleixner wrote:
> On Thu, 28 Jul 2016, Rich Felker wrote:
> > On Thu, Jul 28, 2016 at 04:44:05PM +0200, Thomas Gleixner wrote:
> > > > +	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?
> 
> Up to you. I was just wondering about the MUL.

Well 2 people have raised the issue now so I think I'll go ahead and
try changing it.

> > > > +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.
> 
> Did not see where the other usage site was. Must have missed that. So keep it
> as is and please add notrace to jcore_clocksource_read().

Since we have to assume a singleton now anyway, I'm just going to
switch the caller/callee relationship the other way around and make
the sched version use static objects directly. This shought be
slightly faster/smaller.

> > > 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.
>  
> It's the conversion of a driver from the old to the new style, So it's an
> example how to move your stuff to the new interface or am I missing something
> here?

I botched the link copy and paste. Sorry for the noise. I see it now.

> > 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?
> 
> Most of the users are single instance. We have a dynamic interface for the
> online callbacks and we might get one for the prep stage as well.
> 
> Now for the real core stuff like starting/dying we want explicit states and if
> a driver really is multi instance then having a private list there is not
> rocket science. Most of them have an instance list anyway.

For now I'll just remove some of the dynamic allocation I added and
simplify assuming a single instance. It doesn't make sense to be
incurring costs for the nominal ability to have multiple instances
when it doesn't actually work in the new framework (without a lot more
per-driver code duplication that really should be in the framework)
and when it's not needed anyway.

Rich

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

Trying this change now and I'm getting hangs inside
cpuhp_invoke_ap_callback between wake_up_process and
wait_for_completion. I suspect it's not possible to run a scheduled
task yet because there's no timer yet, or something like that. Do I
need further infrastructure from tip that's not upstream yet in order
to test this? I just rebased on linus/master and still have the same
problem, but none of the other driver changes are in Linus's tree yet.
Following the other drivers in tip (again, I don't have any of these
in my tree), I put the new state between CPUHP_AP_SCHED_STARTING and
CPUHP_AP_NOTIFY_STARTING; is this correct?

Rich

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

[Rich Felker]

On Thu, Jul 28, 2016 at 04:00:47PM -0400, Rich Felker wrote:
> On Thu, Jul 28, 2016 at 04:44:05PM +0200, Thomas Gleixner wrote:
> > > +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
> 
> Trying this change now and I'm getting hangs inside
> cpuhp_invoke_ap_callback between wake_up_process and
> wait_for_completion. I suspect it's not possible to run a scheduled
> task yet because there's no timer yet, or something like that. Do I
> need further infrastructure from tip that's not upstream yet in order
> to test this? I just rebased on linus/master and still have the same
> problem, but none of the other driver changes are in Linus's tree yet.
> Following the other drivers in tip (again, I don't have any of these
> in my tree), I put the new state between CPUHP_AP_SCHED_STARTING and
> CPUHP_AP_NOTIFY_STARTING; is this correct?

I think it's this bug where the fix is not yet in Linus's tree:

http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/kernel/cpu.c?h=smp/hotplug&id=6a4e24518c8a10f78f44da219835239cb5aca90d

Cherry-picking that made it work. FWIW, it's really hard to do
development on top of new infrastructure that's not yet working in any
tree that's intended for others to pull, but I made it work and I'll
have a new version of the patch for you soon.

Rich

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

[Rich Felker]

On Thu, Jul 28, 2016 at 04:18:44PM -0400, Rich Felker wrote:
> On Thu, Jul 28, 2016 at 04:00:47PM -0400, Rich Felker wrote:
> > On Thu, Jul 28, 2016 at 04:44:05PM +0200, Thomas Gleixner wrote:
> > > > +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
> > 
> > Trying this change now and I'm getting hangs inside
> > cpuhp_invoke_ap_callback between wake_up_process and
> > wait_for_completion. I suspect it's not possible to run a scheduled
> > task yet because there's no timer yet, or something like that. Do I
> > need further infrastructure from tip that's not upstream yet in order
> > to test this? I just rebased on linus/master and still have the same
> > problem, but none of the other driver changes are in Linus's tree yet.
> > Following the other drivers in tip (again, I don't have any of these
> > in my tree), I put the new state between CPUHP_AP_SCHED_STARTING and
> > CPUHP_AP_NOTIFY_STARTING; is this correct?
> 
> I think it's this bug where the fix is not yet in Linus's tree:
> 
> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/kernel/cpu.c?h=smp/hotplug&id=6a4e24518c8a10f78f44da219835239cb5aca90d
> 
> Cherry-picking that made it work. FWIW, it's really hard to do
> development on top of new infrastructure that's not yet working in any
> tree that's intended for others to pull, but I made it work and I'll
> have a new version of the patch for you soon.

I hit some more snags with rcu_sched stalls that I thought were my
fault, but it looks like it's actually something in the linus/master I
had to rebase on (I've tagged the revision locally in case this is
something we need to track later). They happen equally with or without
my latest changes for the notify_nb -> cpuhp transition and other
requested changes to the pit driver.

Rich

[PATCH v5 1/2] of: add J-Core timer bindings

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 .../devicetree/bindings/timer/jcore,pit.txt        | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt

diff --git a/Documentation/devicetree/bindings/timer/jcore,pit.txt b/Documentation/devicetree/bindings/timer/jcore,pit.txt
new file mode 100644
index 0000000..0f42af4
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/jcore,pit.txt
@@ -0,0 +1,25 @@
+J-Core Programmable Interval Timer and Clocksource
+
+Required properties:
+
+- compatible: Must be "jcore,pit".
+
+- reg: Memory region(s) for timer/clocksource registers. For SMP,
+  there should be one region per cpu, indexed by the sequential,
+  zero-based hardware cpu number (which is also the logical cpu
+  number).
+
+- interrupts: An interrupt to assign for the timer. The actual pit
+  core is integrated with the aic and allows the timer interrupt
+  assignment to be programmed by software, but this property is
+  required in order to reserve an interrupt number that doesn't
+  conflict with other devices.
+
+
+Example:
+
+timer@200 {
+	compatible = "jcore,pit";
+	reg = < 0x200 0x30 0x500 0x30 >;
+	interrupts = < 0x48 >;
+};
-- 
2.8.1

Re: [PATCH v5 1/2] of: add J-Core timer bindings

[Rob Herring]

On Tue, May 17, 2016 at 11:18:58PM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  .../devicetree/bindings/timer/jcore,pit.txt        | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 1/2] of: add J-Core timer bindings

[Rich Felker]

On Tue, May 17, 2016 at 11:18:58PM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  .../devicetree/bindings/timer/jcore,pit.txt        | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/jcore,pit.txt b/Documentation/devicetree/bindings/timer/jcore,pit.txt
> new file mode 100644
> index 0000000..0f42af4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/jcore,pit.txt
> @@ -0,0 +1,25 @@
> +J-Core Programmable Interval Timer and Clocksource
> +
> +Required properties:
> +
> +- compatible: Must be "jcore,pit".
> +
> +- reg: Memory region(s) for timer/clocksource registers. For SMP,
> +  there should be one region per cpu, indexed by the sequential,
> +  zero-based hardware cpu number (which is also the logical cpu
> +  number).

One detail I missed: Mark Rutland asked me to remove the corresponding
remark about logical cpu numbers from the AIC binding document, so I
think it should be removed here too for the same reason -- it's a
Linux implementation detail, not part of the hw binding.

Rich