| Message ID | c101f29d71634c6c8d70a14d59664e75f11b7d3f.1328031687.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
On 01/31/2012 11:41 AM, Jan Kiszka wrote: > Convert the PC speaker device to a qdev ISA model. Move the public > interface to a dedicated header file at this chance. > > Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> Heh, I did this too more or less the same way. Some comments below: > --- > arch_init.c | 1 + > hw/i82378.c | 3 +- > hw/mips_jazz.c | 3 +- > hw/pc.c | 3 +- > hw/pc.h | 4 --- > hw/pcspk.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++--------- > hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++ > 7 files changed, 104 insertions(+), 17 deletions(-) > create mode 100644 hw/pcspk.h > > diff --git a/arch_init.c b/arch_init.c > index 2366511..a45485b 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -42,6 +42,7 @@ > #include "gdbstub.h" > #include "hw/smbios.h" > #include "exec-memory.h" > +#include "hw/pcspk.h" > > #ifdef TARGET_SPARC > int graphic_width = 1024; > diff --git a/hw/i82378.c b/hw/i82378.c > index 127cadf..79fa8b7 100644 > --- a/hw/i82378.c > +++ b/hw/i82378.c > @@ -20,6 +20,7 @@ > #include "pci.h" > #include "pc.h" > #include "i8254.h" > +#include "pcspk.h" > > //#define DEBUG_I82378 > > @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, I82378State *s) > pit = pit_init(isabus, 0x40, 0, NULL); > > /* speaker */ > - pcspk_init(pit); > + pcspk_init(isabus, pit); > > /* 2 82C37 (dma) */ > DMA_init(1,&s->out[1]); > diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c > index b61b218..65608dc 100644 > --- a/hw/mips_jazz.c > +++ b/hw/mips_jazz.c > @@ -37,6 +37,7 @@ > #include "loader.h" > #include "mc146818rtc.h" > #include "i8254.h" > +#include "pcspk.h" > #include "blockdev.h" > #include "sysbus.h" > #include "exec-memory.h" > @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion *address_space, > cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); > DMA_init(0, cpu_exit_irq); > pit = pit_init(isa_bus, 0x40, 0, NULL); > - pcspk_init(pit); > + pcspk_init(isa_bus, pit); > > /* ISA IO space at 0x90000000 */ > isa_mmio_init(0x90000000, 0x01000000); > diff --git a/hw/pc.c b/hw/pc.c > index 6fb1de7..f6a91a9 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -37,6 +37,7 @@ > #include "multiboot.h" > #include "mc146818rtc.h" > #include "i8254.h" > +#include "pcspk.h" > #include "msi.h" > #include "sysbus.h" > #include "sysemu.h" > @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > /* connect PIT to output control line of the HPET */ > qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0)); > } > - pcspk_init(pit); > + pcspk_init(isa_bus, pit); > > for(i = 0; i< MAX_SERIAL_PORTS; i++) { > if (serial_hds[i]) { > diff --git a/hw/pc.h b/hw/pc.h > index b08708d..1b47bbd 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr); > /* hpet.c */ > extern int no_hpet; > > -/* pcspk.c */ > -void pcspk_init(ISADevice *pit); > -int pcspk_audio_init(ISABus *bus); > - > /* piix_pci.c */ > struct PCII440FXState; > typedef struct PCII440FXState PCII440FXState; > diff --git a/hw/pcspk.c b/hw/pcspk.c > index 43df818..5bd9e32 100644 > --- a/hw/pcspk.c > +++ b/hw/pcspk.c > @@ -28,6 +28,7 @@ > #include "audio/audio.h" > #include "qemu-timer.h" > #include "i8254.h" > +#include "pcspk.h" > > #define PCSPK_BUF_LEN 1792 > #define PCSPK_SAMPLE_RATE 32000 > @@ -35,10 +36,13 @@ > #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / PCSPK_MAX_FREQ) > > typedef struct { > + ISADevice dev; > + MemoryRegion ioport; > + uint32_t iobase; > uint8_t sample_buf[PCSPK_BUF_LEN]; > QEMUSoundCard card; > SWVoiceOut *voice; > - ISADevice *pit; > + void *pit; > unsigned int pit_count; > unsigned int samples; > unsigned int play_pos; > @@ -47,7 +51,7 @@ typedef struct { > } PCSpkState; > > static const char *s_spk = "pcspk"; > -static PCSpkState pcspk_state; > +static PCSpkState *pcspk_state; > > static inline void generate_samples(PCSpkState *s) > { > @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free) > > int pcspk_audio_init(ISABus *bus) > { > - PCSpkState *s =&pcspk_state; > + PCSpkState *s = pcspk_state; > struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0}; This can be a follow up, but what this is really doing is enabling audio for this device. ISABus is unused as a parameter. I think it would be better to make this a qdev bool property (audio_enabled) and then modify the code that calls this function to either set the property as a global, or use the QOM path to specifically set it on this device. Either way, I think setting an audio_enabled flag via qdev makes a whole lot more sense conceptionally than using the -soundhw option. > > AUD_register_card(s_spk,&s->card); > @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus) > return 0; > } > > -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) > +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr, > + unsigned size) > { > PCSpkState *s = opaque; > int out; > @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) > return pit_get_gate(s->pit, 2) | (s->data_on<< 1) | s->dummy_refresh_clock | out; > } > > -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val) > +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, uint64_t val, > + unsigned size) > { > PCSpkState *s = opaque; > const int gate = val& 1; > @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val) > } > } > > -void pcspk_init(ISADevice *pit) > +static const MemoryRegionOps pcspk_io_ops = { > + .read = pcspk_io_read, > + .write = pcspk_io_write, > + .impl = { > + .min_access_size = 1, > + .max_access_size = 1, > + }, > +}; > + > +static int pcspk_initfn(ISADevice *dev) > +{ > + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev); > + > + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1); > + isa_register_ioport(NULL,&s->ioport, s->iobase); Should pass dev as the first argument to isa_register_ioport. Otherwise the resource won't be cleaned up during destruction. > + > + pcspk_state = s; > + > + return 0; > +} > + > +static void pcspk_class_initfn(ObjectClass *klass, void *data) > { > - PCSpkState *s =&pcspk_state; > + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass); > + ic->init = pcspk_initfn; > +} > > - s->pit = pit; > - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s); > - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s); > +static DeviceInfo pcspk_info = { > + .name = "isa-pcspk", > + .size = sizeof(PCSpkState), > + .no_user = 1, > + .props = (Property[]) { > + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), > + DEFINE_PROP_PTR("pit", PCSpkState, pit), Please don't introduce a pointer property here. They cannot be used in a meaningful way in qdev. Why not register a link<TYPE_PIT> in instance_init? Regards, Anthony Liguori > + DEFINE_PROP_END_OF_LIST(), > + }, > + .class_init = pcspk_class_initfn, > +}; > + > +static void pcspk_register(void) > +{ > + isa_qdev_register(&pcspk_info); > } > +device_init(pcspk_register) > diff --git a/hw/pcspk.h b/hw/pcspk.h > new file mode 100644 > index 0000000..7f42bac > --- /dev/null > +++ b/hw/pcspk.h > @@ -0,0 +1,45 @@ > +/* > + * QEMU PC speaker emulation > + * > + * Copyright (c) 2006 Joachim Henke > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#ifndef HW_PCSPK_H > +#define HW_PCSPK_H > + > +#include "hw.h" > +#include "isa.h" > + > +static inline ISADevice *pcspk_init(ISABus *bus, ISADevice *pit) > +{ > + ISADevice *dev; > + > + dev = isa_create(bus, "isa-pcspk"); > + qdev_prop_set_uint32(&dev->qdev, "iobase", 0x61); > + qdev_prop_set_ptr(&dev->qdev, "pit", pit); > + qdev_init_nofail(&dev->qdev); > + > + return dev; > +} > + > +int pcspk_audio_init(ISABus *bus); > + > +#endif /* !HW_PCSPK_H */
On 2012-01-31 21:49, Anthony Liguori wrote: > On 01/31/2012 11:41 AM, Jan Kiszka wrote: >> Convert the PC speaker device to a qdev ISA model. Move the public >> interface to a dedicated header file at this chance. >> >> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> > > Heh, I did this too more or less the same way. Some comments below: > >> --- >> arch_init.c | 1 + >> hw/i82378.c | 3 +- >> hw/mips_jazz.c | 3 +- >> hw/pc.c | 3 +- >> hw/pc.h | 4 --- >> hw/pcspk.c | 62 >> ++++++++++++++++++++++++++++++++++++++++++++++--------- >> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++ >> 7 files changed, 104 insertions(+), 17 deletions(-) >> create mode 100644 hw/pcspk.h >> >> diff --git a/arch_init.c b/arch_init.c >> index 2366511..a45485b 100644 >> --- a/arch_init.c >> +++ b/arch_init.c >> @@ -42,6 +42,7 @@ >> #include "gdbstub.h" >> #include "hw/smbios.h" >> #include "exec-memory.h" >> +#include "hw/pcspk.h" >> >> #ifdef TARGET_SPARC >> int graphic_width = 1024; >> diff --git a/hw/i82378.c b/hw/i82378.c >> index 127cadf..79fa8b7 100644 >> --- a/hw/i82378.c >> +++ b/hw/i82378.c >> @@ -20,6 +20,7 @@ >> #include "pci.h" >> #include "pc.h" >> #include "i8254.h" >> +#include "pcspk.h" >> >> //#define DEBUG_I82378 >> >> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, >> I82378State *s) >> pit = pit_init(isabus, 0x40, 0, NULL); >> >> /* speaker */ >> - pcspk_init(pit); >> + pcspk_init(isabus, pit); >> >> /* 2 82C37 (dma) */ >> DMA_init(1,&s->out[1]); >> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c >> index b61b218..65608dc 100644 >> --- a/hw/mips_jazz.c >> +++ b/hw/mips_jazz.c >> @@ -37,6 +37,7 @@ >> #include "loader.h" >> #include "mc146818rtc.h" >> #include "i8254.h" >> +#include "pcspk.h" >> #include "blockdev.h" >> #include "sysbus.h" >> #include "exec-memory.h" >> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion >> *address_space, >> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); >> DMA_init(0, cpu_exit_irq); >> pit = pit_init(isa_bus, 0x40, 0, NULL); >> - pcspk_init(pit); >> + pcspk_init(isa_bus, pit); >> >> /* ISA IO space at 0x90000000 */ >> isa_mmio_init(0x90000000, 0x01000000); >> diff --git a/hw/pc.c b/hw/pc.c >> index 6fb1de7..f6a91a9 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -37,6 +37,7 @@ >> #include "multiboot.h" >> #include "mc146818rtc.h" >> #include "i8254.h" >> +#include "pcspk.h" >> #include "msi.h" >> #include "sysbus.h" >> #include "sysemu.h" >> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, >> qemu_irq *gsi, >> /* connect PIT to output control line of the HPET */ >> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, >> 0)); >> } >> - pcspk_init(pit); >> + pcspk_init(isa_bus, pit); >> >> for(i = 0; i< MAX_SERIAL_PORTS; i++) { >> if (serial_hds[i]) { >> diff --git a/hw/pc.h b/hw/pc.h >> index b08708d..1b47bbd 100644 >> --- a/hw/pc.h >> +++ b/hw/pc.h >> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice >> *dev, uint8_t addr); >> /* hpet.c */ >> extern int no_hpet; >> >> -/* pcspk.c */ >> -void pcspk_init(ISADevice *pit); >> -int pcspk_audio_init(ISABus *bus); >> - >> /* piix_pci.c */ >> struct PCII440FXState; >> typedef struct PCII440FXState PCII440FXState; >> diff --git a/hw/pcspk.c b/hw/pcspk.c >> index 43df818..5bd9e32 100644 >> --- a/hw/pcspk.c >> +++ b/hw/pcspk.c >> @@ -28,6 +28,7 @@ >> #include "audio/audio.h" >> #include "qemu-timer.h" >> #include "i8254.h" >> +#include "pcspk.h" >> >> #define PCSPK_BUF_LEN 1792 >> #define PCSPK_SAMPLE_RATE 32000 >> @@ -35,10 +36,13 @@ >> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / >> PCSPK_MAX_FREQ) >> >> typedef struct { >> + ISADevice dev; >> + MemoryRegion ioport; >> + uint32_t iobase; >> uint8_t sample_buf[PCSPK_BUF_LEN]; >> QEMUSoundCard card; >> SWVoiceOut *voice; >> - ISADevice *pit; >> + void *pit; >> unsigned int pit_count; >> unsigned int samples; >> unsigned int play_pos; >> @@ -47,7 +51,7 @@ typedef struct { >> } PCSpkState; >> >> static const char *s_spk = "pcspk"; >> -static PCSpkState pcspk_state; >> +static PCSpkState *pcspk_state; >> >> static inline void generate_samples(PCSpkState *s) >> { >> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free) >> >> int pcspk_audio_init(ISABus *bus) >> { >> - PCSpkState *s =&pcspk_state; >> + PCSpkState *s = pcspk_state; >> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0}; > > > This can be a follow up, but what this is really doing is enabling audio > for this device. ISABus is unused as a parameter. Yes, but this is a callback for the audio subsystem, look at arch_init.c. Nothing we can do here, only if we change that callback prototype. > > I think it would be better to make this a qdev bool property > (audio_enabled) and then modify the code that calls this function to > either set the property as a global, or use the QOM path to specifically > set it on this device. > > Either way, I think setting an audio_enabled flag via qdev makes a whole > lot more sense conceptionally than using the -soundhw option. Yep, there is room for improvements. The audio system is just another backend, like a chardev or netdev. It should once we handled like this I guess. > >> >> AUD_register_card(s_spk,&s->card); >> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus) >> return 0; >> } >> >> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) >> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr, >> + unsigned size) >> { >> PCSpkState *s = opaque; >> int out; >> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, >> uint32_t addr) >> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) | >> s->dummy_refresh_clock | out; >> } >> >> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t >> val) >> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, >> uint64_t val, >> + unsigned size) >> { >> PCSpkState *s = opaque; >> const int gate = val& 1; >> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, >> uint32_t addr, uint32_t val) >> } >> } >> >> -void pcspk_init(ISADevice *pit) >> +static const MemoryRegionOps pcspk_io_ops = { >> + .read = pcspk_io_read, >> + .write = pcspk_io_write, >> + .impl = { >> + .min_access_size = 1, >> + .max_access_size = 1, >> + }, >> +}; >> + >> +static int pcspk_initfn(ISADevice *dev) >> +{ >> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev); >> + >> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1); >> + isa_register_ioport(NULL,&s->ioport, s->iobase); > > Should pass dev as the first argument to isa_register_ioport. Otherwise > the resource won't be cleaned up during destruction. Oops, will fix. > >> + >> + pcspk_state = s; >> + >> + return 0; >> +} >> + >> +static void pcspk_class_initfn(ObjectClass *klass, void *data) >> { >> - PCSpkState *s =&pcspk_state; >> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass); >> + ic->init = pcspk_initfn; >> +} >> >> - s->pit = pit; >> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s); >> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s); >> +static DeviceInfo pcspk_info = { >> + .name = "isa-pcspk", >> + .size = sizeof(PCSpkState), >> + .no_user = 1, >> + .props = (Property[]) { >> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), >> + DEFINE_PROP_PTR("pit", PCSpkState, pit), > > Please don't introduce a pointer property here. They cannot be used in > a meaningful way in qdev. Why not register a link<TYPE_PIT> in > instance_init? Once it's properly usable, I will do so. So far I see now in-tree - ideally type-checking - replacement for qdev_prop_set_ptr. Jan
On 01/31/2012 04:00 PM, Jan Kiszka wrote: > On 2012-01-31 21:49, Anthony Liguori wrote: >> On 01/31/2012 11:41 AM, Jan Kiszka wrote: >>> Convert the PC speaker device to a qdev ISA model. Move the public >>> interface to a dedicated header file at this chance. >>> >>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> >> >> Heh, I did this too more or less the same way. Some comments below: >> >>> --- >>> arch_init.c | 1 + >>> hw/i82378.c | 3 +- >>> hw/mips_jazz.c | 3 +- >>> hw/pc.c | 3 +- >>> hw/pc.h | 4 --- >>> hw/pcspk.c | 62 >>> ++++++++++++++++++++++++++++++++++++++++++++++--------- >>> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++ >>> 7 files changed, 104 insertions(+), 17 deletions(-) >>> create mode 100644 hw/pcspk.h >>> >>> diff --git a/arch_init.c b/arch_init.c >>> index 2366511..a45485b 100644 >>> --- a/arch_init.c >>> +++ b/arch_init.c >>> @@ -42,6 +42,7 @@ >>> #include "gdbstub.h" >>> #include "hw/smbios.h" >>> #include "exec-memory.h" >>> +#include "hw/pcspk.h" >>> >>> #ifdef TARGET_SPARC >>> int graphic_width = 1024; >>> diff --git a/hw/i82378.c b/hw/i82378.c >>> index 127cadf..79fa8b7 100644 >>> --- a/hw/i82378.c >>> +++ b/hw/i82378.c >>> @@ -20,6 +20,7 @@ >>> #include "pci.h" >>> #include "pc.h" >>> #include "i8254.h" >>> +#include "pcspk.h" >>> >>> //#define DEBUG_I82378 >>> >>> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, >>> I82378State *s) >>> pit = pit_init(isabus, 0x40, 0, NULL); >>> >>> /* speaker */ >>> - pcspk_init(pit); >>> + pcspk_init(isabus, pit); >>> >>> /* 2 82C37 (dma) */ >>> DMA_init(1,&s->out[1]); >>> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c >>> index b61b218..65608dc 100644 >>> --- a/hw/mips_jazz.c >>> +++ b/hw/mips_jazz.c >>> @@ -37,6 +37,7 @@ >>> #include "loader.h" >>> #include "mc146818rtc.h" >>> #include "i8254.h" >>> +#include "pcspk.h" >>> #include "blockdev.h" >>> #include "sysbus.h" >>> #include "exec-memory.h" >>> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion >>> *address_space, >>> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); >>> DMA_init(0, cpu_exit_irq); >>> pit = pit_init(isa_bus, 0x40, 0, NULL); >>> - pcspk_init(pit); >>> + pcspk_init(isa_bus, pit); >>> >>> /* ISA IO space at 0x90000000 */ >>> isa_mmio_init(0x90000000, 0x01000000); >>> diff --git a/hw/pc.c b/hw/pc.c >>> index 6fb1de7..f6a91a9 100644 >>> --- a/hw/pc.c >>> +++ b/hw/pc.c >>> @@ -37,6 +37,7 @@ >>> #include "multiboot.h" >>> #include "mc146818rtc.h" >>> #include "i8254.h" >>> +#include "pcspk.h" >>> #include "msi.h" >>> #include "sysbus.h" >>> #include "sysemu.h" >>> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, >>> qemu_irq *gsi, >>> /* connect PIT to output control line of the HPET */ >>> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, >>> 0)); >>> } >>> - pcspk_init(pit); >>> + pcspk_init(isa_bus, pit); >>> >>> for(i = 0; i< MAX_SERIAL_PORTS; i++) { >>> if (serial_hds[i]) { >>> diff --git a/hw/pc.h b/hw/pc.h >>> index b08708d..1b47bbd 100644 >>> --- a/hw/pc.h >>> +++ b/hw/pc.h >>> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice >>> *dev, uint8_t addr); >>> /* hpet.c */ >>> extern int no_hpet; >>> >>> -/* pcspk.c */ >>> -void pcspk_init(ISADevice *pit); >>> -int pcspk_audio_init(ISABus *bus); >>> - >>> /* piix_pci.c */ >>> struct PCII440FXState; >>> typedef struct PCII440FXState PCII440FXState; >>> diff --git a/hw/pcspk.c b/hw/pcspk.c >>> index 43df818..5bd9e32 100644 >>> --- a/hw/pcspk.c >>> +++ b/hw/pcspk.c >>> @@ -28,6 +28,7 @@ >>> #include "audio/audio.h" >>> #include "qemu-timer.h" >>> #include "i8254.h" >>> +#include "pcspk.h" >>> >>> #define PCSPK_BUF_LEN 1792 >>> #define PCSPK_SAMPLE_RATE 32000 >>> @@ -35,10 +36,13 @@ >>> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / >>> PCSPK_MAX_FREQ) >>> >>> typedef struct { >>> + ISADevice dev; >>> + MemoryRegion ioport; >>> + uint32_t iobase; >>> uint8_t sample_buf[PCSPK_BUF_LEN]; >>> QEMUSoundCard card; >>> SWVoiceOut *voice; >>> - ISADevice *pit; >>> + void *pit; >>> unsigned int pit_count; >>> unsigned int samples; >>> unsigned int play_pos; >>> @@ -47,7 +51,7 @@ typedef struct { >>> } PCSpkState; >>> >>> static const char *s_spk = "pcspk"; >>> -static PCSpkState pcspk_state; >>> +static PCSpkState *pcspk_state; >>> >>> static inline void generate_samples(PCSpkState *s) >>> { >>> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free) >>> >>> int pcspk_audio_init(ISABus *bus) >>> { >>> - PCSpkState *s =&pcspk_state; >>> + PCSpkState *s = pcspk_state; >>> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0}; >> >> >> This can be a follow up, but what this is really doing is enabling audio >> for this device. ISABus is unused as a parameter. > > Yes, but this is a callback for the audio subsystem, look at > arch_init.c. Nothing we can do here, only if we change that callback > prototype. > >> >> I think it would be better to make this a qdev bool property >> (audio_enabled) and then modify the code that calls this function to >> either set the property as a global, or use the QOM path to specifically >> set it on this device. >> >> Either way, I think setting an audio_enabled flag via qdev makes a whole >> lot more sense conceptionally than using the -soundhw option. > > Yep, there is room for improvements. The audio system is just another > backend, like a chardev or netdev. It should once we handled like this I > guess. > >> >>> >>> AUD_register_card(s_spk,&s->card); >>> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus) >>> return 0; >>> } >>> >>> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) >>> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr, >>> + unsigned size) >>> { >>> PCSpkState *s = opaque; >>> int out; >>> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, >>> uint32_t addr) >>> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) | >>> s->dummy_refresh_clock | out; >>> } >>> >>> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t >>> val) >>> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, >>> uint64_t val, >>> + unsigned size) >>> { >>> PCSpkState *s = opaque; >>> const int gate = val& 1; >>> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, >>> uint32_t addr, uint32_t val) >>> } >>> } >>> >>> -void pcspk_init(ISADevice *pit) >>> +static const MemoryRegionOps pcspk_io_ops = { >>> + .read = pcspk_io_read, >>> + .write = pcspk_io_write, >>> + .impl = { >>> + .min_access_size = 1, >>> + .max_access_size = 1, >>> + }, >>> +}; >>> + >>> +static int pcspk_initfn(ISADevice *dev) >>> +{ >>> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev); >>> + >>> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1); >>> + isa_register_ioport(NULL,&s->ioport, s->iobase); >> >> Should pass dev as the first argument to isa_register_ioport. Otherwise >> the resource won't be cleaned up during destruction. > > Oops, will fix. > >> >>> + >>> + pcspk_state = s; >>> + >>> + return 0; >>> +} >>> + >>> +static void pcspk_class_initfn(ObjectClass *klass, void *data) >>> { >>> - PCSpkState *s =&pcspk_state; >>> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass); >>> + ic->init = pcspk_initfn; >>> +} >>> >>> - s->pit = pit; >>> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s); >>> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s); >>> +static DeviceInfo pcspk_info = { >>> + .name = "isa-pcspk", >>> + .size = sizeof(PCSpkState), >>> + .no_user = 1, >>> + .props = (Property[]) { >>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), >>> + DEFINE_PROP_PTR("pit", PCSpkState, pit), >> >> Please don't introduce a pointer property here. They cannot be used in >> a meaningful way in qdev. Why not register a link<TYPE_PIT> in >> instance_init? > > Once it's properly usable, I will do so. So far I see now in-tree - > ideally type-checking - replacement for qdev_prop_set_ptr. Why is what's in the tree not usable? Just don't do pcspk_init as a static inline (which is not that nice to do anyway) and you don't need to worry about the availability of an accessor. And BTW, there is strict type checking now, which makes it already an improvement over property pointers. Regards, Anthony Liguori > > Jan
On 2012-01-31 23:40, Anthony Liguori wrote: > On 01/31/2012 04:00 PM, Jan Kiszka wrote: >> On 2012-01-31 21:49, Anthony Liguori wrote: >>> On 01/31/2012 11:41 AM, Jan Kiszka wrote: >>>> Convert the PC speaker device to a qdev ISA model. Move the public >>>> interface to a dedicated header file at this chance. >>>> >>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> >>> >>> Heh, I did this too more or less the same way. Some comments below: >>> >>>> --- >>>> arch_init.c | 1 + >>>> hw/i82378.c | 3 +- >>>> hw/mips_jazz.c | 3 +- >>>> hw/pc.c | 3 +- >>>> hw/pc.h | 4 --- >>>> hw/pcspk.c | 62 >>>> ++++++++++++++++++++++++++++++++++++++++++++++--------- >>>> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++ >>>> 7 files changed, 104 insertions(+), 17 deletions(-) >>>> create mode 100644 hw/pcspk.h >>>> >>>> diff --git a/arch_init.c b/arch_init.c >>>> index 2366511..a45485b 100644 >>>> --- a/arch_init.c >>>> +++ b/arch_init.c >>>> @@ -42,6 +42,7 @@ >>>> #include "gdbstub.h" >>>> #include "hw/smbios.h" >>>> #include "exec-memory.h" >>>> +#include "hw/pcspk.h" >>>> >>>> #ifdef TARGET_SPARC >>>> int graphic_width = 1024; >>>> diff --git a/hw/i82378.c b/hw/i82378.c >>>> index 127cadf..79fa8b7 100644 >>>> --- a/hw/i82378.c >>>> +++ b/hw/i82378.c >>>> @@ -20,6 +20,7 @@ >>>> #include "pci.h" >>>> #include "pc.h" >>>> #include "i8254.h" >>>> +#include "pcspk.h" >>>> >>>> //#define DEBUG_I82378 >>>> >>>> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, >>>> I82378State *s) >>>> pit = pit_init(isabus, 0x40, 0, NULL); >>>> >>>> /* speaker */ >>>> - pcspk_init(pit); >>>> + pcspk_init(isabus, pit); >>>> >>>> /* 2 82C37 (dma) */ >>>> DMA_init(1,&s->out[1]); >>>> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c >>>> index b61b218..65608dc 100644 >>>> --- a/hw/mips_jazz.c >>>> +++ b/hw/mips_jazz.c >>>> @@ -37,6 +37,7 @@ >>>> #include "loader.h" >>>> #include "mc146818rtc.h" >>>> #include "i8254.h" >>>> +#include "pcspk.h" >>>> #include "blockdev.h" >>>> #include "sysbus.h" >>>> #include "exec-memory.h" >>>> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion >>>> *address_space, >>>> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); >>>> DMA_init(0, cpu_exit_irq); >>>> pit = pit_init(isa_bus, 0x40, 0, NULL); >>>> - pcspk_init(pit); >>>> + pcspk_init(isa_bus, pit); >>>> >>>> /* ISA IO space at 0x90000000 */ >>>> isa_mmio_init(0x90000000, 0x01000000); >>>> diff --git a/hw/pc.c b/hw/pc.c >>>> index 6fb1de7..f6a91a9 100644 >>>> --- a/hw/pc.c >>>> +++ b/hw/pc.c >>>> @@ -37,6 +37,7 @@ >>>> #include "multiboot.h" >>>> #include "mc146818rtc.h" >>>> #include "i8254.h" >>>> +#include "pcspk.h" >>>> #include "msi.h" >>>> #include "sysbus.h" >>>> #include "sysemu.h" >>>> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, >>>> qemu_irq *gsi, >>>> /* connect PIT to output control line of the HPET */ >>>> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, >>>> 0)); >>>> } >>>> - pcspk_init(pit); >>>> + pcspk_init(isa_bus, pit); >>>> >>>> for(i = 0; i< MAX_SERIAL_PORTS; i++) { >>>> if (serial_hds[i]) { >>>> diff --git a/hw/pc.h b/hw/pc.h >>>> index b08708d..1b47bbd 100644 >>>> --- a/hw/pc.h >>>> +++ b/hw/pc.h >>>> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice >>>> *dev, uint8_t addr); >>>> /* hpet.c */ >>>> extern int no_hpet; >>>> >>>> -/* pcspk.c */ >>>> -void pcspk_init(ISADevice *pit); >>>> -int pcspk_audio_init(ISABus *bus); >>>> - >>>> /* piix_pci.c */ >>>> struct PCII440FXState; >>>> typedef struct PCII440FXState PCII440FXState; >>>> diff --git a/hw/pcspk.c b/hw/pcspk.c >>>> index 43df818..5bd9e32 100644 >>>> --- a/hw/pcspk.c >>>> +++ b/hw/pcspk.c >>>> @@ -28,6 +28,7 @@ >>>> #include "audio/audio.h" >>>> #include "qemu-timer.h" >>>> #include "i8254.h" >>>> +#include "pcspk.h" >>>> >>>> #define PCSPK_BUF_LEN 1792 >>>> #define PCSPK_SAMPLE_RATE 32000 >>>> @@ -35,10 +36,13 @@ >>>> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / >>>> PCSPK_MAX_FREQ) >>>> >>>> typedef struct { >>>> + ISADevice dev; >>>> + MemoryRegion ioport; >>>> + uint32_t iobase; >>>> uint8_t sample_buf[PCSPK_BUF_LEN]; >>>> QEMUSoundCard card; >>>> SWVoiceOut *voice; >>>> - ISADevice *pit; >>>> + void *pit; >>>> unsigned int pit_count; >>>> unsigned int samples; >>>> unsigned int play_pos; >>>> @@ -47,7 +51,7 @@ typedef struct { >>>> } PCSpkState; >>>> >>>> static const char *s_spk = "pcspk"; >>>> -static PCSpkState pcspk_state; >>>> +static PCSpkState *pcspk_state; >>>> >>>> static inline void generate_samples(PCSpkState *s) >>>> { >>>> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free) >>>> >>>> int pcspk_audio_init(ISABus *bus) >>>> { >>>> - PCSpkState *s =&pcspk_state; >>>> + PCSpkState *s = pcspk_state; >>>> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0}; >>> >>> >>> This can be a follow up, but what this is really doing is enabling audio >>> for this device. ISABus is unused as a parameter. >> >> Yes, but this is a callback for the audio subsystem, look at >> arch_init.c. Nothing we can do here, only if we change that callback >> prototype. >> >>> >>> I think it would be better to make this a qdev bool property >>> (audio_enabled) and then modify the code that calls this function to >>> either set the property as a global, or use the QOM path to specifically >>> set it on this device. >>> >>> Either way, I think setting an audio_enabled flag via qdev makes a whole >>> lot more sense conceptionally than using the -soundhw option. >> >> Yep, there is room for improvements. The audio system is just another >> backend, like a chardev or netdev. It should once we handled like this I >> guess. >> >>> >>>> >>>> AUD_register_card(s_spk,&s->card); >>>> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus) >>>> return 0; >>>> } >>>> >>>> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) >>>> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr, >>>> + unsigned size) >>>> { >>>> PCSpkState *s = opaque; >>>> int out; >>>> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, >>>> uint32_t addr) >>>> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) | >>>> s->dummy_refresh_clock | out; >>>> } >>>> >>>> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t >>>> val) >>>> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, >>>> uint64_t val, >>>> + unsigned size) >>>> { >>>> PCSpkState *s = opaque; >>>> const int gate = val& 1; >>>> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, >>>> uint32_t addr, uint32_t val) >>>> } >>>> } >>>> >>>> -void pcspk_init(ISADevice *pit) >>>> +static const MemoryRegionOps pcspk_io_ops = { >>>> + .read = pcspk_io_read, >>>> + .write = pcspk_io_write, >>>> + .impl = { >>>> + .min_access_size = 1, >>>> + .max_access_size = 1, >>>> + }, >>>> +}; >>>> + >>>> +static int pcspk_initfn(ISADevice *dev) >>>> +{ >>>> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev); >>>> + >>>> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1); >>>> + isa_register_ioport(NULL,&s->ioport, s->iobase); >>> >>> Should pass dev as the first argument to isa_register_ioport. Otherwise >>> the resource won't be cleaned up during destruction. >> >> Oops, will fix. >> >>> >>>> + >>>> + pcspk_state = s; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void pcspk_class_initfn(ObjectClass *klass, void *data) >>>> { >>>> - PCSpkState *s =&pcspk_state; >>>> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass); >>>> + ic->init = pcspk_initfn; >>>> +} >>>> >>>> - s->pit = pit; >>>> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s); >>>> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s); >>>> +static DeviceInfo pcspk_info = { >>>> + .name = "isa-pcspk", >>>> + .size = sizeof(PCSpkState), >>>> + .no_user = 1, >>>> + .props = (Property[]) { >>>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), >>>> + DEFINE_PROP_PTR("pit", PCSpkState, pit), >>> >>> Please don't introduce a pointer property here. They cannot be used in >>> a meaningful way in qdev. Why not register a link<TYPE_PIT> in >>> instance_init? >> >> Once it's properly usable, I will do so. So far I see now in-tree - >> ideally type-checking - replacement for qdev_prop_set_ptr. > > Why is what's in the tree not usable? > > Just don't do pcspk_init as a static inline (which is not that nice to > do anyway) and you don't need to worry about the availability of an > accessor. The current pattern requested by some reviewers used to be the one I applied here. I dislike it as well when the device can't be seriously configured out. But I can switch over, no problem. > > And BTW, there is strict type checking now, which makes it already an > improvement over property pointers. OK, for my slow understanding: I use qdev_property_add_link in the device init function to create the property, letting it point to a state field. But what should I call from the outside to actually set its value? And what C type does this value have? A DeviceState * or a char * (path)? Jan
On 01/31/2012 04:48 PM, Jan Kiszka wrote: > On 2012-01-31 23:40, Anthony Liguori wrote: >> Why is what's in the tree not usable? >> >> Just don't do pcspk_init as a static inline (which is not that nice to >> do anyway) and you don't need to worry about the availability of an >> accessor. > > The current pattern requested by some reviewers used to be the one I > applied here. I dislike it as well when the device can't be seriously > configured out. But I can switch over, no problem. > >> >> And BTW, there is strict type checking now, which makes it already an >> improvement over property pointers. > > OK, for my slow understanding: I use qdev_property_add_link in the > device init function to create the property, letting it point to a state > field. But what should I call from the outside to actually set its > value? You have a few options: 1) you can add a generic set_child function to qdev... but don't do this, I'll fix it right in my future series 2) You can take advantage of the fact that the only thing that ever sets this is pcspk_init, move pcspk_init to pcspk.c, and then since you have access to the state structure, directly assign it there. 3) Introduce a pcspk_set_pit() function that just does the same thing as (2). 4) Introduce (1) and then have a pcspk_set_pit() that just calls (1). This is what I think we should do moving forward and I plan to do this in my future refactorings. That all said, I think this isn't going to work for you until my next series is merged. Right now, the link properties have strict type checking. You really need a link<PITCommon> in order to be able to accept either a KVMPIT or a normal PIT. My series on the list relaxes the type checking to use implements instead of a strict equality check. So maybe the best thing to do is drop the ptr type entirely, make pcspk_init() be in pcspk.c and touch the private state, and then I'll refactor it later to use a link<> property. > And what C type does this value have? PITState *. Regards, Anthony Liguroi A DeviceState * or a char * > (path)? > > Jan
On 01/31/2012 09:49 PM, Anthony Liguori wrote: >> >> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), >> + DEFINE_PROP_PTR("pit", PCSpkState, pit), > > Please don't introduce a pointer property here. They cannot be used in > a meaningful way in qdev. Why not register a link<TYPE_PIT> in > instance_init? I'm going to clean this up, you can leave the PTR for now. Paolo
On 2012-02-01 08:29, Paolo Bonzini wrote: > On 01/31/2012 09:49 PM, Anthony Liguori wrote: >>> >>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), >>> + DEFINE_PROP_PTR("pit", PCSpkState, pit), >> >> Please don't introduce a pointer property here. They cannot be used in >> a meaningful way in qdev. Why not register a link<TYPE_PIT> in >> instance_init? > > I'm going to clean this up, you can leave the PTR for now. OK, will then ship v4 with this property still in place and count on you. Jan
diff --git a/arch_init.c b/arch_init.c index 2366511..a45485b 100644 --- a/arch_init.c +++ b/arch_init.c @@ -42,6 +42,7 @@ #include "gdbstub.h" #include "hw/smbios.h" #include "exec-memory.h" +#include "hw/pcspk.h" #ifdef TARGET_SPARC int graphic_width = 1024; diff --git a/hw/i82378.c b/hw/i82378.c index 127cadf..79fa8b7 100644 --- a/hw/i82378.c +++ b/hw/i82378.c @@ -20,6 +20,7 @@ #include "pci.h" #include "pc.h" #include "i8254.h" +#include "pcspk.h" //#define DEBUG_I82378 @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, I82378State *s) pit = pit_init(isabus, 0x40, 0, NULL); /* speaker */ - pcspk_init(pit); + pcspk_init(isabus, pit); /* 2 82C37 (dma) */ DMA_init(1, &s->out[1]); diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c index b61b218..65608dc 100644 --- a/hw/mips_jazz.c +++ b/hw/mips_jazz.c @@ -37,6 +37,7 @@ #include "loader.h" #include "mc146818rtc.h" #include "i8254.h" +#include "pcspk.h" #include "blockdev.h" #include "sysbus.h" #include "exec-memory.h" @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion *address_space, cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); DMA_init(0, cpu_exit_irq); pit = pit_init(isa_bus, 0x40, 0, NULL); - pcspk_init(pit); + pcspk_init(isa_bus, pit); /* ISA IO space at 0x90000000 */ isa_mmio_init(0x90000000, 0x01000000); diff --git a/hw/pc.c b/hw/pc.c index 6fb1de7..f6a91a9 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -37,6 +37,7 @@ #include "multiboot.h" #include "mc146818rtc.h" #include "i8254.h" +#include "pcspk.h" #include "msi.h" #include "sysbus.h" #include "sysemu.h" @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, /* connect PIT to output control line of the HPET */ qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0)); } - pcspk_init(pit); + pcspk_init(isa_bus, pit); for(i = 0; i < MAX_SERIAL_PORTS; i++) { if (serial_hds[i]) { diff --git a/hw/pc.h b/hw/pc.h index b08708d..1b47bbd 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr); /* hpet.c */ extern int no_hpet; -/* pcspk.c */ -void pcspk_init(ISADevice *pit); -int pcspk_audio_init(ISABus *bus); - /* piix_pci.c */ struct PCII440FXState; typedef struct PCII440FXState PCII440FXState; diff --git a/hw/pcspk.c b/hw/pcspk.c index 43df818..5bd9e32 100644 --- a/hw/pcspk.c +++ b/hw/pcspk.c @@ -28,6 +28,7 @@ #include "audio/audio.h" #include "qemu-timer.h" #include "i8254.h" +#include "pcspk.h" #define PCSPK_BUF_LEN 1792 #define PCSPK_SAMPLE_RATE 32000 @@ -35,10 +36,13 @@ #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / PCSPK_MAX_FREQ) typedef struct { + ISADevice dev; + MemoryRegion ioport; + uint32_t iobase; uint8_t sample_buf[PCSPK_BUF_LEN]; QEMUSoundCard card; SWVoiceOut *voice; - ISADevice *pit; + void *pit; unsigned int pit_count; unsigned int samples; unsigned int play_pos; @@ -47,7 +51,7 @@ typedef struct { } PCSpkState; static const char *s_spk = "pcspk"; -static PCSpkState pcspk_state; +static PCSpkState *pcspk_state; static inline void generate_samples(PCSpkState *s) { @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free) int pcspk_audio_init(ISABus *bus) { - PCSpkState *s = &pcspk_state; + PCSpkState *s = pcspk_state; struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0}; AUD_register_card(s_spk, &s->card); @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus) return 0; } -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr, + unsigned size) { PCSpkState *s = opaque; int out; @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr) return pit_get_gate(s->pit, 2) | (s->data_on << 1) | s->dummy_refresh_clock | out; } -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val) +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, uint64_t val, + unsigned size) { PCSpkState *s = opaque; const int gate = val & 1; @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val) } } -void pcspk_init(ISADevice *pit) +static const MemoryRegionOps pcspk_io_ops = { + .read = pcspk_io_read, + .write = pcspk_io_write, + .impl = { + .min_access_size = 1, + .max_access_size = 1, + }, +}; + +static int pcspk_initfn(ISADevice *dev) +{ + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev); + + memory_region_init_io(&s->ioport, &pcspk_io_ops, s, "elcr", 1); + isa_register_ioport(NULL, &s->ioport, s->iobase); + + pcspk_state = s; + + return 0; +} + +static void pcspk_class_initfn(ObjectClass *klass, void *data) { - PCSpkState *s = &pcspk_state; + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass); + ic->init = pcspk_initfn; +} - s->pit = pit; - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s); - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s); +static DeviceInfo pcspk_info = { + .name = "isa-pcspk", + .size = sizeof(PCSpkState), + .no_user = 1, + .props = (Property[]) { + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), + DEFINE_PROP_PTR("pit", PCSpkState, pit), + DEFINE_PROP_END_OF_LIST(), + }, + .class_init = pcspk_class_initfn, +}; + +static void pcspk_register(void) +{ + isa_qdev_register(&pcspk_info); } +device_init(pcspk_register) diff --git a/hw/pcspk.h b/hw/pcspk.h new file mode 100644 index 0000000..7f42bac --- /dev/null +++ b/hw/pcspk.h @@ -0,0 +1,45 @@ +/* + * QEMU PC speaker emulation + * + * Copyright (c) 2006 Joachim Henke + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#ifndef HW_PCSPK_H +#define HW_PCSPK_H + +#include "hw.h" +#include "isa.h" + +static inline ISADevice *pcspk_init(ISABus *bus, ISADevice *pit) +{ + ISADevice *dev; + + dev = isa_create(bus, "isa-pcspk"); + qdev_prop_set_uint32(&dev->qdev, "iobase", 0x61); + qdev_prop_set_ptr(&dev->qdev, "pit", pit); + qdev_init_nofail(&dev->qdev); + + return dev; +} + +int pcspk_audio_init(ISABus *bus); + +#endif /* !HW_PCSPK_H */
Convert the PC speaker device to a qdev ISA model. Move the public interface to a dedicated header file at this chance. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- arch_init.c | 1 + hw/i82378.c | 3 +- hw/mips_jazz.c | 3 +- hw/pc.c | 3 +- hw/pc.h | 4 --- hw/pcspk.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++--------- hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 104 insertions(+), 17 deletions(-) create mode 100644 hw/pcspk.h