linux-kernel - Re: [PATCH 9/9] ARM: pxa27x: device tree support ICP DAS LP-8x4x

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]

Date:	Mon, 9 Dec 2013 02:47:24 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Sergei Ianovich <ynvich@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Daniel Mack <zonque@...il.com>,
	Olof Johansson <olof@...om.net>,
	Linus Walleij <linus.walleij@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Russell King <linux@....linux.org.uk>,
	Eric Miao <eric.y.miao@...il.com>,
	Haojian Zhuang <haojian.zhuang@...il.com>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH 9/9] ARM: pxa27x: device tree support ICP DAS LP-8x4x

On Sunday 08 December 2013, Sergei Ianovich wrote:
> +
> +#ifdef CONFIG_PXA27x
> +extern void __init pxa27x_dt_init_irq(void);

This is not the right place to put an 'extern' declaration, it should go into
a header file if it's really needed. Ideally you would not need it at all
and just add an IRQCHIP_DECLARE() line into the driver to automatically
probe it.

> +static const struct of_dev_auxdata pxa27x_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("mrvl,pxa-uart",		0x40100000, "pxa2xx-uart.0", NULL),
> +	OF_DEV_AUXDATA("mrvl,pxa-uart",		0x40200000, "pxa2xx-uart.1", NULL),
> +	OF_DEV_AUXDATA("mrvl,pxa-uart",		0x40700000, "pxa2xx-uart.2", NULL),
> +	OF_DEV_AUXDATA("mrvl,pxa-uart",		0x41600000, "pxa2xx-uart.3", NULL),
> +	OF_DEV_AUXDATA("marvell,pxa-mmc",	0x41100000, "pxa2xx-mci.0", NULL),
> +	OF_DEV_AUXDATA("intel,pxa27x-gpio",	0x40e00000, "pxa27x-gpio", NULL),
> +	OF_DEV_AUXDATA("marvell,pxa-ohci",	0x4c000000, "pxa27x-ohci", NULL),
> +	OF_DEV_AUXDATA("mrvl,pxa-i2c",		0x40301680, "pxa2xx-i2c.0", NULL),
> +	{}
> +};

I guess these are needed only for clock management purposes at the moment?

> +static void __init pxa27x_init_early(void)
> +{
> +	pxa27x_skip_init();
> +}

I would prefer not to have an init_early function at all, and instead
check "if (of_have_populated_dt())" in pxa27x_init, or to split
that function into two.

> +static const char *pxa27x_dt_board_compat[] __initdata = {
> +	"marvell,pxa27x",
> +	NULL,
> +};
> +
> +#ifdef CONFIG_MACH_LP8X4X
> +static const char *lp8x4x_dt_board_compat[] __initdata = {
> +	"marvell,lp8x4x",
> +	NULL,
> +};

Note that you should not have wildcards in any "compatible" strings.
Just list all the combinations here (pxa270, pxa271, pxa272, and whatever
you need for lp8x4x).

> +static void lp8x4x_restart(enum reboot_mode mode, const char *cmd)
> +{
> +	/* Switch off fast-bus and turbo mode */
> +	asm volatile("mcr       p14, 0, %0, c6, c0, 0" : :
> +			"r"(2));
> +	/* SDRAM hangs on watchdog reset on Marvell PXA270 (erratum 71) */
> +	pxa_restart(REBOOT_SOFT, cmd);
> +}
> +#endif
> +#endif

Since the only difference here is the restart logic, I would prefer here to
have only a single DT_MACHINE_START() and do

static void pxa27x_restart(enum reboot_mode mode, const char *cmd)
{
	/* Switch off fast-bus and turbo mode */
	if (of_machine_is_compatible("marvell,lp8x4x"))
		asm volatile("mcr       p14, 0, %0, c6, c0, 0" : : "r"(2));

	/* SDRAM hangs on watchdog reset on Marvell PXA270 (erratum 71) */
	if (of_machine_is_compatible("marvell,pxa270"))
		pxa_restart(REBOOT_SOFT, cmd);
}

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/