diff mbox series

[7/8] hw/m68k/mcf5206.c: Switch to transaction-based ptimer API

Message ID 20191017132905.5604-8-peter.maydell@linaro.org
State Superseded
Headers show
Series Convert misc-arch devices to new ptimer API | expand

Commit Message

Peter Maydell Oct. 17, 2019, 1:29 p.m. UTC 
Switch the mcf5206 code away from bottom-half based ptimers to
the new transaction-based ptimer API.  This just requires adding
begin/commit calls around the various places that modify the ptimer
state, and using the new ptimer_init() function to create the timer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 hw/m68k/mcf5206.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.20.1

Comments

Richard Henderson Oct. 17, 2019, 3:07 p.m. UTC | #1 
On 10/17/19 6:29 AM, Peter Maydell wrote:
> Switch the mcf5206 code away from bottom-half based ptimers to

> the new transaction-based ptimer API.  This just requires adding

> begin/commit calls around the various places that modify the ptimer

> state, and using the new ptimer_init() function to create the timer.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/m68k/mcf5206.c | 9 +++++----

>  1 file changed, 5 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>



r~
Philippe Mathieu-Daudé Oct. 17, 2019, 3:48 p.m. UTC | #2 
On 10/17/19 3:29 PM, Peter Maydell wrote:
> Switch the mcf5206 code away from bottom-half based ptimers to

> the new transaction-based ptimer API.  This just requires adding

> begin/commit calls around the various places that modify the ptimer

> state, and using the new ptimer_init() function to create the timer.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>   hw/m68k/mcf5206.c | 9 +++++----

>   1 file changed, 5 insertions(+), 4 deletions(-)

> 

> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c

> index a49096367cb..c05401e0e50 100644

> --- a/hw/m68k/mcf5206.c

> +++ b/hw/m68k/mcf5206.c

> @@ -8,7 +8,6 @@

>   

>   #include "qemu/osdep.h"

>   #include "qemu/error-report.h"

> -#include "qemu/main-loop.h"

>   #include "cpu.h"

>   #include "hw/hw.h"

>   #include "hw/irq.h"

> @@ -57,6 +56,7 @@ static void m5206_timer_recalibrate(m5206_timer_state *s)

>       int prescale;

>       int mode;

>   

> +    ptimer_transaction_begin(s->timer);

>       ptimer_stop(s->timer);

>   

>       if ((s->tmr & TMR_RST) == 0)

> @@ -78,6 +78,7 @@ static void m5206_timer_recalibrate(m5206_timer_state *s)

>       ptimer_set_limit(s->timer, s->trr, 0);

>   

>       ptimer_run(s->timer, 0);

> +    ptimer_transaction_commit(s->timer);

>   }

>   

>   static void m5206_timer_trigger(void *opaque)

> @@ -123,7 +124,9 @@ static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)

>           s->tcr = val;

>           break;

>       case 0xc:

> +        ptimer_transaction_begin(s->timer);

>           ptimer_set_count(s->timer, val);

> +        ptimer_transaction_commit(s->timer);

>           break;

>       case 0x11:

>           s->ter &= ~val;

> @@ -137,11 +140,9 @@ static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)

>   static m5206_timer_state *m5206_timer_init(qemu_irq irq)

>   {

>       m5206_timer_state *s;

> -    QEMUBH *bh;

>   

>       s = g_new0(m5206_timer_state, 1);

> -    bh = qemu_bh_new(m5206_timer_trigger, s);

> -    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);

> +    s->timer = ptimer_init(m5206_timer_trigger, s, PTIMER_POLICY_DEFAULT);

>       s->irq = irq;

>       m5206_timer_reset(s);

>       return s;

> 


Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thomas Huth Oct. 19, 2019, 10:48 a.m. UTC | #3 
Am Thu, 17 Oct 2019 14:29:04 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Switch the mcf5206 code away from bottom-half based ptimers to

> the new transaction-based ptimer API.  This just requires adding

> begin/commit calls around the various places that modify the ptimer

> state, and using the new ptimer_init() function to create the timer.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/m68k/mcf5206.c | 9 +++++----

>  1 file changed, 5 insertions(+), 4 deletions(-)


After applying the patch, I now get an assertion:

$ qemu-system-m68k -nographic -kernel
~/tmp/images/image-an5206-big-20000706.bin -M an5206

