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

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

[tkuw584924]

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

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

[Tudor Ambarus]

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

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

[Tudor Ambarus]

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.

Re: [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@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

[PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Tudor Ambarus]

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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Dhruva Gole]

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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Takahiro Kuwano]

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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Pratyush Yadav]

+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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Dhruva Gole]

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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Pratyush Yadav]

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

Re: Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Dhruva Gole]

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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Pratyush Yadav]

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

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Tudor Ambarus]

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!

Re: [PATCH 1/2] mtd: spi-nor: spansion: Consider reserved bits in CFR5 register

[Takahiro Kuwano]

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