Re: [PATCH V2 2/4] mtd: spi-nor: add the basic data structures

[Marek Vasut <marex@denx.de>]

On Wednesday, December 11, 2013 at 07:24:31 AM, Huang Shijie wrote:
> On Tue, Dec 10, 2013 at 02:07:59PM +0100, Marek Vasut wrote:
> > On Friday, December 06, 2013 at 09:32:42 AM, Huang Shijie wrote:
> > > The spi_nor{} is cloned from the m25p{}.
> > > +	struct spi_nor_xfer_cfg	cfg;
> > > +
> > > +	/* for write_reg */
> > > +	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> > > +
> > > +	/* the two fundamental primitives */
> > > +	int (*read_xfer)(struct spi_nor *nor, struct spi_nor_xfer_cfg *cfg,
> > > +			 u8 *buf, size_t len);
> > > +	int (*write_xfer)(struct spi_nor *nor, struct spi_nor_xfer_cfg *cfg,
> > > +			  u8 *buf, size_t len);
> > > +
> > > +	int (*read_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len);
> > > +	int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
> > > +			int write_enable);
> > 
> > Does the vybrid really support accelerated SPI NOR register IO or does it
> 
> The vybrid does not support the accelerated SPI NOR register IO.
> these hooks are for Lee's driver.

Which one please?

> > emulate it with read/write or read_xfer/write_xfer accessors ? If the
> > later, just drop this stuff for now.
> 
> What is the "stuff" mean? the "write_enable" ?

This stuff -- I mean 'read SPI NOR register' and 'write SPI NOR register'. I 
would love to see an API which grows as needed, not an API which is bloated by 
unused function right from the start.

> > > +	const struct spi_device_id *(*read_id)(struct spi_nor *nor);
> > > +	int (*wait_till_ready)(struct spi_nor *nor);
> > > +
> > > +	/* write */
> > > +	void (*write)(struct spi_nor *nor, loff_t to,
> > > +			size_t len, size_t *retlen, const u_char *buf);
> > > +	/* read */
> > > +	int (*read)(struct spi_nor *nor, loff_t from,
> > > +			size_t len, size_t *retlen, u_char *buf);
> > > +	/* erase */
> > > +	int (*erase)(struct spi_nor *nor, loff_t offs);
> > 
> > How do you specify what to erase (sub-sector, sector, whole chip) here ?
> 
> I use a write_reg(OPCODE_ERASE_CHIP) to erase the whole chip.
> Please read the patch 3.
> 
> This hook is used to erase a sector.

I suspect we might need a much better documentation for this structure, it is 
hardly clear which function does what ;-)

> Btw, what is the sub-sector mean? which opcodes for the sub-sector?

On large chips with for example 64K big sectors, you can erase a sub-sector 
area, usually a 4K one. This reduces the wear of the chip. See these defines in 
m25p80.c :

  46 #define OPCODE_BE_4K            0x20    /* Erase 4KiB block */
  48 #define OPCODE_BE_32K           0x52    /* Erase 32KiB block */
  49 #define OPCODE_CHIP_ERASE       0xc7    /* Erase whole flash chip */

Best regards,
Marek Vasut