diff mbox series

mtd: spi-nor: Provide default address width and latency for map selection

Message ID 20181129181519.15681-1-alexander.sverdlin@nokia.com
State Changes Requested
Headers show
Series mtd: spi-nor: Provide default address width and latency for map selection | expand

Commit Message

Alexander A Sverdlin Nov. 29, 2018, 6:15 p.m. UTC 
JESD216 allows "variable address length" and "variable latency" in
Configuration Detection Command Descriptors, in other words "as-is".
And they are still unset during Sector Map Parameter Table parsing,
which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.

Fixes: b038e8e3b ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Boris Brezillon Nov. 30, 2018, 10:40 a.m. UTC | #1 
On Thu, 29 Nov 2018 18:15:34 +0000
"Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
wrote:

> JESD216 allows "variable address length" and "variable latency" in
> Configuration Detection Command Descriptors, in other words "as-is".
> And they are still unset during Sector Map Parameter Table parsing,
> which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
> 
> Fixes: b038e8e3b ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 828d03e..5557c89 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2893,6 +2893,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
>  		nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>  		addr = smpt[i + 1];
>  
> +		/*
> +		 * JESD216 allows to omit particular address length or latency
> +		 * specification in the header and at this point they are still
> +		 * unset, so we need some heuristics. One example is S25FS128S.
> +		 */
> +		if (!nor->addr_width)
> +			nor->addr_width = 3;
> +		if (!nor->read_dummy)
> +			nor->read_dummy = 8;
> +

Looks like the same problem was reported by Yogesh here [1]. One more
proof that parsing SFDP is not trivial. I mean, what's the point of
defining a generic tables to describe NOR capabilities if you then
depend on vendor/chip specific init to read those tables...

Anyway, I think this sort of initialization should be placed in a
pre_sfdp() fixup hook, as getting the right values likely requires
reading some volatile/non-volatile regs.

>  		err = spi_nor_read_raw(nor, addr, 1, &data_byte);
>  		if (err)
>  			goto out;

[1]https://lkml.org/lkml/2018/10/22/354
Alexander A Sverdlin Dec. 3, 2018, 8:03 a.m. UTC | #2 
Hello Boris!

On 30/11/2018 11:40, Boris Brezillon wrote:
>> +		/*
>> +		 * JESD216 allows to omit particular address length or latency
>> +		 * specification in the header and at this point they are still
>> +		 * unset, so we need some heuristics. One example is S25FS128S.
>> +		 */
>> +		if (!nor->addr_width)
>> +			nor->addr_width = 3;
>> +		if (!nor->read_dummy)
>> +			nor->read_dummy = 8;
>> +
> Looks like the same problem was reported by Yogesh here [1]. One more

This is indeed the same problem, although I was not aware of the [1] discussion.

> proof that parsing SFDP is not trivial. I mean, what's the point of
> defining a generic tables to describe NOR capabilities if you then
> depend on vendor/chip specific init to read those tables...
> 
> Anyway, I think this sort of initialization should be placed in a
> pre_sfdp() fixup hook, as getting the right values likely requires

This is still on my TODO list, to learn about new hooks.

> reading some volatile/non-volatile regs.

This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.

Therefore, I don't know if it's possible to provide smarter heuristics
here. But without it Spansion S25FS-S Family is now broken by SFDP.

>>  		err = spi_nor_read_raw(nor, addr, 1, &data_byte);
>>  		if (err)
>>  			goto out;
> [1]https://lkml.org/lkml/2018/10/22/354
>
Boris Brezillon Dec. 3, 2018, 8:23 a.m. UTC | #3 
On Mon, 3 Dec 2018 08:03:18 +0000
"Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
wrote:

> Hello Boris!
> 
> On 30/11/2018 11:40, Boris Brezillon wrote:
> >> +		/*
> >> +		 * JESD216 allows to omit particular address length or latency
> >> +		 * specification in the header and at this point they are still
> >> +		 * unset, so we need some heuristics. One example is S25FS128S.
> >> +		 */
> >> +		if (!nor->addr_width)
> >> +			nor->addr_width = 3;
> >> +		if (!nor->read_dummy)
> >> +			nor->read_dummy = 8;
> >> +  
> > Looks like the same problem was reported by Yogesh here [1]. One more  
> 
> This is indeed the same problem, although I was not aware of the [1] discussion.
> 
> > proof that parsing SFDP is not trivial. I mean, what's the point of
> > defining a generic tables to describe NOR capabilities if you then
> > depend on vendor/chip specific init to read those tables...
> > 
> > Anyway, I think this sort of initialization should be placed in a
> > pre_sfdp() fixup hook, as getting the right values likely requires  
> 
> This is still on my TODO list, to learn about new hooks.

->pre_sfdp() does not exist yet, but I'm about to add it for other
reasons, and it looks like a good place to put this sort of
initialization.

> 
> > reading some volatile/non-volatile regs.  
> 
> This is the same instruction 65h which is used to read regs and which
> appears in SMPT headers, it is a chicken-egg problem.

Oh, right, I remember now. Not a smart decision from Spansion :-/.

> 
> Therefore, I don't know if it's possible to provide smarter heuristics
> here.

Maybe:

ref_cr1 = read_CR1_using_RDCR()

for_each_possible_dummy_and_addr_width
	cr1 = read_CR1_using_RDAR()
	if (cr1 == ref_cr1)
		break;


> But without it Spansion S25FS-S Family is now broken by SFDP.

