From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752307AbdBZO4p (ORCPT ); Sun, 26 Feb 2017 09:56:45 -0500 Received: from mail-wm0-f42.google.com ([74.125.82.42]:32883 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752212AbdBZO4n (ORCPT ); Sun, 26 Feb 2017 09:56:43 -0500 Date: Sun, 26 Feb 2017 15:56:31 +0100 From: Daniel Lezcano To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: arm@kernel.org, linux-arm-kernel@lists.infradead.org, mp-cs@actions-semi.com, 96boards@ucrobotics.com, support@lemaker.org, linux-kernel@vger.kernel.org, Thomas Gleixner Subject: Re: [PATCH v2 04/17] clocksource: Add Owl timer Message-ID: <20170226145631.GB30601@mai> References: <20170224034055.18807-1-afaerber@suse.de> <20170224034055.18807-5-afaerber@suse.de> <20170224222921.GA23152@mai> <20170225215941.GA30601@mai> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 26, 2017 at 03:40:49PM +0100, Andreas Färber wrote: > Am 25.02.2017 um 22:59 schrieb Daniel Lezcano: > > On Sat, Feb 25, 2017 at 12:25:32AM +0100, Andreas Färber wrote: > >> Am 24.02.2017 um 23:29 schrieb Daniel Lezcano: > >>> On Fri, Feb 24, 2017 at 04:40:42AM +0100, Andreas Färber wrote: > >>>> +static struct clock_event_device owl_clockevent = { > >>>> + .name = "owl_tick", > >>>> + .rating = 200, > >>>> + .features = CLOCK_EVT_FEAT_ONESHOT, > >>> > >>> Did you consider adding CLOCK_EVT_FEAT_DYNIRQ ? > >> > >> No, it was not present downstream. Got a good example? > > > > https://lwn.net/Articles/541000/ > > Looking at your current Nomadik code, it seems I can literally should > just add this flag (done), without needing to implement any new hooks. > > On a related topic, how do we determine the cpumask? Downstream and some > in-tree drivers use cpumask_of(0), others use cpu_possible_mask. If you specify the CLOCK_EVT_FEAT_DYNIRQ, the cpumask is not important as it will be changed dynamically. Otherwise, cpumask_of(0) is often the default because it concentrates the wakeup on a single cpu, allowing the other cpus to go to deep idle state and if there are two clusters, it allows to have a cluster idle state. That results on a better energy saving. The usage of cpu_possible_mask will randomly wakeup any cpu. > >> Do you spot anything functionally wrong in this driver? Despite adding > >> this new driver, I am only getting the following additional earlycon output: > >> > >> [ 0.000029] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps > >> every 89478484971ns > >> [ 0.007888] clocksource: timer: mask: 0xffffffff max_cycles: > >> 0xffffffff, max_idle_ns: 79635851949 ns > >> [ 0.017748] Console: colour dummy device 80x30 > >> [ 0.022243] Calibrating delay loop... > >> [ 0.030895] random: fast init done > >> [ 0.231021] random: crng init done > >> > >> For S900 I'm using the generic timer instead. > > > > I don't get the issue, can you elaborate ? > > Found it myself: I forgot to clear the interrupt pending bit in the > interrupt handler routine. > > + writel(OWL_Tx_CTL_PD, owl_timer_base + OWL_T1_CTL); > > Now it goes past this point, initializes the real serial driver and > boots up to not finding the rootfs: > > [ 0.000032] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps > every 89478484971ns > [ 0.007898] clocksource: timer: mask: 0xffffffff max_cycles: > 0xffffffff, max_idle_ns: 79635851949 ns > [ 0.017886] Console: colour dummy device 80x30 > [ 0.022386] Calibrating delay loop... 405.50 BogoMIPS (lpj=2027520) May be you should also consider using register_current_timer_delay() instead of jiffies based delay loops. > [ 0.083523] pid_max: default: 32768 minimum: 301 -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel.lezcano@linaro.org (Daniel Lezcano) Date: Sun, 26 Feb 2017 15:56:31 +0100 Subject: [PATCH v2 04/17] clocksource: Add Owl timer In-Reply-To: References: <20170224034055.18807-1-afaerber@suse.de> <20170224034055.18807-5-afaerber@suse.de> <20170224222921.GA23152@mai> <20170225215941.GA30601@mai> Message-ID: <20170226145631.GB30601@mai> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sun, Feb 26, 2017 at 03:40:49PM +0100, Andreas F?rber wrote: > Am 25.02.2017 um 22:59 schrieb Daniel Lezcano: > > On Sat, Feb 25, 2017 at 12:25:32AM +0100, Andreas F?rber wrote: > >> Am 24.02.2017 um 23:29 schrieb Daniel Lezcano: > >>> On Fri, Feb 24, 2017 at 04:40:42AM +0100, Andreas F?rber wrote: > >>>> +static struct clock_event_device owl_clockevent = { > >>>> + .name = "owl_tick", > >>>> + .rating = 200, > >>>> + .features = CLOCK_EVT_FEAT_ONESHOT, > >>> > >>> Did you consider adding CLOCK_EVT_FEAT_DYNIRQ ? > >> > >> No, it was not present downstream. Got a good example? > > > > https://lwn.net/Articles/541000/ > > Looking at your current Nomadik code, it seems I can literally should > just add this flag (done), without needing to implement any new hooks. > > On a related topic, how do we determine the cpumask? Downstream and some > in-tree drivers use cpumask_of(0), others use cpu_possible_mask. If you specify the CLOCK_EVT_FEAT_DYNIRQ, the cpumask is not important as it will be changed dynamically. Otherwise, cpumask_of(0) is often the default because it concentrates the wakeup on a single cpu, allowing the other cpus to go to deep idle state and if there are two clusters, it allows to have a cluster idle state. That results on a better energy saving. The usage of cpu_possible_mask will randomly wakeup any cpu. > >> Do you spot anything functionally wrong in this driver? Despite adding > >> this new driver, I am only getting the following additional earlycon output: > >> > >> [ 0.000029] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps > >> every 89478484971ns > >> [ 0.007888] clocksource: timer: mask: 0xffffffff max_cycles: > >> 0xffffffff, max_idle_ns: 79635851949 ns > >> [ 0.017748] Console: colour dummy device 80x30 > >> [ 0.022243] Calibrating delay loop... > >> [ 0.030895] random: fast init done > >> [ 0.231021] random: crng init done > >> > >> For S900 I'm using the generic timer instead. > > > > I don't get the issue, can you elaborate ? > > Found it myself: I forgot to clear the interrupt pending bit in the > interrupt handler routine. > > + writel(OWL_Tx_CTL_PD, owl_timer_base + OWL_T1_CTL); > > Now it goes past this point, initializes the real serial driver and > boots up to not finding the rootfs: > > [ 0.000032] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps > every 89478484971ns > [ 0.007898] clocksource: timer: mask: 0xffffffff max_cycles: > 0xffffffff, max_idle_ns: 79635851949 ns > [ 0.017886] Console: colour dummy device 80x30 > [ 0.022386] Calibrating delay loop... 405.50 BogoMIPS (lpj=2027520) May be you should also consider using register_current_timer_delay() instead of jiffies based delay loops. > [ 0.083523] pid_max: default: 32768 minimum: 301 -- Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog