| Message ID | 20230106030601.6530-1-Takahiro.Kuwano@infineon.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable | expand |
Hey, Takahiro, On 06.01.2023 05:06, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > > CFR5V[6] is reserved bit and must always be 1. have you seen some strange behavior? > > Fixes: c3266af101f2 ("mtd: spi-nor: spansion: add support for Cypress Semper flash") > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > Cc: stable@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. Cheers, ta
On 06.01.2023 11:47, Tudor Ambarus wrote: > Hey, Takahiro, > > On 06.01.2023 05:06, tkuw584924@gmail.com wrote: >> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> >> >> CFR5V[6] is reserved bit and must always be 1. > > have you seen some strange behavior? >> >> Fixes: c3266af101f2 ("mtd: spi-nor: spansion: add support for Cypress >> Semper flash") >> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> >> Cc: stable@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. > other idea is to keep some sort of cache register values in the code, to avoid reading repeatedly register values at each update. But we probably don't do so many updates for now, and maybe we can leave this for later.
On 1/6/2023 6:47 PM, Tudor Ambarus wrote: > Hey, Takahiro, > > On 06.01.2023 05:06, tkuw584924@gmail.com wrote: >> From: Takahiro Kuwano <Takahiro.Kuwano@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@infineon.com> >> Cc: stable@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
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 #define SPINOR_OP_CYPRESS_RD_FAST 0xee /* Cypress SPI NOR flash operations. */