[PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable

[Takahiro Kuwano]

On 1/6/2023 6:47 PM, Tudor Ambarus wrote:
> Hey, Takahiro,
> 
> On 06.01.2023 05:06, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>
>> CFR5V[6] is reserved bit and must always be 1.
> 
> have you seen some strange behavior?
No, just to follow the datasheet.

>>
>> Fixes: c3266af101f2 ("mtd: spi-nor: spansion: add support for Cypress Semper flash")
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> Cc: stable at vger.kernel.org
>> ---
>>   drivers/mtd/spi-nor/spansion.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index b621cdfd506f..4e094a432d29 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -21,8 +21,8 @@
>>   #define SPINOR_REG_CYPRESS_CFR3V        0x00800004
>>   #define SPINOR_REG_CYPRESS_CFR3V_PGSZ        BIT(4) /* Page size. */
>>   #define SPINOR_REG_CYPRESS_CFR5V        0x00800006
>> -#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN    0x3
>> -#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS    0
>> +#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN    0x43
>> +#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS    0x40
> 
> No, this looks bad. Instead of overwriting CFR5V with whatever value, we
> should instead first read it and then update only the bit that we're
> interested in. If it happens to write CFR5V before octal enable/disable,
> you'll overwrite the previous set values.
> 
I understand read-modify-write is the right thing in general. Actually
CFR5V[7] and CFR5V[5:2] are reserved and must be 0 so I preferred a simple
fix for this particular case.

Thanks,
Takahiro