[U-Boot] Pull request: u-boot-spi/master

[Jagan Teki]

Hi Marek,

On Thu, Jan 16, 2014 at 1:08 AM, Marek Vasut <marex at denx.de> wrote:
> On Monday, January 13, 2014 at 08:42:18 PM, Tom Rini wrote:
>> On Tue, Jan 14, 2014 at 12:05:32AM +0530, Jagannadha Sutradharudu Teki wrote:
>> > Hi Tom,
>> >
>> > PR have quad and dual_flash change set also includes few fixes.
>> > Tested these changes on spansion, stmicro and sst flash devices.
>> >
>> > --
>> > Thanks,
>> > Jagan.
>> >
>> > The following changes since commit 7f673c99c2d8d1aa21996c5b914f06d784b080ca:
>> >   Merge branch 'master' of git://git.denx.de/u-boot-arm (2014-01-10
>> >   10:56:00 -0500)
>> >
>> > are available in the git repository at:
>> >   git://git.denx.de/u-boot-spi.git master
>> >
>> > for you to fetch changes up to 35a55fb57fffb615e6b20980fb317e162076adb4:
>> >   sf: params: Removed flag SECT_4K for Micron N25Q128 (2014-01-12
>> >   21:40:23 +0530)
>> >
>> > ----------------------------------------------------------------
>> >
>> > Axel Lin (1):
>> >       spi: sh_spi: Use sh_spi_clear_bit() instead of open-coded
>> >
>> > Jagannadha Sutradharudu Teki (17):
>> >       sf: Add extended read commands support
>> >       sf: Add quad read/write commands support
>> >       sf: ops: Add configuration register writing support
>> >       sf: Set quad enable bit support
>> >       sf: probe: Enable RD_FULL and WR_QPP
>> >       sf: Separate the flash params table
>> >       sf: Add QUAD_IO_FAST read support
>> >       sf: Discover read dummy_byte
>> >       sf: Add macronix set QEB support
>> >       sf: probe: Enable macronix quad read/write cmds support
>> >       sf: Divide flash register ops from QEB code
>> >       sf: Code cleanups
>> >       sf: ops: Unify read_ops bank configuration
>> >       sf: Add dual memories support - DUAL_STACKED
>> >       sf: Add dual memories support - DUAL_PARALLEL
>> >       sf: Add CONFIG_SF_DUAL_FLASH
>> >       doc: SPI: Update status.txt
>
> I looked into this patchset and this seems completely misdesigned, sorry.
No issues - OK.

Let me explain the journey with (spi_flash)sf since last 8 months. [1]
- We have a individual class of vendor drivers and removed all vendor specific
  stuff and added a common probe.
- Added Bank addr reg stuff
- Tunned sf almost seems like mtd/nand style where sf.c, sf_probe.c,
sf_params.c and sf_ops.c
- Added memory_mapped and quad commands supports
- Done many of cleanups
- maintained doc/SPI which we're trying to update.

Keeping these enhancements on current sf we are in a good shape than before.
>
> It seems this patchset aims to accomodate an SPI-NOR controllers within the SPI
> API. The SPI-NOR controllers are a completely separate class of hardware though,
> so I disagree with the attempt to integrate them into the SPI framework. I
> cannot use most of the SPI-NOR controllers to drive regular SPI bus (there are
> exceptions), but they are now presented as regular SPI controllers
> indiscriminately.
>
> Instead of going down this path, there should be a completely separate class of
> drivers for the SPI-NOR controllers, same as in Linux [1]. That way, there would
> still be an SF command talking to SF core, but the SF core would be delegating
> the calls to either a layer talking to a SPI flash via regular SPI interface or
> a layer talking the SPI-NOR controller.
>
> I also see some flaws in these patches. For example the struct spi_slave {} now
> contains all kinds of new entries used only by the SPI flash driver. The slave
> can now export for example SPI_OPM_RX_QOF and SPI_OPM_RX_QIOF (see
> include/spi.h) flags, which -- if you inspect drivers/mtd/spi/sf_probe.c and
> include/spi_flash.h -- should match up with enum spi_read_cmds . So we now have
> two sets of flags, which needs to be kept in sync, which is wrong. Besides,
> these flags define the capabilities of the SPI-NOR host controller, so why are
> they even in struct spi_slave {} ?

The spi_slave grown with flash stuff with spi driver terminologies,
and the reason
for taking one extra flag for reads in params is like we have couple
of  commands
for 1, 2 and 4-lines I have given a spi driver has a provision to
verify these one by one.
The reason for going this implementation for improving sf performance.
>
> I also observe a big lack of documentation for all those flags, so it's really
> hard to make heads or tails of how it's supposed to work.

Some how disagree this, because we have started doc/SPI [2] these days
which don't have
before and I'm stressing patch contributors to add as many as doc and
test-cases logs.

and Yes- for this quad I'm planning to add test-case logs once our
zynq qspi is ML.
>
> Also, I don't see any of these new flags used yet, so we're still at a good
> point to avoid going down the wrong path. I would love to see this patchset
> postponed for next if possible, since my feeling of this is it needs severe
> redesign.
>
> [1] http://www.spinics.net/lists/arm-kernel/msg291891.html

And finally - I do understand your comments and agreed that we're
tunning spi framework
towards spi_flash, but the current implementation looks like that and
there is no alternatives
as of now.

It was almost 9 months to spent quad changes to fit into on current
code, for this
I tunned spi_flash as sf to more convient to add extra add-ons, i
guess many of the customers
wants this quad since last year.

I agree that if we have a better framework which will divide spi and
spi_flash separately
like what you said with Linux SPI-NOR and it's good to have that. and
also you're comparing
the current sf stuff with this new approach, Yes - i agree that new
approach will defiantly have a
better view than the current.

But, honestly as of now we're planning to move like this. and I am adding
this new framework approach to my TODO list - Will post one more thread for
this implementation and planning.

[1] http://git.denx.de/?p=u-boot.git;a=blob;f=doc/SPI/status.txt;h=13889f54557cb04b2c011774ff3cace091a50e74;hb=master
[2] http://git.denx.de/?p=u-boot.git;a=tree;f=doc/SPI;h=1464e1bad94f36606d46bca3b45733b8aa1e722d;hb=master

-- 
Thanks,
Jagan.