* [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional @ 2021-09-28 4:45 Bjorn Andersson 2021-09-28 4:45 ` [PATCH 2/3] soc: qcom: smem: Support reserved-memory description Bjorn Andersson ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Bjorn Andersson @ 2021-09-28 4:45 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Rob Herring, Frank Rowand Cc: linux-arm-msm, devicetree, linux-kernel In the olden days the Qualcomm shared memory (SMEM) region consisted of multiple chunks of memory, so SMEM was described as a standalone node with references to its various memory regions. But practically all modern Qualcomm platforms has a single reserved memory region used for SMEM. So rather than having to use two nodes to describe the one SMEM region, update the binding to allow the reserved-memory region alone to describe SMEM. The olden format is preserved as valid, as this is widely used already. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml index f7e17713b3d8..4149cf2b66be 100644 --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml @@ -10,14 +10,18 @@ maintainers: - Andy Gross <agross@kernel.org> - Bjorn Andersson <bjorn.andersson@linaro.org> -description: | - This binding describes the Qualcomm Shared Memory Manager, used to share data - between various subsystems and OSes in Qualcomm platforms. +description: + This binding describes the Qualcomm Shared Memory Manager, a region of + reserved-memory used to share data between various subsystems and OSes in + Qualcomm platforms. properties: compatible: const: qcom,smem + reg: + maxItems: 1 + memory-region: maxItems: 1 description: handle to memory reservation for main SMEM memory region. @@ -29,11 +33,19 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: handle to RPM message memory resource + no-map: true + required: - compatible - - memory-region - hwlocks +oneOf: + - required: + - reg + - no-map + - required: + - memory-region + additionalProperties: false examples: @@ -43,6 +55,20 @@ examples: #size-cells = <1>; ranges; + smem@fa00000 { + compatible = "qcom,smem"; + reg = <0xfa00000 0x200000>; + no-map; + + hwlocks = <&tcsr_mutex 3>; + }; + }; + - | + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + smem_region: smem@fa00000 { reg = <0xfa00000 0x200000>; no-map; -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] soc: qcom: smem: Support reserved-memory description 2021-09-28 4:45 [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Bjorn Andersson @ 2021-09-28 4:45 ` Bjorn Andersson 2021-09-28 4:45 ` [PATCH 3/3] arm64: dts: qcom: sdm845: Drop standalone smem node Bjorn Andersson ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Bjorn Andersson @ 2021-09-28 4:45 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Rob Herring, Frank Rowand Cc: linux-arm-msm, devicetree, linux-kernel Practically all modern Qualcomm platforms has a single reserved-memory region for SMEM. So rather than having to describe SMEM in the form of a node with a reference to a reserved-memory node, allow the SMEM device to be instantiated directly from the reserved-memory node. The current means of falling back to dereferencing the "memory-region" is kept as a fallback, if it's determined that the SMEM node is a reserved-memory node. The "qcom,smem" compatible is added to the reserved_mem_matches list, to allow the reserved-memory device to be probed. In order to retain the readability of the code, the resolution of resources is split from the actual ioremapping. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/of/platform.c | 1 + drivers/soc/qcom/smem.c | 57 ++++++++++++++++++++++++++++------------- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 32d5ff8df747..07813fb1ef37 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -505,6 +505,7 @@ EXPORT_SYMBOL_GPL(of_platform_default_populate); static const struct of_device_id reserved_mem_matches[] = { { .compatible = "qcom,rmtfs-mem" }, { .compatible = "qcom,cmd-db" }, + { .compatible = "qcom,smem" }, { .compatible = "ramoops" }, { .compatible = "nvmem-rmem" }, {} diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c index 4fb5aeeb0843..c7e519bfdc8a 100644 --- a/drivers/soc/qcom/smem.c +++ b/drivers/soc/qcom/smem.c @@ -9,6 +9,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/of_reserved_mem.h> #include <linux/platform_device.h> #include <linux/sizes.h> #include <linux/slab.h> @@ -240,7 +241,7 @@ static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */ * @size: size of the memory region */ struct smem_region { - u32 aux_base; + phys_addr_t aux_base; void __iomem *virt_base; size_t size; }; @@ -499,7 +500,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem, for (i = 0; i < smem->num_regions; i++) { region = &smem->regions[i]; - if (region->aux_base == aux_base || !aux_base) { + if ((u32)region->aux_base == aux_base || !aux_base) { if (size != NULL) *size = le32_to_cpu(entry->size); return region->virt_base + le32_to_cpu(entry->offset); @@ -664,7 +665,7 @@ phys_addr_t qcom_smem_virt_to_phys(void *p) if (p < region->virt_base + region->size) { u64 offset = p - region->virt_base; - return (phys_addr_t)region->aux_base + offset; + return region->aux_base + offset; } } @@ -863,12 +864,12 @@ qcom_smem_enumerate_partitions(struct qcom_smem *smem, u16 local_host) return 0; } -static int qcom_smem_map_memory(struct qcom_smem *smem, struct device *dev, - const char *name, int i) +static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name, + struct smem_region *region) { + struct device *dev = smem->dev; struct device_node *np; struct resource r; - resource_size_t size; int ret; np = of_parse_phandle(dev->of_node, name, 0); @@ -881,13 +882,9 @@ static int qcom_smem_map_memory(struct qcom_smem *smem, struct device *dev, of_node_put(np); if (ret) return ret; - size = resource_size(&r); - smem->regions[i].virt_base = devm_ioremap_wc(dev, r.start, size); - if (!smem->regions[i].virt_base) - return -ENOMEM; - smem->regions[i].aux_base = (u32)r.start; - smem->regions[i].size = size; + region->aux_base = r.start; + region->size = resource_size(&r); return 0; } @@ -895,12 +892,14 @@ static int qcom_smem_map_memory(struct qcom_smem *smem, struct device *dev, static int qcom_smem_probe(struct platform_device *pdev) { struct smem_header *header; + struct reserved_mem *rmem; struct qcom_smem *smem; size_t array_size; int num_regions; int hwlock_id; u32 version; int ret; + int i; num_regions = 1; if (of_find_property(pdev->dev.of_node, "qcom,rpm-msg-ram", NULL)) @@ -914,13 +913,35 @@ static int qcom_smem_probe(struct platform_device *pdev) smem->dev = &pdev->dev; smem->num_regions = num_regions; - ret = qcom_smem_map_memory(smem, &pdev->dev, "memory-region", 0); - if (ret) - return ret; + rmem = of_reserved_mem_lookup(pdev->dev.of_node); + if (rmem) { + smem->regions[0].aux_base = rmem->base; + smem->regions[0].size = rmem->size; + } else { + /* + * Fall back to the memory-region reference, if we're not a + * reserved-memory node. + */ + ret = qcom_smem_resolve_mem(smem, "memory-region", &smem->regions[0]); + if (ret) + return ret; + } - if (num_regions > 1 && (ret = qcom_smem_map_memory(smem, &pdev->dev, - "qcom,rpm-msg-ram", 1))) - return ret; + if (num_regions > 1) { + ret = qcom_smem_resolve_mem(smem, "qcom,rpm-msg-ram", &smem->regions[1]); + if (ret) + return ret; + } + + for (i = 0; i < num_regions; i++) { + smem->regions[i].virt_base = devm_ioremap_wc(&pdev->dev, + smem->regions[i].aux_base, + smem->regions[i].size); + if (!smem->regions[i].virt_base) { + dev_err(&pdev->dev, "failed to remap %pa\n", &smem->regions[i].aux_base); + return -ENOMEM; + } + } header = smem->regions[0].virt_base; if (le32_to_cpu(header->initialized) != 1 || -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] arm64: dts: qcom: sdm845: Drop standalone smem node 2021-09-28 4:45 [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Bjorn Andersson 2021-09-28 4:45 ` [PATCH 2/3] soc: qcom: smem: Support reserved-memory description Bjorn Andersson @ 2021-09-28 4:45 ` Bjorn Andersson 2021-09-28 10:22 ` [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Stephan Gerhold 2021-09-28 12:28 ` Rob Herring 3 siblings, 0 replies; 9+ messages in thread From: Bjorn Andersson @ 2021-09-28 4:45 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Rob Herring, Frank Rowand Cc: linux-arm-msm, devicetree, linux-kernel Now that the SMEM binding and driver allows the SMEM node to be described in the reserved-memory region directly, move the compatible and hwlock properties to the reserved-memory node and drop the standadlone node. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index beee57087d05..2800eae61910 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -99,9 +99,11 @@ aop_cmd_db_mem: memory@85fe0000 { no-map; }; - smem_mem: memory@86000000 { + memory@86000000 { + compatible = "qcom,smem"; reg = <0x0 0x86000000 0 0x200000>; no-map; + hwlocks = <&tcsr_mutex 3>; }; tz_mem: memory@86200000 { @@ -941,12 +943,6 @@ tcsr_mutex: hwlock { #hwlock-cells = <1>; }; - smem { - compatible = "qcom,smem"; - memory-region = <&smem_mem>; - hwlocks = <&tcsr_mutex 3>; - }; - smp2p-cdsp { compatible = "qcom,smp2p"; qcom,smem = <94>, <432>; -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional 2021-09-28 4:45 [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Bjorn Andersson 2021-09-28 4:45 ` [PATCH 2/3] soc: qcom: smem: Support reserved-memory description Bjorn Andersson 2021-09-28 4:45 ` [PATCH 3/3] arm64: dts: qcom: sdm845: Drop standalone smem node Bjorn Andersson @ 2021-09-28 10:22 ` Stephan Gerhold 2021-09-28 17:34 ` Rob Herring 2021-09-28 12:28 ` Rob Herring 3 siblings, 1 reply; 9+ messages in thread From: Stephan Gerhold @ 2021-09-28 10:22 UTC (permalink / raw) To: Bjorn Andersson, Rob Herring Cc: Andy Gross, Frank Rowand, linux-arm-msm, devicetree, linux-kernel On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > In the olden days the Qualcomm shared memory (SMEM) region consisted of > multiple chunks of memory, so SMEM was described as a standalone node > with references to its various memory regions. > > But practically all modern Qualcomm platforms has a single reserved memory > region used for SMEM. So rather than having to use two nodes to describe > the one SMEM region, update the binding to allow the reserved-memory > region alone to describe SMEM. > > The olden format is preserved as valid, as this is widely used already. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > index f7e17713b3d8..4149cf2b66be 100644 > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > [...] > @@ -43,6 +55,20 @@ examples: > #size-cells = <1>; > ranges; > > + smem@fa00000 { I think this is a good opportunity to make a decision which node name should be used here. :) You use smem@ here but mentioned before that you think using the generic memory@ would be better [1]. And you use memory@ in PATCH 3/3: - smem_mem: memory@86000000 { + memory@86000000 { + compatible = "qcom,smem"; reg = <0x0 0x86000000 0 0x200000>; no-map; + hwlocks = <&tcsr_mutex 3>; }; However, if you would use memory@ as example in this DT schema, Rob's bot would complain with the same error that I mentioned earlier [2]: soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property From schema: dtschema/schemas/memory.yaml We should either fix the error when using memory@ or start using some different node name (Stephen Boyd suggested shared-memory@ for example). Otherwise we'll just keep introducing more and more dtbs_check errors for the Qualcomm device trees. Thanks, Stephan [1]: https://lore.kernel.org/linux-arm-msm/YUo0suaIugOco1Vu@builder.lan/ [2]: https://lore.kernel.org/linux-arm-msm/YUo2ZzQktf2iSec%2F@gerhold.net/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional 2021-09-28 10:22 ` [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Stephan Gerhold @ 2021-09-28 17:34 ` Rob Herring 2021-09-28 17:49 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Rob Herring @ 2021-09-28 17:34 UTC (permalink / raw) To: Stephan Gerhold Cc: Bjorn Andersson, Andy Gross, Frank Rowand, linux-arm-msm, devicetree, linux-kernel On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > multiple chunks of memory, so SMEM was described as a standalone node > > with references to its various memory regions. > > > > But practically all modern Qualcomm platforms has a single reserved memory > > region used for SMEM. So rather than having to use two nodes to describe > > the one SMEM region, update the binding to allow the reserved-memory > > region alone to describe SMEM. > > > > The olden format is preserved as valid, as this is widely used already. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > index f7e17713b3d8..4149cf2b66be 100644 > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > [...] > > @@ -43,6 +55,20 @@ examples: > > #size-cells = <1>; > > ranges; > > > > + smem@fa00000 { > > I think this is a good opportunity to make a decision which node name > should be used here. :) reserved-memory node names are kind of a mess, so I haven't tried for any standard... It needs to be solved globally. > > You use smem@ here but mentioned before that you think using the generic > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > - smem_mem: memory@86000000 { > + memory@86000000 { > + compatible = "qcom,smem"; > reg = <0x0 0x86000000 0 0x200000>; > no-map; > + hwlocks = <&tcsr_mutex 3>; > }; > > However, if you would use memory@ as example in this DT schema, > Rob's bot would complain with the same error that I mentioned earlier [2]: > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > From schema: dtschema/schemas/memory.yaml > > We should either fix the error when using memory@ or start using some > different node name (Stephen Boyd suggested shared-memory@ for example). > Otherwise we'll just keep introducing more and more dtbs_check errors > for the Qualcomm device trees. A different node name. A node name should only have 1 meaning and 'memory' is already defined. The main issue here is what to name nodes with only a size and no address. Rob ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional 2021-09-28 17:34 ` Rob Herring @ 2021-09-28 17:49 ` Bjorn Andersson 2021-09-28 19:51 ` Rob Herring 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2021-09-28 17:49 UTC (permalink / raw) To: Rob Herring Cc: Stephan Gerhold, Andy Gross, Frank Rowand, linux-arm-msm, devicetree, linux-kernel On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote: > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > > multiple chunks of memory, so SMEM was described as a standalone node > > > with references to its various memory regions. > > > > > > But practically all modern Qualcomm platforms has a single reserved memory > > > region used for SMEM. So rather than having to use two nodes to describe > > > the one SMEM region, update the binding to allow the reserved-memory > > > region alone to describe SMEM. > > > > > > The olden format is preserved as valid, as this is widely used already. > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > index f7e17713b3d8..4149cf2b66be 100644 > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > [...] > > > @@ -43,6 +55,20 @@ examples: > > > #size-cells = <1>; > > > ranges; > > > > > > + smem@fa00000 { > > > > I think this is a good opportunity to make a decision which node name > > should be used here. :) > > reserved-memory node names are kind of a mess, so I haven't tried for > any standard... It needs to be solved globally. > I'd be happy to paint the shed any color you decide :) That said, the binding itself doesn't mandate any node name, so it's just the example here that would be "wrong" - and just as wrong as it currently is. > > > > You use smem@ here but mentioned before that you think using the generic > > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > > > - smem_mem: memory@86000000 { > > + memory@86000000 { > > + compatible = "qcom,smem"; > > reg = <0x0 0x86000000 0 0x200000>; > > no-map; > > + hwlocks = <&tcsr_mutex 3>; > > }; > > > > However, if you would use memory@ as example in this DT schema, > > Rob's bot would complain with the same error that I mentioned earlier [2]: > > > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > > From schema: dtschema/schemas/memory.yaml > > > > We should either fix the error when using memory@ or start using some > > different node name (Stephen Boyd suggested shared-memory@ for example). > > Otherwise we'll just keep introducing more and more dtbs_check errors > > for the Qualcomm device trees. > > A different node name. A node name should only have 1 meaning and > 'memory' is already defined. > > The main issue here is what to name nodes with only a size and no address. > This particular node has both address and size (as does all of the other reserved-memory regions we use upstream today)... Regards, Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional 2021-09-28 17:49 ` Bjorn Andersson @ 2021-09-28 19:51 ` Rob Herring 2021-09-28 22:06 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Rob Herring @ 2021-09-28 19:51 UTC (permalink / raw) To: Bjorn Andersson Cc: Stephan Gerhold, Andy Gross, Frank Rowand, linux-arm-msm, devicetree, linux-kernel On Tue, Sep 28, 2021 at 12:49 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote: > > > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > > > multiple chunks of memory, so SMEM was described as a standalone node > > > > with references to its various memory regions. > > > > > > > > But practically all modern Qualcomm platforms has a single reserved memory > > > > region used for SMEM. So rather than having to use two nodes to describe > > > > the one SMEM region, update the binding to allow the reserved-memory > > > > region alone to describe SMEM. > > > > > > > > The olden format is preserved as valid, as this is widely used already. > > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > --- > > > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > index f7e17713b3d8..4149cf2b66be 100644 > > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > [...] > > > > @@ -43,6 +55,20 @@ examples: > > > > #size-cells = <1>; > > > > ranges; > > > > > > > > + smem@fa00000 { > > > > > > I think this is a good opportunity to make a decision which node name > > > should be used here. :) > > > > reserved-memory node names are kind of a mess, so I haven't tried for > > any standard... It needs to be solved globally. > > > > I'd be happy to paint the shed any color you decide :) I didn't ask for it to be painted. Unless it is for everyone, I don't care unless there's some clear pattern used already. > That said, the binding itself doesn't mandate any node name, so it's > just the example here that would be "wrong" - and just as wrong as it > currently is. The example is right. The dts is wrong. Perhaps we need a schema for 'any node name that doesn't match already defined ones'. > > > You use smem@ here but mentioned before that you think using the generic > > > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > > > > > - smem_mem: memory@86000000 { > > > + memory@86000000 { > > > + compatible = "qcom,smem"; > > > reg = <0x0 0x86000000 0 0x200000>; > > > no-map; > > > + hwlocks = <&tcsr_mutex 3>; > > > }; > > > > > > However, if you would use memory@ as example in this DT schema, > > > Rob's bot would complain with the same error that I mentioned earlier [2]: > > > > > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > > > From schema: dtschema/schemas/memory.yaml > > > > > > We should either fix the error when using memory@ or start using some > > > different node name (Stephen Boyd suggested shared-memory@ for example). > > > Otherwise we'll just keep introducing more and more dtbs_check errors > > > for the Qualcomm device trees. > > > > A different node name. A node name should only have 1 meaning and > > 'memory' is already defined. > > > > The main issue here is what to name nodes with only a size and no address. > > > > This particular node has both address and size (as does all of the other > reserved-memory regions we use upstream today)... I'm not talking about *just* QCom. If we define something here, it's got to cover everyone. In summary, you can't use 'memory' or anything other established, standard node name. Rob ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional 2021-09-28 19:51 ` Rob Herring @ 2021-09-28 22:06 ` Bjorn Andersson 0 siblings, 0 replies; 9+ messages in thread From: Bjorn Andersson @ 2021-09-28 22:06 UTC (permalink / raw) To: Rob Herring Cc: Stephan Gerhold, Andy Gross, Frank Rowand, linux-arm-msm, devicetree, linux-kernel On Tue 28 Sep 14:51 CDT 2021, Rob Herring wrote: > On Tue, Sep 28, 2021 at 12:49 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote: > > > > > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > > > > multiple chunks of memory, so SMEM was described as a standalone node > > > > > with references to its various memory regions. > > > > > > > > > > But practically all modern Qualcomm platforms has a single reserved memory > > > > > region used for SMEM. So rather than having to use two nodes to describe > > > > > the one SMEM region, update the binding to allow the reserved-memory > > > > > region alone to describe SMEM. > > > > > > > > > > The olden format is preserved as valid, as this is widely used already. > > > > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > --- > > > > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > > index f7e17713b3d8..4149cf2b66be 100644 > > > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > > [...] > > > > > @@ -43,6 +55,20 @@ examples: > > > > > #size-cells = <1>; > > > > > ranges; > > > > > > > > > > + smem@fa00000 { > > > > > > > > I think this is a good opportunity to make a decision which node name > > > > should be used here. :) > > > > > > reserved-memory node names are kind of a mess, so I haven't tried for > > > any standard... It needs to be solved globally. > > > > > > > I'd be happy to paint the shed any color you decide :) > > I didn't ask for it to be painted. Unless it is for everyone, I don't > care unless there's some clear pattern used already. > As Stephan indicated, I feel that I'll set precedence when I change "memory" -> "smem" in the last patch. > > That said, the binding itself doesn't mandate any node name, so it's > > just the example here that would be "wrong" - and just as wrong as it > > currently is. > > The example is right. The dts is wrong. > But I can't both not paint the node and resolve the fact that the dts is wrong. So which one should I go with? Should we leave the node name as is until we've decided what to do with the reserved-memory children? Or should I start accepting patches that changes "memory" to a list of non-generic names? > Perhaps we need a schema for 'any node name that doesn't match already > defined ones'. > > > > > You use smem@ here but mentioned before that you think using the generic > > > > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > > > > > > > - smem_mem: memory@86000000 { > > > > + memory@86000000 { > > > > + compatible = "qcom,smem"; > > > > reg = <0x0 0x86000000 0 0x200000>; > > > > no-map; > > > > + hwlocks = <&tcsr_mutex 3>; > > > > }; > > > > > > > > However, if you would use memory@ as example in this DT schema, > > > > Rob's bot would complain with the same error that I mentioned earlier [2]: > > > > > > > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > > > > From schema: dtschema/schemas/memory.yaml > > > > > > > > We should either fix the error when using memory@ or start using some > > > > different node name (Stephen Boyd suggested shared-memory@ for example). > > > > Otherwise we'll just keep introducing more and more dtbs_check errors > > > > for the Qualcomm device trees. > > > > > > A different node name. A node name should only have 1 meaning and > > > 'memory' is already defined. > > > > > > The main issue here is what to name nodes with only a size and no address. > > > > > > > This particular node has both address and size (as does all of the other > > reserved-memory regions we use upstream today)... > > I'm not talking about *just* QCom. If we define something here, it's > got to cover everyone. > > In summary, you can't use 'memory' or anything other established, > standard node name. > I know that "memory" is wrong, but I'm not sure about what you're asking me to do. Regards, Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional 2021-09-28 4:45 [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Bjorn Andersson ` (2 preceding siblings ...) 2021-09-28 10:22 ` [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Stephan Gerhold @ 2021-09-28 12:28 ` Rob Herring 3 siblings, 0 replies; 9+ messages in thread From: Rob Herring @ 2021-09-28 12:28 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, linux-kernel, linux-arm-msm, Rob Herring, Frank Rowand, devicetree On Mon, 27 Sep 2021 21:45:44 -0700, Bjorn Andersson wrote: > In the olden days the Qualcomm shared memory (SMEM) region consisted of > multiple chunks of memory, so SMEM was described as a standalone node > with references to its various memory regions. > > But practically all modern Qualcomm platforms has a single reserved memory > region used for SMEM. So rather than having to use two nodes to describe > the one SMEM region, update the binding to allow the reserved-memory > region alone to describe SMEM. > > The olden format is preserved as valid, as this is widely used already. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > 1 file changed, 30 insertions(+), 4 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/soc/qcom/qcom,smem.example.dt.yaml:0:0: /example-1/soc/sram@fc428000: failed to match any schema with compatible: ['qcom,rpm-msg-ram'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1533702 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-28 22:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-28 4:45 [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Bjorn Andersson 2021-09-28 4:45 ` [PATCH 2/3] soc: qcom: smem: Support reserved-memory description Bjorn Andersson 2021-09-28 4:45 ` [PATCH 3/3] arm64: dts: qcom: sdm845: Drop standalone smem node Bjorn Andersson 2021-09-28 10:22 ` [PATCH 1/3] dt-bindings: soc: smem: Make indirection optional Stephan Gerhold 2021-09-28 17:34 ` Rob Herring 2021-09-28 17:49 ` Bjorn Andersson 2021-09-28 19:51 ` Rob Herring 2021-09-28 22:06 ` Bjorn Andersson 2021-09-28 12:28 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).