On Wed, Sep 25, 2013 at 3:34 PM, <thomas.lan...@lantiq.com> wrote:
> Hello Jagan,
>
> Jagan Teki wrote on 2013-09-25:
>> On Wed, Sep 25, 2013 at 3:14 PM, <thomas.lan...@lantiq.com> wrote:
>>> Hello Jagan,
>>>
>>>> From: Jagan Teki [mailto:jagannadh.t...@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-
>>>> b...@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.lan...@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;
}
>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.
>
> 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
>
>
--
Thanks,
Jagan.
--------
Jagannadha Sutradharudu Teki,
E: jagannadh.t...@gmail.com, P: +91-9676773388
Engineer - System Software Hacker
U-boot - SPI Custodian and Zynq APSOC
Ln: http://www.linkedin.com/in/jaganteki
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
