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

[James Hogan]

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