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 <ma...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
