Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

+ Lee Jones, others

I'd like to keep a similar CC list for the various conversations going
on about SPI NOR frameworks.

On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
> 1.) Why add a new framework for SPI NOR?
>   The SPI-NOR controller such as Freescale's Quadspi controller is working
>   in a different way from the SPI bus. It should knows the NOR commands to
>   find the right LUT sequence. Unfortunately, the current code can not meet
>   this requirement.
> 
> 2.) How does this patch set do?
>    This patch set adds a new spi-nor layer.
>    Before this patch, the layer is like:
> 
>                    MTD
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	        SPI NOR chip
> 
>    After this patch, the layer is like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	       SPI NOR chip
> 
>   With the spi-nor controller driver(Freescale Quadspi), it looks like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                 fsl-quadspi
>          ------------------------
> 	       SPI NOR chip
> 
> 3.) more details
>    This patch set adds a new data structrue spi_nor{}, clones most the common
>   code to spi-nor.c. Make the m25p80.c use the new APIs.
> 
> 4.) TODO list:
>    3.1) add the new spi_nor_device{} for the spi-nor controller's device.
>    3.2) add the Freescale Quadspi driver.
> 
> 	       
> Huang Shijie (4):
>   mtd: spi-nor: move the SPI NOR commands to a new header file
>   mtd: spi-nor: add a new data structrue spi_nor{}
>   mtd: spi-nor: add the framework for SPI NOR
>   mtd: m25p80: use the new spi-nor APIs
> 
>  drivers/mtd/Kconfig           |    2 +
>  drivers/mtd/Makefile          |    1 +
>  drivers/mtd/devices/Kconfig   |    2 +-
>  drivers/mtd/devices/m25p80.c  | 1249 +++--------------------------------------
>  drivers/mtd/spi-nor/Kconfig   |    6 +
>  drivers/mtd/spi-nor/Makefile  |    1 +
>  drivers/mtd/spi-nor/spi-nor.c | 1057 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |  101 ++++
>  8 files changed, 1239 insertions(+), 1180 deletions(-)
>  create mode 100644 drivers/mtd/spi-nor/Kconfig
>  create mode 100644 drivers/mtd/spi-nor/Makefile
>  create mode 100644 drivers/mtd/spi-nor/spi-nor.c
>  create mode 100644 include/linux/mtd/spi-nor.h
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年11月27日 12:32, Brian Norris 写道:
> 'd like to keep a similar CC list for the various conversations going
> on about SPI NOR frameworks.
it seems i missed to CC linux-SPI maillist.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Huang Shijie,

On 11/27/2013 04:32 AM, Brian Norris wrote:
> + Lee Jones, others
> 
> I'd like to keep a similar CC list for the various conversations going
> on about SPI NOR frameworks.
> 
> On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
>> 1.) Why add a new framework for SPI NOR?
>>   The SPI-NOR controller such as Freescale's Quadspi controller is working
>>   in a different way from the SPI bus. It should knows the NOR commands to
>>   find the right LUT sequence. Unfortunately, the current code can not meet
>>   this requirement.

I fully support your argument for introducing a new framework to support Serial
Flash controllers.  Reiterating my previous comment:

"The kernel offers many examples of others who have struggled with the same
problem.  Some have chosen to write self-contained drivers (e.g.
drivers/mtd/devices/spear_smi.c and drivers/mtd/devices/sst251.c) duplicating
much of m25p80.c, while others have attempted to squeeze into the SPI framework
(e.g. drivers/spi/spi-tegra20-sflash.c and drivers/spi/spi-falcon.c), relying on
fragile heuristics to recreate the semantics of m25p80 that is lost over the ,
current circumstances SPI framework ."

However, having now been through your proposed framework, it would not seem to
provide a generally applicable solution.  Indeed, other than making the Serial
Flash commands and the device table available, it is not obvious how it improves
the situation for your own specific controller either; the
read/write/read_reg/write_reg callbacks do not offer a great deal more than
spi_read_then_write().  (Perhaps all will become clear when you submit the
fsl-quadspi driver.)

As I see it, the main problem with the current support is that dedicated Serial
Flash Controllers require a level of semantics that cannot be conveyed over the
generic SPI framework.  With this in mind, I would start by defining something
along the lines of a "Serial Flash transfer".  Some initial ramblings of my mind:

/**
 * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
 * @wren:		command for "Write Enable", or 0x00 for not required
 * @cmd:		command for operation
 * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
 * @addr:		address for operation
 * @addr_pins:		number of pins to send @addr (1, 2, 4)
 * @addr_width: 	number of address bytes (3,4, or 0 for address not required)
 * @mode:		mode data
 * @mode_pins:		number of pins to send @mode (1, 2, 4)
 * @mode_cycles:	number of mode cycles (0 for mode not required)
 * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
 */
struct spi_nor_xfer_cfg {
	uint8_t		wren;
	uint8_t		cmd;
	int		cmd_pins;
	uint32_t	addr;
	int		addr_pins;
	int		addr_width;
	uint32_t	mode;
	int		mode_pins;
	int		mode_cycles;
	int		dummy_cycles;
};

We could then build up layers of functionality, based on two fundamental primitives:

	int (*read_xfer)(struct spi_nor_info *info,
			 struct spi_nor_xfer_cfg *cfg,
			 uint8_t *buf, size_t len);
	
	int (*write_xfer)(struct spi_nor_info *info,
			  struct spi_nor_xfer_cfg *cfg,
			  uint8_t *buf, size_t len);
	
Default implementations for standard operations could follow:

	int (*read_reg)(struct spi_nor_info *info,
			uint8_t cmd, uint8_t *reg, int len);
	int (*write_reg)(struct spi_nor_info *info,
			 uint8_t cmd, uint8_t *reg, int len,
			 int wren, int wtr);

	int (*readid)(struct spi_nor_info *info, uint8_t *readid);
	int (*wait_till_ready)(struct spi_nor_info *info);
	
	int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs, size_t len);
	int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
	int (*erase_sector)(struct spi_nor_info *info, off_t offs);

Each driver would be required to implement read_xfer() and write_xfer(), and
then free to either use the default implementations, or override with
driver-optimised implementations.

In the case of a pure SPI Controller, read_xfer() and write_xfer() would simply
flatten to spi_messages.  The key benefit is that dedicated Serial Flash
Controllers would be able to take advantage of the richer semantics offered by
the 'spi_nor_xfer_cfg' data.

I would also expect the spi-nor framework to provide the following services,
applicable to all Serial Flash drivers:

	- determine devices properties at runtime, based on the READID data (and
perhaps SFDP, if and when it matures!).  This should include supported opcodes
(e.g. READ_1_1_4, READ_1_4_4, READ4B_1_1_4...), operating frequencies, mode and
dummy requirements, means of setting Quad Enable, entering/exiting 4-byte
addressing etc.

	- provide optimal read, write and erase configurations, based on the combined
capabilities of the Serial Flash device and the H/W Controller.

	- device specific configuration (e.g. setting QE, unlock BPx bits, DYB unlocking).

Getting back to reality, I realise undertaking such a task would be a huge
commitment.  I would be keen to put forward my own proposal, but current
circumstances mean that that is unlikely to happen any time soon.

I guess in summary, while I am pleased that this area is being looked at, my own
feeling is that proposed framework needs to be reworked for it to be generally
applicable to other Serial Flash controllers.  Of course, another option would
be to stick with what is currently offered, and make do.

