spi_nor Erase operation selection is excessively conservative

[Crzyrndm]

Summary: SPI_NOR driver is selecting a smaller erase operation in in many situations it doesn't need to

SPI NOR FLASH devices often have multiple erase sizes. 4k, 32k and 64k are very typical.
When spi_nor_erase (link) is invoked part of the loop attempts to pick the largest erase operation for the current address and remaining size (link)

I was erasing parts of a 512MiB NOR device today and observed extremely slow erase speeds. I found that many of the erase operation where using only the 4kiB erase even when the address/remainder was suitable for a much larger operation.

I extracted the main parts of the op selector in order to confirm my suspicions. This can be seen here

erasing 0x40000 bytes starting at 0x4000
loc: 0x4000, remaining: 0x40000, chosen: 0x1000
loc: 0x5000, remaining: 0x3F000, chosen: 0x1000
loc: 0x6000, remaining: 0x3E000, chosen: 0x1000
loc: 0x7000, remaining: 0x3D000, chosen: 0x1000
loc: 0x8000, remaining: 0x3C000, chosen: 0x1000 // should have been a 32k erase
loc: 0x9000, remaining: 0x3B000, chosen: 0x1000
loc: 0xA000, remaining: 0x3A000, chosen: 0x1000
loc: 0xB000, remaining: 0x39000, chosen: 0x1000
loc: 0xC000, remaining: 0x38000, chosen: 0x1000
loc: 0xD000, remaining: 0x37000, chosen: 0x1000
loc: 0xE000, remaining: 0x36000, chosen: 0x1000
loc: 0xF000, remaining: 0x35000, chosen: 0x1000
loc: 0x10000, remaining: 0x34000, chosen: 0x1000 // should have been a 64k erase
loc: 0x11000, remaining: 0x33000, chosen: 0x1000
...

What I would expect to see is that 4k erases are used until 32k is reached, a 32k erase to get to 64k then 64k erases until the last 16 kB. What actually happens is that only 4k erases are used.

The problem is this line. For a larger erase op to be chosen, the final end address must be aligned with that op size. In my example above, the end is only 16kB aligned so this never occurs

The correct check would be size >= (1 << etp->exp) (updated example. Change is to line 30). This results in the desired/expected output with the largest applicable erase op being used at each step

erasing 0x40000 bytes starting at 0x4000
loc: 0x4000, remaining: 0x40000, chosen: 0x1000
loc: 0x5000, remaining: 0x3F000, chosen: 0x1000
loc: 0x6000, remaining: 0x3E000, chosen: 0x1000
loc: 0x7000, remaining: 0x3D000, chosen: 0x1000
loc: 0x8000, remaining: 0x3C000, chosen: 0x8000 // 32k
loc: 0x10000, remaining: 0x34000, chosen: 0x10000 // 64k
loc: 0x20000, remaining: 0x24000, chosen: 0x10000
loc: 0x30000, remaining: 0x14000, chosen: 0x10000
loc: 0x40000, remaining: 0x4000, chosen: 0x1000 // back to 4k
loc: 0x41000, remaining: 0x3000, chosen: 0x1000
loc: 0x42000, remaining: 0x2000, chosen: 0x1000
loc: 0x43000, remaining: 0x1000, chosen: 0x1000

[Laczen]

@Crzyrndm, the check should indeed be that the (remaining) size is larger or equal to the erase size. Could you provide a PR ?

[Crzyrndm]

@Crzyrndm, the check should indeed be that the (remaining) size is larger or equal to the erase size. Could you provide a PR ?

PR has been opened. I think I got all the neccesary conventions covered but feel free to just fix it if more needs doing