mtd: spi-nor: flash_info db overhaul

* mtd: spi-nor: flash_info db overhaul
@ 2023-07-17 13:51 Michael Walle
  2023-07-17 14:55 ` Tudor Ambarus
  0 siblings, 1 reply; 2+ messages in thread
From: Michael Walle @ 2023-07-17 13:51 UTC (permalink / raw)
  To: tudor.ambarus, pratyush, michael, miquel.raynal, richard,
	vigneshr
  Cc: linux-mtd

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(),
} (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.

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
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've already had a talk with Tudor about that topic. Does anyone else
have thoughts on it?

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 2+ messages in thread