Do you have [1] in your tree? IIRC, this fixed Yogesh issue.

[1]https://patchwork.ozlabs.org/patch/994765/
Alexander A Sverdlin Dec. 3, 2018, 8:34 a.m. UTC | #4 
Hi!

On 03/12/2018 09:23, Boris Brezillon wrote:
>> This is the same instruction 65h which is used to read regs and which
>> appears in SMPT headers, it is a chicken-egg problem.
> Oh, right, I remember now. Not a smart decision from Spansion :-/.
> 
>> Therefore, I don't know if it's possible to provide smarter heuristics
>> here.
> Maybe:
> 
> ref_cr1 = read_CR1_using_RDCR()
> 
> for_each_possible_dummy_and_addr_width
> 	cr1 = read_CR1_using_RDAR()
> 	if (cr1 == ref_cr1)
> 		break;
> 
> 
>> But without it Spansion S25FS-S Family is now broken by SFDP.
> Do you have [1] in your tree? IIRC, this fixed Yogesh issue.
> 
> [1]https://patchwork.ozlabs.org/patch/994765/

This is definitely unrelated, in my case SMPT is happily processed leading
to completely wrong mapping because command 65h with addr_width == 0 always
returns 0.
Alexander A Sverdlin Dec. 3, 2018, 8:37 a.m. UTC | #5 
Hi!

On 03/12/2018 09:23, Boris Brezillon wrote:
>> This is the same instruction 65h which is used to read regs and which
>> appears in SMPT headers, it is a chicken-egg problem.
> Oh, right, I remember now. Not a smart decision from Spansion :-/.
> 
>> Therefore, I don't know if it's possible to provide smarter heuristics
>> here.
> Maybe:
> 
> ref_cr1 = read_CR1_using_RDCR()
> 
> for_each_possible_dummy_and_addr_width
> 	cr1 = read_CR1_using_RDAR()
> 	if (cr1 == ref_cr1)
> 		break;

This will not work, as default value for CR1(N)V is 0 and that is the value
one gets with incorrect addr_width as well.
Boris Brezillon Dec. 3, 2018, 9:08 a.m. UTC | #6 
On Mon, 3 Dec 2018 08:37:17 +0000
"Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
wrote:

> Hi!
> 
> On 03/12/2018 09:23, Boris Brezillon wrote:
> >> This is the same instruction 65h which is used to read regs and which
> >> appears in SMPT headers, it is a chicken-egg problem.  
> > Oh, right, I remember now. Not a smart decision from Spansion :-/.
> >   
> >> Therefore, I don't know if it's possible to provide smarter heuristics
> >> here.  
> > Maybe:
> > 
> > ref_cr1 = read_CR1_using_RDCR()
> > 
> > for_each_possible_dummy_and_addr_width
> > 	cr1 = read_CR1_using_RDAR()
> > 	if (cr1 == ref_cr1)
> > 		break;  
> 
> This will not work, as default value for CR1(N)V is 0 and that is the value
> one gets with incorrect addr_width as well.
> 

How about:

	// set the WE bit in SR1 so that SR1 is not 0
	write_enable()

	ref_sr1 = read_SR1_using_RDSR1()
	for_each_possible_dummy_and_addr_width
	sr1 = read_SR1_using_RDAR()
	if (sr1 == ref_sr1)
		break;

	write_disable()
Boris Brezillon Dec. 3, 2018, 9:34 a.m. UTC | #7 
On Mon, 3 Dec 2018 10:08:14 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Mon, 3 Dec 2018 08:37:17 +0000
> "Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
> wrote:
> 
> > Hi!
> > 
> > On 03/12/2018 09:23, Boris Brezillon wrote:  
> > >> This is the same instruction 65h which is used to read regs and which
> > >> appears in SMPT headers, it is a chicken-egg problem.    
> > > Oh, right, I remember now. Not a smart decision from Spansion :-/.
> > >     
> > >> Therefore, I don't know if it's possible to provide smarter heuristics
> > >> here.    
> > > Maybe:
> > > 
> > > ref_cr1 = read_CR1_using_RDCR()
> > > 
> > > for_each_possible_dummy_and_addr_width
> > > 	cr1 = read_CR1_using_RDAR()
> > > 	if (cr1 == ref_cr1)
> > > 		break;    
> > 
> > This will not work, as default value for CR1(N)V is 0 and that is the value
> > one gets with incorrect addr_width as well.
> >   
> 
> How about:
> 
> 	// set the WE bit in SR1 so that SR1 is not 0
> 	write_enable()
> 
> 	ref_sr1 = read_SR1_using_RDSR1()
> 	for_each_possible_dummy_and_addr_width
> 		sr1 = read_SR1_using_RDAR()
> 		if (sr1 == ref_sr1)
> 			break;

Oh, and you should read CR2 to make sure AL and RL fields match the
selected addr/dummy values.

> 
> 	write_disable()
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 828d03e..5557c89 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2893,6 +2893,16 @@  static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt)
 		nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
 		addr = smpt[i + 1];
 
+		/*
+		 * JESD216 allows to omit particular address length or latency
+		 * specification in the header and at this point they are still
+		 * unset, so we need some heuristics. One example is S25FS128S.
+		 */
+		if (!nor->addr_width)
+			nor->addr_width = 3;
+		if (!nor->read_dummy)
+			nor->read_dummy = 8;
+
 		err = spi_nor_read_raw(nor, addr, 1, &data_byte);
 		if (err)
 			goto out;