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

[Marek Vasut]

On Thursday, January 16, 2014 at 08:44:55 PM, Jagan Teki wrote:
> Hi Marek,
[...]
> >> 
> >> 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.
> > 
> > This patchset does not do this cleanup you describe here. This patchset
> > adds (dead) code to support SPI-NOR controllers via regular SPI API .
> 
> I don't know what you mean by dead code here

I mean all those new flags added to struct spi_slave {} for example. They have 
no user in mainline U-Boot. CONFIG_SF_DUAL_FLASH is not used by anyone in u-
boot/master either.

> I agreed that the current sf code is
> touching spi api because we followed the same approach since so-far.

What does that mean ? The SF code must not ever touch the SPI API, the SPI 
already provides everything the SPI flash can hope for (that is, means to 
send/receive data on/from the bus AND position of the SPI devices (bus # and 
chipselect # )). The approach to extend SPI API for one particular type of SPI 
device is wrong.

> we're using this since so far and honestly if you compare this with newly
> added SPI-NOR framework in Linux - ends defiantly have a side effects.

What kind of side-effects ? The SPI API is different from SPI-NOR controller API 
and we _must_ keep that in mind. The later is in no set relationship to the 
former ; they're neither subset nor a superset, they barely even intersect.

> The reason why I am not agree with this not a dead-code is like even
> if we have a new
> SPI-NOR framework these should be part of spi-nor code i guess.

The actual device-model idea here is as such:

I) The 'sf' command talks to the SF layer and informs it about:
   1) operation to be performed (read/write/erase)
   2) device credentials (bus number and chipselect number)

II) The SF layer figures out the correct struct spi_slave {} based on the 
information passed from the SF command above.

III) The SF layer talks using SPI API to make the SPI controller send/receive 
data to/from device identified by struct spi_slave {}.


To incorporate the new SPI-NOR controllers, the SF layer would need to be 
adjusted.

Step II) would need to be changed such, that an appropriate conversion to either 
struct spi_slave {} or struct spi_nor_device {} would happen depending on which 
kind of connection and API would the SPI flash device use -- whether it is 
generic SPI or specific SPI-NOR.

Step III) would then need to be adjusted such that depending on the controller 
type retrieved in step II), one of the APIs (generic SPI or SPI-NOR) would be 
used.

The adjustment really isn't hard at all ;-)

> I totally agree that if we follow the new SPI-NOR will answer all
> these side effects.
> And also SPI-NOR is not yet ML'ed I am currently understanding this.

Ok, so that's why it's a lot of dead code ;-)

> My plan is to do this new framework addition for next release and wrap this
> code once all gets set.

I would suggest we start heading for the SPI NOR split. Seriously, hit the 
brakes here, otherwise we're going for an unpleasant ride through a sh**storm 
;-)

> >> > 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.
> > 
> > Sorry, I don't understand what you're telling me here.
> > 
> > btw. the struct spi_slave {} has grown quite significantly , it contains:
> > 
> > u8 op_mode_rx
> > u8 op_mode_tx
> > 
> >  -> SPI-NOR controllers' bus caps (like, can it do 4-bit transfer etc.),
> >  but
> > 
> > this is SPI-NOR _controller_ specific, what is this stuff doing in struct
> > spi_slave {} ? btw. /wrt placement of these new entries, you must read
> > [1], since you just generated 2-byte slop.
> > 
> > void *memory_map
> > 
> >  -> this is clearly SPI-NOR controller specific stuff, which cannot be
> >  used by
> > 
> > any other generic SPI peripheral.
> 
> This get added before - Yes will address all one by one soon.
> 
> > u8 options
> > 
> >  -> Quite unclear what this is for.
> > 
> > u8 flags
> > 
> >  -> DTTO.
> > 
> > [1] http://www.catb.org/esr/structure-packing/
> > 
> >> > 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.
> > 
> > I don't see any API documentation there, sorry. Can you point me to such
> > an API documentation or design document or something please ?
> > 
> >> > 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.
> > 
> > Oh, there is an alternative (see [1] above, the spi-nor approach) and we
> > should take the right direction instead of pushing in the wrong one.
> > 
> >> 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.
> > 
> > OK, but this doesn't justify pushing broken code which will bite us in
> > the future.
> > 
> >> 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.
> > 
> > OK, I would still prefer to get this release out _without_ the strange
> > additions to struct spi_slave {}. Is it possible to strip away all those
> > unused quad-spi additions for this release?
> 
> as your point -
> spi_slave {} not only have quad additions and also have memory_map and etc
> stuff and will strip all these one by one with new SPI-NOR framework.

OK, but what about the mess in this release ?

> Summary: Will try to add SPI-NOR framework parallel and will strip the
> spi_flash stuff
> one by one from SPI API

Thanks. Please keep me in CC so I can keep nagging :)