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:

quoted

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

quoted

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