| Message ID | 20210930182111.57353-4-bjorn.andersson@linaro.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | dt-bindings: soc: smem: Make indirection optional | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi Bjorn, On 9/30/21 9:21 PM, Bjorn Andersson wrote: > 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> > --- > > Changes since v1: > - None > > 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; Just a nitpicking, the index can be unsigned. > > 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 || > -- Best wishes, Vladimir
On Fri 01 Oct 00:08 PDT 2021, Vladimir Zapolskiy wrote: > On 9/30/21 9:21 PM, Bjorn Andersson wrote: [..] > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c [..] > > @@ -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; > > Just a nitpicking, the index can be unsigned. > It's compared against the "num_regions", which is also signed. So I think it should be signed. [..] > > + for (i = 0; i < num_regions; i++) { Thanks for the review! Regards, Bjorn
On Thu, Sep 30, 2021 at 11:21:10AM -0700, Bjorn Andersson wrote: > 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> > --- > > Changes since v1: > - None > > drivers/of/platform.c | 1 + Acked-by: Rob Herring <robh@kernel.org> > 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 ||
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> --- Changes since v1: - None drivers/of/platform.c | 1 + drivers/soc/qcom/smem.c | 57 ++++++++++++++++++++++++++++------------- 2 files changed, 40 insertions(+), 18 deletions(-)