Cheers,

Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 660 bytes --]

On Fri, 2013-11-29 at 14:52 +0000, Angus Clark wrote:
> 
> As I see it, the main problem with the current support is that dedicated Serial
> Flash Controllers require a level of semantics that cannot be conveyed over the
> generic SPI framework.  With this in mind, I would start by defining something
> along the lines of a "Serial Flash transfer".  

I think this is the best way to describe the issue, and approach the
solution.

We should be looking to extend the generic SPI framework to support what
we need, rather than hacking around it purely on the MTD side.

As such, I'd be very interested in Mark's thoughts on it...

-- 
dwmw2


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Tue, Dec 03, 2013 at 02:51:13PM +0000, David Woodhouse wrote:
> On Fri, 2013-11-29 at 14:52 +0000, Angus Clark wrote:
> > 
> > As I see it, the main problem with the current support is that dedicated Serial
> > Flash Controllers require a level of semantics that cannot be conveyed over the
> > generic SPI framework.  With this in mind, I would start by defining something
> > along the lines of a "Serial Flash transfer".  
> 
> I think this is the best way to describe the issue, and approach the
> solution.
> 
> We should be looking to extend the generic SPI framework to support what
> we need, rather than hacking around it purely on the MTD side.
> 
> As such, I'd be very interested in Mark's thoughts on it...

Hmm, I thought we already had (some form of) this explanation
weeks/months ago, yet the conclusion was that Mark doesn't want this in
the SPI framework at all, since a dedicated Serial Flash Controller is
not truly SPI. But perhaps I'm wrong.

On this topic, I have one more stick to throw on this fire; I have some
Broadcom hardware that consists of two pieces:

  (1) a true SPI controller, with single-line-I/O and
  (2) a Serial Flash controller that can accelerate flash read, using
      dual/quad I/O.

The latter hardware cannot handle flash writes, only uses quad I/O for
data (not addresses), has some other peculiarities specific to SPI
flash, and is highly dependent on the former for providing complete SPI
flash support. My current driver is not really upstream-ready, since
(like Huang's original driver), it pretends to be a SPI driver yet
requires "incestuous knowledge" of m25p80 / MTD. But I hope that any new
framework can support hardware like mine.

I don't have public documentation for this hardware, but I can provide
more detail if desired.

I mostly bring this up, though, because it is an example of hardware
that

   (a) can operate as a true SPI device (hardware (1) can handle generic
       SPI transfers), but
   (b) requires knowledge of the details of SPI flash in order to get
       optimal usage out of the acceleration from (2).

In my book, point (a) and (b) hold some bearing over the question of
"where should this SPI NOR framework live", because it is reasonable
that a single driver for this hardware should be able to handle either
true SPI devices or accelerated SPI-NOR flash.

Sorry for my late-ish addition to this discussion. There's been a lot of
noise on this topic, and SPI flash is not the highest priority on my
list for now (although it's certainly on the list).

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> 
> I mostly bring this up, though, because it is an example of hardware
> that
> 
>    (a) can operate as a true SPI device (hardware (1) can handle generic
>        SPI transfers), but
>    (b) requires knowledge of the details of SPI flash in order to get
>        optimal usage out of the acceleration from (2).
> 
> In my book, point (a) and (b) hold some bearing over the question of
> "where should this SPI NOR framework live", because it is reasonable
> that a single driver for this hardware should be able to handle either
> true SPI devices or accelerated SPI-NOR flash.

We have add a @read_id hook for the spi-nor layer, you can use it to read out
more info (such as the Device Geometry Definition) for your spi-nor controller driver.

Is it okay?

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Thu, Dec 05, 2013 at 03:12:25PM +0800, Huang Shijie wrote:
> On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> > 
> > I mostly bring this up, though, because it is an example of hardware
> > that
> > 
> >    (a) can operate as a true SPI device (hardware (1) can handle generic
> >        SPI transfers), but
> >    (b) requires knowledge of the details of SPI flash in order to get
> >        optimal usage out of the acceleration from (2).
> > 
> > In my book, point (a) and (b) hold some bearing over the question of
> > "where should this SPI NOR framework live", because it is reasonable
> > that a single driver for this hardware should be able to handle either
> > true SPI devices or accelerated SPI-NOR flash.
> 
> We have add a @read_id hook for the spi-nor layer, you can use it to read out
> more info (such as the Device Geometry Definition) for your spi-nor controller driver.
> 
> Is it okay?

I'm not sure which @read_id hook you're talking about, as I've only
skimmed the most recent thread, and it's not in the last patch series I
have from you, I don't think. But that is not a response at all to the
points I brought up. My statements have nothing to do with device
geometry detection.

What I was actually addressing:

Your framework aligns with Mark's original suggestion: that the "SPI
NOR" framework should be a separate layer held entirely within the MTD
subsystem, and that any given driver is *either* a "SPI NOR" driver *or*
a "true SPI" driver, not both.

I'm simply describing how my hardware is both, and perhaps (as David
seems to be doing) we should reconsider the premise that this framework
will exist solely and entirely within MTD.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 12:11:01AM -0800, Brian Norris wrote:
> On Thu, Dec 05, 2013 at 03:12:25PM +0800, Huang Shijie wrote:
> > On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> > > 
> > > I mostly bring this up, though, because it is an example of hardware
> > > that
> > > 
> > >    (a) can operate as a true SPI device (hardware (1) can handle generic
> > >        SPI transfers), but
> > >    (b) requires knowledge of the details of SPI flash in order to get
> > >        optimal usage out of the acceleration from (2).
> > > 
> > > In my book, point (a) and (b) hold some bearing over the question of
> > > "where should this SPI NOR framework live", because it is reasonable
> > > that a single driver for this hardware should be able to handle either
> > > true SPI devices or accelerated SPI-NOR flash.
> > 
> > We have add a @read_id hook for the spi-nor layer, you can use it to read out
> > more info (such as the Device Geometry Definition) for your spi-nor controller driver.
> > 
> > Is it okay?
> 
> I'm not sure which @read_id hook you're talking about, as I've only
> skimmed the most recent thread, and it's not in the last patch series I
> have from you, I don't think. But that is not a response at all to the
> points I brought up. My statements have nothing to do with device
> geometry detection.

My meaning is You can write a new driver for the (b) with the new spi-nor layer,

In the new driver you can implement the @read_id yourself, and read out the
the necessary info, and implement other hooks for the so-called peculiarities.

> 
> What I was actually addressing:
> 
> Your framework aligns with Mark's original suggestion: that the "SPI
> NOR" framework should be a separate layer held entirely within the MTD
> subsystem, and that any given driver is *either* a "SPI NOR" driver *or*
> a "true SPI" driver, not both.
You can only use a SPI-NOR driver, and you can abandon the "true SPI" driver.

Is there some limits that prevent we just code a SPI-NOR driver for your hardware?

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Thu, Dec 05, 2013 at 03:59:02PM +0800, Huang Shijie wrote:
> On Thu, Dec 05, 2013 at 12:11:01AM -0800, Brian Norris wrote:
> > On Thu, Dec 05, 2013 at 03:12:25PM +0800, Huang Shijie wrote:
> > > On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> > > > 
> > > > I mostly bring this up, though, because it is an example of hardware
> > > > that
> > > > 
> > > >    (a) can operate as a true SPI device (hardware (1) can handle generic
> > > >        SPI transfers), but
> > > >    (b) requires knowledge of the details of SPI flash in order to get
> > > >        optimal usage out of the acceleration from (2).
> > > > 
> > > > In my book, point (a) and (b) hold some bearing over the question of
> > > > "where should this SPI NOR framework live", because it is reasonable
> > > > that a single driver for this hardware should be able to handle either
> > > > true SPI devices or accelerated SPI-NOR flash.
> > > 
> > > We have add a @read_id hook for the spi-nor layer, you can use it to read out
> > > more info (such as the Device Geometry Definition) for your spi-nor controller driver.
> > > 
> > > Is it okay?
> > 
> > I'm not sure which @read_id hook you're talking about, as I've only
> > skimmed the most recent thread, and it's not in the last patch series I
> > have from you, I don't think. But that is not a response at all to the
> > points I brought up. My statements have nothing to do with device
> > geometry detection.
> 
> My meaning is You can write a new driver for the (b) with the new spi-nor layer,

Yes, I can do that.

> In the new driver you can implement the @read_id yourself, and read out the
> the necessary info, and implement other hooks for the so-called peculiarities.

Without giving a full judgment on the framework yet (I haven't followed
as closely as I'd like), I expect that the SPI NOR API will cover the
needs of my SPI NOR hardware.

> > 
> > What I was actually addressing:
> > 
> > Your framework aligns with Mark's original suggestion: that the "SPI
> > NOR" framework should be a separate layer held entirely within the MTD
> > subsystem, and that any given driver is *either* a "SPI NOR" driver *or*
> > a "true SPI" driver, not both.
> You can only use a SPI-NOR driver, and you can abandon the "true SPI" driver.
> 
> Is there some limits that prevent we just code a SPI-NOR driver for your hardware?

SPI is not used only for SPI NOR; it is useful for other things, like
communicating with off-chip peripherals or microcontrollers, supporting
touchscreens, LEDs, media controllers, etc. So with a SPI NOR framework
living only in the MTD subsystem, I have to write two drivers: one
(under drivers/spi/) to use only-(a) for controlling non-flash SPI
devices, and one (under drivers/mtd/) to manage (a)+(b) on SPI NOR
flash.

If, however, we can define a spi_nor_transfer like Angus suggested, and
if we take that a step further and make it a first-class (but optional)
citizen of the SPI framework, then we could have drivers that support
SPI-only, SPI-NOR-only, or both.

Now, I'm not sure how exactly that sort of proposal would work yet, but
I wanted to test the waters to see
  - if this is a complete distortion of what Angus or David may have
    suggested;
  - if Mark thinks this is reasonable, or if he thinks I'm spouting
    nonsense; and
  - if there is some obvious alternate way to support what I describe
    without moving the SPI-NOR layer into the SPI subsystem.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 01:20:48AM -0800, Brian Norris wrote:
> SPI is not used only for SPI NOR; it is useful for other things, like
> communicating with off-chip peripherals or microcontrollers, supporting
> touchscreens, LEDs, media controllers, etc. So with a SPI NOR framework
> living only in the MTD subsystem, I have to write two drivers: one
> (under drivers/spi/) to use only-(a) for controlling non-flash SPI
> devices, and one (under drivers/mtd/) to manage (a)+(b) on SPI NOR
> flash.

If the NOR is connected to a SPI bus controller which can attaches other SPI
devices too, I think you use a pure SPI driver to access it;

If the NOR is connected to a SPI-NOR controller such as your spi-nor controller
, you can use a SPI-NOR driver to access it.


Does your NOR flash connect to both the SPI bus controller and SPI-NOR
controller at the same time? :) If it does connect to both of the controllers,
I do not know how to handle it too.	   


> 
> If, however, we can define a spi_nor_transfer like Angus suggested, and
> if we take that a step further and make it a first-class (but optional)
> citizen of the SPI framework, then we could have drivers that support
> SPI-only, SPI-NOR-only, or both.
> 
I guess Mark will not agree with this. :)


