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

[<Tudor.Ambarus@microchip.com>]

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.

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