[U-Boot] [PATCH v4 31/36] sf: Add extended read commands support

[thomas.langer at lantiq.com]

Hello Jagan,

Jagan Teki wrote on 2013-09-25:
> On Wed, Sep 25, 2013 at 3:14 PM,  <thomas.langer at lantiq.com> wrote:
>> Hello Jagan,
>> 
>>> From: Jagan Teki [mailto:jagannadh.teki at gmail.com] Sent: Wednesday,
>>> September 25, 2013 11:36 AM To: Langer Thomas (LQDE RD ST PON SW) Cc:
>>> Jagannadha Sutradharudu Teki; Tom Rini; jaganna; u-
>>> boot at lists.denx.de; Todd Legler (tlegler); Willis Max; Syed Hussain;
>>> Sascha Silbe Subject: Re: [U-Boot] [PATCH v4 31/36] sf: Add extended
>>> read commands support
>>> 
>>> On Wed, Sep 25, 2013 at 1:40 AM,  <thomas.langer at lantiq.com> wrote:
>>>> Hello Jagan,
>>>> 
>>>> Am 24.09.2013 20:36, schrieb Jagannadha Sutradharudu Teki:
>>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>>>> index ea39d1a..0ac9fab 100644
>>>>> --- a/drivers/spi/spi.c
>>>>> +++ b/drivers/spi/spi.c
>>>>> @@ -7,6 +7,7 @@
>>>>>  #include <common.h>
>>>>>  #include <malloc.h>
>>>>>  #include <spi.h>
>>>>> +#include <spi_flash.h>
>>>>> 
>>>>>  void *spi_do_alloc_slave(int offset, int size, unsigned int bus,
>>>>>                        unsigned int cs)
>>>>> @@ -20,6 +21,7 @@ void *spi_do_alloc_slave(int offset, int size,
>>> unsigned int bus,
>>>>>               slave = (struct spi_slave *)(ptr + offset);
>>>>>               slave->bus = bus;
>>>>>               slave->cs = cs;
>>>>> +             slave->rd_cmd = CMD_READ_ARRAY_FAST;
>>>> 
>>>> This is SPI code, not SF! The spi layer should not know such details of
>>>> the slave!
>>>> What about EEPROMs or other SPI slaves, which are NOT spi_flash???
>>>> Examples (just searched for includes of "spi.h"):
>>>> board/bf527-ezkit/video.c
>>>> drivers/net/enc28j60.c
>>>> 
>>>> Please ensure that the layers are not mixed up!
>>>> SPI is an interface type and SF is ONLY ONE user of this interface!
>>> 
>>> I understand your concern, but here the point is for discovering the
>>> command set.
>>> slave->rd_cmd = CMD_READ_ARRAY_FAST;
>>> is a default controller supported fast read.
>>> 
>>> spi_flash layer will discover the respective rd_cmd based slave and
>>> flash, if slave doesn't have any commands to list, means not support
>>> extended/quad then these fields are filled in spi.c
>>> 
>>> there is nothing harm for respective drivers or code.
>> 
>> But I think this is the wrong approach!
>> Why should the spi controller care about, what devices type is connected?
>> And the "CMD" is a SF specific thing!
>> I agree, that the controller should provide information about his "features",
>> but this should only be something like "tx_width=4" and "rx_width=4",
>> which would tell the SF layer that quad-io is possible!
>> 
>> No details of serial-flashes are necessary for this!
> Yes, no issues.
> I can directly assign to flash side while discovering commends.

I don't understand this sentence. Do you mean "commands"?
Who will "discover" the commands?

The SPI controller does not know about the meaning of commands!
The controller only knows "I must send this block of data and split it on 1/2/4 lines"
(or similar for other transfers).

Maybe your specific controller behaves in another way, but this is not the standard
and you should not force this into the interface.

And another doubt: The might be different commands for dual/quad read/write,
depending on the flash manufacturer. With your solution, the controller drivers
would need these details in advance! Or at least need to be updated on each new
command, which needs to be supported.

> --
> Thanks,
> Jagan.

Best Regards,
Thomas