Re: [PATCH v2 10/10] hw/mips/jazz: specify correct endian for dp8393x device

[Mark Cave-Ayland]

On 25/06/2021 09:51, Philippe Mathieu-Daudé wrote:
> On 6/25/21 8:54 AM, Mark Cave-Ayland wrote:
> > The MIPS magnum machines are available in both big endian (mips64) and little
> > endian (mips64el) configurations. Ensure that the dp893x big_endian property
> > is set accordingly using logic similar to that used for the MIPS malta
> > machines.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/mips/jazz.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> > index 89ca8bb910..ee1789183e 100644
> > --- a/hw/mips/jazz.c
> > +++ b/hw/mips/jazz.c
> > @@ -126,7 +126,7 @@ static void mips_jazz_init(MachineState *machine,
> >   {
> >       MemoryRegion *address_space = get_system_memory();
> >       char *filename;
> > -    int bios_size, n;
> > +    int bios_size, n, big_endian;
>
> Why not use a boolean directly?

Good point. I grepped the codebase for an existing example for using DEFINE_PROP_BOOL 
and setting the value using qdev_prop_set_bit(), and the first hit was in 
hw/arm/allwinner-h3.c for the "start-powered-off" property. The existing MIPS Malta 
code also used an integer variable to store the current endian and so that's what I 
went with.

I wonder why we don't have a qdev_prop_set_bool() to match DEFINE_PROP_BOOL?

> >       Clock *cpuclk;
> >       MIPSCPU *cpu;
> >       MIPSCPUClass *mcc;
> > @@ -158,6 +158,12 @@ static void mips_jazz_init(MachineState *machine,
> >           [JAZZ_PICA61] = {33333333, 4},
> >       };
> >   
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    big_endian = 1;
> > +#else
> > +    big_endian = 0;
> > +#endif
> > +
> >       if (machine->ram_size > 256 * MiB) {
> >           error_report("RAM size more than 256Mb is not supported");
> >           exit(EXIT_FAILURE);
> > @@ -290,6 +296,7 @@ static void mips_jazz_init(MachineState *machine,
> >               dev = qdev_new("dp8393x");
> >               qdev_set_nic_properties(dev, nd);
> >               qdev_prop_set_uint8(dev, "it_shift", 2);
> > +            qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
> >               object_property_set_link(OBJECT(dev), "dma_mr",
> >                                        OBJECT(rc4030_dma_mr), &error_abort);
> >               sysbus = SYS_BUS_DEVICE(dev);


ATB,

Mark.