diff mbox

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

Message ID CAD6G_RQdSZrOc4T-twvy=Vgf-GdqRXqb0bK7WpTSq0GC0uQ__w@mail.gmail.com
State Not Applicable
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Commit Message

Jagan Teki Aug. 26, 2017, 6:34 a.m. UTC
Hi,

Thanks for the changes.

On Tue, Jul 25, 2017 at 12:30 PM, Wenyou Yang <wenyou.yang@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.

         cmd.data_len = read_len;

thanks!

Comments

Wenyou Yang Aug. 30, 2017, 1:58 a.m. UTC | #1
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@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
Wenyou Yang Aug. 30, 2017, 3:25 a.m. UTC | #2
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@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?
> 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_flash
> *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
Bin Meng Aug. 30, 2017, 3:43 a.m. UTC | #3
On Wed, Aug 30, 2017 at 11:25 AM, Yang, Wenyou
<Wenyou.Yang@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@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.

>> 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_flash
>> *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;
>>

Regards,
Bin
Wenyou Yang Aug. 30, 2017, 5:27 a.m. UTC | #4
On 2017/8/30 11:43, Bin Meng wrote:
> On Wed, Aug 30, 2017 at 11:25 AM, Yang, Wenyou
> <Wenyou.Yang@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@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.
>
>>> 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_flash
>>> *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;
>>>
> Regards,
> Bin

Best Regards,
Wenyou Yang
Bin Meng Aug. 30, 2017, 5:41 a.m. UTC | #5
On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou <Wenyou.Yang@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@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@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.

Regards,
Bin
Wenyou Yang Aug. 30, 2017, 5:51 a.m. UTC | #6
On 2017/8/30 13:41, Bin Meng wrote:
> On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou <Wenyou.Yang@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@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@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
Jagan Teki Aug. 30, 2017, 6:30 a.m. UTC | #7
Hi Bin,

On Wed, Aug 30, 2017 at 11:11 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou <Wenyou.Yang@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@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@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

thanks!
Jagan Teki Aug. 30, 2017, 6:33 a.m. UTC | #8
Hi Wenyou Yang,

On Wed, Aug 30, 2017 at 7:28 AM, Yang, Wenyou <Wenyou.Yang@microchip.com> wrote:
> 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@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.

I will send explanation for these on respective patches, that would
rather clean to understand from code point-of-view.

thanks!
Bin Meng Aug. 30, 2017, 7:47 a.m. UTC | #9
Hi Jagan,

On Wed, Aug 30, 2017 at 2:30 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Bin,
>
> On Wed, Aug 30, 2017 at 11:11 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou <Wenyou.Yang@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@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@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.

My point is: if the warnings exist in current mainline, this should be
fixed as a unrelated patch. If the warnings come from this series,
this should be fixed in the particular patch that introduces the
warnings!

Regards,
Bin
Jagan Teki Aug. 30, 2017, 1:25 p.m. UTC | #10
Hi Bin,

On Wed, Aug 30, 2017 at 1:17 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Jagan,
>
> On Wed, Aug 30, 2017 at 2:30 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Bin,
>>
>> On Wed, Aug 30, 2017 at 11:11 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou <Wenyou.Yang@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@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@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!
diff mbox

Patch

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_flash
*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;