From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51621) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAjSm-0007ZM-SQ for qemu-devel@nongnu.org; Thu, 02 Jul 2015 14:43:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAjSj-00034d-Mx for qemu-devel@nongnu.org; Thu, 02 Jul 2015 14:43:56 -0400 Received: from mail-la0-x236.google.com ([2a00:1450:4010:c03::236]:34577) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAjSj-00034C-E9 for qemu-devel@nongnu.org; Thu, 02 Jul 2015 14:43:53 -0400 Received: by lagx9 with SMTP id x9so66942224lag.1 for ; Thu, 02 Jul 2015 11:43:52 -0700 (PDT) Message-ID: <55958665.8010301@gmail.com> Date: Thu, 02 Jul 2015 21:43:49 +0300 From: Dmitry Osipenko MIME-Version: 1.0 References: <55952A1D.2030806@gmail.com> <1435857603-30613-1-git-send-email-digetx@gmail.com> <55957A76.3070007@gmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Paolo Bonzini , Peter Crosthwaite , QEMU Developers 02.07.2015 21:09, Peter Maydell пишет: > On 2 July 2015 at 18:52, Dmitry Osipenko wrote: >> 02.07.2015 20:34, Peter Maydell пишет: >>> >>> >>> This will now cause us to do the "reload the timer" >>> logic if you write a 1 to the control bit when it was >>> already 1, which we didn't do before. >>> >>> The logic I suggested in my previous review >>> comment gets this right... >>> >>> -- PMM >>> >> >> The problem with code you suggested is that won't start periodic count after >> one-shot tick was completed. > > Can you give more detail? This code is only for when > the guest writes to the control register, so it doesn't > get run when a one-shot tick completes. > > In any case, the code currently in master does: > old value new value action > 0 0 nothing > 0 1 reload timer > 1 0 nothing > 1 1 nothing > > Your first patch did: > > old value new value action > 0 0 delete timer > 0 1 reload timer > 1 0 delete timer > 1 1 nothing > > Your second patch does: > > old value new value action > 0 0 nothing > 0 1 reload timer > 1 0 delete timer > 1 1 reload timer > > My suggestion was: > > old value new value action > 0 0 nothing > 0 1 reload timer > 1 0 delete timer > 1 1 nothing > > If you think that's wrong, then surely your first > patch also has the same problem? > > thanks > -- PMM > Yes, my first patch has same problem. Noticed that issue couple hours ago, it's separate bug as I see now... Will make patch for it too. To clarify new issue: 1) load TIMER_LOAD with some value 2) write (TIMER_CONTROL_ENABLE | TIMER_CONTROL_ONESHOT | TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL 3) wait for one-shot complete 4) re-load TIMER_LOAD 5) write (TIMER_CONTROL_ENABLE | TIMER_CONTROL_PERIODIC | TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL <---- it won't start, bug Oh, and just noticed that timer code doesn't handle IT(interrupt enable) bit. Will fix it too. -- Dmitry