[RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hello Tudor and all,

first of all, thank you for your work on SFDP support in Linux!

Unfortunately, I'm debugging a regression caused by 5390a8df769ec
"mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories"
in [out of tree] support for S25FS128S.

The culprit is the following part of your patch:

	/*
	 * For non-uniform SPI flash memory, set mtd->erasesize to the
	 * maximum erase sector size. No need to set nor->erase_opcode.
	 */
	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
		if (map->erase_type[i].size) {
			erase = &map->erase_type[i];
			break;
		}
	}

The problem in our case is, we have existing partitioning with 128k partitions
(the Flash itself supports 256k and 64k erasesize, depending on configuration).
The chip is configured for 64k erasesize, non-uniform mapping.

The mapping itself is being detected correctly, but when it comes to the code
snippet above, it selects the biggest erasesize from all sizes advertised in
SFDP, including 256k, which is not applicable to the current configuration.

Finally when mtd registers the partitions, they are forced read-only:

	partition "..." doesn't start on an erase block boundary -- force read-only

So this change is not backwards compatible to the existing partitionings.

I cannot come with the justification for the above decision myself, so
I have to ask you guys, what is the reason for setting mtd->erasesize to
the *maximum* erase sector size?

I'd appreciate any ideas on the above, maybe I can convert them to a patch.

Or should mtdpart.c be updated to handle non-uniform erase regions?
But they seem to be spi-nor.c-specific currently...

-- 
Best regards,
Alexander Sverdlin.

Re: [RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Tudor.Ambarus]

Hi, Alexander,

On 11/13/2018 06:58 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Hello Tudor and all,
> 
> first of all, thank you for your work on SFDP support in Linux!
> 
> Unfortunately, I'm debugging a regression caused by 5390a8df769ec
> "mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories"
> in [out of tree] support for S25FS128S.
> 
> The culprit is the following part of your patch:
> 
> 	/*
> 	 * For non-uniform SPI flash memory, set mtd->erasesize to the
> 	 * maximum erase sector size. No need to set nor->erase_opcode.
> 	 */
> 	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> 		if (map->erase_type[i].size) {
> 			erase = &map->erase_type[i];
> 			break;
> 		}
> 	}
> 
> The problem in our case is, we have existing partitioning with 128k partitions
> (the Flash itself supports 256k and 64k erasesize, depending on configuration).
> The chip is configured for 64k erasesize, non-uniform mapping.
> 
> The mapping itself is being detected correctly, but when it comes to the code
> snippet above, it selects the biggest erasesize from all sizes advertised in
> SFDP, including 256k, which is not applicable to the current configuration.

The fix would be to save the supported erase types when parsing the SFDP SMPT
table and use those instead.

> 
> Finally when mtd registers the partitions, they are forced read-only:
> 
> 	partition "..." doesn't start on an erase block boundary -- force read-only
> 
> So this change is not backwards compatible to the existing partitionings.

Maybe you have a partition that is not divisible by 256k?

> 
> I cannot come with the justification for the above decision myself, so
> I have to ask you guys, what is the reason for setting mtd->erasesize to
> the *maximum* erase sector size?

"Major" _valid_ erase sector size. It's an API requirement described at the
mtd_info struct definition. I guess it's for performance reasons. You would like
to use the biggest valid erase commands instead of multiple smaller. Also, one
might require the erase starting address to be aligned with major erase size,
for performance reasons as well.

Cheers,
ta

> 
> I'd appreciate any ideas on the above, maybe I can convert them to a patch.
> 
> Or should mtdpart.c be updated to handle non-uniform erase regions?
> But they seem to be spi-nor.c-specific currently...
> 

Re: [RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Boris Brezillon]

On Wed, 14 Nov 2018 08:00:44 +0000
<Tudor.Ambarus@microchip.com> wrote:

> Hi, Alexander,
> 
> On 11/13/2018 06:58 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> > Hello Tudor and all,
> > 
> > first of all, thank you for your work on SFDP support in Linux!
> > 
> > Unfortunately, I'm debugging a regression caused by 5390a8df769ec
> > "mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories"
> > in [out of tree] support for S25FS128S.
> > 
> > The culprit is the following part of your patch:
> > 
> > 	/*
> > 	 * For non-uniform SPI flash memory, set mtd->erasesize to the
> > 	 * maximum erase sector size. No need to set nor->erase_opcode.
> > 	 */
> > 	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> > 		if (map->erase_type[i].size) {
> > 			erase = &map->erase_type[i];
> > 			break;
> > 		}
> > 	}
> > 
> > The problem in our case is, we have existing partitioning with 128k partitions
> > (the Flash itself supports 256k and 64k erasesize, depending on configuration).
> > The chip is configured for 64k erasesize, non-uniform mapping.
> > 
> > The mapping itself is being detected correctly, but when it comes to the code
> > snippet above, it selects the biggest erasesize from all sizes advertised in
> > SFDP, including 256k, which is not applicable to the current configuration.  
> 
> The fix would be to save the supported erase types when parsing the SFDP SMPT
> table and use those instead.
> 
> > 
> > Finally when mtd registers the partitions, they are forced read-only:
> > 
> > 	partition "..." doesn't start on an erase block boundary -- force read-only
> > 
> > So this change is not backwards compatible to the existing partitionings.  
> 
> Maybe you have a partition that is not divisible by 256k?

