[PATCH v3 0/8] m25p80: Add SFDP support

Cédric Le Goater posted 8 patches 1 year, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220722063602.128144-1-clg@kaod.org
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, Alistair Francis <alistair@alistair23.me>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
There is a newer version of this series
hw/block/m25p80_sfdp.h |  27 ++++
hw/arm/aspeed.c        |   6 +-
hw/block/m25p80.c      |  49 ++++++-
hw/block/m25p80_sfdp.c | 296 +++++++++++++++++++++++++++++++++++++++++
MAINTAINERS            |   2 +-
hw/block/meson.build   |   1 +
hw/block/trace-events  |   1 +
7 files changed, 371 insertions(+), 11 deletions(-)
create mode 100644 hw/block/m25p80_sfdp.h
create mode 100644 hw/block/m25p80_sfdp.c
[PATCH v3 0/8] m25p80: Add SFDP support
Posted by Cédric Le Goater 1 year, 9 months ago
Hello, 

This is a refresh of a patchset sent long ago [1] adding support for
JEDEC STANDARD JESD216 Serial Flash Discovery Parameters (SFDP). SFDP
describes the features of a serial flash device using a set of internal
parameter tables. Support in Linux has been added some time ago and
the spi-nor driver is using it more often to detect the flash settings
and even flash models.

Francisco and I are not entirely satisfied with the way the tables are
defined. We add some private discussion on how to resolve that but
neither of us had the time to pursue the study. Latest Francisco
proposal was : 

    #define define_sfdp_read_wrap(model, wrap_sz)            \
    uint8_t m25p80_sdfp_read_##model(SFDPTable t, uint32_t addr) \
    {                                                            \
         return m25p80_sdfp_read(t, addr & (wrap_sz-1));          \
    }
    ...
    define_sfdp_read_wrap(mt25ql512ab, SZ_256)
    
    A new variable in the section would solve it aswell but not convinced at the
    moment if it is clear enough:
    
    typedef struct SFDPSection {
         const uint32_t addr;
         const uint32_t size;
         const uint32_t wrap_sz;
         const uint8_t *data;
    } SFDPSection;
    
    #define SFDP_RAW(start_addr, vals...) \
    {                                     \
       .addr = start_addr,                 \
       .size = sizeof((uint8_t[]){vals}),  \
       .data = (const uint8_t[]){vals}     \
    }
    
    #define SFDP_RAW_WRAP(start_addr, _wrap_sz, vals...) \
    {                                     \
       .addr = start_addr,                 \
       .size = sizeof((uint8_t[]){vals}),  \
       .wrap_sz = _wrap_sz,                \
       .data = (const uint8_t[]){vals}     \
    }
    
    #define SFDP_TABLE_END() { 0 }
    #define IS_SFDP_END(x) (x.size == 0)
    
    #define M35T4545_WRAP_SZ 0x100
    
    static const SFDPTable m35t4545 = {
         SFDP_RAW_WRAP(0, M35T4545_WRAP_SZ,
                       0x53, 0x46, 0x44, 0x50, 0x00, 0x01, 0x00, 0xff,
                       0x00, 0x00, 0x01, 0x09, 0x30, 0x00, 0x00, 0xff),
    
         SFDP_RAW(0x38,
                  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),
    
         SFDP_TABLE_END()
    };
    
    uint8_t m25p80_sfdp_read(SFDPTable t, uint32_t addr)
    {
         if (t[0].wrap_sz) {
             addr &= (t.wrap_sz-1);
         }
    
         for (int i = 0; !IS_SFDP_END(t[i]); i++) {
             if (addr >= t[i].addr && addr < (t[i].addr + t[i].size)) {
                 return t[i].data[addr];
             }
         }
         return 0xFF;
    }

Since there is a need, we have been using these patches in OpenBMC for
some time now and other projects/companies have requested it, I am
resending the patchset as it is to restart the discussion.

Thanks,

C.

