Hello Jagan, On Wed, 16 Mar 2016 22:04:23 +0530, Jagan Teki <jt...@openedev.com> wrote: > Hi Albert, > > On Wednesday 16 March 2016 09:53 PM, Albert ARIBAUD wrote: > > Hello Jagan, > > > > On Wed, 16 Mar 2016 19:44:26 +0530, Jagan Teki <jt...@openedev.com> > > wrote: > >> On Wednesday 16 March 2016 07:00 PM, Cyrille Pitchen wrote: > >>> Le 15/03/2016 19:21, Jagan Teki a écrit : > >>>> On Tuesday 15 March 2016 11:42 PM, Cyrille Pitchen wrote: > >>>>> Hi all, > >>>>> > >>>>> This series of patches fixes and extend the support of QSPI memories > >>>>> in the SPI flash framework. The updates are split into many parts to > >>>>> make it easier to understand and review but they should be considered > >>>>> as a whole. > >>>>> > >>>>> This was tested on a Atmel sama5d2 xplained board with a Micron n25q128a > >>>>> memory. > >>>>> > >>>>> Best regards, > >>>>> > >>>>> Cyrille > >>>>> > >>>>> Cyrille Pitchen (18): > >>>>> Revert "sf: Fix quad bit set for micron devices" > >>>>> sf: call spi_claim_bus() and spi_release_bus() only once per read, > >>>>> write or erase > >>>>> sf: replace spi_flash_read_common() calls by spi_flash_cmd_read() > >>>>> sf: remove spi_flash_write_common() > >>>>> sf: export spi_flash_wait_ready() function > >>>>> sf: share erase generic algorithm > >>>>> sf: share write generic algorithm > >>>>> sf: share read generic algorithm > >>>>> sf: add hooks to handle register read and write operations > >>>>> sf: move support of SST flash into generic spi_flash_write_alg() > >>>>> sf: fix selection of supported READ commands for QSPI memories > >>>>> sf: fix detection of QSPI memories when they boot in Quad or Dual > >>>>> mode > >>>>> sf: add helper function to set the number of dummy bytes > >>>>> sf: add 4byte address opcodes > >>>>> sf: prepare next fixes to support of QSPI memories by manufacturer > >>>>> sf: fix support of Micron memories > >>>>> ARM: at91: clock: add function to get QSPI clocks > >>>>> sf: add driver for Atmel QSPI controller > >>>> > >>>> Appreciate for the work, we're working on spi-nor framework[1] planning > >>>> to push in couple of weeks. Will let you know once it merged so that you > >>>> can add your changes on top of that. > >>>> > >>>> [1] > >>>> http://git.denx.de/?p=u-boot/u-boot-spi.git;a=shortlog;h=refs/heads/spi-nor-next > >>>> > >>> > >>> Hi Jagan, > >>> > >>> I've started to have a look on your branch. I hope it's not to late for > >>> few > >>> comments: > >>> > >>> Globally I see the new code attend to match the spi-nor framework from > >>> Linux. > >>> OK that's fine but please note the current spi-nor framework in Linux has > >>> incomplete and sometime not working support of QSPI memories. > >>> > >>> First, after a discussion with Brian and Bean on linux-mtd [1], Bean's > >>> commit > >>> to add support to Micron QSPI memories was reverted since it didn't work > >>> alone. > >>> In his reply, Brian agreed the code was not tested and should not have > >>> been > >>> merged. > >>> > >>> This highlights a more general issue: currently, there is no mean for the > >>> spi-nor framework to notify the SPI controller driver about a SPI protocol > >>> change at the QSPI memory side. This applies to Micron memories when they > >>> enter > >>> their Quad I/O mode. If so, ALL commands, even JEDEC Read ID, Read Status > >>> Register, ..., MUST use the SPI 4-4-4 protocol. Commands sent using SPI > >>> 1-x-y > >>> protocols are no longer decoded properly. > >>> This also applies to Macronix and Winbond memories if they enter their QPI > >>> mode, which is the equivalent of Micron Quad I/O mode. > >>> This is why I've suggested to add 4 new fields in the struct spi_nor: > >>> - .reg_proto: the SPI protocol to be used by .read_reg() and .write_reg() > >>> hooks. > >>> - .read_proto: the SPI protocol to be used by the .read() hooks, maybe by > >>> the > >>> .read_mmap() also. > >>> - .write_proto: the SPI protocol to be used by the .write() hooks > >>> - .erase_proto: the SPI protocol to be used by the .erase() hooks. > >>> > >>> (Q)SPI controller drivers cannot guess the protocol to be used from the > >>> command > >>> op code. Indeed, taking the Micron case as un example, the very same 0xeb > >>> op > >>> code may be used with the SPI 1-4-4 protocol (Micron Extended SPI mode) or > >>> with the SPI 4-4-4 protocol (Micron Quad I/O mode). > >>> > >>> > >>> Also just some words about the naming of SPI x-y-z protocols: > >>> - x refers to the number of I/O lines used to send the op code byte > >>> - y is the number of I/O lines used to send the address, mode/dummy cycles > >>> (if these cycles exist for the command op code) > >>> - z is the number of I/O lines used to send/receive data (if needed) > >>> > >>> So the SNOR_OP_READ_1_1_2_IO macro for the Fast Read Dual I/O command (as > >>> opposed to the macro SNOR_OP_READ_1_1_2 macro for the Fast Read Dual > >>> Output > >>> command) doesn't make sense: it should be named SNOR_OP_READ_1_2_2. > >>> > >>> > >>> Then about the value used for the dummy cycles, it's not always true that > >>> we > >>> don't care about initializing them. Depending on the configuration of the > >>> memory, some special dummy cycles, sometime called mode cycles, are used > >>> to > >>> during Fast Read operations to make the memory enter/leaver its > >>> Continuous Read > >>> mode. Once is Continuous Read mode, the op code byte is no longer sent, > >>> it is > >>> implicit and the command actually starts from the address cycles. This > >>> mode > >>> is mostly used for XIP applications hence is not relevant for mtd usage. > >>> However we should take care not to enter this Continuous Mode by mistake. > >>> Depending on the memory manufacturer, the Continuous Mode can be disabled > >>> by > >>> updating the relevant bit in some configuration register (e.g. setting > >>> the XIP > >>> bit in Micron Volatile Configuration Register) or by choosing the right > >>> op code > >>> (e.g. for Winbond memories in QPI mode, both the 0x0b and 0xeb op codes > >>> can > >>> be used for Fast Read 4-4-4 operation but only the 0xeb op code cares > >>> about > >>> the dummy cycle value to enter/leave the Continuous Read mode). > >>> Some Spansion memories use 6 dummy cycles for Fast Read 1-4-4 command as > >>> factory default, not 8. > >>> > >>> Besides when sending a regular JEDEC Read ID (0x9f) command to probe the > >>> (Q)SPI > >>> memory, the current spi-nor framework assumes the Quad I/O or QPI mode is > >>> not > >>> already enabled. This not always true, some early bootloarders, such as > >>> the > >>> sama5d2 ROM Code, enables the Micron Quad I/O mode when configured to > >>> boot from > >>> the QSPI memory. If so, the QSPI memory no longer replies to the 0x9f > >>> command > >>> in SPI 1-1-1 protocol but instead to the 0xaf command in SPI 4-4-4 > >>> protocol. > >>> > >>> Finally, about the proper way to describe the SPI controller capabilities, > >>> the SPI_TX_{DUAL, QUAD} and SPI_RX_{DUAL, QUAD} mode flags are set in the > >>> SPI framework based on the "spi-rx-bus-width" and "spi-tx-bus-width" DT > >>> properties already used in Linux. > >>> This is not enough to make the difference between the SPI 1-4-4 and SPI > >>> 4-4-4 > >>> protocols. Maybe some SPI controllers support the first protocol but not > >>> the > >>> latest. It could be good to add another argument to spi_nor_scan() > >>> providing > >>> an exhaustive list of SPI protocols supported by the SPI controller. > >>> Then to match this list with the list of SPI protocols supported by the > >>> SPI > >>> memory and select the proper protocol, this new argument should use the > >>> same > >>> range of values as the .flash_read field in the struct spi_nor_info used > >>> to > >>> describe the SPI memories. > >>> > >>> For backward compatibility, the m25p80 driver could then convert the SPI > >>> modes > >>> into spi-nor read modes. Please have a look at patch 11 of my series; the > >>> chunk related to spi_flash_probe_slave() in sf_probe.c: > >>> > >>> /* Convert SPI mode_rx and mode to SPI flash read commands */ > >>> + mode_rx = spi->mode_rx; > >>> + if (mode_rx & SPI_RX_QUAD) { > >>> + e_rd_cmd = RD_NORM | QUAD_OUTPUT_FAST; > >>> + if (spi->mode & SPI_TX_QUAD) > >>> + e_rd_cmd |= QUAD_IO_FAST; > >>> + } else if (mode_rx & SPI_RX_DUAL) { > >>> + e_rd_cmd = RD_NORM | DUAL_OUTPUT_FAST; > >>> + if (spi->mode & SPI_TX_DUAL) > >>> + e_rd_cmd |= DUAL_IO_FAST; > >>> + } else if ((mode_rx & (SPI_RX_SLOW | SPI_RX_FAST)) == SPI_RX_SLOW) { > >>> + e_rd_cmd = ARRAY_SLOW; > >>> + } else { > >>> + e_rd_cmd = RD_NORM; > >>> + } > >>> + > >>> [...] > >>> - ret = spi_flash_scan(flash); > >>> + ret = spi_flash_scan(flash, e_rd_cmd); > >>> > >>> > >>> I've spent a lot of time working on the QSPI memory topic so I can tell > >>> you > >>> that there are many other traps to avoid! > >>> If I can help you on this topic during your rework of the SPI NOR support, > >>> please let me know. > >> > >> I understand your points, thanks for that and anyway this spi-nor work > >> is a starting point for both syncing with Linux as well with new feature > >> or for better tunning. And more over I started this work in 2014 end and > >> it's been reviewing over and over and we finally landed up with MTD > >> driver model. > >> > >> So, please wait for sometime until this gets merged we definitely work > >> together for better tunning, thanks! > > > > If I understand Cyrille's post correctly, it is not about better > > tuning, it is about fixing existing issues, right? I mean, Cyrille > > is talking about situations where the code will be not simply slow, but > > plain wrong, correct? > > > > If so, and since it appears Cyrills's patch series are bug fixes which > > the framework would not properly work if it did not integrate them > > anyway, I would agree with Marek that it makes more sense applying > > Cyrille's patches first. > > OK, if these are bug fixes then what is the issue on working on top of > spi-nor?
I believe you said spi-nor is not going to be submitted again until a couple of weeks, and then there will be a review period; that may not fit 2016.05. Cyrille's fixes could go out of the review phase before spi-nor does, and possibly even be ready for 2016.05. > and more over this series is on ML just now and need a proper > review as well and will take some time too. Yes, but non-structural bugfixes are easier and quicker to test than structural changes (which btw won't be entirely testable until the bug fixes are applied IIUC). > I have planned spi-nor > series for this release better work on this series - thanks! But if spi-nor is released in 2016.05 without Cyrille's patches, there will be known bugs unfixed, right? > -- > Jagan Amicalement, -- Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