Yes, but apparently it was working before the non-uniform erase
changes, so this is a regression. And I fear this is not the only thing
that breaks when mtd->erasesize changes: UBI relies on mtd->erasesize
to figure out its LEB/PEB size, so exiting UBI partitions won't work
anymore on this flash.

> 
> > 
> > I cannot come with the justification for the above decision myself, so
> > I have to ask you guys, what is the reason for setting mtd->erasesize to
> > the *maximum* erase sector size?  
> 
> "Major" _valid_ erase sector size. It's an API requirement described at the
> mtd_info struct definition. I guess it's for performance reasons. You would like
> to use the biggest valid erase commands instead of multiple smaller. Also, one
> might require the erase starting address to be aligned with major erase size,
> for performance reasons as well.

I don't think this is related to perfs (at least not only), it's just
that some flashes have different erase size across the device, and some
MTD users don't want to deal with that, so instead, they use the
biggest erase size assuming this biggest erasesize is a multiple of the
smaller ones and will always work. That's typically what UBI does.

Re: [RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Boris Brezillon]

On Wed, 14 Nov 2018 08:00:44 +0000
<Tudor.Ambarus@microchip.com> wrote:

> Hi, Alexander,
> 
> On 11/13/2018 06:58 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> > Hello Tudor and all,
> > 
> > first of all, thank you for your work on SFDP support in Linux!
> > 
> > Unfortunately, I'm debugging a regression caused by 5390a8df769ec
> > "mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories"
> > in [out of tree] support for S25FS128S.
> > 
> > The culprit is the following part of your patch:
> > 
> > 	/*
> > 	 * For non-uniform SPI flash memory, set mtd->erasesize to the
> > 	 * maximum erase sector size. No need to set nor->erase_opcode.
> > 	 */
> > 	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> > 		if (map->erase_type[i].size) {
> > 			erase = &map->erase_type[i];
> > 			break;
> > 		}
> > 	}
> > 
> > The problem in our case is, we have existing partitioning with 128k partitions
> > (the Flash itself supports 256k and 64k erasesize, depending on configuration).
> > The chip is configured for 64k erasesize, non-uniform mapping.
> > 
> > The mapping itself is being detected correctly, but when it comes to the code
> > snippet above, it selects the biggest erasesize from all sizes advertised in
> > SFDP, including 256k, which is not applicable to the current configuration.  
> 
> The fix would be to save the supported erase types when parsing the SFDP SMPT
> table and use those instead.

Alexander, Tudor, can one of you work on such a fix?

Re: [RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hello Boris,

On 15/11/2018 11:46, Boris Brezillon wrote:
>>> 	/*
>>> 	 * For non-uniform SPI flash memory, set mtd->erasesize to the
>>> 	 * maximum erase sector size. No need to set nor->erase_opcode.
>>> 	 */
>>> 	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>> 		if (map->erase_type[i].size) {
>>> 			erase = &map->erase_type[i];
>>> 			break;
>>> 		}
>>> 	}
>>>
>>> The problem in our case is, we have existing partitioning with 128k partitions
>>> (the Flash itself supports 256k and 64k erasesize, depending on configuration).
>>> The chip is configured for 64k erasesize, non-uniform mapping.
>>>
>>> The mapping itself is being detected correctly, but when it comes to the code
>>> snippet above, it selects the biggest erasesize from all sizes advertised in
>>> SFDP, including 256k, which is not applicable to the current configuration.  
>> The fix would be to save the supported erase types when parsing the SFDP SMPT
>> table and use those instead.
> Alexander, Tudor, can one of you work on such a fix?

non-uniform case is complicated because currently I don't see the way to tell
mtdpart.c about non-uniform maps from spi-nor.c.

But same problem applies even to uniform maps, I have a patch already which
searches for minimum erase size, not maximum, I just have not tested it...

-- 
Best regards,
Alexander Sverdlin.

Re: [RFC] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Tudor.Ambarus]

On 11/15/2018 12:46 PM, Boris Brezillon wrote:
>> The fix would be to save the supported erase types when parsing the SFDP SMPT
>> table and use those instead.
> 
> Alexander, Tudor, can one of you work on such a fix?

I will fix it, probably next week.