[PATCH 0/7] mps2: Add few more peripherals

[PATCH 0/7] mps2: Add few more peripherals

[Philippe Mathieu-Daudé]

Few patches while playing with Zephyr:

- added i2c
- added fpgaio
- added gpio/wdg as unimp

Unfortunately the blinkled sample app is not blinking.

Zephyr ressources used:
https://docs.zephyrproject.org/latest/boards/arm/mps2_an385/doc/index.html

Philippe Mathieu-Daudé (7):
  hw/arm/mps2: Rename CMSDK AHB peripheral region
  hw/arm/mps2: Rename dualtimer as apb-dualtimer
  hw/arm/mps2: Add CMSDK APB watchdog as unimplemented device
  hw/arm/mps2: Add CMSDK AHB GPIO peripherals as unimplemented devices
  hw/arm/mps2: Add I2C busses on FPGA APB
  hw/arm/mps2: Map the FPGA I/O block
  hw/misc/mps2-fpgaio: Implement push-buttons

 include/hw/misc/mps2-fpgaio.h |  1 +
 hw/arm/mps2.c                 | 29 +++++++++++++++++++++++++----
 hw/misc/mps2-fpgaio.c         | 13 ++++++++++++-
 3 files changed, 38 insertions(+), 5 deletions(-)

-- 
2.21.3

[PATCH 1/7] hw/arm/mps2: Rename CMSDK AHB peripheral region

[Philippe Mathieu-Daudé]

To differenciate with the CMSDK APB peripheral region,
rename this region 'CMSDK AHB peripheral region'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index f246213206..3cc02c9018 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -212,10 +212,11 @@ static void mps2_common_init(MachineState *machine)
      */
     create_unimplemented_device("CMSDK APB peripheral region @0x40000000",
                                 0x40000000, 0x00010000);
