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

Arnaud Mouiche
2017-02-21T09:01:39Z

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 ?

Regards,
Arnaud