[U-Boot] [RFC][PATCH 3/3] Add board support for the eXMeritus HWW-1U-1A devices
Kyle Moffett
kyle at moffetthome.net
Fri Sep 3 07:50:26 CEST 2010
On Fri, Sep 3, 2010 at 00:00, Peter Tyser <ptyser at xes-inc.com> wrote:
> Hi Kyle,
Hi!
Thanks for the great in-depth review, I really appreciate it! I've
included my comments below, and I'll try to respin the patches here
soon. I've dropped the second patch (the e1000e one) as it's
apparently just a workaround for an as-yet-undiagnosed EEPROM issue
with our boards.
>> MAKEALL | 2 +
>> Makefile | 4 +
>
> MAINTAINERS entry is missing.
Fixed in my next patch, thanks!
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2499,6 +2499,10 @@ P2020DS_36BIT_config \
>> P2020DS_config: unconfig
>> @$(MKCONFIG) -t $(@:_config=) P2020DS ppc mpc85xx p2020ds freescale
>>
>> +HWW_1U_1A_36BIT_config \
>> +HWW_1U_1A_config: unconfig
>> + @$(MKCONFIG) -t $(@:_config=) HWW_1U_1A ppc mpc85xx hww-1u-1a exmeritus
>> +
>
> The new way to add a board is via boards.cfg. You shouldn't need to
> modify this Makefile.
Ok, I'll go poke around, thanks for letting me know!
>> +++ b/board/exmeritus/hww-1u-1a/config.mk
>> +#
>> +# p2020ds board
>> +#
>
> P2020ds?
Oops, missed a few. I'll go back through and fix the remaining
p2020ds references.
>> +++ b/board/exmeritus/hww-1u-1a/ddr.c
>> +/* XXX: these values need to be checked for all interleaving modes. */
>> +/* XXX: No reliable dual-rank 800 MHz setting has been found. It may
>> + * seem reliable, but errors will appear when memory intensive
>> + * program is run. */
>> +/* XXX: Single rank at 800 MHz is OK. */
>
> These comments look like the p2020ds comments. Please remove/update
> them if they don't apply to your board.
Yep, sorry. I'd actually fixed this in a different git tree but lost
the change when doing a rebase.
>> +const board_specific_parameters_t board_specific_parameters[][20] = {
>> + {
>> + /* memory controller 0 */
>> + /* lo| hi| num| clk| cpo|wrdata|2T */
>> + /* mhz| mhz|ranks|adjst| | delay| */
>> +#if 0
>> + { 0, 333, 2, 6, 7, 3, 0},
>> + {334, 400, 2, 6, 9, 3, 0},
>> + {401, 549, 2, 6, 11, 3, 0},
>> + {550, 680, 2, 1, 10, 5, 0},
>> + {681, 850, 2, 1, 12, 5, 1},
>> + { 0, 333, 1, 6, 7, 3, 0},
>> + {334, 400, 1, 6, 9, 3, 0},
>> + {401, 549, 1, 6, 11, 3, 0},
>> + {550, 680, 1, 1, 10, 5, 0},
>> + {681, 850, 1, 1, 12, 5, 0}
>> +#else
>> + { 0,1000, 2, 4, 4, 2, 0},
>> + { 0,1000, 1, 4, 4, 2, 0},
>> +#endif
>> + },
>> +};
>
> I'd remove the #if 0 above. Dead code is bad.
Same as above, this part came back during a rebase. Will adjust, thanks!
>> diff --git a/board/exmeritus/hww-1u-1a/gpios.h b/board/exmeritus/hww-1u-1a/gpios.h
>> +static inline void hww1u1a_gpio_set(unsigned int mask,
>> + unsigned int dir, unsigned int val)
>> +{
>> + volatile ccsr_gpio_t *gpio;
>> +
>> + /* First mask off the unwanted parts of "dir" and "val" */
>> + dir &= mask;
>> + val &= mask;
>> +
>> + /* Now read in the values we're supposed to preserve */
>> + gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00);
>> + dir |= (in_be32(&gpio->gpdir) & ~mask);
>> + val |= (in_be32(&gpio->gpdat) & ~mask);
>> +
>> + /* Now write out the new values, writing the direction first */
>> + out_be32(&gpio->gpdir, dir);
>> + asm("sync; isync":::"memory");
>> + out_be32(&gpio->gpdat, val);
>> +}
>> +
>> +static inline void hww1u1a_gpio_set_in(unsigned int gpios)
>> +{
>> + hww1u1a_gpio_set(gpios, 0x00000000, 0x00000000);
>> +}
>> +
>> +static inline void hww1u1a_gpio_set_low(unsigned int gpios)
>> +{
>> + hww1u1a_gpio_set(gpios, 0xFFFFFFFF, 0x00000000);
>> +}
>> +
>> +static inline void hww1u1a_gpio_set_high(unsigned int gpios)
>> +{
>> + hww1u1a_gpio_set(gpios, 0xFFFFFFFF, 0xFFFFFFFF);
>> +}
>> +
>> +static inline unsigned int hww1u1a_gpio_get(unsigned int mask)
>> +{
>> + volatile ccsr_gpio_t *gpio;
>> + unsigned int ret;
>> +
>> + /* Read the requested values */
>> + gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00);
>> + ret = mask & in_be32(&gpio->gpdat);
>> +
>> + return ret;
>> +}
>
> The GPIO functions above aren't hww1u1a specific. What about adding
> generic 85xx GPIO functions so others can use them too?
I can do that. Do you have any particular place you recommend I put them?
>> +static inline unsigned int hww1u1a_is_cpu_a(void)
>> +{
>> + return !hww1u1a_gpio_get(GPIO_CPU_ID);
>> +}
>> +
>> +static inline unsigned int hww1u1a_is_cpu_b(void)
>> +{
>> + return !!hww1u1a_gpio_get(GPIO_CPU_ID);
>> +}
>> +
>
> With only 2 CPUs do you need both the functions above? Eg below you use the logic:
> if (hww1u1a_is_cpu_a())
> stuff
> else
> stuff
>
> I don't see hww1u1a_is_cpu_b() being used anywhere.
Hmm... yeah, I guess I did remove the only user of hww1u1a_is_cpu_b().
Since it's a static inline (and so consumes no space in the binary if
unused) I'm sort of tempted to keep it just for symmetry but it does
strike me as redundant. I guess I'll get rid of it.
>> +++ b/board/exmeritus/hww-1u-1a/hww-1u-1a.c
>> + /* These GPIOs are common */
>> + gpio_in |= IRQ_I2CINT | IRQ_FANINT | IRQ_DIMM_EVENT;
>> + gpio_low |= GPIO_RS422_RE;
>> + gpio_high |= GPIO_RS422_DE;
>> +
>> + /* Ok, now go ahead and program all of those in one go */
>> + hww1u1a_gpio_set( gpio_high|gpio_low|gpio_in,
>> + gpio_high|gpio_low,
>> + gpio_high);
>
> The tab above should be removed.
It looked easier for me to read when aligned like that, but if it's
contrary to coding-style I'll fix it.
>> +/* Create a prompt-like string: "uboot at HOSTNAME% " */
>> +#define PROMPT_PREFIX "uboot at exm"
>> +#define PROMPT_SUFFIX "% "
>> +
>> +/* This function returns a PS1 prompt based on the serial number */
>> +static char *hww1u1a_prompt = NULL;
>> +const char *hww1u1a_get_ps1(void)
>> +{
>> + unsigned long len, i, j;
>> + const char *serialnr;
>> +
>> + /* If our prompt was already set, just use that */
>> + if (hww1u1a_prompt)
>> + return hww1u1a_prompt;
>> +
>> + /* Use our serial number if present, otherwise "UNPROGRAMMED-[AB]" */
>> + serialnr = getenv("serial#");
>> + if (!serialnr || !serialnr[0]) {
>> + if (hww1u1a_is_cpu_a())
>> + serialnr = "999999-XA";
>> + else
>> + serialnr = "999999-XB";
>> + }
>> +
>> + /*
>> + * We will turn the serial number into a hostname by:
>> + * (A) Delete all non-alphanumerics.
>> + * (B) Lowercase all letters
>> + * (C) Prefix "exm"
>> + */
>> + for (i = 0, len = 0; serialnr[i]; i++) {
>> + if ('0' <= serialnr[i] && serialnr[i] <= '9')
>> + len++;
>> + else if ('a' <= serialnr[i] && serialnr[i] <= 'z')
>> + len++;
>> + else if ('A' <= serialnr[i] && serialnr[i] <= 'Z')
>> + len++;
>> + }
>
> There are utility functions in include/linux/ctype.h that do the above
> checks. This, and the similar chunk of code below seems like a lot of
> effort to get rid of non digits/alpha chars. Is there a reason to not
> just assume the serial number variable is valid, or include all chars in
> the prompt?
This is partly compensating for some changes in serial-number format
between various generations of prototypes, making them all have the
same-format prompt. The rest of it is just me being an
overly-defensive programmer. I'll rewrite it to use ctype.h, but the
character fiddling is a relatively small amount of code and I'd like
to keep it around.
>> +int board_eth_init(bd_t *bis)
>> +{
>> + struct tsec_info_struct tsec_info[4];
>> + volatile ccsr_gpio_t *pgpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00);
>> +
>> + SET_STD_TSEC_INFO(tsec_info[0], 1);
>> + SET_STD_TSEC_INFO(tsec_info[1], 2);
>> + SET_STD_TSEC_INFO(tsec_info[2], 3);
>> + if (pgpio->gpdat & GPIO_CPU_ID)
>
> Is the above check the same as the hww1u1a_is_cpu_a() function
> previously added?
Oops, yes it is, will fix. That reminds me of one other question,
though. Several variables are different between the CPU A and the CPU
B on the board but maintaining 2 nearly-identical device trees is kind
of a pain. Is there any good way to manage that? Should I try to
poke the device tree file during boot, or is there some kind of macro
language I can use in the dts file? (EG: "#ifdef CPU_A", etc)
>> +/* We don't have any really usable POST memory, so just fake it */
>> +unsigned long post_word_load (void)
>> +{
>> + return 0l;
>> +}
>> +
>> +void post_word_store (unsigned long val)
>> +{
>> + return;
>> +}
>
> Does this work with POSTs? I had thought you'd need to be able to
> read/write the post word, even if it wasn't saved across reboots. It
> should be in a #ifdef CONFIG_POST if you do keep it around.
I copied this from another board implementation, but I never got
around to testing CONFIG_POST properly. I'll delete this from the
current patch and put it back on my TODO list.
>> +static inline int pca9554_set_low(u8 gpio)
>> +{
>> + return pca9554_setmask(PCA9554_DATA_OUT, 0x00, (1U << gpio));
>> +}
>> +
>> +static inline int pca9554_set_high(u8 gpio)
>> +{
>> + return pca9554_setmask(PCA9554_DATA_OUT, 0xff, (1U << gpio));
>> +}
>> +
>> +static inline int pca9554_set_output(u8 gpio)
>> +{
>> + return pca9554_setmask(PCA9554_IS_INPUT, 0x00, (1U << gpio));
>> +}
>> +
>> +static inline int pca9554_set_input(u8 gpio)
>> +{
>> + return pca9554_setmask(PCA9554_DATA_OUT, 0xff, (1U << gpio));
>> +}
>
> These look very similar to the pca953x driver. It'd be worth
> investigate using it.
Heh, that's new... there wasn't one when I first started this board
port. I'll go poke around, thanks!
>> + /* *I*G* - PCI memory */
>> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE3_MEM_VIRT,
>> + CONFIG_SYS_PCIE3_MEM_PHYS,
>> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>> + 0, 3, BOOKE_PAGESZ_1G, 1),
>
> The LAW is only 512 MB.
>
>> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE3_MEM_VIRT + 0x40000000,
>> + CONFIG_SYS_PCIE3_MEM_PHYS + 0x40000000,
>> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>> + 0, 4, BOOKE_PAGESZ_256M, 1),
>
> Is there a reason you don't use CONFIG_SYS_PCIE2_MEM_* here? And the
> LAW is 512MB above.
I think I just copied these TLB entries unmodified from the P2020DS
eval board... I'll go recheck, but if these are wrong then the p2020ds
board is probably wrong too.
>> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE3_MEM_VIRT + 0x50000000,
>> + CONFIG_SYS_PCIE3_MEM_PHYS + 0x50000000,
>> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>> + 0, 5, BOOKE_PAGESZ_256M, 1),
>> +
>> + /* *I*G* - PCI I/O */
>> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE3_IO_VIRT,
>> + CONFIG_SYS_PCIE3_IO_PHYS,
>> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>> + 0, 6, BOOKE_PAGESZ_256K, 1),
>> +};
>
> I'd personally get rid of the tabs above. Others may differ.
I'd prefer to keep them if possible.
>> +int num_tlb_entries = ARRAY_SIZE(tlb_table);
>> diff --git a/cpu/mpc85xx/cpu.c b/cpu/mpc85xx/cpu.c
>> index 0cc6e03..49d963d 100644
>> --- a/cpu/mpc85xx/cpu.c
>> +++ b/cpu/mpc85xx/cpu.c
>> @@ -190,8 +190,10 @@ int checkcpu (void)
>>
>> int do_reset (cmd_tbl_t *cmdtp, bd_t *bd, int flag, int argc, char *argv[])
>> {
>> -/* Everything after the first generation of PQ3 parts has RSTCR */
>> -#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
>> +#if defined(CONFIG_BOARD_RESET_R)
>> + extern void board_reset_r(void);
>> + board_reset_r();
>> +#elif defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
>> defined(CONFIG_MPC8555) || defined(CONFIG_MPC8560)
>> unsigned long val, msr;
>>
>> @@ -207,6 +209,7 @@ int do_reset (cmd_tbl_t *cmdtp, bd_t *bd, int flag, int argc, char *argv[])
>> val |= 0x70000000;
>> mtspr(DBCR0,val);
>> #else
>> + /* Everything after the first generation of PQ3 parts has RSTCR */
>> volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
>> out_be32(&gur->rstcr, 0x2); /* HRESET_REQ */
>> udelay(100);
>
> This change is not board-specific, it should go in a separate patch.
Ah, whoops! I forgot about this one; I'll go break it out.
>> +/* Enable IRQs and watchdog with a 1000Hz system decrementer */
>> +#define CONFIG_CMD_IRQ
>> +#define CONFIG_SYS_HZ 1000
>> +#if 0 /* FIXME */
>> +#define CONFIG_WATCHDOG 1
>> +
>> +/* Enable Power-On-Self-Tests */
>> +#define CONFIG_POST 1
>> +#define CONFIG_CMD_DIAG
>> +#endif
>
> I'd get rid of this #if 0.
Yeah, more things to pull out of this patch and put back on the TODO
list, thanks!
>> +
>> +/************************************************************************
>> + * Clock crystal configuration: *
>> + * (1) SYS: 66.666MHz +/- 50ppm (drives CPU/PCI/DDR) *
>> + * (2) CCB: Multiplier from SYS_CLK *
>> + * (3) RTC: 25.000MHz +/- 50ppm (sampled against CCB clock) *
>> + ************************************************************************/
>> +#undef CONFIG_CLOCKS_IN_MHZ
>
> You shouldn't need the undef above.
It was copied from P2020DS, I think. I'll go double-check.
>> +/* Physical Memory Map */
>> +#ifdef CONFIG_PHYS_64BIT
>> +#define CONFIG_SYS_PCIE3_MEM_PHYS 0xc00000000ull
>> +#define CONFIG_SYS_PCIE2_MEM_PHYS 0xc20000000ull
>> +#define CONFIG_SYS_PCIE1_MEM_PHYS 0xc40000000ull
>> +#define CONFIG_SYS_FLASH_BASE_PHYS 0xfe0000000ull
>> +#define CONFIG_SYS_PCIE3_IO_PHYS 0xfffc00000ull
>> +#define CONFIG_SYS_PCIE2_IO_PHYS 0xfffc10000ull
>> +#define CONFIG_SYS_PCIE1_IO_PHYS 0xfffc20000ull
>> +#define CONFIG_SYS_CCSRBAR_PHYS 0xfffe00000ull
>> +#else
>> +#define CONFIG_SYS_PCIE3_MEM_PHYS 0x80000000ul
>> +#define CONFIG_SYS_PCIE2_MEM_PHYS 0xa0000000ul
>> +#define CONFIG_SYS_PCIE1_MEM_PHYS 0xc0000000ul
>> +#define CONFIG_SYS_FLASH_BASE_PHYS 0xe0000000ul
>> +#define CONFIG_SYS_PCIE3_IO_PHYS 0xffc00000ul
>> +#define CONFIG_SYS_PCIE2_IO_PHYS 0xffc10000ul
>> +#define CONFIG_SYS_PCIE1_IO_PHYS 0xffc20000ul
>> +#define CONFIG_SYS_CCSRBAR_PHYS 0xffe00000ul
>> +#endif
One extra question about the physical memory mapping. I'd like to
support both 32-bit-phys and 36-bit-phys kernels from a single
device-tree and U-boot image by using a non-linear memory mapping,
specifically, something like this:
0x000000000 - 0x07fffffff: First 2GB of DDR memory
0x080000000 - 0x0ffffffff: PCI and device IO windows
0x100000000 - 0x17fffffff: Remaining 2GB of DDR memory (only
accessible from 36-bit-phys kernel)
It looks like U-boot doesn't support a setup like this right now, any
idea what kind of changes would be involved to make that happen?
>> +/************************************************************************
>> + * Serial Console Configuration *
>> + ************************************************************************/
>> +#define CONFIG_CONS_INDEX 1
>> +#undef CONFIG_SERIAL_SOFTWARE_FIFO
>
> Remove undef if not needed.
Ok, thanks.
>> +#define CONFIG_SYS_NS16550
>> +#define CONFIG_SYS_NS16550_SERIAL
>> +#define CONFIG_SYS_NS16550_REG_SIZE 1
>> +#define CONFIG_SYS_NS16550_CLK get_bus_freq(0)
>> +
>> +#define CONFIG_BAUDRATE 115200
>> +#define CONFIG_SYS_BAUDRATE_TABLE \
>> + {300, 600, 1200, 2400, 4800, 9600, 19200, 38400,115200}
>
> Space before 115200.
Fixed!
>> +/* I2C bus configuration */
>> +#define CONFIG_SYS_I2C_SPEED 400000
>> +#define CONFIG_SYS_I2C_SLAVE 0x7F
>> +#define CONFIG_SYS_I2C_NOPROBES { \
>> + { 0, 0x29 } \
>> +}
>
> It'd be good to document why the devices are no probes.
Dunno, that was copied from an identical undocumented chunk of the
P2020DS board. It's entirely reasonable that it would not apply to
our board. I'll go poke at it a bit.
>> +/* DDR2 SO-RDIMM SPD EEPROM is at I2C0-0x51 */
>> +#define CONFIG_SYS_SPD_BUS_NUM 0
>> +#define SPD_EEPROM_ADDRESS1 0x51
>> +#if 0
>> +#define CONFIG_CMD_EEPROM
>> +#endif
>
> Remove dead code. Same comment applies for future if 0s.
Same as above, things to pull out and put back on TODO list.
>> +/* Flash configuration registers */
>> +#define CONFIG_SYS_BR0_PRELIM (BR_PHYS_ADDR(FLASH0_PHYS) | BR_PS_16 | BR_V)
>> +#define CONFIG_SYS_BR1_PRELIM (BR_PHYS_ADDR(FLASH1_PHYS) | BR_PS_16 | BR_V)
>> +#define CONFIG_SYS_OR0_PRELIM 0xf8000ff7
>> +#define CONFIG_SYS_OR1_PRELIM 0xf8000ff7
>
> There are ORx defines. They should be used to make it clear what the
> settings above are at a glance.
Ok, I'll have to go look up what those bits mean since I copied them
unmodified from the P2020DS board.
>> +/* PCI-E dual-port E1000 (external ethernet ports) */
>> +#define CONFIG_E1000 1
>> +#define CONFIG_E1000_FALLBACK_MAC { 0x00, 0x05, 0x93, 0x81, 0xff, 0xfe }
>
> The general U-Boot policy is to not set default MACs.
The e1000e chips on our board need their EEPROMs to be programmed by
software during provisioning. At one point I think we needed this in
order for the programming to work successfully, but it's possible that
no longer applies; I'll go verify it. Regardless, this particular MAC
address is one I selected from our suballocation specifically for this
purpose.
>> +/* Extra environment parameters */
>> +#define CONFIG_EXTRA_ENV_SETTINGS \
>> + "preboot=setenv bootargs \"${bootargs} "CONFIG_BOOTARGS_DYNAMIC"\"\0" \
>> + "perf_mode=stable\0" \
>
> Do you use perf_mode? If not, it should be removed.
Not sure, but probably not. I vaguely recall that some piece of
generic code was looking at it, but that may just be a leftover from
the P2020DS board.
>> + "memctl_intlv_ctl=2\0" \
>
> I'd use the string instead of number to describe the interleaving, eg
> "bank".
This was copied from P2020DS, but I'll fix.
>> + "flkernel=0xe8020000\0" \
>> + "flinitramfs=0xe8800000\0" \
>> + "fldevicetree=0xeff20000\0" \
>> + "flbootm=bootm ${flkernel} ${flinitramfs} ${fldevicetree}\0" \
>> + "flboot=run preboot; run flbootm\0"
>
> Its preferred to use tabs instead of spaces to start new lines above.
Ok. I was mainly trying to make it all fit into my 80 chars terminal :-D.
Thanks again for a great review! I'll try to have updated patches out shortly!
Cheers,
Kyle Moffett
More information about the U-Boot
mailing list