[U-Boot] [PATCH] arm: at91: at91sam9n12ek: add nandflash/spiflash/mmc/lcd support

["Andreas Bießmann" <andreas.devel@googlemail.com>]

Hi Josh,

On 03/19/2013 11:58 AM, Josh Wu wrote:
> Hi, Andreas
> 
> thanks for the review.
> 
> On 3/18/2013 9:48 PM, Andreas Bie?mann wrote:
>> Dear Josh Wu,
>>
>> this is an additional review. I left out MAINTAINERS, alphabetical
>> ordering, copyright stuff a.s.o. mentioned before.
>>
>> On 03/15/2013 11:17 AM, Josh Wu wrote:
>>> This patch adds at91sam9n12ek support, it enables:
>>> - dbgu
>>> - nand with pmecc
>>> - spi flash
>>> - mmc
>>> - lcd
>>>
>>> TODO:
>>> - usb
>>> - ethernet
>>>
>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---

<snip>

>>> +void spi_cs_activate(struct spi_slave *slave)
>>> +{
>>> +    switch (slave->cs) {
>>> +    case 0:
>>> +        at91_set_pio_output(AT91_PIO_PORTA, 14, 0);
>> Ouch ... before you setup these as peripherial lines, here you use it as
>> PIO. Please a) setup as PIO or b) do not set the line here cause it
>> should be set by SPI IP automagically on transfer (havn't checked that,
>> but should work).
> 
> I prefer to choose a) setup as PIO. for b), it may impact many other
> boards.

I'm fine with this solution.

<snip>

>>> --- a/drivers/spi/atmel_spi.c
>>> +++ b/drivers/spi/atmel_spi.c
>>> @@ -92,7 +92,8 @@ struct spi_slave *spi_setup_slave(unsigned int bus,
>>> unsigned int cs,
>>>       as->slave.cs = cs;
>>>       as->regs = regs;
>>>       as->mr = ATMEL_SPI_MR_MSTR | ATMEL_SPI_MR_MODFDIS
>>> -#if defined(CONFIG_AT91SAM9X5) || defined(CONFIG_AT91SAM9M10G45)
>>> +#if defined(CONFIG_AT91SAM9X5) || defined(CONFIG_AT91SAM9M10G45) \
>>> +    || defined(CONFIG_AT91SAM9N12)
>> I mentioned that before in a mail to Bo, can we please find some better
>> solution here like 'CPU_HAS_MCIx' (like the CPU_HAS_PIO3) or some other
>> identifier?
> 
> for the SPI ip, I will include a extra patch in next version, which will
> use a run-time ip detect for SPI.
> so those macro can be removed.

Wow, great. This will be a step in the right direction.

<snip>

>>> +#define CONFIG_SYS_MEMTEST_START    CONFIG_SYS_SDRAM_BASE
>>> +#define CONFIG_SYS_MEMTEST_END        0x26e00000
>> Wasn't there some change in mtest lately? Are these configs correct then?
> 
> hmm, I don't know the mtest well, this END address is just align with
> 9x5 config file.

Can you please read the doc/README.memory-test introduced in
a2681707b2478abef34b8c403e7ab52daae9c331
Please check that we haven't placed the exception table at
CONFIG_SYS_SDRAM_BASE and that we will not scratch the relocated u-boot
copy at 0x26e00000 (110MiB, i think it is ok).

<snip>

>>> +#else /* CONFIG_SYS_USE_MMC */
>>> +
>>> +/* bootstrap + u-boot + env + linux in mmc */
>>> +#define CONFIG_ENV_IS_IN_MMC
>>> +/* For FAT system, most cases it should be in the reserved sector */
>>> +#define CONFIG_ENV_OFFSET        0x2000
>>> +#define CONFIG_ENV_SIZE            0x1000
>>> +#define CONFIG_SYS_MMC_ENV_DEV        0
>>> +#define CONFIG_BOOTCOMMAND                        \
>>> +    "mmcinfo;fatload mmc 0:1 0x21000000 dtb;"            \
>> Isn't mmcinfo the old command? AFAIR this is obsolete with
>> CONFIG_MMC_GENERIC, please fix and use the newer commands.
> 
> I checked the source, seems not found the information about the mmcinfo
> is old command.
> But if we remove ;mmcinfo' in the bootcommand, it still works well. so I
> will remove 'mmcinfo' here.

Ok, it was my fault. mmcinfo is available in the 'generic'
implementation, but this is to display information of the card (see
common/cmd_mmc.c). The old commands are 'mmc init' (initialize) and 'mmc
device' (show card info). You should however add some 'mmc rescan'
(initialize) and maybe 'mmc dev $mmcdev $mmcpart' (set active mmc and
partition) before to ensure setup the correct mmc device and have it
enabled before accessing it.
I found the omap3_beagle script for CONFIG_BOOTCOMMAND quite useful.

Best regards

Andreas Bie?mann