[U-Boot] [PATCH v4 00/36] sf: Add common probe and extended/quad read/write cmds support

Wed Sep 25 13:30:06 CEST 2013

On Wed, Sep 25, 2013 at 4:43 PM,  <thomas.langer at lantiq.com> wrote:
> 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.langer at 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!

I am unable to understand your point here!.

There is no changes w.r.t to respective controller driver for this
support except if the specific driver support
then fill the supported commands.

discovering the fastest command and send spi_xfer is a job of spi_flash.

>
> 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 at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
> Best Regards,
> Thomas
>
>

-- 
Thanks,
Jagan.
--------
Jagannadha Sutradharudu Teki,
E: jagannadh.teki at gmail.com, P: +91-9676773388
Engineer - System Software Hacker
U-boot - SPI Custodian and Zynq APSOC
Ln: http://www.linkedin.com/in/jaganteki