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 [[email protected]]
Sent: Wednesday, November 1, 2017 6:58 PM
To: Aleksandar Markovic
Cc: [email protected]; Miodrag Dinic; Goran Ferenc; Aleksandar Markovic; David S. Miller; Douglas Leung; Greg Kroah-Hartman; James Hogan; [email protected]; 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 <[email protected]>
> + *
> + * 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
From 1582959043828813778@xxx Thu Nov 02 12:56:24 +0000 2017
X-GM-THRID: 1582683942030413790
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread