[PATCH v4 9/9] mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

Hi Pratyush,

On 2/2/2021 4:40 AM, Pratyush Yadav wrote:
> On 28/01/21 01:37PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>
>> Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny.
>> The volatile QE bit function, spansion_quad_enable_volatile() supports
>> dual/quad die package parts, by taking 'die_size' parameter that is used
>> to iterate register update for all dies in the device.
> 
> I'm not so sure if this is a good idea. spi-nor-tiny should be the 
> minimal set of functionality to get the bootloader to the next stage. 
> 1S-1S-1S mode is sufficient for that. Adding quad enable functions of 
> all the flashes will increase the size quite a bit. I know that some 
> flashes already have their quad enable hooks, and I don't think they 
> should be there either.
> 
> Of course, the maintainers have the final call, but from my side,
>  
> Nacked-by: Pratyush Yadav <p.yadav at ti.com>
> 
I understand your point. Let's leave the decision up to the maintainers.

> Anyway, comments below in case the maintainers do plan on picking this 
> patch up.
> 
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 89 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
>> index 5cc2b7d996..66680df5a9 100644
>> --- a/drivers/mtd/spi/spi-nor-tiny.c
>> +++ b/drivers/mtd/spi/spi-nor-tiny.c
>> @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>>  }
>>  #endif /* CONFIG_SPI_FLASH_SPANSION */
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @die_size:	maximum number of bytes per die ('mtd.size' > 'die_size' in
>> + *              multi die package parts).
>> + * @dummy:	number of dummy cycles for register read
>> + *
>> + * It is recommended to update volatile registers in the field application due
>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>> + * function sets Quad Enable bit in CFR1 volatile.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
>> +					 u8 dummy)
>> +{
>> +	struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),
>> +				   SPI_MEM_OP_ADDR(4, 0, 1),
>> +				   SPI_MEM_OP_DUMMY(0, 1),
>> +				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
>> +	u32 addr;
>> +	u8 cr;
>> +	int ret;
>> +
>> +	/* Use 4-byte address for RDAR/WRAR */
>> +	ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while enabling 4-byte address\n");
>> +		return ret;
>> +	}
>> +
>> +	for (addr = 0; addr < nor->mtd.size; addr += die_size) {
>> +		op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;
> 
> So here you add the register offset to the base address, instead of 
> ORing it. Ok.
> 
>> +
>> +		/* Check current Quad Enable bit value. */
>> +		op.cmd.opcode = SPINOR_OP_RDAR;
>> +		op.dummy.nbytes = dummy / 8;
>> +		op.data.dir = SPI_MEM_DATA_IN;
>> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +		if (ret < 0) {
>> +			dev_dbg(nor->dev,
>> +				"error while reading configuration register\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (cr & CR_QUAD_EN_SPAN)
>> +			return 0;
>> +
>> +		/* Write new value. */
>> +		cr |= CR_QUAD_EN_SPAN;
>> +		op.cmd.opcode = SPINOR_OP_WRAR;
>> +		op.dummy.nbytes = 0;
>> +		op.data.dir = SPI_MEM_DATA_OUT;
>> +		write_enable(nor);
>> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +		if (ret < 0) {
>> +			dev_dbg(nor->dev,
>> +				"error while writing configuration register\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Read back and check it. */
>> +		op.data.dir = SPI_MEM_DATA_IN;
>> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +		if (ret || !(cr & CR_QUAD_EN_SPAN)) {
>> +			dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
> 
> LGTM.
> 
>>  static void
>>  spi_nor_set_read_settings(struct spi_nor_read_command *read,
>>  			  u8 num_mode_clocks,
>> @@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor,
>>  		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>>  					  0, 8, SPINOR_OP_READ_FAST,
>>  					  SNOR_PROTO_1_1_1);
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +		if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS &&
>> +		    (info->id[1] == 0x2a || info->id[1] == 0x2b))
> 
> Add a comment about which flash models these two are.
> 
Will do.

>> +			params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>> +#endif
>>  	}
>>  
>>  	if (info->flags & SPI_NOR_QUAD_READ) {
>> @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
>>  		case SNOR_MFR_MACRONIX:
>>  			err = macronix_quad_enable(nor);
>>  			break;
>> +#endif
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +		case SNOR_MFR_CYPRESS:
>> +			err = spansion_quad_enable_volatile(nor, SZ_128M, 0);
>> +			break;
>>  #endif
>>  		case SNOR_MFR_ST:
>>  		case SNOR_MFR_MICRON:
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro