JEDEC STANDARD JESD216 for Serial Flash Discovery Parameters (SFDP)
provides a mean to describe the features of a serial flash device
using a set of internal parameter tables.

This is the initial framework for the RDSFDP command giving access to
a private SFDP area under the flash. This area now needs to be
populated with the flash device characteristics, using a new 'sfdp'
pointer under FlashPartInfo.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80_sfdp.h | 15 +++++++++++++++
 hw/block/m25p80.c      | 33 +++++++++++++++++++++++++++++++++
 hw/block/trace-events  |  1 +
 3 files changed, 49 insertions(+)
 create mode 100644 hw/block/m25p80_sfdp.h

diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
new file mode 100644
index 000000000000..b75fd0b0c13f
--- /dev/null
+++ b/hw/block/m25p80_sfdp.h
@@ -0,0 +1,15 @@
+/*
+ * M25P80 SFDP
+ *
+ * Copyright (c) 2020, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef HW_M25P80_SFDP_H
+#define HW_M25P80_SFDP_H
+
+#define M25P80_SFDP_AREA_SIZE 0x100
+
+#endif
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 82270884416e..32925589ec7a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -33,6 +33,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "trace.h"
+#include "m25p80_sfdp.h"
 
 /* Fields for FlashPartInfo->flags */
 
@@ -72,6 +73,7 @@ typedef struct FlashPartInfo {
      * This field inform how many die is in the chip.
      */
     uint8_t die_cnt;
+    const uint8_t *sfdp;
 } FlashPartInfo;
 
 /* adapted from linux */
@@ -333,6 +335,7 @@ typedef enum {
     BULK_ERASE = 0xc7,
     READ_FSR = 0x70,
     RDCR = 0x15,
+    RDSFDP = 0x5a,
 
     READ = 0x03,
     READ4 = 0x13,
@@ -398,6 +401,7 @@ typedef enum {
     STATE_COLLECTING_DATA,
     STATE_COLLECTING_VAR_LEN_DATA,
     STATE_READING_DATA,
+    STATE_READING_SFDP,
 } CMDState;
 
 typedef enum {
@@ -619,6 +623,8 @@ static inline int get_addr_length(Flash *s)
     }
 
    switch (s->cmd_in_progress) {
+   case RDSFDP:
+       return 3;
    case PP4:
    case PP4_4:
    case QPP_4:
@@ -744,6 +750,17 @@ static void complete_collecting_data(Flash *s)
                           " by device\n");
         }
         break;
+
+    case RDSFDP:
+        if (s->cur_addr < M25P80_SFDP_AREA_SIZE) {
+            s->state = STATE_READING_SFDP;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "M25P80: Invalid SFDP address %#" PRIx32 "\n",
+                          s->cur_addr);
+        }
+        break;
+
     default:
         break;
     }
@@ -1160,6 +1177,16 @@ static void decode_new_cmd(Flash *s, uint32_t value)
     case RSTQIO:
         s->quad_enable = false;
         break;
+    case RDSFDP:
+        if (s->pi->sfdp) {
+            s->needed_bytes = get_addr_length(s) + 1 ; /* SFDP addr + dummy */
+            s->pos = 0;
+            s->len = 0;
+            s->state = STATE_COLLECTING_DATA;
+            break;
+        }
+        /* Fallthrough */
+
     default:
         s->pos = 0;
         s->len = 1;
@@ -1256,6 +1283,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
             }
         }
         break;
+    case STATE_READING_SFDP:
+        assert(s->pi->sfdp);
+        r = s->pi->sfdp[s->cur_addr];
+        trace_m25p80_read_sfdp(s, s->cur_addr, (uint8_t)r);
+        s->cur_addr = (s->cur_addr + 1) & (M25P80_SFDP_AREA_SIZE - 1);
+        break;
 
     default:
     case STATE_IDLE:
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 958fcc5508d1..53d377ca2b46 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -152,5 +152,6 @@ m25p80_page_program(void *s, uint32_t addr, uint8_t tx) "[%p] page program cur_a
 m25p80_transfer(void *s, uint8_t state, uint32_t len, uint8_t needed, uint32_t pos, uint32_t cur_addr, uint8_t t) "[%p] Transfer state 0x%"PRIx8" len 0x%"PRIx32" needed 0x%"PRIx8" pos 0x%"PRIx32" addr 0x%"PRIx32" tx 0x%"PRIx8
 m25p80_read_byte(void *s, uint32_t addr, uint8_t v) "[%p] Read byte 0x%"PRIx32"=0x%"PRIx8
 m25p80_read_data(void *s, uint32_t pos, uint8_t v) "[%p] Read data 0x%"PRIx32"=0x%"PRIx8
