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

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:06 Fri 04 Jul     , Maxime Ripard wrote:
> 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...

inline is horrible to read and maintain NACK

keep it in an assembly file it's so easy to read and follow

and you just have to move the file existing to the driver/power

Best Regards,
J.
> 
> > 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


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