-    create_unimplemented_device("CMSDK peripheral region @0x40010000",
+    create_unimplemented_device("CMSDK AHB peripheral region @0x40010000",
                                 0x40010000, 0x00010000);
     create_unimplemented_device("Extra peripheral region @0x40020000",
                                 0x40020000, 0x00010000);
+
     create_unimplemented_device("RESERVED 4", 0x40030000, 0x001D0000);
     create_unimplemented_device("VGA", 0x41000000, 0x0200000);
 
-- 
2.21.3

Re: [PATCH 1/7] hw/arm/mps2: Rename CMSDK AHB peripheral region

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> To differenciate with the CMSDK APB peripheral region,
> rename this region 'CMSDK AHB peripheral region'.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

thanks
-- PMM

[PATCH 2/7] hw/arm/mps2: Rename dualtimer as apb-dualtimer

[Philippe Mathieu-Daudé]

The dualtimer is connected to AMBA APB.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 3cc02c9018..1e8d5f21f9 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -306,7 +306,7 @@ static void mps2_common_init(MachineState *machine)
     cmsdk_apb_timer_create(0x40000000, qdev_get_gpio_in(armv7m, 8), SYSCLK_FRQ);
     cmsdk_apb_timer_create(0x40001000, qdev_get_gpio_in(armv7m, 9), SYSCLK_FRQ);
 
-    sysbus_init_child_obj(OBJECT(mms), "dualtimer", &mms->dualtimer,
+    sysbus_init_child_obj(OBJECT(mms), "apb-dualtimer", &mms->dualtimer,
                           sizeof(mms->dualtimer), TYPE_CMSDK_APB_DUALTIMER);
     qdev_prop_set_uint32(DEVICE(&mms->dualtimer), "pclk-frq", SYSCLK_FRQ);
     object_property_set_bool(OBJECT(&mms->dualtimer), true, "realized",
-- 
2.21.3

Re: [PATCH 2/7] hw/arm/mps2: Rename dualtimer as apb-dualtimer

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The dualtimer is connected to AMBA APB.

True, but that doesn't mean we need to name it "apb-something".
The AN385 TRM just calls it "Dual timer".

thanks
-- PMM

[PATCH 3/7] hw/arm/mps2: Add CMSDK APB watchdog as unimplemented device

[Philippe Mathieu-Daudé]

Register the watchdog peripheral as unimplemented to better
follow its accesses, for example booting Zephyr:

  ----------------
  IN: wdog_cmsdk_apb_init
  0x00000900:  b508       push     {r3, lr}
  0x00000902:  4b07       ldr      r3, [pc, #0x1c]
  0x00000904:  4a07       ldr      r2, [pc, #0x1c]
  0x00000906:  6018       str      r0, [r3]
  0x00000908:  4b07       ldr      r3, [pc, #0x1c]
  0x0000090a:  4808       ldr      r0, [pc, #0x20]
  0x0000090c:  f8c3 2c00  str.w    r2, [r3, #0xc00]
  0x00000910:  4a07       ldr      r2, [pc, #0x1c]
  0x00000912:  6812       ldr      r2, [r2]
  0x00000914:  601a       str      r2, [r3]
  0x00000916:  f000 f9f9  bl       #0xd0c
  cmsdk-apb-watchdog: unimplemented device write (size 4, value 0x1acce551, offset 0xc00)
  cmsdk-apb-watchdog: unimplemented device write (size 4, value 0xe4e1c00, offset 0x0)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 1e8d5f21f9..570ec50aa8 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -314,6 +314,7 @@ static void mps2_common_init(MachineState *machine)
     sysbus_connect_irq(SYS_BUS_DEVICE(&mms->dualtimer), 0,
                        qdev_get_gpio_in(armv7m, 10));
     sysbus_mmio_map(SYS_BUS_DEVICE(&mms->dualtimer), 0, 0x40002000);
+    create_unimplemented_device("cmsdk-apb-watchdog", 0x40008000, 0x1000);
 
     sysbus_init_child_obj(OBJECT(mms), "scc", &mms->scc,
                           sizeof(mms->scc), TYPE_MPS2_SCC);
-- 
2.21.3

Re: [PATCH 3/7] hw/arm/mps2: Add CMSDK APB watchdog as unimplemented device

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Register the watchdog peripheral as unimplemented to better
> follow its accesses, for example booting Zephyr:

We have a TYPE_CMSDK_APB_WATCHDOG device; so we could use
that, I think ?

thanks
-- PMM

Re: [PATCH 3/7] hw/arm/mps2: Add CMSDK APB watchdog as unimplemented device

[Philippe Mathieu-Daudé]

On 6/16/20 12:17 PM, Peter Maydell wrote:
> On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Register the watchdog peripheral as unimplemented to better
>> follow its accesses, for example booting Zephyr:
> 
> We have a TYPE_CMSDK_APB_WATCHDOG device; so we could use
> that, I think ?

Oh I missed it, excellent! Thanks :)

> 
> thanks
> -- PMM
> 

[PATCH 4/7] hw/arm/mps2: Add CMSDK AHB GPIO peripherals as unimplemented devices

[Philippe Mathieu-Daudé]

Register the GPIO peripherals as unimplemented to better
follow their accesses, for example booting Zephyr:

  ----------------
  IN: arm_mps2_pinmux_init
  0x00001160:  f64f 0231  movw     r2, #0xf831
  0x00001164:  4b06       ldr      r3, [pc, #0x18]
  0x00001166:  2000       movs     r0, #0
  0x00001168:  619a       str      r2, [r3, #0x18]
  0x0000116a:  f24c 426f  movw     r2, #0xc46f
  0x0000116e:  f503 5380  add.w    r3, r3, #0x1000
  0x00001172:  619a       str      r2, [r3, #0x18]
  0x00001174:  f44f 529e  mov.w    r2, #0x13c0
  0x00001178:  f503 5380  add.w    r3, r3, #0x1000
  0x0000117c:  619a       str      r2, [r3, #0x18]
  0x0000117e:  4770       bx       lr
  cmsdk-ahb-gpio: unimplemented device write (size 4, value 0xf831, offset 0x18)
  cmsdk-ahb-gpio: unimplemented device write (size 4, value 0xc46f, offset 0x18)
  cmsdk-ahb-gpio: unimplemented device write (size 4, value 0x13c0, offset 0x18)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 570ec50aa8..4a49bfa9b9 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -111,6 +111,7 @@ static void mps2_common_init(MachineState *machine)
     MemoryRegion *system_memory = get_system_memory();
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     DeviceState *armv7m, *sccdev;
+    int i;
 
     if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
         error_report("This board can only be used with CPU %s",
@@ -228,7 +229,6 @@ static void mps2_common_init(MachineState *machine)
          */
         Object *orgate;
         DeviceState *orgate_dev;
-        int i;
 
         orgate = object_new(TYPE_OR_IRQ);
         object_property_set_int(orgate, 6, "num-lines", &error_fatal);
@@ -265,7 +265,6 @@ static void mps2_common_init(MachineState *machine)
          */
         Object *orgate;
         DeviceState *orgate_dev;
-        int i;
 
         orgate = object_new(TYPE_OR_IRQ);
         object_property_set_int(orgate, 10, "num-lines", &error_fatal);
@@ -315,6 +314,12 @@ static void mps2_common_init(MachineState *machine)
                        qdev_get_gpio_in(armv7m, 10));
     sysbus_mmio_map(SYS_BUS_DEVICE(&mms->dualtimer), 0, 0x40002000);
     create_unimplemented_device("cmsdk-apb-watchdog", 0x40008000, 0x1000);
+    for (i = 0; i < 4; i++) {
+        static const hwaddr gpiobase[] = {0x40010000, 0x40011000,
+                                          0x40012000, 0x40013000};
+
+        create_unimplemented_device("cmsdk-ahb-gpio", gpiobase[i], 0x1000);
+    }
 
     sysbus_init_child_obj(OBJECT(mms), "scc", &mms->scc,
                           sizeof(mms->scc), TYPE_MPS2_SCC);
-- 
2.21.3

Re: [PATCH 4/7] hw/arm/mps2: Add CMSDK AHB GPIO peripherals as unimplemented devices

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Register the GPIO peripherals as unimplemented to better
> follow their accesses, for example booting Zephyr:
>

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

thanks
-- PMM

[PATCH 5/7] hw/arm/mps2: Add I2C busses on FPGA APB

[Philippe Mathieu-Daudé]

There are 4 different I2C peripherals on the FPGA APB.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 4a49bfa9b9..6224d7a63c 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -321,6 +321,7 @@ static void mps2_common_init(MachineState *machine)
         create_unimplemented_device("cmsdk-ahb-gpio", gpiobase[i], 0x1000);
     }
 
+    /* FPGA APB */
     sysbus_init_child_obj(OBJECT(mms), "scc", &mms->scc,
                           sizeof(mms->scc), TYPE_MPS2_SCC);
     sccdev = DEVICE(&mms->scc);
@@ -330,6 +331,12 @@ static void mps2_common_init(MachineState *machine)
     object_property_set_bool(OBJECT(&mms->scc), true, "realized",
                              &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(sccdev), 0, 0x4002f000);
+    for (i = 0; i < 4; i++) {
+        static const hwaddr i2cbase[] = {0x40022000, 0x40023000,
+                                         0x40029000, 0x4002a000};
+
+        sysbus_create_simple("versatile_i2c", i2cbase[i], NULL);
+    }
 
     /* In hardware this is a LAN9220; the LAN9118 is software compatible
      * except that it doesn't support the checksum-offload feature.
-- 
2.21.3

Re: [PATCH 5/7] hw/arm/mps2: Add I2C busses on FPGA APB

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> There are 4 different I2C peripherals on the FPGA APB.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/mps2.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
> index 4a49bfa9b9..6224d7a63c 100644
> --- a/hw/arm/mps2.c
> +++ b/hw/arm/mps2.c
> @@ -321,6 +321,7 @@ static void mps2_common_init(MachineState *machine)
>          create_unimplemented_device("cmsdk-ahb-gpio", gpiobase[i], 0x1000);
>      }
>
> +    /* FPGA APB */
>      sysbus_init_child_obj(OBJECT(mms), "scc", &mms->scc,
>                            sizeof(mms->scc), TYPE_MPS2_SCC);
>      sccdev = DEVICE(&mms->scc);
> @@ -330,6 +331,12 @@ static void mps2_common_init(MachineState *machine)
>      object_property_set_bool(OBJECT(&mms->scc), true, "realized",
>                               &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(sccdev), 0, 0x4002f000);
> +    for (i = 0; i < 4; i++) {
> +        static const hwaddr i2cbase[] = {0x40022000, 0x40023000,
> +                                         0x40029000, 0x4002a000};
> +
> +        sysbus_create_simple("versatile_i2c", i2cbase[i], NULL);
> +    }

Is this device really the same as the I2C controller h/w
on the versatile board ?

thanks
-- PMM

Re: [PATCH 5/7] hw/arm/mps2: Add I2C busses on FPGA APB

[Philippe Mathieu-Daudé]

On 6/16/20 12:21 PM, Peter Maydell wrote:
> On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> There are 4 different I2C peripherals on the FPGA APB.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/mps2.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
>> index 4a49bfa9b9..6224d7a63c 100644
>> --- a/hw/arm/mps2.c
>> +++ b/hw/arm/mps2.c
>> @@ -321,6 +321,7 @@ static void mps2_common_init(MachineState *machine)
>>          create_unimplemented_device("cmsdk-ahb-gpio", gpiobase[i], 0x1000);
>>      }
>>
>> +    /* FPGA APB */
>>      sysbus_init_child_obj(OBJECT(mms), "scc", &mms->scc,
>>                            sizeof(mms->scc), TYPE_MPS2_SCC);
>>      sccdev = DEVICE(&mms->scc);
>> @@ -330,6 +331,12 @@ static void mps2_common_init(MachineState *machine)
>>      object_property_set_bool(OBJECT(&mms->scc), true, "realized",
>>                               &error_fatal);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(sccdev), 0, 0x4002f000);
>> +    for (i = 0; i < 4; i++) {
>> +        static const hwaddr i2cbase[] = {0x40022000, 0x40023000,
>> +                                         0x40029000, 0x4002a000};
>> +
>> +        sysbus_create_simple("versatile_i2c", i2cbase[i], NULL);
>> +    }
> 
> Is this device really the same as the I2C controller h/w
> on the versatile board ?

It seems the opposite way around, the I2C controller modeled as
'versatile_i2c' is the real 'ARM SBCon two-wire serial bus interface'
bitbanging I2C. IIUC the name 'ARM SBCon two-wire serial bus interface'
became official /after/ the versatile was released, so 'versatile-i2c'
is the popular name.

Yes, the MPS2(+) implement the ARM SBCon two-wire serial bus interface.

I'll see to add a TYPE_SBCON_I2C alias, it might be easier.

> 
> thanks
> -- PMM
> 

[PATCH 6/7] hw/arm/mps2: Map the FPGA I/O block

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 6224d7a63c..345ba551ab 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -38,6 +38,7 @@
 #include "hw/timer/cmsdk-apb-timer.h"
 #include "hw/timer/cmsdk-apb-dualtimer.h"
 #include "hw/misc/mps2-scc.h"
+#include "hw/misc/mps2-fpgaio.h"
 #include "hw/net/lan9118.h"
 #include "net/net.h"
 
@@ -66,6 +67,7 @@ typedef struct {
     MemoryRegion blockram_m3;
     MemoryRegion sram;
     MPS2SCC scc;
+    MPS2FPGAIO fpgaio;
     CMSDKAPBDualTimer dualtimer;
 } MPS2MachineState;
 
@@ -337,6 +339,11 @@ static void mps2_common_init(MachineState *machine)
 
         sysbus_create_simple("versatile_i2c", i2cbase[i], NULL);
     }
+    sysbus_init_child_obj(OBJECT(mms), "fpgaio", &mms->fpgaio,
+                          sizeof(mms->fpgaio), TYPE_MPS2_FPGAIO);
+    object_property_set_bool(OBJECT(&mms->fpgaio), true, "realized",
+                             &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&mms->fpgaio), 0, 0x40028000);
 
     /* In hardware this is a LAN9220; the LAN9118 is software compatible
      * except that it doesn't support the checksum-offload feature.
-- 
2.21.3

Re: [PATCH 6/7] hw/arm/mps2: Map the FPGA I/O block

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

> @@ -337,6 +339,11 @@ static void mps2_common_init(MachineState *machine)
>
>          sysbus_create_simple("versatile_i2c", i2cbase[i], NULL);
>      }
> +    sysbus_init_child_obj(OBJECT(mms), "fpgaio", &mms->fpgaio,
> +                          sizeof(mms->fpgaio), TYPE_MPS2_FPGAIO);
> +    object_property_set_bool(OBJECT(&mms->fpgaio), true, "realized",
> +                             &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&mms->fpgaio), 0, 0x40028000);

AN385 TRM isn't entirely clear but I suspect that you need to set
the FPGAIO's prescale-clk property because the default of 20MHz
isn't what the AN385 runs at. The FPGAIO model's default is written
to match the AN505, which is 20MHz, but AN385 and AN511 are both
25MHz:
https://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

thanks
-- PMM

Re: [PATCH 6/7] hw/arm/mps2: Map the FPGA I/O block

[Philippe Mathieu-Daudé]

On 6/16/20 12:26 PM, Peter Maydell wrote:
> On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
>> @@ -337,6 +339,11 @@ static void mps2_common_init(MachineState *machine)
>>
>>          sysbus_create_simple("versatile_i2c", i2cbase[i], NULL);
>>      }
>> +    sysbus_init_child_obj(OBJECT(mms), "fpgaio", &mms->fpgaio,
>> +                          sizeof(mms->fpgaio), TYPE_MPS2_FPGAIO);
>> +    object_property_set_bool(OBJECT(&mms->fpgaio), true, "realized",
>> +                             &error_fatal);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&mms->fpgaio), 0, 0x40028000);
> 
> AN385 TRM isn't entirely clear but I suspect that you need to set
> the FPGAIO's prescale-clk property because the default of 20MHz
> isn't what the AN385 runs at. The FPGAIO model's default is written
> to match the AN505, which is 20MHz, but AN385 and AN511 are both
> 25MHz:
> https://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

Ah I only checked for the I/O register. I'll check the clocks.

> 
> thanks
> -- PMM
> 

[PATCH 7/7] hw/misc/mps2-fpgaio: Implement push-buttons

[Philippe Mathieu-Daudé]

The FPGA system control block has 2 push-buttons labelled PB0/PB1.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/mps2-fpgaio.h |  1 +
 hw/misc/mps2-fpgaio.c         | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h
index 69e265cd4b..521900ae81 100644
--- a/include/hw/misc/mps2-fpgaio.h
+++ b/include/hw/misc/mps2-fpgaio.h
@@ -34,6 +34,7 @@ typedef struct {
     MemoryRegion iomem;
 
     uint32_t led0;
+    uint32_t button;
     uint32_t prescale;
     uint32_t misc;
 
diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c
index 2f3fbeef34..655e7a14fc 100644
--- a/hw/misc/mps2-fpgaio.c
+++ b/hw/misc/mps2-fpgaio.c
@@ -18,6 +18,7 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.h"
 #include "qapi/error.h"
 #include "trace.h"
 #include "hw/sysbus.h"
@@ -36,6 +37,13 @@ REG32(PRESCALE, 0x1c)
 REG32(PSCNTR, 0x20)
 REG32(MISC, 0x4c)
 
+static void mps2_fpgaio_push_button(void *opaque, int irq, int level)
+{
+    MPS2FPGAIO *s = MPS2_FPGAIO(opaque);
+
+    s->button = deposit32(s->button, irq, 1, level);
+}
+
 static uint32_t counter_from_tickoff(int64_t now, int64_t tick_offset, int frq)
 {
     return muldiv64(now - tick_offset, frq, NANOSECONDS_PER_SECOND);
@@ -131,7 +139,7 @@ static uint64_t mps2_fpgaio_read(void *opaque, hwaddr offset, unsigned size)
         /* User-pressable board buttons. We don't model that, so just return
          * zeroes.
          */
-        r = 0;
+        r = s->button;
         break;
     case A_PRESCALE:
         r = s->prescale;
@@ -232,6 +240,7 @@ static void mps2_fpgaio_reset(DeviceState *dev)
 
     trace_mps2_fpgaio_reset();
     s->led0 = 0;
+    s->button = 0;
     s->prescale = 0;
     s->misc = 0;
     s->clk1hz_tick_offset = tickoff_from_counter(now, 0, 1);
@@ -249,6 +258,8 @@ static void mps2_fpgaio_init(Object *obj)
     memory_region_init_io(&s->iomem, obj, &mps2_fpgaio_ops, s,
                           "mps2-fpgaio", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
+
+    qdev_init_gpio_in_named(DEVICE(s), mps2_fpgaio_push_button, "PB", 2);
 }
 
 static bool mps2_fpgaio_counters_needed(void *opaque)
-- 
2.21.3

Re: [PATCH 7/7] hw/misc/mps2-fpgaio: Implement push-buttons

[Peter Maydell]

On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The FPGA system control block has 2 push-buttons labelled PB0/PB1.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

> @@ -249,6 +258,8 @@ static void mps2_fpgaio_init(Object *obj)
>      memory_region_init_io(&s->iomem, obj, &mps2_fpgaio_ops, s,
>                            "mps2-fpgaio", 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
> +
> +    qdev_init_gpio_in_named(DEVICE(s), mps2_fpgaio_push_button, "PB", 2);
>  }

This change seems kind of pointless unless these GPIO lines are
actually wired up to something.

thanks
-- PMM

Re: [PATCH 7/7] hw/misc/mps2-fpgaio: Implement push-buttons

[Philippe Mathieu-Daudé]

On 6/16/20 12:27 PM, Peter Maydell wrote:
> On Tue, 16 Jun 2020 at 07:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The FPGA system control block has 2 push-buttons labelled PB0/PB1.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
>> @@ -249,6 +258,8 @@ static void mps2_fpgaio_init(Object *obj)
>>      memory_region_init_io(&s->iomem, obj, &mps2_fpgaio_ops, s,
>>                            "mps2-fpgaio", 0x1000);
>>      sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    qdev_init_gpio_in_named(DEVICE(s), mps2_fpgaio_push_button, "PB", 2);
>>  }
> 
> This change seems kind of pointless unless these GPIO lines are
> actually wired up to something.

Yes, I should have kept it out of this series, or documented
better the goal in the cover.

I'm setting the roots to motivate a team of developers to
work on a visualization of the MPS2 board. The push-button is
supported by Zephyr, so the the idea is the visualizer generates
QMP GPIO event to be processed such in pca9552_set_led(), and
interact with the guest firmware.

> 
> thanks
> -- PMM
> 

Re: [PATCH 7/7] hw/misc/mps2-fpgaio: Implement push-buttons

[Peter Maydell]

On Tue, 16 Jun 2020 at 11:40, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 6/16/20 12:27 PM, Peter Maydell wrote:
> > This change seems kind of pointless unless these GPIO lines are
> > actually wired up to something.
>
> Yes, I should have kept it out of this series, or documented
> better the goal in the cover.
>
> I'm setting the roots to motivate a team of developers to
> work on a visualization of the MPS2 board. The push-button is
> supported by Zephyr, so the the idea is the visualizer generates
> QMP GPIO event to be processed such in pca9552_set_led(), and
> interact with the guest firmware.

I think that having a framework so we can better model this kind
of push button / LED / similar thing is definitely good. I just
think we need to review it at the framework level first -- it
might turn out that actually the right way to wire up the push
button to the UI framework isn't with a GPIO wire at all.
Similarly with the other patchset that sends QMP events for
LEDs -- that also seems like it's half of a design and a bit
awkward to review without the context for what it connects to.

thanks
-- PMM

Re: [PATCH 7/7] hw/misc/mps2-fpgaio: Implement push-buttons

[Philippe Mathieu-Daudé]

On 6/16/20 2:29 PM, Peter Maydell wrote:
> On Tue, 16 Jun 2020 at 11:40, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 6/16/20 12:27 PM, Peter Maydell wrote:
>>> This change seems kind of pointless unless these GPIO lines are
>>> actually wired up to something.
>>
>> Yes, I should have kept it out of this series, or documented
>> better the goal in the cover.
>>
>> I'm setting the roots to motivate a team of developers to
>> work on a visualization of the MPS2 board. The push-button is
>> supported by Zephyr, so the the idea is the visualizer generates
>> QMP GPIO event to be processed such in pca9552_set_led(), and
>> interact with the guest firmware.
> 
> I think that having a framework so we can better model this kind
> of push button / LED / similar thing is definitely good. I just
> think we need to review it at the framework level first -- it
> might turn out that actually the right way to wire up the push
> button to the UI framework isn't with a GPIO wire at all.
> Similarly with the other patchset that sends QMP events for
> LEDs -- that also seems like it's half of a design and a bit
> awkward to review without the context for what it connects to.

On my side feedback are helpful, but I understand.
I'll see if there are still any motivated soul left,
else wait for next GSoC.

> 
> thanks
> -- PMM
>