| Message ID | 20211006224659.21434-4-digetx@gmail.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tegra20-emc: Identify memory chip by LPDDR configuration | expand |
On Thu, 07 Oct 2021 01:46:53 +0300, Dmitry Osipenko wrote: > Add optional revision-id standard LPDDR2 properties which will help to > identify memory chip. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > .../memory-controllers/ddr/jedec,lpddr2.yaml | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
Apologies for only noticing this a couple of months too late... but this patch added the same thing to the "jedec,lpddr2" bindings that were previously added by https://www.spinics.net/lists/devicetree/msg413733.html to "jedec,lpddr3", but in a slightly incompatible manner. Should we adjust it to make them consistent (two bytes in one property, rather than a separate property for each)? Or is it too late to change that?
On 08/02/2022 03:06, Julius Werner wrote: > Apologies for only noticing this a couple of months too late... but > this patch added the same thing to the "jedec,lpddr2" bindings that > were previously added by > https://www.spinics.net/lists/devicetree/msg413733.html to > "jedec,lpddr3", but in a slightly incompatible manner. Should we > adjust it to make them consistent (two bytes in one property, rather > than a separate property for each)? Or is it too late to change that? Unfortunately I have no clue what patch you talk about ("this patch"). There is no context here, no link except the older LPDDR3. The latest discussion is about dtschema conversion, so no new fields are being added: https://lore.kernel.org/lkml/20220206135807.211767-1-krzysztof.kozlowski@canonical.com/ Feel free to propose something new on top of it. Best regards, Krzysztof
> Unfortunately I have no clue what patch you talk about ("this patch"). > There is no context here, no link except the older LPDDR3. Sorry, I tried to reply to https://lore.kernel.org/all/20211006224659.21434-4-digetx@gmail.com/ ([PATCH v5 3/9] dt-bindings: memory: lpddr2: Add revision-id properties) and was hoping that would automatically provide context. That patch added two one-cell properties `revision-id1` and `revision-id2` to "jedec,lpddr2". Earlier in https://www.spinics.net/lists/devicetree/msg413733.html ([PATCH] dt-bindings: ddr: Add optional manufacturer and revision ID to LPDDR3), I had added a single two-cell property `revision-id` for the same purpose to "jedec,lpddr3". I think it would be better if this was consistent between the two types of LPDDR memory. Should I just send a patch that replaces the two revision IDs in "jedec,lpddr2" with a single one according to the principle of "jedec,lpddr3"? Or is it too late for that now and the binding already considered stable and unchangeable?
On 09/02/2022 00:46, Julius Werner wrote: >> Unfortunately I have no clue what patch you talk about ("this patch"). >> There is no context here, no link except the older LPDDR3. > > Sorry, I tried to reply to > https://lore.kernel.org/all/20211006224659.21434-4-digetx@gmail.com/ > ([PATCH v5 3/9] dt-bindings: memory: lpddr2: Add revision-id > properties) and was hoping that would automatically provide context. > That patch added two one-cell properties `revision-id1` and > `revision-id2` to "jedec,lpddr2". Earlier in > https://www.spinics.net/lists/devicetree/msg413733.html ([PATCH] > dt-bindings: ddr: Add optional manufacturer and revision ID to > LPDDR3), I had added a single two-cell property `revision-id` for the > same purpose to "jedec,lpddr3". > > I think it would be better if this was consistent between the two > types of LPDDR memory. Should I just send a patch that replaces the > two revision IDs in "jedec,lpddr2" with a single one according to the > principle of "jedec,lpddr3"? Or is it too late for that now and the > binding already considered stable and unchangeable? Hi Julius, Having same bindings for revision ID makes sense. Sadly this was not spotted during review, eh, life... Unfortunately the bindings are already in a mainline release, so they are considered stable. You can however bring patches (bindings + drivers/memory/of + dts) which make the revision-id[12] deprecated and introduce new revision-id. It should be something similar to what I did for max-freq: https://lore.kernel.org/all/20220206135807.211767-7-krzysztof.kozlowski@canonical.com/ Dmitry, Any early comments on such approach from you? Best regards, Krzysztof
09.02.2022 11:58, Krzysztof Kozlowski пишет: > On 09/02/2022 00:46, Julius Werner wrote: >>> Unfortunately I have no clue what patch you talk about ("this patch"). >>> There is no context here, no link except the older LPDDR3. >> >> Sorry, I tried to reply to >> https://lore.kernel.org/all/20211006224659.21434-4-digetx@gmail.com/ >> ([PATCH v5 3/9] dt-bindings: memory: lpddr2: Add revision-id >> properties) and was hoping that would automatically provide context. >> That patch added two one-cell properties `revision-id1` and >> `revision-id2` to "jedec,lpddr2". Earlier in >> https://www.spinics.net/lists/devicetree/msg413733.html ([PATCH] >> dt-bindings: ddr: Add optional manufacturer and revision ID to >> LPDDR3), I had added a single two-cell property `revision-id` for the >> same purpose to "jedec,lpddr3". >> >> I think it would be better if this was consistent between the two >> types of LPDDR memory. Should I just send a patch that replaces the >> two revision IDs in "jedec,lpddr2" with a single one according to the >> principle of "jedec,lpddr3"? Or is it too late for that now and the >> binding already considered stable and unchangeable? > > Hi Julius, > > Having same bindings for revision ID makes sense. Sadly this was not > spotted during review, eh, life... Unfortunately the bindings are > already in a mainline release, so they are considered stable. You can > however bring patches (bindings + drivers/memory/of + dts) which make > the revision-id[12] deprecated and introduce new revision-id. > > It should be something similar to what I did for max-freq: > https://lore.kernel.org/all/20220206135807.211767-7-krzysztof.kozlowski@canonical.com/ > > Dmitry, > Any early comments on such approach from you? I don't mind, but I also don't see where the revision-id property of LPDDR3 is used at all. I can't find any device-tree with LPDDR3 revision-id and don't see it being used in the code either. Maybe it's the LPDDR3 binding that needs to be changed? I made each LPDDR2 revision-id property to correspond to a dedicated MR of LPDDR, which feels okay to me to since it matches h/w.
> I don't mind, but I also don't see where the revision-id property of > LPDDR3 is used at all. I can't find any device-tree with LPDDR3 > revision-id and don't see it being used in the code either. Maybe it's > the LPDDR3 binding that needs to be changed? We are using the revision ID in userspace (read through /proc/device-tree) for runtime memory identification. We don't have a kernel driver bound to it. Our boot firmware is inserting this value at runtime into the FDT (that's basically the reason we have this, our firmware auto-detects memory during boot and we use the FDT to report what it found to userspace), that's why you can't find it anywhere in the static device trees in boot/dts/. > I made each LPDDR2 revision-id property to correspond to a dedicated MR > of LPDDR, which feels okay to me to since it matches h/w. I'm not super married to my solution, so if that makes things easier we can standardize on the two-property version as well. I mostly designed it my way because I thought we may one day also want to do something like this for the 8-byte LPDDR5 serial-id, and then it would get kinda cumbersome to have serial-id1 through serial-id8 all as separate properties. But that's also a bridge we can cross when we get there. My use case is in a position where we could still change this now without requiring backwards-compatibility. Krzysztof, would you be okay if I instead changed the "jedec,lpddr3" to the same thing "jedec,lpddr2" does -- seeing as the original patch was from me, my use case could handle the switch, there has never been any actual kernel code using the property, and it seems very unlikely that anyone else has silently started using the same thing in the time it's been in the tree? Or do we also need to go the official deprecation route for that?
В Wed, 9 Feb 2022 16:32:25 -0800 Julius Werner <jwerner@chromium.org> пишет: > > I don't mind, but I also don't see where the revision-id property of > > LPDDR3 is used at all. I can't find any device-tree with LPDDR3 > > revision-id and don't see it being used in the code either. Maybe > > it's the LPDDR3 binding that needs to be changed? > > We are using the revision ID in userspace (read through > /proc/device-tree) for runtime memory identification. We don't have a > kernel driver bound to it. Our boot firmware is inserting this value > at runtime into the FDT (that's basically the reason we have this, our > firmware auto-detects memory during boot and we use the FDT to report > what it found to userspace), that's why you can't find it anywhere in > the static device trees in boot/dts/. Thank you for the clarification. Which device is that and why userspace needs to know so much about memory? > > I made each LPDDR2 revision-id property to correspond to a > > dedicated MR of LPDDR, which feels okay to me to since it matches > > h/w. > > I'm not super married to my solution, so if that makes things easier > we can standardize on the two-property version as well. I mostly > designed it my way because I thought we may one day also want to do > something like this for the 8-byte LPDDR5 serial-id, and then it would > get kinda cumbersome to have serial-id1 through serial-id8 all as > separate properties. But that's also a bridge we can cross when we get > there. > > My use case is in a position where we could still change this now > without requiring backwards-compatibility. Krzysztof, would you be > okay if I instead changed the "jedec,lpddr3" to the same thing > "jedec,lpddr2" does -- seeing as the original patch was from me, my > use case could handle the switch, there has never been any actual > kernel code using the property, and it seems very unlikely that anyone > else has silently started using the same thing in the time it's been > in the tree? Or do we also need to go the official deprecation route > for that? If you're going to use multiple cells for other properties, then indeed will be better to keep it consistent.
On 11/02/2022 00:17, Dmitry Osipenko wrote: > В Wed, 9 Feb 2022 16:32:25 -0800 > Julius Werner <jwerner@chromium.org> пишет: > > >>> I made each LPDDR2 revision-id property to correspond to a >>> dedicated MR of LPDDR, which feels okay to me to since it matches >>> h/w. >> >> I'm not super married to my solution, so if that makes things easier >> we can standardize on the two-property version as well. I mostly >> designed it my way because I thought we may one day also want to do >> something like this for the 8-byte LPDDR5 serial-id, and then it would >> get kinda cumbersome to have serial-id1 through serial-id8 all as >> separate properties. But that's also a bridge we can cross when we get >> there. >> >> My use case is in a position where we could still change this now >> without requiring backwards-compatibility. Krzysztof, would you be >> okay if I instead changed the "jedec,lpddr3" to the same thing >> "jedec,lpddr2" does -- seeing as the original patch was from me, my >> use case could handle the switch, there has never been any actual >> kernel code using the property, and it seems very unlikely that anyone >> else has silently started using the same thing in the time it's been >> in the tree? Or do we also need to go the official deprecation route >> for that? > > If you're going to use multiple cells for other properties, then indeed > will be better to keep it consistent. Yeah, LPDDR5 is a nice argument. Let's go with the array approach (so LPDDR3). Julius, Official deprecation is needed, because the property might be used also in other projects or customers. But this is not a big deal - we will just keep old property for some time. Will you send a patch for it? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml index f931fe910ce5..fe573750577e 100644 --- a/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/ddr/jedec,lpddr2.yaml @@ -24,6 +24,18 @@ properties: - enum: - jedec,lpddr2-nvm + revision-id1: + $ref: /schemas/types.yaml#/definitions/uint32 + maximum: 255 + description: | + Revision 1 value of SDRAM chip. Obtained from device datasheet. + + revision-id2: + $ref: /schemas/types.yaml#/definitions/uint32 + maximum: 255 + description: | + Revision 2 value of SDRAM chip. Obtained from device datasheet. + density: $ref: /schemas/types.yaml#/definitions/uint32 description: | @@ -151,6 +163,8 @@ examples: compatible = "elpida,ECB240ABACN", "jedec,lpddr2-s4"; density = <2048>; io-width = <32>; + revision-id1 = <1>; + revision-id2 = <0>; tRPab-min-tck = <3>; tRCD-min-tck = <3>;
Add optional revision-id standard LPDDR2 properties which will help to identify memory chip. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- .../memory-controllers/ddr/jedec,lpddr2.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+)