[U-Boot] [PATCH v5 5/5] Kirkwood: add lschlv2 and lsxhl board support

[Michael Walle]

Hi Prafulla,

thanks for the review.

On Thu, May 24, 2012 09:24, Prafulla Wadaskar wrote:
>> -----Original Message-----
>> From: Michael Walle [mailto:michael at walle.cc]
>> Sent: 12 May 2012 04:21
>> To: u-boot at lists.denx.de
>> Cc: Prafulla Wadaskar; Wolfgang Denk; Mike Frysinger; Joe Hershberger;
>> Michael Walle
>> Subject: [PATCH v5 5/5] Kirkwood: add lschlv2 and lsxhl board support
>>
>> This patch adds support for both the Linkstation Live (LS-CHLv2) and
>> Linkstation Pro (LS-XHL) by Buffalo.
>>
>> Signed-off-by: Michael Walle <michael at walle.cc>
>> Cc: Prafulla Wadaskar <prafulla at marvell.com>
>> ---
>>  MAINTAINERS                           |    5 +
>>  board/buffalo/lsxl/Makefile           |   50 ++++++
>>  board/buffalo/lsxl/kwbimage-lschl.cfg |  229
>> +++++++++++++++++++++++++
>>  board/buffalo/lsxl/kwbimage-lsxhl.cfg |  229
>> +++++++++++++++++++++++++
>
> BTW: These two file looks similar, what is the difference?

Mainly the memory configuration, eg LSCHLv2 has one 16x 512MBit, whereas
the LSXHL has two 8x 1GBit dram chips. And of course some memory
parameters are different.

[..snip..]


>> +     return 0;
>> +}
>> +
>> +#ifdef CONFIG_MISC_INIT_R
>> +void check_enetaddr(void)
>> +{
>> +     uchar enetaddr[6];
>> +
>> +     if (!eth_getenv_enetaddr_by_index("eth", 0, enetaddr)) {
>> +             /* signal unset/invalid ethaddr to user */
>> +             set_led(LED_INFO_BLINKING);
>> +     }
>> +}
>> +
>> +static void erase_environment(void)
>
> Why do you need this function?

Mh? To erase the environment; I don't think that is the answer you wanted
to know ;) Remember the only recovery method for a totally messed up
environment is:
 - push button longer than 10s -> erases the environment, resets the board
 - push button longer than 3s -> boots into rescue mode, eg. set temporary
enetaddr and enable netconsole.

So i can't rely on a functional environment, that is i can't run stored
commands. Another way would be to run a command directly, eg.
  run("sf probe 0 && sf erase 70000 +10000 && reset")

dunno if that is better, back then i felt like hardcoding is better. Esp.
since there may be some side effects if some special environment variables
are set. Eg. there may be the spi speed set in the environment. I don't
know if this is true now, but i guess one may come up with such a feature.
Then this might break my recovery procedure.

>> +{
>> +     struct spi_flash *flash;
>> +
>> +     printf("Erasing environment..\n");
>> +     flash = spi_flash_probe(0, 0, 1000000, SPI_MODE_3);
>> +     if (!flash) {
>> +             printf("Erasing flash failed\n");
>> +             return;
>> +     }
>> +
>> +     spi_flash_erase(flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE);
>> +     spi_flash_free(flash);
>> +     do_reset(NULL, 0, 0, NULL);
>> +}
>> +
>> +static void rescue_mode(void)
>> +{
>> +     uchar enetaddr[6];
>> +
>> +     printf("Entering rescue mode..\n");
>> +     if (!eth_getenv_enetaddr_by_index("eth", 0, enetaddr)) {
>> +             eth_random_enetaddr(enetaddr);
>> +             if (eth_setenv_enetaddr_by_index("eth", 0, enetaddr)) {
>> +                     printf("Failed to set ethernet address\n");
>> +                             set_led(LED_ALARM_BLINKING);
>> +                     return;
>> +             }
>> +     }
>> +     setenv("bootsource", "rescue");
>> +}
>> +
>> +static void check_push_button(void)
>> +{
>> +     int i = 0;
>> +
>> +     while (!kw_gpio_get_value(GPIO_FUNC_BUTTON)) {
>> +             udelay(100000);
>> +             i++;
>> +
>> +             if (i == 10)
>> +                     set_led(LED_INFO_ON);
>> +
>> +             if (i >= 100) {
>> +                     set_led(LED_INFO_BLINKING);
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (i >= 100)
>> +             erase_environment();
>> +     else if (i >= 10)
>> +             rescue_mode();
>> +}
>
> You can overload this functionality to Reset key, you don't need any
> supporting code for the same, you can handle it through additional
> environment variables.
> For more info check for "SYSTSTn Duration counter support" in
> Kirkwood/cpu.c

I saw that code, but unfortunately this is not the reset button.


>> +
>> +int misc_init_r(void)
>> +{
>> +     check_enetaddr();
> This need to be ifdefed with CONFIG_MISC_INIT_R

it is, see above. check_enetaddr() and check_push_button() are guarded, too.

>> +     check_push_button();
>> +
>> +     return 0;
>> +}
>> +#endif
>> +

[..snip..]

>> +/*
>> + *  Environment variables configurations
>> + */
>> +#ifdef CONFIG_SPI_FLASH
>> +#define CONFIG_SYS_MAX_FLASH_BANKS   1
>> +#define CONFIG_SYS_MAX_FLASH_SECT    8
>> +#define CONFIG_ENV_IS_IN_SPI_FLASH   1
>> +#define CONFIG_ENV_SECT_SIZE         0x10000 /* 64K */
>> +#else
>> +#define CONFIG_ENV_IS_NOWHERE
>> +#endif
>> +
>> +#define CONFIG_ENV_SIZE                      0x10000 /* 64k */
>> +#define CONFIG_ENV_OFFSET            0x70000 /* env starts here */
>
> Why this is too far, what it u-boot.bin size?

Thats the original flash layout, 0x0-0x6ffff is uboot and 0x70000-0x7ffff
is  environment. ATM uboot size is around 40000 (hex) bytes.

-- 
michael