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

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

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
> 

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

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
>