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

[Andreas Bießmann]

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?

<snip>

>>> +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, 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.
If you have the time I would be grateful if you could prepare patches
for that.

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

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

(Just my 2¢, no need to do it that way)

Best regards

Andreas Bießmann