linux-kernel - Re: [RFC] Correct memory layout reporting for "jedec,lpddr2" and related bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f51aed8-956b-ac09-3baf-2b4572db1352@linaro.org>
Date:   Thu, 30 Jun 2022 10:05:17 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Julius Werner <jwerner@...omium.org>
Cc:     Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Dmitry Osipenko <digetx@...il.com>,
        Jian-Jia Su <jjsu@...gle.com>,
        Doug Anderson <dianders@...omium.org>,
        Rob Herring <robh+dt@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Nikola Milosavljevic <mnidza@...look.com>
Subject: Re: [RFC] Correct memory layout reporting for "jedec,lpddr2" and
 related bindings

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