[PATCH v3 4/8] mtd: spi-nor: sfdp: Add support for SCCR map for multi-chip device

[Takahiro Kuwano]

Hi Tudor,

On 4/6/2023 7:44 PM, Tudor Ambarus wrote:
> 
> 
> On 4/6/23 07:18, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>
>> SCCR map for multi-chip devices contains the number of additional dice in
>> the device and register offset values for each additional dice.
>>
>> spi_nor_parse_sccr_mc() is added to determine the number of dice and
>> volatile register offset for each die. The volatile register offset table
>> may already be allocated and contains offset value for die-0 via SCCR map
>> parse. So, we should use devm_krealloc() to expand the table with
>> preserving die-0 offset.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>>  drivers/mtd/spi-nor/sfdp.c | 73 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 9d43e18d3770..00a40bd8aa1e 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -26,6 +26,11 @@
>>  					 * Status, Control and Configuration
>>  					 * Register Map.
>>  					 */
>> +#define SFDP_SCCR_MAP_MC_ID	0xff88	/*
>> +					 * Status, Control and Configuration
>> +					 * Register Map Offsets for Multi-Chip
>> +					 * SPI Memory Devices.
>> +					 */
>>  
>>  #define SFDP_SIGNATURE		0x50444653U
>>  
>> @@ -1264,6 +1269,70 @@ static int spi_nor_parse_sccr(struct spi_nor *nor,
>>  	return ret;
>>  }
>>  
>> +#define SCCR_MC_MAX_DICE		255
>> +
>> +/**
>> + * spi_nor_parse_sccr_mc() - Parse the Status, Control and Configuration
>> + *                           Register Map Offsets for Multi-Chip SPI Memory
>> + *                           Devices.
>> + * @nor:		pointer to a 'struct spi_nor'
>> + * @sccr_mc_header:	pointer to the 'struct sfdp_parameter_header' describing
>> + *			the SCCR Map offsets table length and version.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spi_nor_parse_sccr_mc(struct spi_nor *nor,
>> +				 const struct sfdp_parameter_header *sccr_mc_header)
>> +{
>> +	struct spi_nor_flash_parameter *params = nor->params;
>> +	u32 *dwords, addr;
>> +	u8 i, n_dice;
>> +	size_t len;
>> +	int ret;
>> +
>> +	len = sccr_mc_header->length * sizeof(*dwords);
>> +	dwords = kmalloc(len, GFP_KERNEL);
>> +	if (!dwords)
>> +		return -ENOMEM;
>> +
>> +	addr = SFDP_PARAM_HEADER_PTP(sccr_mc_header);
>> +	ret = spi_nor_read_sfdp(nor, addr, len, dwords);
>> +	if (ret)
>> +		goto out;
>> +
>> +	le32_to_cpu_array(dwords, sccr_mc_header->length);
>> +
>> +	/*
>> +	 * Pair of DOWRDs (volatile and non-volatile register offsets) per
>> +	 * additional die. Hence, length = 2 * (number of additional dice).
>> +	 */
>> +	n_dice = 1 + sccr_mc_header->length / 2;
>> +	if (params->n_dice > SCCR_MC_MAX_DICE) {
s/params->n_dice/n_dice

> 
> smatch warnings:
> drivers/mtd/spi-nor/sfdp.c:1310 spi_nor_parse_sccr_mc() warn: impossible
> condition '(params->n_dice > 255) => (0-255 > 255)'
> 
> declaring the local variable n_dice as unsigned int should solve the
> problem.
> 
'sccr_mc_header->length' is u8 so n_dice can be 1-128. We can keep
n_dice as u8.

(n_dice > SCCR_MC_MAX_DICE) is impossible condition anyway...
We don't need n_dice check. Otherwise define SCCR_MC_MAX_DICE as smaller
value something like 16. What do you think?

>> +		dev_dbg(nor->dev, "Improbable number of dice defined in SCCR MC table, use one die.\n");
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	/* Address offset for volatile registers of additional dice */
>> +	params->vreg_offset =
>> +			devm_krealloc(nor->dev, params->vreg_offset,
>> +				      n_dice * sizeof(*dwords),
>> +				      GFP_KERNEL);
>> +	if (!params->vreg_offset) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	for (i = 1; i < n_dice; i++)
>> +		params->vreg_offset[i] = dwords[SFDP_DWORD(i) * 2];
>> +
>> +	params->n_dice = n_dice;
> 
> we then truncate it to u8, which is fine.