Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-07-22 Thread Philippe Mathieu-Daudé 
On 7/22/20 10:02 AM, Cédric Le Goater wrote:
> On 7/21/20 9:57 PM, Guenter Roeck wrote:
>> On 7/21/20 10:36 AM, Cédric Le Goater wrote:
>>> Hello,
>>>
>>> On 2/6/20 7:32 PM, Guenter Roeck wrote:
 When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
 always requests 6 bytes. The current implementation only returns three
 bytes, and interprets the remaining three bytes as new commands.
 While this does not matter most of the time, it is at the very least
 confusing. To avoid the problem, always report up to 6 bytes of JEDEC
 data. Fill remaining data with 0.

 Signed-off-by: Guenter Roeck 
 ---
 v2: Split patch into two parts; improved decription

  hw/block/m25p80.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
 index 5ff8d270c4..53bf63856f 100644
 --- a/hw/block/m25p80.c
 +++ b/hw/block/m25p80.c
 @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
  for (i = 0; i < s->pi->id_len; i++) {
  s->data[i] = s->pi->id[i];
  }
 +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
 +s->data[i] = 0;
 +}
>>>
>>> This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
>>> board : 
>>>
>>> https://www.supermicro.com/en/products/motherboard/X11SSL-F 
>>>
>>> which expects the flash ID to repeat. Erik solved the problem with the 
>>> patch 
>>> below and I think it makes sense to wrap around. Anyone knows what should 
>>> be 
>>> the expected behavior ? 
>>>
>>
>> No idea what the expected behavior is, but I am fine with the code below
>> if it works.
> 
> I checked on a few real systems and indeed the mx25l25635e behaves
> differently.
> 
> AST2400
> 
> [5.594176] aspeed-smc 1e62.spi: reading JEDEC ID 20:BA:19:10:00:00
> [5.602226] aspeed-smc 1e62.spi: n25q256a (32768 Kbytes)
> ...
> [6.174052] aspeed-smc 1e63.spi: reading JEDEC ID C2:20:19:C2:20:19
> [6.181682] aspeed-smc 1e63.spi: mx25l25635e (32768 Kbytes)
> 
> AST2500
> 
> [1.641317] aspeed-smc 1e62.spi: reading JEDEC ID EF:40:19:00:00:00
> [1.648174] aspeed-smc 1e62.spi: w25q256 (32768 Kbytes)
> ...
> [1.179214] aspeed-smc 1e63.spi: reading JEDEC ID EF:40:19:00:00:00
> [1.186737] aspeed-smc 1e63.spi: w25q256 (32768 Kbytes)
> 
> AST2600
> 
> [1.020255] aspeed-smc 1e62.spi: reading JEDEC ID EF:40:20:00:00:00
> [1.027830] aspeed-smc 1e62.spi: w25q512jv (65536 Kbytes)
> ...
> [1.884171] aspeed-smc 1e63.spi: reading JEDEC ID EF:40:19:00:00:00
> [1.890937] aspeed-smc 1e63.spi: w25q256 (32768 Kbytes)

FWIW QEMU models this one as 64KiB.

> 
> 
> I think we need a special case for it.
> 
> C. 
>

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-07-22 Thread Cédric Le Goater 
On 7/21/20 9:57 PM, Guenter Roeck wrote:
> On 7/21/20 10:36 AM, Cédric Le Goater wrote:
>> Hello,
>>
>> On 2/6/20 7:32 PM, Guenter Roeck wrote:
>>> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
>>> always requests 6 bytes. The current implementation only returns three
>>> bytes, and interprets the remaining three bytes as new commands.
>>> While this does not matter most of the time, it is at the very least
>>> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
>>> data. Fill remaining data with 0.
>>>
>>> Signed-off-by: Guenter Roeck 
>>> ---
>>> v2: Split patch into two parts; improved decription
>>>
>>>  hw/block/m25p80.c | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>>> index 5ff8d270c4..53bf63856f 100644
>>> --- a/hw/block/m25p80.c
>>> +++ b/hw/block/m25p80.c
>>> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>>  for (i = 0; i < s->pi->id_len; i++) {
>>>  s->data[i] = s->pi->id[i];
>>>  }
>>> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
>>> +s->data[i] = 0;
>>> +}
>>
>> This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
>> board : 
>>
>>  https://www.supermicro.com/en/products/motherboard/X11SSL-F 
>>
>> which expects the flash ID to repeat. Erik solved the problem with the patch 
>> below and I think it makes sense to wrap around. Anyone knows what should be 
>> the expected behavior ? 
>>
> 
> No idea what the expected behavior is, but I am fine with the code below
> if it works.

