> You need to base your upstream work on upstream tree. My email was > changed like three months ago... Apologies, I just used the same email that I sent patches to last year. Once I write an actual patch for this issue, I'll make sure to use get_maintainer.pl. > >> 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. > > Why? > > At beginning of your message you kind of mixed two different usages: > 1. Knowing the topology of the memory. > 2. Figuring out total memory. > > Implementing (1) above would probably solve your (2) use case. But if > you only need (2), do you really need to define entire topology? Okay, sorry, I wasn't clear here. We are really interested in topology (for documentation and SKU identification purposes), so "just" figuring out total memory is not enough. The point I wanted to make is more that we want to be able to identify the whole topology down to the exact number of components on each layer, so the existing binding (which just defines one LPDDR chip without explaining how many instances of it there are and how they're hooked up together) is not enough. Saying "I want to be able to figure out total memory from this" is more like an easy way to verify that all the information we're looking for is available... i.e. if all the LPDDR chips and their amounts and relations to each other are described in a way that's detailed enough that I can total up their density values and come up with the same number that the /memory node says, then I know we're not missing any layer of information. But ultimately I'm interested in being able to read out the whole topology, not just total capacity. >> 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`, 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). 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. > 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. > >> (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). So merging the bindings would fix that. 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.
