Hello Jagan, Jagan Teki wrote on 2013-09-25:
> Thanks for your comments, see below > > On Wed, Sep 25, 2013 at 1:02 AM, <thomas.lan...@lantiq.com> wrote: >> Hello Jagan, >> >> Am 24.09.2013 20:38, schrieb Jagannadha Sutradharudu Teki: >>> This patch series is a combination of "sf: Add common probe support" >>> "sf: Add support for extended/quad read and write commands" >>> http://www.mail-archive.com/u-boot@lists.denx.de/msg121668.html >>> http://u-boot.10912.n7.nabble.com/PATCH-v2-00-10-sf-Add-support-for- >>> extended-quad-read-and-write-commands-td160964.html >>> >>> This patch series adds common probe support for all flash vendors >>> except ramtron. >>> >>> spi_flash_probe is a new addition where all flash driver >>> probing is combined into a common file, this means spi_flash_probe.c >>> adds a new probing style common to all flashes. >>> >>> >>> Apart from common probing, this series also adds support for >>> extended read commands and quad read/write commands. >>> >>> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/150148 >>> http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/167026 >>> >>> There is a bit discussion going on for supporting this: >>> http://u-boot.10912.n7.nabble.com/PATCH-00-12-cmd-sf-Add-support- >>> for-read-and-write-instructions-td143749.html >>> >>> Concept: Initially I tried to add individual sf write/read commands to >>> respective flash read/write commands, but later some discussion to >>> mainline about this and changed the implementation. >>> >>> As Michal and me were trying to change this as like the >>> "implementation will discover the fastest command by taking the >>> supported commands from flash and a controller. Controller supported >>> commands will always been a priority." >>> >>> Means I have added rd_cmd and wr_cmd params on spi_flash_id_params >>> table. So the flash user may fill these with flash supported commands, >>> and also spi controller use is also have rd_cmd and wr_cmd from spi.h, >>> so the spi user will fill these with controller supported commands. >>> and the resultent command is calculated based fastest commands by >>> taking inputs from spi and flash, but spi(controller) has always a >>> priority. >>> >>> Supported commands: >>> - CMD_READ_ARRAY_SLOW >>> - CMD_READ_ARRAY_FAST >>> - CMD_READ_DUAL_OUTPUT_FAST >>> - CMD_READ_DUAL_IO_FAST >>> - CMD_READ_QUAD_OUTPUT_FAST >>> - CMD_PAGE_PROGRAM >>> - CMD_QUAD_PAGE_PROGRAM >> I miss an important detail in this series, regarding dual/quad-io support: >> How is the spi controller is informed about the transfer details? >> I see only setting the cmds according the features of flash and controller, >> but no additional indication to the spi controller, that this is then a >> dual- >> or quad-io transfer.How the spi controller should know about this??? > > This implementation will discover the fastest command by taking the > supported commands from flash and a controller. Controller supported > commands will always been a priority. > > Means controller driver should have a code to support whether I am > supporting this > new extended read/write or quad command. > > And this support will discover the fastest command by comparing the > commands from flash and spi. > flash anyway we added in params list and controller the respective > controller will initialize spi slave in the driver itself . > > qspi->slave.rd_cmd = READ_CMD_FULL; > qspi->slave.wr_cmd = PAGE_PROGRAM | QUAD_PAGE_PROGRAM; > > Please refer https://github.com/Xilinx/u-boot-xlnx/blob/master- > next/drivers/spi/zynq_qspi.c I had a look at this code. And found exactly, what I expected: A huge table with flash opcodes! Why does a spi controller need to know these details? Because of information about, which part of a message needs single/dual or quad transfers? If we would do it this way, each spi controller needs to implement similar things! >From my point of view, this information should come from the serial flash >driver! There you know details of manufacturer, supported opcodes and so on. And there you can decide, if you switch a flash to a "full quad" mode, that the same opcode now needs to be send on 4 lines! In the controller driver you don't know these details! > > spi_flash layer should send any commands based on the respective > supported, but it is supported by respective spi controller that should > controller driver must aware. > > This is more generic solution as we discussed so-many months ago. > http://u-boot.10912.n7.nabble.com/PATCH-00-12-cmd-sf-Add-support-for- > read-and-write-instructions-td143749.html Yes, I agree that this a huge step forward! But I think, we still have not reached a state which everyone can accept. > > We tried to discover the supported commands runtime from respective > flashes, but ie. going to be a huge > code to decode all these. Instead filling params, like sectors and > idcodes should be an code driven task as per as > u-boot code is concern. As I already said, this is very specific to your controller! > >> By decoding the cmd itself again? But the data should be a transparent byte >> stream to the controller! >> Otherwise you need a table of commands for decoding also inside the >> controller >> driver, which I consider a bad idea, as you need to update them (in each >> driver) >> for new cmds added to the serial-flash driver! >> >> In the linux kernel, the solution in the spi layer was to add new >> elements to >> the transfer structure (struct spi_transfer -> bitwidth), which can be set >> for each part of a message. >> In u-boot we have the spi_xfer function, which has a "flags" parameter >> we could >> use for this. >> >> As long as the details for this (including a spi controller driver, >> which can handle dual/quad-io transfers) are not fixed, I would suggest >> to leave the patches regarding the extended cmds out of the series of >> sf-cleanup (which really looks good!) and fix the issues until the next >> merge window! > > I am planning to push this series, as we started this since long months back. > We discussed many threads, and tested this approach on read hw as well. > > Please let me know for any thoughts. I already said: Please leave some room for more discussions on the extended / quad read topic! And don't let the rest of the improvements be delayed by this! > >> >>> Testing repo branch: >>> ------------------- >>> $ git clone git://git.denx.de/u-boot-spi.git >>> $ cd u-boot-spi >>> $ git checkout -b master-next-test origin/master-next-test >>> >>> REQUEST FOR ALL SPI CODE CONTRIBUTORS/USERS, PLEASE TEST THESE CHANGES >>> W.R.T YOUR HW IF POSSIBLE. >>> >>> Please let me know for any issues/concerns/questions. >>> >>> -- >>> Thanks, >>> Jagan. >> Best Regards, >> Thomas >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot > Best Regards, Thomas _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