thanks
Huang Shijie


> Now, I'm not sure how exactly that sort of proposal would work yet, but
> I wanted to test the waters to see
>   - if this is a complete distortion of what Angus or David may have
>     suggested;
>   - if Mark thinks this is reasonable, or if he thinks I'm spouting
>     nonsense; and
>   - if there is some obvious alternate way to support what I describe
>     without moving the SPI-NOR layer into the SPI subsystem.
> 
> Brian
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Brian,

On 12/04/2013 06:44 PM, Brian Norris wrote:
> On this topic, I have one more stick to throw on this fire; I have some
> Broadcom hardware that consists of two pieces:
> 
>   (1) a true SPI controller, with single-line-I/O and
>   (2) a Serial Flash controller that can accelerate flash read, using
>       dual/quad I/O.

We support one platform with properties similar to the hardware you describe,
and such a hybrid configuration was in mind when I was thinking about the
spi-nor framework.  I am not sure how well this will come across over email, but
I envisage something like this:

                             spi-nor
                                |
                                v
                          [spi-nor-xfer]
                                |
         ------------------------------------------------
        |                       |                        |
        v                       v                        v
   spi-wrapper (<------)  hybrid-driver          serial-flash-driver
        |                       |                        |
      [SPI]                     |                        |
        |                       |                        |
        v                       |                        |
generic-spi-driver              |                        |
        |                       |                        |
        v                       v                        v
[generic-SPI-Cntrl]   [SPI-Flash-Read Cntrl]   [SPI-Flash-Controller]

The 'spi-nor' layer is responsible for Serial Flash domain knowledge, and how to
generate 'spi-nor-xfer' messages.  The 'spi-wrapper' would provide an interface
to the generic SPI framework, flattening the 'spi-nor-xfer' message to standard
SPI calls.  The combination of 'spi-nor' and 'spi-wrapper' should achieve what
we have today for the m25p80 driver.  The 'serial-flash-driver' in the example
provides an implementation for a dedicated Serial Flash Controller, making use
of the spi-nor-xfer semantics where required.  The 'hybrid-driver' could use the
'spi-wrapper' implementation for write and erase hooks, and optimised
implementations for read hooks.

[...]
> I mostly bring this up, though, because it is an example of hardware
> that
> 
>    (a) can operate as a true SPI device (hardware (1) can handle generic
>        SPI transfers), but
>    (b) requires knowledge of the details of SPI flash in order to get
>        optimal usage out of the acceleration from (2).
> 
> In my book, point (a) and (b) hold some bearing over the question of
> "where should this SPI NOR framework live", because it is reasonable
> that a single driver for this hardware should be able to handle either
> true SPI devices or accelerated SPI-NOR flash.
> 

Yes, I agree.  At present, within the m25p80 driver, the knowledge of how to
drive Serial Flash devices is entwined with an implementation that assumes a
pure SPI Controller.  Creating an additional layer of abstraction allows us to
separate the two issues, and write drivers that can make us of generic SPI
controllers, or take advantage of accelerated Serial Flash Controllers, or a
combination of both.

I realise it is going to take a significant effort to get this off the ground
and the framework will require some tuning to make sure it can accommodate the
eccentric range of hardware out there.  Unfortunately I am not in a position to
commit much time myself, although I will help advise where I can.  Ironically,
once the ST Bristol site closes next year, I will have lots of time available,
but no access to equipment!

