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

[Tudor.Ambarus at microchip.com]

On 10/22/21 2:37 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-10-21 11:30, schrieb Pratyush Yadav:
>> On 21/10/21 08:44AM, Tudor.Ambarus at microchip.com wrote:
>>> On 10/20/21 12:55 PM, Tudor.Ambarus at 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.

right. Anyway, we should aim to have less and less flags. I'll try to get rid
of few.

Cheers,
ta