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

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

[Rich Felker]

Resubmitted with the (incorrect) workaround for rcu_sched stalls
removed, since the problem was found in the interrupt controller
driver; a separate patch will be submitted for it.

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

 .../devicetree/bindings/timer/jcore,pit.txt        |  24 +++
 drivers/clocksource/Kconfig                        |  10 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/jcore-pit.c                    | 231 +++++++++++++++++++++
 include/linux/cpuhotplug.h                         |   1 +
 5 files changed, 267 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
 create mode 100644 drivers/clocksource/jcore-pit.c

-- 
2.10.0

[PATCH v8 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.

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

Signed-off-by: Rich Felker <dalias@libc.org>
---
 drivers/clocksource/Kconfig     |  10 ++
 drivers/clocksource/Makefile    |   1 +
 drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h      |   1 +
 4 files changed, 243 insertions(+)
 create mode 100644 drivers/clocksource/jcore-pit.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5677886..95dd78b 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU
 config SYS_SUPPORTS_EM_STI
         bool
 
+config CLKSRC_JCORE_PIT
+	bool "J-Core PIT timer driver"
+	depends on OF && (SUPERH || COMPILE_TEST)
+	depends on GENERIC_CLOCKEVENTS
+	depends on HAS_IOMEM
+	select CLKSRC_MMIO
+	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 fd9d6df..cf87f40 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..4f3ef69
--- /dev/null
+++ b/drivers/clocksource/jcore-pit.c
@@ -0,0 +1,231 @@
+/*
+ * 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/cpuhotplug.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_pit {
+	struct clock_event_device	ced;
+	__iomem void			*base;
+	unsigned long			periodic_delta;
+	unsigned			cpu;
+	u32				enable_val;
+};
+
+static __iomem void		*jcore_pit_base;
+struct jcore_pit __percpu	*jcore_pit_percpu;
+
+static notrace u64 jcore_sched_clock_read(void)
+{
+	u32 seclo, nsec, seclo0;
+	__iomem void *base = jcore_pit_base;
+
+	seclo = readl(base + REG_SECLO);
+	do {
+		seclo0 = seclo;
+		nsec  = readl(base + REG_NSEC);
+		seclo = readl(base + REG_SECLO);
+	} while (seclo0 != seclo);
+
+	return seclo * NSEC_PER_SEC + nsec;
+}
+
+static cycle_t jcore_clocksource_read(struct clocksource *cs)
+{
+	return jcore_sched_clock_read();
+}
+
+static int jcore_pit_disable(struct jcore_pit *pit)
+{
+	writel(0, pit->base + REG_PITEN);
+	return 0;
+}
+
+static int jcore_pit_set(unsigned long delta, struct jcore_pit *pit)
+{
+	jcore_pit_disable(pit);
+	writel(delta, pit->base + REG_THROT);
+	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(unsigned cpu)
+{
+	struct jcore_pit *pit = this_cpu_ptr(jcore_pit_percpu);
+	unsigned buspd, freq;
+
+	pr_info("Local J-Core PIT init on cpu %u\n", pit->cpu);
+
+	buspd = 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 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;
+	unsigned pit_irq, cpu;
+	unsigned long hwirq;
+	u32 irqprio, enable_val;
+
+	jcore_pit_base = of_iomap(node, 0);
+	if (!jcore_pit_base) {
+		pr_err("Error: Cannot map base address for J-Core PIT\n");
+		return -ENXIO;
+	}
+
+	pit_irq = irq_of_parse_and_map(node, 0);
+	if (!pit_irq) {
+		pr_err("Error: J-Core PIT has no IRQ\n");
+		return -ENXIO;
+	}
+
+	pr_info("Initializing J-Core PIT at %p IRQ %d\n",
+		jcore_pit_base, pit_irq);
+
+	err = clocksource_mmio_init(jcore_pit_base, "jcore_pit_cs",
+				    NSEC_PER_SEC, 400, 32,
+				    jcore_clocksource_read);
+	if (err) {
+		pr_err("Error registering clocksource device: %d\n", err);
+		return err;
+	}
+
+	sched_clock_register(jcore_sched_clock_read, 32, NSEC_PER_SEC);
+
+	jcore_pit_percpu = alloc_percpu(struct jcore_pit);
+	if (!jcore_pit_percpu) {
+		pr_err("Failed to allocate memory for clock event device\n");
+		return -ENOMEM;
+	}
+
+	err = request_irq(pit_irq, jcore_timer_interrupt,
+			  IRQF_TIMER | IRQF_PERCPU,
+			  "jcore_pit", jcore_pit_percpu);
+	if (err) {
+		pr_err("pit irq request failed: %d\n", err);
+		free_percpu(jcore_pit_percpu);
+		return err;
+	}
+
+	/*
+	 * 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_present_cpu(cpu) {
+		struct jcore_pit *pit = per_cpu_ptr(jcore_pit_percpu, cpu);
+
+		pit->base = of_iomap(node, cpu);
+		if (!pit->base) {
+			pr_err("Unable to map PIT for cpu %u\n", cpu);
+			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;
+	}
+
+	cpuhp_setup_state(CPUHP_AP_JCORE_TIMER_STARTING,
+			  "AP_JCORE_TIMER_STARTING",
+			  jcore_pit_local_init, NULL);
+
+	return 0;
+}
+
+CLOCKSOURCE_OF_DECLARE(jcore_pit, "jcore,pit", jcore_pit_init);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 34bd805..e2e6c8c 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -52,6 +52,7 @@ enum cpuhp_state {
 	CPUHP_AP_ARM_ARCH_TIMER_STARTING,
 	CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
 	CPUHP_AP_DUMMY_TIMER_STARTING,
+	CPUHP_AP_JCORE_TIMER_STARTING,
 	CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
 	CPUHP_AP_ARM_TWD_STARTING,
 	CPUHP_AP_METAG_TIMER_STARTING,
-- 
2.10.0

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

[Daniel Lezcano]

Hi Rich,

On Sun, Oct 09, 2016 at 05:34:22AM +0000, Rich Felker wrote:
> 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.

For my personnal information, why no broadcast timer is needed ?

Are the CPUs on always-on power down ?

> A nanosecond-resolution clocksource is provided using the J-Core "RTC"
> registers, which give a 64-bit seconds count and 32-bit nanoseconds
> that wrap every second. The driver converts these to a full-range
> 32-bit nanoseconds count.
> 
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  drivers/clocksource/Kconfig     |  10 ++
>  drivers/clocksource/Makefile    |   1 +
>  drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h      |   1 +
>  4 files changed, 243 insertions(+)
>  create mode 100644 drivers/clocksource/jcore-pit.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 5677886..95dd78b 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU
>  config SYS_SUPPORTS_EM_STI
>          bool
>  
> +config CLKSRC_JCORE_PIT
> +	bool "J-Core PIT timer driver"
> +	depends on OF && (SUPERH || COMPILE_TEST)

Actually the idea is to have the SUPERH to select this timer, not create
a dependency on SUPERH from here.

We don't want to prompt in the configuration menu the drivers because it
would be impossible to anyone to know which timer comes with which
hardware, so we let the platform to select the timer it needs.

The exception is to enable in order to compile on non-native platform to
compile-test a bunch of drivers (eg. compile most of the clocksource / 
clockevents drivers on a x86 big server).

That's why we should have:

config CLKSRC_JCORE_PIT
	bool "J-Core PIT timer driver" if COMPILE_TEST

So the jcore pit driver option appears only if compile test is enabled
otherwise it is a silent option and the user doesn't have to deal with
it. Having consistent dependency like the other drivers will help future
changes to consolidate the Kconfig.

[ ... ]              

> +#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_pit {
> +	struct clock_event_device	ced;
> +	__iomem void			*base;

It is not '__iomem void *' but 'void __iomem *'. This appears multiple
times in this code.

> +	unsigned long			periodic_delta;
> +	unsigned			cpu;

This field is pointless, 'cpu' is only used for a trace in the init
function which has already the cpu has parameter.

> +	u32				enable_val;
> +};
> +
> +static __iomem void		*jcore_pit_base;

static void __iomem *jcore_pit_base;

> +struct jcore_pit __percpu	*jcore_pit_percpu;

static.

[ ... ]

> +static int __init jcore_pit_init(struct device_node *node)
> +{

[ ... ]

> +	/*
> +	 * 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);
> +

Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?

Will be the same information available if the irqchip is AIC1 ?

I have to admin I find strange this driver has to invoke irq_get_irq_data(),
it is the only one and it sounds even strange it has to be stored per cpu below.

> +	for_each_present_cpu(cpu) {

		[ ... ]

> +		pit->enable_val = enable_val;
> +	}
> +
> +	cpuhp_setup_state(CPUHP_AP_JCORE_TIMER_STARTING,
> +			  "AP_JCORE_TIMER_STARTING",
> +			  jcore_pit_local_init, NULL);
> +
> +	return 0;
> +}
> +

Thanks !

  -- Daniel 

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

[Rich Felker]

On Tue, Oct 11, 2016 at 08:18:12PM +0200, Daniel Lezcano wrote:
> 
> Hi Rich,
> 
> On Sun, Oct 09, 2016 at 05:34:22AM +0000, Rich Felker wrote:
> > 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.
> 
> For my personnal information, why no broadcast timer is needed ?

Broadcast timer is only needed if you don't have percpu local timers.
Early on in SMP development I actually tested with an ipi broadcast
timer and performance was noticably worse.

> Are the CPUs on always-on power down ?

For now they are always on and don't even have the sleep instruction
(i.e. stop cpu clock until interrupt) implemented. Adding sleep will
be the first power-saving step, and perhaps the only one for now,
since there doesn't seem to be any indication (according to the ppl
working on the hardware) that a deeper sleep would provide significant
additional savings.

> > A nanosecond-resolution clocksource is provided using the J-Core "RTC"
> > registers, which give a 64-bit seconds count and 32-bit nanoseconds
> > that wrap every second. The driver converts these to a full-range
> > 32-bit nanoseconds count.
> > 
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  drivers/clocksource/Kconfig     |  10 ++
> >  drivers/clocksource/Makefile    |   1 +
> >  drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/cpuhotplug.h      |   1 +
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 drivers/clocksource/jcore-pit.c
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 5677886..95dd78b 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU
> >  config SYS_SUPPORTS_EM_STI
> >          bool
> >  
> > +config CLKSRC_JCORE_PIT
> > +	bool "J-Core PIT timer driver"
> > +	depends on OF && (SUPERH || COMPILE_TEST)
> 
> Actually the idea is to have the SUPERH to select this timer, not create
> a dependency on SUPERH from here.
> 
> We don't want to prompt in the configuration menu the drivers because it
> would be impossible to anyone to know which timer comes with which
> hardware, so we let the platform to select the timer it needs.

I thought we discussed this before. For users building a kernel for
legacy SH systems, especially in the current state where they're only
supported with hard-coded board files rather than device tree, it
makes no sense to build drivers for J-core hardware. It would make
sense to be on by default for CONFIG_SH_DEVICE_TREE with a compatible
CPU selection, but at least at this time, not for SUPERH in general.

Anyway I'd really like to do this non-invasively as long as we have a
mix of legacy and new stuff and the legacy stuff is not readily
testable. Once all of arch/sh is moved over to device tree, could we
revisit this and make all the drivers follow a common policy (on by
default if they're associated with boards/SoCs using a matching or
compatible CPU model, or something like that, but still able to be
disabled manually, since the user might be trying to get a tiny-ish
embedded kernel)?

> The exception is to enable in order to compile on non-native platform to
> compile-test a bunch of drivers (eg. compile most of the clocksource / 
> clockevents drivers on a x86 big server).
> 
> That's why we should have:
> 
> config CLKSRC_JCORE_PIT
> 	bool "J-Core PIT timer driver" if COMPILE_TEST
> 
> So the jcore pit driver option appears only if compile test is enabled
> otherwise it is a silent option and the user doesn't have to deal with
> it. Having consistent dependency like the other drivers will help future
> changes to consolidate the Kconfig.

I don't think this matches the user expectation for the arch yet. For
now we have a j2_defconfig that enables the drivers and other kernel
settings expected to be useful for J-core socs. I want to modernize
this all but that's a separate project.

> [ ... ]              
> 
> > +#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_pit {
> > +	struct clock_event_device	ced;
> > +	__iomem void			*base;
> 
> It is not '__iomem void *' but 'void __iomem *'. This appears multiple
> times in this code.

OK, I'll change that.

> > +	unsigned long			periodic_delta;
> > +	unsigned			cpu;
> 
> This field is pointless, 'cpu' is only used for a trace in the init
> function which has already the cpu has parameter.

OK, will remove. It was needed for the old notify framework but not
for cpu hotplug framework, I think.

> > +	u32				enable_val;
> > +};
> > +
> > +static __iomem void		*jcore_pit_base;
> 
> static void __iomem *jcore_pit_base;

OK.

> > +struct jcore_pit __percpu	*jcore_pit_percpu;
> 
> static.

OK.

> [ ... ]
> 
> > +static int __init jcore_pit_init(struct device_node *node)
> > +{
> 
> [ ... ]

Was there something analogous you wanted me to change here, or just
leftover quoting?

> > +	/*
> > +	 * 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);
> > +
> 
> Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
> 
> Will be the same information available if the irqchip is AIC1 ?

The bit layout of the PIT enable register is:

	.....e..ppppiiiiiiii............

where the .'s indicate unrelated/unused bits, e is enable, p is
priority, and i is hard irq number.

For the PIT included in AIC1 (obsolete but still in use), any hard irq
(trap number) can be programmed via the 8 iiiiiiii bits, and a
priority (0-15) is programmable separately in the pppp bits.

For the PIT included in AIC2 (current), the programming interface is
equivalent modulo interrupt mapping. This is why a different
compatible tag was not used. However only traps 64-127 (the ones
actually intended to be used for interrupts, rather than
syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are
ignored) and the priority pppp is <<2'd and or'd onto the irq number.
This was a poor decision made on the hardware engineering side based
on a wrong assumption that preserving old priority mapping of outdated
software was important, whereas priorities weren't/aren't even being
used.

When we do the next round of interrupt controller improvements (AIC3)
the PIT programming interface should remain compatible with the
driver; likely the priority bits will just be ignored.

If we do want to change the programming interface beyond this at some
point (that maay be a good idea, since we have identified several
things that are less than ideal for Linux, like the sechi/seclo/ns
clocksource), a new compatible tag will be added instead.

> I have to admin I find strange this driver has to invoke irq_get_irq_data(),
> it is the only one and it sounds even strange it has to be stored per cpu below.

The timer will not actually generate the irq it's assigned to (or any
interrupt at all) unless/until it's programmed for the (hard) irq
number. The need to use irq_get_irq_data comes from the fact that the
hardware needs the hard irq number, not a virq number, which could in
principle be different.

There's probably some argument that could have been made that the
irqchip and clocksource/timer driver should have been unified since
they're unified in the hardware and have this awkward programming
relationship, but that didn't fit the Linux model very well and I
think having them factored like this encourages future versions of the
hardware to adapt to the model the software wants.

Rich

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

[Daniel Lezcano]

On Tue, Oct 11, 2016 at 04:28:50PM -0400, Rich Felker wrote:
> On Tue, Oct 11, 2016 at 08:18:12PM +0200, Daniel Lezcano wrote:
> > 
> > Hi Rich,
> > 
> > On Sun, Oct 09, 2016 at 05:34:22AM +0000, Rich Felker wrote:
> > > 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.
> > 
> > For my personnal information, why no broadcast timer is needed ?
> 
> Broadcast timer is only needed if you don't have percpu local timers.
> Early on in SMP development I actually tested with an ipi broadcast
> timer and performance was noticably worse.

Obviously. I thought there were another reason related to power management.
 
> > Are the CPUs on always-on power down ?
> 
> For now they are always on and don't even have the sleep instruction
> (i.e. stop cpu clock until interrupt) implemented. Adding sleep will
> be the first power-saving step, and perhaps the only one for now,
> since there doesn't seem to be any indication (according to the ppl
> working on the hardware) that a deeper sleep would provide significant
> additional savings.

Ok.

However, the 'sleep' state is not, in the power management terminology,
the idle state described above. It is called "clock gated" / "Wait for
Interrupt".

The 'sleep' state lose the CPU context.
 
> > > A nanosecond-resolution clocksource is provided using the J-Core "RTC"
> > > registers, which give a 64-bit seconds count and 32-bit nanoseconds
> > > that wrap every second. The driver converts these to a full-range
> > > 32-bit nanoseconds count.
> > > 
> > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > ---
> > >  drivers/clocksource/Kconfig     |  10 ++
> > >  drivers/clocksource/Makefile    |   1 +
> > >  drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/cpuhotplug.h      |   1 +
> > >  4 files changed, 243 insertions(+)
> > >  create mode 100644 drivers/clocksource/jcore-pit.c
> > > 
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index 5677886..95dd78b 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU
> > >  config SYS_SUPPORTS_EM_STI
> > >          bool
> > >  
> > > +config CLKSRC_JCORE_PIT
> > > +	bool "J-Core PIT timer driver"
> > > +	depends on OF && (SUPERH || COMPILE_TEST)
> > 
> > Actually the idea is to have the SUPERH to select this timer, not create
> > a dependency on SUPERH from here.
> > 
> > We don't want to prompt in the configuration menu the drivers because it
> > would be impossible to anyone to know which timer comes with which
> > hardware, so we let the platform to select the timer it needs.
> 
> I thought we discussed this before. For users building a kernel for
> legacy SH systems, especially in the current state where they're only
> supported with hard-coded board files rather than device tree, it
> makes no sense to build drivers for J-core hardware. It would make
> sense to be on by default for CONFIG_SH_DEVICE_TREE with a compatible
> CPU selection, but at least at this time, not for SUPERH in general.

Probably I am missing the point but why the user would have to unselect
this driver manually ? The user wants a config file nothing more or a very
trivial option. Can you imagine someone can know every single IP block for
each boards of the same arch and be able to disable/enable the right ones ?

> Anyway I'd really like to do this non-invasively as long as we have a
> mix of legacy and new stuff and the legacy stuff is not readily
> testable. Once all of arch/sh is moved over to device tree, could we
> revisit this and make all the drivers follow a common policy (on by
> default if they're associated with boards/SoCs using a matching or
> compatible CPU model, or something like that, but still able to be
> disabled manually, since the user might be trying to get a tiny-ish
> embedded kernel)?

I understand the goal is to have one single configuration and everything
DT based and it sounds great but what is missing here is just a subarch,
not an option to enable/disable the timer.

Give a try with:

make ARCH=arm multi_v7_defconfig menuconfig

--> System Type

That is what you are looking for, a SUPERH config option selecting all the
common options and then a JCORE config option adding the different missing
bits, namely the CLKSRC_JCORE_PIT.

> > The exception is to enable in order to compile on non-native platform to
> > compile-test a bunch of drivers (eg. compile most of the clocksource / 
> > clockevents drivers on a x86 big server).
> > 
> > That's why we should have:
> > 
> > config CLKSRC_JCORE_PIT
> > 	bool "J-Core PIT timer driver" if COMPILE_TEST
> > 
> > So the jcore pit driver option appears only if compile test is enabled
> > otherwise it is a silent option and the user doesn't have to deal with
> > it. Having consistent dependency like the other drivers will help future
> > changes to consolidate the Kconfig.
> 
> I don't think this matches the user expectation for the arch yet. For
> now we have a j2_defconfig that enables the drivers and other kernel
> settings expected to be useful for J-core socs. I want to modernize
> this all but that's a separate project.
> 
> > [ ... ]              
> > 
> > > +#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_pit {
> > > +	struct clock_event_device	ced;
> > > +	__iomem void			*base;
> > 
> > It is not '__iomem void *' but 'void __iomem *'. This appears multiple
> > times in this code.
> 
> OK, I'll change that.
> 
> > > +	unsigned long			periodic_delta;
> > > +	unsigned			cpu;
> > 
> > This field is pointless, 'cpu' is only used for a trace in the init
> > function which has already the cpu has parameter.
> 
> OK, will remove. It was needed for the old notify framework but not
> for cpu hotplug framework, I think.
> 
> > > +	u32				enable_val;
> > > +};
> > > +
> > > +static __iomem void		*jcore_pit_base;
> > 
> > static void __iomem *jcore_pit_base;
> 
> OK.
> 
> > > +struct jcore_pit __percpu	*jcore_pit_percpu;
> > 
> > static.
> 
> OK.
> 
> > [ ... ]
> > 
> > > +static int __init jcore_pit_init(struct device_node *node)
> > > +{
> > 
> > [ ... ]
> 
> Was there something analogous you wanted me to change here, or just
> leftover quoting?

No, I just cut until the function name in order to give the context for
the next comment and then cut inside the function.

> > > +	/*
> > > +	 * 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);
> > > +
> > 
> > Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
> > 
> > Will be the same information available if the irqchip is AIC1 ?
> 
> The bit layout of the PIT enable register is:
> 
> 	.....e..ppppiiiiiiii............
> 
> where the .'s indicate unrelated/unused bits, e is enable, p is
> priority, and i is hard irq number.
> 
> For the PIT included in AIC1 (obsolete but still in use), any hard irq
> (trap number) can be programmed via the 8 iiiiiiii bits, and a
> priority (0-15) is programmable separately in the pppp bits.
> 
> For the PIT included in AIC2 (current), the programming interface is
> equivalent modulo interrupt mapping. This is why a different
> compatible tag was not used. However only traps 64-127 (the ones
> actually intended to be used for interrupts, rather than
> syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are
> ignored) and the priority pppp is <<2'd and or'd onto the irq number.
> This was a poor decision made on the hardware engineering side based
> on a wrong assumption that preserving old priority mapping of outdated
> software was important, whereas priorities weren't/aren't even being
> used.
> 
> When we do the next round of interrupt controller improvements (AIC3)
> the PIT programming interface should remain compatible with the
> driver; likely the priority bits will just be ignored.
> 
> If we do want to change the programming interface beyond this at some
> point (that maay be a good idea, since we have identified several
> things that are less than ideal for Linux, like the sechi/seclo/ns
> clocksource), a new compatible tag will be added instead.

Ok, thanks for the clarification. Can you add your answer as a comment for
the bits dance above ?

> > I have to admin I find strange this driver has to invoke irq_get_irq_data(),
> > it is the only one and it sounds even strange it has to be stored per cpu below.
> 
> The timer will not actually generate the irq it's assigned to (or any
> interrupt at all) unless/until it's programmed for the (hard) irq
> number. The need to use irq_get_irq_data comes from the fact that the
> hardware needs the hard irq number, not a virq number, which could in
> principle be different.
> 
> There's probably some argument that could have been made that the
> irqchip and clocksource/timer driver should have been unified since
> they're unified in the hardware and have this awkward programming
> relationship, but that didn't fit the Linux model very well and I
> think having them factored like this encourages future versions of the
> hardware to adapt to the model the software wants.

Ok, thanks.

  -- Daniel

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

[Rich Felker]

On Wed, Oct 12, 2016 at 11:27:11AM +0200, Daniel Lezcano wrote:
> > > Are the CPUs on always-on power down ?
> > 
> > For now they are always on and don't even have the sleep instruction
> > (i.e. stop cpu clock until interrupt) implemented. Adding sleep will
> > be the first power-saving step, and perhaps the only one for now,
> > since there doesn't seem to be any indication (according to the ppl
> > working on the hardware) that a deeper sleep would provide significant
> > additional savings.
> 
> Ok.
> 
> However, the 'sleep' state is not, in the power management terminology,
> the idle state described above. It is called "clock gated" / "Wait for
> Interrupt".
> 
> The 'sleep' state lose the CPU context.

I use the term "sleep" because that's the name of the SH instruction
mnemonic for the opcode for entering the wait-for-interrupt state.
Sorry it's confusing like the "RTC" all over again. :-)

> > > > A nanosecond-resolution clocksource is provided using the J-Core "RTC"
> > > > registers, which give a 64-bit seconds count and 32-bit nanoseconds
> > > > that wrap every second. The driver converts these to a full-range
> > > > 32-bit nanoseconds count.
> > > > 
> > > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > > ---
> > > >  drivers/clocksource/Kconfig     |  10 ++
> > > >  drivers/clocksource/Makefile    |   1 +
> > > >  drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/cpuhotplug.h      |   1 +
> > > >  4 files changed, 243 insertions(+)
> > > >  create mode 100644 drivers/clocksource/jcore-pit.c
> > > > 
> > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > > index 5677886..95dd78b 100644
> > > > --- a/drivers/clocksource/Kconfig
> > > > +++ b/drivers/clocksource/Kconfig
> > > > @@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU
> > > >  config SYS_SUPPORTS_EM_STI
> > > >          bool
> > > >  
> > > > +config CLKSRC_JCORE_PIT
> > > > +	bool "J-Core PIT timer driver"
> > > > +	depends on OF && (SUPERH || COMPILE_TEST)
> > > 
> > > Actually the idea is to have the SUPERH to select this timer, not create
> > > a dependency on SUPERH from here.
> > > 
> > > We don't want to prompt in the configuration menu the drivers because it
> > > would be impossible to anyone to know which timer comes with which
> > > hardware, so we let the platform to select the timer it needs.
> > 
> > I thought we discussed this before. For users building a kernel for
> > legacy SH systems, especially in the current state where they're only
> > supported with hard-coded board files rather than device tree, it
> > makes no sense to build drivers for J-core hardware. It would make
> > sense to be on by default for CONFIG_SH_DEVICE_TREE with a compatible
> > CPU selection, but at least at this time, not for SUPERH in general.
> 
> Probably I am missing the point but why the user would have to unselect
> this driver manually ? The user wants a config file nothing more or a very
> trivial option. Can you imagine someone can know every single IP block for
> each boards of the same arch and be able to disable/enable the right ones ?

The common case I imagine is just accepting defaults that include more
than you need, but for space-constrained (or more likely, fast boot,
when the kernel is being loaded from a slow spi-based SD card
interface) setups the user might be trying to minimize kernel size and
turn off drivers they know they don't want. I'm not so worried about
this driver, specifically, because it's small, but I am concerned
about the general policy -- when we get rid of all the legacy board
files and everything is move to device tree, will the user be stuck
including a bunch of Renesas SH drivers when building a kernel the
intend to use only on J-core?

> > Anyway I'd really like to do this non-invasively as long as we have a
> > mix of legacy and new stuff and the legacy stuff is not readily
> > testable. Once all of arch/sh is moved over to device tree, could we
> > revisit this and make all the drivers follow a common policy (on by
> > default if they're associated with boards/SoCs using a matching or
> > compatible CPU model, or something like that, but still able to be
> > disabled manually, since the user might be trying to get a tiny-ish
> > embedded kernel)?
> 
> I understand the goal is to have one single configuration and everything
> DT based and it sounds great but what is missing here is just a subarch,
> not an option to enable/disable the timer.
> 
> Give a try with:
> 
> make ARCH=arm multi_v7_defconfig menuconfig
> 
> --> System Type
> 
> That is what you are looking for, a SUPERH config option selecting all the
> common options and then a JCORE config option adding the different missing
> bits, namely the CLKSRC_JCORE_PIT.

We do have something like "system type" in arch/sh, and it's what I'm
trying to deprecate since it's the switch to select between all the
hard-coded board files, _or_ device tree.

Since part of the goal of my DT overhaul is to be able (but not
forced) to produce kernels that run on a wide range of hardware,
rather than having a "system type (select one)" option, what about
individual boolean options like:

config JCORE_SOC
	bool "Support for J-Core SoCs"
	select CLKSRC_JCORE_PIT
	select JCORE_AIC
	...

Note that there are other drivers that should probably be optional
even if you have JCORE_SOC enabled, like the SPI controller, DMA
controller (not implemented yet), Ethernet (not submitted upstream
yet), etc. Maybe they could depend on JCORE_SOC and be default-yes but
configurable if available?

In any case, the SoC support is supposedly there in the current kernel
release (4.8) but not working because of missing essential drivers, so
I'd really like to fix that without making the fix dependent on
restructuring the arch/sh system type handling, which is an ongoing,
independent project for which I'm waiting for help converting and
testing the conversions of legacy board support. My preference would
be to keep the Kconfig stuff the way I submitted it for now --
j2_defconfig already handles enabling thse right drivers -- and do
something more user-friendly as part of the bigger arch overhaul
project.

> > > > +	/*
> > > > +	 * 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);
> > > > +
> > > 
> > > Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
> > > 
> > > Will be the same information available if the irqchip is AIC1 ?
> > 
> > The bit layout of the PIT enable register is:
> > 
> > 	.....e..ppppiiiiiiii............
> > 
> > where the .'s indicate unrelated/unused bits, e is enable, p is
> > priority, and i is hard irq number.
> > 
> > For the PIT included in AIC1 (obsolete but still in use), any hard irq
> > (trap number) can be programmed via the 8 iiiiiiii bits, and a
> > priority (0-15) is programmable separately in the pppp bits.
> > 
> > For the PIT included in AIC2 (current), the programming interface is
> > equivalent modulo interrupt mapping. This is why a different
> > compatible tag was not used. However only traps 64-127 (the ones
> > actually intended to be used for interrupts, rather than
> > syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are
> > ignored) and the priority pppp is <<2'd and or'd onto the irq number.
> > This was a poor decision made on the hardware engineering side based
> > on a wrong assumption that preserving old priority mapping of outdated
> > software was important, whereas priorities weren't/aren't even being
> > used.
> > 
> > When we do the next round of interrupt controller improvements (AIC3)
> > the PIT programming interface should remain compatible with the
> > driver; likely the priority bits will just be ignored.
> > 
> > If we do want to change the programming interface beyond this at some
> > point (that maay be a good idea, since we have identified several
> > things that are less than ideal for Linux, like the sechi/seclo/ns
> > clocksource), a new compatible tag will be added instead.
> 
> Ok, thanks for the clarification. Can you add your answer as a comment for
> the bits dance above ?

Are you happy with the whole quoted text above as a comment? If so I'm
happy to include it verbatim. I would lean towards condensing or
omitting the last 2 paragraphs (starting with "When we do...") if
that's okay with you since they are not documenting the hw but future
plans/policy.

Rich

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

[Daniel Lezcano]

On Wed, Oct 12, 2016 at 01:02:36PM -0400, Rich Felker wrote:
> On Wed, Oct 12, 2016 at 11:27:11AM +0200, Daniel Lezcano wrote:
> > > > Are the CPUs on always-on power down ?
> > > 
> > > For now they are always on and don't even have the sleep instruction
> > > (i.e. stop cpu clock until interrupt) implemented. Adding sleep will
> > > be the first power-saving step, and perhaps the only one for now,
> > > since there doesn't seem to be any indication (according to the ppl
> > > working on the hardware) that a deeper sleep would provide significant
> > > additional savings.
> > 
> > Ok.
> > 
> > However, the 'sleep' state is not, in the power management terminology,
> > the idle state described above. It is called "clock gated" / "Wait for
> > Interrupt".
> > 
> > The 'sleep' state lose the CPU context.
> 
> I use the term "sleep" because that's the name of the SH instruction
> mnemonic for the opcode for entering the wait-for-interrupt state.
> Sorry it's confusing like the "RTC" all over again. :-)
> 
> > > > > A nanosecond-resolution clocksource is provided using the J-Core "RTC"
> > > > > registers, which give a 64-bit seconds count and 32-bit nanoseconds
> > > > > that wrap every second. The driver converts these to a full-range
> > > > > 32-bit nanoseconds count.
> > > > > 
> > > > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > > > ---
> > > > >  drivers/clocksource/Kconfig     |  10 ++
> > > > >  drivers/clocksource/Makefile    |   1 +
> > > > >  drivers/clocksource/jcore-pit.c | 231 ++++++++++++++++++++++++++++++++++++++++
> > > > >  include/linux/cpuhotplug.h      |   1 +
> > > > >  4 files changed, 243 insertions(+)
> > > > >  create mode 100644 drivers/clocksource/jcore-pit.c
> > > > > 
> > > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > > > index 5677886..95dd78b 100644
> > > > > --- a/drivers/clocksource/Kconfig
> > > > > +++ b/drivers/clocksource/Kconfig
> > > > > @@ -407,6 +407,16 @@ config SYS_SUPPORTS_SH_TMU
> > > > >  config SYS_SUPPORTS_EM_STI
> > > > >          bool
> > > > >  
> > > > > +config CLKSRC_JCORE_PIT
> > > > > +	bool "J-Core PIT timer driver"
> > > > > +	depends on OF && (SUPERH || COMPILE_TEST)
> > > > 
> > > > Actually the idea is to have the SUPERH to select this timer, not create
> > > > a dependency on SUPERH from here.
> > > > 
> > > > We don't want to prompt in the configuration menu the drivers because it
> > > > would be impossible to anyone to know which timer comes with which
> > > > hardware, so we let the platform to select the timer it needs.
> > > 
> > > I thought we discussed this before. For users building a kernel for
> > > legacy SH systems, especially in the current state where they're only
> > > supported with hard-coded board files rather than device tree, it
> > > makes no sense to build drivers for J-core hardware. It would make
> > > sense to be on by default for CONFIG_SH_DEVICE_TREE with a compatible
> > > CPU selection, but at least at this time, not for SUPERH in general.
> > 
> > Probably I am missing the point but why the user would have to unselect
> > this driver manually ? The user wants a config file nothing more or a very
> > trivial option. Can you imagine someone can know every single IP block for
> > each boards of the same arch and be able to disable/enable the right ones ?
> 
> The common case I imagine is just accepting defaults that include more
> than you need, but for space-constrained (or more likely, fast boot,
> when the kernel is being loaded from a slow spi-based SD card
> interface) setups the user might be trying to minimize kernel size and
> turn off drivers they know they don't want. I'm not so worried about
> this driver, specifically, because it's small, but I am concerned
> about the general policy -- when we get rid of all the legacy board
> files and everything is move to device tree, will the user be stuck
> including a bunch of Renesas SH drivers when building a kernel the
> intend to use only on J-core?
> 
> > > Anyway I'd really like to do this non-invasively as long as we have a
> > > mix of legacy and new stuff and the legacy stuff is not readily
> > > testable. Once all of arch/sh is moved over to device tree, could we
> > > revisit this and make all the drivers follow a common policy (on by
> > > default if they're associated with boards/SoCs using a matching or
> > > compatible CPU model, or something like that, but still able to be
> > > disabled manually, since the user might be trying to get a tiny-ish
> > > embedded kernel)?
> > 
> > I understand the goal is to have one single configuration and everything
> > DT based and it sounds great but what is missing here is just a subarch,
> > not an option to enable/disable the timer.
> > 
> > Give a try with:
> > 
> > make ARCH=arm multi_v7_defconfig menuconfig
> > 
> > --> System Type
> > 
> > That is what you are looking for, a SUPERH config option selecting all the
> > common options and then a JCORE config option adding the different missing
> > bits, namely the CLKSRC_JCORE_PIT.
> 
> We do have something like "system type" in arch/sh, and it's what I'm
> trying to deprecate since it's the switch to select between all the
> hard-coded board files, _or_ device tree.
> 
> Since part of the goal of my DT overhaul is to be able (but not
> forced) to produce kernels that run on a wide range of hardware,
> rather than having a "system type (select one)" option, what about
> individual boolean options like:
> 
> config JCORE_SOC
> 	bool "Support for J-Core SoCs"
> 	select CLKSRC_JCORE_PIT
> 	select JCORE_AIC
> 	...

I'm perfectly fine with this.
 
> Note that there are other drivers that should probably be optional
> even if you have JCORE_SOC enabled, like the SPI controller, DMA
> controller (not implemented yet), Ethernet (not submitted upstream
> yet), etc. Maybe they could depend on JCORE_SOC and be default-yes but
> configurable if available?

That sounds fine also.

> In any case, the SoC support is supposedly there in the current kernel
> release (4.8) but not working because of missing essential drivers, so
> I'd really like to fix that without making the fix dependent on
> restructuring the arch/sh system type handling, which is an ongoing,
> independent project for which I'm waiting for help converting and
> testing the conversions of legacy board support. My preference would
> be to keep the Kconfig stuff the way I submitted it for now --
> j2_defconfig already handles enabling thse right drivers -- and do
> something more user-friendly as part of the bigger arch overhaul
> project.

I prefer the move the option to config JCORE_SOC. That is not a big deal
to add this bool in the sh's Kconfig and select the timer from there.
 
> > > > > +	/*
> > > > > +	 * 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);
> > > > > +
> > > > 
> > > > Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
> > > > 
> > > > Will be the same information available if the irqchip is AIC1 ?
> > > 
> > > The bit layout of the PIT enable register is:
> > > 
> > > 	.....e..ppppiiiiiiii............
> > > 
> > > where the .'s indicate unrelated/unused bits, e is enable, p is
> > > priority, and i is hard irq number.
> > > 
> > > For the PIT included in AIC1 (obsolete but still in use), any hard irq
> > > (trap number) can be programmed via the 8 iiiiiiii bits, and a
> > > priority (0-15) is programmable separately in the pppp bits.
> > > 
> > > For the PIT included in AIC2 (current), the programming interface is
> > > equivalent modulo interrupt mapping. This is why a different
> > > compatible tag was not used. However only traps 64-127 (the ones
> > > actually intended to be used for interrupts, rather than
> > > syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are
> > > ignored) and the priority pppp is <<2'd and or'd onto the irq number.
> > > This was a poor decision made on the hardware engineering side based
> > > on a wrong assumption that preserving old priority mapping of outdated
> > > software was important, whereas priorities weren't/aren't even being
> > > used.
> > > 
> > > When we do the next round of interrupt controller improvements (AIC3)
> > > the PIT programming interface should remain compatible with the
> > > driver; likely the priority bits will just be ignored.
> > > 
> > > If we do want to change the programming interface beyond this at some
> > > point (that maay be a good idea, since we have identified several
> > > things that are less than ideal for Linux, like the sechi/seclo/ns
> > > clocksource), a new compatible tag will be added instead.
> > 
> > Ok, thanks for the clarification. Can you add your answer as a comment for
> > the bits dance above ?
> 
> Are you happy with the whole quoted text above as a comment? If so I'm
> happy to include it verbatim. I would lean towards condensing or
> omitting the last 2 paragraphs (starting with "When we do...") if
> that's okay with you since they are not documenting the hw but future
> plans/policy.

Makes sense.

Agree for the verbatim minus the last 2 paragraphs.

  -- Daniel

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

[Rich Felker]

On Wed, Oct 12, 2016 at 11:31:26PM +0200, Daniel Lezcano wrote:
> > > I understand the goal is to have one single configuration and everything
> > > DT based and it sounds great but what is missing here is just a subarch,
> > > not an option to enable/disable the timer.
> > > 
> > > Give a try with:
> > > 
> > > make ARCH=arm multi_v7_defconfig menuconfig
> > > 
> > > --> System Type
> > > 
> > > That is what you are looking for, a SUPERH config option selecting all the
> > > common options and then a JCORE config option adding the different missing
> > > bits, namely the CLKSRC_JCORE_PIT.
> > 
> > We do have something like "system type" in arch/sh, and it's what I'm
> > trying to deprecate since it's the switch to select between all the
> > hard-coded board files, _or_ device tree.
> > 
> > Since part of the goal of my DT overhaul is to be able (but not
> > forced) to produce kernels that run on a wide range of hardware,
> > rather than having a "system type (select one)" option, what about
> > individual boolean options like:
> > 
> > config JCORE_SOC
> > 	bool "Support for J-Core SoCs"
> > 	select CLKSRC_JCORE_PIT
> > 	select JCORE_AIC
> > 	...
> 
> I'm perfectly fine with this.
>  
> > Note that there are other drivers that should probably be optional
> > even if you have JCORE_SOC enabled, like the SPI controller, DMA
> > controller (not implemented yet), Ethernet (not submitted upstream
> > yet), etc. Maybe they could depend on JCORE_SOC and be default-yes but
> > configurable if available?
> 
> That sounds fine also.
> 
> > In any case, the SoC support is supposedly there in the current kernel
> > release (4.8) but not working because of missing essential drivers, so
> > I'd really like to fix that without making the fix dependent on
> > restructuring the arch/sh system type handling, which is an ongoing,
> > independent project for which I'm waiting for help converting and
> > testing the conversions of legacy board support. My preference would
> > be to keep the Kconfig stuff the way I submitted it for now --
> > j2_defconfig already handles enabling thse right drivers -- and do
> > something more user-friendly as part of the bigger arch overhaul
> > project.
> 
> I prefer the move the option to config JCORE_SOC. That is not a big deal
> to add this bool in the sh's Kconfig and select the timer from there.

OK, I'll do this and add the patch to my planned pull request, and
send a corresponding Kconfig patch for the interrupt controller.

> > > > > > +	/*
> > > > > > +	 * 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);
> > > > > > +
> > > > > 
> > > > > Why mention AIC1 if there is not test to check if AIC1 || AIC2 ?
> > > > > 
> > > > > Will be the same information available if the irqchip is AIC1 ?
> > > > 
> > > > The bit layout of the PIT enable register is:
> > > > 
> > > > 	.....e..ppppiiiiiiii............
> > > > 
> > > > where the .'s indicate unrelated/unused bits, e is enable, p is
> > > > priority, and i is hard irq number.
> > > > 
> > > > For the PIT included in AIC1 (obsolete but still in use), any hard irq
> > > > (trap number) can be programmed via the 8 iiiiiiii bits, and a
> > > > priority (0-15) is programmable separately in the pppp bits.
> > > > 
> > > > For the PIT included in AIC2 (current), the programming interface is
> > > > equivalent modulo interrupt mapping. This is why a different
> > > > compatible tag was not used. However only traps 64-127 (the ones
> > > > actually intended to be used for interrupts, rather than
> > > > syscalls/exceptions/etc.) can be programmed (the high 2 bits of i are
> > > > ignored) and the priority pppp is <<2'd and or'd onto the irq number.
> > > > This was a poor decision made on the hardware engineering side based
> > > > on a wrong assumption that preserving old priority mapping of outdated
> > > > software was important, whereas priorities weren't/aren't even being
> > > > used.
> > > > 
> > > > When we do the next round of interrupt controller improvements (AIC3)
> > > > the PIT programming interface should remain compatible with the
> > > > driver; likely the priority bits will just be ignored.
> > > > 
> > > > If we do want to change the programming interface beyond this at some
> > > > point (that maay be a good idea, since we have identified several
> > > > things that are less than ideal for Linux, like the sechi/seclo/ns
> > > > clocksource), a new compatible tag will be added instead.
> > > 
> > > Ok, thanks for the clarification. Can you add your answer as a comment for
> > > the bits dance above ?
> > 
> > Are you happy with the whole quoted text above as a comment? If so I'm
> > happy to include it verbatim. I would lean towards condensing or
> > omitting the last 2 paragraphs (starting with "When we do...") if
> > that's okay with you since they are not documenting the hw but future
> > plans/policy.
> 
> Makes sense.
> 
> Agree for the verbatim minus the last 2 paragraphs.

OK. I'll prepare a new patch with this and the previously-discussed
changes.

Rich

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

[Rich Felker]

On Wed, Oct 12, 2016 at 11:31:26PM +0200, Daniel Lezcano wrote:
> > > --> System Type
> > > 
> > > That is what you are looking for, a SUPERH config option selecting all the
> > > common options and then a JCORE config option adding the different missing
> > > bits, namely the CLKSRC_JCORE_PIT.
> > 
> > We do have something like "system type" in arch/sh, and it's what I'm
> > trying to deprecate since it's the switch to select between all the
> > hard-coded board files, _or_ device tree.
> > 
> > Since part of the goal of my DT overhaul is to be able (but not
> > forced) to produce kernels that run on a wide range of hardware,
> > rather than having a "system type (select one)" option, what about
> > individual boolean options like:
> > 
> > config JCORE_SOC
> > 	bool "Support for J-Core SoCs"
> > 	select CLKSRC_JCORE_PIT
> > 	select JCORE_AIC
> > 	...
> 
> I'm perfectly fine with this.

Does this adequately ensure that dependencies for the clocksource and
irq driver are met? If not, what else do I need to do?

Rich

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

[Rich Felker]

On Thu, Oct 13, 2016 at 03:25:42PM -0400, Rich Felker wrote:
> On Wed, Oct 12, 2016 at 11:31:26PM +0200, Daniel Lezcano wrote:
> > > > --> System Type
> > > > 
> > > > That is what you are looking for, a SUPERH config option selecting all the
> > > > common options and then a JCORE config option adding the different missing
> > > > bits, namely the CLKSRC_JCORE_PIT.
> > > 
> > > We do have something like "system type" in arch/sh, and it's what I'm
> > > trying to deprecate since it's the switch to select between all the
> > > hard-coded board files, _or_ device tree.
> > > 
> > > Since part of the goal of my DT overhaul is to be able (but not
> > > forced) to produce kernels that run on a wide range of hardware,
> > > rather than having a "system type (select one)" option, what about
> > > individual boolean options like:
> > > 
> > > config JCORE_SOC
> > > 	bool "Support for J-Core SoCs"
> > > 	select CLKSRC_JCORE_PIT
> > > 	select JCORE_AIC
> > > 	...
> > 
> > I'm perfectly fine with this.
> 
> Does this adequately ensure that dependencies for the clocksource and
> irq driver are met? If not, what else do I need to do?

I've checked and it seems the dependencies are always met for arch/sh,
at least with device tree enabled (on which JCORE_SOC will depend), so
it shouldn't matter.

Rich
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rich Felker]

Signed-off-by: Rich Felker <dalias-8zAoT0mYgF4@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/timer/jcore,pit.txt        | 24 ++++++++++++++++++++++
 1 file changed, 24 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..af5dd35
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/jcore,pit.txt
@@ -0,0 +1,24 @@
+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.
+
+- 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.10.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html