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

[Pratyush Yadav <p.yadav@ti.com>]

On 06/07/21 05:19AM, Tudor.Ambarus@microchip.com wrote:
> On 7/5/21 7:09 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > 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.
> > 
> 
> Yeah, that's my goal, to solve all these once and for all. I've started working on it,
> and refactored the params init sequence. I have a WIP branch at:
> https://github.com/ambarus/linux-0day/commits/spi-nor/next-id-collisions.
> I'll post everything after I finish, we can ignore all this for the moment.

Fair enough. Let's pick this topic up again once you have everything 
ready.

> 
> Thanks Pratyush for your insights!

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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