[U-Boot] [PATCH v3 1/2] m68k: add support for mcf5307 cpu
Angelo Dureghello
sysamfw at gmail.com
Mon Nov 5 21:52:21 CET 2012
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 at 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
More information about the U-Boot
mailing list