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

Boris Brezillon
2017-02-21T10:51:33Z

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();