Cheers,

Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 02:35:47PM +0000, Angus Clark wrote:
> Hi Brian,
> 
> On 12/04/2013 06:44 PM, Brian Norris wrote:
> > On this topic, I have one more stick to throw on this fire; I have some
> > Broadcom hardware that consists of two pieces:
> > 
> >   (1) a true SPI controller, with single-line-I/O and
> >   (2) a Serial Flash controller that can accelerate flash read, using
> >       dual/quad I/O.
> 
> We support one platform with properties similar to the hardware you describe,
> and such a hybrid configuration was in mind when I was thinking about the
> spi-nor framework.  I am not sure how well this will come across over email, but
> I envisage something like this:
> 
>                              spi-nor
>                                 |
>                                 v
>                           [spi-nor-xfer]
>                                 |
>          ------------------------------------------------
>         |                       |                        |
>         v                       v                        v
>    spi-wrapper (<------)  hybrid-driver          serial-flash-driver
>         |                       |                        |
>       [SPI]                     |                        |
>         |                       |                        |
>         v                       |                        |
> generic-spi-driver              |                        |
>         |                       |                        |
>         v                       v                        v
> [generic-SPI-Cntrl]   [SPI-Flash-Read Cntrl]   [SPI-Flash-Controller]
> 
> The 'spi-nor' layer is responsible for Serial Flash domain knowledge, and how to
> generate 'spi-nor-xfer' messages.  The 'spi-wrapper' would provide an interface
> to the generic SPI framework, flattening the 'spi-nor-xfer' message to standard
> SPI calls.  The combination of 'spi-nor' and 'spi-wrapper' should achieve what
> we have today for the m25p80 driver.  The 'serial-flash-driver' in the example
> provides an implementation for a dedicated Serial Flash Controller, making use
> of the spi-nor-xfer semantics where required.  The 'hybrid-driver' could use the
> 'spi-wrapper' implementation for write and erase hooks, and optimised
> implementations for read hooks.
> 
For the diagram above, I think we can regard the hybrid driver as a kind of
serial-flash-driver too.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Thu, Dec 05, 2013 at 02:35:47PM +0000, Angus Clark wrote:
> On 12/04/2013 06:44 PM, Brian Norris wrote:
> > On this topic, I have one more stick to throw on this fire; I have some
> > Broadcom hardware that consists of two pieces:
> > 
> >   (1) a true SPI controller, with single-line-I/O and
> >   (2) a Serial Flash controller that can accelerate flash read, using
> >       dual/quad I/O.
> 
> We support one platform with properties similar to the hardware you describe,
> and such a hybrid configuration was in mind when I was thinking about the
> spi-nor framework.  I am not sure how well this will come across over email, but
> I envisage something like this:
> 
>                              spi-nor
>                                 |
>                                 v
>                           [spi-nor-xfer]
>                                 |
>          ------------------------------------------------
>         |                       |                        |
>         v                       v                        v
>    spi-wrapper (<------)  hybrid-driver          serial-flash-driver
>         |                       |                        |
>       [SPI]                     |                        |
>         |                       |                        |
>         v                       |                        |
> generic-spi-driver              |                        |
>         |                       |                        |
>         v                       v                        v
> [generic-SPI-Cntrl]   [SPI-Flash-Read Cntrl]   [SPI-Flash-Controller]
> 
> The 'spi-nor' layer is responsible for Serial Flash domain knowledge, and how to
> generate 'spi-nor-xfer' messages.  The 'spi-wrapper' would provide an interface
> to the generic SPI framework, flattening the 'spi-nor-xfer' message to standard
> SPI calls.  The combination of 'spi-nor' and 'spi-wrapper' should achieve what
> we have today for the m25p80 driver.  The 'serial-flash-driver' in the example
> provides an implementation for a dedicated Serial Flash Controller, making use
> of the spi-nor-xfer semantics where required.  The 'hybrid-driver' could use the
> 'spi-wrapper' implementation for write and erase hooks, and optimised
> implementations for read hooks.

OK, that sounds reasonable. But how do I bind my "generic SPI driver"
with my "SPI NOR accelerated read driver" to represent this? Does the
hybrid driver (my accelerated-read driver) have to register itself as a
SPI protocol driver (struct spi_driver)? The "controller" boundaries
seem to get a little fuzzy under this design.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

Hi Angus:
   thanks for the long suggestion. it's very useful.

Let's push the spi nor framework.:)
> Hi Huang Shijie,
>
> On 11/27/2013 04:32 AM, Brian Norris wrote:
>> + Lee Jones, others
>>
>> I'd like to keep a similar CC list for the various conversations going
>> on about SPI NOR frameworks.
>>
>> On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
>>> 1.) Why add a new framework for SPI NOR?
>>>    The SPI-NOR controller such as Freescale's Quadspi controller is working
>>>    in a different way from the SPI bus. It should knows the NOR commands to
>>>    find the right LUT sequence. Unfortunately, the current code can not meet
>>>    this requirement.
> I fully support your argument for introducing a new framework to support Serial
> Flash controllers.  Reiterating my previous comment:
>
> "The kernel offers many examples of others who have struggled with the same
> problem.  Some have chosen to write self-contained drivers (e.g.
> drivers/mtd/devices/spear_smi.c and drivers/mtd/devices/sst251.c) duplicating
> much of m25p80.c, while others have attempted to squeeze into the SPI framework
> (e.g. drivers/spi/spi-tegra20-sflash.c and drivers/spi/spi-falcon.c), relying on
> fragile heuristics to recreate the semantics of m25p80 that is lost over the ,
> current circumstances SPI framework ."
>
> However, having now been through your proposed framework, it would not seem to
> provide a generally applicable solution.  Indeed, other than making the Serial
> Flash commands and the device table available, it is not obvious how it improves
> the situation for your own specific controller either; the
> read/write/read_reg/write_reg callbacks do not offer a great deal more than
> spi_read_then_write().  (Perhaps all will become clear when you submit the
> fsl-quadspi driver.)

> As I see it, the main problem with the current support is that dedicated Serial
> Flash Controllers require a level of semantics that cannot be conveyed over the
> generic SPI framework.  With this in mind, I would start by defining something
> along the lines of a "Serial Flash transfer".  Some initial ramblings of my mind:
>
> /**
>   * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
>   * @wren:		command for "Write Enable", or 0x00 for not required
why this wren is needed?
>   * @cmd:		command for operation
>   * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
>   * @addr:		address for operation
>   * @addr_pins:		number of pins to send @addr (1, 2, 4)
>   * @addr_width: 	number of address bytes (3,4, or 0 for address not required)
this field has been in the spi_nor{} , it should be a fixed value.
so i think we do not need this argument.
>   * @mode:		mode data
>   * @mode_pins:		number of pins to send @mode (1, 2, 4)
>   * @mode_cycles:	number of mode cycles (0 for mode not required)
>   * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
>   */
> struct spi_nor_xfer_cfg {
> 	uint8_t		wren;
> 	uint8_t		cmd;
> 	int		cmd_pins;
> 	uint32_t	addr;
> 	int		addr_pins;
> 	int		addr_width;
> 	uint32_t	mode;
> 	int		mode_pins;
> 	int		mode_cycles;
> 	int		dummy_cycles;
> };
>
> We could then build up layers of functionality, based on two fundamental primitives:
>
> 	int (*read_xfer)(struct spi_nor_info *info,
> 			 struct spi_nor_xfer_cfg *cfg,
> 			 uint8_t *buf, size_t len);
> 	
> 	int (*write_xfer)(struct spi_nor_info *info,
> 			  struct spi_nor_xfer_cfg *cfg,
> 			  uint8_t *buf, size_t len);
> 	
> Default implementations for standard operations could follow:
>
> 	int (*read_reg)(struct spi_nor_info *info,
> 			uint8_t cmd, uint8_t *reg, int len);
> 	int (*write_reg)(struct spi_nor_info *info,
> 			 uint8_t cmd, uint8_t *reg, int len,
> 			 int wren, int wtr);
>
> 	int (*readid)(struct spi_nor_info *info, uint8_t *readid);
> 	int (*wait_till_ready)(struct spi_nor_info *info);

