[09/12] Support for 6Bytes jdec.

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
---
 hw/block/m25p80.c | 61 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 20 deletions(-)

You need to add more notes in commit messages about what you have
changed. Specifically here, you need to say something about the
arrayification of the Jedec ID. You are also correcting terminology by
changing ext_jedec to ext_id which shuld be mentioned.

On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>

Who is the Author? The patch authorship is Marcin, but the signature
is Pawel. The original author should be the author (the From:) as well
as have the Signed-off-by. Extra people (who resend, queue or commit
the patch) add their SOB. So if this is Pawel's code you need to do a
git commit --amend --author with Pawel's name. But either way, as
sender your own (Marcin) SoB should be here.

> ---
>  hw/block/m25p80.c | 61 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index a41c2f1..6fc55a3 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -50,12 +50,17 @@
>  /* 16 MiB max in 3 byte address mode */
>  #define MAX_3BYTES_SIZE 0x1000000
>
> +#define SPI_NOR_MAX_ID_LEN    6
> +
>  typedef struct FlashPartInfo {
>      const char *part_name;
> -    /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
> -    uint32_t jedec;
> -    /* extended jedec code */
> -    uint16_t ext_jedec;
> +    /*
> +     * This array stores the ID bytes.

Comment should be of form

/* some text wrapping
 * to multiple lines
 */

for consistency with surrounding code.

> +     * The first three bytes are the JEDIC ID.

"JEDEC"

> +     * JEDEC ID zero means "no ID" (mostly older chips).
> +     */
> +    uint8_t id[SPI_NOR_MAX_ID_LEN];
> +    uint8_t id_len;

This arrayification is a very nice change.

>      /* there is confusion between manufacturers as to what a sector is. In this
>       * device model, a "sector" is the size that is erased by the ERASE_SECTOR
>       * command (opcode 0xd8).
> @@ -67,11 +72,33 @@ typedef struct FlashPartInfo {
>  } FlashPartInfo;
>
>  /* adapted from linux */
> -

Keep this blank.

> -#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, _flags)\
> -    .part_name = (_part_name),\
> -    .jedec = (_jedec),\
> -    .ext_jedec = (_ext_jedec),\
> +/* Used when the "_ext_id" is two bytes at most */
> +#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
> +    .part_name = _part_name,\
> +    .id = {\
> +        ((_jedec_id) >> 16) & 0xff,\
> +        ((_jedec_id) >> 8) & 0xff,\
> +        (_jedec_id) & 0xff,\
> +        ((_ext_id) >> 8) & 0xff,\
> +        (_ext_id) & 0xff,\
> +          },\
> +    .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
> +    .sector_size = (_sector_size),\
> +    .n_sectors = (_n_sectors),\
> +    .page_size = 256,\
> +    .flags = (_flags),
> +
> +#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\

I am not sure we need the extra macro. Can you just make the 6th byte
an extra _flags flag and use the original INFO()? If you do need the
extra macro, one macro should call the other, or both should call a a
common macro (accepting _id_len as a macro arg) to avoid the dup of
the majority of the macro body.

Regards,
Peter

> +    .part_name = _part_name,\
> +    .id = {\
> +        ((_jedec_id) >> 16) & 0xff,\
> +        ((_jedec_id) >> 8) & 0xff,\
> +        (_jedec_id) & 0xff,\
> +        ((_ext_id) >> 16) & 0xff,\
> +        ((_ext_id) >> 8) & 0xff,\
> +        (_ext_id) & 0xff,\
> +          },\
> +    .id_len = 6,\
>      .sector_size = (_sector_size),\
>      .n_sectors = (_n_sectors),\
>      .page_size = 256,\
> @@ -519,7 +546,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>
>      case DIOR:
> -        switch ((s->pi->jedec >> 16) & 0xFF) {
> +        switch (s->pi->id[0]) {
>          case JEDEC_WINBOND:
>          case JEDEC_SPANSION:
>              s->needed_bytes = 4;
> @@ -534,7 +561,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>
>      case QIOR:
> -        switch ((s->pi->jedec >> 16) & 0xFF) {
> +        switch (s->pi->id[0]) {
>          case JEDEC_WINBOND:
>          case JEDEC_SPANSION:
>              s->needed_bytes = 6;
> @@ -573,16 +600,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>
>      case JEDEC_READ:
>          DB_PRINT_L(0, "populated jedec code\n");
> -        s->data[0] = (s->pi->jedec >> 16) & 0xff;
> -        s->data[1] = (s->pi->jedec >> 8) & 0xff;
> -        s->data[2] = s->pi->jedec & 0xff;
> -        if (s->pi->ext_jedec) {
> -            s->data[3] = (s->pi->ext_jedec >> 8) & 0xff;
> -            s->data[4] = s->pi->ext_jedec & 0xff;
> -            s->len = 5;
> -        } else {
> -            s->len = 3;
> +        for (i = 0; i < s->pi->id_len; i++) {
> +            s->data[i] = s->pi->id[i];
>          }
> +        s->len = s->pi->id_len;
>          s->pos = 0;
>          s->state = STATE_READING_DATA;
>          break;
> --
> 2.5.0
>
>

W dniu 21.12.2015 o 11:47, Peter Crosthwaite pisze:
> You need to add more notes in commit messages about what you have
> changed. Specifically here, you need to say something about the
> arrayification of the Jedec ID. You are also correcting terminology by
> changing ext_jedec to ext_id which shuld be mentioned.
>
> On Wed, Dec 16, 2015 at 4:57 AM,  <marcin.krzeminski@nokia.com> wrote:
>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>
>> Signed-off-by: Pawel Lenkow <pawel.lenkow@nokia.com>
>
> Who is the Author? The patch authorship is Marcin, but the signature
> is Pawel. The original author should be the author (the From:) as well
> as have the Signed-off-by. Extra people (who resend, queue or commit
> the patch) add their SOB. So if this is Pawel's code you need to do a
> git commit --amend --author with Pawel's name. But either way, as
> sender your own (Marcin) SoB should be here.
My understanding was that SOB is enough to identify an author.
In all this patches whee Pawel is SOB, he is also the author.
I will correct it.
Thanks,
Marcin
>
>
>> ---
>>  hw/block/m25p80.c | 61 +++++++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index a41c2f1..6fc55a3 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -50,12 +50,17 @@
>>  /* 16 MiB max in 3 byte address mode */
>>  #define MAX_3BYTES_SIZE 0x1000000
>>
>> +#define SPI_NOR_MAX_ID_LEN    6
>> +
>>  typedef struct FlashPartInfo {
>>      const char *part_name;
>> -    /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
>> -    uint32_t jedec;
>> -    /* extended jedec code */
>> -    uint16_t ext_jedec;
>> +    /*
>> +     * This array stores the ID bytes.
>
> Comment should be of form
>
> /* some text wrapping
>  * to multiple lines
>  */
>
> for consistency with surrounding code.
>
>> +     * The first three bytes are the JEDIC ID.
>
> "JEDEC"
>
>> +     * JEDEC ID zero means "no ID" (mostly older chips).
>> +     */
>> +    uint8_t id[SPI_NOR_MAX_ID_LEN];
>> +    uint8_t id_len;
>
> This arrayification is a very nice change.
>
>>      /* there is confusion between manufacturers as to what a sector is. In this
>>       * device model, a "sector" is the size that is erased by the ERASE_SECTOR
>>       * command (opcode 0xd8).
>> @@ -67,11 +72,33 @@ typedef struct FlashPartInfo {
>>  } FlashPartInfo;
>>
>>  /* adapted from linux */
>> -
>
> Keep this blank.
>
>> -#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, _flags)\
>> -    .part_name = (_part_name),\
>> -    .jedec = (_jedec),\
>> -    .ext_jedec = (_ext_jedec),\
>> +/* Used when the "_ext_id" is two bytes at most */
>> +#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
>> +    .part_name = _part_name,\
>> +    .id = {\
>> +        ((_jedec_id) >> 16) & 0xff,\
>> +        ((_jedec_id) >> 8) & 0xff,\
>> +        (_jedec_id) & 0xff,\
>> +        ((_ext_id) >> 8) & 0xff,\
>> +        (_ext_id) & 0xff,\
>> +          },\
>> +    .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
>> +    .sector_size = (_sector_size),\
>> +    .n_sectors = (_n_sectors),\
>> +    .page_size = 256,\
>> +    .flags = (_flags),
>> +
>> +#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, _flags)\
>
> I am not sure we need the extra macro. Can you just make the 6th byte
> an extra _flags flag and use the original INFO()? If you do need the
> extra macro, one macro should call the other, or both should call a a
> common macro (accepting _id_len as a macro arg) to avoid the dup of
> the majority of the macro body.
>
> Regards,
> Peter
>
>> +    .part_name = _part_name,\
>> +    .id = {\
>> +        ((_jedec_id) >> 16) & 0xff,\
>> +        ((_jedec_id) >> 8) & 0xff,\
>> +        (_jedec_id) & 0xff,\
>> +        ((_ext_id) >> 16) & 0xff,\
>> +        ((_ext_id) >> 8) & 0xff,\
>> +        (_ext_id) & 0xff,\
>> +          },\
>> +    .id_len = 6,\
>>      .sector_size = (_sector_size),\
>>      .n_sectors = (_n_sectors),\
>>      .page_size = 256,\
>> @@ -519,7 +546,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>
>>      case DIOR:
>> -        switch ((s->pi->jedec >> 16) & 0xFF) {
>> +        switch (s->pi->id[0]) {
>>          case JEDEC_WINBOND:
>>          case JEDEC_SPANSION:
>>              s->needed_bytes = 4;
>> @@ -534,7 +561,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>
>>      case QIOR:
>> -        switch ((s->pi->jedec >> 16) & 0xFF) {
>> +        switch (s->pi->id[0]) {
>>          case JEDEC_WINBOND:
>>          case JEDEC_SPANSION:
>>              s->needed_bytes = 6;
>> @@ -573,16 +600,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>
>>      case JEDEC_READ:
>>          DB_PRINT_L(0, "populated jedec code\n");
>> -        s->data[0] = (s->pi->jedec >> 16) & 0xff;
>> -        s->data[1] = (s->pi->jedec >> 8) & 0xff;
>> -        s->data[2] = s->pi->jedec & 0xff;
>> -        if (s->pi->ext_jedec) {
>> -            s->data[3] = (s->pi->ext_jedec >> 8) & 0xff;
>> -            s->data[4] = s->pi->ext_jedec & 0xff;
>> -            s->len = 5;
>> -        } else {
>> -            s->len = 3;
>> +        for (i = 0; i < s->pi->id_len; i++) {
>> +            s->data[i] = s->pi->id[i];
>>          }
>> +        s->len = s->pi->id_len;
>>          s->pos = 0;
>>          s->state = STATE_READING_DATA;
>>          break;
>> --
>> 2.5.0
>>
>>
>
>