From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
CFR5V[6] is reserved bit and must always be 1.
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 #define SPINOR_OP_CYPRESS_RD_FAST 0xee
/* Cypress SPI NOR flash operations. */
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
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) +#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 #define SPINOR_OP_CYPRESS_RD_FAST 0xee
/* Cypress SPI NOR flash operations. */
On 10/01/23 22:17, 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
Reviewed-by: Dhruva Gole d-gole@ti.com
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) +#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 #define SPINOR_OP_CYPRESS_RD_FAST 0xee /* Cypress SPI NOR flash operations. */
On 1/11/2023 1:47 AM, 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) +#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 #define SPINOR_OP_CYPRESS_RD_FAST 0xee /* Cypress SPI NOR flash operations. */
Thank you, Tudor! I will fix u-boot in the same manner.
Takahiro
+Cc Dhruva
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.
Reviewed-by: Pratyush Yadav ptyadav@amazon.de
#define SPINOR_OP_CYPRESS_RD_FAST 0xee
/* Cypress SPI NOR flash operations. */
2.34.1
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.
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. */
2.34.1
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. */
On 24/01/23 16:13, Pratyush Yadav wrote:
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.
Yes, I believe 8D-8D-8D is being used. Here's the output: ... root@am62xx-evm:~# cat /sys/kernel/debug/spi-nor/spi0.0/params name s28hs512t id 34 5b 1a 0f 03 90 size 64.0 MiB write size 16 page size 256 address nbytes 4 flags 4B_OPCODES | HAS_4BAIT | HAS_16BIT_SR | IO_MODE_EN_VOLATILE | SOFT_RESET
opcodes read 0xee dummy cycles 24 erase 0xd8 program 0x12 8D extension repeat
protocols read 8D-8D-8D write 8D-8D-8D register 8D-8D-8D
erase commands 21 (4.00 KiB) [2] dc (256 KiB) [3] c7 (64.0 MiB)
sector map region (in hex) | erase mask | flags ------------------+------------+---------- 00000000-0001ffff | [ 2 ] | 00020000-0003ffff | [ 3] | overlaid 00040000-03ffffff | [ 3] | ...
Also, the raw read speed is about 40 MBPS The frequency we set from DT is 25 MHz, assuming 1MB is transfered every cycle's rising and falling edge, ie. 25*2 = 50 MBPS is ideal speed.
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. */
On Tue, Jan 24 2023, Dhruva Gole wrote:
On 24/01/23 16:13, Pratyush Yadav wrote:
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.
Yes, I believe 8D-8D-8D is being used. Here's the output: ... root@am62xx-evm:~# cat /sys/kernel/debug/spi-nor/spi0.0/params name s28hs512t id 34 5b 1a 0f 03 90 size 64.0 MiB write size 16 page size 256 address nbytes 4 flags 4B_OPCODES | HAS_4BAIT | HAS_16BIT_SR | IO_MODE_EN_VOLATILE | SOFT_RESET
opcodes read 0xee dummy cycles 24 erase 0xd8 program 0x12 8D extension repeat
protocols read 8D-8D-8D write 8D-8D-8D register 8D-8D-8D
erase commands 21 (4.00 KiB) [2] dc (256 KiB) [3] c7 (64.0 MiB)
sector map region (in hex) | erase mask | flags ------------------+------------+---------- 00000000-0001ffff | [ 2 ] | 00020000-0003ffff | [ 3] | overlaid 00040000-03ffffff | [ 3] | ...
Also, the raw read speed is about 40 MBPS The frequency we set from DT is 25 MHz, assuming 1MB is transfered every cycle's rising and falling edge, ie. 25*2 = 50 MBPS is ideal speed.
Yep, that's what I would expect as well. Thanks for confirming.
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. */
-- Best regards, Dhruva Gole Texas Instruments Incorporated
Hi Dhruva and Pratyush,
On 1/24/2023 1:31 AM, 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,
I had tested on my side as well. Sorry for the late reply.
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.
Reviewed-by: Pratyush Yadav ptyadav@amazon.de
For this series,
Tested-by: Dhruva Gole d-gole@ti.com
Thank you!
#define SPINOR_OP_CYPRESS_RD_FAST 0xee
/* Cypress SPI NOR flash operations. */
2.34.1
Takahiro
On 10.01.2023 18:47, 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 KuwanoTakahiro.Kuwano@infineon.com Signed-off-by: Tudor Ambarustudor.ambarus@linaro.org
drivers/mtd/spi-nor/spansion.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
both applied, thanks!
linux-stable-mirror@lists.linaro.org
-
Dhruva Gole -
Pratyush Yadav -
Takahiro Kuwano -
tkuw584924＠gmail.com
-
Tudor Ambarus