RE: [PATCH v6 5/5] MIPS: ranchu: Add Ranchu as a new generic-based board

[Miodrag Dinic]

Hello James,

sorry for a late reply, please find the answers in-lined :

> On Mon, Oct 30, 2017 at 12:56:36PM +0100, Aleksandar Markovic wrote:
> > From: Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> > 
> > Provide amendments to the MIPS generic platform framework so that
> > the new generic-based board Ranchu can be chosen to be built.
> 
> A bit more info about the board would be good here. What boot protocol
> is used? Does QEMU generate the DT dynamically?

I agree, we will fill the commit message with necessary information about
the newly introduced virtual board.

> > 
> > Signed-off-by: Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> > Signed-off-by: Goran Ferenc <goran.ferenc@xxxxxxxx>
> > Signed-off-by: Aleksandar Markovic <aleksandar.markovic@xxxxxxxx>
> > ---
> >  MAINTAINERS                                   |  6 ++
> >  arch/mips/configs/generic/board-ranchu.config | 30 ++++++++++
> >  arch/mips/generic/Kconfig                     | 10 ++++
> >  arch/mips/generic/Makefile                    |  1 +
> >  arch/mips/generic/board-ranchu.c              | 79 +++++++++++++++++++++++++++
> >  5 files changed, 126 insertions(+)
> >  create mode 100644 arch/mips/configs/generic/board-ranchu.config
> >  create mode 100644 arch/mips/generic/board-ranchu.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f1be016..e429cc2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11308,6 +11308,12 @@ S:   Maintained
> >  F:   Documentation/blockdev/ramdisk.txt
> >  F:   drivers/block/brd.c
> >  
> > +RANCHU VIRTUAL BOARD FOR MIPS
> > +M:   Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> > +L:   linux-mips@xxxxxxxxxxxxxx
> > +S:   Supported
> > +F:   arch/mips/generic/board-ranchu.c
> 
> Maybe worth adding arch/mips/configs/generic/board-ranchu.config too.

It will be addressed in V7.

> > +
> >  RANDOM NUMBER DRIVER
> >  M:   "Theodore Ts'o" <tytso@xxxxxxx>
> >  S:   Maintained
> 
> > diff --git a/arch/mips/generic/Kconfig b/arch/mips/generic/Kconfig
> > index e0436aa..93582be 100644
> > --- a/arch/mips/generic/Kconfig
> > +++ b/arch/mips/generic/Kconfig
> > @@ -42,4 +42,14 @@ config FIT_IMAGE_FDT_NI169445
> >          Enable this to include the FDT for the 169445 platform from
> >          National Instruments in the FIT kernel image.
> >  
> > +config VIRT_BOARD_RANCHU
> > +     bool "Ranchu platform for Android emulator"
> > +     help
> > +       This enables support for the platform used by Android emulator.
> > +
> > +       Ranchu platform consists of a set of virtual devices. This platform
> > +       enables emulation of variety of virtual configurations while using
> > +       Android emulator. Android emulator is based on Qemu, and contains
> > +       the support for the same set of virtual devices.
> 
> This is effectively in the section "FIT/UHI Boards", but it has a
> platform file and no DT/FIT stuff in tree a bit like the boards in the
> section "Legacy (non-UHI/non-FIT) Boards".
> 
> I'm guessing it might be something in between, with UHI + platform code,
> but DT provided by QEMU (i.e. FIT support makes no sense)?

The Ranchu emulator was designed to provide the DT dynamically generated on
the QEMU side and passed to the kernel using UHI boot protocol in DTB handover mode.
Currently we do not support FIT, but I think it is doable to enable it
at some point later in time when we stop supporting android 3.10/3.18 kernels
in Ranchu.

> If it uses UHI I suppose it doesn't belong in the legacy section, but I
> think a consistent prompt would be beneficial, e.g.
> 
> +config VIRT_BOARD_RANCHU
> +       bool "Support Ranchu platform for Android emulator"
> ...

It will be addressed in V7.

