[v2] mtd: spi-nor: Fixup page size for S25FS-S
diff mbox series

Message ID 20200205165736.4964-1-alexander.sverdlin@nokia.com
State New
Headers show
Series
  • [v2] mtd: spi-nor: Fixup page size for S25FS-S
Related show

Commit Message

Alexander X Sverdlin Feb. 5, 2020, 4:57 p.m. UTC
From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this
is configurable in the non-volatile CR3NV register and even factory
default configuration is "wrap at 256 bytes". So blind relying on BFPT
breaks write operation on these Flashes.

All this story is vendor-specific, so add the corresponding fixup hook
which first restores the safe page size of 256 bytes from
struct flash_info but checks is more performant 512 bytes configuration
is active and adjusts the page_size accordingly.

Cc: stable@vger.kernel.org
Fixes: f384b352c ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
Changelog:
v2: Thankfully rebased on the hint from John Garry

 drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++++++++++++--
 include/linux/mtd/spi-nor.h   |  5 +++++
 2 files changed, 42 insertions(+), 2 deletions(-)

Comments

John Garry Feb. 6, 2020, 11:38 a.m. UTC | #1
On 05/02/2020 16:57, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
> DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this
> is configurable in the non-volatile CR3NV register and even factory
> default configuration is "wrap at 256 bytes". So blind relying on BFPT
> breaks write operation on these Flashes.
> 
> All this story is vendor-specific, so add the corresponding fixup hook
> which first restores the safe page size of 256 bytes from
> struct flash_info but checks is more performant 512 bytes configuration
> is active and adjusts the page_size accordingly.
> 
> Cc: stable@vger.kernel.org
> Fixes: f384b352c ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
> Changelog:
> v2: Thankfully rebased on the hint from John Garry
> 
>   drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++++++++++++++--
>   include/linux/mtd/spi-nor.h   |  5 +++++
>   2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 928a660..c0a5041 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2302,6 +2302,39 @@ static struct spi_nor_fixups gd25q256_fixups = {
>   	.default_init = gd25q256_default_init,
>   };
>   

Hi Alexander,

Thanks for the quick turnaround. But, sorry to say, this does not look 
right:

> +/* Spansion S25FS-S SFDP workarounds */
> +static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
> +	const struct sfdp_parameter_header *bfpt_header,
> +	const struct sfdp_bfpt *bfpt,
> +	struct spi_nor_flash_parameter *params)
> +{
> +	const struct flash_info *info = nor->info;
> +	u8 read_opcode, buf;
> +	int ret;
> +
> +	/* Default is safe */
> +	params->page_size = info->page_size;
> +
> +	/*
> +	 * But is the chip configured for more performant 512 bytes write page
> +	 * size?
> +	 */
> +	read_opcode = nor->read_opcode;
> +
> +	nor->read_opcode = SPINOR_OP_RDAR;
> +	ret = nor->read(nor, SPINOR_REG_CR3V, 1, &buf);

The read method is now gone from struct spi_nor, moved into 
spi_nor.controller_ops. And we also support spi_mem ops now.

In fact, I find that the SFDP signature is not correct for me for this 
part, so I need to check that first...

Thanks,
John

> +	if (!ret && (buf & CR3V_02H_V))
> +		params->page_size = 512;
> +
> +	nor->read_opcode = read_opcode;
> +
> +	return ret;
> +}
> +
> +static const struct spi_nor_fixups s25fs_s_fixups = {
> +	.post_bfpt = s25fs_s_post_bfpt_fixups,
> +};
> +
>   /* NOTE: double check command sets and memory organization when you add
>    * more nor chips.  This current list focusses on newer chips, which
>    * have been converging on command sets which including JEDEC ID.
> @@ -2536,7 +2569,8 @@ static const struct flash_info spi_nor_ids[] = {
>   			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>   	{ "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>   			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
> +	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
> +			.fixups = &s25fs_s_fixups, },
>   	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>   	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
>   			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> @@ -2546,7 +2580,8 @@ static const struct flash_info spi_nor_ids[] = {
>   	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>   	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>   	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> +	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +			.fixups = &s25fs_s_fixups, },
>   	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>   	{ "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
>   	{ "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 5abd91c..7ce3e79 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -116,6 +116,7 @@
>   /* Used for Spansion flashes only. */
>   #define SPINOR_OP_BRWR		0x17	/* Bank register write */
>   #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
> +#define SPINOR_OP_RDAR		0x65	/* Read Any Register */
>   
>   /* Used for Micron flashes only. */
>   #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
> @@ -150,6 +151,10 @@
>   #define SR2_QUAD_EN_BIT1	BIT(1)
>   #define SR2_QUAD_EN_BIT7	BIT(7)
>   
> +/* Used for Spansion flashes RDAR command only. */
> +#define SPINOR_REG_CR3V		0x800004
> +#define CR3V_02H_V		BIT(4)	/* Page Buffer Wrap */
> +
>   /* Supported SPI protocols */
>   #define SNOR_PROTO_INST_MASK	GENMASK(23, 16)
>   #define SNOR_PROTO_INST_SHIFT	16
>
Vignesh Raghavendra Feb. 7, 2020, 5:03 a.m. UTC | #2
Hi Alexander,

