Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

From: Pratyush Yadav <p.yadav@ti.com>
To: <Tudor.Ambarus@microchip.com>
Cc: <michael@walle.cc>, <vigneshr@ti.com>, <figgyc@figgyc.uk>,
	<mail@david-bauer.net>, <linux@rasmusvillemoes.dk>,
	<esben@geanix.com>, <knaerzche@gmail.com>,
	<code@reto-schneider.ch>, <zhengxunli@mxic.com.tw>,
	<jaimeliao@mxic.com.tw>, <heiko.thiery@gmail.com>,
	<macromorgan@hotmail.com>, <sr@denx.de>,
	<miquel.raynal@bootlin.com>, <richard@nod.at>,
	<linux-mtd@lists.infradead.org>
Subject: Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions
Date: Mon, 5 Jul 2021 21:39:52 +0530	[thread overview]
Message-ID: <20210705160950.yaablyujefffhz7o@ti.com> (raw)
In-Reply-To: <579514fe-3134-c964-48ac-af729c4ccffc@microchip.com>

On 05/07/21 01:24PM, Tudor.Ambarus@microchip.com wrote:
> On 7/5/21 4:13 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 02/07/21 05:41PM, Tudor Ambarus wrote:
> >> Provide a way to report the correct flash name in case of ID collisions.
> >> There will be a single flash_info entry when flash IDs collide, and the
> >> differentiation between the flash types will be made at runtime
> >> if possible.
> > 
> > I am not sure if having one entry for all flashes with a collision is a
> > good idea. For example, say we have one flash which supports SFDP and
> > that's all we need for it. Another flash with the same ID does not
> > support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
> > you handle this case with the same entry? You would have to set all the
> > flags in the disambiguation function. And nor->info is declared as const
> > so you can't change the flags in there. Any code checking for
> > info->flags would not work properly for these type of flashes. Wouldn't
> > it be better to have multiple entries with the same ID and just pick the
> > one we need in the disambiguation function?
> 
> Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
> a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
> well as all the other required flash info flags that statically init the flash.
> The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash
> will issue a RDSFDP command that will fail, and then it will init all the NOR
> flags based on the flash info flags.

spi_nor_info_init_params() is called before spi_nor_sfdp_init_params(). 
So if flash A sets SPI_NOR_QUAD_READ, flash B will also inherit it even 
though it might not actually support quad mode reads. You would need to 
refactor the parameter initialization path to do SFDP first and skip the 
flags based init.

But that doesn't solve all the problems. What about the flags that can't 
be discovered by SFDP? I see SPI_NOR_NO_ERASE, SPI_NOR_BP3_SR_BIT6, etc. 
that are not set by the SFDP path. Some of those might be discoverable, 
and we just don't do it yet, but I am not convinced all of them can be. 
For example, I don't see any way to discover SPI_NOR_NO_ERASE via SFDP. 
Erase type 1 field in BFPT is not optional and has to be specified.

How will you differentiate from the SFDP-discoverable and 
non-SFDP-discoverable flags? How can you which belongs to which flash? I 
know this is a bit far fetched but let's solve this problem once and for 
all.

> 
> > 
> > Anyway, if you do decide to go with this approach, comments below.
> 
> Right, thanks.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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