[U-Boot] [PATCH 2/6] sf: Optimize flash features code

[Jagan Teki]

Hi Marek,

On Tue, Jan 21, 2014 at 4:31 AM, Marek Vasut <marex at denx.de> wrote:
> On Monday, January 20, 2014 at 02:32:27 PM, Jagan Teki wrote:
>> On Mon, Jan 20, 2014 at 6:49 PM, Marek Vasut <marex at denx.de> wrote:
>> > On Saturday, January 18, 2014 at 09:51:56 PM, Jagan Teki wrote:
>> >> On Sun, Jan 19, 2014 at 2:09 AM, Marek Vasut <marex at denx.de> wrote:
>> >> > On Saturday, January 18, 2014 at 09:06:29 PM, Jagannadha Sutradharudu
>> >> > Teki
>> >> >
>> >> > wrote:
>> >> >> - Shrink spi_slave {}
>> >> >> - Shrink spi_flash_params {}
>> >> >> - Documentation for sf features
>> >> >>
>> >> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna at xilinx.com>
>> >> >> Cc: Marek Vasut <marex at denx.de>
>> >> >> ---
>> >> >>
>> >> >>  doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
>> >> >>  drivers/mtd/spi/sf.c          |   4 +-
>> >> >>  drivers/mtd/spi/sf_internal.h |   1 -
>> >> >>  drivers/mtd/spi/sf_ops.c      |   8 +-
>> >> >>  drivers/mtd/spi/sf_params.c   | 172
>> >> >>
>> >> >> +++++++++++++++++++++--------------------- drivers/mtd/spi/sf_probe.c
>> >> >>
>> >> >>  71 ++++++++---------
>> >> >>  include/spi.h                 |  42 ++++-------
>> >> >>  include/spi_flash.h           |  24 +++---
>> >> >>  8 files changed, 270 insertions(+), 174 deletions(-)
>> >> >>  create mode 100644 doc/SPI/README.sf-features
>> >> >>
>> >> >> diff --git a/doc/SPI/README.sf-features b/doc/SPI/README.sf-features
>> >> >> new file mode 100644
>> >> >> index 0000000..d35f56d
>> >> >> --- /dev/null
>> >> >> +++ b/doc/SPI/README.sf-features
>> >> >> @@ -0,0 +1,122 @@
>> >> >> +SPI FLASH feature enhancements:
>> >> >> +==============================
>> >> >> +
>> >> >> +This document describes how to extend the current data structures in
>> >> >> spi subsystem +for making use of new flash features/operations w.r.t
>> >> >> to controller driver support. +
>> >> >> +1. spi_slave:
>> >> >> +
>> >> >> +struct spi_slave {
>> >> >> +    ..........
>> >> >> +    u32 mode_bits;
>> >> >> +    ........
>> >> >> +};
>> >> >> +
>> >> >> + at mode_bits can be used to expose the SPI RX/TX operation modes, bus
>> >> >> options and +few flags which are used to extended the flash specific
>> >> >> features/operations +- include/spi.h
>> >> >> +
>> >> >> +mode_bits:
>> >> >> +- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
>> >> >> +- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
>> >> >> +- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
>> >> >> +- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
>> >> >> +- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
>> >> >> +- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
>> >> >> +- SPI_SHARED: dual flash devices are connected in shared bus
>> >> >> connection +- SPI_SEPARATED: dual flash devices are connected in
>> >> >> separate bus connection +- SPI_U_PAGE: select the upper flash in
>> >> >> dual flash shared bus connection [1] +
>> >> >
>> >> > A generic SPI controller _does_ _not_ _care_ about any SPI flash crud.
>> >> > The SPI bus controller (which is what this is for) and SPI-NOR
>> >> > controller are two different things and must have two different slave
>> >> > structures.
>> >>
>> >> You mean mode_bits need to move in one more structure.
>> >> Just leave about new SPI-NOR as of now for this release we discuss more
>> >> soon.
>> >
>> > The mode_bits have no place in this structure. The slave can indicate
>> > whether it can be connected over 1,2,4... lines , but must not indicate
>> > that it supports some SPI-flash specific properties.
>>
>> What do you mean by this - can you elaborate.
>
> I mean there do exist SPI controllers which can output data via more than one
> (MOSI) line . They do support 1, 2 or even 4 lines. This comes useful when
> driving SPI flashes indeed, _BUT_ (!) this is not tied to driving SPI flashes.
> On the contrary, you can connect any device which can be driven over such
> "parallelised" SPI and it will work. And only such property shall also land in
> the struct spi_slave {} .
>
> Properties like "let's assume the SPI slave is a SPI NOR and it supports SPI NOR
> command FOO" do not go into struct spi_slave.
>
>> As of now drivers in drivers/spi need to inform the flash through spi_slave
>> {} no other alternative ie way remaining flash properties memory_map etc..
>> handle. Even Linux follow the same w/o new SPI-NOR framework.
>
> Linux is now going for a separate SPI-NOR controller framework and this is what
> we will do as well. The SF layer will be only a unifying layer providing access
> to either SPI-NOR API (for the SPI-NOR controllers) or a translation layer to
> communicate SPI-NOR commands over generic SPI API (for the old-school regular
> way of doing SPI NOR connection to SPI bus).

Please correct me logic here!
Just take an example of ti qspi controller.

cmd_sf.c
------------
spi_flash.h
--------------
sf (_probe, _ops), sf.c
-------------------------------------------
drivers/spi/ti_qspi.c (inform flash info mode_bits through spi_slave {}
------------------------------------------------------------

Suppose If we have one more qspi controller(zynq) which is connected to SPI-NOR

                                                cmd_sf.c
                                                -----------
                                               spi_flash.h
                                              ---------------
                                               SF-NOR
------------------------------------------------------------------------------------
drivers/spi/ti_qspi.c
drivers/mtd/sf_nor/zynq_qspi.c
(inform flash properties
(inform flash properties
through "mode_bits" of
through "mode_bits" of
spi_slave {})





>
>> If your question, like need a separate structure for flash specific
>> properties please wait will wound-up all these in new framework.
>>
>> I'm planning to push in today release.
>
> I doubt anything is going in today's release ;-)
>
> Actually, you need to really start discerning which _must_ go into a release and
> which simply does not go into a release and waits for -next. This is important
> role of a maintainer and plays important role in keeping the codebase in good
> shape.
>
> Code which is untested, code which changes core stuff and isn't an obvious
> bugfix etc. simply can wait. Code which is obvious bugfix goes in even in the -
> rc period. Of course, there are all kinds of exceptions, that depends on the
> rationale and negotiation.



-- 
Jagan.