From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54935) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZBWdL-0002Xd-KS for qemu-devel@nongnu.org; Sat, 04 Jul 2015 19:14:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZBWdI-0002zO-F0 for qemu-devel@nongnu.org; Sat, 04 Jul 2015 19:14:07 -0400 Received: from mail-la0-x22c.google.com ([2a00:1450:4010:c03::22c]:34727) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZBWdI-0002zE-6S for qemu-devel@nongnu.org; Sat, 04 Jul 2015 19:14:04 -0400 Received: by lagx9 with SMTP id x9so118289666lag.1 for ; Sat, 04 Jul 2015 16:14:03 -0700 (PDT) Message-ID: <559868B5.3020006@gmail.com> Date: Sun, 05 Jul 2015 02:13:57 +0300 From: Dmitry Osipenko MIME-Version: 1.0 References: <1435877531-24983-1-git-send-email-digetx@gmail.com> <1435877531-24983-3-git-send-email-digetx@gmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/3] arm_mptimer: Fix ONE-SHOT -> PERIODIC mode change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Peter Maydell Cc: Paolo Bonzini , QEMU Developers 04.07.2015 22:36, Peter Crosthwaite пишет: > > Your do-nothing code paths are now inconsistent between the 0 and 1 > cases. I think this if can be consolidated with: > > if (value & 1) { > if ((old & 1) && (tb->count != 0)) { > break; > } > if (tb->control & 2) { > ... > } > tb_reload(); > } else if (old & 1) { > timer_del(); > } > break; > Code, you are suggesting, is logically equal to my patch. Your variant looks cleaner, I'll switch to it. Thanks. > You have dropped the tb->count == 0 which looks like another bugfix > again. It looks like you are making sure the load value is loaded > regardless of timer run-state. If this is correct it just needs a note > in commit message. > Sorry, I don't see where tb->count == 0 got dropped. I think you missed that timerblock_reload() checks for tb->count == 0. To clarify: Timer is stopped in two cases: 1) timer was shutdown; (old & 1) == 0, tb->count may be unequal to 0 (if timer was shutdown while it was ticking). 2) one-shot was completed; (old & 1) == 1, tb->count = 0 On timer enabling we don't need to do anything if either one-shot or periodic count is currently in fly, because timerblock_tick() would correctly handle mode change, otherwise we re-load tb->count in case of periodic mode or starting with whatever(left after periodic shutdown or after loading 'count' via load register) tb->count in case of one-shot. BTW, timer pausing is not implemented and timer can be in two states only: running and stopped. This leads to opportunity to convert arm_mptimer to use ptimer, I'd like to do it after fixing current issues. > I think it's ok to squash patches 1 and 2. and just make a note in the > commit message of the multiple issues. It's hard to split this without > churning. > Well... yeah, it may worth squashing them. I'll do it. Thanks for review. -- Dmitry