From mboxrd@z Thu Jan  1 00:00:00 1970
From: Angelo Dureghello <sysamfw@gmail.com>
Date: Mon, 5 Nov 2012 21:52:21 +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: <20121105205220.GA24682@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,

i have still a question here, please see below.

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
> 
> 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?
> 
> 
> > +#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.
> 
> ...
> > +/*
> > + *	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.
> 


I used this approach looking all other coldfire cpu files, and this notation
seems accepted.
I started m5307.h looking those files (i.e. m5249.h) maintaing the same scheme.

Let me know how i have to proceed.

Best Regards,
Angelo Dureghello