On Mon, Jan 23 2023, Dhruva Gole wrote:
Hi Pratyush,
On 23/01/23 20:07, Pratyush Yadav wrote:
+Cc Dhruva
I had already reviewed this, but now I have locally applied the patches, to linux master and built and tested - seems okay,
Hi Tudor,
On Tue, Jan 10 2023, Tudor Ambarus wrote:
CFR5[6] is reserved bit and must be always 1. Set it to comply with flash requirements. While fixing SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_{EN, DS} definition, stop using magic numbers and describe the missing bit fields in CFR5 register. This is useful for both readability and future possible addition of Octal STR mode support.
Fixes: c3266af101f2 ("mtd: spi-nor: spansion: add support for Cypress Semper flash") Cc: stable@vger.kernel.org Reported-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com Signed-off-by: Tudor Ambarus tudor.ambarus@linaro.org
drivers/mtd/spi-nor/spansion.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index b621cdfd506f..07fe0f6fdfe3 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -21,8 +21,13 @@ #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_CFR5_BIT6 BIT(6)
Perhaps comment here that this bit is reserved. Otherwise it is not obvious what this does and why we are setting it without going through git-blame. No need for a re-roll, I think it is fine if you add this when applying.
+#define SPINOR_REG_CYPRESS_CFR5_DDR BIT(1) +#define SPINOR_REG_CYPRESS_CFR5_OPI BIT(0) +#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN \
(SPINOR_REG_CYPRESS_CFR5_BIT6 | SPINOR_REG_CYPRESS_CFR5_DDR | \SPINOR_REG_CYPRESS_CFR5_OPI)+#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS SPINOR_REG_CYPRESS_CFR5_BIT6
I would say don't fix what isn't broken. But if you do, test it. Do you or Takahiro have a Cypress S28* flash to test this change on? If no, then perhaps Dhruva can help here since TI uses this flash on a bunch of their boards?
The change looks fine to me with the above comment added, I just would like someone to test it once.
Tested OSPI_S_FUNC_DD_RW_ERASESIZE_UBIFS from ltp-ddt and test seemed to pass on my AM625 SK EVM having an OSPI NOR S28HS512T Flash.
Thanks.
BTW, one interesting bit about this in case you didn't know already. Since this is playing with Octal DTR enable/disable, you might also want to double-check you are actually using Octal DTR mode. This can be done by looking at the SPI NOR debugfs entry (usually in /sys/kernel/debug/spi-nor). You can "cat params" and see what protocols are being used, and make sure 8D-8D-8D is being used.
AM625 SK _should_ be using 8D-8D-8D mode already, but I think it is useful if you know how to confirm this assumption :-)
Reviewed-by: Pratyush Yadav ptyadav@amazon.de
For this series,
Tested-by: Dhruva Gole d-gole@ti.com
#define SPINOR_OP_CYPRESS_RD_FAST 0xee
/* Cypress SPI NOR flash operations. */