[RFC PATCH 00/13] hw/timer/allwinner: Make it reusable

[RFC PATCH 00/13] hw/timer/allwinner: Make it reusable

[Philippe Mathieu-Daudé]

Hi,

Niek added the H3 SoC in [1] and noticed in [2] the timer
controller is very similar (less timers, watchdog register
placed at different address).

On 12/18/19 9:14 PM, Niek Linnenbank wrote:
> Actually, I copied the timer support code from the existing cubieboard.c
> that has
> the Allwinner A10, so potentially the same problem is there.
>
> While looking more closer at this part, I now also discovered that the
> timer module from the Allwinner H3 is
> mostly a stripped down version of the timer module in the Allwinner A10:
>
>    Allwinner A10, 10.2 Timer Register List, page 85:
> https://linux-sunxi.org/images/1/1e/Allwinner_A10_User_manual_V1.5.pdf
>
> The A10 version has six timers, where the H3 has only two. That should
> be fine I would say, the guest would simply
> use those available on H3 and ignore the rest. There is however one
> conflicting difference: the WDOG0 registers in the Allwinner H3 start
> at a different offset and are also different. The current A10 timer does
> not currently implement the watchdog part.
[...]
> So in my opinion its a bit of a trade off here: we can keep it like this
> and re-use the A10 timer for now, and perhaps
> attempt to generalize that module for proper use in both SoCs. Or we can
> introduce a new H3 specific timer module.
> What do you think?

As an answer to his question, this series is to help him to
reuse the A10 timer controller instead of adding a new model
to the codebase.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg665532.html
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg666304.html

Philippe Mathieu-Daudé (13):
  hw/timer/allwinner: Use the AW_A10_PIT_TIMER_NR definition
  hw/timer/allwinner: Add AW_PIT_TIMER_MAX definition
  hw/timer/allwinner: Remove unused definitions
  hw/timer/allwinner: Move definitions from header to source
  hw/timer/allwinner: Rename the ptimer field
  hw/timer/allwinner: Rename 'timer_context' as 'timer'
  hw/timer/allwinner: Move timer specific fields into AwA10TimerContext
  hw/timer/allwinner: Add a timer_count field
  hw/timer/allwinner: Rename AwA10TimerContext as AllwinnerTmrState
  hw/timer/allwinner: Rename AwA10PITState as AllwinnerTmrCtrlState
  hw/timer/allwinner: Introduce TYPE_AW_COMMON_PIT abstract device
  hw/timer/allwinner: Rename AW_A10_PIT() as AW_TIMER_CTRL()
  hw/timer/allwinner: Rename functions not specific to the A10 SoC

 include/hw/arm/allwinner-a10.h       |   2 +-
 include/hw/timer/allwinner-a10-pit.h |  54 ++----
 hw/timer/allwinner-a10-pit.c         | 271 +++++++++++++++++----------
 3 files changed, 192 insertions(+), 135 deletions(-)

-- 
2.21.0

[PATCH 01/13] hw/timer/allwinner: Use the AW_A10_PIT_TIMER_NR definition

[Philippe Mathieu-Daudé]

We have a definition for this magic value '6', use it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h | 2 +-
 hw/timer/allwinner-a10-pit.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 871c95b512..6aceda81ee 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -32,7 +32,7 @@
 
 #define AW_A10_PIT_TIMER_BASE      0x10
 #define AW_A10_PIT_TIMER_BASE_END  \
-    (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
+    (AW_A10_PIT_TIMER_BASE * AW_A10_PIT_TIMER_NR + AW_A10_PIT_TIMER_COUNT)
 
 #define AW_A10_PIT_DEFAULT_CLOCK   0x4
 
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index aae880f5b3..117e5c7bf8 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -225,7 +225,7 @@ static void a10_pit_reset(DeviceState *dev)
     s->irq_status = 0;
     a10_pit_update_irq(s);
 
-    for (i = 0; i < 6; i++) {
+    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
         s->control[i] = AW_A10_PIT_DEFAULT_CLOCK;
         s->interval[i] = 0;
         s->count[i] = 0;
-- 
2.21.0

Re: [PATCH 01/13] hw/timer/allwinner: Use the AW_A10_PIT_TIMER_NR definition

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> We have a definition for this magic value '6', use it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/timer/allwinner-a10-pit.h | 2 +-
>  hw/timer/allwinner-a10-pit.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h
> b/include/hw/timer/allwinner-a10-pit.h
> index 871c95b512..6aceda81ee 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -32,7 +32,7 @@
>
>  #define AW_A10_PIT_TIMER_BASE      0x10
>  #define AW_A10_PIT_TIMER_BASE_END  \
> -    (AW_A10_PIT_TIMER_BASE * 6 + AW_A10_PIT_TIMER_COUNT)
> +    (AW_A10_PIT_TIMER_BASE * AW_A10_PIT_TIMER_NR + AW_A10_PIT_TIMER_COUNT)
>
>  #define AW_A10_PIT_DEFAULT_CLOCK   0x4
>
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index aae880f5b3..117e5c7bf8 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -225,7 +225,7 @@ static void a10_pit_reset(DeviceState *dev)
>      s->irq_status = 0;
>      a10_pit_update_irq(s);
>
> -    for (i = 0; i < 6; i++) {
> +    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
>          s->control[i] = AW_A10_PIT_DEFAULT_CLOCK;
>          s->interval[i] = 0;
>          s->count[i] = 0;
> --
> 2.21.0
>
> Change looks fine to me:
  Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Works fine with -M orangepi-pc and -M cubieboard:
  Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>



-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 2433 bytes --]

[PATCH 02/13] hw/timer/allwinner: Add AW_PIT_TIMER_MAX definition

[Philippe Mathieu-Daudé]

This controller is able to use up to 6 timers.
Later we will reuse part of it to model other similar controllers
but with less timers. To simplify the VMSTATE, we'll keep a max
of 6 timers. Add a definition for that value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h | 14 ++++++++------
 hw/timer/allwinner-a10-pit.c         |  8 ++++----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 6aceda81ee..54c40c7db6 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -7,6 +7,8 @@
 #define TYPE_AW_A10_PIT "allwinner-A10-timer"
 #define AW_A10_PIT(obj) OBJECT_CHECK(AwA10PITState, (obj), TYPE_AW_A10_PIT)
 
+#define AW_PIT_TIMER_MAX        6
+
 #define AW_A10_PIT_TIMER_NR    6
 #define AW_A10_PIT_TIMER_IRQ   0x1
 #define AW_A10_PIT_WDOG_IRQ    0x100
@@ -47,17 +49,17 @@ struct AwA10PITState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
-    qemu_irq irq[AW_A10_PIT_TIMER_NR];
-    ptimer_state * timer[AW_A10_PIT_TIMER_NR];
-    AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
+    qemu_irq irq[AW_PIT_TIMER_MAX];
+    ptimer_state * timer[AW_PIT_TIMER_MAX];
+    AwA10TimerContext timer_context[AW_PIT_TIMER_MAX];
     MemoryRegion iomem;
     uint32_t clk_freq[4];
 
     uint32_t irq_enable;
     uint32_t irq_status;
-    uint32_t control[AW_A10_PIT_TIMER_NR];
-    uint32_t interval[AW_A10_PIT_TIMER_NR];
-    uint32_t count[AW_A10_PIT_TIMER_NR];
+    uint32_t control[AW_PIT_TIMER_MAX];
+    uint32_t interval[AW_PIT_TIMER_MAX];
+    uint32_t count[AW_PIT_TIMER_MAX];
     uint32_t watch_dog_mode;
     uint32_t watch_dog_control;
     uint32_t count_lo;
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 117e5c7bf8..b31a0bcd43 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -203,15 +203,15 @@ static const VMStateDescription vmstate_a10_pit = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(irq_enable, AwA10PITState),
         VMSTATE_UINT32(irq_status, AwA10PITState),
-        VMSTATE_UINT32_ARRAY(control, AwA10PITState, AW_A10_PIT_TIMER_NR),
-        VMSTATE_UINT32_ARRAY(interval, AwA10PITState, AW_A10_PIT_TIMER_NR),
-        VMSTATE_UINT32_ARRAY(count, AwA10PITState, AW_A10_PIT_TIMER_NR),
+        VMSTATE_UINT32_ARRAY(control, AwA10PITState, AW_PIT_TIMER_MAX),
+        VMSTATE_UINT32_ARRAY(interval, AwA10PITState, AW_PIT_TIMER_MAX),
+        VMSTATE_UINT32_ARRAY(count, AwA10PITState, AW_PIT_TIMER_MAX),
         VMSTATE_UINT32(watch_dog_mode, AwA10PITState),
         VMSTATE_UINT32(watch_dog_control, AwA10PITState),
         VMSTATE_UINT32(count_lo, AwA10PITState),
         VMSTATE_UINT32(count_hi, AwA10PITState),
         VMSTATE_UINT32(count_ctl, AwA10PITState),
-        VMSTATE_PTIMER_ARRAY(timer, AwA10PITState, AW_A10_PIT_TIMER_NR),
+        VMSTATE_PTIMER_ARRAY(timer, AwA10PITState, AW_PIT_TIMER_MAX),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.21.0

Re: [PATCH 02/13] hw/timer/allwinner: Add AW_PIT_TIMER_MAX definition

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> This controller is able to use up to 6 timers.
> Later we will reuse part of it to model other similar controllers
> but with less timers. To simplify the VMSTATE, we'll keep a max
> of 6 timers. Add a definition for that value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/timer/allwinner-a10-pit.h | 14 ++++++++------
>  hw/timer/allwinner-a10-pit.c         |  8 ++++----
>  2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h
> b/include/hw/timer/allwinner-a10-pit.h
> index 6aceda81ee..54c40c7db6 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -7,6 +7,8 @@
>  #define TYPE_AW_A10_PIT "allwinner-A10-timer"
>  #define AW_A10_PIT(obj) OBJECT_CHECK(AwA10PITState, (obj),
> TYPE_AW_A10_PIT)
>
> +#define AW_PIT_TIMER_MAX        6
> +
>  #define AW_A10_PIT_TIMER_NR    6
>  #define AW_A10_PIT_TIMER_IRQ   0x1
>  #define AW_A10_PIT_WDOG_IRQ    0x100
> @@ -47,17 +49,17 @@ struct AwA10PITState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>      /*< public >*/
> -    qemu_irq irq[AW_A10_PIT_TIMER_NR];
> -    ptimer_state * timer[AW_A10_PIT_TIMER_NR];
> -    AwA10TimerContext timer_context[AW_A10_PIT_TIMER_NR];
> +    qemu_irq irq[AW_PIT_TIMER_MAX];
> +    ptimer_state * timer[AW_PIT_TIMER_MAX];
> +    AwA10TimerContext timer_context[AW_PIT_TIMER_MAX];
>      MemoryRegion iomem;
>      uint32_t clk_freq[4];
>
>      uint32_t irq_enable;
>      uint32_t irq_status;
> -    uint32_t control[AW_A10_PIT_TIMER_NR];
> -    uint32_t interval[AW_A10_PIT_TIMER_NR];
> -    uint32_t count[AW_A10_PIT_TIMER_NR];
> +    uint32_t control[AW_PIT_TIMER_MAX];
> +    uint32_t interval[AW_PIT_TIMER_MAX];
> +    uint32_t count[AW_PIT_TIMER_MAX];
>      uint32_t watch_dog_mode;
>      uint32_t watch_dog_control;
>      uint32_t count_lo;
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index 117e5c7bf8..b31a0bcd43 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -203,15 +203,15 @@ static const VMStateDescription vmstate_a10_pit = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(irq_enable, AwA10PITState),
>          VMSTATE_UINT32(irq_status, AwA10PITState),
> -        VMSTATE_UINT32_ARRAY(control, AwA10PITState, AW_A10_PIT_TIMER_NR),
> -        VMSTATE_UINT32_ARRAY(interval, AwA10PITState,
> AW_A10_PIT_TIMER_NR),
> -        VMSTATE_UINT32_ARRAY(count, AwA10PITState, AW_A10_PIT_TIMER_NR),
> +        VMSTATE_UINT32_ARRAY(control, AwA10PITState, AW_PIT_TIMER_MAX),
> +        VMSTATE_UINT32_ARRAY(interval, AwA10PITState, AW_PIT_TIMER_MAX),
> +        VMSTATE_UINT32_ARRAY(count, AwA10PITState, AW_PIT_TIMER_MAX),
>          VMSTATE_UINT32(watch_dog_mode, AwA10PITState),
>          VMSTATE_UINT32(watch_dog_control, AwA10PITState),
>          VMSTATE_UINT32(count_lo, AwA10PITState),
>          VMSTATE_UINT32(count_hi, AwA10PITState),
>          VMSTATE_UINT32(count_ctl, AwA10PITState),
> -        VMSTATE_PTIMER_ARRAY(timer, AwA10PITState, AW_A10_PIT_TIMER_NR),
> +        VMSTATE_PTIMER_ARRAY(timer, AwA10PITState, AW_PIT_TIMER_MAX),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> --
> 2.21.0
>
> Looks good and works fine with -M orangepi-pc and -M cubieboard.

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 4545 bytes --]

