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