uClinux/COLDFIRE(m5206)
COLDFIRE port done by Greg Ungerer, gerg@moreton.com.au
Flat model support (C) 1998,1999 Kenneth Albanowski, D. Jeff Dionne
KERNEL -> TEXT=0x010000-0x077cb8 DATA=0x077cb8-0x08b0d0 BSS=0x08b0d0-0x0a2d58
KERNEL -> ROMFS=0x0a2d58-0x183b10 MEM=0x183b10-0x1fff000 STACK=0x1fff000-0x2000000
qemu-system-m68k: hw/core/ptimer.c:410: ptimer_transaction_begin: Assertion `!s->in_transaction || !s->callback' failed.

Looks like something is still wrong here...

 Thomas
Thomas Huth Oct. 19, 2019, 11:10 a.m. UTC | #4 
Am Sat, 19 Oct 2019 12:48:59 +0200
schrieb Thomas Huth <huth@tuxfamily.org>:

> Am Thu, 17 Oct 2019 14:29:04 +0100

> schrieb Peter Maydell <peter.maydell@linaro.org>:

> 

> > Switch the mcf5206 code away from bottom-half based ptimers to

> > the new transaction-based ptimer API.  This just requires adding

> > begin/commit calls around the various places that modify the ptimer

> > state, and using the new ptimer_init() function to create the timer.

> > 

> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> > ---

> >  hw/m68k/mcf5206.c | 9 +++++----

> >  1 file changed, 5 insertions(+), 4 deletions(-)  

> 

> After applying the patch, I now get an assertion:

> 

> $ qemu-system-m68k -nographic -kernel

> ~/tmp/images/image-an5206-big-20000706.bin -M an5206

> 

> uClinux/COLDFIRE(m5206)

> COLDFIRE port done by Greg Ungerer, gerg@moreton.com.au

> Flat model support (C) 1998,1999 Kenneth Albanowski, D. Jeff Dionne

> KERNEL -> TEXT=0x010000-0x077cb8 DATA=0x077cb8-0x08b0d0

> BSS=0x08b0d0-0x0a2d58 KERNEL -> ROMFS=0x0a2d58-0x183b10

> MEM=0x183b10-0x1fff000 STACK=0x1fff000-0x2000000 qemu-system-m68k:

> hw/core/ptimer.c:410: ptimer_transaction_begin: Assertion

> `!s->in_transaction || !s->callback' failed.

> 

> Looks like something is still wrong here...


FWIW, the image for testing can be found here:

https://web.archive.org/web/20181018135822/http://www.uclinux.org/ports/coldfire/image-an5206-big-20000706.bin.gz

 Thomas
Thomas Huth Oct. 19, 2019, 11:38 a.m. UTC | #5 
Am Sat, 19 Oct 2019 13:10:27 +0200
schrieb Thomas Huth <huth@tuxfamily.org>:

> Am Sat, 19 Oct 2019 12:48:59 +0200

> schrieb Thomas Huth <huth@tuxfamily.org>:

> 

> > Am Thu, 17 Oct 2019 14:29:04 +0100

> > schrieb Peter Maydell <peter.maydell@linaro.org>:

> > 

> > > Switch the mcf5206 code away from bottom-half based ptimers to

> > > the new transaction-based ptimer API.  This just requires adding

> > > begin/commit calls around the various places that modify the

> > > ptimer state, and using the new ptimer_init() function to create

> > > the timer.

> > > 

> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> > > ---

> > >  hw/m68k/mcf5206.c | 9 +++++----

> > >  1 file changed, 5 insertions(+), 4 deletions(-)  

> > 

> > After applying the patch, I now get an assertion:

> > 

> > $ qemu-system-m68k -nographic -kernel

> > ~/tmp/images/image-an5206-big-20000706.bin -M an5206

> > 

> > uClinux/COLDFIRE(m5206)

> > COLDFIRE port done by Greg Ungerer, gerg@moreton.com.au

> > Flat model support (C) 1998,1999 Kenneth Albanowski, D. Jeff Dionne

> > KERNEL -> TEXT=0x010000-0x077cb8 DATA=0x077cb8-0x08b0d0

> > BSS=0x08b0d0-0x0a2d58 KERNEL -> ROMFS=0x0a2d58-0x183b10

> > MEM=0x183b10-0x1fff000 STACK=0x1fff000-0x2000000 qemu-system-m68k:

> > hw/core/ptimer.c:410: ptimer_transaction_begin: Assertion

> > `!s->in_transaction || !s->callback' failed.

> > 

> > Looks like something is still wrong here...


Looking at the code a little bit, I think you missed the early return
in m5206_timer_recalibrate() :

    if ((s->tmr & TMR_RST) == 0)
        return;

That needs a ptimer_transaction_commit() now, too.

 Thomas
diff mbox series

Patch

diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index a49096367cb..c05401e0e50 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -8,7 +8,6 @@ 
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/irq.h"
@@ -57,6 +56,7 @@  static void m5206_timer_recalibrate(m5206_timer_state *s)
     int prescale;
     int mode;
 
+    ptimer_transaction_begin(s->timer);
     ptimer_stop(s->timer);
 
     if ((s->tmr & TMR_RST) == 0)
@@ -78,6 +78,7 @@  static void m5206_timer_recalibrate(m5206_timer_state *s)
     ptimer_set_limit(s->timer, s->trr, 0);
 
     ptimer_run(s->timer, 0);
+    ptimer_transaction_commit(s->timer);
 }
 
 static void m5206_timer_trigger(void *opaque)
@@ -123,7 +124,9 @@  static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)
         s->tcr = val;
         break;
     case 0xc:
+        ptimer_transaction_begin(s->timer);
         ptimer_set_count(s->timer, val);
+        ptimer_transaction_commit(s->timer);
         break;
     case 0x11:
         s->ter &= ~val;
@@ -137,11 +140,9 @@  static void m5206_timer_write(m5206_timer_state *s, uint32_t addr, uint32_t val)
 static m5206_timer_state *m5206_timer_init(qemu_irq irq)
 {
     m5206_timer_state *s;
-    QEMUBH *bh;
 
     s = g_new0(m5206_timer_state, 1);
-    bh = qemu_bh_new(m5206_timer_trigger, s);
-    s->timer = ptimer_init_with_bh(bh, PTIMER_POLICY_DEFAULT);
+    s->timer = ptimer_init(m5206_timer_trigger, s, PTIMER_POLICY_DEFAULT);
     s->irq = irq;
     m5206_timer_reset(s);
     return s;