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

[Jagan Teki]

Hi Bin,

On Wed, Aug 30, 2017 at 1:17 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Jagan,
>
> On Wed, Aug 30, 2017 at 2:30 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>> Hi Bin,
>>
>> On Wed, Aug 30, 2017 at 11:11 AM, Bin Meng <bmeng.cn at gmail.com> 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?
>>>
>>> If so, Jagan can you please explain why you mention this? This is
>>> nothing related to this patch review.
>>
>> Please read my previous comments, I was clearly explain the issue and
>> diff. Issue came up this series with 4BAIS on spi_flash_cmd_read_ops
>>
>
> So your words and Wenyou are contradictory. Wenyou claimed the
> warnings do not come from his patchset while you said yes. Anyway will
> leave you guys to resolve.

No, I didn't say that I wrote "Will send separate patch for this."
for 4BIAS fix not for the warning issue.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.