[U-Boot] [RFC][PATCH 3/3] Add board support for the eXMeritus HWW-1U-1A devices

[Peter Tyser]

<snip>

> >> 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?

The 2 places that jump to mind are drivers/gpio, or
arch/powerpc/cpu/mpc8xxx.  My personal preference would be drivers/gpio.

<snip>

> >> +++ 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.

I believe its contrary to normal coding style, you can read the details
at http://www.denx.de/wiki/U-Boot/CodingStyle.  Probably not a huge deal
either way.


> 
> >> +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)

There isn't a macro language in the dts file that you can use.  Its hard
to say if you should tweak the dtb in U-Boot.  People's opinions vary:
http://www.mail-archive.com/u-boot@lists.denx.de/msg36029.html

I'd give modifying the dtb in U-Boot a try, and people will let you know
if its outside of the scope of U-Boot's responsibility.

<snip>

> >> +/* 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.

If you are using different localbus devices and/or board clocking from
the p2020ds, it'd be good to review the settings as they likely could be
improved for your specific setup.

> 
> >> +/* 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.

The general philosophy is that U-Boot shouldn't include default MAC
addresses as inevitably at some point you'll have multiple boards on the
network with identical MACs, or customers will corrupt their EEPROM and
fall back to the default MAC, etc.

Best,
Peter