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

[Marek Vasut]

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.

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 {} ?

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.

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

Best regards,
Marek Vasut