Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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~
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
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~
Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property
On Mon, Jul 4, 2022 at 2:59 AM Stafford Horne wrote:
>
> Add an endianness property to allow configuring the RTC as either
> native, little or big endian.
>
> Cc: Laurent Vivier
> Signed-off-by: Stafford Horne
Looks good to me.
Reviewed-by: Anup Patel
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
>
>
[PATCH v2 03/11] goldfish_rtc: Add endianness property
Add an endianness property to allow configuring the RTC as either
native, little or big endian.
Cc: Laurent Vivier
Signed-off-by: Stafford Horne
---
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
