Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

[Michael Walle]

Am 2022-07-28 15:31, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
> On 7/28/22 16:12, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> > the content is safe
> >
> > > > > +#define SNOR_ID3(_jedec_id)
> > >
> > > How about SFDP_ID3 and SFDP_ID6 instead?
> >
> > Yes, probably a better name. I was also thinking about splitting
> > the id in vendor, device and additional bytes. But I haven't
> > thought of the actual implementation that much. Such as:
> >
> > #define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
> > #define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>,
> > <variable aux bytes>)
> >
> > Couldn't make up a better name than that _FULL for now. Happy to hear
> > suggestions :)
>
> You mean splitting the ID in manufacturer ID, flash ID and extended 
> flash ID?
> I'd like to understand the benefits of splitting this, can you give me 
> an
> example? In the past I though about introducing some flash info macros 
> for
> families of flashes of the same vendor, it will reduce the number of 
> lines
> on flash definition, but not really related.

First, why would you combine the vendor and part id into one three byte
field? Isn't it natural to have these as two fields? We know the did is
8 bit and the vid is 16 bit. And there might be N continuation codes. So
putting that all in one value is error prone. See also below for the
is25cd512.
Second, the extended (so maybe SFDP_ID_EXT?) bytes is variable. I've
seen flashes with one additional byte.

If you want to have per vendor convenience macros you could do

#define WINBOND_SFDP_ID(did, ...) SFDP_ID(0xNN, did, __VA_ARGS__)

I don't see much benefit here. OTHO "#define SFDP_VID_WINBOND 0xNN"
might make sense. But then we'd need to come up with support for
continuation codes. So mhhh.

As for the examples:
 SFDP_ID(SFDP_VID_WINBOND, 0x6019)
 SFDP_ID(SFDP_VID_MICRON, 0xba19, 0x10, 0x44, 0x00)
or maybe
 SFDP_ID(SFDP_VID_MICRON, 0xba19, 0x10, 0x44)

Currently we have the is25cd512 which (correctly) uses
continuation codes. So, we'd need have something like
#define SFDP_VID_ISSI_LEGACY 0x9d
#define SFDP_VID_ISSI 1, 0x9d

And preferrably we'd have something like:

 SFDP_ID(SFDP_VID_ISSI_LEGACY, 0x4013)
 SFDP_ID(SFDD_VID_ISSI, 0x20xx)

(note that "xx" is unknown here.. we are lacking that because
the entry is just using INFO() and I'm too lazy to look up the
datasheet now.)

Now I know that above probably won't compile, but maybe someone
could come up with macros which actually work :)

The following might work, but feels awful like a hack:
#define SFDP_VID_ISSI_LEGACY 0, 0x9d
#define SFDP_VID_ISSI 1, 0x9d

-michael