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

[Arnaud Mouiche <arnaud.mouiche@gmail.com>]

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).
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.

The other way is to already have a "Micron_spi_nand_cmd_fn" that will 
patch the address fields for "read cache op", and then call the 
"generic_spi_nand_cmd_fn" to finalize the job.

Arnaud

>
> Thanks,
> Peter Pan