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

Peter Pan
2017-02-21T09:41:19Z

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?

Thanks,
Peter Pan