diff mbox series

[v5,02/13] m25p80: Add support for SST READ ID 0x90/0xAB commands

Message ID 20171029101343.15544-3-frasse.iglesias@gmail.com
State New
Headers show
Series Add support for the ZynqMP Generic QSPI | expand

Commit Message

Francisco Iglesias Oct. 29, 2017, 10:13 a.m. UTC 
Add support for SST READ ID 0x90/0xAB commands for reading out the flash
manufacuter ID and device ID.

Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/block/m25p80.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

mar.krzeminski Oct. 29, 2017, 3:21 p.m. UTC | #1 
W dniu 29.10.2017 o 11:13, Francisco Iglesias pisze:
> Add support for SST READ ID 0x90/0xAB commands for reading out the flash
> manufacuter ID and device ID.
>
> Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Acked-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>   hw/block/m25p80.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 2971519..7a5c137 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -355,6 +355,8 @@ typedef enum {
>       DPP = 0xa2,
>       QPP = 0x32,
>       QPP_4 = 0x34,
> +    RDID_90 = 0x90,
> +    RDID_AB = 0xab,
>   
>       ERASE_4K = 0x20,
>       ERASE4_4K = 0x21,
> @@ -405,6 +407,7 @@ typedef enum {
>       MAN_MACRONIX,
>       MAN_NUMONYX,
>       MAN_WINBOND,
> +    MAN_SST,
>       MAN_GENERIC,
>   } Manufacturer;
>   
> @@ -476,6 +479,8 @@ static inline Manufacturer get_man(Flash *s)
>           return MAN_SPANSION;
>       case 0xC2:
>           return MAN_MACRONIX;
> +    case 0xBF:
> +        return MAN_SST;
>       default:
>           return MAN_GENERIC;
>       }
> @@ -711,6 +716,22 @@ static void complete_collecting_data(Flash *s)
>       case WEVCR:
>           s->enh_volatile_cfg = s->data[0];
>           break;
> +    case RDID_90:
> +    case RDID_AB:
> +        if (get_man(s) == MAN_SST && s->cur_addr <= 1) {
> +            if (s->cur_addr) {
> +                s->data[0] = s->pi->id[2];
> +                s->data[1] = s->pi->id[0];
> +            } else {
> +                s->data[0] = s->pi->id[0];
> +                s->data[1] = s->pi->id[2];
> +            }
> +            s->pos = 0;
> +            s->len = 2;
> +            s->data_read_loop = true;
> +            s->state = STATE_READING_DATA;
Do you know how the real HW will behave?
When you get two bytes, it will send them once again or will wait for 
address?
> +        }
> +        break;
>       default:
>           break;
>       }
> @@ -926,6 +947,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>       case PP4:
>       case PP4_4:
>       case DIE_ERASE:
> +    case RDID_90:
> +    case RDID_AB:
>           s->needed_bytes = get_addr_length(s);
If I understand correctly, for above commands allowed address length is 
3 bytes,
but get_add_length can return 4. To avoid strange error I suggest to add 
case entry for them
in get_addr_length.

>           s->pos = 0;
>           s->len = 0;
Regards,
Marcin
Francisco Iglesias Oct. 29, 2017, 9:13 p.m. UTC | #2 
On 29 October 2017 at 16:21, mar.krzeminski <mar.krzeminski@gmail.com>
wrote:

