Re: [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib

[Cyrille Pitchen]

Le 07/12/2016 Ã 17:32, Marek Vasut a Ãcrit :
> On 12/07/2016 05:29 PM, Cyrille Pitchen wrote:
>> Le 07/12/2016 Ã 17:20, Marek Vasut a Ãcrit :
>>> On 12/06/2016 05:52 PM, Cyrille Pitchen wrote:
>>>> This patch provides an alternative mean to support memory above 16MiB
>>>> (128Mib) by replacing 3byte address op codes by their associated 4byte
>>>> address versions.
>>>>
>>>> Using the dedicated 4byte address op codes doesn't change the internal
>>>> state of the SPI NOR memory as opposed to using other means such as
>>>> updating a Base Address Register (BAR) and sending command to enter/leave
>>>> the 4byte mode.
>>>>
>>>> Hence when a CPU reset occurs, early bootloaders don't need to be aware
>>>> of BAR value or 4byte mode being enabled: they can still access the first
>>>> 16MiB of the SPI NOR memory using the regular 3byte address op codes.
>>>>
>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>
>>>> Tested-by: Vignesh R <vigneshr@xxxxxx>
>>>> ---
>>>>  drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 93 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 8abe134e174a..606c030c566d 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -75,6 +75,10 @@ struct flash_info {
>>>>  					 * bit. Must be used with
>>>>  					 * SPI_NOR_HAS_LOCK.
>>>>  					 */
>>>> +#define SPI_NOR_4B_OPCODES	BIT(10)	/*
>>>> +					 * Use dedicated 4byte address op codes
>>>> +					 * to support memory size above 128Mib.
>>>> +					 */
>>>>  };
>>>>  
>>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>>> @@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>>>>  	return mtd->priv;
>>>>  }
>>>>  
>>>> +
>>>> +struct spi_nor_address_entry {
>>>> +	u8	src_opcode;
>>>> +	u8	dst_opcode;
>>>> +};
>>>> +
>>>> +static u8 spi_nor_convert_opcode(u8 opcode,
>>>> +				 const struct spi_nor_address_entry *entries,
>>>> +				 size_t num_entries)
>>>> +{
>>>> +	int min, max;
>>>> +
>>>> +	/*
>>>> +	 * This function implements a dichotomic search in the entries[]
>>>> +	 * array indexed by src_opcode. Hence we assume that the entries[]
>>>> +	 * array is sorted by src_opcode.
>>>> +	 * The dichotomic search has a logarithmic complexity as opposed
>>>> +	 * to a simple loop on all entires, which has a linear complexity:
>>>> +	 * it means that when n is the number of entries in the input array,
>>>> +	 * the dichotomic search performs O(log2(n)) comparisons whereas
>>>> +	 * a simple loop performs O(n) comparisons.
>>>> +	 */
>>>> +	min = 0;
>>>> +	max = num_entries - 1;
>>>> +	while (min <= max) {
>>>> +		int mid = (min + max) >> 1;
>>>> +		const struct spi_nor_address_entry *entry = &entries[mid];
>>>> +
>>>> +		if (opcode == entry->src_opcode)
>>>> +			return entry->dst_opcode;
>>>> +
>>>> +		if (opcode < entry->src_opcode)
>>>> +			max = mid - 1;
>>>> +		else
>>>> +			min = mid + 1;
>>>> +	}
>>>
>>> You have like 16 entries in that table, just do a linear search, this is
>>> only complex for no benefit.
>>
>> Well ok, I agree with you, it's too much overkill for what it does.
>> For readiness purpose, what about a simple and straight forward switch()
>> statement? Let's forget about cache-line and other memory/time optimizations :)
> 
> If you do a switch, you cannot walk a table which you pass in, see below.
> 

I meant I remove the table too, a hidden "table" would still be generated
by the compiler in the assembly code to translate the switch statement:

static u8 spi_nor_3to4_opcode(u8 3byte_address_opcode)
{
	#define CASE_3TO4(_opcode)	case _opcode: return _opcode##_4B
	
	switch (3_byte_address_opcode) {
	/* Without macro. */
	case SPINOR_OP_PP:
		return SPINOR_OP_PP_4B;

	case SPINOR_OP_READ:
		return SPINOR_OP_READ_4B;

	case SPINOR_OP_READ_FAST:
		return SPINOR_OP_READ_FAST_4B;

	/* With macro. */
	CASE_3TO4(SPINOR_OP_READ_FAST);

	default:
		break;
	}

	/* No conversion found */
	return opcode;
}

For readiness, I don't whether people prefer the version with macro or the
version without. Just tell me :)

