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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171101175820.nhepxzdwfokof6q2@pburton-laptop>
Date:   Wed, 1 Nov 2017 10:58:20 -0700
From:   Paul Burton <paul.burton@...s.com>
To:     Aleksandar Markovic <aleksandar.markovic@...rk.com>
CC:     <linux-mips@...ux-mips.org>,
        Miodrag Dinic <miodrag.dinic@...s.com>,
        "Goran Ferenc" <goran.ferenc@...s.com>,
        Aleksandar Markovic <aleksandar.markovic@...s.com>,
        "David S. Miller" <davem@...emloft.net>,
        Douglas Leung <douglas.leung@...s.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        James Hogan <james.hogan@...s.com>,
        <linux-kernel@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Miodrag Dinic <miodrag.dinic@...tec.com>,
        Paul Burton <paul.burton@...tec.com>,
        Petar Jovanovic <petar.jovanovic@...s.com>,
        "Raghu Gandham" <raghu.gandham@...s.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        "Randy Dunlap" <rdunlap@...radead.org>
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@...s.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