>
>
> W dniu 29.10.2017 o 11:13, Francisco Iglesias pisze:
>
> Add support for SST READ ID 0x90/0xAB commands for reading out the flash
>> manufacuter ID and device ID.
>>
>> Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>> Acked-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>   hw/block/m25p80.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 2971519..7a5c137 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -355,6 +355,8 @@ typedef enum {
>>       DPP = 0xa2,
>>       QPP = 0x32,
>>       QPP_4 = 0x34,
>> +    RDID_90 = 0x90,
>> +    RDID_AB = 0xab,
>>         ERASE_4K = 0x20,
>>       ERASE4_4K = 0x21,
>> @@ -405,6 +407,7 @@ typedef enum {
>>       MAN_MACRONIX,
>>       MAN_NUMONYX,
>>       MAN_WINBOND,
>> +    MAN_SST,
>>       MAN_GENERIC,
>>   } Manufacturer;
>>   @@ -476,6 +479,8 @@ static inline Manufacturer get_man(Flash *s)
>>           return MAN_SPANSION;
>>       case 0xC2:
>>           return MAN_MACRONIX;
>> +    case 0xBF:
>> +        return MAN_SST;
>>       default:
>>           return MAN_GENERIC;
>>       }
>> @@ -711,6 +716,22 @@ static void complete_collecting_data(Flash *s)
>>       case WEVCR:
>>           s->enh_volatile_cfg = s->data[0];
>>           break;
>> +    case RDID_90:
>> +    case RDID_AB:
>> +        if (get_man(s) == MAN_SST && s->cur_addr <= 1) {
>> +            if (s->cur_addr) {
>> +                s->data[0] = s->pi->id[2];
>> +                s->data[1] = s->pi->id[0];
>> +            } else {
>> +                s->data[0] = s->pi->id[0];
>> +                s->data[1] = s->pi->id[2];
>> +            }
>> +            s->pos = 0;
>> +            s->len = 2;
>> +            s->data_read_loop = true;
>> +            s->state = STATE_READING_DATA;
>>
> Do you know how the real HW will behave?
> When you get two bytes, it will send them once again or will wait for
> address?
>

Dear Marcin,

First of all thank you vey much for reviewing again! About your question
above, I have not tested it on HW but the SST flash datasheets for the
supported flashes in m25p80 state that "The manufacturer's and device ID
output stream is continuous until terminated by a low to high transition on
CE#.". Also in the diagram for the command in the datasheets it can be seen
that no address is needed (two continuous man/dev ids are read). This is
the reason to why data_read_loop is set to true above. Do you find this ok?



> +        }
>> +        break;
>>       default:
>>           break;
>>       }
>> @@ -926,6 +947,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>       case PP4:
>>       case PP4_4:
>>       case DIE_ERASE:
>> +    case RDID_90:
>> +    case RDID_AB:
>>           s->needed_bytes = get_addr_length(s);
>>
> If I understand correctly, for above commands allowed address length is 3
> bytes,
> but get_add_length can return 4. To avoid strange error I suggest to add
> case entry for them
> in get_addr_length.
>

