Re: [PATCH linux-next v4 3/5] mtd: spi-nor: allow to tune the number of dummy cycles

[Cyrille Pitchen <cyrille.pitchen@atmel.com>]

Hi Marek,

Le 24/08/2015 12:48, Marek Vasut a écrit :
> On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote:
>> The number of dummy cycles used during Fast Read commands can be reduced
>> to improve transfer performances. Each manufacturer has a dedicated set of
>> registers to provide the memory with the exact number of dummy cycles it
>> should expect. Both the memory and the (Q)SPI controller must agree on
>> this number of dummy cycles.
>>
>> The number of dummy cycles can be found into the memory datasheet and
>> mostly depends on the SPI clock frequency, the Fast Read op code and the
>> Single/Dual Data Rate mode.
>>
>> Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would
>> only provide the driver with a high enough number of dummy cycles for each
>> Fast Read command to be used for all clock frequencies: this solution
>> would not be optimized.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> 
> Hi!
> 
>>  drivers/mtd/spi-nor/spi-nor.c | 97
>> ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h  
>> |  2 +
>>  2 files changed, 80 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index e2a6029dc056..869e098a6841 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor)
>>  }
>>
>>  /*
>> - * Dummy Cycle calculation for different type of read.
>> - * It can be used to support more commands with
>> - * different dummy cycle requirements.
>> - */
>> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
>> -{
>> -	switch (nor->flash_read) {
>> -	case SPI_NOR_FAST:
>> -	case SPI_NOR_DUAL:
>> -	case SPI_NOR_QUAD:
>> -		return 8;
>> -	case SPI_NOR_NORMAL:
>> -		return 0;
>> -	}
>> -	return 0;
>> -}
> 
> You can probably just soup up this function so that it sets the
> nor->read_dummy, no ?
>

Actually, this is what the patch does: spi_nor_read_dummy_cycles() was reused
and enhanced few lines below where you've pointed out the 
"switch (nor->flash_read)" block should be move after the else block.

I think when I wrote the code I've chosen to move the definition of this
function instead of adding forward declarations of functions such as read_cr()
or write_sr_cr(), which are now called by micron_set_dummy_cycles().

>> -/*
>>   * Write status register 1 byte
>>   * Returns negative if error occurred.
>>   */
>> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct
>> flash_info *info) }
>>  }
>>
>> +static int micron_set_dummy_cycles(struct spi_nor *nor)
>> +{
>> +	int ret;
>> +	u8 val, mask;
>> +
>> +	/* read the Volatile Configuration Register (VCR) */
> 
> NIT: If this is a sentence, start it with capital letter and end it with 
> fullstop :)
> 

done for the next version

>> +	ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1);
>> +	if (ret < 0) {
>> +		dev_err(nor->dev, "error %d reading VCR\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	write_enable(nor);
>> +
>> +	/* update the number of dummy into the VCR */
> 
> DTTO
> 

done for the next version

>> +	mask = GENMASK(7, 4);
>> +	val &= ~mask;
>> +	val |= (nor->read_dummy << 4) & mask;
>> +	ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0);
>> +	if (ret < 0) {
>> +		dev_err(nor->dev, "error while writing VCR register\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = spi_nor_wait_till_ready(nor);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Dummy Cycle calculation for different type of read.
>> + * It can be used to support more commands with
>> + * different dummy cycle requirements.
>> + */
>> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor,
>> +				     const struct flash_info *info)
>> +{
>> +	struct device_node *np = nor->dev->of_node;
>> +	u32 num_dummy_cycles;
>> +
>> +	if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles",
>> +					&num_dummy_cycles)) {
>> +		nor->read_dummy = num_dummy_cycles;
>> +
>> +		/*
>> +		 * This switch block might be moved after the if...then...else
>> +		 * statement but it was not tested with all Spansion or Micron
>> +		 * memories.
>> +		 * Now the "m25p,num-dummy-cycles" property needs to be
>> +		 * explicitly set in the device tree so the switch statement is
>> +		 * executed. This should avoid unwanted side effects and keep
>> +		 * backward compatibility.
>> +		 */
>> +		switch (JEDEC_MFR(info)) {
>> +		case CFI_MFR_ST:
>> +			return micron_set_dummy_cycles(nor);
>> +		default:
> 
> If you do have m25p,num-dummy-cycles set for non-micron flash, you have a 
> problem here I believe.
> 
>> +			break;
>> +		}
>> +	} else {
> 
> The solution would be to drop this else {} bit here, so that if you fail in
> the DT-based configuration, you fall back to this old behavior. What do you 
> think please ? :)
> 

Good idea!
I also add a trace for the default case of "switch (JEDEC_MFR(info))":

dev_warn(dev, "can't set the number of dummy cycles\n");

So the user is notified that the driver could not use the value of
"m25p,num-dummy-cycles" from the DT before falling back to the legacy
code.

>> +		switch (nor->flash_read) {
>> +		case SPI_NOR_FAST:
>> +		case SPI_NOR_DUAL:
>> +		case SPI_NOR_QUAD:
>> +			nor->read_dummy = 8;
>> +		case SPI_NOR_NORMAL:
>> +			nor->read_dummy = 0;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> 
> [...]
> 

thanks for the review!

Best regards,

Cyrille