From mboxrd@z Thu Jan  1 00:00:00 1970
From: Angelo Dureghello <sysamfw@gmail.com>
Date: Mon, 5 Nov 2012 15:50:36 +0100
Subject: [U-Boot] [PATCH v3 1/2] m68k: add support for mcf5307 cpu
In-Reply-To: <20121104215014.7A395200257@gemini.denx.de>
References: <20121104195901.GA5141@angel3>
	<20121104215014.7A395200257@gemini.denx.de>
Message-ID: <20121105145034.GA7576@angel3>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Wolfgang,

thanks for reviewing.
See my comments blow.


On Sun, Nov 04, 2012 at 10:50:14PM +0100, Wolfgang Denk wrote:
> Dear angelo,
> 
> In message <20121104195901.GA5141@angel3> you wrote:
> > Add support for freescale coldfire mcf5307 cpu.
> ...
> > --- /dev/null
> > +++ b/arch/m68k/cpu/mcf530x/cpu_init.c
> ...
> > +#define MCF5307_SP_ERR_FIX(cs_base,mask) \
> > +        if((cs_base+(mask&0xffff0000))>=0xffff0000)mask|=0x20
> 

Done.

> Please never do this.  Please ALWAYS use the standard "do { ... }
> while (0)" construct to make sure such macros can be used savely in
> any call envionment.
> 
> > +void init_csm(void)
> > +{
> > +	volatile csm_t *csm = (csm_t *) (MMAP_CSM);
> 
> NAK.  Please so not use volatile.  Hey, did you run your ptches
> through checkpatch?  What did it say?

Ok, i forgot pass through checkpatch, will do it always.
 
> 
> > +#if (defined(CONFIG_SYS_CS0_BASE) && defined(CONFIG_SYS_CS0_MASK) \
> > +     && defined(CONFIG_SYS_CS0_CTRL))
> > +	csm->csar0 = (CONFIG_SYS_CS0_BASE>>16);
> > +	csm->cscr0 = CONFIG_SYS_CS0_CTRL;
> > +	csm->csmr0 = CONFIG_SYS_CS0_MASK;
> > +	MCF5307_SP_ERR_FIX(CONFIG_SYS_CS0_BASE,csm->csmr0);
> 
> We do not allow accesses to device registers through plain (even
> volatile) pointers.  Please make sure to use proper I/O accessors
> instead.

Done.

> ...
> > +/*
> > + *	Define the 5249 SIM register set addresses.
> > + */
> > +
> > +/*
> > + ***** MBAR  *****
> > + */
> > +#define MCFSIM_RSR		0x00	/* Reset Status reg (r/w) */
> > +#define MCFSIM_SYPCR		0x01	/* System Protection reg (r/w) */
> > +#define MCFSIM_SWIVR		0x02	/* SW Watchdog intr reg (r/w) */
> > +#define MCFSIM_SWSR		0x03	/* SW Watchdog service (r/w) */
> > +#define MCFSIM_PAR		0x04    /* Parallel pin assignment reg */
> > +#define MCFSIM_PLLCR		0x08	/* PLL Control register */
> > +#define MCFSIM_MPARK		0x0c	/* Bus master park register (r/w) */
> > +
> > +#define MCFSIM_SIMR		0x00	/* SIM Config reg (r/w) */
> > +#define MCFSIM_ICR0		0x4c	/* Intr Ctrl reg 0 (r/w) */
> > +#define MCFSIM_ICR1		0x4d	/* Intr Ctrl reg 1 (r/w) */
> > +#define MCFSIM_ICR2		0x4e	/* Intr Ctrl reg 2 (r/w) */
> > +#define MCFSIM_ICR3		0x4f	/* Intr Ctrl reg 3 (r/w) */
> > +#define MCFSIM_ICR4		0x50	/* Intr Ctrl reg 4 (r/w) */
> > +#define MCFSIM_ICR5		0x51	/* Intr Ctrl reg 5 (r/w) */
> > +#define MCFSIM_ICR6		0x52	/* Intr Ctrl reg 6 (r/w) */
> > +#define MCFSIM_ICR7		0x53	/* Intr Ctrl reg 7 (r/w) */
> > +#define MCFSIM_ICR8		0x54	/* Intr Ctrl reg 8 (r/w) */
> > +#define MCFSIM_ICR9		0x55	/* Intr Ctrl reg 9 (r/w) */
> > +#define MCFSIM_ICR10		0x56	/* Intr Ctrl reg 10 (r/w) */
> > +#define MCFSIM_ICR11		0x57	/* Intr Ctrl reg 11 (r/w) */
> > +
> > +#define MCFSIM_IPR		0x40	/* Interrupt Pend reg (r/w) */
> > +#define MCFSIM_IMR		0x44	/* Interrupt Mask reg (r/w) */
> > +
> > +#define MCFSIM_DCR		0x100	/* DRAM Control reg (r/w) */
> > +#define MCFSIM_DACR0		0x108	/* DRAM 0 Addr and Ctrl (r/w) */
> > +#define MCFSIM_DMR0		0x10c	/* DRAM 0 Mask reg (r/w) */
> > +#define MCFSIM_DACR1		0x110	/* DRAM 1 Addr and Ctrl (r/w) */
> > +#define MCFSIM_DMR1		0x114	/* DRAM 1 Mask reg (r/w) */
> > +
> > +#define MCFSIM_PADDR		0x244   /* Parallel data direction reg */
> > +#define MCFSIM_PADAT		0x248   /* Parallel data direction reg */
> 
> We don't allow any base address + offset notation. Please define a
> proper C structure instead.
> 
> Please fix globally.

Clear, will fix it.
> 
> 
> > -#if defined(CONFIG_M5249) || defined(CONFIG_M5253) || defined(CONFIG_M5272)
> > +#if defined(CONFIG_M5307) || defined(CONFIG_M5249) || \
> > +    defined(CONFIG_M5253) || defined(CONFIG_M5272)
> 
> Please keep the list sorted.
> 

Done.

> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> I believe you find life such a problem because you  think  there  are
> the  good  people  and the bad people. You're wrong, of course. There
> are, always and only, the bad people, but some of them are  on  oppo-
> site sides.                      - Terry Pratchett, _Guards! Guards!_