[PATCH 03/13] hw/timer/allwinner: Remove unused definitions

[Philippe Mathieu-Daudé]

Keeping unused definition is rather confusing when reviewing.
Remove them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 54c40c7db6..e4a644add9 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -10,8 +10,6 @@
 #define AW_PIT_TIMER_MAX        6
 
 #define AW_A10_PIT_TIMER_NR    6
-#define AW_A10_PIT_TIMER_IRQ   0x1
-#define AW_A10_PIT_WDOG_IRQ    0x100
 
 #define AW_A10_PIT_TIMER_IRQ_EN    0
 #define AW_A10_PIT_TIMER_IRQ_ST    0x4
-- 
2.21.0

Re: [PATCH 03/13] hw/timer/allwinner: Remove unused definitions

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Keeping unused definition is rather confusing when reviewing.
> Remove them.
>
Perhaps make it more clear that the definitions are unused IRQ defines?


>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/timer/allwinner-a10-pit.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h
> b/include/hw/timer/allwinner-a10-pit.h
> index 54c40c7db6..e4a644add9 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -10,8 +10,6 @@
>  #define AW_PIT_TIMER_MAX        6
>
>  #define AW_A10_PIT_TIMER_NR    6
> -#define AW_A10_PIT_TIMER_IRQ   0x1
> -#define AW_A10_PIT_WDOG_IRQ    0x100
>
>  #define AW_A10_PIT_TIMER_IRQ_EN    0
>  #define AW_A10_PIT_TIMER_IRQ_ST    0x4
> --
> 2.21.0
>
> Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 1858 bytes --]

[PATCH 04/13] hw/timer/allwinner: Move definitions from header to source

[Philippe Mathieu-Daudé]

These definitions are only used in the implementation, thus don't
need to be exported. Move them in the source file.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h | 28 --------------------------
 hw/timer/allwinner-a10-pit.c         | 30 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index e4a644add9..c28ee5ca47 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -5,37 +5,9 @@
 #include "hw/sysbus.h"
 
 #define TYPE_AW_A10_PIT "allwinner-A10-timer"
-#define AW_A10_PIT(obj) OBJECT_CHECK(AwA10PITState, (obj), TYPE_AW_A10_PIT)
 
 #define AW_PIT_TIMER_MAX        6
 