> > diff --git a/arch/mips/generic/board-ranchu.c b/arch/mips/generic/board-ranchu.c
> > new file mode 100644
> > index 0000000..0397752
> > --- /dev/null
> > +++ b/arch/mips/generic/board-ranchu.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * Support code for virtual Ranchu board for MIPS.
> > + *
> > + * Author: Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include <linux/of_address.h>
> > +
> > +#include <asm/machine.h>
> > +#include <asm/time.h>
> > +
> > +#define GOLDFISH_TIMER_LOW           0x00
> > +#define GOLDFISH_TIMER_HIGH          0x04
> > +
> > +static __init uint64_t read_rtc_time(void __iomem *base)
> > +{
> > +     u64 time_low;
> > +     u64 time_high;
> > +
> > +     time_low = readl(base + GOLDFISH_TIMER_LOW);
> > +     time_high = readl(base + GOLDFISH_TIMER_HIGH);
> > +
> > +     return (time_high << 32) | time_low;
> 
> What if high changes while reading this?
> 
> E.g.
> TIMER_LOW        0x00000000 *0xffffffff*
> TIMER_HIGH      *0x00000001* 0x00000000
> 
> You'd presumably get 0x00000001ffffffff.
> 
> Perhaps it should read HIGH before too, and retry if it has changed.

This was already discussed in some earlier posts. (https://patchwork.linux-mips.org/patch/16628/)
Reading the low value first actually latches the high value,
so it is safe to leave it this way. Here is the relevant RTC device
implementation in QEMU:

static uint64_t goldfish_rtc_read(void *opaque, hwaddr offset, unsigned size)
{
    struct rtc_state *s = (struct rtc_state *)opaque;
    switch(offset) {
        case TIMER_TIME_LOW:
            s->now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->time_base;
            return s->now_ns;
        case TIMER_TIME_HIGH:
            return s->now_ns >> 32;

Kind regards,
Miodrag


________________________________________
From: James Hogan
Sent: Monday, October 30, 2017 5:45 PM
To: Aleksandar Markovic
Cc: linux-mips@xxxxxxxxxxxxxx; Miodrag Dinic; Goran Ferenc; Aleksandar Markovic; David S. Miller; Douglas Leung; Greg Kroah-Hartman; linux-kernel@xxxxxxxxxxxxxxx; Mauro Carvalho Chehab; Miodrag Dinic; Paul Burton; Paul Burton; Petar Jovanovic; Raghu Gandham; Ralf Baechle; Randy Dunlap
Subject: Re: [PATCH v6 5/5] MIPS: ranchu: Add Ranchu as a new generic-based board

On Mon, Oct 30, 2017 at 12:56:36PM +0100, Aleksandar Markovic wrote:
> From: Miodrag Dinic <miodrag.dinic@xxxxxxxx>
>
> Provide amendments to the MIPS generic platform framework so that
> the new generic-based board Ranchu can be chosen to be built.

A bit more info about the board would be good here. What boot protocol
is used? Does QEMU generate the DT dynamically?

>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> Signed-off-by: Goran Ferenc <goran.ferenc@xxxxxxxx>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@xxxxxxxx>
> ---
>  MAINTAINERS                                   |  6 ++
>  arch/mips/configs/generic/board-ranchu.config | 30 ++++++++++
>  arch/mips/generic/Kconfig                     | 10 ++++
>  arch/mips/generic/Makefile                    |  1 +
>  arch/mips/generic/board-ranchu.c              | 79 +++++++++++++++++++++++++++
>  5 files changed, 126 insertions(+)
>  create mode 100644 arch/mips/configs/generic/board-ranchu.config
>  create mode 100644 arch/mips/generic/board-ranchu.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f1be016..e429cc2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11308,6 +11308,12 @@ S:   Maintained
>  F:   Documentation/blockdev/ramdisk.txt
>  F:   drivers/block/brd.c
>
> +RANCHU VIRTUAL BOARD FOR MIPS
> +M:   Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> +L:   linux-mips@xxxxxxxxxxxxxx
> +S:   Supported
> +F:   arch/mips/generic/board-ranchu.c

Maybe worth adding arch/mips/configs/generic/board-ranchu.config too.

> +
>  RANDOM NUMBER DRIVER
>  M:   "Theodore Ts'o" <tytso@xxxxxxx>
>  S:   Maintained

> diff --git a/arch/mips/generic/Kconfig b/arch/mips/generic/Kconfig
> index e0436aa..93582be 100644
> --- a/arch/mips/generic/Kconfig
> +++ b/arch/mips/generic/Kconfig
> @@ -42,4 +42,14 @@ config FIT_IMAGE_FDT_NI169445
>         Enable this to include the FDT for the 169445 platform from
>         National Instruments in the FIT kernel image.
>
> +config VIRT_BOARD_RANCHU
> +     bool "Ranchu platform for Android emulator"
> +     help
> +       This enables support for the platform used by Android emulator.
> +
> +       Ranchu platform consists of a set of virtual devices. This platform
> +       enables emulation of variety of virtual configurations while using
> +       Android emulator. Android emulator is based on Qemu, and contains
> +       the support for the same set of virtual devices.

This is effectively in the section "FIT/UHI Boards", but it has a
platform file and no DT/FIT stuff in tree a bit like the boards in the
section "Legacy (non-UHI/non-FIT) Boards".

I'm guessing it might be something in between, with UHI + platform code,
but DT provided by QEMU (i.e. FIT support makes no sense)?

If it uses UHI I suppose it doesn't belong in the legacy section, but I
think a consistent prompt would be beneficial, e.g.

+config VIRT_BOARD_RANCHU
+       bool "Support Ranchu platform for Android emulator"
..

> diff --git a/arch/mips/generic/board-ranchu.c b/arch/mips/generic/board-ranchu.c
> new file mode 100644
> index 0000000..0397752
> --- /dev/null
> +++ b/arch/mips/generic/board-ranchu.c
> @@ -0,0 +1,79 @@
> +/*
> + * Support code for virtual Ranchu board for MIPS.
> + *
> + * Author: Miodrag Dinic <miodrag.dinic@xxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/of_address.h>
> +
> +#include <asm/machine.h>
> +#include <asm/time.h>
> +
> +#define GOLDFISH_TIMER_LOW           0x00
> +#define GOLDFISH_TIMER_HIGH          0x04
> +
> +static __init uint64_t read_rtc_time(void __iomem *base)
> +{
> +     u64 time_low;
> +     u64 time_high;
> +
> +     time_low = readl(base + GOLDFISH_TIMER_LOW);
> +     time_high = readl(base + GOLDFISH_TIMER_HIGH);
> +
> +     return (time_high << 32) | time_low;

What if high changes while reading this?

E.g.
TIMER_LOW        0x00000000 *0xffffffff*
TIMER_HIGH      *0x00000001* 0x00000000

You'd presumably get 0x00000001ffffffff.

Perhaps it should read HIGH before too, and retry if it has changed.

Cheers
James