[U-Boot] [PATCH v3] sf: Add auto detection of 4-byte mode (vs standard 3-byte mode)

[Stefan Roese]

Hi Rajat,

On 25.10.18 11:28, Rajat Srivastava wrote:

<snip>

>>> Simply adding SPI_FLASH_USE_SFDP flag is not enough to make SFDP work.
>> You'll
>>> need to add the driver code corresponding to the mtd layer code (in
>> spi_flash.c)
>>> which will send the actual READ SFDP command to flash.
>>>
>>> The patch-set I floated adds driver code in fsl_qspi.c (Freescale/NXP QSPI
>> driver).
>>> Please find the patch at
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F985329&data=02%7C01%7Crajat.srivastava%40nxp.com%7Cc967de2c68464c6561b508d639091e21%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636759108670133878&sdata=uZfP5lWvGmpA%2Fxi5AS06c15PKqYwwndgU%2BC3zYu2K3w%3D&reserved=0.
>>>
>>> After the mtd layer calls spi_flash_read_common() function to send any read
>>> command to flash, it lands on driver which ultimately fires the command
>>> (in this case 0x5A command to read SFDP) to flash.
>>
>> So you say, that each SPI driver needs to get some changes to
>> support the SFDP reading? That does not sound like a generic
>> approach to me. But maybe I misunderstood this.
> 
> To read SFDP parameters, a READ_SFDP command (0x5A) needs to be
> sent to flash which can be sent only with help of a SPI driver.
> Even if one wants to initiate a simple basic 1-byte read operation,
> there must exist a SPI driver that actually sends the read command
> to flash.

But does this READ SFDP stuff need to be handled in each and every
SPI driver (platform specific)? Can't this be moved to a common
location in the SPI flash framework so that the platform specific
drivers don't need to be changed here?
  
> I think this is a generic approach because that is how mtd
> framework is designed.
> 
>>
>>> Since you say the flash is designed to support only 4-byte addressing mode
>>
>> No. The flash itself (as you have seen in the datasheet) supports
>> both. But the special chip version equipped on the LinkIt MT7688
>> boards is somehow strapped to only support 4-byte mode. This is
>> not documented anywhere (and did cost me quite some time to figure
>> it out).
> 
> Ok I get it. I think this flash does not follow standard SFDP
> framework. Can you please confirm?

This specific flash chip most likely not. But I'm not 100% sure,
as I've never tried to use it this way yet.
  
>>
>>> then it is possible that the READ_SFDP command (0x5A) is also required to be
>>> sent in 4-byte mode (My patch sends 0x5A in 3-byte addressing mode which
>>> is also the SFDP standard that every other flash supports).
>>> Although I looked up the datasheet of mx25l25635f and it says that the
>>> READ_SFDP command will be sent in 3-byte mode (as supported by my patch).
>>>
>>>>
>>>> SF: Detected mx25l25635f with page size 0 Bytes, erase size 64 KiB, total 0
>> Bytes
>>>> *** Warning - bad CRC, using default environment
>>>>
>>>> Please note that I'm not opposed to using your SFDP support. But
>>>> in our case it does not work - at least not without the
>>>> SPI_FLASH_USE_SFDP addition. And it needs some fixing as well to
>>>> fully detect the chip parameters. So I would prefer to go ahead
>>>> with my patch for now.
>>>
>>> My point is that if your case can be handled by adding a generic code,
>>> instead of flash specific code then we should prefer the generic approach,
>> what
>>> do you say?
>>
>> Please see above, my questions about SPI driver additions for this
>> SFDP support. This does not sound very "generic" to me.
> 
> As mentioned above, this is the generic approach and this is how
> Linux has also implemented SFDP. By design, the SPI driver should
> adapt to changes in mtd framework.

Again my question from above, if this SFDP read needs to be
added to all SPI device drivers, or if this can be handled
in a common place?
  
>>
>> But I think both solutions, your SFDP support and my "simple"
>> pre-configured 3-byte vs 4-byte addressing mode detection and
>> usage can co-exist.
> 
> Ok.
> Let us wait for the SPI maintainer to comment on this since for
> both solutions to co-exist we may have to resolve code conflicts.

Sure. Resolving such potential merge conflicts should not be too
hard though.

Thanks,
Stefan