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

[Michael Walle <michael@walle.cc>]

Am 2021-10-21 11:30, schrieb Pratyush Yadav:
> 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.

I'm sorry, I'm late to this. But I'd prefer the flags, simply because 
the
"set flags with a function" doesn't scale very well; you can't ORing
functions together. So we'll eventually have many functions for 
different
combinations of the flags.

Is running out of bits really a problem? Even if we need more than 32 
bits,
we can just use set_bit() with an array of ulongs.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel