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

[thomas.langer at lantiq.com]

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!

Please look up similar discussions on the linux-mtd and linux-spi mailing lists!

> --
> Thanks,
> Jagan.

Best Regards,
Thomas