diff mbox series

[v2,11/21] hw/timer/exynos4210_mct.c: Switch GFRC to transaction-based ptimer API

Message ID 20191008171740.9679-12-peter.maydell@linaro.org
State Superseded
Headers show
Series transaction-based ptimer API | expand

Commit Message

Peter Maydell Oct. 8, 2019, 5:17 p.m. UTC 
We want to switch the exynos MCT code away from bottom-half based ptimers to
the new transaction-based ptimer API. The MCT is complicated
and uses multiple different ptimers, so it's clearer to switch
it a piece at a time. Here we change over only the GFRC.

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

---
 hw/timer/exynos4210_mct.c | 48 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

-- 
2.20.1

Comments

Richard Henderson Oct. 9, 2019, 1:56 a.m. UTC | #1 
On 10/8/19 1:17 PM, Peter Maydell wrote:
> @@ -945,7 +977,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)

>          DPRINTF("freq=%dHz\n", s->freq);

>  

>          /* global timer */

> -        ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);

> +        tx_ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);

>  

>          /* local timer */

>          ptimer_set_freq(s->l_timer[0].tick_timer.ptimer_tick, s->freq);


Failed to update them all, it would appear.


r~
Richard Henderson Oct. 9, 2019, 1:58 a.m. UTC | #2 
On 10/8/19 9:56 PM, Richard Henderson wrote:
> On 10/8/19 1:17 PM, Peter Maydell wrote:

>> @@ -945,7 +977,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)

>>          DPRINTF("freq=%dHz\n", s->freq);

>>  

>>          /* global timer */

>> -        ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);

>> +        tx_ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);

>>  

>>          /* local timer */

>>          ptimer_set_freq(s->l_timer[0].tick_timer.ptimer_tick, s->freq);

> 

> Failed to update them all, it would appear.


Ho hum.  Done in a subsequent patch.
I didn't realize that you weren't converting the entire file at once.


r~
Richard Henderson Oct. 9, 2019, 1:58 a.m. UTC | #3 
On 10/8/19 1:17 PM, Peter Maydell wrote:
> We want to switch the exynos MCT code away from bottom-half based ptimers to

> the new transaction-based ptimer API. The MCT is complicated

> and uses multiple different ptimers, so it's clearer to switch

> it a piece at a time. Here we change over only the GFRC.

> 

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

> ---

>  hw/timer/exynos4210_mct.c | 48 ++++++++++++++++++++++++++++++++++++---

>  1 file changed, 45 insertions(+), 3 deletions(-)


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


r~
Peter Maydell Oct. 9, 2019, 1:49 p.m. UTC | #4 
On Wed, 9 Oct 2019 at 02:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 10/8/19 9:56 PM, Richard Henderson wrote:

> > On 10/8/19 1:17 PM, Peter Maydell wrote:

> >> @@ -945,7 +977,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)

> >>          DPRINTF("freq=%dHz\n", s->freq);

> >>

> >>          /* global timer */

> >> -        ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);

> >> +        tx_ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);

> >>

> >>          /* local timer */

> >>          ptimer_set_freq(s->l_timer[0].tick_timer.ptimer_tick, s->freq);

> >

> > Failed to update them all, it would appear.

>

> Ho hum.  Done in a subsequent patch.

> I didn't realize that you weren't converting the entire file at once.


