* [PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable
@ 2023-01-06 3:06 tkuw584924
2023-01-06 9:47 ` Tudor Ambarus
0 siblings, 1 reply; 14+ messages in thread
From: tkuw584924 @ 2023-01-06 3:06 UTC (permalink / raw)
To: linux-mtd
Cc: tudor.ambarus, pratyush, michael, miquel.raynal, richard,
vigneshr, tkuw584924, Bacem.Daassi, Takahiro Kuwano, stable
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. */
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable
2023-01-06 3:06 [PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable tkuw584924
@ 2023-01-06 9:47 ` Tudor Ambarus
2023-01-06 9:55 ` Tudor Ambarus
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Tudor Ambarus @ 2023-01-06 9:47 UTC (permalink / raw)
To: tkuw584924, linux-mtd
Cc: pratyush, michael, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable
2023-01-06 9:47 ` Tudor Ambarus
@ 2023-01-06 9:55 ` Tudor Ambarus
2023-01-10 4:39 ` Takahiro Kuwano
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
2 siblings, 0 replies; 14+ messages in thread
From: Tudor Ambarus @ 2023-01-06 9:55 UTC (permalink / raw)
To: tkuw584924, linux-mtd
Cc: pratyush, michael, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano, stable
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable
2023-01-06 9:47 ` Tudor Ambarus
2023-01-06 9:55 ` Tudor Ambarus
@ 2023-01-10 4:39 ` Takahiro Kuwano
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
2 siblings, 0 replies; 14+ messages in thread
From: Takahiro Kuwano @ 2023-01-10 4:39 UTC (permalink / raw)
To: Tudor Ambarus, linux-mtd
Cc: pratyush, michael, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-06 9:47 ` Tudor Ambarus
2023-01-06 9:55 ` Tudor Ambarus
2023-01-10 4:39 ` Takahiro Kuwano
@ 2023-01-10 16:47 ` Tudor Ambarus
2023-01-11 6:28 ` Dhruva Gole
` (3 more replies)
2 siblings, 4 replies; 14+ messages in thread
From: Tudor Ambarus @ 2023-01-10 16:47 UTC (permalink / raw)
To: Takahiro.Kuwano, tkuw584924
Cc: linux-mtd, pratyush, michael, miquel.raynal, richard,
Bacem.Daassi, Tudor Ambarus, stable
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. */
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
@ 2023-01-11 6:28 ` Dhruva Gole
2023-01-18 7:34 ` Takahiro Kuwano
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Dhruva Gole @ 2023-01-11 6:28 UTC (permalink / raw)
To: Tudor Ambarus, Takahiro.Kuwano, tkuw584924
Cc: linux-mtd, pratyush, michael, miquel.raynal, richard,
Bacem.Daassi, stable
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. */
--
Thanks and Regards,
Dhruva Gole
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
2023-01-11 6:28 ` Dhruva Gole
@ 2023-01-18 7:34 ` Takahiro Kuwano
2023-01-23 14:37 ` Pratyush Yadav
2023-01-31 9:00 ` Tudor Ambarus
3 siblings, 0 replies; 14+ messages in thread
From: Takahiro Kuwano @ 2023-01-18 7:34 UTC (permalink / raw)
To: Tudor Ambarus, Takahiro.Kuwano
Cc: linux-mtd, pratyush, michael, miquel.raynal, richard,
Bacem.Daassi, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
2023-01-11 6:28 ` Dhruva Gole
2023-01-18 7:34 ` Takahiro Kuwano
@ 2023-01-23 14:37 ` Pratyush Yadav
2023-01-23 16:31 ` Dhruva Gole
2023-01-31 9:00 ` Tudor Ambarus
3 siblings, 1 reply; 14+ messages in thread
From: Pratyush Yadav @ 2023-01-23 14:37 UTC (permalink / raw)
To: Tudor Ambarus
Cc: Takahiro.Kuwano, tkuw584924, linux-mtd, pratyush, michael,
miquel.raynal, richard, Bacem.Daassi, Dhruva Gole, stable
+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
>
>
--
Regards,
Pratyush Yadav
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-23 14:37 ` Pratyush Yadav
@ 2023-01-23 16:31 ` Dhruva Gole
2023-01-24 10:43 ` Pratyush Yadav
2023-02-01 7:14 ` Takahiro Kuwano
0 siblings, 2 replies; 14+ messages in thread
From: Dhruva Gole @ 2023-01-23 16:31 UTC (permalink / raw)
To: Pratyush Yadav, Tudor Ambarus
Cc: Takahiro.Kuwano, tkuw584924, linux-mtd, pratyush, michael,
miquel.raynal, richard, Bacem.Daassi, stable
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
>>
>>
--
Best regards,
Dhruva Gole
Texas Instruments Incorporated
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-23 16:31 ` Dhruva Gole
@ 2023-01-24 10:43 ` Pratyush Yadav
2023-01-24 11:12 ` Dhruva Gole
2023-02-01 7:14 ` Takahiro Kuwano
1 sibling, 1 reply; 14+ messages in thread
From: Pratyush Yadav @ 2023-01-24 10:43 UTC (permalink / raw)
To: Dhruva Gole
Cc: Tudor Ambarus, Takahiro.Kuwano, tkuw584924, linux-mtd, pratyush,
michael, miquel.raynal, richard, Bacem.Daassi, stable
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. */
--
Regards,
Pratyush Yadav
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-24 10:43 ` Pratyush Yadav
@ 2023-01-24 11:12 ` Dhruva Gole
2023-01-24 12:03 ` Pratyush Yadav
0 siblings, 1 reply; 14+ messages in thread
From: Dhruva Gole @ 2023-01-24 11:12 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Tudor Ambarus, Takahiro.Kuwano, tkuw584924, linux-mtd, pratyush,
michael, miquel.raynal, richard, Bacem.Daassi, stable
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. */
--
Best regards,
Dhruva Gole
Texas Instruments Incorporated
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-24 11:12 ` Dhruva Gole
@ 2023-01-24 12:03 ` Pratyush Yadav
0 siblings, 0 replies; 14+ messages in thread
From: Pratyush Yadav @ 2023-01-24 12:03 UTC (permalink / raw)
To: Dhruva Gole
Cc: Tudor Ambarus, Takahiro.Kuwano, tkuw584924, linux-mtd, pratyush,
michael, miquel.raynal, richard, Bacem.Daassi, stable
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
>
--
Regards,
Pratyush Yadav
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
` (2 preceding siblings ...)
2023-01-23 14:37 ` Pratyush Yadav
@ 2023-01-31 9:00 ` Tudor Ambarus
3 siblings, 0 replies; 14+ messages in thread
From: Tudor Ambarus @ 2023-01-31 9:00 UTC (permalink / raw)
To: Takahiro.Kuwano, tkuw584924
Cc: linux-mtd, pratyush, michael, miquel.raynal, richard,
Bacem.Daassi, stable
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 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(-)
both applied, thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register
2023-01-23 16:31 ` Dhruva Gole
2023-01-24 10:43 ` Pratyush Yadav
@ 2023-02-01 7:14 ` Takahiro Kuwano
1 sibling, 0 replies; 14+ messages in thread
From: Takahiro Kuwano @ 2023-02-01 7:14 UTC (permalink / raw)
To: Dhruva Gole, Pratyush Yadav, Tudor Ambarus
Cc: Takahiro.Kuwano, linux-mtd, pratyush, michael, miquel.raynal,
richard, Bacem.Daassi, stable
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-02-01 7:14 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-06 3:06 [PATCH] mtd: spi-nor: spansion: Keep CFR5V[6] as 1 in Octal DTR enable/disable tkuw584924
2023-01-06 9:47 ` Tudor Ambarus
2023-01-06 9:55 ` Tudor Ambarus
2023-01-10 4:39 ` Takahiro Kuwano
2023-01-10 16:47 ` [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register Tudor Ambarus
2023-01-11 6:28 ` Dhruva Gole
2023-01-18 7:34 ` Takahiro Kuwano
2023-01-23 14:37 ` Pratyush Yadav
2023-01-23 16:31 ` Dhruva Gole
2023-01-24 10:43 ` Pratyush Yadav
2023-01-24 11:12 ` Dhruva Gole
2023-01-24 12:03 ` Pratyush Yadav
2023-02-01 7:14 ` Takahiro Kuwano
2023-01-31 9:00 ` Tudor Ambarus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).