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>
In-Reply-To: <CAODwPW9E8wWwxbYKyf4_-JFb4F-JSmLR3qOF_iudjX0f9ndF0A@mail.gmail.com>
From:   Julius Werner <jwerner@chromium.org>
Date:   Tue, 14 Jun 2022 19:28:41 -0700
Message-ID: <CAODwPW8fiFSNehZbZDdR9kjHxohLGiyE7edU=Opy0xV_P8JbEQ@mail.gmail.com>
Subject: Re: [RFC] Correct memory layout reporting for "jedec,lpddr2" and
 related bindings
To:     Julius Werner <jwerner@chromium.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc:     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

Sorry, wrong email for Krzysztof.

On Tue, Jun 14, 2022 at 7:25 PM Julius Werner <jwerner@chromium.org> wrote:
>
> We (Chromium OS) have been trying to find a way to pass LPDDR memory
> chip information that is available to the firmware through the FDT
> (mostly for userspace informational purposes, for now). We have been
> using and expanding the existing "jedec,lpddr2" and "jedec,lpddr3"
> bindings for this (e.g. [1]). The goal is to be able to identify the
> memory layout of the system (how the parts look like, how they're tied
> together, how much capacity there is in total) as accurately as
> possible from software-probed values.
>
> The existing bindings contain the fields "io-width" and "density",
> which is terminology directly matching what an LPDDR chip can report
> to firmware through the "Mode Register" interface, specifically MR8.
> (The LPDDR specs describing this are not public, but you can see the
> register definitions in most LPDDR chip datasheets that can be
> randomly found online, e.g. [2] page 37.) The code in
> drivers/memory/of_memory.c also suggests that these are supposed to
> directly correspond to the MR8 values read from the chip, since when
> of_lpddr2_get_info() copies the device tree values into struct
> lpddr2_info, it encodes them in a format that directly matches the
> mode register bit field patterns.
>
> The problem with this is that each individual LPDDR chip has its own
> set of mode registers (per rank) that only describe the density of
> that particular chip (rank). The host memory controller may have
> multiple channels (each of which is basically an entirely separate set
> of physical LPDDR pins on the board), a single channel may be
> connected to multiple LPDDR chips (e.g. if the memory controller has
> an outgoing 32-bit channel, that channel could be tied to two 16-bit
> LPDDR chips by tying the low 16 bits to one and the high 16 bits to
> the other), and then each of those chips may offer multiple
> independent ranks (which rank is being accessed at a given time is
> controlled by a separate chip select pin).
>
> So if we just have one "io-width" and one "density" field in the FDT,
> there's no way to figure out how much memory there's actually
> connected in total, because that only describes a single LPDDR chip.
> Worse, there may be chips where different ranks have different
> densities (e.g. a 6GB dual-rank chip with one 4GB and one 2GB rank),
> and different channels could theoretically be connected to chips of
> completely different manufacturers.
>
> 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. For the former, I'd suggest
> creating a separate FDT node for each channel, and then creating
> subnodes under those for each rank that implement the binding. For the
> latter, I would suggest adding a new property "channel-io-width" which
> describes the width of the channel from the host memory controller's
> point of view, so that you can divide that property by the already
> existing "io-width" property to figure out how many parts are tied
> together in series in a single channel. The final layout, then, would
> look something like this:
>
> lpddr2-channel0 {
>     rank0 {
>         compatible = "jedec,lpddr2";
>         density = <2048>;
>         channel-io-width = <32>;
>         io-width = <16>;
>     };
>     rank1 {
>         compatible = "jedec,lpddr2";
>         density = <1024>;
>         channel-io-width = <32>;
>         io-width = <16>;
>     };
> };
> lpddr2-channel0 {
>     rank0 {
>         compatible = "jedec,lpddr2";
>         density = <2048>;
>         channel-io-width = <32>;
>         io-width = <16>;
>     };
>     rank1 {
>         compatible = "jedec,lpddr2";
>         density = <1024>;
>         channel-io-width = <32>;
>         io-width = <16>;
>     };
> };
>
> This would be describing a dual-channel, dual-rank layout where each
> 32-bit channel is connected to two 16-bit LPDDR chips in series. The
> total capacity would be (2048 Mbits * (32/16) chips + 1024 Mbits *
> (32/16) chips) * 2 channels = 12Gbits.
>
> Does this seem reasonable? If nobody has any objections, I can draft
> up a real patch to change the respective bindings. (The two existing
> uses in platform device trees would stay how they are until the
> respective platform maintainers choose to update them, since only they
> would know the exact configuration. They wouldn't technically violate
> the changed binding since they still contain the same properties
> (other than "channel-io-width" which could be declared optional), but
> they wouldn't represent the total memory layout.)
>
> (Also, btw, would it make sense to use this opportunity to combine the
> "jedec,lpddr2" and "jedec,lpddr3" bindings into a single document?
> They contain all the same properties and I think it makes sense to
> keep them in sync, so duplicating the documentation is just
> unnecessary maintenance overhead. I would also like to add a
> "jedec,lpddr4" binding that has the same properties.)
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml
> [2] https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/dram/mobile-dram/low-power-dram/lpddr2/2gb_automotive_lpddr2_u89n.pdf