currently, we poll the status register.
why this hook is needed?

> 	
> 	int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs, size_t len);
> 	int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
> 	int (*erase_sector)(struct spi_nor_info *info, off_t offs);
I also confused at this hook.

if we need erase_sector, do we still need the erase_chip()?

> Each driver would be required to implement read_xfer() and write_xfer(), and
> then free to either use the default implementations, or override with
> driver-optimised implementations.
>
> In the case of a pure SPI Controller, read_xfer() and write_xfer() would simply
> flatten to spi_messages.  The key benefit is that dedicated Serial Flash
> Controllers would be able to take advantage of the richer semantics offered by
> the 'spi_nor_xfer_cfg' data.
>
> I would also expect the spi-nor framework to provide the following services,
> applicable to all Serial Flash drivers:
>
> 	- determine devices properties at runtime, based on the READID data (and
> perhaps SFDP, if and when it matures!).  This should include supported opcodes
> (e.g. READ_1_1_4, READ_1_4_4, READ4B_1_1_4...), operating frequencies, mode and
> dummy requirements, means of setting Quad Enable, entering/exiting 4-byte
> addressing etc.
>
> 	- provide optimal read, write and erase configurations, based on the combined
> capabilities of the Serial Flash device and the H/W Controller.
>
> 	- device specific configuration (e.g. setting QE, unlock BPx bits, DYB unlocking).
>
Does Jones have any opinion about this?

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Gupta, Pekon]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

>From: Huang Shijie [mailto:b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org]
>Hi Angus:
>   thanks for the long suggestion. it's very useful.
>
>Let's push the spi nor framework.:)
> 
Yes, indeed nicely explained ..

[...]

>> 	int (*wait_till_ready)(struct spi_nor_info *info);
>
>currently, we poll the status register.
>why this hook is needed?
>
Currently there is no uniformity between different vendors to R/W Status
and Config registers. Also the layout and functionality of various fields in
Status and Config register vary from vendor to vendor. So, either you can
define this for-each vendor.. Or let each driver handle it separately.

[...]

>> 	int (*erase_sector)(struct spi_nor_info *info, off_t offs);
>I also confused at this hook.
>
>if we need erase_sector, do we still need the erase_chip()?
>
Yes, there are multiple types of erase commands..
(1) sector erase.
(2) block erase.
(3) bulk erase or chip erase.
Refer attachment ..


with regards, pekon

[-- Attachment #2: Serial_Flash_commands.png --]
[-- Type: image/png, Size: 59763 bytes --]

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Huang Shijie

On 12/02/2013 10:06 AM, Huang Shijie wrote:
> Hi Angus:
>   thanks for the long suggestion. it's very useful.
> 
> Let's push the spi nor framework.:)
[...]
>>
>> /**
>>   * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash
>> transfer
>>   * @wren:        command for "Write Enable", or 0x00 for not required
> why this wren is needed?

Issuing WREN is needed for certain sequences of operations.  I guess your
question relates to why have I included it as part of the 'spi_nor_xfer_cfg'
structure, when it could be controlled from the spi_nor layer with a separate
spi_nor_xfer.

Well, firstly, I am aware of at least one H/W controller that can be optimised
to issue the WREN command as part of a longer erase or write sequence, thereby
avoiding the overheads of issuing a separate spi_nor_xfer.

However, the main reason was to try and come up with a generic representation
for a typical Serial Flash transfer, capturing, where possible, the semantics of
the intended operation, just in case that information can be used by the
underlying H/W in some way.  In this instance, the WREN cycles just represents
another part of the transfer, in the same way as the command, address, mode, and
dummy cycles.

>>   * @cmd:        command for operation
>>   * @cmd_pins:        number of pins to send @cmd (1, 2, 4)
>>   * @addr:        address for operation
>>   * @addr_pins:        number of pins to send @addr (1, 2, 4)
>>   * @addr_width:     number of address bytes (3,4, or 0 for address
>> not required)
> this field has been in the spi_nor{} , it should be a fixed value.
> so i think we do not need this argument.

The aim of the 'spi_nor_xfer_cfg' is to provide an interface between the spi-nor
layer and the underlying H/W driver.   In some cases, the H/W may need to know
the address width (e.g. issuing a new command sequence, with an incremented
address, following an arbitration stall).  You could argue that the H/W driver
should be able query the spi_nor structure, or perhaps store the addr_width
field during an initialisation process, but I feel having it as part of the
'spi_nor_xfer_cfg' provides a cleaner interface between the two layers.

>>   * @mode:        mode data
>>   * @mode_pins:        number of pins to send @mode (1, 2, 4)
>>   * @mode_cycles:    number of mode cycles (0 for mode not required)
>>   * @dummy_cycles:    number of dummy cycles (0 for dummy not required)
>>   */
>> struct spi_nor_xfer_cfg {
>>     uint8_t        wren;
>>     uint8_t        cmd;
>>     int        cmd_pins;
>>     uint32_t    addr;
>>     int        addr_pins;
>>     int        addr_width;
>>     uint32_t    mode;
>>     int        mode_pins;
>>     int        mode_cycles;
>>     int        dummy_cycles;
>> };
>>
>> We could then build up layers of functionality, based on two
>> fundamental primitives:
>>
>>     int (*read_xfer)(struct spi_nor_info *info,
>>              struct spi_nor_xfer_cfg *cfg,
>>              uint8_t *buf, size_t len);
>>     
>>     int (*write_xfer)(struct spi_nor_info *info,
>>               struct spi_nor_xfer_cfg *cfg,
>>               uint8_t *buf, size_t len);
>>     
>> Default implementations for standard operations could follow:
>>
>>     int (*read_reg)(struct spi_nor_info *info,
>>             uint8_t cmd, uint8_t *reg, int len);
>>     int (*write_reg)(struct spi_nor_info *info,
>>              uint8_t cmd, uint8_t *reg, int len,
>>              int wren, int wtr);
>>
>>     int (*readid)(struct spi_nor_info *info, uint8_t *readid);
>>     int (*wait_till_ready)(struct spi_nor_info *info);
> 
> currently, we poll the status register.
> why this hook is needed?

The default implementation would do just as you suggest, and I would expect most
H/W drivers to use the default implementation.  However, some H/W Controllers
can automate the polling of status registers, so having a hook allows the driver
override the default behaviour.

>>     int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs,
>> size_t len);
>>     int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
>>     int (*erase_sector)(struct spi_nor_info *info, off_t offs);
> I also confused at this hook.
> 
> if we need erase_sector, do we still need the erase_chip()?

Serial Flash devices support various "Erase Sector" commands, and an "Erase
Chip" command.  The erase_sector() and erase_chip() hooks would just form the
appropriate command sequences (e.g. Erase Chip does not require address cycles).

I have to admit, the circumstances that provoke an erase_chip() are rather
limited.  As far as I can see, the MTD tool 'mtd_debug' is the only component
that can lead to an Erase Chip operation, and even then, only when the device is
not partitioned.  However, m25p80 supports Erase Chip, so I thought I would
maintain that support here.

