Re: [PATCH 05/18] power: reset: Add AT91 reset driver

[Maxime Ripard]

On Fri, Jul 04, 2014 at 09:14:43AM +0200, Boris BREZILLON wrote:
> On Fri, 4 Jul 2014 11:08:20 +0800
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx> wrote:
> 
> > 
> > On Jul 3, 2014, at 10:59 PM, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > On Thu, Jul 03, 2014 at 10:39:08PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>> +++ b/drivers/power/reset/at91-reset.c
> > >>> @@ -0,0 +1,202 @@
> > >>> +/*
> > >>> + * Atmel AT91 SAM9 SoCs reset code
> > >>> + *
> > >>> + * Copyright (C) 2014 Maxime Ripard
> > >>> + *
> > >>> + * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> > >> 
> > >> you can not own the copyright as itâs basically a copy of other
> > >> people code
> > > 
> > > The previous names are missing, right.
> > > 
> > >>> + *
> > >>> + * This file is licensed under the terms of the GNU General Public
> > >>> + * License version 2.  This program is licensed "as is" without any
> > >>> + * warranty of any kind, whether express or implied.
> > >>> + */
> > >>> +
> > >>> +#include <linux/io.h>
> > >>> +#include <linux/module.h>
> > >>> +#include <linux/of_address.h>
> > >>> +#include <linux/platform_device.h>
> > >>> +#include <linux/reboot.h>
> > >>> +
> > >>> +#include <asm/system_misc.h>
> > >>> +
> > >>> +#include <mach/at91sam9_ddrsdr.h>
> > >>> +#include <mach/at91sam9_sdramc.h>
> > >>> +
> > >>> +#define AT91_RSTC_CR	0x00		/* Reset Controller Control Register */
> > >>> +#define	AT91_RSTC_PROCRST	BIT(0)		/* Processor Reset */
> > >>> +#define	AT91_RSTC_PERRST	BIT(2)		/* Peripheral Reset */
> > >>> +#define	AT91_RSTC_EXTRST	BIT(3)		/* External Reset */
> > >>> +#define	AT91_RSTC_KEY		(0xa5 << 24)	/* KEY Password */
> > >>> +
> > >>> +#define AT91_RSTC_SR	0x04		/* Reset Controller Status Register */
> > >>> +#define	AT91_RSTC_URSTS		BIT(0)		/* User Reset Status */
> > >>> +#define	AT91_RSTC_RSTTYP	GENMASK(10, 8)	/* Reset Type */
> > >>> +#define	AT91_RSTC_NRSTL		BIT(16)		/* NRST Pin Level */
> > >>> +#define	AT91_RSTC_SRCMP		BIT(17)		/* Software Reset Command in Progress */
> > >>> +
> > >>> +#define AT91_RSTC_MR	0x08		/* Reset Controller Mode Register */
> > >>> +#define	AT91_RSTC_URSTEN	BIT(0)		/* User Reset Enable */
> > >>> +#define	AT91_RSTC_URSTIEN	BIT(4)		/* User Reset Interrupt Enable */
> > >>> +#define	AT91_RSTC_ERSTL		GENMASK(11, 8)	/* External Reset Length */
> > >>> +
> > >>> +enum reset_type {
> > >>> +	RESET_TYPE_GENERAL	= 0,
> > >>> +	RESET_TYPE_WAKEUP	= 1,
> > >>> +	RESET_TYPE_WATCHDOG	= 2,
> > >>> +	RESET_TYPE_SOFTWARE	= 3,
> > >>> +	RESET_TYPE_USER		= 4,
> > >>> +};
> > >>> +
> > >>> +static void __iomem *at91_ramc_base[2], *at91_rstc_base;
> > >>> +
> > >>> +static void at91sam9_restart(enum reboot_mode mode, const char *cmd)
> > >>> +{
> > >>> +	asm volatile(
> > >>> +		".balign 32\n\t"
> > >>> +
> > >>> +		"str	%2, [%0, #" __stringify(AT91_SDRAMC_TR) "]\n\t"
> > >>> +		"str	%3, [%0, #" __stringify(AT91_SDRAMC_LPR) "]\n\t"
> > >>> +		"str	%4, [%1, #" __stringify(AT91_RSTC_CR) "]\n\t"
> > >>> +
> > >>> +		"b	.\n\t"
> > >>> +		:
> > >>> +		: "r" (at91_ramc_base[0]),
> > >>> +		  "r" (at91_rstc_base),
> > >>> +		  "r" (1),
> > >>> +		  "r" (AT91_SDRAMC_LPCB_POWER_DOWN),
> > >>> +		  "r" (AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST));
> > >>> +}
> > >>> +
> > >>> +static void at91sam9g45_restart(enum reboot_mode mode, const char *cmd)
> > >>> +{
> > >>> +	asm volatile(
> > >>> +		"cmp	%1, #0\n\t"
> > >>> +		"beq	1f\n\t"
> > >>> +
> > >>> +		"ldr	r0, [%1]\n\t"
> > >>> +		"cmp	r0, #0\n\t"
> > >>> +
> > >>> +		".balign 32\n\t"
> > >>> +
> > >>> +		"1:	str	%3, [%0, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > >>> +		"	str	%4, [%0, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > >>> +		"	strne	%3, [%1, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > >>> +		"	strne	%4, [%1, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > >>> +		"	str	%5, [%2, #" __stringify(AT91_RSTC_CR) "]\n\t"
> > >>> +
> > >>> +		"	b	.\n\t"
> > >>> +		:
> > >>> +		: "r" (at91_ramc_base[0]),
> > >>> +		  "r" (at91_ramc_base[1]),
> > >>> +		  "r" (at91_rstc_base),
> > >>> +		  "r" (1),
> > >>> +		  "r" (AT91_DDRSDRC_LPCB_POWER_DOWN),
> > >>> +		  "r" (AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST)
> > >>> +		: "r0");
> > >>> +}
> > >>> +
> > >> move this to an assembly file more easy to read than a C code
> > > 
> > > Nope. It's a pain to pass variable to an external assembly file, and
> > > this makes the use of global variable pretty much mandatory, which is
> > > definitely not great.
> > 
> > Not at all I did for the PM slow clock code just write a function and pas it as a parameter
> > you have 3
> > 
> > so basically you have to use the current and just pass at91_ramc_base[0], at91_ramc_base[1]
> > and at91_rstc_base
> > itâs 3 lignes of modification, if you have at91_ramc_base and at91_ramc_base same
> > 
> 
> Yes, retrieving function parameters from assembly code is not that
> complicated (the first 4 pointer values are accessible through r0-r3),
> but then you'll have to store your assembly file somewhere.

Like I was saying, there's a strong preference for the inline
assembly...

> Subsystem directories (drivers/xxx) are supposed to be architecture
> agnostics, and I'm not sure subsystem maintainers will accept these
> assembly files in their directory.

... and I've told that some maintainers don't even want assembly files
in their maintainance area.

> ITOH, leaving these assembly files in arch/arm/mach-at91 will just
> spread the code over linux src tree and will definitely not help other
> people find out the relationship between these files.

Given that the end-goal is to remove most (if not all) of mach-at91,
it seems a bit backward to me.

> Regarding the readability concern, I think some comments could help
> understanding what's being done here.

Yep, I have been a bit too eager to send the patches, and forgot to
reintroduce the comments that were there in the first place.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachment: signature.asc
Description: Digital signature