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;
Message-ID: <8f51aed8-956b-ac09-3baf-2b4572db1352@linaro.org>
Date:   Thu, 30 Jun 2022 10:05:17 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [RFC] Correct memory layout reporting for "jedec,lpddr2" and
 related bindings
Content-Language: en-US
To:     Julius Werner <jwerner@chromium.org>
Cc:     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>
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>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CAODwPW89xZQZiZdQNt6+CcRjz=nbEAAFH0h_dBFSE5v3aFU4rQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 30/06/2022 03:03, Julius Werner wrote:
>>> For the latter, I would suggest adding a new property "channel-io-width" which
>>
>> No, because io-width is a standard property, so it should be used
>> instead. It could be defined in channel node.
> 
> What exactly do you mean by "standard property" -- do you mean in an
> LPDDR context, or for device tree bindings in general? In other device
> tree bindings, the only thing I can find is `reg-io-width`,

I had impression I saw io-width outside of LPPDR bindings, but
apparently it's only reg-io-width

>  so that's
> not quite the same (and wouldn't seem to preclude calling a field here
> `channel-io-width`, since the width that's talking about is not the
> width of a register).

reg-io-width is not only about register width, but width of access size
or width of IO.


> In LPDDR context, the term "IO width" mostly
> appears specifically for the bit field in Mode Register 8 that
> describes the amount of DQ pins going into one individual LPDDR chip.
> The field that I need to encode for the channel here is explicitly
> *not* that, it's the amount of DQ pins coming *out* of the LPDDR
> controller, and as explained in my original email those two numbers
> need not necessarily be the same when multiple LPDDR chips are hooked
> up in parallel. So, yes, I could call both of these properties
> `io-width` with one in the rank node and one in the channel node...
> but I think giving the latter one a different name (e.g.
> `channel-io-width`) would be better to avoid confusion and provide a
> hint that there's an important difference between these numbers.

Send the bindings, we'll see what the DT binding maintainers will say. :)

> 
>> You also need a timings node. I don't think it would be different for
>> each of ranks, would it?
> 
> I think it might be? I'm honestly not a memory expert so I'm not
> really sure (Jian-Jia in CC might know this?), but since different
> ranks can be asymmetric (even when they're on the same part), I could
> imagine that, say, the larger rank might need slightly longer
> precharge time or something like that. They at least all implement a
> separate set of mode registers, so they could theoretically be
> configured with different latency settings through those.

This feels weird... although maybe one or few parameters of timings
could be different.

How the asymmetric SDRAMs report density? This is a field with
fixed/enum values, so does it mean two-rank-asymmetric module has two
registers, one per each rank and choice of register depends on chip select?

> 
>>>> (Also, btw, would it make sense to use this opportunity to combine the
>>>> "jedec,lpddr2" and "jedec,lpddr3" bindings into a single document?
>>
>> These bindings are quite different, so combining would result in big
>> allOf. I am not sure if there is benefit in that.
> 
> They should basically be 100% identical outside of the timings. I can
> see that jedec,lpddr2 is currently missing the manufacturer-id
> property, that's probably an oversight -- Mode Register 5 with that ID
> exists for LPDDR2 just as well as for LPDDR3, and we're already
> passing the revision IDs which is kinda useless without also passing
> the manufacturer ID as well (because the revision IDs are
> vendor-specific).

Manufacturer ID is taken from compatible. LPDDR3 has it deprecated.

> So merging the bindings would fix that. 

Nothing to fix, it was by choice.

> The only
> other difference I can see are the deprecated
> `revision-id1`/`revision-id2` fields for jedec,lpddr2 -- if I use a
> property inclusion mechanism like Doug suggested, those could stay
> separate in jedec,lpddr2 only (since they're deprecated anyway and
> replaced by `revision-id` in the combined bindings).
> 
> For the timings, I'm okay with keeping them separate.


Best regards,
Krzysztof