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

[Michael Walle <mwalle@kernel.org>]

Am 2023-12-20 09:20, schrieb Tudor Ambarus:
> On 19.12.2023 12:21, Jaime Liao wrote:
>> From: JaimeLiao <jaimeliao@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@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.

> 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.

> 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.

-michael

>> +
>>  	if (nor->spimem) {
>>  		/*
>>  		 * When called from spi_nor_probe(), all caps are set and we
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index cdcfe0fd2e7d..78a119192ee0 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps {
>>  #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
>>  #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
>> 
>> +#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
>> +				 SNOR_HWCAPS_PP_4_4_4)
>> +
>> +#define SNOR_HWCAPS_8_8_8_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR |	\
>> +				 SNOR_HWCAPS_PP_8_8_8_DTR)
>> +
>>  #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
>>  				 SNOR_HWCAPS_READ_4_4_4 |	\
>>  				 SNOR_HWCAPS_READ_8_8_8 |	\

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/