As I said before, my proposal was just some initial ramblings of my mind, and I
am definitely open to discussion and other suggestions.

To be honest, I am just glad that someone is taking a look at this area.
However, whether or not is it possible to come up with something that solves all
our problems remains to be seen.  It might turn out that the various H/W
Controllers are just too different to fit into a single unified framework.

Cheers,

Angus

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年12月02日 19:19, Angus Clark 写道:
> To be honest, I am just glad that someone is taking a look at this area.
> However, whether or not is it possible to come up with something that solves all
> our problems remains to be seen.  It might turn out that the various H/W
> Controllers are just too different to fit into a single unified framework.
>
Hi Angus & Pekon:
thanks for the explanations.

I am not sure that after we add more hooks, if it will meet Jones's new 
driver.

i will wait for Jones' comment for several days, and send out the next 
version if he does not reply this thread.


thanks
Huang Shijie



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Lee Jones]

> >To be honest, I am just glad that someone is taking a look at this area.
> >However, whether or not is it possible to come up with something that solves all
> >our problems remains to be seen.  It might turn out that the various H/W
> >Controllers are just too different to fit into a single unified framework.
> >
> Hi Angus & Pekon:
> thanks for the explanations.
> 
> I am not sure that after we add more hooks, if it will meet Jones's
> new driver.

Angus is the author of "Lee's" driver and knows more about it that I
ever will, so whatever he says goes in that regard.

I would like to make a suggestion to Brian though. Even if the new
framework is written within the next couple of months and the
semantics do suit the FSM Controller driver, I'd still like the
implementation that's currently on the list to be applied. That way
we'd have a known good version of the driver which is almost identical
to how ST's internal driver does now. The one that is present out in
the wild (i.e. _real_ products).

I will subsequently volunteer to provide my utmost best efforts to
port the driver over to the new framework as a new task once it has
landed.

> i will wait for Jones' comment for several days, and send out the
> next version if he does not reply this thread.

/me waves \o/

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Tue, Dec 03, 2013 at 08:23:43AM +0000, Lee Jones wrote:
> I would like to make a suggestion to Brian though. Even if the new
> framework is written within the next couple of months and the
> semantics do suit the FSM Controller driver, I'd still like the
> implementation that's currently on the list to be applied. That way
> we'd have a known good version of the driver which is almost identical
> to how ST's internal driver does now. The one that is present out in
> the wild (i.e. _real_ products).

I think this suggestion is feasible: that we might merge this driver
even if it doesn't try to fit to some (still vague) idea of what a "SPI
NOR framework" should be. However, I don't know if the latter argument
is valid; just because it's in real products doesn't mean we really want
it as-is. But I think that in this case, the perfect may be the enemy of
the good -- the "good" driver now is better than the "perfect" driver
that never comes.

> I will subsequently volunteer to provide my utmost best efforts to
> port the driver over to the new framework as a new task once it has
> landed.

I'll hold you to that! Do we have any collateral for that guarantee? ;)

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Lee Jones]

On Tue, 10 Dec 2013, Brian Norris wrote:

> On Tue, Dec 03, 2013 at 08:23:43AM +0000, Lee Jones wrote:
> > I would like to make a suggestion to Brian though. Even if the new
> > framework is written within the next couple of months and the
> > semantics do suit the FSM Controller driver, I'd still like the
> > implementation that's currently on the list to be applied. That way
> > we'd have a known good version of the driver which is almost identical
> > to how ST's internal driver does now. The one that is present out in
> > the wild (i.e. _real_ products).
> 
> I think this suggestion is feasible: that we might merge this driver
> even if it doesn't try to fit to some (still vague) idea of what a "SPI
> NOR framework" should be. However, I don't know if the latter argument
> is valid; just because it's in real products doesn't mean we really want
> it as-is.

True, it just makes things easier for us internally when we are due
for a kernel version up-lift.

> But I think that in this case, the perfect may be the enemy of
> the good -- the "good" driver now is better than the "perfect" driver
> that never comes.

+1

> > I will subsequently volunteer to provide my utmost best efforts to
> > port the driver over to the new framework as a new task once it has
> > landed.
> 
> I'll hold you to that! Do we have any collateral for that guarantee? ;)

My word is my bond Brian. Failing that, beer? :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Hi Angus,

_nicely_ explained indeed :)

> Hi Huang Shijie,
> 
> On 11/27/2013 04:32 AM, Brian Norris wrote:
> > + Lee Jones, others
> > 
> > I'd like to keep a similar CC list for the various conversations going
> > on about SPI NOR frameworks.
> > 
> > On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
> >> 1.) Why add a new framework for SPI NOR?
> >> 
> >>   The SPI-NOR controller such as Freescale's Quadspi controller is
> >>   working in a different way from the SPI bus. It should knows the NOR
> >>   commands to find the right LUT sequence. Unfortunately, the current
> >>   code can not meet this requirement.
> 
> I fully support your argument for introducing a new framework to support
> Serial Flash controllers.

So do I, now I finally get the problem.

> Reiterating my previous comment:
> 
> "The kernel offers many examples of others who have struggled with the same
> problem.  Some have chosen to write self-contained drivers (e.g.
> drivers/mtd/devices/spear_smi.c and drivers/mtd/devices/sst251.c)
> duplicating much of m25p80.c, while others have attempted to squeeze into
> the SPI framework (e.g. drivers/spi/spi-tegra20-sflash.c and
> drivers/spi/spi-falcon.c), relying on fragile heuristics to recreate the
> semantics of m25p80 that is lost over the , current circumstances SPI
> framework ."
> 
> However, having now been through your proposed framework, it would not seem
> to provide a generally applicable solution.  Indeed, other than making the
> Serial Flash commands and the device table available, it is not obvious
> how it improves the situation for your own specific controller either; the
> read/write/read_reg/write_reg callbacks do not offer a great deal more than
> spi_read_then_write().  (Perhaps all will become clear when you submit the
> fsl-quadspi driver.)
> 
> As I see it, the main problem with the current support is that dedicated
> Serial Flash Controllers require a level of semantics that cannot be
> conveyed over the generic SPI framework.  With this in mind, I would start
> by defining something along the lines of a "Serial Flash transfer".  Some
> initial ramblings of my mind:
> 
> /**
>  * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
>  * @wren:		command for "Write Enable", or 0x00 for not required
>  * @cmd:		command for operation
>  * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
>  * @addr:		address for operation
>  * @addr_pins:		number of pins to send @addr (1, 2, 4)
>  * @addr_width: 	number of address bytes (3,4, or 0 for address not
> required) * @mode:		mode data
>  * @mode_pins:		number of pins to send @mode (1, 2, 4)
>  * @mode_cycles:	number of mode cycles (0 for mode not required)
>  * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
>  */
> struct spi_nor_xfer_cfg {
> 	uint8_t		wren;
> 	uint8_t		cmd;
> 	int		cmd_pins;
> 	uint32_t	addr;
> 	int		addr_pins;
> 	int		addr_width;
> 	uint32_t	mode;
> 	int		mode_pins;
> 	int		mode_cycles;
> 	int		dummy_cycles;
> };
> 
> We could then build up layers of functionality, based on two fundamental
> primitives:
> 
> 	int (*read_xfer)(struct spi_nor_info *info,
> 			 struct spi_nor_xfer_cfg *cfg,
> 			 uint8_t *buf, size_t len);
> 
> 	int (*write_xfer)(struct spi_nor_info *info,
> 			  struct spi_nor_xfer_cfg *cfg,
> 			  uint8_t *buf, size_t len);
> 
> Default implementations for standard operations could follow:
> 
> 	int (*read_reg)(struct spi_nor_info *info,
> 			uint8_t cmd, uint8_t *reg, int len);
> 	int (*write_reg)(struct spi_nor_info *info,
> 			 uint8_t cmd, uint8_t *reg, int len,
> 			 int wren, int wtr);
> 
> 	int (*readid)(struct spi_nor_info *info, uint8_t *readid);
> 	int (*wait_till_ready)(struct spi_nor_info *info);
> 
> 	int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs, size_t
> len); int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
> 	int (*erase_sector)(struct spi_nor_info *info, off_t offs);
> 
> Each driver would be required to implement read_xfer() and write_xfer(),
> and then free to either use the default implementations, or override with
> driver-optimised implementations.

