From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966107AbcHDTm4 (ORCPT ); Thu, 4 Aug 2016 15:42:56 -0400 Received: from 216-12-86-13.cv.mvl.ntelos.net ([216.12.86.13]:59584 "EHLO brightrain.aerifal.cx" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965309AbcHDTmw (ORCPT ); Thu, 4 Aug 2016 15:42:52 -0400 Date: Thu, 4 Aug 2016 15:42:39 -0400 From: Rich Felker To: Alexnader Kuleshov Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, Rob Herring , Mark Rutland , Daniel Lezcano , Thomas Gleixner Subject: Re: [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver Message-ID: <20160804194238.GL15995@brightrain.aerifal.cx> References: <20160804082451.GA2509@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160804082451.GA2509@localhost> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 04, 2016 at 02:24:51PM +0600, Alexnader Kuleshov wrote: > Hello Rich, > > On 08-04-16, 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. 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 > > ... > > ... > > ... > > + > > +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: > > > 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. > > +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