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

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:

quoted

On 21/10/21 08:44AM, Tudor.Ambarus@microchip.com wrote:

quoted

On 10/20/21 12:55 PM, Tudor.Ambarus@microchip.com wrote:

quoted

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:

quoted

quoted

quoted

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.

quoted

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.

quoted

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()

quoted

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.

quoted

late_init() so it certainly doesn't make anything worse.

yes, this is a different topic.

quoted

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

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