Dear Dipen Dudhat,
In message <1295509580-28959-1-git-send-email-dipen.dud...@freescale.com> you
wrote:
>
> The Integrated Flash Controller (IFC) is used to access the external
> NAND Flash, NOR Flash, EPROM, SRAM and Generic ASIC memories.Four chip
> selects are provided in IFC so that maximum of four Flash devices can be
> hooked, but only one can be accessed at a given time.
...
> - GPCM Machine (Generic ASIC Mode)
> - Support for x8/16/32 bit device
> - Address and Data are shared on I/O bus
> - Following Address and Data sequences can be supported
> on I/O bus
> â 32 bit I/O: AD
> â 16 bit I/O: AADD
> â 8 bit I/O : AAAADDDD
Please use plain ASCII characters.
Checkpatch says:
ERROR: do not modify files in include/asm, change architecture specific files
in include/asm-<architecture>
#307: +++ b/arch/powerpc/include/asm/config.h
ERROR: do not modify files in include/asm, change architecture specific files
in include/asm-<architecture>
#323: +++ b/arch/powerpc/include/asm/fsl_ifc.h
ERROR: do not modify files in include/asm, change architecture specific files
in include/asm-<architecture>
#1257: +++ b/arch/powerpc/include/asm/fsl_law.h
ERROR: do not modify files in include/asm, change architecture specific files
in include/asm-<architecture>
#1269: +++ b/arch/powerpc/include/asm/immap_85xx.h
total: 4 errors, 0 warnings, 1084 lines checked
Please fix.
> +void print_ifc_regs(void)
> +{
> + int i, j;
> +
> + printf("\nIFC Controller Registers\n");
> + for (i = 0; i < FSL_IFC_BANK_COUNT; i++) {
> + printf("\nCSPR%d\t0x%08X\tAMASK%d\t0x%08X\tCSOR%d\t0x%08X\n",
> + i, get_ifc_cspr(i), i, get_ifc_amask(i),
> + i, get_ifc_csor(i));
> + for (j = 0; j < 4; j++)
> + printf("IFC_FTIM%d:0x%08X ", j, get_ifc_ftim(i, j));
> + }
> +}
Please note that printf formats starting with "\n..." are almost
always wrong or inappropriate. "\n" _terminates_ a line of text, it
does not start one.
Don't print useless blank lines.
> +/*
> + * Instruction opcodes to be programmed
> + * in FIR registers- 6bits
> + */
> +#define IFC_FIR_OP_NOP 0x00
> +#define IFC_FIR_OP_CA0 0x01
> +#define IFC_FIR_OP_CA1 0x02
> +#define IFC_FIR_OP_CA2 0x03
> +#define IFC_FIR_OP_CA3 0x04
> +#define IFC_FIR_OP_RA0 0x05
> +#define IFC_FIR_OP_RA1 0x06
> +#define IFC_FIR_OP_RA2 0x07
> +#define IFC_FIR_OP_RA3 0x08
> +#define IFC_FIR_OP_CMD0 0x09
> +#define IFC_FIR_OP_CMD1 0x0A
> +#define IFC_FIR_OP_CMD2 0x0B
> +#define IFC_FIR_OP_CMD3 0x0C
> +#define IFC_FIR_OP_CMD4 0x0D
> +#define IFC_FIR_OP_CMD5 0x0E
> +#define IFC_FIR_OP_CMD6 0x0F
> +#define IFC_FIR_OP_CMD7 0x10
> +#define IFC_FIR_OP_CW0 0x11
> +#define IFC_FIR_OP_CW1 0x12
> +#define IFC_FIR_OP_CW2 0x13
> +#define IFC_FIR_OP_CW3 0x14
> +#define IFC_FIR_OP_CW4 0x15
> +#define IFC_FIR_OP_CW5 0x16
> +#define IFC_FIR_OP_CW6 0x17
> +#define IFC_FIR_OP_CW7 0x18
> +#define IFC_FIR_OP_WBCD 0x19
> +#define IFC_FIR_OP_RBCD 0x1A
> +#define IFC_FIR_OP_BTRD 0x1B
> +#define IFC_FIR_OP_RDSTAT 0x1C
> +#define IFC_FIR_OP_NWAIT 0x1D
> +#define IFC_FIR_OP_WFR 0x1E
> +#define IFC_FIR_OP_SBRD 0x1F
> +#define IFC_FIR_OP_UA 0x20
> +#define IFC_FIR_OP_RB 0x21
Make this an enum.
> +/*
> + * ECC Error Status Registers (ECCSTAT0-ECCSTAT3)
> + */
> +/* Number of ECC errors on sector n (n = 0-15) */
> +#define IFC_NAND_ECCSTAT0_NUMBER0 0x0F000000
> +#define IFC_NAND_ECCSTAT0_NUMBER1 0x000F0000
> +#define IFC_NAND_ECCSTAT0_NUMBER2 0x00000F00
> +#define IFC_NAND_ECCSTAT0_NUMBER3 0x0000000F
> +#define IFC_NAND_ECCSTAT1_NUMBER4 0x0F000000
> +#define IFC_NAND_ECCSTAT1_NUMBER5 0x000F0000
> +#define IFC_NAND_ECCSTAT1_NUMBER6 0x00000F00
> +#define IFC_NAND_ECCSTAT1_NUMBER7 0x0000000F
> +#define IFC_NAND_ECCSTAT2_NUMBER8 0x0F000000
> +#define IFC_NAND_ECCSTAT2_NUMBER9 0x000F0000
> +#define IFC_NAND_ECCSTAT2_NUMBER10 0x00000F00
> +#define IFC_NAND_ECCSTAT2_NUMBER11 0x0000000F
> +#define IFC_NAND_ECCSTAT3_NUMBER12 0x0F000000
> +#define IFC_NAND_ECCSTAT3_NUMBER13 0x000F0000
> +#define IFC_NAND_ECCSTAT3_NUMBER14 0x00000F00
> +#define IFC_NAND_ECCSTAT3_NUMBER15 0x0000000F
Do you think these macros are really useful? If I understand your
intention, then the name is at least misleading - these appear to be
masks, not numbers. To get a number, you rpobably need both a mask
and a shift operation for most of these?
> +/*
> + * IFC Controller NAND Machine registers
> + */
> +struct fsl_ifc_nand {
> + u32 ncfgr;
> + u8 res17[0x10];
> + u32 nand_fcr0;
> + u32 nand_fcr1;
> + u8 res18[0x20];
> + u32 row0;
> + u8 res19[0x4];
> + u32 col0;
> + u8 res20[0x4];
> + u32 row1;
> + u8 res21[0x4];
> + u32 col1;
> + u8 res22[0x4];
> + u32 row2;
> + u8 res23[0x4];
> + u32 col2;
> + u8 res24[0x4];
> + u32 row3;
> + u8 res25[0x4];
> + u32 col3;
> + u8 res26[0x90];
> + u32 nand_fbcr;
> + u8 res27[0x4];
> + u32 nand_fir0;
> + u32 nand_fir1;
> + u32 nand_fir2;
> + u8 res28[0x40];
> + u32 nand_csel;
> + u8 res29[0x4];
> + u32 nandseq_strt;
> + u8 res30[0x4];
> + u32 nand_evter_stat;
> + u8 res31[0x4];
> + u32 pgrdcmpl_evt_stat;
> + u8 res32[0x8];
> + u32 nand_evter_en;
> + u8 res33[0x8];
> + u32 nand_evter_intr_en;
> + u8 res34[0x8];
> + u32 nand_erattr0;
> + u32 nand_erattr1;
> + u8 res35[0x40];
> + u32 nand_fsr;
> + u8 res36[0x4];
> + u32 nand_eccstat0;
> + u32 nand_eccstat1;
> + u32 nand_eccstat2;
> + u32 nand_eccstat3;
> + u8 res37[0x80];
> + u32 nanndcr;
> + u8 res38[0x8];
> + u32 nand_autoboot_trgr;
> + u8 res39[0x4];
> + u32 nand_mdr;
> + u8 res40[0x170];
> +};
Why do you use u8 for all the served entries when the existing
entries are all u32?
Ditto for the following structs.
Also note that code would be better readable if the variable names
were vertically aligned (which comes for free when you convert the
reserved entries to u32).
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: w...@denx.de
Every living thing wants to survive.
-- Spock, "The Ultimate Computer", stardate 4731.3
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
