| Message ID | 20220119000506.1299843-3-laurent@vivier.eu |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | m68k: Add Virtual M68k Machine | expand |
Hi Laurent, On Wed, Jan 19, 2022 at 1:05 AM Laurent Vivier <laurent@vivier.eu> wrote: > The goldfish device always uses the same endianness as the architecture > using it: > https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-master-dev/hw/timer/goldfish_timer.c#177 > > On a big-endian machine, the device is also big-endian, on a > little-endian machine the device is little-endian. > > So we need to use the right accessor to read/write values to the goldfish > registers: ioread32()/iowrite32() on a little-endian machine, > ioread32be()/iowrite32be() on a big-endian machine. > > This patch introduces goldfish_ioread32()/goldfish_iowrite32() to allow > architectures to define them accordlingly to their endianness. > > We define them by default in asm-generic/io.h to access the device using > little-endian access as it is the current use, but we will be able to define > big-endian version when new architectures will use them. > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> Thanks for your patch! > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -906,6 +906,13 @@ static inline void iowrite64_rep(volatile void __iomem *addr, > #endif /* CONFIG_64BIT */ > #endif /* CONFIG_GENERIC_IOMAP */ > > +#ifndef goldfish_ioread32 > +#define goldfish_ioread32 ioread32 > +#endif > +#ifndef goldfish_iowrite32 > +#define goldfish_iowrite32 iowrite32 > +#endif > + > #ifdef __KERNEL__ I've just discovered include/linux/goldfish.h, which already has gf_*() accessors for 64-bit, so it'd make sense to move the above there, and adjust the names. Arnd: note that the existing ones do use __raw_writel(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Jan 19, 2022 at 9:21 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jan 19, 2022 at 1:05 AM Laurent Vivier <laurent@vivier.eu> wrote: > > I've just discovered include/linux/goldfish.h, which already has gf_*() > accessors for 64-bit, so it'd make sense to move the above there, > and adjust the names. Yes, good idea. > > Arnd: note that the existing ones do use __raw_writel(). It looks like Laurent introduced that bug in da31de35cd2f ("tty: goldfish: use __raw_writel()/__raw_readl()") and could fix it up here. Laurent, was the intent of this earlier patch also to make the driver usabel for m68k, or are there any other targets you looked at that had mixed up endianness? Arnd
Le 19/01/2022 à 09:49, Arnd Bergmann a écrit : > On Wed, Jan 19, 2022 at 9:21 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Wed, Jan 19, 2022 at 1:05 AM Laurent Vivier <laurent@vivier.eu> wrote: >> >> I've just discovered include/linux/goldfish.h, which already has gf_*() >> accessors for 64-bit, so it'd make sense to move the above there, >> and adjust the names. > > Yes, good idea. So the idea is to put goldfish accessors inside a "#ifdef CONFIG_M68K ... #else ... #endif" in include/linux/goldfish.h and not in generic-asm/io.h for the generic version and m68k/include/ams/io.h for the m68k version? > >> >> Arnd: note that the existing ones do use __raw_writel(). > > It looks like Laurent introduced that bug in da31de35cd2f ("tty: goldfish: use > __raw_writel()/__raw_readl()") and could fix it up here. Laurent, was the intent The idea was to use the native endianness of the CPU, I missed it can differ from the one of the architecture. > of this earlier patch also to make the driver usabel for m68k, or are there > any other targets you looked at that had mixed up endianness? > Yes, the intent was to make it usable for m68k. I think all the targets that use goldfish are little-endian, it's why there was no problem until now. Let me know which solution you prefer, I will update the series accordingly. Thanks, Laurent
Hi Laurent, On Wed, Jan 19, 2022 at 10:11 AM Laurent Vivier <laurent@vivier.eu> wrote: > Le 19/01/2022 à 09:49, Arnd Bergmann a écrit : > > On Wed, Jan 19, 2022 at 9:21 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> On Wed, Jan 19, 2022 at 1:05 AM Laurent Vivier <laurent@vivier.eu> wrote: > >> > >> I've just discovered include/linux/goldfish.h, which already has gf_*() > >> accessors for 64-bit, so it'd make sense to move the above there, > >> and adjust the names. > > > > Yes, good idea. > > So the idea is to put goldfish accessors inside a "#ifdef CONFIG_M68K ... #else ... #endif" in > include/linux/goldfish.h and not in generic-asm/io.h for the generic version and > m68k/include/ams/io.h for the m68k version? No, just move +#ifndef goldfish_ioread32 +#define goldfish_ioread32 ioread32 +#endif +#ifndef goldfish_iowrite32 +#define goldfish_iowrite32 iowrite32 +#endif to <linux/goldfish.h>, and rename them to gf_*(). Architectures can still override them in their own <asm/io.h> (<linux/goldfish.h> includes <linux/io.h>). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/rtc/rtc-goldfish.c b/drivers/rtc/rtc-goldfish.c index 7ab95d052644..1b2180ccfcdd 100644 --- a/drivers/rtc/rtc-goldfish.c +++ b/drivers/rtc/rtc-goldfish.c @@ -41,8 +41,8 @@ static int goldfish_rtc_read_alarm(struct device *dev, rtcdrv = dev_get_drvdata(dev); base = rtcdrv->base; - rtc_alarm_low = readl(base + TIMER_ALARM_LOW); - rtc_alarm_high = readl(base + TIMER_ALARM_HIGH); + rtc_alarm_low = goldfish_ioread32(base + TIMER_ALARM_LOW); + rtc_alarm_high = goldfish_ioread32(base + TIMER_ALARM_HIGH); rtc_alarm = (rtc_alarm_high << 32) | rtc_alarm_low; do_div(rtc_alarm, NSEC_PER_SEC); @@ -50,7 +50,7 @@ static int goldfish_rtc_read_alarm(struct device *dev, rtc_time64_to_tm(rtc_alarm, &alrm->time); - if (readl(base + TIMER_ALARM_STATUS)) + if (goldfish_ioread32(base + TIMER_ALARM_STATUS)) alrm->enabled = 1; else alrm->enabled = 0; @@ -71,18 +71,18 @@ static int goldfish_rtc_set_alarm(struct device *dev, if (alrm->enabled) { rtc_alarm64 = rtc_tm_to_time64(&alrm->time) * NSEC_PER_SEC; - writel((rtc_alarm64 >> 32), base + TIMER_ALARM_HIGH); - writel(rtc_alarm64, base + TIMER_ALARM_LOW); - writel(1, base + TIMER_IRQ_ENABLED); + goldfish_iowrite32((rtc_alarm64 >> 32), base + TIMER_ALARM_HIGH); + goldfish_iowrite32(rtc_alarm64, base + TIMER_ALARM_LOW); + goldfish_iowrite32(1, base + TIMER_IRQ_ENABLED); } else { /* * if this function was called with enabled=0 * then it could mean that the application is * trying to cancel an ongoing alarm */ - rtc_status_reg = readl(base + TIMER_ALARM_STATUS); + rtc_status_reg = goldfish_ioread32(base + TIMER_ALARM_STATUS); if (rtc_status_reg) - writel(1, base + TIMER_CLEAR_ALARM); + goldfish_iowrite32(1, base + TIMER_CLEAR_ALARM); } return 0; @@ -98,9 +98,9 @@ static int goldfish_rtc_alarm_irq_enable(struct device *dev, base = rtcdrv->base; if (enabled) - writel(1, base + TIMER_IRQ_ENABLED); + goldfish_iowrite32(1, base + TIMER_IRQ_ENABLED); else - writel(0, base + TIMER_IRQ_ENABLED); + goldfish_iowrite32(0, base + TIMER_IRQ_ENABLED); return 0; } @@ -110,7 +110,7 @@ static irqreturn_t goldfish_rtc_interrupt(int irq, void *dev_id) struct goldfish_rtc *rtcdrv = dev_id; void __iomem *base = rtcdrv->base; - writel(1, base + TIMER_CLEAR_INTERRUPT); + goldfish_iowrite32(1, base + TIMER_CLEAR_INTERRUPT); rtc_update_irq(rtcdrv->rtc, 1, RTC_IRQF | RTC_AF); @@ -128,8 +128,8 @@ static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm) rtcdrv = dev_get_drvdata(dev); base = rtcdrv->base; - time_low = readl(base + TIMER_TIME_LOW); - time_high = readl(base + TIMER_TIME_HIGH); + time_low = goldfish_ioread32(base + TIMER_TIME_LOW); + time_high = goldfish_ioread32(base + TIMER_TIME_HIGH); time = (time_high << 32) | time_low; do_div(time, NSEC_PER_SEC); @@ -149,8 +149,8 @@ static int goldfish_rtc_set_time(struct device *dev, struct rtc_time *tm) base = rtcdrv->base; now64 = rtc_tm_to_time64(tm) * NSEC_PER_SEC; - writel((now64 >> 32), base + TIMER_TIME_HIGH); - writel(now64, base + TIMER_TIME_LOW); + goldfish_iowrite32((now64 >> 32), base + TIMER_TIME_HIGH); + goldfish_iowrite32(now64, base + TIMER_TIME_LOW); return 0; } diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7ce93aaf69f8..7f7e9d8c2eef 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -906,6 +906,13 @@ static inline void iowrite64_rep(volatile void __iomem *addr, #endif /* CONFIG_64BIT */ #endif /* CONFIG_GENERIC_IOMAP */ +#ifndef goldfish_ioread32 +#define goldfish_ioread32 ioread32 +#endif +#ifndef goldfish_iowrite32 +#define goldfish_iowrite32 iowrite32 +#endif + #ifdef __KERNEL__ #include <linux/vmalloc.h>
The goldfish device always uses the same endianness as the architecture using it: https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-master-dev/hw/timer/goldfish_timer.c#177 On a big-endian machine, the device is also big-endian, on a little-endian machine the device is little-endian. So we need to use the right accessor to read/write values to the goldfish registers: ioread32()/iowrite32() on a little-endian machine, ioread32be()/iowrite32be() on a big-endian machine. This patch introduces goldfish_ioread32()/goldfish_iowrite32() to allow architectures to define them accordlingly to their endianness. We define them by default in asm-generic/io.h to access the device using little-endian access as it is the current use, but we will be able to define big-endian version when new architectures will use them. Signed-off-by: Laurent Vivier <laurent@vivier.eu> --- drivers/rtc/rtc-goldfish.c | 30 +++++++++++++++--------------- include/asm-generic/io.h | 7 +++++++ 2 files changed, 22 insertions(+), 15 deletions(-)