Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220831013359.1807905-1-jwerner@chromium.org>
 <20220831013359.1807905-4-jwerner@chromium.org> <983c1224-8174-3534-a276-d1ab1f9968a4@linaro.org>
In-Reply-To: <983c1224-8174-3534-a276-d1ab1f9968a4@linaro.org>
From:   Julius Werner <jwerner@chromium.org>
Date:   Wed, 31 Aug 2022 18:10:51 -0700
Message-ID: <CAODwPW_70kdn4XTCs_vhbWwjEXS8E8zC9MTa6-szb5SayvcSag@mail.gmail.com>
Subject: Re: [PATCH 3/4] dt-bindings: memory: Add jedec,lpddr4 and
 jedec,lpddr5 bindings
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Julius Werner <jwerner@chromium.org>,
        Rob Herring <robh+dt@kernel.org>,
        Dmitry Osipenko <digetx@gmail.com>,
        Doug Anderson <dianders@chromium.org>,
        Jian-Jia Su <jjsu@google.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > index 0c7d2feafd77c8..e1182e75ca1a3f 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr-props.yaml
> > @@ -53,9 +53,13 @@ properties:
> >        - 512
> >        - 1024
> >        - 2048
> > +      - 3072
> >        - 4096
> > +      - 6144
> >        - 8192
> > +      - 12288
> >        - 16384
> > +      - 24576
> >        - 32768
>
> Either you limit now LPDDR2 and LPDDR3 to old values or instead add this
> bigger list to LPDDR4 and LPDDR5 (if it works that way).

The problem is that each spec has its own set of valid values, e.g.
LPDDR3 only defines 4GB, 8GB, 16GB and 32GB, and then LPDDR4 inserted
the 6GB, 12GB and 24GB options in between there. I don't think there's
a way to exactly describe the valid values for each version without
having a whole separate enum list for each. Do you think checking for
that is important enough to be worth having all that extra duplication
between the schemas? I don't think it adds that much (e.g. a value for
an individual memory part can still be wrong even if it is one of the
valid values for that type, so how much use is this validation
anyway?), but I can split it out if you want to.

> > +  serial-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      Serial IDs read from Mode Registers 47 through 54. One byte per uint32
> > +      cell (i.e. <MR47 MR48 MR49 MR50 MR51 MR52 MR53 MR54>).
> > +    minItems: 8
>
> No need for minItems.

Can you explain why? I'm okay with taking these out, but it is a real
constraint so I'm not sure why we shouldn't be describing it here?
(The serial ID always has exactly 8 bytes, an ID with less than 8
would not be valid and probably a typo.)