[U-Boot] [PATCH v4 31/36] sf: Add extended read commands support
thomas.langer at lantiq.com
thomas.langer at lantiq.com
Wed Sep 25 23:40:21 CEST 2013
Hello Jagan,
Jagan Teki wrote on 2013-09-25:
> On Wed, Sep 25, 2013 at 3:34 PM, <thomas.langer at lantiq.com> wrote:
>> 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).
>
> "implementation will discover the fastest command by taking the supported
> commands from flash and a controller. Controller supported commands
> will always been a priority."
>
> From SPI controller:
> ----------------------------
> spi_setup_slave() {
> spi = spi_alloc_slave(struct zynq_spi_slave, bus, cs);
>
> spi->slave.rd_cmd = RD_CMD_FULL;
> spi->slave.wr_cmd = WR_CMD_FULL;
> }
These are already FLASH definitions!
Please define it the other way round:
Add properties for number of rx and tx lines to the spi-slave.
(Do you notice the different naming?)
>
>
> From SPI FLASH: (drivers/mtd/spi/spi_flash_probe.c)
> -----------------------------------------------------------------------------
> /* Look for the fastest read cmd */
> cmd = fls(params->rd_cmd & flash->spi->rd_cmd);
> if (cmd) {
> cmd = spi_read_cmds_array[cmd - 1]; flash->read_cmd =
> cmd; } else { /* Go for controller supported command */
> flash->read_cmd = CMD_READ_ARRAY_FAST;
> }
>
> /* Look for the fastest write cmd */
> cmd = fls(params->wr_cmd & flash->spi->wr_cmd);
> if (cmd) {
> cmd = spi_write_cmds_array[cmd - 1]; flash->write_cmd =
> cmd; } else { /* Go for controller supported command */
> flash->write_cmd = CMD_PAGE_PROGRAM;
> }
> include/spi_flash.h:
> ---------------------------
> /* Supported write cmds enum list */
> enum spi_write_cmds {
> PAGE_PROGRAM = 1 << 0,
> QUAD_PAGE_PROGRAM = 1 << 1,
> };
> #define WR_CMD_FULL PAGE_PROGRAM | QUAD_PAGE_PROGRAM
>
> /* Supported read cmds enum list */
> enum spi_read_cmds {
> ARRAY_SLOW = 1 << 0,
> ARRAY_FAST = 1 << 1,
> DUAL_OUTPUT_FAST = 1 << 2,
> DUAL_IO_FAST = 1 << 3,
> QUAD_OUTPUT_FAST = 1 << 4,
> };
> #define RD_CMD_FULL ARRAY_SLOW | ARRAY_FAST |
> DUAL_OUTPUT_FAST | \
> DUAL_IO_FAST | QUAD_OUTPUT_FAST
>
> If the controller is not filling slave.rd_cmd and slave.wr_cmd if it's
> not supporting even if connected flash
> supports, fill the read/write commands to default ones.
>
> Controller is a master to decide which command is to use, and flash is
> slave to transfer the discovered command through spi_xfer(), so the
> controller will take care of the command processing.
>
> This is well detailed explanation, I hope you understand.
I understand, but need to repeat:
All this is flash specific and should not be part of the generic spi
interface layer!
For spi, you have number of data lines, which is independent of the cmd
to transfer!
And what I am missing is the information about the lines to use when calling
spi_xfer(). In your implementation, the controller itself needs to know
this from decoding the opcodes.
How do you intend to handle this in the future?
Let's assume you want to add support for a new flash, which needs
a different opcode and/or has different requirements for dummy cycles.
Then you add this to the spi_flash driver and to ALL controller driver,
which have been integrated until then?
I think, all this should be handled only in one place, which is the
spi_flash!
If you rely on a table with flash opcodes in the controller driver, you
have a too strong dependency!
I want to avoid this from the beginning, that's why I am so persistent
on this topic!
Please try to do a step back and ignore the internal details of your
zync spi controller.
Then think about the details the spi_flash should provide to the spi
controller
for a specific transfer:
Example read for S25FL129P (no preference, just the first I found):
For QOR (Quad Output Read Mode)
- send opcode, address and dummy byte on SI / IO0 only
- receive data on IO0-IO3 in parallel
and for QIOR (Quad I/O High Performance Read Mode)
- send opcode on SI / IO0
- send address and dummy byte on IO0-IO3 in parallel
- receive data on IO0-IO3
With your approach, the controller needs to know both opcodes and the
details, when to switch from single to quad mode.
I suggest this:
- spi_xfer for cmd with SPI_XFER_BEGIN
- spi_xfer for addr and dummy with 0 or "SPI_XFER_QUAD", depending on
opcode above
- spi_xfer for data with SPI_XFER_QUAD (and maybe SPI_XFER_END)
The same new flags (SPI_XFER_QUAD, SPI_XFER_DUAL) could be used to
indicate the features of the spi controller.
This allows to provide every information of the transfer from spi_flash
and don't rely on additional information in the controller part.
>
>>
>> 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
More information about the U-Boot
mailing list