[PATCH v2] mtd: spi-nor: core: Discard HW capabilities if no enable function

[Tudor Ambarus]

On 20.12.2023 10:50, Michael Walle wrote:
> Am 2023-12-20 09:20, schrieb Tudor Ambarus:
>> On 19.12.2023 12:21, Jaime Liao wrote:
>>> From: JaimeLiao <jaimeliao at mxic.com.tw>
>>>
>>> Discard corresponding HW capabilities to prevent carrying the
>>> wrong protocol if no QUAD/Octal DTR enable function hooked.
>>>
>>> Signed-off-by: JaimeLiao <jaimeliao at mxic.com.tw>
>>> ---
>>> changes in v2
>>>  - Add SNOR_HWCAPS_8_8_8_DTR
>>>  - Restore the enable function judgement in spi_nor_set_octal_dtr()
>>>  - Restore the enable function judgement in spi_nor_quad_enable()
>>> ---
>>>  drivers/mtd/spi-nor/core.c  | 7 +++++++
>>>  include/linux/mtd/spi-nor.h | 6 ++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 1c443fe568cf..14359101c6cf 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct
>>> spi_nor *nor,
>>>       */
>>>      shared_mask = hwcaps->mask & params->hwcaps.mask;
>>>
>>> +    /* Mask out Octal DTR if no enable function */
>>> +    if (!params->set_octal_dtr)
>>> +        shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR;
>>> +
>>> +    if (!params->quad_enable)
>>> +        shared_mask &= ~SNOR_HWCAPS_4_4_4;
>>
>> and these should have been in the late init hook, and instead discard
>> them from the params->hwcaps.mask.
> 
> Maybe there is a better place to mask these bits. But IMHO the core
> should do it on itself and we shouldn't need to provide an extra
> hook function for every driver ourselves. The core knows that there
> is no .octal_enable op and thus it shouldn't even try to enable
> this mode.

I meant in the core, at the end of the late_init_params(). Here:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n2962
> 
>> And why is this extra check needed in the first place? For octal I
>> assume it's needed by macronix, where the flash is octal capable, but
>> there's no octal enable method defined yet.
> 
> Because it is the generic spi nor flash driver, which doesn't provide
> any of these enable x-mode helpers.

right, that was my guess too. The commit message has to be updated.
> 
>> What about the quad mode? Is
>> there any flash that has no quad enable method defined but still caries
>> quad mode in its hwcaps?
> 
> No it is for completeness and correctness. At the moment we are always
> setting a (random) default quad enable op, due to legacy reasons.
> 

Still, the commit message shall indicate this. Can't add extra checks
out of the blue. Especially since nobody is affected.

Cheers,
ta