Re: [PATCH v2 18/35] mtd: spi-nor: Get rid of SPI_NOR_4B_OPCODES flag

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

On 21/10/21 08:44AM, Tudor.Ambarus@microchip.com wrote:
> On 10/20/21 12:55 PM, Tudor.Ambarus@microchip.com wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 10/19/21 8:26 PM, Pratyush Yadav wrote:
> >>>> While we are on this topic, I find this a bit "ugly". Having to set
> >>>> late_init() for setting these flags for each flash is not exactly very
> >>>> clean or readable. I don't know how the future will look like, but if
> >>>> each flash/family needs its own late_init() to set some flags, it won't
> >>>> be very readable. We seem to be trading one type of complexity for
> >>>> another. I dunno which is the lesser evil though...
> >>> Your point is valid. This patch removes SPI_NOR_4B_OPCODES and sets
> >>> SNOR_F_4B_OPCODES in a late_init() hook, forcing the reader to go through
> >>> the late_init() function to see what's there. As you saw, late_init() can be
> >>> used for tweaking flash's parameters, settings and methods, not just NOR flags,
> >>> so I would expect that this hook to be present among flashes that don't define
> >>> the SFDP tables or for flashes that have parameters that are not SFDP discoverable,
> >>> the hook will be there anyway.
> >>>
> >>> This patch opens the door on how we could handle the flash_info flags. All flash_info
> >>> flags that can be determined when parsing SFDP can be removed and use for flashes that
> >>> skip SFDP, SNOR_F equivalents in late_init() methods. spi_nor_info_init_params()
> >>> should NOT be called for SFDP capable flashes anyway, because in case of SFDP flashes,
> >>> all the settings done in spi_nor_info_init_params() are overwritten when parsing SFDP.
> >>> 1/ flashes with SFDP will set the flags as:
> >>> SPI_NOR_PARSE_SFDP | non-sfdp-discoverable-flags
> >>> 2/ flashes without SFDP:
> >>> SPI_NOR_SKIP_SFDP | non-sfdp-discoverable-flags
> >>> and a late_init() for SNOR_F equivalents of flash_info flags from
> >>> spi_nor_info_init_params()
> >>> 3/ flashes that collide, one with SFDP and the other without:
> >>> SPI_NOR_PARSE_SFDP | non-sfdp-discoverable-flags
> >>> and a late_init() for SNOR_F equivalents of flash_info flags from
> >>> spi_nor_info_init_params(), that will be used for the flash without SFDP.
> >>> 4/ individual flash, no collisions, a flavor supports SFDP, the other not:
> >>> SPI_NOR_PARSE_SFDP | non-sfdp-discoverable-flags
> >>> and a late_init() for SNOR_F equivalents of flash_info flags from
> >>> spi_nor_info_init_params(), that will be used for the flash without SFDP.
> >> To me it looks like you can separate these flags into three classes:
> >>
> >>   1. Whether to parse SFDP or not.
> >>   2. Flags that can't be discovered via SFDP.
> >>   3. Flags that can be discovered by SFDP ideally but can't be
> >>      discovered for this particular flash because either SFDP is missing
> >>      or the table for this flag is missing.
> > 
> > These are the flash_info flags, indeed. Apart of these there are the SNOR_F flags
> > which are set either statically (one sets a flash_info flag equivalent when
> > declaring the flash), or dynamically when parsing SFDP. Check
> > SPI_NOR_4B_OPCODES and SNOR_F_4B_OPCODES for example.
> > 
> >>
> >> With your series, flags from 1 and 2 are populated via .flags in
> >> flash_info and the ones from 3 are populated via late_init().
> > 
> > My proposal was to get rid of the flash_info flags from the 3rd category that you
> > described, and set the SNOR_F equivalents in a late_init() hook. This way we also
> > control when the SNOR_F equivalents are set, late in the init call. But this can
> > be achieved with your proposal as well, let's see.
> > 
> >>
> >> Why can't we have 3 different fields for these 3 different flags? In
> >> flash_info, we can set .parse_sfdp to true/false to indicate SFDP
> >> support. We can set .nonsfdp_flags = X | Y | Z for non-sfdp-discoverable
> >> flags. And we can set .fixup_flags = A | B | C (can probably pick a
> >> better name) for the flags that your series sets through late_init().
> >>
> >> This way, you have a clear separation between the three and they are all
> >> clearly visible in the flash entry itself.
> > 
> > The downside that I see with this is that we extend the flash_info struct with new
> > fields and the spi-nor.o's size will increase whether the fields are used or not,
> > as we have lots of flash_info entries. This reminds me that probably I should have
> > put the late_init() hook inside const struct spi_nor_fixups. Anyway, we can avoid
> > increasing the size with some flash_info flags masks. We use the same flash_info flags
> > entry, but we introduce some masks, to separate the type of flags. Something like:
> > SPI_NOR_PARSE_SFDP |
> >         NON_SFDP_FLAGS(SPI_NOR_TB_SR_BIT6 | SPI_NOR_4BIT_BP | SPI_NOR_SWP_IS_VOLATILE)
> > these are for category 1 and 2 in your description
> > 
> > or
> > SPI_NOR_SKIP_SFDP | SFDP_FLAGS(SPI_NOR_OCTAL_DTR_READ | SPI_NOR_OCTAL_DTR_PP)
> > for categories 1 and 3 in your description
> > 
> > but you can end up with flags like:
> > SPI_NOR_SKIP_SFDP | SFDP_FLAGS() | NON_SFDP_FLAGS()
> > 
> > 
> >>
> >> The only case where this might run into trouble is when a SFDP flash has
> >> a collision with a non-SFDP flash and they both need different
> >> fixup_flags. But I supposed that is a problem even if you use
> > 
> > we can probably solve this by putting the minimum supported flags by both
> > and fill the rest in fixup hooks after we determine which flash is which.
> > 
> >> late_init() so it certainly doesn't make anything worse.
> > 
> > yes, this is a different topic.
> > 
> >>
> >> I have not given this extensive thought, but it seems to make sense to
> >> me, and I feel that it would make the flow easier to follow. Thoughts?
> > 
> > Both approaches are fine. Your method keeps all flags in one place but duplicates
> > the setting of flags, you'll have "if flash_info flag, set SNOR_F flag".
> > Mine gets rid of the SFDP flash_info flags and directly sets SNOR_F equivalents
> > with the detriment of introducing fixup hooks at flash declaration. Can we involve
> > Michael and Vignesh to get their preference so that we come to an agreement and move
> > forward?
> > 
> 
> I'll go with the flags mask idea.

Fine by me. I am worried about running out of flag bits but we should be 
able to bump up the flags field to 64 bits without much trouble when 
that happens.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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