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

[Miodrag Dinic <Miodrag.Dinic@mips.com>]

Hi Paul,

thank you for the review. Please find the answers in-lined:

> > +#include <linux/of_address.h>
> > +
> > +#include <asm/machine.h>
> 
> You should also include asm/mipsregs.h for read_c0_count(), even though it's
> presumably being pulled in indirectly as-is.

Thanks, we will address this in V7.

> > +#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;
> > +}
> > +
> > +static __init unsigned int ranchu_measure_hpt_freq(void)
> > +{
> > +     u64 rtc_start, rtc_current, rtc_delta;
> > +     unsigned int start, count;
> > +     struct device_node *np;
> > +     void __iomem *rtc_base;
> > +
> > +     np = of_find_compatible_node(NULL, NULL, "google,goldfish-rtc");
> > +     if (!np)
> > +             panic("%s(): Failed to find 'google,goldfish-rtc' dt node!",
> > +                   __func__);
> > +
> > +     rtc_base = of_iomap(np, 0);
> > +     if (!rtc_base)
> > +             panic("%s(): Failed to ioremap Goldfish RTC base!", __func__);
> > +
> > +     /*
> > +      * poll the nanosecond resolution RTC for 1 second
> > +      * to calibrate the CPU frequency
> > +      */
> > +     rtc_start = read_rtc_time(rtc_base);
> > +     start = read_c0_count();
> > +
> > +     do {
> > +             rtc_current = read_rtc_time(rtc_base);
> > +             rtc_delta = rtc_current - rtc_start;
> > +     } while (rtc_delta < NSEC_PER_SEC);
> > +
> > +     count = read_c0_count() - start;
> > +
> > +     count += 5000;  /* round */
> > +     count -= count % 10000;
> > +
> > +     return count;
> > +}
> 
> Would it be possible to have the emulator write the frequency into the
> devicetree, as the frequency of a fixed-clock used as the CPU's clock? If that
> were possible then there'd be no need for this board setup code at all. Not a
> big deal, but it'd be nice.

Well, yes I think that would be possible, but since this will be run on the emulator,
fixed-clock may not be the best because the point of this code is to make emulator
calibrate itself according to speed of the host which runs the emulation.
We may consider more advanced approach on this issue in the future, but for now
this works fine for us.

> > +
> > +static const struct of_device_id ranchu_of_match[];
> > +
> > +MIPS_MACHINE(ranchu) = {
> > +     .matches = ranchu_of_match,
> > +     .measure_hpt_freq = ranchu_measure_hpt_freq,
> > +};
> > +
> > +static const struct of_device_id ranchu_of_match[] = {
> > +     {
> > +             .compatible = "mti,ranchu",
> > +             .data = &__mips_mach_ranchu,
> > +     },
> > +};
> 
> Could you move ranchu_of_match before the MIPS_MACHINE & drop the forward
> declaration? That would feel tidier to me. It could also be marked as
> __initdata.

We can not remove the forward declaration because we need to define
__mips_mach_ranchu (which is done by MIPS_MACHINE(ranchu)) before ranchu_of_match.

Kind regards,
Miodrag

________________________________________
From: Paul Burton [paul.burton@mips.com]
Sent: Wednesday, November 1, 2017 6:58 PM
To: Aleksandar Markovic
Cc: linux-mips@linux-mips.org; Miodrag Dinic; Goran Ferenc; Aleksandar Markovic; David S. Miller; Douglas Leung; Greg Kroah-Hartman; James Hogan; linux-kernel@vger.kernel.org; Mauro Carvalho Chehab; Miodrag Dinic; 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

Hi Aleksandar,

On Mon, Oct 30, 2017 at 12:56:36PM +0100, Aleksandar Markovic wrote:
> 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@mips.com>
> + *
> + * 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>

You should also include asm/mipsregs.h for read_c0_count(), even though it's
presumably being pulled in indirectly as-is.

> +#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;
> +}
> +
> +static __init unsigned int ranchu_measure_hpt_freq(void)
> +{
> +     u64 rtc_start, rtc_current, rtc_delta;
> +     unsigned int start, count;
> +     struct device_node *np;
> +     void __iomem *rtc_base;
> +
> +     np = of_find_compatible_node(NULL, NULL, "google,goldfish-rtc");
> +     if (!np)
> +             panic("%s(): Failed to find 'google,goldfish-rtc' dt node!",
> +                   __func__);
> +
> +     rtc_base = of_iomap(np, 0);
> +     if (!rtc_base)
> +             panic("%s(): Failed to ioremap Goldfish RTC base!", __func__);
> +
> +     /*
> +      * poll the nanosecond resolution RTC for 1 second
> +      * to calibrate the CPU frequency
> +      */
> +     rtc_start = read_rtc_time(rtc_base);
> +     start = read_c0_count();
> +
> +     do {
> +             rtc_current = read_rtc_time(rtc_base);
> +             rtc_delta = rtc_current - rtc_start;
> +     } while (rtc_delta < NSEC_PER_SEC);
> +
> +     count = read_c0_count() - start;
> +
> +     count += 5000;  /* round */
> +     count -= count % 10000;
> +
> +     return count;
> +}

Would it be possible to have the emulator write the frequency into the
devicetree, as the frequency of a fixed-clock used as the CPU's clock? If that
were possible then there'd be no need for this board setup code at all. Not a
big deal, but it'd be nice.

> +
> +static const struct of_device_id ranchu_of_match[];
> +
> +MIPS_MACHINE(ranchu) = {
> +     .matches = ranchu_of_match,
> +     .measure_hpt_freq = ranchu_measure_hpt_freq,
> +};
> +
> +static const struct of_device_id ranchu_of_match[] = {
> +     {
> +             .compatible = "mti,ranchu",
> +             .data = &__mips_mach_ranchu,
> +     },
> +};

Could you move ranchu_of_match before the MIPS_MACHINE & drop the forward
declaration? That would feel tidier to me. It could also be marked as
__initdata.

In general though, with those & James' comments addressed, I think this is
looking good.

Thanks,
    Paul