+m25p80_read_sfdp(void *s, uint32_t addr, uint8_t v) "[%p] Read SFDP 0x%"PRIx32"=0x%"PRIx8
 m25p80_binding(void *s) "[%p] Binding to IF_MTD drive"
 m25p80_binding_no_bdrv(void *s) "[%p] No BDRV - binding to RAM"
-- 
2.25.4

Hi Cedric,

On [2020 Aug 27] Thu 11:11:29, Cédric Le Goater wrote:
> JEDEC STANDARD JESD216 for Serial Flash Discovery Parameters (SFDP)
> provides a mean to describe the features of a serial flash device
> using a set of internal parameter tables.
> 
> This is the initial framework for the RDSFDP command giving access to
> a private SFDP area under the flash. This area now needs to be
> populated with the flash device characteristics, using a new 'sfdp'
> pointer under FlashPartInfo.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/block/m25p80_sfdp.h | 15 +++++++++++++++
>  hw/block/m25p80.c      | 33 +++++++++++++++++++++++++++++++++
>  hw/block/trace-events  |  1 +
>  3 files changed, 49 insertions(+)
>  create mode 100644 hw/block/m25p80_sfdp.h
> 
> diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
> new file mode 100644
> index 000000000000..b75fd0b0c13f
> --- /dev/null
> +++ b/hw/block/m25p80_sfdp.h
> @@ -0,0 +1,15 @@
> +/*
> + * M25P80 SFDP
> + *
> + * Copyright (c) 2020, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_M25P80_SFDP_H
> +#define HW_M25P80_SFDP_H
> +
> +#define M25P80_SFDP_AREA_SIZE 0x100
> +
> +#endif
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 82270884416e..32925589ec7a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -33,6 +33,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "trace.h"
> +#include "m25p80_sfdp.h"
>  
>  /* Fields for FlashPartInfo->flags */
>  
> @@ -72,6 +73,7 @@ typedef struct FlashPartInfo {
>       * This field inform how many die is in the chip.
>       */
>      uint8_t die_cnt;
> +    const uint8_t *sfdp;
>  } FlashPartInfo;
>  
>  /* adapted from linux */
> @@ -333,6 +335,7 @@ typedef enum {
>      BULK_ERASE = 0xc7,
>      READ_FSR = 0x70,
>      RDCR = 0x15,
> +    RDSFDP = 0x5a,
>  
>      READ = 0x03,
>      READ4 = 0x13,
> @@ -398,6 +401,7 @@ typedef enum {
>      STATE_COLLECTING_DATA,
>      STATE_COLLECTING_VAR_LEN_DATA,
>      STATE_READING_DATA,
> +    STATE_READING_SFDP,
>  } CMDState;
>  
>  typedef enum {
> @@ -619,6 +623,8 @@ static inline int get_addr_length(Flash *s)
>      }
>  
>     switch (s->cmd_in_progress) {
> +   case RDSFDP:
> +       return 3;
>     case PP4:
>     case PP4_4:
>     case QPP_4:
> @@ -744,6 +750,17 @@ static void complete_collecting_data(Flash *s)
>                            " by device\n");
>          }
>          break;
> +
> +    case RDSFDP:
> +        if (s->cur_addr < M25P80_SFDP_AREA_SIZE) {

Is perhaps M25P80_SFDP_AREA_SIZE a limit for the micron flashes in patch
2 (and not sfdp)? An option might be to change .sfdp to a
'.sfdp_read' function decoding the address internally (for the
micron flashes it could just return the array value in case the address is
less than M25P80_SFDP_AREA_SIZE or 0xFF else). It might become easier
to add flashes containing parameter tables in higher addresses (the
function could then decode the address into a specific table/area).

> +            s->state = STATE_READING_SFDP;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "M25P80: Invalid SFDP address %#" PRIx32 "\n",
> +                          s->cur_addr);
> +        }
> +        break;
> +
>      default:
>          break;
>      }
> @@ -1160,6 +1177,16 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case RSTQIO:
>          s->quad_enable = false;
>          break;
> +    case RDSFDP:
> +        if (s->pi->sfdp) {
> +            s->needed_bytes = get_addr_length(s) + 1 ; /* SFDP addr + dummy */

Should above be changed to:

s->needed_bytes = get_addr_length(s) + 8;  /* SFDP addr + dummy */

(I think it might fail else when the flashes are operating in 2 lines and
4 lines and are generating 8 dummy cycles for the RDSFDP)

Best regards,
Francisco Iglesias

> +            s->pos = 0;
> +            s->len = 0;
> +            s->state = STATE_COLLECTING_DATA;
> +            break;
> +        }
> +        /* Fallthrough */
> +
>      default:
>          s->pos = 0;
>          s->len = 1;
> @@ -1256,6 +1283,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>              }
>          }
>          break;
> +    case STATE_READING_SFDP:
> +        assert(s->pi->sfdp);
> +        r = s->pi->sfdp[s->cur_addr];
> +        trace_m25p80_read_sfdp(s, s->cur_addr, (uint8_t)r);
> +        s->cur_addr = (s->cur_addr + 1) & (M25P80_SFDP_AREA_SIZE - 1);
> +        break;
>  
>      default:
>      case STATE_IDLE:
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 958fcc5508d1..53d377ca2b46 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -152,5 +152,6 @@ m25p80_page_program(void *s, uint32_t addr, uint8_t tx) "[%p] page program cur_a
>  m25p80_transfer(void *s, uint8_t state, uint32_t len, uint8_t needed, uint32_t pos, uint32_t cur_addr, uint8_t t) "[%p] Transfer state 0x%"PRIx8" len 0x%"PRIx32" needed 0x%"PRIx8" pos 0x%"PRIx32" addr 0x%"PRIx32" tx 0x%"PRIx8
>  m25p80_read_byte(void *s, uint32_t addr, uint8_t v) "[%p] Read byte 0x%"PRIx32"=0x%"PRIx8
>  m25p80_read_data(void *s, uint32_t pos, uint8_t v) "[%p] Read data 0x%"PRIx32"=0x%"PRIx8
> +m25p80_read_sfdp(void *s, uint32_t addr, uint8_t v) "[%p] Read SFDP 0x%"PRIx32"=0x%"PRIx8
>  m25p80_binding(void *s) "[%p] Binding to IF_MTD drive"
>  m25p80_binding_no_bdrv(void *s) "[%p] No BDRV - binding to RAM"
> -- 
> 2.25.4
> 

