mtd: spi-nor: flash_info db overhaul

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/

I like SNOR_OTP.

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

the assumption will work indeed.

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 confirm that I agree with the changes proposed.

Cheers,
ta

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