Hi Marek,
On Fri, Jan 17, 2014 at 12:34 AM, Marek Vasut marex@denx.de wrote:
On Thursday, January 16, 2014 at 07:06:20 AM, Jagan Teki wrote:
Hi Marek,
On Thu, Jan 16, 2014 at 1:08 AM, Marek Vasut marex@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.
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 agreed that the current sf code is touching spi api because we followed the same approach since so-far. 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.
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.
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.
My plan is to do this new framework addition for next release and wrap this code once all gets set.
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.
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.
Summary: Will try to add SPI-NOR framework parallel and will strip the spi_flash stuff one by one from SPI API