Re: [Qemu-devel] [PATCH v4 12/16] piix4: add a mc146818rtc controller as specified in datasheet

[Philippe Mathieu-Daudé]

Hi Hervé,

I haven't forgot this series (still 15 months passed...) and it is now
integrated in a bigger one. While retesting the whole previous to post,
I noticed an error...

On 1/6/18 4:37 PM, Hervé Poussineau wrote:
> Remove mc146818rtc instanciated in malta board, to not have it twice.
> 
> Acked-by: Michael S. Tsirkin <address@hidden>
> Acked-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Hervé Poussineau <address@hidden>
> ---
>  hw/isa/piix4.c       | 12 ++++++++++++
>  hw/mips/mips_malta.c |  5 -----
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 7a13e83270..0d68fcb193 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -31,6 +31,7 @@
>  #include "hw/char/isa.h"
>  #include "hw/sysbus.h"
>  #include "hw/timer/i8254.h"
> +#include "hw/timer/mc146818rtc.h"
>  #include "qapi/error.h"
>  
>  PCIDevice *piix4_dev;
> @@ -43,6 +44,7 @@ typedef struct PIIX4State {
>      FDCtrlISABus floppy;
>      ISASerialState serial[2];
>      ISAParallelState parallel;
> +    RTCState rtc;
>  
>      /* Reset Control Register */
>      MemoryRegion rcr_mem;
> @@ -217,6 +219,15 @@ static void piix4_realize(PCIDevice *pci_dev, Error 
> **errp)
>          return;
>      }
>  
> +    /* timer */
> +    qdev_set_parent_bus(DEVICE(&s->rtc), BUS(isa_bus));

... here. You don't set the "base_year" property.
If unset, the default of 1980 is used.
However ...

> +    object_property_set_bool(OBJECT(&s->rtc), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ);
> +
>      piix4_dev = pci_dev;
>      qemu_register_reset(piix4_reset, s);
>  }
> @@ -231,6 +242,7 @@ static void piix4_init(Object *obj)
>          object_initialize(&s->serial[i], sizeof(s->serial[i]), 
> TYPE_ISA_SERIAL);
>      }
>      object_initialize(&s->parallel, sizeof(s->parallel), TYPE_ISA_PARALLEL);
> +    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC);
>  
>      object_property_add_alias(obj, "floppy", OBJECT(&s->floppy), "driveA",
>                                &error_abort);
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 30fb30fc0e..3d304a6e0a 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -995,7 +995,6 @@ void mips_malta_init(MachineState *machine)
>      uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
>      int64_t kernel_entry, bootloader_run_addr;
>      PCIBus *pci_bus;
> -    ISABus *isa_bus;
>      qemu_irq cbus_irq, i8259_irq;
>      PCIDevice *pci;
>      int piix4_devfn;
> @@ -1197,7 +1196,6 @@ void mips_malta_init(MachineState *machine)
>      qdev_prop_set_chr(dev, "parallel", parallel_hds[0]);
>  
>      qdev_init_nofail(dev);
> -    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>      piix4_devfn = pci->devfn;
>  
>      /* Interrupt controller */
> @@ -1213,9 +1211,6 @@ void mips_malta_init(MachineState *machine)
>      smbus_eeprom_init(smbus, 8, smbus_eeprom_buf, smbus_eeprom_size);
>      g_free(smbus_eeprom_buf);
>  
> -    /* Super I/O */
> -    mc146818_rtc_init(isa_bus, 2000, NULL);

... here it is set to 2000.
The same value is used in the PC machine (which uses the PIIX3).

> -
>      /* Network card */
>      network_init(pci_bus);
>  
> 

I'll amend this snippet to your patch:

-- >8 --
     /* RTC */
     qdev_set_parent_bus(DEVICE(&s->rtc), BUS(isa_bus));
+    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
     object_property_set_bool(OBJECT(&s->rtc), true, "realized", &err);
--

Paolo/Michael: I'll keep your Acked-by, I hope you are OK with that.

Regards,

Phil.