From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Wed, 30 Aug 2017 15:47:07 +0800 Subject: [U-Boot] [PATCH v3 0/8] sf: improve support of (Q)SPI flash memories In-Reply-To: References: <20170725070102.1344-1-wenyou.yang@microchip.com> <285a60d7-6f71-5196-d52b-a5a2aad383b3@Microchip.com> <63106f2a-0fd9-4dcd-4077-b10cc40a1275@Microchip.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Hi Jagan, On Wed, Aug 30, 2017 at 2:30 PM, Jagan Teki wrote: > Hi Bin, > > On Wed, Aug 30, 2017 at 11:11 AM, Bin Meng wrote: >> On Wed, Aug 30, 2017 at 1:27 PM, Yang, Wenyou wrote: >>> >>> >>> On 2017/8/30 11:43, Bin Meng wrote: >>>> >>>> On Wed, Aug 30, 2017 at 11:25 AM, Yang, Wenyou >>>> 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 >>>>>> >>>>>> 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 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