Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function

[Mark Cave-Ayland]

On 17/10/2020 10:45, Philippe Mathieu-Daudé wrote:

> On 10/16/20 10:38 PM, BALATON Zoltan via wrote:
> > On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > > hw/ppc/ppc405_boards.c | 10 +++++++++-
> > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> > > index 6198ec1035..4687715b15 100644
> > > --- a/hw/ppc/ppc405_boards.c
> > > +++ b/hw/ppc/ppc405_boards.c
> > > @@ -28,6 +28,8 @@
> > > #include "qemu-common.h"
> > > #include "cpu.h"
> > > #include "hw/ppc/ppc.h"
> > > +#include "hw/qdev-properties.h"
> > > +#include "hw/sysbus.h"
> > > #include "ppc405.h"
> > > #include "hw/rtc/m48t59.h"
> > > #include "hw/block/flash.h"
> > > @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
> > >     char *filename;
> > >     ppc4xx_bd_info_t bd;
> > >     CPUPPCState *env;
> > > +    DeviceState *dev;
> > > +    SysBusDevice *s;
> > >     qemu_irq *pic;
> > >     MemoryRegion *bios;
> > >     MemoryRegion *sram = g_new(MemoryRegion, 1);
> > > @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
> > >     /* Register FPGA */
> > >     ref405ep_fpga_init(sysmem, 0xF0300000);
> > >     /* Register NVRAM */
> > > -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
> > > +    dev = qdev_new("sysbus-m48t08");
> > > +    qdev_prop_set_int32(dev, "base-year", 1968);
> >
> > Is there anything that uses other than 1968 as base year? If not this could be the 
> > default in the device so you don't need these set prop calls here and in sun machines.
> >
> > The only other place this device is used seems to be ppc/prep machine that uses the 
> > isa version but does not set a base year. Is that a bug? The original prep machine 
> > removed in b2ce76a0730 used 2000 but that's unlikely as well as these machines 
> > predate that.
>
> =)

This is quite interesting, since if you look at prep.c you see that the machine has 
both a MC146818 RTC and a M48T59 RTC, and the NVRAM variables are stored in the 
M48T59. My guess is that a real 40p has a MC146818, but OHW only supported M48T59 and 
so it was only ever used to pass data from QEMU to OHW - the OS would use MC146818 
for the clock which has the base year set correctly.

> > Anyway, the sysbus and isa versions are different
>
> They shouldn't, it is the same chipset, wired differently.
>
> > so their default base-year could be set independently and then boards won't need to 
> > set this propery and may be could use qdev_create_simple instead or whatever that 
> > was renamed to.
>
> Agreed.
>
> Preferably following Zoltan's suggestion:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

There is certainly some more untangling to be done here, but given my current backlog 
it is something that will need to wait until after 5.2.


ATB,

Mark.