| Message ID | 20200206183219.3756-2-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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 <linux@roeck-us.net> > --- > 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é <philmd@redhat.com>
On Thu, Feb 6, 2020 at 10:33 AM Guenter Roeck <linux@roeck-us.net> 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 <linux@roeck-us.net> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> 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 > >
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 <linux@roeck-us.net> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > 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; >
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 <linux@roeck-us.net> > --- > 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;
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 <linux@roeck-us.net> >> --- >> 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; >
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 <linux@roeck-us.net> >>> --- >>> 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 1e620000.spi: reading JEDEC ID 20:BA:19:10:00:00 [ 5.602226] aspeed-smc 1e620000.spi: n25q256a (32768 Kbytes) ... [ 6.174052] aspeed-smc 1e630000.spi: reading JEDEC ID C2:20:19:C2:20:19 [ 6.181682] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) AST2500 [ 1.641317] aspeed-smc 1e620000.spi: reading JEDEC ID EF:40:19:00:00:00 [ 1.648174] aspeed-smc 1e620000.spi: w25q256 (32768 Kbytes) ... [ 1.179214] aspeed-smc 1e630000.spi: reading JEDEC ID EF:40:19:00:00:00 [ 1.186737] aspeed-smc 1e630000.spi: w25q256 (32768 Kbytes) AST2600 [ 1.020255] aspeed-smc 1e620000.spi: reading JEDEC ID EF:40:20:00:00:00 [ 1.027830] aspeed-smc 1e620000.spi: w25q512jv (65536 Kbytes) ... [ 1.884171] aspeed-smc 1e630000.spi: reading JEDEC ID EF:40:19:00:00:00 [ 1.890937] aspeed-smc 1e630000.spi: w25q256 (32768 Kbytes) I think we need a special case for it. C.
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 <linux@roeck-us.net> >>>> --- >>>> 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 1e620000.spi: reading JEDEC ID 20:BA:19:10:00:00 > [ 5.602226] aspeed-smc 1e620000.spi: n25q256a (32768 Kbytes) > ... > [ 6.174052] aspeed-smc 1e630000.spi: reading JEDEC ID C2:20:19:C2:20:19 > [ 6.181682] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes) > > AST2500 > > [ 1.641317] aspeed-smc 1e620000.spi: reading JEDEC ID EF:40:19:00:00:00 > [ 1.648174] aspeed-smc 1e620000.spi: w25q256 (32768 Kbytes) > ... > [ 1.179214] aspeed-smc 1e630000.spi: reading JEDEC ID EF:40:19:00:00:00 > [ 1.186737] aspeed-smc 1e630000.spi: w25q256 (32768 Kbytes) > > AST2600 > > [ 1.020255] aspeed-smc 1e620000.spi: reading JEDEC ID EF:40:20:00:00:00 > [ 1.027830] aspeed-smc 1e620000.spi: w25q512jv (65536 Kbytes) > ... > [ 1.884171] aspeed-smc 1e630000.spi: reading JEDEC ID EF:40:19:00:00:00 > [ 1.890937] aspeed-smc 1e630000.spi: w25q256 (32768 Kbytes) FWIW QEMU models this one as 64KiB. > > > I think we need a special case for it. > > C. >
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;
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 <linux@roeck-us.net> --- v2: Split patch into two parts; improved decription hw/block/m25p80.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)