Re: [PATCH v2 2/9] m25p80: Add the n25q256a SFDP table

[Francisco Iglesias]

Hi Cedric,

On [2020 Sep 02] Wed 11:31:00, Cédric Le Goater wrote:
> The same values were collected on 4 differents OpenPower systems,
> palmettos, romulus and tacoma.
> 
> The SFDP table size is defined as being 0x100 bytes but it could be
> bigger. Only the mandatory table for basic features is available at
> byte 0x30.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/block/m25p80_sfdp.h |  2 ++
>  hw/block/m25p80.c      |  6 +++--
>  hw/block/m25p80_sfdp.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>  hw/block/meson.build   |  1 +
>  4 files changed, 65 insertions(+), 2 deletions(-)
>  create mode 100644 hw/block/m25p80_sfdp.c
> 
> diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
> index 230b07ef3308..d3a0a778ae84 100644
> --- a/hw/block/m25p80_sfdp.h
> +++ b/hw/block/m25p80_sfdp.h
> @@ -15,4 +15,6 @@
>   */
>  #define M25P80_SFDP_MAX_SIZE  (1 << 24)
>  
> +extern uint8_t m25p80_sfdp_n25q256a(uint32_t addr);

s/extern//

> +
>  #endif
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 4d6133b3594e..dbfd88b41a12 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -234,11 +234,13 @@ static const FlashPartInfo known_devices[] = {
>      { INFO("n25q128a11",  0x20bb18,      0,  64 << 10, 256, ER_4K) },
>      { INFO("n25q128a13",  0x20ba18,      0,  64 << 10, 256, ER_4K) },
>      { INFO("n25q256a11",  0x20bb19,      0,  64 << 10, 512, ER_4K) },
> -    { INFO("n25q256a13",  0x20ba19,      0,  64 << 10, 512, ER_4K) },
> +    { INFO("n25q256a13",  0x20ba19,      0,  64 << 10, 512, ER_4K),
> +      .sfdp_read = m25p80_sfdp_n25q256a },
>      { INFO("n25q512a11",  0x20bb20,      0,  64 << 10, 1024, ER_4K) },
>      { INFO("n25q512a13",  0x20ba20,      0,  64 << 10, 1024, ER_4K) },
>      { INFO("n25q128",     0x20ba18,      0,  64 << 10, 256, 0) },
> -    { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512, ER_4K) },
> +    { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512, ER_4K),
> +      .sfdp_read = m25p80_sfdp_n25q256a },
>      { INFO("n25q512a",    0x20ba20,      0,  64 << 10, 1024, ER_4K) },
>      { INFO("n25q512ax3",  0x20ba20,  0x1000,  64 << 10, 1024, ER_4K) },
>      { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) 
> },
> diff --git a/hw/block/m25p80_sfdp.c b/hw/block/m25p80_sfdp.c
> new file mode 100644
> index 000000000000..24ec05de79a1
> --- /dev/null
> +++ b/hw/block/m25p80_sfdp.c
> @@ -0,0 +1,58 @@
> +/*
> + * M25P80 Serial Flash Discoverable Parameter (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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/host-utils.h"
> +#include "m25p80_sfdp.h"
> +
> +#define define_sfdp_read(model)                                       \
> +    uint8_t m25p80_sfdp_##model(uint32_t addr)                        \
> +    {                                                                 \
> +        assert(is_power_of_2(sizeof(sfdp_##model)));                  \
> +        return sfdp_##model[addr & (sizeof(sfdp_##model) - 1)];       \
> +    }
> +
> +/*
> + * Micron
> + */
> +static const uint8_t sfdp_n25q256a[] = {
> +    0x53, 0x46, 0x44, 0x50, 0x00, 0x01, 0x00, 0xff,
> +    0x00, 0x00, 0x01, 0x09, 0x30, 0x00, 0x00, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xe5, 0x20, 0xfb, 0xff, 0xff, 0xff, 0xff, 0x0f,
> +    0x29, 0xeb, 0x27, 0x6b, 0x08, 0x3b, 0x27, 0xbb,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x27, 0xbb,
> +    0xff, 0xff, 0x29, 0xeb, 0x0c, 0x20, 0x10, 0xd8,
> +    0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,

Looking at the SFDP spec I have it says that reserved locations (unused or not
yet defined memory in the address space) are recommended to return 0xFF. Do you
think we could use that and somehow and return 0xFF by default above instead of
having them in the array? (The file will not grow as fast then)

Best regards,
Francisco Iglesias

> +};
> +define_sfdp_read(n25q256a);
> diff --git a/hw/block/meson.build b/hw/block/meson.build
> index 78cad8f7cba1..6f0380a12081 100644
> --- a/hw/block/meson.build
> +++ b/hw/block/meson.build
> @@ -10,6 +10,7 @@ softmmu_ss.add(when: 'CONFIG_ONENAND', if_true: 
> files('onenand.c'))
>  softmmu_ss.add(when: 'CONFIG_PFLASH_CFI01', if_true: files('pflash_cfi01.c'))
>  softmmu_ss.add(when: 'CONFIG_PFLASH_CFI02', if_true: files('pflash_cfi02.c'))
>  softmmu_ss.add(when: 'CONFIG_SSI_M25P80', if_true: files('m25p80.c'))
> +softmmu_ss.add(when: 'CONFIG_SSI_M25P80', if_true: files('m25p80_sfdp.c'))
>  softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
>  softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
>  softmmu_ss.add(when: 'CONFIG_SH4', if_true: files('tc58128.c'))
> -- 
> 2.25.4
>