| Message ID | 20220703212823.10067-4-shorne@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | OpenRISC Virtual Machine | expand |
On Mon, Jul 4, 2022 at 2:59 AM Stafford Horne <shorne@gmail.com> wrote: > > Add an endianness property to allow configuring the RTC as either > native, little or big endian. > > Cc: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Stafford Horne <shorne@gmail.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > hw/rtc/goldfish_rtc.c | 46 ++++++++++++++++++++++++++++------- > include/hw/rtc/goldfish_rtc.h | 2 ++ > 2 files changed, 39 insertions(+), 9 deletions(-) > > diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c > index 35e493be31..24f6587086 100644 > --- a/hw/rtc/goldfish_rtc.c > +++ b/hw/rtc/goldfish_rtc.c > @@ -216,14 +216,34 @@ static int goldfish_rtc_post_load(void *opaque, int version_id) > return 0; > } > > -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 > + } > + }, > }; > > static const VMStateDescription goldfish_rtc_vmstate = { > @@ -265,7 +285,8 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) > SysBusDevice *dev = SYS_BUS_DEVICE(d); > GoldfishRTCState *s = GOLDFISH_RTC(d); > > - memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_rtc_ops, s, > + memory_region_init_io(&s->iomem, OBJECT(s), > + &goldfish_rtc_ops[s->endianness], s, > "goldfish_rtc", 0x24); > sysbus_init_mmio(dev, &s->iomem); > > @@ -274,10 +295,17 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) > s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); > } > > +static Property goldfish_rtc_properties[] = { > + DEFINE_PROP_UINT8("endianness", GoldfishRTCState, endianness, > + DEVICE_NATIVE_ENDIAN), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void goldfish_rtc_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > > + device_class_set_props(dc, goldfish_rtc_properties); > dc->realize = goldfish_rtc_realize; > dc->reset = goldfish_rtc_reset; > dc->vmsd = &goldfish_rtc_vmstate; > diff --git a/include/hw/rtc/goldfish_rtc.h b/include/hw/rtc/goldfish_rtc.h > index 79ca7daf5d..8e1aeb85e3 100644 > --- a/include/hw/rtc/goldfish_rtc.h > +++ b/include/hw/rtc/goldfish_rtc.h > @@ -42,6 +42,8 @@ struct GoldfishRTCState { > uint32_t irq_pending; > uint32_t irq_enabled; > uint32_t time_high; > + > + uint8_t endianness; > }; > > #endif > -- > 2.36.1 > >
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. r~
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. Thanks, Laurent
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. r~
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. Thanks, Laurent
On Mon, Jul 04, 2022 at 03:29:57PM +0530, 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. Yeah, that makes sense. -Stafford
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
On Tue, Jul 05, 2022 at 05:40:31AM +0900, Stafford Horne wrote: > 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) I wish the powers that be would lighten up a little bit and let us change m68k to be LE, and then we could avoid all this... Just a last grumble, I guess. Jason
diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c index 35e493be31..24f6587086 100644 --- a/hw/rtc/goldfish_rtc.c +++ b/hw/rtc/goldfish_rtc.c @@ -216,14 +216,34 @@ static int goldfish_rtc_post_load(void *opaque, int version_id) return 0; } -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 + } + }, }; static const VMStateDescription goldfish_rtc_vmstate = { @@ -265,7 +285,8 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) SysBusDevice *dev = SYS_BUS_DEVICE(d); GoldfishRTCState *s = GOLDFISH_RTC(d); - memory_region_init_io(&s->iomem, OBJECT(s), &goldfish_rtc_ops, s, + memory_region_init_io(&s->iomem, OBJECT(s), + &goldfish_rtc_ops[s->endianness], s, "goldfish_rtc", 0x24); sysbus_init_mmio(dev, &s->iomem); @@ -274,10 +295,17 @@ static void goldfish_rtc_realize(DeviceState *d, Error **errp) s->timer = timer_new_ns(rtc_clock, goldfish_rtc_interrupt, s); } +static Property goldfish_rtc_properties[] = { + DEFINE_PROP_UINT8("endianness", GoldfishRTCState, endianness, + DEVICE_NATIVE_ENDIAN), + DEFINE_PROP_END_OF_LIST(), +}; + static void goldfish_rtc_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + device_class_set_props(dc, goldfish_rtc_properties); dc->realize = goldfish_rtc_realize; dc->reset = goldfish_rtc_reset; dc->vmsd = &goldfish_rtc_vmstate; diff --git a/include/hw/rtc/goldfish_rtc.h b/include/hw/rtc/goldfish_rtc.h index 79ca7daf5d..8e1aeb85e3 100644 --- a/include/hw/rtc/goldfish_rtc.h +++ b/include/hw/rtc/goldfish_rtc.h @@ -42,6 +42,8 @@ struct GoldfishRTCState { uint32_t irq_pending; uint32_t irq_enabled; uint32_t time_high; + + uint8_t endianness; }; #endif
Add an endianness property to allow configuring the RTC as either native, little or big endian. Cc: Laurent Vivier <lvivier@redhat.com> Signed-off-by: Stafford Horne <shorne@gmail.com> --- hw/rtc/goldfish_rtc.c | 46 ++++++++++++++++++++++++++++------- include/hw/rtc/goldfish_rtc.h | 2 ++ 2 files changed, 39 insertions(+), 9 deletions(-)