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: <CAODwPW9E8wWwxbYKyf4_-JFb4F-JSmLR3qOF_iudjX0f9ndF0A@mail.gmail.com>
 <CAODwPW8fiFSNehZbZDdR9kjHxohLGiyE7edU=Opy0xV_P8JbEQ@mail.gmail.com>
 <3bb0ffa0-8091-0848-66af-180a41a68bf7@linaro.org> <CAODwPW89xZQZiZdQNt6+CcRjz=nbEAAFH0h_dBFSE5v3aFU4rQ@mail.gmail.com>
 <8f51aed8-956b-ac09-3baf-2b4572db1352@linaro.org> <CAODwPW9MvYJo8QbKOoVcUAKJ8Hxon2MCv_H5qpv=yaSTLLc+ug@mail.gmail.com>
 <628a7302-1409-81f7-f72b-6b1645df9225@linaro.org>
In-Reply-To: <628a7302-1409-81f7-f72b-6b1645df9225@linaro.org>
From:   Julius Werner <jwerner@chromium.org>
Date:   Thu, 7 Jul 2022 18:20:42 -0700
Message-ID: <CAODwPW-4i+idH8Nz6=EmNUXYWgWkoOHs3wOZ7BbrH5GwGDZ1Ww@mail.gmail.com>
Subject: Re: [RFC] Correct memory layout reporting for "jedec,lpddr2" and
 related bindings
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Julius Werner <jwerner@chromium.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Dmitry Osipenko <digetx@gmail.com>,
        Jian-Jia Su <jjsu@google.com>,
        Doug Anderson <dianders@chromium.org>,
        Rob Herring <robh+dt@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Nikola Milosavljevic <mnidza@outlook.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> Then I would assume that all lpddr properties can differ between ranks,
> including the timings. But probably some SDRAM memory expert should
> clarify that.

Right, so that's what my proposal does -- separate timings nodes per
rank (and channel).

> > That really doesn't work for our use case, we can't generate a
> > specific compatible string for each part number. This may work when
> > your board is only using a single memory part and you can hardcode
> > that in the DTB blob bundled with the kernel, but we are trying to do
> > runtime identification between dozens of different parts on our
> > boards. The whole point of us wanting to add these bindings is that we
> > want to have the firmware inject the raw values it can read from mode
> > registers into the device tree (with just the compatible string
> > "jedec,lpddr3"),
>
> You cannot have jedec,lpddr3 alone. You need specific compatible.

Sorry, what do you mean we cannot? Why not? That's the way we need to
do it for our use case. Why shouldn't it work that way? As far as I
understand the binding definition, this is one of the legal compatible
strings for it. (I'm not saying other platforms can't register and
provide specific compatible strings if they want to, of course, but
for our situation that really doesn't work.)

> > so that we can then delegate the task of matching
> > those values to part numbers to a userspace process.
>
> Constructing a vendor from mode registers is like 10 lines of C code, so
> this is not a problem. Trouble would be with device part of compatible.

There's potentially 255 different manufacturer codes, and the
assignments may be different for different LPDDR versions. That's a
big string table that we don't want to have to fit in our firmware
flash. Besides, as you said, that still only gives you the vendor...
so then should we use "micron,lpddr3" or "elpida,lpddr3" instead of
"jedec,lpddr3"? Where's the advantage in that?

> > Can we please revert that deprecation and at least keep the property
> > around as optional?
>
> Yes, we can. You still would need to generate the compatible according
> to the current bindings. Whether we can change it I am not sure. I think
> it depends how much customization is possible per vendor, according to
> JEDEC spec. If we never ever have to identify specific part, because
> JEDEC spec and registers tell us everything, then we could skip it,
> similarly to lpddr2 and jedec,spi-nor.

Shouldn't that be decided per use case? In general LPDDR is a pretty
rigid set of standards and memory controllers are generally compatible
with any vendor without hardcoding vendor-specific behavior, so I
don't anticipate that this would be likely (particularly since there
is no "real" kernel device driver that needs to initialize the full
memory controller, after all, these bindings are mostly
informational). Of course there may always be mistakes and broken
devices that need custom handling, and if someone has a platform with
such a case I of course don't want to preclude them from tying special
behavior to a custom compatible string. But why would that mean we
need to make this mandatory for all platforms even if it's not
relevant (and not practically feasible) for them? Why not allow both?

> > We need to be able to report the information that's currently encoded
> > in the "jedec,lpddr2" binding separately for each channel+rank
> > combination, and we need to be able to tell how many LPDDR chips are
> > combined under a single memory channel.
>
> Who and why needs that information?
>
> To me it's not a very useful information without knowing how memory
> ranges are mapped to the chips and then only kernel drivers should be
> able to utilize that info in a meaningful way. What driver are we
> talking about?

We're using this for diagnostic purposes, to be able to accurately
report the installed memory configuration to the user or in automated
error reporting. We're planning to just read it from userspace (via
/proc/device-tree) and not actually add a kernel driver for it, but
since it needs to come from the firmware through the device tree it
should have a standardized binding all the same.