Re: [PATCH 03/11] nand: spi: Abstract SPI NAND cmd set to functions

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 21 Feb 2017 11:22:52 +0100
Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:

> On 21/02/2017 10:40, Peter Pan wrote:
> > Hi Arnaud,
> >
> > On Tue, Feb 21, 2017 at 5:01 PM, Arnaud Mouiche
> > <arnaud.mouiche@gmail.com> wrote:  
> >> Hello Peter.
> >> First thank you for this effort to finally bring the spinand framework back
> >> on stage.
> >>
> >> On 21/02/2017 09:00, Peter Pan wrote:  
> >>> This commit abstracts basic SPI NAND commands to
> >>> functions in spi-nand-base.c. Because command sets
> >>> have difference by vendors, we create spi-nand-cmd.c
> >>> to define this difference by command config table.
> >>>
> >>> [...]
> >>> +
> >>> +/**
> >>> + * spi_nand_read_page_to_cache - send command 13h to read data from Nand
> >>> + * to cache
> >>> + * @chip: SPI-NAND device structure
> >>> + * @page_addr: page to read
> >>> + */
> >>> +static int spi_nand_read_page_to_cache(struct spi_nand_chip *chip,
> >>> +                                       u32 page_addr)
> >>> +{
> >>> +       struct spi_nand_cmd cmd;
> >>> +
> >>> +       memset(&cmd, 0, sizeof(struct spi_nand_cmd));
> >>> +       cmd.cmd = SPINAND_CMD_PAGE_READ;
> >>> +       cmd.n_addr = 3;
> >>> +       cmd.addr[0] = (u8)(page_addr >> 16);
> >>> +       cmd.addr[1] = (u8)(page_addr >> 8);
> >>> +       cmd.addr[2] = (u8)page_addr;
> >>> +
> >>> +       return spi_nand_issue_cmd(chip, &cmd);
> >>> +}  
> >>
> >> Just a question. Don't you try to map too exactly the spi_nand_cmd to the
> >> internal micron SPI command ?
> >> Why "cmd.addr" should be a byte array, and not a u32, leaving each chip to
> >> details to handle the u32 to SPI byte stream mapping.
> >>
> >>
> >>  
> >>> +
> >>> +/**
> >>> + * spi_nand_read_from_cache - read data out from cache register
> >>> + * @chip: SPI-NAND device structure
> >>> + * @page_addr: page to read
> >>> + * @column: the location to read from the cache
> >>> + * @len: number of bytes to read
> >>> + * @rbuf: buffer held @len bytes
> >>> + * Description:
> >>> + *   Command can be 03h, 0Bh, 3Bh, 6Bh, BBh, EBh
> >>> + *   The read can specify 1 to (page size + spare size) bytes of data
> >>> read at
> >>> + *   the corresponding locations.
> >>> + *   No tRd delay.
> >>> + */
> >>> +static int spi_nand_read_from_cache(struct spi_nand_chip *chip,
> >>> +               u32 page_addr, u32 column, size_t len, u8 *rbuf)
> >>> +{
> >>> +       struct nand_device *nand = &chip->base;
> >>> +       struct spi_nand_cmd cmd;
> >>> +
> >>> +       memset(&cmd, 0, sizeof(struct spi_nand_cmd));
> >>> +       cmd.cmd = chip->read_cache_op;
> >>> +       cmd.n_addr = 2;
> >>> +       cmd.addr[0] = (u8)(column >> 8);
> >>> +       if (chip->options & SPINAND_NEED_PLANE_SELECT)
> >>> +               cmd.addr[0] |= (u8)((nand_page_to_eraseblock(nand,
> >>> page_addr)
> >>> +                               & 0x1) << 4);
> >>> +       cmd.addr[1] = (u8)column;
> >>> +       cmd.n_rx = len;
> >>> +       cmd.rx_buf = rbuf;
> >>> +
> >>> +       return spi_nand_issue_cmd(chip, &cmd);
> >>> +}
> >>> +  
> >> Some reasons to handle the plane concept (Micron specific), and even move
> >> the fact that it touch the bit 4 of cdm.addr[0], in common code ?  
> > Let me try to remove this from framework when separate controller and device
> > related code. Any suggestion on this?  
> I know, Micron was the first spinand chipset available, will be the 
> first chipset supported by this framework, but it is already not so 
> generic regarding to the page addressing compared with winbond, esmt, 
> gigadevice or micronix...
> 
> But may be this the sign that "generic_spi_nand_cmd_fn" if "too" 
> straight in your implementation and just try to map the "struct 
> spi_nand_cmd" into the SPI frame without any more place for chip 
> specialization.
> 
> I guess I would prefer to see multiple 'command primitive' inside each 
> chip structures (eg. chip->read_from_cache() and so on).

I'm not against the idea, but I'd prefer to see these operations in a
separate struct:

struct spinand_manufacturer_ops {
	int (*read_from_cache)();
	/* ... */
};

struct spinand_device {
	/* ... */

	struct {
		struct spinand_manufacturer_ops *ops;
		void *priv;
		/* ... */
	} manufacturer;
};

> But in the meantime, spinand chipset are made to be generic and behave 
> mostly the same way, which would make this primitive declaration a 
> little bit redundant.

Well, for the generic case, you could either provide generic helpers
that could be used by NAND manufacturer drivers in their
spinand_manufacturer_ops definition, or you could let the core do

	if (!nand->manufacturer->ops || !nand->manufacturer->ops->func)
		default_func();