Hi Andreas,
On Sat, Apr 27, 2013 at 5:16 PM, Andreas Färber <afaer...@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 <atar4q...@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
