Re: [PATCH v2 0/9] 5p80: Add SFDP support

[Cédric Le Goater]

On 10/7/20 3:43 AM, Joel Stanley wrote:
> On Wed, 2 Sep 2020 at 09:31, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Hello,
>>
>> 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. 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.
>>
>> This is the initial framework for the RDSFDP command giving access to
>> a private SFDP area under the flash.
>>
>> The patches available here :
>>
>>   https://github.com/legoater/qemu/commits/aspeed-5.2
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Tested-by: Joel Stanley <joel@jms.id.au>
> 
> Note that these need to be rebased on master; there are some minor conflicts.
> 
> These patches have proved essential in debugging a recent kernel
> regression. Thanks for adding this support Cédric.

We have been discussing offline with Francisco of a more subtle approach 
to reduce the size of the definitions of the SFDP tables. I agree that 
the current approach is brutal (and efficient :) but I haven't had time 
to take a close look at his proposal. See below.

    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;
    }


The SFDP header (SFDP_RAW_WRAP) contains the list of the SFDP tables, 
the first being the BFPT at offset 0x30. It would be nice to be able 
to build the list in the header from the different table definitions.

C.