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

[Yang, Wenyou]

On 2017/8/30 13:41, Bin Meng wrote:
> On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou <Wenyou.Yang at microchip.com> wrote:
>>
>> On 2017/8/30 11:43, Bin Meng wrote:
>>> On Wed, Aug 30, 2017 at 11:25 AM, Yang, Wenyou
>>> <Wenyou.Yang at microchip.com> wrote:
>>>>
>>>> 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?
>>>>>
>>>>> 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;
>>>>>           ^~~~~~~~~~
>>>>>      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.
>>>> Will You send a separate patch? or I include it in this patch set?
>>> This should not be a separate patch. Since every patch needs to pass
>>> buildman testing.
>> But it is not introduced by this patch set. So should be a separate patch to
>> fix.
> Do you mean the build warnings exist in current u-boot/master?
Here are two issue, one is the build warning, other is the issue with 
spi_flash_cmd_read_ops.
The build warning is related with this patch, I will fix it in the next 
version.
But  the issue with spi_flash_cmd_read_ops on bank length is not related 
with this patch.

>
> If so, Jagan can you please explain why you mention this? This is
> nothing related to this patch review.
>
> Regards,
> Bin

Best Regards,
Wenyou Yang