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@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!_