I checked on a few real systems and indeed the mx25l25635e behaves
differently.

AST2400

[5.594176] aspeed-smc 1e62.spi: reading JEDEC ID 20:BA:19:10:00:00
[5.602226] aspeed-smc 1e62.spi: n25q256a (32768 Kbytes)
...
[6.174052] aspeed-smc 1e63.spi: reading JEDEC ID C2:20:19:C2:20:19
[6.181682] aspeed-smc 1e63.spi: mx25l25635e (32768 Kbytes)

AST2500

[1.641317] aspeed-smc 1e62.spi: reading JEDEC ID EF:40:19:00:00:00
[1.648174] aspeed-smc 1e62.spi: w25q256 (32768 Kbytes)
...
[1.179214] aspeed-smc 1e63.spi: reading JEDEC ID EF:40:19:00:00:00
[1.186737] aspeed-smc 1e63.spi: w25q256 (32768 Kbytes)

AST2600

[1.020255] aspeed-smc 1e62.spi: reading JEDEC ID EF:40:20:00:00:00
[1.027830] aspeed-smc 1e62.spi: w25q512jv (65536 Kbytes)
...
[1.884171] aspeed-smc 1e63.spi: reading JEDEC ID EF:40:19:00:00:00
[1.890937] aspeed-smc 1e63.spi: w25q256 (32768 Kbytes)


I think we need a special case for it.

C.

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-07-21 Thread Guenter Roeck 
On 7/21/20 10:36 AM, Cédric Le Goater wrote:
> Hello,
> 
> On 2/6/20 7:32 PM, Guenter Roeck wrote:
>> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
>> always requests 6 bytes. The current implementation only returns three
>> bytes, and interprets the remaining three bytes as new commands.
>> While this does not matter most of the time, it is at the very least
>> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
>> data. Fill remaining data with 0.
>>
>> Signed-off-by: Guenter Roeck 
>> ---
>> v2: Split patch into two parts; improved decription
>>
>>  hw/block/m25p80.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 5ff8d270c4..53bf63856f 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  for (i = 0; i < s->pi->id_len; i++) {
>>  s->data[i] = s->pi->id[i];
>>  }
>> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
>> +s->data[i] = 0;
>> +}
> 
> This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
> board : 
> 
>   https://www.supermicro.com/en/products/motherboard/X11SSL-F 
> 
> which expects the flash ID to repeat. Erik solved the problem with the patch 
> below and I think it makes sense to wrap around. Anyone knows what should be 
> the expected behavior ? 
> 

No idea what the expected behavior is, but I am fine with the code below
if it works.

Thanks,
Guenter

> Thanks,
> 
> C. 
> 
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8227088441..5000930800 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  s->data[i] = s->pi->id[i];
>  }
>  for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -s->data[i] = 0;
> +s->data[i] = s->pi->id[i % s->pi->id_len];
>  }
> 
>  s->len = SPI_NOR_MAX_ID_LEN;
>

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-07-21 Thread Cédric Le Goater 
Hello,

On 2/6/20 7:32 PM, Guenter Roeck wrote:
> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
> always requests 6 bytes. The current implementation only returns three
> bytes, and interprets the remaining three bytes as new commands.
> While this does not matter most of the time, it is at the very least
> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
> data. Fill remaining data with 0.
> 
> Signed-off-by: Guenter Roeck 
> ---
> v2: Split patch into two parts; improved decription
> 
>  hw/block/m25p80.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 5ff8d270c4..53bf63856f 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  for (i = 0; i < s->pi->id_len; i++) {
>  s->data[i] = s->pi->id[i];
>  }
> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> +s->data[i] = 0;
> +}

This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
board : 

https://www.supermicro.com/en/products/motherboard/X11SSL-F 

which expects the flash ID to repeat. Erik solved the problem with the patch 
below and I think it makes sense to wrap around. Anyone knows what should be 
the expected behavior ? 