Cédric Le Goater (8):
  m25p80: Add basic support for the SFDP command
  m25p80: Add the n25q256a SFDP table
  m25p80: Add the mx25l25635e SFPD table
  m25p80: Add the mx25l25635f SFPD table
  m25p80: Add the mx66l1g45g SFDP table
  m25p80: Add the w25q256 SFPD table
  m25p80: Add the w25q512jv SFPD table
  arm/aspeed: Replace mx25l25635e chip model

 hw/block/m25p80_sfdp.h |  27 ++++
 hw/arm/aspeed.c        |   6 +-
 hw/block/m25p80.c      |  49 ++++++-
 hw/block/m25p80_sfdp.c | 296 +++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS            |   2 +-
 hw/block/meson.build   |   1 +
 hw/block/trace-events  |   1 +
 7 files changed, 371 insertions(+), 11 deletions(-)
 create mode 100644 hw/block/m25p80_sfdp.h
 create mode 100644 hw/block/m25p80_sfdp.c

-- 
2.35.3


Re: [PATCH v3 0/8] m25p80: Add SFDP support
Posted by Patrick Williams 1 year, 7 months ago
On Fri, Jul 22, 2022 at 08:35:54AM +0200, Cédric Le Goater wrote:
> 
> Cédric Le Goater (8):
>   m25p80: Add basic support for the SFDP command
>   m25p80: Add the n25q256a SFDP table
>   m25p80: Add the mx25l25635e SFPD table
>   m25p80: Add the mx25l25635f SFPD table
>   m25p80: Add the mx66l1g45g SFDP table
>   m25p80: Add the w25q256 SFPD table
>   m25p80: Add the w25q512jv SFPD table
>   arm/aspeed: Replace mx25l25635e chip model
> 
>  hw/block/m25p80_sfdp.h |  27 ++++
>  hw/arm/aspeed.c        |   6 +-
>  hw/block/m25p80.c      |  49 ++++++-
>  hw/block/m25p80_sfdp.c | 296 +++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS            |   2 +-
>  hw/block/meson.build   |   1 +
>  hw/block/trace-events  |   1 +
>  7 files changed, 371 insertions(+), 11 deletions(-)
>  create mode 100644 hw/block/m25p80_sfdp.h
>  create mode 100644 hw/block/m25p80_sfdp.c
> 
> -- 
> 2.35.3
> 
> 

It seems that the kernel spi-nor driver maintainers really prefer to use
SFDP for parsing rather than relying on flags, so this support would be
really good to get added to QEMU now.  I've tested this patch set and
added (in reply to patch 8) support for the w25q01jvq chip.

Tested-by: Patrick Williams <patrick@stwcx.xyz>

-- 
Patrick Williams
Re: [PATCH v3 0/8] m25p80: Add SFDP support
Posted by Cédric Le Goater 1 year, 9 months ago
On 7/22/22 08:35, Cédric Le Goater wrote:
> Hello,
> 
> This is a refresh of a patchset sent long ago [1] adding support for

[1] was lost while writing the cover :

   https://lore.kernel.org/qemu-devel/20200902093107.608000-1-clg@kaod.org/

C.