I checked the VYBRIDRM.pdf datasheet. I grok down that the thing Vybrid does is 
it has such a lookup table. The table has 16 slots , each can contain up to 8 
SPI NOR instructions . These instructions are programmed when the driver 
probe()s I suppose. To do a communication with the SPI NOR, you give the 
controller an index in this lookup table and it will execute this pre-programmed 
sequence of commands on the SPI NOR.

So yes, your API would work nicely with Vybrid. I agree about read_reg() and 
write_reg() functions, they will be needed for reading the standard CR/SR etc 
registers on the SPI NOR and also SPI NOR specific cruft if need be. As for 
readid(), I wonder if we cannot implement it via some generalized read_reg() 
call, I suspect we can for starters. Same goes for wait_till_read(), let's 
implement it in the spi-nor framework first and only if a piece of hardware can 
speed this up significantly by re-implementing it itself, they will change the 
framework.

The read() and write() calls are nice, something like mmap() might be 
interesting here as well, since based on what Poddar explained, his controller 
can somehow map the SPI flash into the CPU address space. I don't know how this 
should work precisely, but again, this can be left until such controller driver 
hits mainline and the benefits of such implementation would become obvious.

Finally, the erase_sector() call would probably need to be extended. I would 
rename it to simple erase(), since we can erase from sub-sectors (4K) to whole 
chip.

btw. I really like the idea of the new SPI NOR framework. The M25P80 driver 
could then be rewritten and plugged into the SPI NOR framework just like any 
other SPI NOR controller driver with "advanced capabilities".

> In the case of a pure SPI Controller, read_xfer() and write_xfer() would
> simply flatten to spi_messages.  The key benefit is that dedicated Serial
> Flash Controllers would be able to take advantage of the richer semantics
> offered by the 'spi_nor_xfer_cfg' data.
> 
> I would also expect the spi-nor framework to provide the following
> services, applicable to all Serial Flash drivers:
> 
> 	- determine devices properties at runtime, based on the READID data (and
> perhaps SFDP, if and when it matures!).  This should include supported
> opcodes (e.g. READ_1_1_4, READ_1_4_4, READ4B_1_1_4...), operating
> frequencies, mode and dummy requirements, means of setting Quad Enable,
> entering/exiting 4-byte addressing etc.
> 
> 	- provide optimal read, write and erase configurations, based on the
> combined capabilities of the Serial Flash device and the H/W Controller.

Talking about read-write-erase, why don't we -- instead of passing buffer -- 
pass a scatterlist ? I suppose that might make it easier for these "advanced" 
controller to do DMA on it.

> 	- device specific configuration (e.g. setting QE, unlock BPx bits, DYB
> unlocking).
> 
> Getting back to reality, I realise undertaking such a task would be a huge
> commitment.  I would be keen to put forward my own proposal, but current
> circumstances mean that that is unlikely to happen any time soon.

I'd love to take this on, but I need to flush my pipeline first ;-( Besides, it 
seems we have an abundance of good engineers here already, so maybe someone will 
actually write it ;-)

> I guess in summary, while I am pleased that this area is being looked at,
> my own feeling is that proposed framework needs to be reworked for it to
> be generally applicable to other Serial Flash controllers.  Of course,
> another option would be to stick with what is currently offered, and make
> do.

Full ACK.

> Cheers,
> 
> Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年12月03日 08:32, Marek Vasut 写道:
> Finally, the erase_sector() call would probably need to be extended. I would
> rename it to simple erase(), since we can erase from sub-sectors (4K) to whole
> chip.
yes. this is what i am coding now. I rename it to erase().

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年11月29日 22:52, Angus Clark 写道:
> 	int (*write_reg)(struct spi_nor_info *info,
> 			 uint8_t cmd, uint8_t *reg, int len,
> 			 int wren, int wtr);
I guess you add the 'wren' for the issuing write-enable before issuing 
the 'cmd'.

but what's 'wtr' for? i do not know what's the meaning of the 'wtr'.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

On 12/04/2013 02:46 AM, Huang Shijie wrote:
> 于 2013年11月29日 22:52, Angus Clark 写道:
>>     int (*write_reg)(struct spi_nor_info *info,
>>              uint8_t cmd, uint8_t *reg, int len,
>>              int wren, int wtr);
> I guess you add the 'wren' for the issuing write-enable before issuing
> the 'cmd'.
> 
> but what's 'wtr' for? i do not know what's the meaning of the 'wtr'.
> 

'wtr' is for "Wait 'til Ready".  Some register writes are instant, while others
require polling of the "Write In Progress" bit.

Cheers,

Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Gupta, Pekon]

>From: Angus Clark [mailto:angus.clark@st.com]
>>On 12/04/2013 02:46 AM, Huang Shijie wrote:
>> 于 2013年11月29日 22:52, Angus Clark 写道:
>>>     int (*write_reg)(struct spi_nor_info *info,
>>>              uint8_t cmd, uint8_t *reg, int len,
>>>              int wren, int wtr);
>> I guess you add the 'wren' for the issuing write-enable before issuing
>> the 'cmd'.
>>
>> but what's 'wtr' for? i do not know what's the meaning of the 'wtr'.
>>
>
>'wtr' is for "Wait 'til Ready".  Some register writes are instant, while others
>require polling of the "Write In Progress" bit.
>
Unless a register is controlling a status of internal state-machine, or something
It should be instantaneously writable.

