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

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 .

...

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.

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?

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

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 :)