[U-Boot] [PATCH 00/12] cmd_sf: Add support for read and write instructions

[Langer Thomas (LQDE RD ST PON SW)]

Hello Jagan,

Jagan Teki wrote on 2013-01-16:

> Hi Thomas,
> 
> On Thu, Jan 10, 2013 at 8:03 PM, Langer Thomas (LQDE RD ST PON SW)
> <thomas.langer at lantiq.com> wrote:
>> Hello Jagan,
>> 
>>> All these patches are added a support for read and write instruction
>>> for programming/reading SPI flash.
>> 
>> I have written some weeks ago that I would really appreciate the
>> support of dual and quad IO accesses for serial flashes. I just  think,
>> this is not an acceptable way to do this.
>> 
> 
> First of all thank you very much for your comments.
> 
>> It is important to know for all, who do not know the details on the hardware
>> requirements of this feature, that these new transfers require a special SPI
>> controller! A standard SPI hardware, which is being used on most (maybe all)
>> existing boards, cannot use the dual or quad IO features at all!
> 
> I am some how unclear about your  concern.
> 
> Basically I have added this code that is purely specific to mtd flash
> layer, means
> the entire code is alters the functionalists of flash parts(stmicro,
> spansion, winbond..etc).
> 
> whether the respective SOC SPI/QSPI controller driver can support
> these transfer or not, may be
> we couldn't take care at this point of time.
> 
> Please comment if I am wrong with my analyses.

You are right, the MTD/SF layer does not know this. But in your board config you are
able to set a compile option to enable this feature.

It is an important rule for U-Boot, that new code must be added in a way that the code
size is not increased as long as the feature is disabled!

See here (Note 4): http://www.denx.de/wiki/view/U-Boot/Patches#Notes
In the current form, your patches increase the code size for all boards with serial flash!

> 
>> 
>> And in addition, I still haven't seen any change, which indicates the required
>> transfer mode to the SPI layer! How do you tell your SPI driver, which part
>> of the transfer should be done in dual or quad mode?
> 
> I don't think so, but the SPI/QSPI controller  driver should aware
> respective read/write/erase instructions
> transfer changes those are trying to pass from mtd flash. .
> 
> Suppose if the flash used QUAD PP , then the respective h/w SPI/QSPI
> controller must have a support to do the QUAD PP transfers (by adding
> QUAD_PP command transfer).
> 
> I think this job is specific to respective h/w driver, they need to
> add these transfers support.
> currently I have used a zynq qspi controller for testing these
> instruction by adding respective
> instructions are this driver.
> 
> http://git.xilinx.com/?p=u-boot-
> xlnx.git;a=blob;f=drivers/spi/zynq_qspi.c;h=642241d0fc25d04c06ecbeb5d01ba
> 0 6108307c62;hb=master
> 

I don't think, this is a good idea! You are mixing the SPI driver with details of the flashes!
What happens, if you connect something, which is not a flash, to this SPI controller?
And then try to send data, which is detected as "qpp" or any other quad/dual io instruction?
I assume, this will not work!

I would expect the SPI as transparent interface. The SPI driver should provide an interface
to specify the use of 1/2/4 signals for a transfer or part of it.
And the flash driver then should use this extended interface to specify the required parts,
as it is his job!

For example, with your driver, it would be difficult to support the Macronix MX25L12835F,
which allows to enable their "QPI" mode, which then expects some command, with the
same opcode as in "None-QPI" mode, have to be send in parallel.
And Macronix is also known to use different opcodes than others for some instructions.
Does this mean, for supporting this flash, you would have to extend your SPI driver first?

I don't expect something like this to be accepted, either in u-boot nor in the kernel.
Your driver has some complex handling inside for things, which are only valid for
serial flashes! Please don't do this!

Please define and use a clear interface, where the flash driver can tell the spi driver,
what kind of transfer should be used (1/2/4) for which phase (command/address/data).
In u-boot, I would use the "flags" argument of the "spi_xfer" function for this.

I would assume, you will get more flexibility, as your spi driver will get smaller and less
complex and it does not need to be changed, if some other flash type should be supported.
This will then be part of the specific flash driver! (you see??)

>> 
>>> 
>>> Read and Write instruction are implemented as a command line
>>> arguments for 'sf write' , 'sf read' and 'sf update' commands.
>>> 
>>> Currently I have added below instructions those are commonly available
>>> on all flash types.
>>> pp - Page Program (existing one)
>>> qpp - Quad-input Page Program
>>> afr - Array Fast Read (existing one)
>>> asr - Array Slow Read
>>> dofr - Dual Output Fast Read
>>> qofr - Quad Output Fast Read
>>> diofr - Dual IO Fast Read
>>> qiofr - Quad IO Fast Read
>>> 
>>> I have tested mostly of the instruction on real h/w.
>>> 
>>> This entire implementation will change the current sf framework little
>>> bit but I thought these changes are worth to add.
>> 
>> This means, all your patches adding new code, which has no benefit for
>> existing boards, but changing the commands in an incompatible way,
>> which forces all users to adapt their definitions! I don't think this
>> will be accepted, as I would not.
>> 
>> The biggest question is: Do somebody really need the flexibility to
>> select the used instruction at this level? If you have a platform,
>> which contains an extended SPI controller and has also a supported
>> flash, I expect it would be okay to always use the dual or quad
>> instructions! And this can be completely handled by the lower level
>> functions, no need to expose this to the command line!
> 
> Yes, basically if some don't want/they don't support the respective
> instruction at their h/w or
> QSPI/SPI controller level certainly they couldn't use.
> instead they may use the respective supported instructions at command
> level, I suspect this could be
> a selective features from users point of view.
> 
>> 
>> I think, first of all, you should add some config options (e.g.
>> CONFIG_SYS_SF_DUAL / CONFIG_SYS_SF_QUAD) to indicate, that the platform
>> / board will support these. Then, during "sf probe", there should be a
>> detection, if the flash will support this (depending on the type) and
>> is enabled (checking the config bit for quad IO). Depending on this,
>> either specific functions for read/write could be assigned or some
>> other data, which indicate the usable instructions, can be set.
> 
> May be your taking about run-time detection of specific instruction
> supported by respective flash.
> Actually i don't have any idea how to detect whether the flash
> supports  pp, qpp, fr, sr, dor, qor?
> 
> Please pass any information if there is a flexible way to detect this feature.

I expect, that these flashes either have a specific ID and some flags can be added to the
tables, which are used to identify them already. Or some newer flashes might support
the JEDEC Standard JESD216: "Serial Flash Discoverable Parameters (SFDP)"?

For the quad-io mode, the corresponding bit from the configuration register should 
be used!

> 
> But currently these(pp, qpp, afr, asr, dor, qor, dior, qior)
> instructions are commonly supported once from
> spansion, numonyx and winbond flashes.this is the reason for me to
> give these instructions support at this point of time.

But it needs a spi driver with specific adaption to these commands,
instead of a generic interface, which can be adapted for other spi drivers also.

> 
> Thanks,
> Jagan.
> 

Best Regards,
Thomas