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

[Jagan Teki]

Hi Marek,

On Fri, Jan 17, 2014 at 12:34 AM, Marek Vasut <marex at 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 at 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.
>> >
>> > [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.

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

-- 
Thanks,
Jagan.