Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property

[Stafford Horne]

On Mon, Jul 04, 2022 at 12:23:23PM +0200, Laurent Vivier wrote:
> On 04/07/2022 12:21, Richard Henderson wrote:
> > On 7/4/22 15:46, Laurent Vivier wrote:
> > > On 04/07/2022 11:59, Richard Henderson wrote:
> > > > On 7/4/22 02:58, Stafford Horne wrote:
> > > > > -static const MemoryRegionOps goldfish_rtc_ops = {
> > > > > -    .read = goldfish_rtc_read,
> > > > > -    .write = goldfish_rtc_write,
> > > > > -    .endianness = DEVICE_NATIVE_ENDIAN,
> > > > > -    .valid = {
> > > > > -        .min_access_size = 4,
> > > > > -        .max_access_size = 4
> > > > > -    }
> > > > > +static const MemoryRegionOps goldfish_rtc_ops[3] = {
> > > > > +    [DEVICE_NATIVE_ENDIAN] = {
> > > > > +        .read = goldfish_rtc_read,
> > > > > +        .write = goldfish_rtc_write,
> > > > > +        .endianness = DEVICE_NATIVE_ENDIAN,
> > > > > +        .valid = {
> > > > > +            .min_access_size = 4,
> > > > > +            .max_access_size = 4
> > > > > +        }
> > > > > +    },
> > > > > +    [DEVICE_LITTLE_ENDIAN] = {
> > > > > +        .read = goldfish_rtc_read,
> > > > > +        .write = goldfish_rtc_write,
> > > > > +        .endianness = DEVICE_LITTLE_ENDIAN,
> > > > > +        .valid = {
> > > > > +            .min_access_size = 4,
> > > > > +            .max_access_size = 4
> > > > > +        }
> > > > > +    },
> > > > > +    [DEVICE_BIG_ENDIAN] = {
> > > > > +        .read = goldfish_rtc_read,
> > > > > +        .write = goldfish_rtc_write,
> > > > > +        .endianness = DEVICE_BIG_ENDIAN,
> > > > > +        .valid = {
> > > > > +            .min_access_size = 4,
> > > > > +            .max_access_size = 4
> > > > > +        }
> > > > > +    },
> > > > >   };
> > > > 
> > > > You don't need 3 copies, only big and little.
> > > > 
> > > > > +static Property goldfish_rtc_properties[] = {
> > > > > +    DEFINE_PROP_UINT8("endianness", GoldfishRTCState, endianness,
> > > > > +                      DEVICE_NATIVE_ENDIAN),
> > > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > > +};
> > > > 
> > > > ... and I think the clear desire for default is little-endian. 
> > > > I would make the property be bool, and add a comment that this
> > > > is only for m68k compatibility, so don't use it in new code.
> > > 
> > > m68k doesn't really need this.
> > > 
> > > kernel with the m68k virt machine and goldfish device supports
> > > "native" mode so I think it's not needed to add another layer of
> > > complexity for it.
> > 
> > "Another level"?  I'm talking about removing "native", and only having
> > "big" and "little", which is less complexity.
> 
> "Less complexity" is to keep only native. I'm not against the change, I'm
> just saying it's not needed by m68k.

Hi Laurent,

I would agree if we only had m68k.  But I am making this change so that OpenRISC
(another big-endian architecture) could use this.  In the OpenRISC case we want
to use this as little-endian so no kernel updates would be needed.

So in the end we will have the following qemu platforms:

  riscv{LE}--------------->goldfish_rtc{LE}
  mips-longsoon3{LE}------>goldfish_rtc{LE}
  openrisc{BE}------------>goldfish_rtc{LE} (LE to BE conversion done in driver)
  m68k{BE}---------------->goldfish_rtc{BE} (only big-endian user)

-Stafford