| Message ID | 20200616063157.16389-8-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | mps2: Add few more peripherals | expand |
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 >
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)
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(-)