On Thu, Aug 04, 2016 at 02:24:51PM +0600, Alexnader Kuleshov wrote:

Hello Rich,

On 08-04-16, Rich Felker wrote:

quoted

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>
...
...
...
+
+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);
+}

Maybe to use only jcore_pit_set_state_shutdown() for both shutdown/oneshot
states as it is implemented for some of others clocksource drivers?

all the more so as described in your commit message, they do the same:

quoted

in oneshot mode the interrupt handler simply disables the timer
as soon as it fires

right?

I separated these out semantically due to another request earlier in
this patch's life cycle. At this point I feel like issues like this
are really a bikeshed, and rather than changing trivial details back
and forth over and over I'd like to see it go upstream so that I don't
have to keep rebasing it on infrastructure that's changing underneath
it.

quoted

+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);
+
+	jcore_cs.name = "jcore_pit_cs";
+	jcore_cs.rating = 400;
+	jcore_cs.read = jcore_clocksource_read;
+	jcore_cs.mult = 1;
+	jcore_cs.shift = 0;
+	jcore_cs.mask = CLOCKSOURCE_MASK(32);
+	jcore_cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+	err = clocksource_register_hz(&jcore_cs, NSEC_PER_SEC);
+	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);
+		return err;
+	}

free_percpu() missed in a case when request_irq() failed.

Shall I submit a new version with this change? I don't think it's
particular useful since you're not going to have a working system if
the timer fails to initialize anyway, but I can do it if desired.

Rich