Yeah, the MCT device is pretty complicated and (per the commit
message) I felt it was easier to write and debug by switching
each ptimer one at a time rather than trying to do the whole
lot at the same time.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 9f2e8dd0a42..fcf91c75cc5 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -364,6 +364,7 @@  static void exynos4210_mct_update_freq(Exynos4210MCTState *s);
 
 /*
  * Set counter of FRC global timer.
+ * Must be called within exynos4210_gfrc_tx_begin/commit block.
  */
 static void exynos4210_gfrc_set_count(Exynos4210MCTGT *s, uint64_t count)
 {
@@ -385,6 +386,7 @@  static uint64_t exynos4210_gfrc_get_count(Exynos4210MCTGT *s)
 
 /*
  * Stop global FRC timer
+ * Must be called within exynos4210_gfrc_tx_begin/commit block.
  */
 static void exynos4210_gfrc_stop(Exynos4210MCTGT *s)
 {
@@ -395,6 +397,7 @@  static void exynos4210_gfrc_stop(Exynos4210MCTGT *s)
 
 /*
  * Start global FRC timer
+ * Must be called within exynos4210_gfrc_tx_begin/commit block.
  */
 static void exynos4210_gfrc_start(Exynos4210MCTGT *s)
 {
@@ -403,6 +406,21 @@  static void exynos4210_gfrc_start(Exynos4210MCTGT *s)
     ptimer_run(s->ptimer_frc, 1);
 }
 
+/*
+ * Start ptimer transaction for global FRC timer; this is just for
+ * consistency with the way we wrap operations like stop and run.
+ */
+static void exynos4210_gfrc_tx_begin(Exynos4210MCTGT *s)
+{
+    ptimer_transaction_begin(s->ptimer_frc);
+}
+
+/* Commit ptimer transaction for global FRC timer. */
+static void exynos4210_gfrc_tx_commit(Exynos4210MCTGT *s)
+{
+    ptimer_transaction_commit(s->ptimer_frc);
+}
+
 /*
  * Find next nearest Comparator. If current Comparator value equals to other
  * Comparator value, skip them both
@@ -492,6 +510,7 @@  static uint64_t exynos4210_gcomp_get_distance(Exynos4210MCTState *s, int32_t id)
 
 /*
  * Restart global FRC timer
+ * Must be called within exynos4210_gfrc_tx_begin/commit block.
  */
 static void exynos4210_gfrc_restart(Exynos4210MCTState *s)
 {
@@ -933,6 +952,19 @@  static void exynos4210_ltick_event(void *opaque)
     exynos4210_ltick_int_start(&s->tick_timer);
 }
 
+static void tx_ptimer_set_freq(ptimer_state *s, uint32_t freq)
+{
+    /*
+     * callers of exynos4210_mct_update_freq() never do anything
+     * else that needs to be in the same ptimer transaction, so
+     * to avoid a lot of repetition we have a convenience function
+     * for begin/set_freq/commit.
+     */
+    ptimer_transaction_begin(s);
+    ptimer_set_freq(s, freq);
+    ptimer_transaction_commit(s);
+}
+
 /* update timer frequency */
 static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
 {
@@ -945,7 +977,7 @@  static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
         DPRINTF("freq=%dHz\n", s->freq);
 
         /* global timer */
-        ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);
+        tx_ptimer_set_freq(s->g_timer.ptimer_frc, s->freq);
 
         /* local timer */
         ptimer_set_freq(s->l_timer[0].tick_timer.ptimer_tick, s->freq);
@@ -965,7 +997,9 @@  static void exynos4210_mct_reset(DeviceState *d)
 
     /* global timer */
     memset(&s->g_timer.reg, 0, sizeof(s->g_timer.reg));
+    exynos4210_gfrc_tx_begin(&s->g_timer);
     exynos4210_gfrc_stop(&s->g_timer);
+    exynos4210_gfrc_tx_commit(&s->g_timer);
 
     /* local timer */
     memset(s->l_timer[0].reg.cnt, 0, sizeof(s->l_timer[0].reg.cnt));
@@ -1144,7 +1178,9 @@  static void exynos4210_mct_write(void *opaque, hwaddr offset,
         }
 
         s->g_timer.reg.cnt = new_frc;
+        exynos4210_gfrc_tx_begin(&s->g_timer);
         exynos4210_gfrc_restart(s);
+        exynos4210_gfrc_tx_commit(&s->g_timer);
         break;
 
     case G_CNT_WSTAT:
@@ -1168,7 +1204,9 @@  static void exynos4210_mct_write(void *opaque, hwaddr offset,
             s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
         }
 
+        exynos4210_gfrc_tx_begin(&s->g_timer);
         exynos4210_gfrc_restart(s);
+        exynos4210_gfrc_tx_commit(&s->g_timer);
         break;
 
     case G_TCON:
@@ -1178,6 +1216,8 @@  static void exynos4210_mct_write(void *opaque, hwaddr offset,
 
         DPRINTF("global timer write to reg.g_tcon %llx\n", value);
 
+        exynos4210_gfrc_tx_begin(&s->g_timer);
+
         /* Start FRC if transition from disabled to enabled */
         if ((value & G_TCON_TIMER_ENABLE) > (old_val &
                 G_TCON_TIMER_ENABLE)) {
@@ -1195,6 +1235,8 @@  static void exynos4210_mct_write(void *opaque, hwaddr offset,
                 exynos4210_gfrc_restart(s);
             }
         }
+
+        exynos4210_gfrc_tx_commit(&s->g_timer);
         break;
 
     case G_INT_CSTAT:
@@ -1428,8 +1470,8 @@  static void exynos4210_mct_init(Object *obj)
     QEMUBH *bh[2];
 
     /* Global timer */
-    bh[0] = qemu_bh_new(exynos4210_gfrc_event, s);
-    s->g_timer.ptimer_frc = ptimer_init_with_bh(bh[0], PTIMER_POLICY_DEFAULT);
+    s->g_timer.ptimer_frc = ptimer_init(exynos4210_gfrc_event, s,
+                                        PTIMER_POLICY_DEFAULT);
     memset(&s->g_timer.reg, 0, sizeof(struct gregs));
 
     /* Local timers */