On 9/1/20 3:36 PM, Francisco Iglesias wrote:
> Hi Cedric,
> 
> On [2020 Aug 27] Thu 11:11:29, Cédric Le Goater wrote:
>> JEDEC STANDARD JESD216 for Serial Flash Discovery Parameters (SFDP)
>> provides a mean to describe the features of a serial flash device
>> using a set of internal parameter tables.
>>
>> This is the initial framework for the RDSFDP command giving access to
>> a private SFDP area under the flash. This area now needs to be
>> populated with the flash device characteristics, using a new 'sfdp'
>> pointer under FlashPartInfo.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/block/m25p80_sfdp.h | 15 +++++++++++++++
>>  hw/block/m25p80.c      | 33 +++++++++++++++++++++++++++++++++
>>  hw/block/trace-events  |  1 +
>>  3 files changed, 49 insertions(+)
>>  create mode 100644 hw/block/m25p80_sfdp.h
>>
>> diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
>> new file mode 100644
>> index 000000000000..b75fd0b0c13f
>> --- /dev/null
>> +++ b/hw/block/m25p80_sfdp.h
>> @@ -0,0 +1,15 @@
>> +/*
>> + * M25P80 SFDP
>> + *
>> + * Copyright (c) 2020, IBM Corporation.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef HW_M25P80_SFDP_H
>> +#define HW_M25P80_SFDP_H
>> +
>> +#define M25P80_SFDP_AREA_SIZE 0x100
>> +
>> +#endif
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 82270884416e..32925589ec7a 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -33,6 +33,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qapi/error.h"
>>  #include "trace.h"
>> +#include "m25p80_sfdp.h"
>>  
>>  /* Fields for FlashPartInfo->flags */
>>  
>> @@ -72,6 +73,7 @@ typedef struct FlashPartInfo {
>>       * This field inform how many die is in the chip.
>>       */
>>      uint8_t die_cnt;
>> +    const uint8_t *sfdp;
>>  } FlashPartInfo;
>>  
>>  /* adapted from linux */
>> @@ -333,6 +335,7 @@ typedef enum {
>>      BULK_ERASE = 0xc7,
>>      READ_FSR = 0x70,
>>      RDCR = 0x15,
>> +    RDSFDP = 0x5a,
>>  
>>      READ = 0x03,
>>      READ4 = 0x13,
>> @@ -398,6 +401,7 @@ typedef enum {
>>      STATE_COLLECTING_DATA,
>>      STATE_COLLECTING_VAR_LEN_DATA,
>>      STATE_READING_DATA,
>> +    STATE_READING_SFDP,
>>  } CMDState;
>>  
>>  typedef enum {
>> @@ -619,6 +623,8 @@ static inline int get_addr_length(Flash *s)
>>      }
>>  
>>     switch (s->cmd_in_progress) {
>> +   case RDSFDP:
>> +       return 3;
>>     case PP4:
>>     case PP4_4:
>>     case QPP_4:
>> @@ -744,6 +750,17 @@ static void complete_collecting_data(Flash *s)
>>                            " by device\n");
>>          }
>>          break;
>> +
>> +    case RDSFDP:
>> +        if (s->cur_addr < M25P80_SFDP_AREA_SIZE) {
> 
> Is perhaps M25P80_SFDP_AREA_SIZE a limit for the micron flashes in patch
> 2 (and not sfdp)? 

No. It's a common limit of the chips available on the systems I have 
access to. Nothing is said in the specs on a maximum size and only 
one table is required. But, theoretically, we have a 24bits address 
space.  

> An option might be to change .sfdp to a '.sfdp_read' function decoding 
> the address internally (for the micron flashes it could just return the 
> array value in case the address is less than M25P80_SFDP_AREA_SIZE 

Yes. This is one way to make the SFDP table size depend on the chip 
model. 

> or 0xFF else). 

Some flash seems to wrap around when the max size is reached. See 
the mx25l25635e in my git tree. But I have only read 0x100 bytes.

> It might become easier
> to add flashes containing parameter tables in higher addresses (the
> function could then decode the address into a specific table/area).

yes. Something like that.

>> +            s->state = STATE_READING_SFDP;
>> +        } else {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "M25P80: Invalid SFDP address %#" PRIx32 "\n",
>> +                          s->cur_addr);
>> +        }
>> +        break;
>> +
>>      default:
>>          break;
>>      }
>> @@ -1160,6 +1177,16 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>      case RSTQIO:
>>          s->quad_enable = false;
>>          break;
>> +    case RDSFDP:
>> +        if (s->pi->sfdp) {
>> +            s->needed_bytes = get_addr_length(s) + 1 ; /* SFDP addr + dummy */
> 
> Should above be changed to:
> 
> s->needed_bytes = get_addr_length(s) + 8;  /* SFDP addr + dummy */

oops. I have been lucky. I need to change the Aspeed SMC snooping 
routine to compute the number of dummies for the RDSFDP command.

> (I think it might fail else when the flashes are operating in 2 lines and
> 4 lines and are generating 8 dummy cycles for the RDSFDP)

The specs say that the requirement are independent of the I/O mode.

    4.5 Instruction Input Modes
    
    The Read SFDP instruction can be used with device supported modes
    of (1-1-1), (2-2-2), or (4-4-4), but the opcode (0x5A), address
    (24 bits), eight dummy clocks (8 wait states), and 50 MHz
    requirements remain the same.

Thanks,

C. 


> Best regards,
> Francisco Iglesias
> 
>> +            s->pos = 0;
>> +            s->len = 0;
>> +            s->state = STATE_COLLECTING_DATA;
>> +            break;
>> +        }
>> +        /* Fallthrough */
>> +
>>      default:
>>          s->pos = 0;
>>          s->len = 1;
>> @@ -1256,6 +1283,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>>              }
>>          }
>>          break;
>> +    case STATE_READING_SFDP:
>> +        assert(s->pi->sfdp);
>> +        r = s->pi->sfdp[s->cur_addr];
>> +        trace_m25p80_read_sfdp(s, s->cur_addr, (uint8_t)r);
>> +        s->cur_addr = (s->cur_addr + 1) & (M25P80_SFDP_AREA_SIZE - 1);
>> +        break;
>>  
>>      default:
>>      case STATE_IDLE:
>> diff --git a/hw/block/trace-events b/hw/block/trace-events
>> index 958fcc5508d1..53d377ca2b46 100644
>> --- a/hw/block/trace-events
>> +++ b/hw/block/trace-events
>> @@ -152,5 +152,6 @@ m25p80_page_program(void *s, uint32_t addr, uint8_t tx) "[%p] page program cur_a
>>  m25p80_transfer(void *s, uint8_t state, uint32_t len, uint8_t needed, uint32_t pos, uint32_t cur_addr, uint8_t t) "[%p] Transfer state 0x%"PRIx8" len 0x%"PRIx32" needed 0x%"PRIx8" pos 0x%"PRIx32" addr 0x%"PRIx32" tx 0x%"PRIx8
>>  m25p80_read_byte(void *s, uint32_t addr, uint8_t v) "[%p] Read byte 0x%"PRIx32"=0x%"PRIx8
>>  m25p80_read_data(void *s, uint32_t pos, uint8_t v) "[%p] Read data 0x%"PRIx32"=0x%"PRIx8
>> +m25p80_read_sfdp(void *s, uint32_t addr, uint8_t v) "[%p] Read SFDP 0x%"PRIx32"=0x%"PRIx8
>>  m25p80_binding(void *s) "[%p] Binding to IF_MTD drive"
>>  m25p80_binding_no_bdrv(void *s) "[%p] No BDRV - binding to RAM"
>> -- 
>> 2.25.4
>>