Message-ID: <d52f4b7c-5170-20e2-d176-fccb746845d7@rock-chips.com>
Date:   Wed, 23 Aug 2017 16:46:42 +0800
From:   Andy Yan <andy.yan@...k-chips.com>
To:     Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>
Cc:     linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
        boris.brezillon@...e-electrons.com, marek.vasut@...il.com,
        computersforpeace@...il.com, richard@....at, dwmw2@...radead.org
Subject: Re: [PATCH v4] mtd: spi-nor: add support for GD25Q256

Hi Cyrille:


On 2017年08月23日 15:46, Cyrille Pitchen wrote:
> 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@...k-chips.com>
>>>>
>>>> ---
>>>>
>>>> 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([...])
> +	},

     Thanks very much for your guidance. I will try a new patch very soon.
>>> Best regards,
>>>
>>> Cyrille
>>>
>>>>                break;
>>>>   
>>>
>>>
>>
>>
>
>
>

Powered by blists - more mailing lists