Also polling should not be part of this particular *(write_reg), if a register
needs to be polled then it should be part of *(write) or *(read) .. Like 
WIP: Write-in-progress bit of flash
generic_flash_write(...) {
	while ((read_reg(STATUS_REG) & WIP) || ~timeout) {
		timeout--;
	};


With this it just came to my mind, that you also need a 'timeout' field
in 'struct spinor-cfg'. This is because different controllers would be
running at different clock speeds (and different SPI_CLK frequencies).
So they need to specify a 'timeout' value for all polling accesses.


with regards, pekon

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Pekon,

On 12/04/2013 07:19 AM, Gupta, Pekon wrote:
> Unless a register is controlling a status of internal state-machine, or something
> It should be instantaneously writable.

Yes, that is my point.  Some register writes are instant, other are not,
particularly those that involve non-volatile status or configuration bits.

> Also polling should not be part of this particular *(write_reg), if a register
> needs to be polled then it should be part of *(write) or *(read) .. Like 
> WIP: Write-in-progress bit of flash
> generic_flash_write(...) {
> 	while ((read_reg(STATUS_REG) & WIP) || ~timeout) {
> 		timeout--;
> 	};

I am in two minds about where the WTR loop should live.  On the one hand, moving
it to the "generic_flash_write()" call offers H/W controllers the opportunity to
optimise the polling, if they support such a feature (I know of some that will
in the future).

However, requiring the H/W driver to perform the polling, including knowledge of
the STATUS CMD, and the WIP bit-field, just seems like moving too much
"intelligence" away from the spi-nor layer.  What concerns me is when we get on
to reading/clearing error flags (as now mandated on some Serial Flash devices,
e.g. n25p512 and n25q00a), this will also need to be part of the generic write.

> With this it just came to my mind, that you also need a 'timeout' field
> in 'struct spinor-cfg'. 

Yes, that is a good point.

I would propose that the spi-nor layer retains responsibility for determining
whether or not 'WTR' is required for a particular operation, and a suitable
timeout.  At some point, during initialisation, the H/W driver will need to
inform the spi-nor layer about its capabilities (e.g. support for DUAL or QUAD
mode, any warm-reset requirements, etc), and native support of WIP polling could
be part of this set.  The spi-nor layer could then decide whether to add the WTR
flag and timeout to the spinor-cfg transfer, or to take responsibility itself,
and execute its own polling loop.  This gives greatest flexibility, which seems
to be an important factor, while retaining the knowledge in the spi-nor layer.

Cheers,

Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

On Wednesday, December 04, 2013 at 09:21:05 AM, Angus Clark wrote:
> Hi Pekon,
> 
> On 12/04/2013 07:19 AM, Gupta, Pekon wrote:
> > Unless a register is controlling a status of internal state-machine, or
> > something It should be instantaneously writable.
> 
> Yes, that is my point.  Some register writes are instant, other are not,
> particularly those that involve non-volatile status or configuration bits.
> 
> > Also polling should not be part of this particular *(write_reg), if a
> > register needs to be polled then it should be part of *(write) or
> > *(read) .. Like WIP: Write-in-progress bit of flash
> > generic_flash_write(...) {
> > 
> > 	while ((read_reg(STATUS_REG) & WIP) || ~timeout) {
> > 	
> > 		timeout--;
> > 	
> > 	};
> 
> I am in two minds about where the WTR loop should live.  On the one hand,
> moving it to the "generic_flash_write()" call offers H/W controllers the
> opportunity to optimise the polling, if they support such a feature (I
> know of some that will in the future).

I disagree we should bloat the API with features, that will be used by "possible 
future controllers" or "possible single controller", right from the start. The 
real question here is, can the API be designed in such a way, that the SR 
polling will happen in software NOW and only when a controller appears that can 
do polling in HW will the API be extended to support it ?

> However, requiring the H/W driver to perform the polling, including
> knowledge of the STATUS CMD, and the WIP bit-field, just seems like moving
> too much "intelligence" away from the spi-nor layer.  What concerns me is
> when we get on to reading/clearing error flags (as now mandated on some
> Serial Flash devices, e.g. n25p512 and n25q00a), this will also need to be
> part of the generic write.
> 
> > With this it just came to my mind, that you also need a 'timeout' field
> > in 'struct spinor-cfg'.
> 
> Yes, that is a good point.
> 
> I would propose that the spi-nor layer retains responsibility for
> determining whether or not 'WTR' is required for a particular operation,
> and a suitable timeout.  At some point, during initialisation, the H/W
> driver will need to inform the spi-nor layer about its capabilities (e.g.
> support for DUAL or QUAD mode, any warm-reset requirements, etc), and
> native support of WIP polling could be part of this set.  The spi-nor
> layer could then decide whether to add the WTR flag and timeout to the
> spinor-cfg transfer, or to take responsibility itself, and execute its own
> polling loop.  This gives greatest flexibility, which seems to be an
> important factor, while retaining the knowledge in the spi-nor layer.

Sounds good!

> Cheers,
> 
> Angus

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年12月04日 23:36, Marek Vasut 写道:
> I disagree we should bloat the API with features, that will be used by "possible
> future controllers" or "possible single controller", right from the start. The
> real question here is, can the API be designed in such a way, that the SR
> polling will happen in software NOW and only when a controller appears that can
> do polling in HW will the API be extended to support it ?
ok. Let's add this feature in the future when a real case occurs.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Gupta, Pekon]

>From: Huang Shijie [mailto:b32955@freescale.com]
>
>>于 2013年12月04日 23:36, Marek Vasut 写道:
>> I disagree we should bloat the API with features, that will be used by "possible
>> future controllers" or "possible single controller", right from the start. The
>> real question here is, can the API be designed in such a way, that the SR
>> polling will happen in software NOW and only when a controller appears that can
>> do polling in HW will the API be extended to support it ?
>ok. Let's add this feature in the future when a real case occurs.
>
Sorry got lost.. Can you please summarize following plans for your next patch:

(1) Polling of status registers should be 
  (a) done in S/W (generic driver)
  (b) done controller driver, which can be optimized for specific hardware

(2) How do you plan to have timeout value, (as serial flash can be pretty slow
   to access) ?
  (a) independent for each access
       - read timeouts per sector, which can be multiplied with read_len/sector_size
       - write timeout per sector, which can be multiplied with write_len/sector_size
       - erase timeout per block, which can be multiplied with erase_len/block_size
  (b) Any other method, or leave it to controller driver ?

In my view, before proceeding to writing down a patch, I think
we should come-up with a doc/Readme, which defines all API and interface
structs (taking Angus' proposal as baseline). And then refine that spec first.
Otherwise there may be too many revisions spent on patch RFC's .. 

Any thoughts ?
And is there a space in kernel_docs for such specs ?

With regards, pekon

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 05:43:05AM +0000, Gupta, Pekon wrote:
> >From: Huang Shijie [mailto:b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org]
> >
> >>于 2013年12月04日 23:36, Marek Vasut 写道:
> >> I disagree we should bloat the API with features, that will be used by "possible
> >> future controllers" or "possible single controller", right from the start. The
> >> real question here is, can the API be designed in such a way, that the SR
> >> polling will happen in software NOW and only when a controller appears that can
> >> do polling in HW will the API be extended to support it ?
> >ok. Let's add this feature in the future when a real case occurs.
> >
> Sorry got lost.. Can you please summarize following plans for your next patch:
> 
> (1) Polling of status registers should be 
>   (a) done in S/W (generic driver)
>   (b) done controller driver, which can be optimized for specific hardware
> 
For (b), we can implement the @wait_till_ready hook for the controller driver.

For (a), we use the default hook for @wait_till_ready.



> (2) How do you plan to have timeout value, (as serial flash can be pretty slow
>    to access) ?
>   (a) independent for each access
>        - read timeouts per sector, which can be multiplied with read_len/sector_size
>        - write timeout per sector, which can be multiplied with write_len/sector_size
>        - erase timeout per block, which can be multiplied with erase_len/block_size
>   (b) Any other method, or leave it to controller driver ?
> 

Since i have no idea about this kind of spi-nor controller, so I prefer the (b). 

> In my view, before proceeding to writing down a patch, I think
> we should come-up with a doc/Readme, which defines all API and interface
> structs (taking Angus' proposal as baseline). And then refine that spec first.
> Otherwise there may be too many revisions spent on patch RFC's .. 
> 
> Any thoughts ?
> And is there a space in kernel_docs for such specs ?

maybe the best solution is I send out the new version as soon as possible.

thanks
Huang Shijie


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html