Of course with the switch statement I no longer need the struct
spi_nor_address_entry.

Also for now I don't need a 4TO3 translation.


>>>> +	/* No conversion found */
>>>> +	return opcode;
>>>> +}
>>>> +
>>>> +static u8 spi_nor_3to4_opcode(u8 opcode)
>>>> +{
>>>> +	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
>>>> +#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
>>>> +	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {
>>>
>>> You can make this static const struct const for extra constness :-)
>>>
>>>> +		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
>>>> +		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
>>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
>>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
>>>> +		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
>>>> +		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */
>>>
>>> I'd probably break this into three smaller tables, read/program/erase
>>> and then call something like:
>>>
>>> spi_nor_3to4_opcode(nor->read_opcode, read_opcode_table,
>>>                     ARRAY_SIZE(read_opcode_table));
>>>
>>> This would further reduce the table size (heck, it'd probably fit into a
>>> cacheline), so linear search would be more than enough.
>>>
>>>> +	};
>>>> +#undef ENTRY_3TO4
>>>> +
>>>> +	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
>>>> +				      ARRAY_SIZE(spi_nor_3to4_table));
>>>> +}
>>>> +
>>>> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>>>> +				      const struct flash_info *info)
>>>> +{
>>>> +	/* Do some manufacturer fixups first */
>>>> +	switch (JEDEC_MFR(info)) {
>>>> +	case SNOR_MFR_SPANSION:
>>>> +		/* No small sector erase for 4-byte command set */
>>>> +		nor->erase_opcode = SPINOR_OP_SE;
>>>> +		nor->mtd.erasesize = info->sector_size;
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
>>>> +	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
>>>> +	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
>>>> +}
>>>> +
>>>>  /* Enable/disable 4-byte addressing mode. */
>>>>  static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>>>  			    int enable)
>>>> @@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>  	else if (mtd->size > 0x1000000) {
>>>>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>>>>  		nor->addr_width = 4;
>>>> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
>>>> -			/* Dedicated 4-byte command set */
>>>> -			switch (nor->flash_read) {
>>>> -			case SPI_NOR_QUAD:
>>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
>>>> -				break;
>>>> -			case SPI_NOR_DUAL:
>>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
>>>> -				break;
>>>> -			case SPI_NOR_FAST:
>>>> -				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
>>>> -				break;
>>>> -			case SPI_NOR_NORMAL:
>>>> -				nor->read_opcode = SPINOR_OP_READ_4B;
>>>> -				break;
>>>> -			}
>>>> -			nor->program_opcode = SPINOR_OP_PP_4B;
>>>> -			/* No small sector erase for 4-byte command set */
>>>> -			nor->erase_opcode = SPINOR_OP_SE_4B;
>>>> -			mtd->erasesize = info->sector_size;
>>>> -		} else
>>>> +		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>>>> +		    info->flags & SPI_NOR_4B_OPCODES)
>>>> +			spi_nor_set_4byte_opcodes(nor, info);
>>>> +		else
>>>>  			set_4byte(nor, info, 1);
>>>>  	} else {
>>>>  		nor->addr_width = 3;
>>>>
>>>
>>>
>>
> 
>