Thanks,

C. 


diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8227088441..5000930800 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 s->data[i] = s->pi->id[i];
 }
 for (; i < SPI_NOR_MAX_ID_LEN; i++) {
-s->data[i] = 0;
+s->data[i] = s->pi->id[i % s->pi->id_len];
 }

 s->len = SPI_NOR_MAX_ID_LEN;

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-02-06 Thread Cédric Le Goater 
On 2/6/20 7:32 PM, Guenter Roeck wrote:
> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
> always requests 6 bytes. The current implementation only returns three
> bytes, and interprets the remaining three bytes as new commands.
> While this does not matter most of the time, it is at the very least
> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
> data. Fill remaining data with 0.
> 
> Signed-off-by: Guenter Roeck 


Reviewed-by: Cédric Le Goater 

> ---
> v2: Split patch into two parts; improved decription
> 
>  hw/block/m25p80.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 5ff8d270c4..53bf63856f 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  for (i = 0; i < s->pi->id_len; i++) {
>  s->data[i] = s->pi->id[i];
>  }
> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> +s->data[i] = 0;
> +}
>  
> -s->len = s->pi->id_len;
> +s->len = SPI_NOR_MAX_ID_LEN;
>  s->pos = 0;
>  s->state = STATE_READING_DATA;
>  break;
>

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-02-06 Thread Alistair Francis 
On Thu, Feb 6, 2020 at 10:33 AM Guenter Roeck  wrote:
>
> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
> always requests 6 bytes. The current implementation only returns three
> bytes, and interprets the remaining three bytes as new commands.
> While this does not matter most of the time, it is at the very least
> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
> data. Fill remaining data with 0.
>
> Signed-off-by: Guenter Roeck 

Reviewed-by: Alistair Francis 

Alistair

> ---
> v2: Split patch into two parts; improved decription
>
>  hw/block/m25p80.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 5ff8d270c4..53bf63856f 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  for (i = 0; i < s->pi->id_len; i++) {
>  s->data[i] = s->pi->id[i];
>  }
> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> +s->data[i] = 0;
> +}
>
> -s->len = s->pi->id_len;
> +s->len = SPI_NOR_MAX_ID_LEN;
>  s->pos = 0;
>  s->state = STATE_READING_DATA;
>  break;
> --
> 2.17.1
>
>

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-02-06 Thread Philippe Mathieu-Daudé 

On 2/6/20 7:32 PM, Guenter Roeck wrote:

When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
always requests 6 bytes. The current implementation only returns three
bytes, and interprets the remaining three bytes as new commands.
While this does not matter most of the time, it is at the very least
confusing. To avoid the problem, always report up to 6 bytes of JEDEC
data. Fill remaining data with 0.

Signed-off-by: Guenter Roeck 
---
v2: Split patch into two parts; improved decription

  hw/block/m25p80.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 5ff8d270c4..53bf63856f 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
  for (i = 0; i < s->pi->id_len; i++) {
  s->data[i] = s->pi->id[i];
  }
+for (; i < SPI_NOR_MAX_ID_LEN; i++) {
+s->data[i] = 0;
+}
  
-s->len = s->pi->id_len;

+s->len = SPI_NOR_MAX_ID_LEN;
  s->pos = 0;
  s->state = STATE_READING_DATA;
  break;



Reviewed-by: Philippe Mathieu-Daudé

[PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-02-06 Thread Guenter Roeck 
When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
always requests 6 bytes. The current implementation only returns three
bytes, and interprets the remaining three bytes as new commands.
While this does not matter most of the time, it is at the very least
confusing. To avoid the problem, always report up to 6 bytes of JEDEC
data. Fill remaining data with 0.

Signed-off-by: Guenter Roeck 
---
v2: Split patch into two parts; improved decription

 hw/block/m25p80.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 5ff8d270c4..53bf63856f 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 for (i = 0; i < s->pi->id_len; i++) {
 s->data[i] = s->pi->id[i];
 }
+for (; i < SPI_NOR_MAX_ID_LEN; i++) {
+s->data[i] = 0;
+}
 
-s->len = s->pi->id_len;
+s->len = SPI_NOR_MAX_ID_LEN;
 s->pos = 0;
 s->state = STATE_READING_DATA;
 break;
-- 
2.17.1