On 06/02/20 5:08 pm, John Garry wrote:
> On 05/02/2020 16:57, Alexander A Sverdlin wrote:
>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>
[...]
>> +static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
>> +    const struct sfdp_parameter_header *bfpt_header,
>> +    const struct sfdp_bfpt *bfpt,
>> +    struct spi_nor_flash_parameter *params)
>> +{
>> +    const struct flash_info *info = nor->info;
>> +    u8 read_opcode, buf;
>> +    int ret;
>> +
>> +    /* Default is safe */
>> +    params->page_size = info->page_size;
>> +
>> +    /*
>> +     * But is the chip configured for more performant 512 bytes write
>> page
>> +     * size?
>> +     */
>> +    read_opcode = nor->read_opcode;
>> +
>> +    nor->read_opcode = SPINOR_OP_RDAR;
>> +    ret = nor->read(nor, SPINOR_REG_CR3V, 1, &buf);
> 
> The read method is now gone from struct spi_nor, moved into
> spi_nor.controller_ops. And we also support spi_mem ops now.
> 

Yes, please rebase patch on top of latest spi-nor/next or linux-next tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next

Regards
Vignesh


> 
>> +    if (!ret && (buf & CR3V_02H_V))
>> +        params->page_size = 512;
>> +
>> +    nor->read_opcode = read_opcode;
>> +
>> +    return ret;
>> +}
>> +
>> +static const struct spi_nor_fixups s25fs_s_fixups = {
>> +    .post_bfpt = s25fs_s_post_bfpt_fixups,
>> +};
>> +
>>   /* NOTE: double check command sets and memory organization when you add
>>    * more nor chips.  This current list focusses on newer chips, which
>>    * have been converging on command sets which including JEDEC ID.
>> @@ -2536,7 +2569,8 @@ static const struct flash_info spi_nor_ids[] = {
>>               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>       { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>>               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> -    { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
>> +    { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
>> +            .fixups = &s25fs_s_fixups, },
>>       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>       { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
>>               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> @@ -2546,7 +2580,8 @@ static const struct flash_info spi_nor_ids[] = {
>>       { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>       { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>>       { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256,
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +            .fixups = &s25fs_s_fixups, },
>>       { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>>       { "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
>>       { "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 5abd91c..7ce3e79 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -116,6 +116,7 @@
>>   /* Used for Spansion flashes only. */
>>   #define SPINOR_OP_BRWR        0x17    /* Bank register write */
>>   #define SPINOR_OP_CLSR        0x30    /* Clear status register 1 */
>> +#define SPINOR_OP_RDAR        0x65    /* Read Any Register */
>>     /* Used for Micron flashes only. */
>>   #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>> @@ -150,6 +151,10 @@
>>   #define SR2_QUAD_EN_BIT1    BIT(1)
>>   #define SR2_QUAD_EN_BIT7    BIT(7)
>>   +/* Used for Spansion flashes RDAR command only. */
>> +#define SPINOR_REG_CR3V        0x800004
>> +#define CR3V_02H_V        BIT(4)    /* Page Buffer Wrap */
>> +
>>   /* Supported SPI protocols */
>>   #define SNOR_PROTO_INST_MASK    GENMASK(23, 16)
>>   #define SNOR_PROTO_INST_SHIFT    16
>>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
John Garry Feb. 12, 2020, 6 p.m. UTC | #3
On 07/02/2020 05:03, Vignesh Raghavendra wrote:
> Hi Alexander,
> 
> On 06/02/20 5:08 pm, John Garry wrote:
>> On 05/02/2020 16:57, Alexander A Sverdlin wrote:
>>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>>
> [...]
>>> +static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
>>> +    const struct sfdp_parameter_header *bfpt_header,
>>> +    const struct sfdp_bfpt *bfpt,
>>> +    struct spi_nor_flash_parameter *params)
>>> +{
>>> +    const struct flash_info *info = nor->info;
>>> +    u8 read_opcode, buf;
>>> +    int ret;
>>> +
>>> +    /* Default is safe */
>>> +    params->page_size = info->page_size;
>>> +
>>> +    /*
>>> +     * But is the chip configured for more performant 512 bytes write
>>> page
>>> +     * size?
>>> +     */
>>> +    read_opcode = nor->read_opcode;
>>> +
>>> +    nor->read_opcode = SPINOR_OP_RDAR;
>>> +    ret = nor->read(nor, SPINOR_REG_CR3V, 1, &buf);
>>
>> The read method is now gone from struct spi_nor, moved into
>> spi_nor.controller_ops. And we also support spi_mem ops now.
>>
> 
> Yes, please rebase patch on top of latest spi-nor/next or linux-next tree at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
> 
> Regards
> Vignesh
> 

I don't mean to hijack this thread, but I have tried to enable QUAD mode 
for part s25fl129p1, and it fails in spi_nor_write_16bit_cr_and_check():

[ 47.263365] spi-nor spi-PRP0001:00: CR: read back test failed
[ 47.306567] spi-nor spi-PRP0001:00: quad mode not supported
[ 47.322413] spi-nor: probe of spi-PRP0001:00 failed with error -5

Hacking the flags to set SNOR_F_NO_READ_CR, and at least I can 
successfully probe the driver.

Does anyone know if this part does not support reading the config 
register. The limited datasheet here doesn't mention it, AFAICT:

https://www.cypress.com/file/196851/download

Thanks,
John

> 
>>
>>> +    if (!ret && (buf & CR3V_02H_V))
>>> +        params->page_size = 512;
>>> +
>>> +    nor->read_opcode = read_opcode;
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct spi_nor_fixups s25fs_s_fixups = {
>>> +    .post_bfpt = s25fs_s_post_bfpt_fixups,
>>> +};
>>> +
>>>    /* NOTE: double check command sets and memory organization when you add
>>>     * more nor chips.  This current list focusses on newer chips, which
>>>     * have been converging on command sets which including JEDEC ID.
>>> @@ -2536,7 +2569,8 @@ static const struct flash_info spi_nor_ids[] = {
>>>                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>        { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>>>                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>> -    { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
>>> +    { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
>>> +            .fixups = &s25fs_s_fixups, },
>>>        { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>        { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
>>>                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>> @@ -2546,7 +2580,8 @@ static const struct flash_info spi_nor_ids[] = {
>>>        { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>>        { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>>>        { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64,
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256,
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256,
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>>> +            .fixups = &s25fs_s_fixups, },
>>>        { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>>>        { "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
>>>        { "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
>>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>>> index 5abd91c..7ce3e79 100644
>>> --- a/include/linux/mtd/spi-nor.h
>>> +++ b/include/linux/mtd/spi-nor.h
>>> @@ -116,6 +116,7 @@
>>>    /* Used for Spansion flashes only. */
>>>    #define SPINOR_OP_BRWR        0x17    /* Bank register write */
>>>    #define SPINOR_OP_CLSR        0x30    /* Clear status register 1 */
>>> +#define SPINOR_OP_RDAR        0x65    /* Read Any Register */
>>>      /* Used for Micron flashes only. */
>>>    #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>>> @@ -150,6 +151,10 @@
>>>    #define SR2_QUAD_EN_BIT1    BIT(1)
>>>    #define SR2_QUAD_EN_BIT7    BIT(7)
>>>    +/* Used for Spansion flashes RDAR command only. */
>>> +#define SPINOR_REG_CR3V        0x800004
>>> +#define CR3V_02H_V        BIT(4)    /* Page Buffer Wrap */
>>> +
>>>    /* Supported SPI protocols */
>>>    #define SNOR_PROTO_INST_MASK    GENMASK(23, 16)
>>>    #define SNOR_PROTO_INST_SHIFT    16
>>>
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Vignesh Raghavendra Feb. 18, 2020, 4:53 a.m. UTC | #4
On 12/02/20 11:30 pm, John Garry wrote:
> On 07/02/2020 05:03, Vignesh Raghavendra wrote:
>> Hi Alexander,
>>
>> On 06/02/20 5:08 pm, John Garry wrote:
>>> On 05/02/2020 16:57, Alexander A Sverdlin wrote:
>>>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>>>
>> [...]
[...]
> 
> I don't mean to hijack this thread, but I have tried to enable QUAD mode
> for part s25fl129p1, and it fails in spi_nor_write_16bit_cr_and_check():
> 
> [ 47.263365] spi-nor spi-PRP0001:00: CR: read back test failed
> [ 47.306567] spi-nor spi-PRP0001:00: quad mode not supported
> [ 47.322413] spi-nor: probe of spi-PRP0001:00 failed with error -5
> 
> Hacking the flags to set SNOR_F_NO_READ_CR, and at least I can
> successfully probe the driver.
> 
> Does anyone know if this part does not support reading the config
> register. The limited datasheet here doesn't mention it, AFAICT:
> 
> https://www.cypress.com/file/196851/download

Above datasheet is for s25fl128p.

Per, s25fl129p datasheet[1], part does support 0x35 (SPINOR_OP_RDCR)
command and support 16bit write status register command (0x1)

Could you debug further and see what exactly fails to match when
read back fails?

[1]https://www.cypress.com/file/197121/download


Regards
Vignesh

> 
> Thanks,
> John
> 
>>
>>>
>>>> +    if (!ret && (buf & CR3V_02H_V))
>>>> +        params->page_size = 512;
>>>> +
>>>> +    nor->read_opcode = read_opcode;
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct spi_nor_fixups s25fs_s_fixups = {
>>>> +    .post_bfpt = s25fs_s_post_bfpt_fixups,
>>>> +};
>>>> +
>>>>    /* NOTE: double check command sets and memory organization when
>>>> you add
>>>>     * more nor chips.  This current list focusses on newer chips, which
>>>>     * have been converging on command sets which including JEDEC ID.
>>>> @@ -2536,7 +2569,8 @@ static const struct flash_info spi_nor_ids[] = {
>>>>                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>>        { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>>>>                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>> -    { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,
>>>> USE_CLSR) },
>>>> +    { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
>>>> +            .fixups = &s25fs_s_fixups, },
>>>>        { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>>        { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
>>>>                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>>> @@ -2546,7 +2580,8 @@ static const struct flash_info spi_nor_ids[] = {
>>>>        { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>>>        { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>>>>        { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64,
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256,
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256,
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>>>> +            .fixups = &s25fs_s_fixups, },
>>>>        { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>>>>        { "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
>>>>        { "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
>>>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>>>> index 5abd91c..7ce3e79 100644
>>>> --- a/include/linux/mtd/spi-nor.h
>>>> +++ b/include/linux/mtd/spi-nor.h
>>>> @@ -116,6 +116,7 @@
>>>>    /* Used for Spansion flashes only. */
>>>>    #define SPINOR_OP_BRWR        0x17    /* Bank register write */
>>>>    #define SPINOR_OP_CLSR        0x30    /* Clear status register 1 */
>>>> +#define SPINOR_OP_RDAR        0x65    /* Read Any Register */
>>>>      /* Used for Micron flashes only. */
>>>>    #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>>>> @@ -150,6 +151,10 @@
>>>>    #define SR2_QUAD_EN_BIT1    BIT(1)
>>>>    #define SR2_QUAD_EN_BIT7    BIT(7)
>>>>    +/* Used for Spansion flashes RDAR command only. */
>>>> +#define SPINOR_REG_CR3V        0x800004
>>>> +#define CR3V_02H_V        BIT(4)    /* Page Buffer Wrap */
>>>> +
>>>>    /* Supported SPI protocols */
>>>>    #define SNOR_PROTO_INST_MASK    GENMASK(23, 16)
>>>>    #define SNOR_PROTO_INST_SHIFT    16
>>>>
>>>
>>>
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>
John Garry Feb. 18, 2020, 6:01 p.m. UTC | #5
On 18/02/2020 04:53, Vignesh Raghavendra wrote:
>> I don't mean to hijack this thread, but I have tried to enable QUAD mode
>> for part s25fl129p1, and it fails in spi_nor_write_16bit_cr_and_check():
>>
>> [ 47.263365] spi-nor spi-PRP0001:00: CR: read back test failed
>> [ 47.306567] spi-nor spi-PRP0001:00: quad mode not supported
>> [ 47.322413] spi-nor: probe of spi-PRP0001:00 failed with error -5
>>
>> Hacking the flags to set SNOR_F_NO_READ_CR, and at least I can
>> successfully probe the driver.
>>
>> Does anyone know if this part does not support reading the config
>> register. The limited datasheet here doesn't mention it, AFAICT:
>>

Hi Vignesh,

>> https://www.cypress.com/file/196851/download
> Above datasheet is for s25fl128p.
> 

Right, I figured this out soon enough. I shouldn't just click on the 
first page which google produces...

> Per, s25fl129p datasheet[1], part does support 0x35 (SPINOR_OP_RDCR)
> command and support 16bit write status register command (0x1)
> 
> Could you debug further and see what exactly fails to match when
> read back fails?

I was trying to figure out the issue. So in 
spi_nor_write_16bit_cr_and_check(), the sr check passes (it holds 0, so 
that may be somewhat inconclusive) but the value for comparison return 0 
in the CR also versus expected 3.

Maybe it is a host driver issue, but I am doubtful.

I can continue to investigate. Any ideas would be appreciated.

> 
> [1]https://www.cypress.com/file/197121/download
> 
> 

Thanks,
John

Patch
diff mbox series

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 928a660..c0a5041 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2302,6 +2302,39 @@  static struct spi_nor_fixups gd25q256_fixups = {
 	.default_init = gd25q256_default_init,
 };
 
+/* Spansion S25FS-S SFDP workarounds */
+static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
+	const struct sfdp_parameter_header *bfpt_header,
+	const struct sfdp_bfpt *bfpt,
+	struct spi_nor_flash_parameter *params)
+{
+	const struct flash_info *info = nor->info;
+	u8 read_opcode, buf;
+	int ret;
+
+	/* Default is safe */
+	params->page_size = info->page_size;
+
+	/*
+	 * But is the chip configured for more performant 512 bytes write page
+	 * size?
+	 */
+	read_opcode = nor->read_opcode;
+
+	nor->read_opcode = SPINOR_OP_RDAR;
+	ret = nor->read(nor, SPINOR_REG_CR3V, 1, &buf);
+	if (!ret && (buf & CR3V_02H_V))
+		params->page_size = 512;
+
+	nor->read_opcode = read_opcode;
+
+	return ret;
+}
+
+static const struct spi_nor_fixups s25fs_s_fixups = {
+	.post_bfpt = s25fs_s_post_bfpt_fixups,
+};
+
 /* NOTE: double check command sets and memory organization when you add
  * more nor chips.  This current list focusses on newer chips, which
  * have been converging on command sets which including JEDEC ID.
@@ -2536,7 +2569,8 @@  static const struct flash_info spi_nor_ids[] = {
 			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
 	{ "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
 			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
+	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
+			.fixups = &s25fs_s_fixups, },
 	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
 	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
 			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
@@ -2546,7 +2580,8 @@  static const struct flash_info spi_nor_ids[] = {
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
 	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
+	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+			.fixups = &s25fs_s_fixups, },
 	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
 	{ "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
 	{ "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5abd91c..7ce3e79 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -116,6 +116,7 @@ 
 /* Used for Spansion flashes only. */
 #define SPINOR_OP_BRWR		0x17	/* Bank register write */
 #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
+#define SPINOR_OP_RDAR		0x65	/* Read Any Register */
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
@@ -150,6 +151,10 @@ 
 #define SR2_QUAD_EN_BIT1	BIT(1)
 #define SR2_QUAD_EN_BIT7	BIT(7)
 
+/* Used for Spansion flashes RDAR command only. */
+#define SPINOR_REG_CR3V		0x800004
+#define CR3V_02H_V		BIT(4)	/* Page Buffer Wrap */
+
 /* Supported SPI protocols */
 #define SNOR_PROTO_INST_MASK	GENMASK(23, 16)
 #define SNOR_PROTO_INST_SHIFT	16