Re: [PATCH v4] mtd: spi-nor: add support for GD25Q256

[Cyrille Pitchen]

Hi Andy,

Le 16/08/2017 Ã 05:40, Andy Yan a Ãcrit :
> Hi Cyrille:
> 
> 
> On 2017å08æ16æ 00:04, Cyrille Pitchen wrote:
>> Hi Andy,
>>
>> Le 25/07/2017 Ã 12:12, Andy Yan a Ãcrit :
>>> Add support for GD25Q256, a 32MiB SPI Nor
>>> flash from Gigadevice.
>>>
>>> Signed-off-by: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
>>>
>>> ---
>>>
>>> Changes in v4:
>>> - add SPI_NOR_HAS_LOCK and SPI_NOR_HAS_TB
>> Between v3 and v4, I see that you've also changed the procedure to the
>> the Quad Enable bit on all Gigadevice memories with QSPI capabilities.
>> This is not a detail and should have been reported here.
> 
>  Sorry, I will keep this in mind.
>>
>>> Changes in v3:
>>> - rebase on top of spi-nor tree
>>> - add SPI_NOR_4B_OPCODES flag
>>>
>>> Changes in v2:
>>> - drop one line unnecessary modification
>>>
>>>   drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>> b/drivers/mtd/spi-nor/spi-nor.c
>>> index 196b52f..e4145cd 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -986,6 +986,11 @@ static const struct flash_info spi_nor_ids[] = {
>>>               SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>>               SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>>       },
>>> +    {
>>> +        "gd25q256", INFO(0xc84019, 0, 64 * 1024, 512,
>>> +            SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>> +            SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>> +    },
>>>         /* Intel/Numonyx -- xxxs33b */
>>>       { "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
>>> @@ -2365,6 +2370,7 @@ static int spi_nor_init_params(struct spi_nor
>>> *nor,
>>>                      SNOR_HWCAPS_PP_QUAD)) {
>>>           switch (JEDEC_MFR(info)) {
>>>           case SNOR_MFR_MACRONIX:
>>> +        case SNOR_MFR_GIGADEVICE:
>>>               params->quad_enable = macronix_quad_enable;
>> Here, you've have changed the Quad Enable requirement for *all*
>> Gigadevice memories with Quad SPI capabilities.
>>
>> However, I'm reading the GD25Q128 datasheet and it claims that the QE
>> bit is BIT(1) of the Status Register 2. Hence some
>> spansion*_quad_enable() should be used, as before your patch.
>>
>> Then, still according to the datasheet, the GD25Q128 memory is compliant
>> with the JESD216 specification (minor 0) but neither with rev A (minor
>> 5) nor rev B (minor 6).
>> So its Basic Flash Parameter Table is limited to 9 DWORDs instead of 16
>> DWORDs, hence doesn't provide the Quad Enable requirements. It means
>> that the SFDP tables would not help to select the right _quad_enable()
>> function by overriding the choice made by the switch() statement above.
>>
>> tl;dr
>> This chunk would introduce a regression with some already supported
>> Gigadevice memories. So I reject this patch, sorry.
> 
>     After check some other Gigadevice memories, I found it's true as you
> mentioned.
> Some memories use S9 as the QE bit, but some use S6.
>     Do you have some ideas for this case? Add a check for the full
> jedec_id or encode
> the QE bit in the flash_info?
>     I am a new bee in the flash failed, very appreciate for your advice.

Historically spi-nor.c assumed that the procedure to set the QE bit
could be chosen based on the manufacturer ID only, hence without needing
to check the whole JEDEC ID. Obviouly this is no longer true for
Gigadevice SPI NOR memories...

So you need a solution which is backward compatible with the existing
code so you patch would not introduce any regression for other SPI NOR
memories.

Then, I suggest you add a .quad_enable member in 'struct flash_info'.

 #define USE_CLSR		BIT(14)	/* use CLSR command */
+
+	int		(*quad_enable)(struct spi_nor *nor);
 };

 #define JEDEC_MFR(info)	((info)->id[0]



Also in spi_nor_init_params():

 	/* Select the procedure to set the Quad Enable bit. */
 	if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
 				   SNOR_HWCAPS_PP_QUAD)) {
 		switch (JEDEC_MFR(info)) {
 		case SNOR_MFR_MACRONIX:
 			params->quad_enable = macronix_quad_enable;
 			break;

 		case SNOR_MFR_MICRON:
 			break;

 		default:
 			/* Kept only for backward compatibility purpose. */
 			params->quad_enable = spansion_quad_enable;
 			break;
 		}
 	}
+
+	if (info->quad_enable)
+		params->quad_enable = info->quad_enable;

 	/* Override the parameters with data read from SFDP tables. */
 	nor->addr_width = 0;
 	nor->mtd.erasesize = 0;
 	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
 	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
 		struct spi_nor_flash_parameter sfdp_params;

 		memcpy(&sfdp_params, params, sizeof(sfdp_params));
 		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
 			nor->addr_width = 0;
 			nor->mtd.erasesize = 0;
 		} else {
 			memcpy(params, &sfdp_params, sizeof(*params));
 		}
 	}



And finally when adding the gigadevice entries in the spi_nor_ids[]
array, don't forget to initialize the new member:

+	{
+		"gd25q256", .quad_enable = macronix_quad_enable, 	+		INFO([...])
+	},

>>
>> Best regards,
>>
>> Cyrille
>>
>>>               break;
>>>  
>>
>>
>>
> 
> 
>