| Message ID | 1357911723-12688-1-git-send-email-wolfgang.mauerer@siemens.com |
|---|---|
| State | New |
| Headers | show |
Hello, Am 11.01.2013 14:42, schrieb Wolfgang Mauerer: > The configuration register for the onboard LEDs is > emulated, but the state is not exported, which makes > the feature not particularly useful. Create a character > device to make status changes accessible to the host. > > For example, use the command line argument > > -chardev socket,id=leds,host=localhost,port=12345,server,nowait > > to observe status changes via a socket. > > Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> > --- > hw/arm_sysctl.c | 24 ++++++++++++++++++++++++ > 1 files changed, 24 insertions(+), 0 deletions(-) > > diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c > index b733617..7cb8559 100644 > --- a/hw/arm_sysctl.c > +++ b/hw/arm_sysctl.c > @@ -19,6 +19,7 @@ typedef struct { > SysBusDevice busdev; > MemoryRegion iomem; > qemu_irq pl110_mux_ctrl; > + CharDriverState *display; > > uint32_t sys_id; > uint32_t leds; > @@ -92,6 +93,24 @@ static void arm_sysctl_reset(DeviceState *d) > } > } > > +static void notify_led_change(CharDriverState *chr, uint32_t old, uint32_t new) > +{ > + uint32_t changed; > + unsigned int i; > + > + if (chr == NULL) { > + return; > + } > + > + changed = old ^ new; > + for (i = 0; i < 8; i++) { > + if (changed & (1 << i)) { > + qemu_chr_fe_printf(chr, "%u:%s\r\n", i, > + new & (1 << i) ? "on" : "off"); Instead of inventing a custom text-based protocol, would exposing a bool QOM property per LED be an option? What exactly are you trying to do with the state? > + } > + } > +} > + > static uint64_t arm_sysctl_read(void *opaque, hwaddr offset, > unsigned size) > { > @@ -198,6 +217,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset, > > switch (offset) { > case 0x08: /* LED */ > + notify_led_change(s->display, s->leds, (uint32_t)val); > s->leds = val; > case 0x0c: /* OSC0 */ > case 0x10: /* OSC1 */ > @@ -386,6 +406,10 @@ static int arm_sysctl_init(SysBusDevice *dev) > { > arm_sysctl_state *s = FROM_SYSBUS(arm_sysctl_state, dev); > > + s->display = qemu_chr_new("leds0", "chardev:leds", NULL); > + if (s->display) { Can this ever be NULL? > + qemu_chr_fe_printf(s->display, "8 LEDs available\r\n"); > + } > memory_region_init_io(&s->iomem, &arm_sysctl_ops, s, "arm-sysctl", 0x1000); > sysbus_init_mmio(dev, &s->iomem); > qdev_init_gpio_in(&s->busdev.qdev, arm_sysctl_gpio_set, 2); Regards, Andreas
On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote: > The configuration register for the onboard LEDs is > emulated, but the state is not exported, which makes > the feature not particularly useful. Create a character > device to make status changes accessible to the host. > > For example, use the command line argument > > -chardev socket,id=leds,host=localhost,port=12345,server,nowait > > to observe status changes via a socket. This isn't the only board we emulate which has LEDs. I'd rather see this problem tackled with a general plan for "this is how we handle LEDs in QEMU boards" rather than a versatile board specific patch. thanks -- PMM
Am 11.01.2013 16:00, schrieb Peter Maydell: > On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote: >> The configuration register for the onboard LEDs is >> emulated, but the state is not exported, which makes >> the feature not particularly useful. Create a character >> device to make status changes accessible to the host. >> >> For example, use the command line argument >> >> -chardev socket,id=leds,host=localhost,port=12345,server,nowait >> >> to observe status changes via a socket. > > This isn't the only board we emulate which has LEDs. I'd > rather see this problem tackled with a general plan for "this > is how we handle LEDs in QEMU boards" rather than a > versatile board specific patch. Hm, mips_malta.c does use a CharDriverState... just a more "structured" one using ASCII art. Andreas
On 11/01/13 16:23, Andreas Färber wrote: > Am 11.01.2013 16:00, schrieb Peter Maydell: >> On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote: >>> The configuration register for the onboard LEDs is >>> emulated, but the state is not exported, which makes >>> the feature not particularly useful. Create a character >>> device to make status changes accessible to the host. >>> >>> For example, use the command line argument >>> >>> -chardev socket,id=leds,host=localhost,port=12345,server,nowait >>> >>> to observe status changes via a socket. >> >> This isn't the only board we emulate which has LEDs. I'd >> rather see this problem tackled with a general plan for "this >> is how we handle LEDs in QEMU boards" rather than a >> versatile board specific patch. > > Hm, mips_malta.c does use a CharDriverState... just a more "structured" > one using ASCII art. that's one of the reaons why I used a character device to export the status information. However, a simple custom protocol (or maybe the same information in JSON) seemed more appripriate since I assume that graphical frontends will eventually visualise the LED status. Having to parse some ASCII art output for this seems suboptimal. Exporting a binary QOM property (as you suggested in another email) might be an option, although I'm not really sure if LED status changes should be communicated this way -- to me, using a chardev feels nicer. Do the QMP maintainers have an opinion? Regards, Wolfgang
Hello, On 11/01/13 15:58, Andreas Färber wrote: >> @@ -386,6 +406,10 @@ static int arm_sysctl_init(SysBusDevice *dev) >> > { >> > arm_sysctl_state *s = FROM_SYSBUS(arm_sysctl_state, dev); >> > >> > + s->display = qemu_chr_new("leds0", "chardev:leds", NULL); >> > + if (s->display) { > Can this ever be NULL? if can be NULL if no leds chardev is available. Best, Wolfgang
On Fri, 11 Jan 2013 16:44:49 +0100 Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote: > On 11/01/13 16:23, Andreas Färber wrote: > > Am 11.01.2013 16:00, schrieb Peter Maydell: > >> On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote: > >>> The configuration register for the onboard LEDs is > >>> emulated, but the state is not exported, which makes > >>> the feature not particularly useful. Create a character > >>> device to make status changes accessible to the host. > >>> > >>> For example, use the command line argument > >>> > >>> -chardev socket,id=leds,host=localhost,port=12345,server,nowait > >>> > >>> to observe status changes via a socket. > >> > >> This isn't the only board we emulate which has LEDs. I'd > >> rather see this problem tackled with a general plan for "this > >> is how we handle LEDs in QEMU boards" rather than a > >> versatile board specific patch. > > > > Hm, mips_malta.c does use a CharDriverState... just a more "structured" > > one using ASCII art. > > that's one of the reaons why I used a character device to export the > status information. > > However, a simple custom protocol (or maybe the same information in > JSON) seemed more appripriate since I assume that graphical frontends > will eventually visualise the LED status. Having to parse some ASCII > art output for this seems suboptimal. > > Exporting a binary QOM property (as you suggested in another email) > might be an option, although I'm not really sure if LED status changes > should be communicated this way -- to me, using a chardev feels nicer. > Do the QMP maintainers have an opinion? I'm not completely sure I understand the problem here, but if this is some kind of status to be queried then exporting it through a QOM property seems the right thing to do. If you go for a custom protocol, then using QMP syntax might be a good idea, as you can use our infra and clients which already know QMP will also appreciate this (this is what qemu-ga did, btw).
diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c index b733617..7cb8559 100644 --- a/hw/arm_sysctl.c +++ b/hw/arm_sysctl.c @@ -19,6 +19,7 @@ typedef struct { SysBusDevice busdev; MemoryRegion iomem; qemu_irq pl110_mux_ctrl; + CharDriverState *display; uint32_t sys_id; uint32_t leds; @@ -92,6 +93,24 @@ static void arm_sysctl_reset(DeviceState *d) } } +static void notify_led_change(CharDriverState *chr, uint32_t old, uint32_t new) +{ + uint32_t changed; + unsigned int i; + + if (chr == NULL) { + return; + } + + changed = old ^ new; + for (i = 0; i < 8; i++) { + if (changed & (1 << i)) { + qemu_chr_fe_printf(chr, "%u:%s\r\n", i, + new & (1 << i) ? "on" : "off"); + } + } +} + static uint64_t arm_sysctl_read(void *opaque, hwaddr offset, unsigned size) { @@ -198,6 +217,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset, switch (offset) { case 0x08: /* LED */ + notify_led_change(s->display, s->leds, (uint32_t)val); s->leds = val; case 0x0c: /* OSC0 */ case 0x10: /* OSC1 */ @@ -386,6 +406,10 @@ static int arm_sysctl_init(SysBusDevice *dev) { arm_sysctl_state *s = FROM_SYSBUS(arm_sysctl_state, dev); + s->display = qemu_chr_new("leds0", "chardev:leds", NULL); + if (s->display) { + qemu_chr_fe_printf(s->display, "8 LEDs available\r\n"); + } memory_region_init_io(&s->iomem, &arm_sysctl_ops, s, "arm-sysctl", 0x1000); sysbus_init_mmio(dev, &s->iomem); qdev_init_gpio_in(&s->busdev.qdev, arm_sysctl_gpio_set, 2);
The configuration register for the onboard LEDs is emulated, but the state is not exported, which makes the feature not particularly useful. Create a character device to make status changes accessible to the host. For example, use the command line argument -chardev socket,id=leds,host=localhost,port=12345,server,nowait to observe status changes via a socket. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com> --- hw/arm_sysctl.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-)