[U-Boot] [PATCH 7/7] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices
Moffett, Kyle D
Kyle.D.Moffett at boeing.com
Mon Feb 21 23:12:08 CET 2011
On Feb 21, 2011, at 16:47, Wolfgang Denk wrote:
> In message <1298311199-18775-8-git-send-email-Kyle.D.Moffett at boeing.com> you wrote:
>> The eXMeritus HWW-1U-1A unit is a DO-160-certified 13lb 1U chassis
>> with 3 independent TEMPEST zones. Two independent P2020 computers may
>> be found inside each zone. Complete hardware support is included.
> ...
>
> There are a number of formal issues - please run checkpatch:
> total: 12 errors, 12 warnings, 1443 lines checked
>
> ERROR: do not initialise statics to 0 or NULL
> ERROR: space prohibited after that open parenthesis '('
> ERROR: space prohibited before that close parenthesis ')'
> WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> WARNING: externs should be avoided in .c files
> WARNING: please, no spaces at the start of a line
>
> Please fix.
Ok, will fix. Most of the "volatile" problems were copied from the P2020DS board and other MPC85xx boards, those are definitely wrong there too.
> ...
>> + puts(" Waiting 1 sec for reset...");
>> + for (i = 0; i < 10; i++) {
>> + udelay(100000);
>> + puts(".");
>> + }
>
> Do you really need a full second here?
Probably not, but this code only ever executes when a JTAG debugger has been poking around already (the pins are always low after CPU reset), so I'd prefer not to risk breaking it unless absolutely necessary.
> ...
>> +U_BOOT_CMD(
>> + hww1u1a_is_cpu_a, 1, 0, do_hww1u1a_is_cpu_a,
>> + "Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
>> + /* */" && echo This is CPU A\n"
>> + "if hww1u1a_is_cpu_a; then\n"
>> + " ## Do stuff\n"
>> + "else\n"
>> + " ## Do other stuff\n"
>> + "fi"
>
> Please don't misuse the help messages like that. Use external
> documentation instead.
Ok, is just " && echo This is CPU A\n" OK? I'm trying to show possible usage of the command and it has no actual arguments or output on its own.
>> +#ifndef CONFIG_DDR_SPD
>> +phys_size_t fixed_sdram(void)
>> +{
>> + /* This is manual (non-SPD) DDR2 SDRAM configuration (2GB ECC) */
>> + volatile ccsr_ddr_t *ddr = (ccsr_ddr_t *)CONFIG_SYS_MPC85xx_DDR_ADDR;
>> + phys_size_t dram_size = (2048ULL) << 20;
>> +
>> + /*
>> + * First program the DDR registers, then allow time for the SDRAM
>> + * configuration and clocks to settle before enabling everything.
>> + */
>> + out_be32(&ddr->cs0_bnds, 0x0000003F);
>> + out_be32(&ddr->cs1_bnds, 0x0040007F);
>> + out_be32(&ddr->cs0_config, 0x80014202);
>> + out_be32(&ddr->cs1_config, 0x80014202);
>> + out_be32(&ddr->timing_cfg_0, 0x00330804);
>> + out_be32(&ddr->timing_cfg_1, 0x6f69f643);
>> + out_be32(&ddr->timing_cfg_2, 0x022068cd);
>> + out_be32(&ddr->timing_cfg_3, 0x00030000);
>> + out_be32(&ddr->sdram_cfg_2, 0x04400010);
>> + out_be32(&ddr->sdram_mode, 0x00440452);
>> + out_be32(&ddr->sdram_mode_2, 0x00000000);
>> + out_be32(&ddr->sdram_md_cntl, 0x00000000);
>> + out_be32(&ddr->sdram_interval, 0x0a280110);
>> + out_be32(&ddr->sdram_data_init, 0xDEADBEEF);
>> + out_be32(&ddr->sdram_clk_cntl, 0x02000000);
>> + out_be32(&ddr->timing_cfg_4, 0x00000000);
>> + out_be32(&ddr->timing_cfg_5, 0x00000000);
>> + out_be32(&ddr->ddr_zq_cntl, 0x00000000);
>> + out_be32(&ddr->ddr_wrlvl_cntl, 0x00000000);
>> + out_be32(&ddr->ip_rev1, 0x00020403);
>> + out_be32(&ddr->ip_rev2, 0x00000100);
>> + out_be32(&ddr->err_int_en, 0x0000000D);
>> + out_be32(&ddr->err_disable, 0x00000000);
>> + out_be32(&ddr->err_sbe, 0x00010000);
>> + out_be32(&ddr->ddr_cdr1, 0x00040000);
>> + out_be32(&ddr->ddr_cdr2, 0x00000000);
>> + out_be32(&ddr->sdram_cfg, 0x73000000);
>> + udelay(500);
>> + out_be32(&ddr->sdram_cfg, 0xF3000000);
>> +
>> + /* Now wait until memory is initialized */
>> + while (in_be32(&ddr->sdram_cfg_2) & 0x00000010)
>> + udelay(1000);
>
> Please avoid all these magic constants and use symbolic names instead
> like other boards are doing.
Hum.
The only difference between this code and P2020DS is that the P2020DS development board has:
include/configs/P2020DS.h:
#define CONFIG_SYS_DDR_CS0_BNDS 0x0000003F
#define CONFIG_SYS_DDR_CS1_BNDS 0x00000000
[...]
board/freescale/p2020ds/p2020ds.c:
volatile ccsr_ddr_t *ddr = (ccsr_ddr_t *)CONFIG_SYS_MPC85xx_DDR_ADDR;
ddr->cs0_config = CONFIG_SYS_DDR_CS0_CONFIG;
ddr->cs1_config = CONFIG_SYS_DDR_CS1_CONFIG;
[...]
I don't see how using a bunch of #defines with the exact same names as the CCSR DDR registers makes the code any more readable.
I only use this code for debugging so if the numeric constants are completely unacceptable I will just delete it rather than try to rewrite it to make everybody happy.
> ...
>> diff --git a/include/configs/HWW1U1A.h b/include/configs/HWW1U1A.h
>> new file mode 100644
>> index 0000000..cb8664e
>> --- /dev/null
>> +++ b/include/configs/HWW1U1A.h
> ...
>> +/************************************************************************
>> + * High-level system configuration options *
>> + ************************************************************************/
>
> Incorrect multiline comment style. Please fix globally.
Hmm, the intent was to group config options into "sections" of a sort (such as I2C, PCI, networking, etc).
Is there a better syntax for me to use or am I not allowed to do that?
>> +/*#define DEBUG 1*/
>
> Please remove dead code.
Ok.
>> +#define CONFIG_BOOKE 1 /* Power/PowerPC Book-E */
>> +#define CONFIG_E500 1 /* e500 (Power ISA v2.03 with SPE) */
>> +#define CONFIG_MPC85xx 1 /* MPC8540/60/55/41/48 family */
>> +#define CONFIG_FSL_ELBC 1 /* FreeScale Enhanced LocalBus Cntlr */
>> +#define CONFIG_FSL_LAW 1 /* FreeScale Local Access Window */
>> +#define CONFIG_P2020 1 /* FreeScale P2020 */
>> +#define CONFIG_HWW1U1A 1 /* eXMeritus HardwareWall HWW-1U-1A */
>> +#define CONFIG_MP 1 /* Multiprocessing support */
>> +#define CONFIG_HWCONFIG 1 /* Use hwconfig from environment */
>> +
>> +#define CONFIG_L2_CACHE 1 /* L2 cache enabled */
>> +#define CONFIG_BTB 1 /* Branch predition enabled */
>> +
>> +#define CONFIG_PANIC_HANG 1 /* No board reset on panic */
>> +#define CONFIG_BOARD_EARLY_INIT_R 1 /* Call board_early_init_r() */
>> +#define CONFIG_BOARD_RESET_R 1 /* Call board_reset_r() */
>> +#define CONFIG_CMD_REGINFO 1 /* Dump various CPU regs */
>
> Please remove the "1" for all macros that are actually used with
> "#ifdef"s. Please fix globally.
I added all of the "1"s after I experienced random build breakage from a couple config variables being "#ifdef" in one place and "#if" in another. Is there a convenient list somewhere of which should be which?
>> +#undef CONFIG_CLOCKS_IN_MHZ
>
> Please don't undef what is not defined anyway.
Copied from P2020DS, deleted.
>> +#define CONFIG_ENV_SECT_SIZE 0x20000 /* 128kB (1 flash sector) */
> ...
>> +#define CONFIG_SYS_PCIE3_MEM_SIZE 0x20000000 /* 512M */
>> +#define CONFIG_SYS_PCIE2_MEM_SIZE 0x20000000 /* 512M */
>> +#define CONFIG_SYS_PCIE1_MEM_SIZE 0x20000000 /* 512M */
>> +#define CONFIG_SYS_PCIE3_IO_SIZE 0x00010000 /* 64k */
>> +#define CONFIG_SYS_PCIE2_IO_SIZE 0x00010000 /* 64k */
>> +#define CONFIG_SYS_PCIE1_IO_SIZE 0x00010000 /* 64k */
>
> If you write such constants as "(128 << 10)", "(512 << 20)" resp.
> "(64 << 10)" they may become better readable.
>
>> + ************************************************************************/
>> +#define CONFIG_ENV_SIZE 0x02000 /* 8kB */
>> +#define CONFIG_ENV_SIZE_REDUND 0x02000 /* 8kB */
>
> Ditto.
This was also copied from P2020DS.
These are address-space sizes next to the actual base addresses, so I would rather leave them in this form to make them easier to correlate against the base addresses.
Also, the environment sector size (with comment about flash sector) makes it much more obvious that 0xefff6000 specified later on is exactly one sector prior to the 0xefff8000 base address of the U-Boot image.
>> +/* Don't bother checksumming the flash on boot */
>> +#undef CONFIG_SYS_FLASH_CHECKSUM
>
> Don't undef what is not defined anyway.
Fixed.
Thanks for the review!
Cheers,
Kyle Moffett
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6686 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110221/61eb5dfb/attachment.bin
More information about the U-Boot
mailing list