About your suggestion above, I actually thought of this while creating the
patch but since I could not find support for 4 byte addresses in any of the
SST datasheets (and then no support for EN_4BYTE_ADDR (0xB7)), I could only
see get_addr_length to return 3 for these SST commands (and then the extra
cases shouldn't be needed), which is one of the reasons to why I decided to
add the commands to the above switch case. Do you find it ok to keep the
code as it is or would you like me to add the cases?

Best regards,
Francisco Iglesias


>
>           s->pos = 0;
>>           s->len = 0;
>>
> Regards,
> Marcin
>
>
mar.krzeminski Oct. 31, 2017, 10:11 a.m. UTC | #3 
Hi Fancisco,

W dniu 29.10.2017 o 22:13, francisco iglesias pisze:
>
>
> On 29 October 2017 at 16:21, mar.krzeminski <mar.krzeminski@gmail.com 
> <mailto:mar.krzeminski@gmail.com>> wrote:
>
>
>
>     W dniu 29.10.2017 o 11:13, Francisco Iglesias pisze:
>
>         Add support for SST READ ID 0x90/0xAB commands for reading out
>         the flash
>         manufacuter ID and device ID.
>
>         Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com
>         <mailto:frasse.iglesias@gmail.com>>
>         Acked-by: Alistair Francis <alistair.francis@xilinx.com
>         <mailto:alistair.francis@xilinx.com>>
>         ---
>           hw/block/m25p80.c | 23 +++++++++++++++++++++++
>           1 file changed, 23 insertions(+)
>
>         diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>         index 2971519..7a5c137 100644
>         --- a/hw/block/m25p80.c
>         +++ b/hw/block/m25p80.c
>         @@ -355,6 +355,8 @@ typedef enum {
>               DPP = 0xa2,
>               QPP = 0x32,
>               QPP_4 = 0x34,
>         +    RDID_90 = 0x90,
>         +    RDID_AB = 0xab,
>                 ERASE_4K = 0x20,
>               ERASE4_4K = 0x21,
>         @@ -405,6 +407,7 @@ typedef enum {
>               MAN_MACRONIX,
>               MAN_NUMONYX,
>               MAN_WINBOND,
>         +    MAN_SST,
>               MAN_GENERIC,
>           } Manufacturer;
>           @@ -476,6 +479,8 @@ static inline Manufacturer get_man(Flash *s)
>                   return MAN_SPANSION;
>               case 0xC2:
>                   return MAN_MACRONIX;
>         +    case 0xBF:
>         +        return MAN_SST;
>               default:
>                   return MAN_GENERIC;
>               }
>         @@ -711,6 +716,22 @@ static void
>         complete_collecting_data(Flash *s)
>               case WEVCR:
>                   s->enh_volatile_cfg = s->data[0];
>                   break;
>         +    case RDID_90:
>         +    case RDID_AB:
>         +        if (get_man(s) == MAN_SST && s->cur_addr <= 1) {
>         +            if (s->cur_addr) {
>         +                s->data[0] = s->pi->id[2];
>         +                s->data[1] = s->pi->id[0];
>         +            } else {
>         +                s->data[0] = s->pi->id[0];
>         +                s->data[1] = s->pi->id[2];
>         +            }
>         +            s->pos = 0;
>         +            s->len = 2;
>         +            s->data_read_loop = true;
>         +            s->state = STATE_READING_DATA;
>
>     Do you know how the real HW will behave?
>     When you get two bytes, it will send them once again or will wait
>     for address?
>
>
> Dear Marcin,
>
> First of all thank you vey much for reviewing again! About your 
> question above, I have not tested it on HW but the SST flash 
> datasheets for the supported flashes in m25p80 state that "The 
> manufacturer's and device ID output stream is continuous until 
> terminated by a low to high transition on CE#.". Also in the diagram 
> for the command in the datasheets it can be seen that no address is 
> needed (two continuous man/dev ids are read). This is the reason to 
> why data_read_loop is set to true above. Do you find this ok?
I asked about HW because t I have never used such feature, and NOR flash 
datasheets are specific.
Since we are basing on datasheet only, it looks good for me.
>
>         +        }
>         +        break;
>               default:
>                   break;
>               }
>         @@ -926,6 +947,8 @@ static void decode_new_cmd(Flash *s,
>         uint32_t value)
>               case PP4:
>               case PP4_4:
>               case DIE_ERASE:
>         +    case RDID_90:
>         +    case RDID_AB:
>                   s->needed_bytes = get_addr_length(s);
>
>     If I understand correctly, for above commands allowed address
>     length is 3 bytes,
>     but get_add_length can return 4. To avoid strange error I suggest
>     to add case entry for them
>     in get_addr_length.
>
>
> About your suggestion above, I actually thought of this while creating 
> the patch but since I could not find support for 4 byte addresses in 
> any of the SST datasheets (and then no support for EN_4BYTE_ADDR 
> (0xB7)), I could only see get_addr_length to return 3 for these SST 
> commands (and then the extra cases shouldn't be needed), which is one 
> of the reasons to why I decided to add the commands to the above 
> switch case. Do you find it ok to keep the code as it is or would you 
> like me to add the cases?
If you add cases then code will be more readable, so smaller chance to 
introduce regression in the feature.
Since it is working as it should now choice is yours. If you decide to 
not change:
Acked-by: Marcin Krzemiński <mar.krzeminski@gmail.com>

>
> Best regards,
> Francisco Iglesias
>
>
>                   s->pos = 0;
>                   s->len = 0;
>
>     Regards,
>     Marcin
>
>
Thanks,
Marcin
diff mbox series

Patch

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 2971519..7a5c137 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -355,6 +355,8 @@  typedef enum {
     DPP = 0xa2,
     QPP = 0x32,
     QPP_4 = 0x34,
+    RDID_90 = 0x90,
+    RDID_AB = 0xab,
 
     ERASE_4K = 0x20,
     ERASE4_4K = 0x21,
@@ -405,6 +407,7 @@  typedef enum {
     MAN_MACRONIX,
     MAN_NUMONYX,
     MAN_WINBOND,
+    MAN_SST,
     MAN_GENERIC,
 } Manufacturer;
 
@@ -476,6 +479,8 @@  static inline Manufacturer get_man(Flash *s)
         return MAN_SPANSION;
     case 0xC2:
         return MAN_MACRONIX;
+    case 0xBF:
+        return MAN_SST;
     default:
         return MAN_GENERIC;
     }
@@ -711,6 +716,22 @@  static void complete_collecting_data(Flash *s)
     case WEVCR:
         s->enh_volatile_cfg = s->data[0];
         break;
+    case RDID_90:
+    case RDID_AB:
+        if (get_man(s) == MAN_SST && s->cur_addr <= 1) {
+            if (s->cur_addr) {
+                s->data[0] = s->pi->id[2];
+                s->data[1] = s->pi->id[0];
+            } else {
+                s->data[0] = s->pi->id[0];
+                s->data[1] = s->pi->id[2];
+            }
+            s->pos = 0;
+            s->len = 2;
+            s->data_read_loop = true;
+            s->state = STATE_READING_DATA;
+        }
+        break;
     default:
         break;
     }
@@ -926,6 +947,8 @@  static void decode_new_cmd(Flash *s, uint32_t value)
     case PP4:
     case PP4_4:
     case DIE_ERASE:
+    case RDID_90:
+    case RDID_AB:
         s->needed_bytes = get_addr_length(s);
         s->pos = 0;
         s->len = 0;