[U-Boot] [PATCH v5] at91: add support for CDU9G25 board

[Jiří Prchal]

Dear Andreas,

Dne 16.9.2013 10:27, Andreas Bießmann napsal(a):
> Dear Jiří Prchal,
>
> On 09/13/2013 04:41 PM, Jiří Prchal wrote:
>> Dne 13.9.2013 16:10, Andreas Bießmann napsal(a):
>>> On 09/13/2013 03:00 PM, Jiri Prchal wrote:
>
> <snip>
>
>>>> diff --git a/arch/arm/include/asm/mach-types.h
>>>> b/arch/arm/include/asm/mach-types.h
>>>> index 440b041..9b274ba 100644
>>>> --- a/arch/arm/include/asm/mach-types.h
>>>> +++ b/arch/arm/include/asm/mach-types.h
>>>> @@ -986,6 +986,7 @@ extern unsigned int __machine_arch_type;
>>>>    #define MACH_TYPE_VIT_IBOX             3371
>>>>    #define MACH_TYPE_DM6441_ESP           3372
>>>>    #define MACH_TYPE_AT91SAM9X5EK         3373
>>>> +#define MACH_TYPE_CDU9G25              3373
>>>
>>> NAK, please obtain a mach type:
>>> http://www.arm.linux.org.uk/developer/machines/?action=new
>>
>> Should I register machine? I develop DT only:
>> "NOTE 1:If you are developing a DT-only platform, you do not need to
>> register a machine type for it.
>> Please do not register a machine type. Thanks."
>> Do I need this MACH_TYPE_* at all?
>
> I'm not really familiar with FDT only boards, but I think it is sane to
> just use a zero machid then. Could you please check this and if working
> just omit the machid setting?

I'm not really familiar with FDT too, but I tested with 0 and it works.

>>>> +err_spi_xfer:
>>>> +    spi_release_bus(slave);
>>>> +err_spi_claim_bus:
>>>> +    spi_free_slave(slave);
>>>> +err_spi_setup_slave:
>>>> +    if (!is_valid_ether_addr(sernum)) {
>>>> +        eth_random_enetaddr(sernum);
>>>> +        printf("Using random MAC address %pM\n", sernum);
>>>
>>> is that %p formating intended? Shouldn't you print the ethaddr here in
>>> hex rather than the pointer to the memory location?
>>
>> No, this is no pointer modifier, it's together %pM and it prints ethaddr
>> like this: 02:22:23:15:86:a5.
>
> Sorry, my fault.
>
> <snip>
>
>>>> +#define CONFIG_CMD_BOOTZ
>>>> +#define CONFIG_OF_LIBFDT
>>>> +
>>>> +/* general purpose I/O */
>>>> +#define CONFIG_ATMEL_LEGACY        /* required until (g)pio is fixed */
>>>
>>> I doubt you need this for gpio. Could you please check, if it is really
>>> required?

Yes, it's required.

>> Yes, I knew that, but I have looked many other board files and they use
>> both GPIO and PIO in one file.
>> If is necessary I'll re-base it to PIO.
>
> No, nand is not yet prepared for the generic gpio framework. I'll try to
> post patches next weeks (for 2014.01), but I'm quite busy right now.
> Since your board will also end up in 2014.01 this should go together.

I'll wait for that.

> If you have the time I would be grateful if you could prepare patches
> for that.

Sorry, but I have look in atmel_nand.c and didn't find any pio. Seems to me it's prepared to gpio.
So I leave it up to you.

>
> <snip>
>
>>>> +#define CONFIG_MTD_DEVICE
>>>> +#define CONFIG_CMD_MTDPARTS
>>>> +#define CONFIG_MTD_PARTITIONS
>>>> +#define CONFIG_RBTREE
>>>> +#define CONFIG_LZO
>>>> +#define CONFIG_CMD_UBI
>>>> +#define CONFIG_CMD_UBIFS
>>>> +#define CONFIG_CMD_NAND_TRIMFFS
>>>> +#define MTDIDS_DEFAULT            "nand0=nand"
>>>> +#define MTDPARTS_DEFAULT        "mtdparts=nand:256k(bootstrap),"\
>>>> +                    "768k(uboot),256k(ubootenv),"\
>>>> +                    "4864k(kernel),"\
>>>> +                    "-(root)"
>>>
>>> just to mention it:
>>>    a) a secondary env is sometimes useful
>>
>> Is that true? I thought use one copy env, if not then use default.
>
> Well, your point is true, but how about heavily changed env in the
> field? If something goes wrong (two bad blocks at worst place, wrong crc
> due to too much bit errors, ...) it will fall back to the compiled in
> env, which may be completely wrong then.
> But it is also true that an enabled secondary env will be read on every
> startup and compared to the first env. Also you always need to write two
> locations. It depends on your use case, but for me it is careless to
> have just one env on NAND.
>

We use in 99% compiled in env and even if not there is no need to heavily change env.

> <snip>
>
>>>> +#define CONFIG_BOOTCOMMAND    "nand read 21000000 kernel; bootm"
>>>> +#define CONFIG_BOOTARGS        "console=ttyS0,115200 ubi.mtd=root "\
>>>> +                "root=ubi0:root rootfstype=ubifs rw"
>>>> +#define CONFIG_SERVERIP        10.0.1.1
>>>> +#define CONFIG_BOOTFILE        "kernel_cdu9g25"
>>>> +#define CONFIG_PREBOOT        "mtdparts default" /* for partitions */
>>>
>>> Would you really like to always reset a new mtd partitioning on every
>>> boot to the default one?
>> Does it mean that I don't need run this command if I like use partitions
>> in nand commands?
>
> No, not fully. You'll need the environment parameter 'mtdparts' with
> contend 'mdtparts=...' and respective mtdids env. This could be set by
> the command 'mtdparts default', it will set the compiled in values of
> MTDIDS_DEFAULT and MTDPARTS_DEFAULT.
> If you like to change the nand partitioning in any way you need to
> change the mtdparts env. But when you call 'mtdparts default' afterwards
> you will reset this to the default value ...
> To avoid this we have in our default environment following setting:
>
> ---8<---
> #define CONFIG_EXTRA_ENV_SETTINGS \
>    "mtdparts=" MTDPARTS_DEFAULT "\0" \
>    "mtdids=" MTDIDS_DEFAULT "\0" \
> ...
> --->8---
>
> Additionally we avoid to use the 'mtdparts default' command.
> With this setup one could change the mtdparts environment any time and
> it would take effect.

I've changed it this way, you're right. I'm still learning new things.

>
> (Just my 2¢, no need to do it that way)
>
> Best regards
>
> Andreas Bießmann
>
Thanks
Jiri