Re: [PATCH v4 05/20] dt-bindings: memory: factorise LPDDR props into SDRAM props

[Clement LE GOFFIC]

Hi Julius,

On 7/23/25 23:48, Julius Werner wrote:
> > +      Compatible strings can be either explicit vendor names and part numbers
> > +      (e.g. elpida,ECB240ABACN), or generated strings of the form
> > +      (lp)?ddrX-Y,Z where X, Y and Z are in lower case hexadecimal with leading
> > +      zeroes and :
> > +        - X is the SDRAM version (2, 3, 4, etc.)
> > +        - for LPDDR :
> > +          - Y is the manufacturer ID (from MR5), 2 bytes
> > +          - Z is the revision ID (from MR6 and MR7), 4 bytes
>
> It's actually one byte manufacturer, two bytes revision. The YY,ZZZZ
> is supposed to represent the amount of hex digits needed.

Oh yes I see my error..

> > +        - for DDR4 with SPD, according to JEDEC SPD4.1.2.L-6 :
> > +          - Y is the manufacturer ID, 2 bytes, from bytes 320 and 321
> > +          - Z is the revision ID, 1 byte, from byte 349
>
> I don't think this will identify a part unambiguously, I would expect
> the DDR revision ID to be specific to the part number. (In fact, we're
> also not sure whether manufacturer+revision identifies LPDDR parts
> unambiguously for every vendor, we just didn't have anything more to
> work with there.) I would suggest to use either `ddrX-YYYY,AAA...,ZZ`
> or `ddrX-YYYY,ZZ,AAA...` (where AAA... is the part number string from
> SPD 329-348 without the trailing spaces). The first version looks a
> bit more natural but it might get confusing on the off chance that
> someone uses a comma in a part number string.

The first one seems better indeed.
If the manufacturer put a comma in the part number we should handle it 
at a software level to me and if it is a devicetree error it is up to 
the devicetree writer to fix it.
What do you think ?

> > +      The latter form can be useful when SDRAM nodes are created at runtime by
> > +      boot firmware that doesn't have access to static part number information.
>
> nit: This text slightly doesn't make sense anymore when in the DDR
> case we do actually have the part number. I guess the real thing the
> bootloader wouldn't have access to is the JEDEC manufacturer ID to
> name mapping.

Yes I will update it.

> > +      SDRAM revision ID:
> > +        - LPDDR SDRAM, decoded from Mode Register 6 and 7.
> > +        - DDR4 SDRAM, decoded from the SPD from bytes 349 according to
> > +          JEDEC SPD4.1.2.L-6.
>
> nit: Clarify that this is always two bytes for LPDDR and always one
> byte for DDR.

Ok

> > +      Density of SDRAM chip in megabits:
> > +        - LPDDR SDRAM, decoded from Mode Register 8.
> > +        - DDR4 SDRAM, decoded from the SPD from bytes 322 to 325 according to
> > +          JEDEC SPD4.1.2.L-6.
>
> Are these numbers correct? I downloaded SPD4.1.2.L-6 now and it looks
> like 322 is manufacturing location and 323-324 are manufacturing date.
> (Also, I think all of these are specific to DDR4 (and possibly 5?),
> but not to earlier versions. I don't think we need to list it for
> every version, but we should at least be specific what it applies to.)

I just reopen the doc and you are right, didn't have my glasses on I guess.
Accordingly to SPD4.1.2.L-6 it the info seems in the byte 4 on bits 3~0.

Best regards,
Clément