| Message ID | e2498b66b4aeec3b9850d89d3bc87af1d7d37e4b.1367046225.git.atar4qemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 27.04.2013 09:12, schrieb Artyom Tarasenko: > PrEP and SPARC machines use slightly different variations of PReP :) > a Mostek NVRAM chip. Since the SPARC variant is much closer > to a m48t08 type, the model can be used to differentiate between > the PIO and MMIO accesses. > > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > hw/timer/m48t59.c | 38 +++++++++++++++++++++++++++++++++++--- > 1 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c > index 5019e06..00ad417 100644 > --- a/hw/timer/m48t59.c > +++ b/hw/timer/m48t59.c > @@ -632,6 +632,33 @@ static const MemoryRegionOps m48t59_io_ops = { > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > +static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned size) > +{ > + M48t59State *NVRAM = opaque; Probably this is just copy&paste from old code, but please use lower_case_names for variables in new code. > + uint32_t retval; > + > + retval = m48t59_read(NVRAM, addr); > + return retval; > +} > + > +static void nvram_write(void *opaque, hwaddr addr, uint64_t value, > + unsigned size) Indentation one-off. > +{ > + M48t59State *NVRAM = opaque; > + > + m48t59_write(NVRAM, addr, value & 0xff); > +} > + > +static const MemoryRegionOps m48t59_mmio_ops = { > + .read = nvram_read, > + .write = nvram_write, > + .impl = { > + .min_access_size = 1, > + .max_access_size = 1, > + }, > + .endianness = DEVICE_LITTLE_ENDIAN, > +}; > + > /* Initialisation routine */ > M48t59State *m48t59_init(qemu_irq IRQ, hwaddr mem_base, > uint32_t io_base, uint16_t size, int model) > @@ -676,7 +703,11 @@ M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size, > d = DO_UPCAST(M48t59ISAState, busdev, dev); > s = &d->state; > > - memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); > + if (model == 59) { > + memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); > + } else { > + memory_region_init_io(&d->io, &m48t59_mmio_ops, s, "m48t59", size); If you distinguish here, it may be a good idea to reflect that in the region's name. > + } > if (io_base != 0) { > isa_register_ioport(dev, &d->io, io_base); > } > @@ -700,8 +731,9 @@ static int m48t59_init_isa1(ISADevice *dev) > { > M48t59ISAState *d = DO_UPCAST(M48t59ISAState, busdev, dev); > M48t59State *s = &d->state; > - > - isa_init_irq(dev, &s->IRQ, 8); > + if (s->model == 59) { > + isa_init_irq(dev, &s->IRQ, 8); > + } > m48t59_init_common(s); > > return 0; Regarding your question of whether to move this: With my ISA realize series this function becomes a ..._realize function. isa_init_irq() relies on the device being on an ISA bus to retrieve the qemu_irq, so this cannot be done in instance_init, thus in DeviceClass::init/realize. The existing legacy m48t59_init_isa() function should probably rather shrink in size and one day possibly be inlined rather than growing with stuff that was encapsulated in initfn or realizefn. Regards, Andreas
Hi Andreas, On Sat, Apr 27, 2013 at 5:16 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 27.04.2013 09:12, schrieb Artyom Tarasenko: >> PrEP and SPARC machines use slightly different variations of > > PReP :) Ops. :) >> a Mostek NVRAM chip. Since the SPARC variant is much closer >> to a m48t08 type, the model can be used to differentiate between >> the PIO and MMIO accesses. >> >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >> --- >> hw/timer/m48t59.c | 38 +++++++++++++++++++++++++++++++++++--- >> 1 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c >> index 5019e06..00ad417 100644 >> --- a/hw/timer/m48t59.c >> +++ b/hw/timer/m48t59.c >> @@ -632,6 +632,33 @@ static const MemoryRegionOps m48t59_io_ops = { >> .endianness = DEVICE_LITTLE_ENDIAN, >> }; >> >> +static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned size) >> +{ >> + M48t59State *NVRAM = opaque; > > Probably this is just copy&paste from old code, but please use > lower_case_names for variables in new code. Will do. >> + uint32_t retval; >> + >> + retval = m48t59_read(NVRAM, addr); >> + return retval; >> +} >> + >> +static void nvram_write(void *opaque, hwaddr addr, uint64_t value, >> + unsigned size) > > Indentation one-off. Good catch. Is btw a test case for checkpatch.pl - it doesn't complain. >> +{ >> + M48t59State *NVRAM = opaque; >> + >> + m48t59_write(NVRAM, addr, value & 0xff); >> +} >> + >> +static const MemoryRegionOps m48t59_mmio_ops = { >> + .read = nvram_read, >> + .write = nvram_write, >> + .impl = { >> + .min_access_size = 1, >> + .max_access_size = 1, >> + }, >> + .endianness = DEVICE_LITTLE_ENDIAN, >> +}; >> + >> /* Initialisation routine */ >> M48t59State *m48t59_init(qemu_irq IRQ, hwaddr mem_base, >> uint32_t io_base, uint16_t size, int model) >> @@ -676,7 +703,11 @@ M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size, >> d = DO_UPCAST(M48t59ISAState, busdev, dev); >> s = &d->state; >> >> - memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); >> + if (model == 59) { >> + memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); >> + } else { >> + memory_region_init_io(&d->io, &m48t59_mmio_ops, s, "m48t59", size); > > If you distinguish here, it may be a good idea to reflect that in the > region's name. Will do. >> + } >> if (io_base != 0) { >> isa_register_ioport(dev, &d->io, io_base); >> } >> @@ -700,8 +731,9 @@ static int m48t59_init_isa1(ISADevice *dev) >> { >> M48t59ISAState *d = DO_UPCAST(M48t59ISAState, busdev, dev); >> M48t59State *s = &d->state; >> - >> - isa_init_irq(dev, &s->IRQ, 8); >> + if (s->model == 59) { >> + isa_init_irq(dev, &s->IRQ, 8); >> + } >> m48t59_init_common(s); >> >> return 0; > > Regarding your question of whether to move this: With my ISA realize > series this function becomes a ..._realize function. isa_init_irq() > relies on the device being on an ISA bus to retrieve the qemu_irq, so > this cannot be done in instance_init, thus in DeviceClass::init/realize. > The existing legacy m48t59_init_isa() function should probably rather > shrink in size and one day possibly be inlined rather than growing with > stuff that was encapsulated in initfn or realizefn. Totally agree with this approach. The question is how to model the various chip models. Should M48t59ISAState get an "irq_num" field? Hardcoding interrupt number just doesn't feel right. Artyom -- Regards, Artyom Tarasenko linux/sparc and solaris/sparc under qemu blog: http://tyom.blogspot.com/search/label/qemu
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c index 5019e06..00ad417 100644 --- a/hw/timer/m48t59.c +++ b/hw/timer/m48t59.c @@ -632,6 +632,33 @@ static const MemoryRegionOps m48t59_io_ops = { .endianness = DEVICE_LITTLE_ENDIAN, }; +static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned size) +{ + M48t59State *NVRAM = opaque; + uint32_t retval; + + retval = m48t59_read(NVRAM, addr); + return retval; +} + +static void nvram_write(void *opaque, hwaddr addr, uint64_t value, + unsigned size) +{ + M48t59State *NVRAM = opaque; + + m48t59_write(NVRAM, addr, value & 0xff); +} + +static const MemoryRegionOps m48t59_mmio_ops = { + .read = nvram_read, + .write = nvram_write, + .impl = { + .min_access_size = 1, + .max_access_size = 1, + }, + .endianness = DEVICE_LITTLE_ENDIAN, +}; + /* Initialisation routine */ M48t59State *m48t59_init(qemu_irq IRQ, hwaddr mem_base, uint32_t io_base, uint16_t size, int model) @@ -676,7 +703,11 @@ M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size, d = DO_UPCAST(M48t59ISAState, busdev, dev); s = &d->state; - memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); + if (model == 59) { + memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4); + } else { + memory_region_init_io(&d->io, &m48t59_mmio_ops, s, "m48t59", size); + } if (io_base != 0) { isa_register_ioport(dev, &d->io, io_base); } @@ -700,8 +731,9 @@ static int m48t59_init_isa1(ISADevice *dev) { M48t59ISAState *d = DO_UPCAST(M48t59ISAState, busdev, dev); M48t59State *s = &d->state; - - isa_init_irq(dev, &s->IRQ, 8); + if (s->model == 59) { + isa_init_irq(dev, &s->IRQ, 8); + } m48t59_init_common(s); return 0;
PrEP and SPARC machines use slightly different variations of a Mostek NVRAM chip. Since the SPARC variant is much closer to a m48t08 type, the model can be used to differentiate between the PIO and MMIO accesses. Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> --- hw/timer/m48t59.c | 38 +++++++++++++++++++++++++++++++++++--- 1 files changed, 35 insertions(+), 3 deletions(-)