> JEDEC STANDARD JESD216 Serial Flash Discovery Parameters (SFDP). SFDP
> describes the features of a serial flash device using a set of internal
> parameter tables. Support in Linux has been added some time ago and
> the spi-nor driver is using it more often to detect the flash settings
> and even flash models.
> 
> Francisco and I are not entirely satisfied with the way the tables are
> defined. We add some private discussion on how to resolve that but
> neither of us had the time to pursue the study. Latest Francisco
> proposal was :
> 
>      #define define_sfdp_read_wrap(model, wrap_sz)            \
>      uint8_t m25p80_sdfp_read_##model(SFDPTable t, uint32_t addr) \
>      {                                                            \
>           return m25p80_sdfp_read(t, addr & (wrap_sz-1));          \
>      }
>      ...
>      define_sfdp_read_wrap(mt25ql512ab, SZ_256)
>      
>      A new variable in the section would solve it aswell but not convinced at the
>      moment if it is clear enough:
>      
>      typedef struct SFDPSection {
>           const uint32_t addr;
>           const uint32_t size;
>           const uint32_t wrap_sz;
>           const uint8_t *data;
>      } SFDPSection;
>      
>      #define SFDP_RAW(start_addr, vals...) \
>      {                                     \
>         .addr = start_addr,                 \
>         .size = sizeof((uint8_t[]){vals}),  \
>         .data = (const uint8_t[]){vals}     \
>      }
>      
>      #define SFDP_RAW_WRAP(start_addr, _wrap_sz, vals...) \
>      {                                     \
>         .addr = start_addr,                 \
>         .size = sizeof((uint8_t[]){vals}),  \
>         .wrap_sz = _wrap_sz,                \
>         .data = (const uint8_t[]){vals}     \
>      }
>      
>      #define SFDP_TABLE_END() { 0 }
>      #define IS_SFDP_END(x) (x.size == 0)
>      
>      #define M35T4545_WRAP_SZ 0x100
>      
>      static const SFDPTable m35t4545 = {
>           SFDP_RAW_WRAP(0, M35T4545_WRAP_SZ,
>                         0x53, 0x46, 0x44, 0x50, 0x00, 0x01, 0x00, 0xff,
>                         0x00, 0x00, 0x01, 0x09, 0x30, 0x00, 0x00, 0xff),
>      
>           SFDP_RAW(0x38,
>                    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),
>      
>           SFDP_TABLE_END()
>      };
>      
>      uint8_t m25p80_sfdp_read(SFDPTable t, uint32_t addr)
>      {
>           if (t[0].wrap_sz) {
>               addr &= (t.wrap_sz-1);
>           }
>      
>           for (int i = 0; !IS_SFDP_END(t[i]); i++) {
>               if (addr >= t[i].addr && addr < (t[i].addr + t[i].size)) {
>                   return t[i].data[addr];
>               }
>           }
>           return 0xFF;
>      }
> 
> Since there is a need, we have been using these patches in OpenBMC for
> some time now and other projects/companies have requested it, I am
> resending the patchset as it is to restart the discussion.
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (8):
>    m25p80: Add basic support for the SFDP command
>    m25p80: Add the n25q256a SFDP table
>    m25p80: Add the mx25l25635e SFPD table
>    m25p80: Add the mx25l25635f SFPD table
>    m25p80: Add the mx66l1g45g SFDP table
>    m25p80: Add the w25q256 SFPD table
>    m25p80: Add the w25q512jv SFPD table
>    arm/aspeed: Replace mx25l25635e chip model
> 
>   hw/block/m25p80_sfdp.h |  27 ++++
>   hw/arm/aspeed.c        |   6 +-
>   hw/block/m25p80.c      |  49 ++++++-
>   hw/block/m25p80_sfdp.c | 296 +++++++++++++++++++++++++++++++++++++++++
>   MAINTAINERS            |   2 +-
>   hw/block/meson.build   |   1 +
>   hw/block/trace-events  |   1 +
>   7 files changed, 371 insertions(+), 11 deletions(-)
>   create mode 100644 hw/block/m25p80_sfdp.h
>   create mode 100644 hw/block/m25p80_sfdp.c
> 


Re: [PATCH v3 0/8] m25p80: Add SFDP support
Posted by Ben Dooks 1 year, 9 months ago
On Fri, Jul 22, 2022 at 09:05:39AM +0200, Cédric Le Goater wrote:
> On 7/22/22 08:35, Cédric Le Goater wrote:
> > Hello,
> > 
> > This is a refresh of a patchset sent long ago [1] adding support for
> 
> [1] was lost while writing the cover :
> 
>   https://lore.kernel.org/qemu-devel/20200902093107.608000-1-clg@kaod.org/

Is there a git branch this could be pulled from to have a look at and
test on our setup?

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.
Re: [PATCH v3 0/8] m25p80: Add SFDP support
Posted by Cédric Le Goater 1 year, 9 months ago
On 7/22/22 10:06, Ben Dooks wrote:
> On Fri, Jul 22, 2022 at 09:05:39AM +0200, Cédric Le Goater wrote:
>> On 7/22/22 08:35, Cédric Le Goater wrote:
>>> Hello,
>>>
>>> This is a refresh of a patchset sent long ago [1] adding support for
>>
>> [1] was lost while writing the cover :
>>
>>    https://lore.kernel.org/qemu-devel/20200902093107.608000-1-clg@kaod.org/
> 
> Is there a git branch this could be pulled from to have a look at and
> test on our setup?

Yes,

   https://github.com/legoater/qemu/commits/aspeed-7.1

Thanks,

C.