mtd: spi-nor: flash_info db overhaul

[Tudor Ambarus]

On 7/17/23 14:51, Michael Walle wrote:
> Hi,
> 
> I'm planning to do a makeover of how the in-kernel flash database of
> the SPI-NOR parts are represented. It all started with the quest for
> dropping unneeded properties like size and sector_size for newer
> flashes which already contains self describing tables within the flash
> itself :)
> 
> Soon additional drawbacks (or maybe legacy features :) were discovered:
>  * the SNOR_IDs are hardcoded to either 3 or 6 bytes. But with the dawn
>    of continuation codes, these IDs have a flexible length.
>    Fun fact, we have one flash with a correct continuation code,
>    "is25cd512" with id 0x7f9d20, 0x7f9d is the vendor id and 0x20 the
>    device id, which is just one byte for this flash. First, I though that
>    was a mistake, but actually it is in line with the datasheet. Apparently,
>    the vendor is afraid of spi code which only supports 3 id bytes :) The
>    same is true for "pm25lq032".
>  * all the macros contain a trailing comma, and thus the trailing comma
>    is omitted in the flash db which makes is somewhat inconsistent if you
>    also want to use non-macro entires, like ".fixups = fixup,"
>  * macros which just returns their argument, e.g.
>    #define NO_SFDP_FLAGS(x) .no_sfdp_flags = (x),
>  * newer flashes only need the id, a name and some flags, thus INFO()
>    has a lot of empty fields,
>  * newer flashes need to explicitly specify PARSE_SFDP.
> 
> Goals I'd like to achieve:
>  * first and formost, new flash entries should be as slim as possible,
>  * the id should have a flexible length,
>  * same (consistent) format for legacy flash entries and newer ones,
>  * no overuse of macros
> 
> 
> Here are some old examples:
> { "mt25ql256a",  INFO6(0x20ba19, 0x104400, 64 * 1024,  512)
>     NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>     FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
>     MFR_FLAGS(USE_FSR)
> },  // (1)
> { "n25q256ax1",  INFO(0x20bb19, 0, 64 * 1024,  512)
>     NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
>     MFR_FLAGS(USE_FSR)
> },  // (2)
> { "w25q512nwq", INFO(0xef6020, 0, 0, 0)
>     PARSE_SFDP
>     OTP_INFO(256, 3, 0x1000, 0x1000)
> }, // (3)
> { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
>     PARSE_SFDP
>     FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>     NO_SFDP_FLAGS(SECT_4K)
> }, // (4)
> 
> Here's the (idea of the) final version:
> {
>   .id = SNOR_ID(0x20, 0xba, 0x19, 0x10, 0x44, 0x00),
>   .name = "mt25ql256a",
>   .size = SZ_32M,
>   .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>   .fixup_flags = SPI_NOR_4B_OPCODES,
>   .mfr_flags = USE_FSR,
> } (1)
> {
>   .id = SNOR_ID(0x20, 0xbb, 0x19),
>   .name = "n25q256ax1",
>   .size = SZ_32M,
>   .no_sfdp_flags = SECT_4K | SPI_NOR_QUAD_READ,
>   .mfr_flags = USE_FSR,
> } (2)
> {
>   .id = SNOR_ID(0xef, 0x60, 0x20),
>   .name = "w25q512nwq",
>   .otp_org = SNOR_OTP_ORG(256, 3, 0x1000, 0x1000),
>   // or maybe just .otp = SNOR_OTP(),

I like SNOR_OTP.

> } (3)
> {
>   .id = SNOR_ID(0xef, 0x40, 0x18),
>   .name = "w25q128",
>   .size = SZ_16M,
>   .force_sfdp = true,
>   .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB,
>   .no_sfdp_flags = SECT_4K,
> }, // (4)
> 
> The full template would be:
> {
>  .id = SNOR_ID(),
>  .name = ,
>  .size = , // will replace n_sectors
>  .sector_size = , // if 0, SZ_64K is assumed, otherwise has to be set
>  .n_banks = , // if 0, 1 is assumed, otherwise has to be set
>  .page_size = , // if 0, 256 is assumed, otherwise has to be set
>  .addr_nbytes = , // addr_nbytes is already special, see spi_nor_set_addr_nbytes()
>  .flags = ,
>  .no_sfdp_flags = ,
>  .mfr_flags = ,
>  .otp_org = SNOR_OTP_ORG()
>  .fixups = .
>  .fixup_flags = ,
>  .force_sfdp = , // see below!
> }
> 
> The only mandatory field is .id because that is the key into
> the db entry. Of course, with the dawn of the generic spi flash
> driver, an entry with no additional flags doesn't make any sense.
> Therefore, I'd expect an entry to have at least .*flags or
> .otp_org set.

Right, the goal is to define a flash by name, id, and everything else that
can't be discovered when parsing SFDP.

> 
> Alternatively, the second key is the name for legacy flashes which
> are looked up by the name instead of the id. If we ever need to
> resort to individual DT compatibles, I'd use a new .compatible entry
> for that like everywhere else in the kernel.
> 
> One word on .parse_sfdp (@Tudor this is new, so let me know). Because
> I want new entries as slim as possible and I expect that all new flashes
> will support SFDP, I'd like to get rid of the mandatory
> ".parse_sfdp = true" line. The first idea was to just invert the logic
> and have a .no_sfdp which will need to be set to true for all the old
> flashes. But I think we can do better. We can assume that each entry
> with .size != 0 (or .n_sectors in the old format) will implicitly be
> ".no_sfdp = true" (or parse_sfdp = false in the old format). There is

the assumption will work indeed.

> of course one exception to this rule. Flashes with the same ID where
> one supports SFDP and one doesn't. In the end the outcome is the same,
> we need to force SFDP parsing even if .size non-zero. Thus for this
> case, we need a .force_sfdp = true entry. See also example (4) above.
> 
I wouldn't introduce a new member just for exceptions. How about setting
.parse_sfdp = true at definition only in this case and add a comment on
top of it saying that we force sfdp parsing to differentiate between the
flashes.

> I've already had a talk with Tudor about that topic. Does anyone else
> have thoughts on it?
> 

I confirm that I agree with the changes proposed.

Cheers,
ta