-#define AW_A10_PIT_TIMER_NR    6
-
-#define AW_A10_PIT_TIMER_IRQ_EN    0
-#define AW_A10_PIT_TIMER_IRQ_ST    0x4
-
-#define AW_A10_PIT_TIMER_CONTROL   0x0
-#define AW_A10_PIT_TIMER_EN        0x1
-#define AW_A10_PIT_TIMER_RELOAD    0x2
-#define AW_A10_PIT_TIMER_MODE      0x80
-
-#define AW_A10_PIT_TIMER_INTERVAL  0x4
-#define AW_A10_PIT_TIMER_COUNT     0x8
-#define AW_A10_PIT_WDOG_CONTROL    0x90
-#define AW_A10_PIT_WDOG_MODE       0x94
-
-#define AW_A10_PIT_COUNT_CTL       0xa0
-#define AW_A10_PIT_COUNT_RL_EN     0x2
-#define AW_A10_PIT_COUNT_CLR_EN    0x1
-#define AW_A10_PIT_COUNT_LO        0xa4
-#define AW_A10_PIT_COUNT_HI        0xa8
-
-#define AW_A10_PIT_TIMER_BASE      0x10
-#define AW_A10_PIT_TIMER_BASE_END  \
-    (AW_A10_PIT_TIMER_BASE * AW_A10_PIT_TIMER_NR + AW_A10_PIT_TIMER_COUNT)
-
-#define AW_A10_PIT_DEFAULT_CLOCK   0x4
-
 typedef struct AwA10PITState AwA10PITState;
 
 typedef struct AwA10TimerContext {
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index b31a0bcd43..00f7cc492d 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -24,6 +24,36 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 
+#define AW_A10_PIT_TIMER_NR         6
+
+#define AW_A10_PIT_TIMER_IRQ_EN     0
+#define AW_A10_PIT_TIMER_IRQ_ST     0x4
+
+#define AW_A10_PIT_TIMER_CONTROL    0x0
+#define AW_A10_PIT_TIMER_EN         0x1
+#define AW_A10_PIT_TIMER_RELOAD     0x2
+#define AW_A10_PIT_TIMER_MODE       0x80
+
+#define AW_A10_PIT_TIMER_INTERVAL   0x4
+#define AW_A10_PIT_TIMER_COUNT      0x8
+#define AW_A10_PIT_WDOG_CONTROL     0x90
+#define AW_A10_PIT_WDOG_MODE        0x94
+
+#define AW_A10_PIT_COUNT_CTL        0xa0
+#define AW_A10_PIT_COUNT_RL_EN      0x2
+#define AW_A10_PIT_COUNT_CLR_EN     0x1
+#define AW_A10_PIT_COUNT_LO         0xa4
+#define AW_A10_PIT_COUNT_HI         0xa8
+
+#define AW_A10_PIT_TIMER_BASE       0x10
+#define AW_A10_PIT_TIMER_BASE_END   \
+    (AW_A10_PIT_TIMER_BASE * AW_A10_PIT_TIMER_NR + AW_A10_PIT_TIMER_COUNT)
+
+#define AW_A10_PIT_DEFAULT_CLOCK    0x4
+
+#define AW_A10_PIT(obj) \
+    OBJECT_CHECK(AwA10PITState, (obj), TYPE_AW_A10_PIT)
+
 static void a10_pit_update_irq(AwA10PITState *s)
 {
     int i;
-- 
2.21.0

Re: [PATCH 04/13] hw/timer/allwinner: Move definitions from header to source

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 3567 bytes --]

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> These definitions are only used in the implementation, thus don't
> need to be exported. Move them in the source file.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/timer/allwinner-a10-pit.h | 28 --------------------------
>  hw/timer/allwinner-a10-pit.c         | 30 ++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h
> b/include/hw/timer/allwinner-a10-pit.h
> index e4a644add9..c28ee5ca47 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -5,37 +5,9 @@
>  #include "hw/sysbus.h"
>
>  #define TYPE_AW_A10_PIT "allwinner-A10-timer"
> -#define AW_A10_PIT(obj) OBJECT_CHECK(AwA10PITState, (obj),
> TYPE_AW_A10_PIT)
>
>  #define AW_PIT_TIMER_MAX        6
>
> -#define AW_A10_PIT_TIMER_NR    6
> -
> -#define AW_A10_PIT_TIMER_IRQ_EN    0
> -#define AW_A10_PIT_TIMER_IRQ_ST    0x4
> -
> -#define AW_A10_PIT_TIMER_CONTROL   0x0
> -#define AW_A10_PIT_TIMER_EN        0x1
> -#define AW_A10_PIT_TIMER_RELOAD    0x2
> -#define AW_A10_PIT_TIMER_MODE      0x80
> -
> -#define AW_A10_PIT_TIMER_INTERVAL  0x4
> -#define AW_A10_PIT_TIMER_COUNT     0x8
> -#define AW_A10_PIT_WDOG_CONTROL    0x90
> -#define AW_A10_PIT_WDOG_MODE       0x94
> -
> -#define AW_A10_PIT_COUNT_CTL       0xa0
> -#define AW_A10_PIT_COUNT_RL_EN     0x2
> -#define AW_A10_PIT_COUNT_CLR_EN    0x1
> -#define AW_A10_PIT_COUNT_LO        0xa4
> -#define AW_A10_PIT_COUNT_HI        0xa8
> -
> -#define AW_A10_PIT_TIMER_BASE      0x10
> -#define AW_A10_PIT_TIMER_BASE_END  \
> -    (AW_A10_PIT_TIMER_BASE * AW_A10_PIT_TIMER_NR + AW_A10_PIT_TIMER_COUNT)
> -
> -#define AW_A10_PIT_DEFAULT_CLOCK   0x4
> -
>  typedef struct AwA10PITState AwA10PITState;
>
>  typedef struct AwA10TimerContext {
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index b31a0bcd43..00f7cc492d 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -24,6 +24,36 @@
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>
> +#define AW_A10_PIT_TIMER_NR         6
> +
> +#define AW_A10_PIT_TIMER_IRQ_EN     0
> +#define AW_A10_PIT_TIMER_IRQ_ST     0x4
> +
> +#define AW_A10_PIT_TIMER_CONTROL    0x0
> +#define AW_A10_PIT_TIMER_EN         0x1
> +#define AW_A10_PIT_TIMER_RELOAD     0x2
> +#define AW_A10_PIT_TIMER_MODE       0x80
> +
> +#define AW_A10_PIT_TIMER_INTERVAL   0x4
> +#define AW_A10_PIT_TIMER_COUNT      0x8
> +#define AW_A10_PIT_WDOG_CONTROL     0x90
> +#define AW_A10_PIT_WDOG_MODE        0x94
> +
> +#define AW_A10_PIT_COUNT_CTL        0xa0
> +#define AW_A10_PIT_COUNT_RL_EN      0x2
> +#define AW_A10_PIT_COUNT_CLR_EN     0x1
> +#define AW_A10_PIT_COUNT_LO         0xa4
> +#define AW_A10_PIT_COUNT_HI         0xa8
> +
> +#define AW_A10_PIT_TIMER_BASE       0x10
> +#define AW_A10_PIT_TIMER_BASE_END   \
> +    (AW_A10_PIT_TIMER_BASE * AW_A10_PIT_TIMER_NR + AW_A10_PIT_TIMER_COUNT)
> +
> +#define AW_A10_PIT_DEFAULT_CLOCK    0x4
> +
> +#define AW_A10_PIT(obj) \
> +    OBJECT_CHECK(AwA10PITState, (obj), TYPE_AW_A10_PIT)
> +
>  static void a10_pit_update_irq(AwA10PITState *s)
>  {
>      int i;
> --
> 2.21.0
>
>
Looks fine and tested on -M orangepi-pc, -M cubieboard:
   Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>
   Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 4545 bytes --]

[RFC PATCH 05/13] hw/timer/allwinner: Rename the ptimer field

[Philippe Mathieu-Daudé]

We will later use the 'timer' field name to access all the
timer related fields. The name is already use, we need to
rename first. 'ptimer' is a good name.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h |  2 +-
 hw/timer/allwinner-a10-pit.c         | 34 ++++++++++++++--------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index c28ee5ca47..a60b9f3031 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -20,7 +20,7 @@ struct AwA10PITState {
     SysBusDevice parent_obj;
     /*< public >*/
     qemu_irq irq[AW_PIT_TIMER_MAX];
-    ptimer_state * timer[AW_PIT_TIMER_MAX];
+    ptimer_state * ptimer[AW_PIT_TIMER_MAX];
     AwA10TimerContext timer_context[AW_PIT_TIMER_MAX];
     MemoryRegion iomem;
     uint32_t clk_freq[4];
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 00f7cc492d..effdf91344 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -83,7 +83,7 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
         case AW_A10_PIT_TIMER_INTERVAL:
             return s->interval[index];
         case AW_A10_PIT_TIMER_COUNT:
-            s->count[index] = ptimer_get_count(s->timer[index]);
+            s->count[index] = ptimer_get_count(s->ptimer[index]);
             return s->count[index];
         default:
             qemu_log_mask(LOG_GUEST_ERROR,
@@ -109,7 +109,7 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
     return 0;
 }
 
-/* Must be called inside a ptimer transaction block for s->timer[index] */
+/* Must be called inside a ptimer transaction block for s->ptimer[index] */
 static void a10_pit_set_freq(AwA10PITState *s, int index)
 {
     uint32_t prescaler, source, source_freq;
@@ -119,7 +119,7 @@ static void a10_pit_set_freq(AwA10PITState *s, int index)
     source_freq = s->clk_freq[source];
 
     if (source_freq) {
-        ptimer_set_freq(s->timer[index], source_freq / prescaler);
+        ptimer_set_freq(s->ptimer[index], source_freq / prescaler);
     } else {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid clock source %u\n",
                       __func__, source);
@@ -148,27 +148,27 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
         switch (offset & 0x0f) {
         case AW_A10_PIT_TIMER_CONTROL:
             s->control[index] = value;
-            ptimer_transaction_begin(s->timer[index]);
+            ptimer_transaction_begin(s->ptimer[index]);
             a10_pit_set_freq(s, index);
             if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
-                ptimer_set_count(s->timer[index], s->interval[index]);
+                ptimer_set_count(s->ptimer[index], s->interval[index]);
             }
             if (s->control[index] & AW_A10_PIT_TIMER_EN) {
                 int oneshot = 0;
                 if (s->control[index] & AW_A10_PIT_TIMER_MODE) {
                     oneshot = 1;
                 }
-                ptimer_run(s->timer[index], oneshot);
+                ptimer_run(s->ptimer[index], oneshot);
             } else {
-                ptimer_stop(s->timer[index]);
+                ptimer_stop(s->ptimer[index]);
             }
-            ptimer_transaction_commit(s->timer[index]);
+            ptimer_transaction_commit(s->ptimer[index]);
             break;
         case AW_A10_PIT_TIMER_INTERVAL:
             s->interval[index] = value;
-            ptimer_transaction_begin(s->timer[index]);
-            ptimer_set_limit(s->timer[index], s->interval[index], 1);
-            ptimer_transaction_commit(s->timer[index]);
+            ptimer_transaction_begin(s->ptimer[index]);
+            ptimer_set_limit(s->ptimer[index], s->interval[index], 1);
+            ptimer_transaction_commit(s->ptimer[index]);
             break;
         case AW_A10_PIT_TIMER_COUNT:
             s->count[index] = value;
@@ -241,7 +241,7 @@ static const VMStateDescription vmstate_a10_pit = {
         VMSTATE_UINT32(count_lo, AwA10PITState),
         VMSTATE_UINT32(count_hi, AwA10PITState),
         VMSTATE_UINT32(count_ctl, AwA10PITState),
-        VMSTATE_PTIMER_ARRAY(timer, AwA10PITState, AW_PIT_TIMER_MAX),
+        VMSTATE_PTIMER_ARRAY(ptimer, AwA10PITState, AW_PIT_TIMER_MAX),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -259,10 +259,10 @@ static void a10_pit_reset(DeviceState *dev)
         s->control[i] = AW_A10_PIT_DEFAULT_CLOCK;
         s->interval[i] = 0;
         s->count[i] = 0;
-        ptimer_transaction_begin(s->timer[i]);
-        ptimer_stop(s->timer[i]);
+        ptimer_transaction_begin(s->ptimer[i]);
+        ptimer_stop(s->ptimer[i]);
         a10_pit_set_freq(s, i);
-        ptimer_transaction_commit(s->timer[i]);
+        ptimer_transaction_commit(s->ptimer[i]);
     }
     s->watch_dog_mode = 0;
     s->watch_dog_control = 0;
@@ -280,7 +280,7 @@ static void a10_pit_timer_cb(void *opaque)
     if (s->control[i] & AW_A10_PIT_TIMER_EN) {
         s->irq_status |= 1 << i;
         if (s->control[i] & AW_A10_PIT_TIMER_MODE) {
-            ptimer_stop(s->timer[i]);
+            ptimer_stop(s->ptimer[i]);
             s->control[i] &= ~AW_A10_PIT_TIMER_EN;
         }
         a10_pit_update_irq(s);
@@ -305,7 +305,7 @@ static void a10_pit_init(Object *obj)
 
         tc->container = s;
         tc->index = i;
-        s->timer[i] = ptimer_init(a10_pit_timer_cb, tc, PTIMER_POLICY_DEFAULT);
+        s->ptimer[i] = ptimer_init(a10_pit_timer_cb, tc, PTIMER_POLICY_DEFAULT);
     }
 }
 
-- 
2.21.0

[RFC PATCH 06/13] hw/timer/allwinner: Rename 'timer_context' as 'timer'

[Philippe Mathieu-Daudé]

The previous 'timer' field has been renamed as 'ptimer'. The
'timer_context' can now be simplified as 'timer'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h | 2 +-
 hw/timer/allwinner-a10-pit.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index a60b9f3031..b5ac6898fa 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -21,7 +21,7 @@ struct AwA10PITState {
     /*< public >*/
     qemu_irq irq[AW_PIT_TIMER_MAX];
     ptimer_state * ptimer[AW_PIT_TIMER_MAX];
-    AwA10TimerContext timer_context[AW_PIT_TIMER_MAX];
+    AwA10TimerContext timer[AW_PIT_TIMER_MAX];
     MemoryRegion iomem;
     uint32_t clk_freq[4];
 
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index effdf91344..44e6eee3a8 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -301,7 +301,7 @@ static void a10_pit_init(Object *obj)
     sysbus_init_mmio(sbd, &s->iomem);
 
     for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
-        AwA10TimerContext *tc = &s->timer_context[i];
+        AwA10TimerContext *tc = &s->timer[i];
 
         tc->container = s;
         tc->index = i;
-- 
2.21.0

[RFC PATCH 07/13] hw/timer/allwinner: Move timer specific fields into AwA10TimerContext

[Philippe Mathieu-Daudé]

Move all the timer-related fields into the same structure.
We scrambled the migration structure, so we need to increase the
version_id.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Before I was using g_new(), now I keep AW_PIT_TIMER_MAX so
We might avoid this patch.
---
 include/hw/timer/allwinner-a10-pit.h | 10 +--
 hw/timer/allwinner-a10-pit.c         | 99 ++++++++++++++++------------
 2 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index b5ac6898fa..e0f864a954 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -13,23 +13,23 @@ typedef struct AwA10PITState AwA10PITState;
 typedef struct AwA10TimerContext {
     AwA10PITState *container;
     int index;
+    ptimer_state *ptimer;
+    qemu_irq irq;
+    uint32_t control;
+    uint32_t interval;
+    uint32_t count;
 } AwA10TimerContext;
 
 struct AwA10PITState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
-    qemu_irq irq[AW_PIT_TIMER_MAX];
-    ptimer_state * ptimer[AW_PIT_TIMER_MAX];
     AwA10TimerContext timer[AW_PIT_TIMER_MAX];
     MemoryRegion iomem;
     uint32_t clk_freq[4];
 
     uint32_t irq_enable;
     uint32_t irq_status;
-    uint32_t control[AW_PIT_TIMER_MAX];
-    uint32_t interval[AW_PIT_TIMER_MAX];
-    uint32_t count[AW_PIT_TIMER_MAX];
     uint32_t watch_dog_mode;
     uint32_t watch_dog_control;
     uint32_t count_lo;
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 44e6eee3a8..ea92fdda32 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -59,7 +59,8 @@ static void a10_pit_update_irq(AwA10PITState *s)
     int i;
 
     for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
-        qemu_set_irq(s->irq[i], !!(s->irq_status & s->irq_enable & (1 << i)));
+        qemu_set_irq(s->timer[i].irq,
+                     !!(s->irq_status & s->irq_enable & (1 << i)));
     }
 }
 
@@ -79,12 +80,12 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
         index -= 1;
         switch (offset & 0x0f) {
         case AW_A10_PIT_TIMER_CONTROL:
-            return s->control[index];
+            return s->timer[index].control;
         case AW_A10_PIT_TIMER_INTERVAL:
-            return s->interval[index];
+            return s->timer[index].interval;
         case AW_A10_PIT_TIMER_COUNT:
-            s->count[index] = ptimer_get_count(s->ptimer[index]);
-            return s->count[index];
+            s->timer[index].count = ptimer_get_count(s->timer[index].ptimer);
+            return s->timer[index].count;
         default:
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: Bad offset 0x%x\n",  __func__, (int)offset);
@@ -109,17 +110,17 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
     return 0;
 }
 
-/* Must be called inside a ptimer transaction block for s->ptimer[index] */
+/* Must be called inside a ptimer transaction block for s->timer[idx].ptimer */
 static void a10_pit_set_freq(AwA10PITState *s, int index)
 {
     uint32_t prescaler, source, source_freq;
 
-    prescaler = 1 << extract32(s->control[index], 4, 3);
-    source = extract32(s->control[index], 2, 2);
+    prescaler = 1 << extract32(s->timer[index].control, 4, 3);
+    source = extract32(s->timer[index].control, 2, 2);
     source_freq = s->clk_freq[source];
 
     if (source_freq) {
-        ptimer_set_freq(s->ptimer[index], source_freq / prescaler);
+        ptimer_set_freq(s->timer[index].ptimer, source_freq / prescaler);
     } else {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid clock source %u\n",
                       __func__, source);
@@ -147,31 +148,33 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
         index -= 1;
         switch (offset & 0x0f) {
         case AW_A10_PIT_TIMER_CONTROL:
-            s->control[index] = value;
-            ptimer_transaction_begin(s->ptimer[index]);
+            s->timer[index].control = value;
+            ptimer_transaction_begin(s->timer[index].ptimer);
             a10_pit_set_freq(s, index);
-            if (s->control[index] & AW_A10_PIT_TIMER_RELOAD) {
-                ptimer_set_count(s->ptimer[index], s->interval[index]);
+            if (s->timer[index].control & AW_A10_PIT_TIMER_RELOAD) {
+                ptimer_set_count(s->timer[index].ptimer,
+                                 s->timer[index].interval);
             }
-            if (s->control[index] & AW_A10_PIT_TIMER_EN) {
+            if (s->timer[index].control & AW_A10_PIT_TIMER_EN) {
                 int oneshot = 0;
-                if (s->control[index] & AW_A10_PIT_TIMER_MODE) {
+                if (s->timer[index].control & AW_A10_PIT_TIMER_MODE) {
                     oneshot = 1;
                 }
-                ptimer_run(s->ptimer[index], oneshot);
+                ptimer_run(s->timer[index].ptimer, oneshot);
             } else {
-                ptimer_stop(s->ptimer[index]);
+                ptimer_stop(s->timer[index].ptimer);
             }
-            ptimer_transaction_commit(s->ptimer[index]);
+            ptimer_transaction_commit(s->timer[index].ptimer);
             break;
         case AW_A10_PIT_TIMER_INTERVAL:
-            s->interval[index] = value;
-            ptimer_transaction_begin(s->ptimer[index]);
-            ptimer_set_limit(s->ptimer[index], s->interval[index], 1);
-            ptimer_transaction_commit(s->ptimer[index]);
+            s->timer[index].interval = value;
+            ptimer_transaction_begin(s->timer[index].ptimer);
+            ptimer_set_limit(s->timer[index].ptimer,
+                             s->timer[index].interval, 1);
+            ptimer_transaction_commit(s->timer[index].ptimer);
             break;
         case AW_A10_PIT_TIMER_COUNT:
-            s->count[index] = value;
+            s->timer[index].count = value;
             break;
         default:
             qemu_log_mask(LOG_GUEST_ERROR,
@@ -226,22 +229,35 @@ static Property a10_pit_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static const VMStateDescription vmstate_aw_timer = {
+    .name = "aw_timer",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(control, AwA10TimerContext),
+        VMSTATE_UINT32(interval, AwA10TimerContext),
+        VMSTATE_UINT32(count, AwA10TimerContext),
+        VMSTATE_PTIMER(ptimer, AwA10TimerContext),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_a10_pit = {
     .name = "a10.pit",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(irq_enable, AwA10PITState),
         VMSTATE_UINT32(irq_status, AwA10PITState),
-        VMSTATE_UINT32_ARRAY(control, AwA10PITState, AW_PIT_TIMER_MAX),
-        VMSTATE_UINT32_ARRAY(interval, AwA10PITState, AW_PIT_TIMER_MAX),
-        VMSTATE_UINT32_ARRAY(count, AwA10PITState, AW_PIT_TIMER_MAX),
+        VMSTATE_STRUCT_ARRAY(timer, AwA10PITState,
+                             AW_PIT_TIMER_MAX,
+                             0, vmstate_aw_timer,
+                             AwA10TimerContext),
         VMSTATE_UINT32(watch_dog_mode, AwA10PITState),
         VMSTATE_UINT32(watch_dog_control, AwA10PITState),
         VMSTATE_UINT32(count_lo, AwA10PITState),
         VMSTATE_UINT32(count_hi, AwA10PITState),
         VMSTATE_UINT32(count_ctl, AwA10PITState),
-        VMSTATE_PTIMER_ARRAY(ptimer, AwA10PITState, AW_PIT_TIMER_MAX),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -256,13 +272,13 @@ static void a10_pit_reset(DeviceState *dev)
     a10_pit_update_irq(s);
 
     for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
-        s->control[i] = AW_A10_PIT_DEFAULT_CLOCK;
-        s->interval[i] = 0;
-        s->count[i] = 0;
-        ptimer_transaction_begin(s->ptimer[i]);
-        ptimer_stop(s->ptimer[i]);
+        s->timer[i].control = AW_A10_PIT_DEFAULT_CLOCK;
+        s->timer[i].interval = 0;
+        s->timer[i].count = 0;
+        ptimer_transaction_begin(s->timer[i].ptimer);
+        ptimer_stop(s->timer[i].ptimer);
         a10_pit_set_freq(s, i);
-        ptimer_transaction_commit(s->ptimer[i]);
+        ptimer_transaction_commit(s->timer[i].ptimer);
     }
     s->watch_dog_mode = 0;
     s->watch_dog_control = 0;
@@ -277,11 +293,11 @@ static void a10_pit_timer_cb(void *opaque)
     AwA10PITState *s = tc->container;
     uint8_t i = tc->index;
 
-    if (s->control[i] & AW_A10_PIT_TIMER_EN) {
+    if (s->timer[i].control & AW_A10_PIT_TIMER_EN) {
         s->irq_status |= 1 << i;
-        if (s->control[i] & AW_A10_PIT_TIMER_MODE) {
-            ptimer_stop(s->ptimer[i]);
-            s->control[i] &= ~AW_A10_PIT_TIMER_EN;
+        if (s->timer[i].control & AW_A10_PIT_TIMER_MODE) {
+            ptimer_stop(s->timer[i].ptimer);
+            s->timer[i].control &= ~AW_A10_PIT_TIMER_EN;
         }
         a10_pit_update_irq(s);
     }
@@ -294,7 +310,7 @@ static void a10_pit_init(Object *obj)
     uint8_t i;
 
     for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
-        sysbus_init_irq(sbd, &s->irq[i]);
+        sysbus_init_irq(sbd, &s->timer[i].irq);
     }
     memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
                           TYPE_AW_A10_PIT, 0x400);
@@ -305,7 +321,8 @@ static void a10_pit_init(Object *obj)
 
         tc->container = s;
         tc->index = i;
-        s->ptimer[i] = ptimer_init(a10_pit_timer_cb, tc, PTIMER_POLICY_DEFAULT);
+        s->timer[i].ptimer = ptimer_init(a10_pit_timer_cb, tc,
+                                         PTIMER_POLICY_DEFAULT);
     }
 }
 
-- 
2.21.0

[RFC PATCH 08/13] hw/timer/allwinner: Add a timer_count field

[Philippe Mathieu-Daudé]

To be able to support controllers with less than 6 timers, we
need a field to be able to iterate over the different count.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h |  1 +
 hw/timer/allwinner-a10-pit.c         | 10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index e0f864a954..8c64c33f01 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -24,6 +24,7 @@ struct AwA10PITState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
+    size_t timer_count;
     AwA10TimerContext timer[AW_PIT_TIMER_MAX];
     MemoryRegion iomem;
     uint32_t clk_freq[4];
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index ea92fdda32..3f47588703 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -58,7 +58,7 @@ static void a10_pit_update_irq(AwA10PITState *s)
 {
     int i;
 
-    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
+    for (i = 0; i < s->timer_count; i++) {
         qemu_set_irq(s->timer[i].irq,
                      !!(s->irq_status & s->irq_enable & (1 << i)));
     }
@@ -271,7 +271,7 @@ static void a10_pit_reset(DeviceState *dev)
     s->irq_status = 0;
     a10_pit_update_irq(s);
 
-    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
+    for (i = 0; i < s->timer_count; i++) {
         s->timer[i].control = AW_A10_PIT_DEFAULT_CLOCK;
         s->timer[i].interval = 0;
         s->timer[i].count = 0;
@@ -309,14 +309,16 @@ static void a10_pit_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     uint8_t i;
 
-    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
+    s->timer_count = AW_A10_PIT_TIMER_NR;
+
+    for (i = 0; i < s->timer_count; i++) {
         sysbus_init_irq(sbd, &s->timer[i].irq);
     }
     memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
                           TYPE_AW_A10_PIT, 0x400);
     sysbus_init_mmio(sbd, &s->iomem);
 
-    for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) {
+    for (i = 0; i < s->timer_count; i++) {
         AwA10TimerContext *tc = &s->timer[i];
 
         tc->container = s;
-- 
2.21.0

[RFC PATCH 09/13] hw/timer/allwinner: Rename AwA10TimerContext as AllwinnerTmrState

[Philippe Mathieu-Daudé]

This structure will be common to various Allwinner timer
controllers, rename it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/allwinner-a10-pit.h |  6 +++---
 hw/timer/allwinner-a10-pit.c         | 14 +++++++-------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 8c64c33f01..3a47633cc4 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -10,7 +10,7 @@
 
 typedef struct AwA10PITState AwA10PITState;
 
-typedef struct AwA10TimerContext {
+typedef struct AllwinnerTmrState {
     AwA10PITState *container;
     int index;
     ptimer_state *ptimer;
@@ -18,14 +18,14 @@ typedef struct AwA10TimerContext {
     uint32_t control;
     uint32_t interval;
     uint32_t count;
-} AwA10TimerContext;
+} AllwinnerTmrState;
 
 struct AwA10PITState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
     size_t timer_count;
-    AwA10TimerContext timer[AW_PIT_TIMER_MAX];
+    AllwinnerTmrState timer[AW_PIT_TIMER_MAX];
     MemoryRegion iomem;
     uint32_t clk_freq[4];
 
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 3f47588703..ecfc198937 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -234,10 +234,10 @@ static const VMStateDescription vmstate_aw_timer = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(control, AwA10TimerContext),
-        VMSTATE_UINT32(interval, AwA10TimerContext),
-        VMSTATE_UINT32(count, AwA10TimerContext),
-        VMSTATE_PTIMER(ptimer, AwA10TimerContext),
+        VMSTATE_UINT32(control, AllwinnerTmrState),
+        VMSTATE_UINT32(interval, AllwinnerTmrState),
+        VMSTATE_UINT32(count, AllwinnerTmrState),
+        VMSTATE_PTIMER(ptimer, AllwinnerTmrState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -252,7 +252,7 @@ static const VMStateDescription vmstate_a10_pit = {
         VMSTATE_STRUCT_ARRAY(timer, AwA10PITState,
                              AW_PIT_TIMER_MAX,
                              0, vmstate_aw_timer,
-                             AwA10TimerContext),
+                             AllwinnerTmrState),
         VMSTATE_UINT32(watch_dog_mode, AwA10PITState),
         VMSTATE_UINT32(watch_dog_control, AwA10PITState),
         VMSTATE_UINT32(count_lo, AwA10PITState),
@@ -289,7 +289,7 @@ static void a10_pit_reset(DeviceState *dev)
 
 static void a10_pit_timer_cb(void *opaque)
 {
-    AwA10TimerContext *tc = opaque;
+    AllwinnerTmrState *tc = opaque;
     AwA10PITState *s = tc->container;
     uint8_t i = tc->index;
 
@@ -319,7 +319,7 @@ static void a10_pit_init(Object *obj)
     sysbus_init_mmio(sbd, &s->iomem);
 
     for (i = 0; i < s->timer_count; i++) {
-        AwA10TimerContext *tc = &s->timer[i];
+        AllwinnerTmrState *tc = &s->timer[i];
 
         tc->container = s;
         tc->index = i;
-- 
2.21.0

[RFC PATCH 10/13] hw/timer/allwinner: Rename AwA10PITState as AllwinnerTmrCtrlState

[Philippe Mathieu-Daudé]

This structure will be common to various Allwinner timer
controllers, rename it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/allwinner-a10.h       |  2 +-
 include/hw/timer/allwinner-a10-pit.h |  6 ++--
 hw/timer/allwinner-a10-pit.c         | 42 ++++++++++++++--------------
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
index 7d2d215630..28c043db39 100644
--- a/include/hw/arm/allwinner-a10.h
+++ b/include/hw/arm/allwinner-a10.h
@@ -30,7 +30,7 @@ typedef struct AwA10State {
 
     ARMCPU cpu;
     qemu_irq irq[AW_A10_PIC_INT_NR];
-    AwA10PITState timer;
+    AllwinnerTmrCtrlState timer;
     AwA10PICState intc;
     AwEmacState emac;
     AllwinnerAHCIState sata;
diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 3a47633cc4..9e28c6697a 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -8,10 +8,10 @@
 
 #define AW_PIT_TIMER_MAX        6
 
-typedef struct AwA10PITState AwA10PITState;
+typedef struct AllwinnerTmrCtrlState AllwinnerTmrCtrlState;
 
 typedef struct AllwinnerTmrState {
-    AwA10PITState *container;
+    AllwinnerTmrCtrlState *container;
     int index;
     ptimer_state *ptimer;
     qemu_irq irq;
@@ -20,7 +20,7 @@ typedef struct AllwinnerTmrState {
     uint32_t count;
 } AllwinnerTmrState;
 
-struct AwA10PITState {
+struct AllwinnerTmrCtrlState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index ecfc198937..f2ac271e80 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -52,9 +52,9 @@
 #define AW_A10_PIT_DEFAULT_CLOCK    0x4
 
 #define AW_A10_PIT(obj) \
-    OBJECT_CHECK(AwA10PITState, (obj), TYPE_AW_A10_PIT)
+    OBJECT_CHECK(AllwinnerTmrCtrlState, (obj), TYPE_AW_A10_PIT)
 
-static void a10_pit_update_irq(AwA10PITState *s)
+static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
 {
     int i;
 
@@ -66,7 +66,7 @@ static void a10_pit_update_irq(AwA10PITState *s)
 
 static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
 {
-    AwA10PITState *s = AW_A10_PIT(opaque);
+    AllwinnerTmrCtrlState *s = AW_A10_PIT(opaque);
     uint8_t index;
 
     switch (offset) {
@@ -111,7 +111,7 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
 }
 
 /* Must be called inside a ptimer transaction block for s->timer[idx].ptimer */
-static void a10_pit_set_freq(AwA10PITState *s, int index)
+static void a10_pit_set_freq(AllwinnerTmrCtrlState *s, int index)
 {
     uint32_t prescaler, source, source_freq;
 
@@ -130,7 +130,7 @@ static void a10_pit_set_freq(AwA10PITState *s, int index)
 static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
                             unsigned size)
 {
-     AwA10PITState *s = AW_A10_PIT(opaque);
+     AllwinnerTmrCtrlState *s = AW_A10_PIT(opaque);
      uint8_t index;
 
     switch (offset) {
@@ -222,10 +222,10 @@ static const MemoryRegionOps a10_pit_ops = {
 };
 
 static Property a10_pit_properties[] = {
-    DEFINE_PROP_UINT32("clk0-freq", AwA10PITState, clk_freq[0], 0),
-    DEFINE_PROP_UINT32("clk1-freq", AwA10PITState, clk_freq[1], 0),
-    DEFINE_PROP_UINT32("clk2-freq", AwA10PITState, clk_freq[2], 0),
-    DEFINE_PROP_UINT32("clk3-freq", AwA10PITState, clk_freq[3], 0),
+    DEFINE_PROP_UINT32("clk0-freq", AllwinnerTmrCtrlState, clk_freq[0], 0),
+    DEFINE_PROP_UINT32("clk1-freq", AllwinnerTmrCtrlState, clk_freq[1], 0),
+    DEFINE_PROP_UINT32("clk2-freq", AllwinnerTmrCtrlState, clk_freq[2], 0),
+    DEFINE_PROP_UINT32("clk3-freq", AllwinnerTmrCtrlState, clk_freq[3], 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -247,24 +247,24 @@ static const VMStateDescription vmstate_a10_pit = {
     .version_id = 2,
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(irq_enable, AwA10PITState),
-        VMSTATE_UINT32(irq_status, AwA10PITState),
-        VMSTATE_STRUCT_ARRAY(timer, AwA10PITState,
+        VMSTATE_UINT32(irq_enable, AllwinnerTmrCtrlState),
+        VMSTATE_UINT32(irq_status, AllwinnerTmrCtrlState),
+        VMSTATE_STRUCT_ARRAY(timer, AllwinnerTmrCtrlState,
                              AW_PIT_TIMER_MAX,
                              0, vmstate_aw_timer,
                              AllwinnerTmrState),
-        VMSTATE_UINT32(watch_dog_mode, AwA10PITState),
-        VMSTATE_UINT32(watch_dog_control, AwA10PITState),
-        VMSTATE_UINT32(count_lo, AwA10PITState),
-        VMSTATE_UINT32(count_hi, AwA10PITState),
-        VMSTATE_UINT32(count_ctl, AwA10PITState),
+        VMSTATE_UINT32(watch_dog_mode, AllwinnerTmrCtrlState),
+        VMSTATE_UINT32(watch_dog_control, AllwinnerTmrCtrlState),
+        VMSTATE_UINT32(count_lo, AllwinnerTmrCtrlState),
+        VMSTATE_UINT32(count_hi, AllwinnerTmrCtrlState),
+        VMSTATE_UINT32(count_ctl, AllwinnerTmrCtrlState),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static void a10_pit_reset(DeviceState *dev)
 {
-    AwA10PITState *s = AW_A10_PIT(dev);
+    AllwinnerTmrCtrlState *s = AW_A10_PIT(dev);
     uint8_t i;
 
     s->irq_enable = 0;
@@ -290,7 +290,7 @@ static void a10_pit_reset(DeviceState *dev)
 static void a10_pit_timer_cb(void *opaque)
 {
     AllwinnerTmrState *tc = opaque;
-    AwA10PITState *s = tc->container;
+    AllwinnerTmrCtrlState *s = tc->container;
     uint8_t i = tc->index;
 
     if (s->timer[i].control & AW_A10_PIT_TIMER_EN) {
@@ -305,7 +305,7 @@ static void a10_pit_timer_cb(void *opaque)
 
 static void a10_pit_init(Object *obj)
 {
-    AwA10PITState *s = AW_A10_PIT(obj);
+    AllwinnerTmrCtrlState *s = AW_A10_PIT(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     uint8_t i;
 
@@ -341,7 +341,7 @@ static void a10_pit_class_init(ObjectClass *klass, void *data)
 static const TypeInfo a10_pit_info = {
     .name = TYPE_AW_A10_PIT,
     .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(AwA10PITState),
+    .instance_size = sizeof(AllwinnerTmrCtrlState),
     .instance_init = a10_pit_init,
     .class_init = a10_pit_class_init,
 };
-- 
2.21.0

[RFC PATCH 11/13] hw/timer/allwinner: Introduce TYPE_AW_COMMON_PIT abstract device

[Philippe Mathieu-Daudé]

Extract the common code from the TYPE_AW_A10_PIT device into a new
abstract device: TYPE_AW_COMMON_PIT, then use it as parent, so we
inherit the same functionalities.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
At this point, the only fields we can modify are the timer_count
and the region_size. Not enough to implement the H3 timer, since
we need to move the WDOG register. Still some progress, so Niek
can continue ;)
---
 include/hw/timer/allwinner-a10-pit.h |  1 +
 hw/timer/allwinner-a10-pit.c         | 50 +++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/include/hw/timer/allwinner-a10-pit.h b/include/hw/timer/allwinner-a10-pit.h
index 9e28c6697a..8453a62706 100644
--- a/include/hw/timer/allwinner-a10-pit.h
+++ b/include/hw/timer/allwinner-a10-pit.h
@@ -4,6 +4,7 @@
 #include "hw/ptimer.h"
 #include "hw/sysbus.h"
 
+#define TYPE_AW_COMMON_PIT "allwinner-timer-controller"
 #define TYPE_AW_A10_PIT "allwinner-A10-timer"
 
 #define AW_PIT_TIMER_MAX        6
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index f2ac271e80..ad409b96a1 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -54,6 +54,20 @@
 #define AW_A10_PIT(obj) \
     OBJECT_CHECK(AllwinnerTmrCtrlState, (obj), TYPE_AW_A10_PIT)
 
+typedef struct AllwinnerTmrCtrlClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+
+    size_t timer_count;
+    size_t region_size;
+} AllwinnerTmrCtrlClass;
+
+#define AW_TIMER_CLASS(klass) \
+     OBJECT_CLASS_CHECK(AllwinnerTmrCtrlClass, (klass), TYPE_AW_COMMON_PIT)
+#define AW_TIMER_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(AllwinnerTmrCtrlClass, (obj), TYPE_AW_COMMON_PIT)
+
 static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
 {
     int i;
@@ -303,19 +317,20 @@ static void a10_pit_timer_cb(void *opaque)
     }
 }
 
-static void a10_pit_init(Object *obj)
+static void aw_pit_instance_init(Object *obj)
 {
     AllwinnerTmrCtrlState *s = AW_A10_PIT(obj);
+    AllwinnerTmrCtrlClass *c = AW_TIMER_GET_CLASS(s);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     uint8_t i;
 
-    s->timer_count = AW_A10_PIT_TIMER_NR;
+    s->timer_count = c->timer_count;
 
     for (i = 0; i < s->timer_count; i++) {
         sysbus_init_irq(sbd, &s->timer[i].irq);
     }
     memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
-                          TYPE_AW_A10_PIT, 0x400);
+                          TYPE_AW_A10_PIT, c->region_size);
     sysbus_init_mmio(sbd, &s->iomem);
 
     for (i = 0; i < s->timer_count; i++) {
@@ -328,26 +343,45 @@ static void a10_pit_init(Object *obj)
     }
 }
 
-static void a10_pit_class_init(ObjectClass *klass, void *data)
+static void aw_timer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->reset = a10_pit_reset;
     dc->props = a10_pit_properties;
-    dc->desc = "allwinner a10 timer";
+    dc->desc = "Allwinner Timer Controller";
     dc->vmsd = &vmstate_a10_pit;
 }
 
+static const TypeInfo allwinner_pit_info = {
+    .name = TYPE_AW_COMMON_PIT,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = aw_pit_instance_init,
+    .instance_size = sizeof(AllwinnerTmrCtrlState),
+    .class_init = aw_timer_class_init,
+    .class_size = sizeof(AllwinnerTmrCtrlClass),
+    .abstract   = true,
+};
+
+static void a10_pit_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AllwinnerTmrCtrlClass *atc = AW_TIMER_CLASS(klass);
+
+    dc->desc = "Allwinner A10 Timer Controller";
+    atc->timer_count = AW_A10_PIT_TIMER_NR;
+    atc->region_size = 0x400;
+}
+
 static const TypeInfo a10_pit_info = {
     .name = TYPE_AW_A10_PIT,
-    .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(AllwinnerTmrCtrlState),
-    .instance_init = a10_pit_init,
+    .parent = TYPE_AW_COMMON_PIT,
     .class_init = a10_pit_class_init,
 };
 
 static void a10_register_types(void)
 {
+    type_register_static(&allwinner_pit_info);
     type_register_static(&a10_pit_info);
 }
 
-- 
2.21.0

Re: [RFC PATCH 11/13] hw/timer/allwinner: Introduce TYPE_AW_COMMON_PIT abstract device

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 5026 bytes --]

Hi Philippe,

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Extract the common code from the TYPE_AW_A10_PIT device into a new
> abstract device: TYPE_AW_COMMON_PIT, then use it as parent, so we
> inherit the same functionalities.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> At this point, the only fields we can modify are the timer_count
> and the region_size. Not enough to implement the H3 timer, since
> we need to move the WDOG register. Still some progress, so Niek
> can continue ;)
> ---
>  include/hw/timer/allwinner-a10-pit.h |  1 +
>  hw/timer/allwinner-a10-pit.c         | 50 +++++++++++++++++++++++-----
>  2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h
> b/include/hw/timer/allwinner-a10-pit.h
> index 9e28c6697a..8453a62706 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -4,6 +4,7 @@
>  #include "hw/ptimer.h"
>  #include "hw/sysbus.h"
>
> +#define TYPE_AW_COMMON_PIT "allwinner-timer-controller"
>  #define TYPE_AW_A10_PIT "allwinner-A10-timer"
>

So for the Allwinner H3, that means we'll need another TYPE_AW_H3_PIT
definition?


>
>  #define AW_PIT_TIMER_MAX        6
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index f2ac271e80..ad409b96a1 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
>

Perhaps we can rename the hw/timer/allwinner-a10-pit.c to a generic name,
for example hw/timer/allwinner-pit.c ?


> @@ -54,6 +54,20 @@
>  #define AW_A10_PIT(obj) \
>      OBJECT_CHECK(AllwinnerTmrCtrlState, (obj), TYPE_AW_A10_PIT)
>
> +typedef struct AllwinnerTmrCtrlClass {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +    /*< public >*/
> +
> +    size_t timer_count;
> +    size_t region_size;
> +} AllwinnerTmrCtrlClass;
> +
> +#define AW_TIMER_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(AllwinnerTmrCtrlClass, (klass),
> TYPE_AW_COMMON_PIT)
> +#define AW_TIMER_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(AllwinnerTmrCtrlClass, (obj), TYPE_AW_COMMON_PIT)
> +
>  static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
>  {
>      int i;
> @@ -303,19 +317,20 @@ static void a10_pit_timer_cb(void *opaque)
>      }
>  }
>
> -static void a10_pit_init(Object *obj)
> +static void aw_pit_instance_init(Object *obj)
>  {
>      AllwinnerTmrCtrlState *s = AW_A10_PIT(obj);
> +    AllwinnerTmrCtrlClass *c = AW_TIMER_GET_CLASS(s);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      uint8_t i;
>
> -    s->timer_count = AW_A10_PIT_TIMER_NR;
> +    s->timer_count = c->timer_count;
>
>      for (i = 0; i < s->timer_count; i++) {
>          sysbus_init_irq(sbd, &s->timer[i].irq);
>      }
>      memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
>

I am curious how to support the different WDOG0 registers for the Allwinner
H3 while keeping
the A10 functionality also working :-) Will you give the TYPE_AW_H3_PIT
its own MemoryRegionOps with read/write?


> -                          TYPE_AW_A10_PIT, 0x400);
> +                          TYPE_AW_A10_PIT, c->region_size);
>      sysbus_init_mmio(sbd, &s->iomem);
>
>      for (i = 0; i < s->timer_count; i++) {
> @@ -328,26 +343,45 @@ static void a10_pit_init(Object *obj)
>      }
>  }
>
> -static void a10_pit_class_init(ObjectClass *klass, void *data)
> +static void aw_timer_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->reset = a10_pit_reset;
>      dc->props = a10_pit_properties;
> -    dc->desc = "allwinner a10 timer";
> +    dc->desc = "Allwinner Timer Controller";
>      dc->vmsd = &vmstate_a10_pit;
>  }
>
> +static const TypeInfo allwinner_pit_info = {
> +    .name = TYPE_AW_COMMON_PIT,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_init = aw_pit_instance_init,
> +    .instance_size = sizeof(AllwinnerTmrCtrlState),
> +    .class_init = aw_timer_class_init,
> +    .class_size = sizeof(AllwinnerTmrCtrlClass),
> +    .abstract   = true,
> +};
> +
> +static void a10_pit_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AllwinnerTmrCtrlClass *atc = AW_TIMER_CLASS(klass);
> +
> +    dc->desc = "Allwinner A10 Timer Controller";
> +    atc->timer_count = AW_A10_PIT_TIMER_NR;
> +    atc->region_size = 0x400;
> +}
> +
>  static const TypeInfo a10_pit_info = {
>      .name = TYPE_AW_A10_PIT,
> -    .parent = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(AllwinnerTmrCtrlState),
> -    .instance_init = a10_pit_init,
> +    .parent = TYPE_AW_COMMON_PIT,
>      .class_init = a10_pit_class_init,
>  };
>
>  static void a10_register_types(void)
>  {
> +    type_register_static(&allwinner_pit_info);
>      type_register_static(&a10_pit_info);
>  }
>
> --
> 2.21.0
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 6596 bytes --]

[RFC PATCH 12/13] hw/timer/allwinner: Rename AW_A10_PIT() as AW_TIMER_CTRL()

[Philippe Mathieu-Daudé]

This macro is now used by different Allwinner timer controllers,
rename it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/timer/allwinner-a10-pit.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index ad409b96a1..7413f046cc 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -51,8 +51,8 @@
 
 #define AW_A10_PIT_DEFAULT_CLOCK    0x4
 
-#define AW_A10_PIT(obj) \
-    OBJECT_CHECK(AllwinnerTmrCtrlState, (obj), TYPE_AW_A10_PIT)
+#define AW_TIMER_CTRL(obj) \
+    OBJECT_CHECK(AllwinnerTmrCtrlState, (obj), TYPE_AW_COMMON_PIT)
 
 typedef struct AllwinnerTmrCtrlClass {
     /*< private >*/
@@ -80,7 +80,7 @@ static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
 
 static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
 {
-    AllwinnerTmrCtrlState *s = AW_A10_PIT(opaque);
+    AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(opaque);
     uint8_t index;
 
     switch (offset) {
@@ -144,7 +144,7 @@ static void a10_pit_set_freq(AllwinnerTmrCtrlState *s, int index)
 static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
                             unsigned size)
 {
-     AllwinnerTmrCtrlState *s = AW_A10_PIT(opaque);
+     AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(opaque);
      uint8_t index;
 
     switch (offset) {
@@ -278,7 +278,7 @@ static const VMStateDescription vmstate_a10_pit = {
 
 static void a10_pit_reset(DeviceState *dev)
 {
-    AllwinnerTmrCtrlState *s = AW_A10_PIT(dev);
+    AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(dev);
     uint8_t i;
 
     s->irq_enable = 0;
@@ -319,7 +319,7 @@ static void a10_pit_timer_cb(void *opaque)
 
 static void aw_pit_instance_init(Object *obj)
 {
-    AllwinnerTmrCtrlState *s = AW_A10_PIT(obj);
+    AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(obj);
     AllwinnerTmrCtrlClass *c = AW_TIMER_GET_CLASS(s);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     uint8_t i;
@@ -330,7 +330,7 @@ static void aw_pit_instance_init(Object *obj)
         sysbus_init_irq(sbd, &s->timer[i].irq);
     }
     memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
-                          TYPE_AW_A10_PIT, c->region_size);
+                          TYPE_AW_COMMON_PIT, c->region_size);
     sysbus_init_mmio(sbd, &s->iomem);
 
     for (i = 0; i < s->timer_count; i++) {
-- 
2.21.0

[RFC PATCH 13/13] hw/timer/allwinner: Rename functions not specific to the A10 SoC

[Philippe Mathieu-Daudé]

These functions are used by different Allwinner timer controllers,
rename them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/timer/allwinner-a10-pit.c | 52 ++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index 7413f046cc..dff534cfef 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -68,7 +68,7 @@ typedef struct AllwinnerTmrCtrlClass {
 #define AW_TIMER_GET_CLASS(obj) \
      OBJECT_GET_CLASS(AllwinnerTmrCtrlClass, (obj), TYPE_AW_COMMON_PIT)
 
-static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
+static void allwinner_timer_update_irq(AllwinnerTmrCtrlState *s)
 {
     int i;
 
@@ -78,7 +78,7 @@ static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
     }
 }
 
-static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
+static uint64_t allwinner_timer_read(void *opaque, hwaddr offset, unsigned size)
 {
     AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(opaque);
     uint8_t index;
@@ -125,7 +125,7 @@ static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
 }
 
 /* Must be called inside a ptimer transaction block for s->timer[idx].ptimer */
-static void a10_pit_set_freq(AllwinnerTmrCtrlState *s, int index)
+static void allwinner_ptimer_set_freq(AllwinnerTmrCtrlState *s, int index)
 {
     uint32_t prescaler, source, source_freq;
 
@@ -141,8 +141,8 @@ static void a10_pit_set_freq(AllwinnerTmrCtrlState *s, int index)
     }
 }
 
-static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
-                            unsigned size)
+static void allwinner_timer_write(void *opaque, hwaddr offset,
+                                  uint64_t value, unsigned size)
 {
      AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(opaque);
      uint8_t index;
@@ -150,11 +150,11 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
     switch (offset) {
     case AW_A10_PIT_TIMER_IRQ_EN:
         s->irq_enable = value;
-        a10_pit_update_irq(s);
+        allwinner_timer_update_irq(s);
         break;
     case AW_A10_PIT_TIMER_IRQ_ST:
         s->irq_status &= ~value;
-        a10_pit_update_irq(s);
+        allwinner_timer_update_irq(s);
         break;
     case AW_A10_PIT_TIMER_BASE ... AW_A10_PIT_TIMER_BASE_END:
         index = offset & 0xf0;
@@ -164,7 +164,7 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
         case AW_A10_PIT_TIMER_CONTROL:
             s->timer[index].control = value;
             ptimer_transaction_begin(s->timer[index].ptimer);
-            a10_pit_set_freq(s, index);
+            allwinner_ptimer_set_freq(s, index);
             if (s->timer[index].control & AW_A10_PIT_TIMER_RELOAD) {
                 ptimer_set_count(s->timer[index].ptimer,
                                  s->timer[index].interval);
@@ -229,13 +229,13 @@ static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value,
     }
 }
 
-static const MemoryRegionOps a10_pit_ops = {
-    .read = a10_pit_read,
-    .write = a10_pit_write,
+static const MemoryRegionOps allwinner_timer_ops = {
+    .read = allwinner_timer_read,
+    .write = allwinner_timer_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static Property a10_pit_properties[] = {
+static Property allwinner_timer_properties[] = {
     DEFINE_PROP_UINT32("clk0-freq", AllwinnerTmrCtrlState, clk_freq[0], 0),
     DEFINE_PROP_UINT32("clk1-freq", AllwinnerTmrCtrlState, clk_freq[1], 0),
     DEFINE_PROP_UINT32("clk2-freq", AllwinnerTmrCtrlState, clk_freq[2], 0),
@@ -276,14 +276,14 @@ static const VMStateDescription vmstate_a10_pit = {
     }
 };
 
-static void a10_pit_reset(DeviceState *dev)
+static void allwinner_timer_reset(DeviceState *dev)
 {
     AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(dev);
     uint8_t i;
 
     s->irq_enable = 0;
     s->irq_status = 0;
-    a10_pit_update_irq(s);
+    allwinner_timer_update_irq(s);
 
     for (i = 0; i < s->timer_count; i++) {
         s->timer[i].control = AW_A10_PIT_DEFAULT_CLOCK;
@@ -291,7 +291,7 @@ static void a10_pit_reset(DeviceState *dev)
         s->timer[i].count = 0;
         ptimer_transaction_begin(s->timer[i].ptimer);
         ptimer_stop(s->timer[i].ptimer);
-        a10_pit_set_freq(s, i);
+        allwinner_ptimer_set_freq(s, i);
         ptimer_transaction_commit(s->timer[i].ptimer);
     }
     s->watch_dog_mode = 0;
@@ -301,7 +301,7 @@ static void a10_pit_reset(DeviceState *dev)
     s->count_ctl = 0;
 }
 
-static void a10_pit_timer_cb(void *opaque)
+static void allwinner_ptimer_cb(void *opaque)
 {
     AllwinnerTmrState *tc = opaque;
     AllwinnerTmrCtrlState *s = tc->container;
@@ -313,11 +313,11 @@ static void a10_pit_timer_cb(void *opaque)
             ptimer_stop(s->timer[i].ptimer);
             s->timer[i].control &= ~AW_A10_PIT_TIMER_EN;
         }
-        a10_pit_update_irq(s);
+        allwinner_timer_update_irq(s);
     }
 }
 
-static void aw_pit_instance_init(Object *obj)
+static void allwinner_timer_instance_init(Object *obj)
 {
     AllwinnerTmrCtrlState *s = AW_TIMER_CTRL(obj);
     AllwinnerTmrCtrlClass *c = AW_TIMER_GET_CLASS(s);
@@ -329,8 +329,8 @@ static void aw_pit_instance_init(Object *obj)
     for (i = 0; i < s->timer_count; i++) {
         sysbus_init_irq(sbd, &s->timer[i].irq);
     }
-    memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
-                          TYPE_AW_COMMON_PIT, c->region_size);
+    memory_region_init_io(&s->iomem, OBJECT(s), &allwinner_timer_ops,
+                          s, TYPE_AW_COMMON_PIT, c->region_size);
     sysbus_init_mmio(sbd, &s->iomem);
 
     for (i = 0; i < s->timer_count; i++) {
@@ -338,7 +338,7 @@ static void aw_pit_instance_init(Object *obj)
 
         tc->container = s;
         tc->index = i;
-        s->timer[i].ptimer = ptimer_init(a10_pit_timer_cb, tc,
+        s->timer[i].ptimer = ptimer_init(allwinner_ptimer_cb, tc,
                                          PTIMER_POLICY_DEFAULT);
     }
 }
@@ -347,8 +347,8 @@ static void aw_timer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->reset = a10_pit_reset;
-    dc->props = a10_pit_properties;
+    dc->reset = allwinner_timer_reset;
+    dc->props = allwinner_timer_properties;
     dc->desc = "Allwinner Timer Controller";
     dc->vmsd = &vmstate_a10_pit;
 }
@@ -356,7 +356,7 @@ static void aw_timer_class_init(ObjectClass *klass, void *data)
 static const TypeInfo allwinner_pit_info = {
     .name = TYPE_AW_COMMON_PIT,
     .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_init = aw_pit_instance_init,
+    .instance_init = allwinner_timer_instance_init,
     .instance_size = sizeof(AllwinnerTmrCtrlState),
     .class_init = aw_timer_class_init,
     .class_size = sizeof(AllwinnerTmrCtrlClass),
@@ -379,10 +379,10 @@ static const TypeInfo a10_pit_info = {
     .class_init = a10_pit_class_init,
 };
 
-static void a10_register_types(void)
+static void allwinner_timer_register_types(void)
 {
     type_register_static(&allwinner_pit_info);
     type_register_static(&a10_pit_info);
 }
 
-type_init(a10_register_types);
+type_init(allwinner_timer_register_types);
-- 
2.21.0

Re: [RFC PATCH 00/13] hw/timer/allwinner: Make it reusable

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 4658 bytes --]

Hi Philippe,

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi,
>
> Niek added the H3 SoC in [1] and noticed in [2] the timer
> controller is very similar (less timers, watchdog register
> placed at different address).
>
> On 12/18/19 9:14 PM, Niek Linnenbank wrote:
> > Actually, I copied the timer support code from the existing cubieboard.c
> > that has
> > the Allwinner A10, so potentially the same problem is there.
> >
> > While looking more closer at this part, I now also discovered that the
> > timer module from the Allwinner H3 is
> > mostly a stripped down version of the timer module in the Allwinner A10:
> >
> >    Allwinner A10, 10.2 Timer Register List, page 85:
> > https://linux-sunxi.org/images/1/1e/Allwinner_A10_User_manual_V1.5.pdf
> >
> > The A10 version has six timers, where the H3 has only two. That should
> > be fine I would say, the guest would simply
> > use those available on H3 and ignore the rest. There is however one
> > conflicting difference: the WDOG0 registers in the Allwinner H3 start
> > at a different offset and are also different. The current A10 timer does
> > not currently implement the watchdog part.
> [...]
> > So in my opinion its a bit of a trade off here: we can keep it like this
> > and re-use the A10 timer for now, and perhaps
> > attempt to generalize that module for proper use in both SoCs. Or we can
> > introduce a new H3 specific timer module.
> > What do you think?
>
> As an answer to his question, this series is to help him to
> reuse the A10 timer controller instead of adding a new model
> to the codebase.
>

Great!! This certainly answers my question indeed!

I've applied this patch on top of the allwinner H3 v2 series to test it,
and after
changing the type from AwA10PITState to the new AllwinnerTmrCtrlState,
the code compiled and ran linux/u-boot without any problems:

diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
index 357bdfa711..fa0219fa1b 100644
--- a/include/hw/arm/allwinner-h3.h
+++ b/include/hw/arm/allwinner-h3.h
@@ -76,7 +76,7 @@ typedef struct AwH3State {

     ARMCPU cpus[AW_H3_NUM_CPUS];
     const hwaddr *memmap;
-    AwA10PITState timer;
+    AllwinnerTmrCtrlState timer;
     AwH3ClockState ccu;
     AwH3CpuCfgState cpucfg;
     AwH3SysconState syscon;

Also, I tested with the A10 cubieboard machine, and it also still works
fine:

./arm-softmmu/qemu-system-arm -M cubieboard -kernel zImage -nographic
-append 'console=ttyS0,115200 earlyprintk usbcore.nousb root=/dev/sda ro
init=/sbin/init' -dtb sun4i-a10-cubieboard.dtb -m 512 -drive
file=rootfs.ext2,if=none,id=drive-sata0-0-0,format=raw -device
ide-hd,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 -nic user
[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 5.2.11 (me@host) (gcc version 5.4.0 20160609
(Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9)) #1 SMP Fri Sep 13 22:48:39 CEST 2019
[    0.000000] CPU: ARMv7 Processor [410fc080] revision 0 (ARMv7),
cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT nonaliasing
instruction cache
[    0.000000] OF: fdt: Machine model: Cubietech Cubieboard
...

So for me this works with both the H3 and A10:
  Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Regards,
Niek

>
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg665532.html
> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg666304.html
>
> Philippe Mathieu-Daudé (13):
>   hw/timer/allwinner: Use the AW_A10_PIT_TIMER_NR definition
>   hw/timer/allwinner: Add AW_PIT_TIMER_MAX definition
>   hw/timer/allwinner: Remove unused definitions
>   hw/timer/allwinner: Move definitions from header to source
>   hw/timer/allwinner: Rename the ptimer field
>   hw/timer/allwinner: Rename 'timer_context' as 'timer'
>   hw/timer/allwinner: Move timer specific fields into AwA10TimerContext
>   hw/timer/allwinner: Add a timer_count field
>   hw/timer/allwinner: Rename AwA10TimerContext as AllwinnerTmrState
>   hw/timer/allwinner: Rename AwA10PITState as AllwinnerTmrCtrlState
>   hw/timer/allwinner: Introduce TYPE_AW_COMMON_PIT abstract device
>   hw/timer/allwinner: Rename AW_A10_PIT() as AW_TIMER_CTRL()
>   hw/timer/allwinner: Rename functions not specific to the A10 SoC
>
>  include/hw/arm/allwinner-a10.h       |   2 +-
>  include/hw/timer/allwinner-a10-pit.h |  54 ++----
>  hw/timer/allwinner-a10-pit.c         | 271 +++++++++++++++++----------
>  3 files changed, 192 insertions(+), 135 deletions(-)
>
> --
> 2.21.0
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 6493 bytes --]