[U-Boot] [PATCH v3 0/8] sf: improve support of (Q)SPI flash memories

[Yang, Wenyou]

Hi Jagan,

On 2017/8/26 14:34, Jagan Teki wrote:
> Hi,
>
> Thanks for the changes.
>
> On Tue, Jul 25, 2017 at 12:30 PM, Wenyou Yang <wenyou.yang at microchip.com> wrote:
>> This series of patches are based and have been tested on the 'master'
>> branch of the u-boot.git tree.
>>
>> Tests were passed with a sama5d2 xplained board which embeds both SPI and
>> QSPI controllers.
>>
>> The following tests have been passed:
>>
>> - QSPI0 + Macronix MX25L25673G:
>>    + probe: OK
>>    + Fast Read 1-1-4 at offset 0x10000 (u-boot env): OK
>>    + Page Program 1-1-4 at offset 0x10000: OK
>>      The Macronix datasheet tells that only Page Program 1-4-4 is
>>      supported, not Page Program 1-1-4, however it worked, I don't know
>>      why...
>>
>> - QSPI0 + Microchip SST26
>>    + probe: OK
>>    + Fast Read 1-1-4 at offset 0x10000 (u-boot env): OK
>>    + Page Program 1-1-1 at offset 0x10000: OK
>>      SST26 memories support Page Program 1-4-4 but with the op code of
>>      Page Program 1-1-4, which is not standard so I don't use it.
>>
>> - QSPI0 + Adesto AT25DF321A
>>    + probe: OK
>>    + Fast Read 1-1-1 at offset 0x10000 (u-boot env): OK
>>    + Page Program 1-1-1 at offset 0x10000: OK
>>
>> - SPI0 + Adesto AT25DF321A
>>    + probe: OK
>>    + Fast Read 1-1-1 at offset 0x6000 (u-boot env): OK
>>    + Page Program 1-1-1 at offest 0x6000: OK
>>
>> - SPI1 + Atmel AT45
>>    + probe: OK
>>    + Read at offset 0 and other than 0: OK
>>    + Write at offset 0 and other than 0: OK
>>
>> During my tests, I used:
>>    - setenv/saveenv, reboot, printenv
>>    or
>>    - sf probe, sf read, sf write, sf erase and sf update.
>>
>> Changes in v3:
>>   - Add the include <spi.h> to fix build error for corvus_defconfig.
>>
>> Changes in v2:
>>   - Rebase on the latest u-boot/master(2710d54f5).
>>
>> Cyrille Pitchen (8):
>>    spi: add support of SPI flash commands
>>    sf: describe all SPI flash commands with 'struct spi_flash_command'
>>    sf: select the relevant SPI flash protocol for read and write commands
>>    sf: differentiate Page Program 1-1-4 and 1-4-4
>>    sf: add 'addr_len' member to 'struct spi_flash'
>>    sf: add new option to support SPI flash above 16MiB
>>    sf: add support to Microchip SST26 QSPI memories
>>    sf: add driver for Atmel QSPI controller
> Comments:
> How about writing struct spi_flash_command in spi_flash area
> (include/spi_flash.h)? and then write atmel_qspi with
> UCLASS_SPI_FLASH?
The spi_flash_command struct describes the relevant features of the spi 
controller, instead of the spi_flash device.
The purpose of patch set is used to supersede the spi_xfer() function to 
access the spi_flash device.
So putting it in include/spi.h is suitable, we should not move it in the 
spi_flash area.

Moreover, why do we write atmel_qspi with  UCLASS_SPI_FLASH?  It is not 
easy to understand.
>
> Testing:
> Basic testing works fine.
>
> Issues:
> - Build issue: with zynq_microzed_defconfig
> drivers/mtd/spi/spi_flash.c: In function ‘spi_flash_scan’:
> drivers/mtd/spi/spi_flash.c:1049:7: warning: variable ‘above_16MB’ set
> but not used [-Wunused-but-set-variable]
>    bool above_16MB;

Will send a new version to fix it.  Thanks a lot.

>         ^~~~~~~~~~
>    CC      spl/lib/membuff.o
>
> - issue with spi_flash_cmd_read_ops 4BAIS
> Need to calculate bank length only if BAR is in use. Otherwise,
> consider the given len as read_len.
>
> Will send separate patch for this.
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 89ceae2..b5d8ef3 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -558,13 +558,15 @@ int spi_flash_cmd_read_ops(struct spi_flashmake
> *flash, u32 offset,
>           if (ret < 0)
>               return ret;
>           bank_sel = flash->bank_curr;
> -#endif
>           remain_len = ((SPI_FLASH_16MB_BOUN << flash->shift) *
>                   (bank_sel + 1)) - offset;
>           if (len < remain_len)
>               read_len = len;
>           else
>               read_len = remain_len;
> +#else
> +        read_len = len;
> +#endif
>
>           cmd.addr = read_addr;
>           cmd.data_len = read_len;
> thanks!

Best Regards,
Wenyou Yang