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

[<Tudor.Ambarus@microchip.com>]

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...
>