From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Sat, 18 Jan 2014 21:26:30 +0100 Subject: [U-Boot] Pull request: u-boot-spi/master In-Reply-To: References: <201401162004.10366.marex@denx.de> Message-ID: <201401182126.30489.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 :)