Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Ezequiel Garcia <ezequiel.garcia@imgtec.com>]

On 12/22/2014 01:34 AM, Qi Wang 王起 (qiwang) wrote:
> Hi Ezequiel,
> 
>> +static struct nand_ecclayout ecc_layout_mt29f = {
>> +	.eccbytes = 32,
>> +	.eccpos = {
>> +		8, 9, 10, 11, 12, 13, 14, 15,
>> +		24, 25, 26, 27, 28, 29, 30, 31,
>> +		40, 41, 42, 43, 44, 45, 46, 47,
>> +		56, 57, 58, 59, 60, 61, 62, 63,
>> +	 },
> 
> Seems "OOB free" variable is missed. As below:
> 
> .oobfree = {
> { .offset = 2, .length = 6 },
> { .offset = 16, .length = 8 },
> { .offset = 32, .length = 8 },
> { .offset = 48, .length = 8 },
> },
> 

OK, great.

Have you tested this and made sure it works for MT29F and allows to
access the OOB?

> 
>> +static int spi_nand_send_command(struct spi_device *spi, int command,
>> +				 struct spi_nand_device_cmd *cmd) {
>> +	struct spi_message message;
>> +	struct spi_transfer x[4];
>> +
>> +	spi_message_init(&message);
>> +	memset(x, 0, sizeof(x));
>> +
>> +	/* Command */
>> +	cmd->cmd = command;
>> +	x[0].len = 1;
>> +	x[0].tx_buf = &cmd->cmd;
>> +	spi_message_add_tail(&x[0], &message);
>> +
>> +	/* Address */
>> +	if (cmd->n_addr) {
>> +		x[1].len = cmd->n_addr;
>> +		x[1].tx_buf = cmd->addr;
>> +		spi_message_add_tail(&x[1], &message);
>> +	}
>> +
>> +	/* Data to be transmitted */
>> +	if (cmd->n_tx) {
>> +		x[3].len = cmd->n_tx;
>> +		x[3].tx_buf = cmd->tx_buf;
>> +		spi_message_add_tail(&x[3], &message);
>> +	}
>> +
>> +	/* Data to be received */
>> +	if (cmd->n_rx) {
>> +		x[3].len = cmd->n_rx;
>> +		x[3].rx_buf = cmd->rx_buf;
>> +		spi_message_add_tail(&x[3], &message);
>> +	}
> 
> Only x[3] is used in above code, I suggest to separate transfer and received 
> For x[2] and x[3], in case some command need to send dummy byte before received any data.
> 

Ah, yes. Good catch.

>> +static int spi_nand_device_block_erase(struct spi_nand *snand,
>> unsigned
>> +int page_addr) {
>> +	struct spi_nand_device *snand_dev = snand->priv;
>> +	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
>> +
>> +	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
>> +	cmd->n_addr = 3;
>> +	cmd->addr[0] = 0;
> 
> Page address may beyond 16 bit in large density product. 
> So I don't think addr[0] can be forced to be set 0
> 

Right, this was a bad copy paste from the staging driver.

> 
>> +static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf) {
>> +	struct spi_nand_device *snand_dev = snand->priv;
>> +	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
>> +
>> +	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
>> +	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
>> +	cmd->rx_buf = buf;
>> +
> 
> Micron SPI NAND need 1 more dummy byte before received ID data.
> Code for mt29f_read_id should be like this:
> 
> static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf) {
> 	struct spi_nand_device *snand_dev = snand->priv;
> 	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> 	u8 dummy = 0;
> 	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> "
> 	cmd->n_tx = 1;
> 	cmd->tx_buf = &dummy;
> "
> 	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
> 	cmd->rx_buf = buf;
> 

That's true. However, notice the staging driver does not use any dummy
byte, and instead fakes it by reading an extra byte.

Maybe you can test this on a MT29F and point at all the required fixes?

Thanks a lot for the feedback!
-- 
Ezequiel