[Qemu-devel] [PATCH 0/2] sparc64: Fix NVRAM access mode

[Qemu-devel] [PATCH 0/2] sparc64: Fix NVRAM access mode

[Artyom Tarasenko]

The sun4u machines use a slightly different modification of the ISA Mostek chip.
It has MMIO, 1968 as a base year and no IRQ line. Since it matches our m48t08,
make keep PIO behavior for the 59 model and use MMIO for the 8 model.

This series fixes recognising Mostek RTC in the Linux/sparc64 guest.

Artyom Tarasenko (2):
  m48t59: use mmio for the m48t08 model of the m48t59_isa card
  sparc64: Use the correct type of the Mostek NVRAM chip

 hw/sparc64/sun4u.c |    5 ++++-
 hw/timer/m48t59.c  |   38 +++++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH 1/2] m48t59: use mmio for the m48t08 model of the m48t59_isa card

[Artyom Tarasenko]

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(-)

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;
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] sparc64: Use the correct type of the Mostek NVRAM chip

[Artyom Tarasenko]

The sun4u machines use an ISA Mostek chip which has MMIO, uses 1968
as a base year and no IRQ line. So, use the m48t08.

Please note that this patch has its OpenBIOS counterpart.
Together they fix recognising NVRAM in the sun4u machine.

Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
---
 hw/sparc64/sun4u.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 5c2bbd4..bfb19d4 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -866,7 +866,10 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
         fd[i] = drive_get(IF_FLOPPY, 0, i);
     }
     fdctrl_init_isa(isa_bus, fd);
-    nvram = m48t59_init_isa(isa_bus, 0x0074, NVRAM_SIZE, 59);
+
+    /* XXX Ultra-5 has it at the beginning of the EBUS range, but we have
+     * serial port and other isa devices there */
+    nvram = m48t59_init_isa(isa_bus, 0x2000, NVRAM_SIZE, 8);
 
     initrd_size = 0;
     initrd_addr = 0;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/2] sparc64: Fix NVRAM access mode

[Blue Swirl]

On Sat, Apr 27, 2013 at 7:12 AM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
> The sun4u machines use a slightly different modification of the ISA Mostek chip.
> It has MMIO, 1968 as a base year and no IRQ line. Since it matches our m48t08,
> make keep PIO behavior for the 59 model and use MMIO for the 8 model.

The chip is definitely m48t59 (SGS-Thompson, M48T59Y-70MH1), even if
the IRQ is not connected and software uses 1968 as the base year.

Please make the base year a qdev property and keep the model unchanged.

>
> This series fixes recognising Mostek RTC in the Linux/sparc64 guest.
>
> Artyom Tarasenko (2):
>   m48t59: use mmio for the m48t08 model of the m48t59_isa card
>   sparc64: Use the correct type of the Mostek NVRAM chip
>
>  hw/sparc64/sun4u.c |    5 ++++-
>  hw/timer/m48t59.c  |   38 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 39 insertions(+), 4 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/2] sparc64: Fix NVRAM access mode

[Artyom Tarasenko]

On Sat, Apr 27, 2013 at 1:39 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sat, Apr 27, 2013 at 7:12 AM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>> The sun4u machines use a slightly different modification of the ISA Mostek chip.
>> It has MMIO, 1968 as a base year and no IRQ line. Since it matches our m48t08,
>> make keep PIO behavior for the 59 model and use MMIO for the 8 model.
>
> The chip is definitely m48t59 (SGS-Thompson, M48T59Y-70MH1), even if
> the IRQ is not connected and software uses 1968 as the base year.

Ops. But it's not just software afaics - think about leap years. Do you by any
chance have any docs beyond m48t59y.pdf? There is no information about base
year in there. And in the Linux kernel there is just some CONFIG_SPARC
#ifdefery.

> Please make the base year a qdev property and keep the model unchanged.

Maybe rather use something like "is_sun" property? It could be used to
decide whether the IRQ should be connected and whether a PIO or a MMIO
access should be used.

Btw can "isa_init_irq(dev, &s->IRQ, 8)" be moved from m48t59_init_isa1
to m48t59_init_isa ? Or shall the irq be silenced at the isa bridge
level?

--
Regards,
Artyom Tarasenko

linux/sparc and solaris/sparc under qemu blog:
http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH 1/2] m48t59: use mmio for the m48t08 model of the m48t59_isa card

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/2] m48t59: use mmio for the m48t08 model of the m48t59_isa card

[Artyom Tarasenko]

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