From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754774AbbFRMXu (ORCPT ); Thu, 18 Jun 2015 08:23:50 -0400 Received: from mail-pd0-f169.google.com ([209.85.192.169]:36613 "EHLO mail-pd0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752794AbbFRMXl (ORCPT ); Thu, 18 Jun 2015 08:23:41 -0400 Date: Thu, 18 Jun 2015 17:53:36 +0530 From: Viresh Kumar To: Maxime Ripard Cc: Thomas Gleixner , Daniel Lezcano , linaro-kernel@lists.linaro.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 23/41] clocksource: sun4i: Migrate to new 'set-state' interface Message-ID: <20150618122336.GB1982@linux> References: <20150618120105.GM11732@lukather> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150618120105.GM11732@lukather> 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 18-06-15, 14:01, Maxime Ripard wrote: > On Thu, Jun 18, 2015 at 04:24:37PM +0530, Viresh Kumar wrote: > > +static int sun4i_clkevt_shutdown(struct clock_event_device *evt) > > { > > - switch (mode) { > > - case CLOCK_EVT_MODE_PERIODIC: > > - sun4i_clkevt_time_stop(0); > > - sun4i_clkevt_time_setup(0, ticks_per_jiffy); > > - sun4i_clkevt_time_start(0, true); > > - break; > > - case CLOCK_EVT_MODE_ONESHOT: > > - sun4i_clkevt_time_stop(0); > > - sun4i_clkevt_time_start(0, false); > > - break; > > - case CLOCK_EVT_MODE_UNUSED: > > - case CLOCK_EVT_MODE_SHUTDOWN: > > - default: > > - sun4i_clkevt_time_stop(0); > > - break; Because sun4i_clkevt_time_stop() is getting called in default case, it is getting called for tick-resume mode already with the old set_mode interface. > > + .tick_resume = sun4i_clkevt_shutdown, > > I'm not exactly sure of the context here, but I wouldn't expect a > callback called tick_resume to stop a timer. Is this expected? And so this patch carried the same logic here. At suspend: clockevents core calls ->set_state_shutdown() and at resume it calls ->tick_resume() followed by setting to the proper mode, i.e. periodic or oneshot. Many driver authors didn't knew about these details and so did shutdown in resume path as well. For me, you might not even need a tick_resume() at all, as your driver would have already shutted down on suspend and is just required to be put to the right mode again. But, I didn't wanted to change the way things behaved until now. I can add another patch to get things fixed separately if you want me to. -- viresh From mboxrd@z Thu Jan 1 00:00:00 1970 From: viresh.kumar@linaro.org (Viresh Kumar) Date: Thu, 18 Jun 2015 17:53:36 +0530 Subject: [PATCH 23/41] clocksource: sun4i: Migrate to new 'set-state' interface In-Reply-To: <20150618120105.GM11732@lukather> References: <20150618120105.GM11732@lukather> Message-ID: <20150618122336.GB1982@linux> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 18-06-15, 14:01, Maxime Ripard wrote: > On Thu, Jun 18, 2015 at 04:24:37PM +0530, Viresh Kumar wrote: > > +static int sun4i_clkevt_shutdown(struct clock_event_device *evt) > > { > > - switch (mode) { > > - case CLOCK_EVT_MODE_PERIODIC: > > - sun4i_clkevt_time_stop(0); > > - sun4i_clkevt_time_setup(0, ticks_per_jiffy); > > - sun4i_clkevt_time_start(0, true); > > - break; > > - case CLOCK_EVT_MODE_ONESHOT: > > - sun4i_clkevt_time_stop(0); > > - sun4i_clkevt_time_start(0, false); > > - break; > > - case CLOCK_EVT_MODE_UNUSED: > > - case CLOCK_EVT_MODE_SHUTDOWN: > > - default: > > - sun4i_clkevt_time_stop(0); > > - break; Because sun4i_clkevt_time_stop() is getting called in default case, it is getting called for tick-resume mode already with the old set_mode interface. > > + .tick_resume = sun4i_clkevt_shutdown, > > I'm not exactly sure of the context here, but I wouldn't expect a > callback called tick_resume to stop a timer. Is this expected? And so this patch carried the same logic here. At suspend: clockevents core calls ->set_state_shutdown() and at resume it calls ->tick_resume() followed by setting to the proper mode, i.e. periodic or oneshot. Many driver authors didn't knew about these details and so did shutdown in resume path as well. For me, you might not even need a tick_resume() at all, as your driver would have already shutted down on suspend and is just required to be put to the right mode again. But, I didn't wanted to change the way things behaved until now. I can add another patch to get things fixed separately if you want me to. -- viresh