* [PATCH v4 0/5] remoteproc: qcom: PIL info support @ 2020-03-10 6:33 Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding Bjorn Andersson ` (4 more replies) 0 siblings, 5 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 6:33 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd Introduce support for filling out the relocation information in IMEM, to aid post mortem debug tools to locate the various remoteprocs. Bjorn Andersson (5): dt-bindings: remoteproc: Add Qualcomm PIL info binding remoteproc: qcom: Introduce driver to store pil info in IMEM remoteproc: qcom: Update PIL relocation info on load arm64: dts: qcom: qcs404: Add IMEM and PIL info region arm64: dts: qcom: sdm845: Add IMEM and PIL info region .../bindings/remoteproc/qcom,pil-info.yaml | 44 +++++ arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 ++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 15 ++ drivers/remoteproc/Kconfig | 6 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/qcom_pil_info.c | 180 ++++++++++++++++++ drivers/remoteproc/qcom_pil_info.h | 8 + drivers/remoteproc/qcom_q6v5_adsp.c | 20 +- drivers/remoteproc/qcom_q6v5_mss.c | 7 + drivers/remoteproc/qcom_q6v5_pas.c | 19 +- drivers/remoteproc/qcom_q6v5_wcss.c | 14 +- drivers/remoteproc/qcom_wcnss.c | 18 +- 12 files changed, 335 insertions(+), 12 deletions(-) create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml create mode 100644 drivers/remoteproc/qcom_pil_info.c create mode 100644 drivers/remoteproc/qcom_pil_info.h -- 2.24.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding 2020-03-10 6:33 [PATCH v4 0/5] remoteproc: qcom: PIL info support Bjorn Andersson @ 2020-03-10 6:33 ` Bjorn Andersson 2020-03-10 17:54 ` Stephen Boyd 2020-03-10 18:37 ` Rob Herring 2020-03-10 6:33 ` [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM Bjorn Andersson ` (3 subsequent siblings) 4 siblings, 2 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 6:33 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd Add a devicetree binding for the Qualcomm peripheral image loader relocation information region found in the IMEM. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Change since v3: - Fixed spelling mistakes pointed out by Mathieu - Fixed description as requested by Stephen - Specify unit address in example - Add missing ranges in example .../bindings/remoteproc/qcom,pil-info.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml new file mode 100644 index 000000000000..44d87fd2a07e --- /dev/null +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/remoteproc/qcom,pil-info.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm peripheral image loader relocation info binding + +maintainers: + - Bjorn Andersson <bjorn.andersson@linaro.org> + +description: + The Qualcomm peripheral image loader relocation memory region, in IMEM, is + used for communicating remoteproc relocation information to post mortem + debugging tools. + +properties: + compatible: + const: qcom,pil-reloc-info + + reg: + maxItems: 1 + +required: + - compatible + - reg + +examples: + - | + imem@146bf000 { + compatible = "syscon", "simple-mfd"; + reg = <0 0x146bf000 0 0x1000>; + + #address-cells = <1>; + #size-cells = <1>; + + ranges = <0 0 0x146bf000 0x1000>; + + pil-reloc@94c { + compatible = "qcom,pil-reloc-info"; + reg = <0x94c 0xc8>; + }; + }; +... -- 2.24.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding 2020-03-10 6:33 ` [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding Bjorn Andersson @ 2020-03-10 17:54 ` Stephen Boyd 2020-03-10 18:37 ` Rob Herring 1 sibling, 0 replies; 20+ messages in thread From: Stephen Boyd @ 2020-03-10 17:54 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier Quoting Bjorn Andersson (2020-03-09 23:33:34) > Add a devicetree binding for the Qualcomm peripheral image loader > relocation information region found in the IMEM. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding 2020-03-10 6:33 ` [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding Bjorn Andersson @ 2020-03-10 18:37 ` Rob Herring 2020-03-10 18:37 ` Rob Herring 1 sibling, 0 replies; 20+ messages in thread From: Rob Herring @ 2020-03-10 18:37 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Ohad Ben-Cohen, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd On Mon, 9 Mar 2020 23:33:34 -0700, Bjorn Andersson wrote: > Add a devicetree binding for the Qualcomm peripheral image loader > relocation information region found in the IMEM. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Change since v3: > - Fixed spelling mistakes pointed out by Mathieu > - Fixed description as requested by Stephen > - Specify unit address in example > - Add missing ranges in example > > .../bindings/remoteproc/qcom,pil-info.yaml | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/remoteproc/qcom,pil-info.example.dts:24.11-44: Warning (ranges_format): /example-0/imem@146bf000:ranges: "ranges" property has invalid length (16 bytes) (parent #address-cells == 1, child #address-cells == 1, #size-cells == 1) See https://patchwork.ozlabs.org/patch/1251981 Please check and re-submit. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding @ 2020-03-10 18:37 ` Rob Herring 0 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2020-03-10 18:37 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd On Mon, 9 Mar 2020 23:33:34 -0700, Bjorn Andersson wrote: > Add a devicetree binding for the Qualcomm peripheral image loader > relocation information region found in the IMEM. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Change since v3: > - Fixed spelling mistakes pointed out by Mathieu > - Fixed description as requested by Stephen > - Specify unit address in example > - Add missing ranges in example > > .../bindings/remoteproc/qcom,pil-info.yaml | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/remoteproc/qcom,pil-info.example.dts:24.11-44: Warning (ranges_format): /example-0/imem@146bf000:ranges: "ranges" property has invalid length (16 bytes) (parent #address-cells == 1, child #address-cells == 1, #size-cells == 1) See https://patchwork.ozlabs.org/patch/1251981 Please check and re-submit. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM 2020-03-10 6:33 [PATCH v4 0/5] remoteproc: qcom: PIL info support Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding Bjorn Andersson @ 2020-03-10 6:33 ` Bjorn Andersson 2020-03-10 20:29 ` Stephen Boyd 2020-03-10 6:33 ` [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load Bjorn Andersson ` (2 subsequent siblings) 4 siblings, 1 reply; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 6:33 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd A region in IMEM is used to communicate load addresses of remoteproc to post mortem debug tools. Implement a driver that can be used to store this information in order to enable these tools to process collected ramdumps. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - Don't carry shadow of all entries - Reworked indexing of entries in qcom_pil_info_store() - Marked global __read_mostly Stephen requested, in v3, that the driver should be turned into a library that, in the context of the remoteproc drivers, would resolve the pil info region and update an appropriate entry, preferably a statically assigned one. Unfortunately the entries must be packed, so it's not possible to pre-assign entries for each remoteproc, in case some of them aren't booted. Further more, it turns out that the IMEM region must be zero-initialized in order to have a reliable way to determining which entries are available. We therefor have the need for global mutual exclusion and a mechanism that is guaranteed to run before any clients attempt to update the content - so the previously proposed design is maintained. drivers/remoteproc/Kconfig | 3 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/qcom_pil_info.c | 180 +++++++++++++++++++++++++++++ drivers/remoteproc/qcom_pil_info.h | 8 ++ 4 files changed, 192 insertions(+) create mode 100644 drivers/remoteproc/qcom_pil_info.c create mode 100644 drivers/remoteproc/qcom_pil_info.h diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index de3862c15fcc..20c8194e610e 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -95,6 +95,9 @@ config KEYSTONE_REMOTEPROC It's safe to say N here if you're not interested in the Keystone DSPs or just want to use a bare minimum kernel. +config QCOM_PIL_INFO + tristate + config QCOM_RPROC_COMMON tristate diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index e30a1b15fbac..2ab32bd41b44 100644 --- a/drivers/remoteproc/Makefile +++ b/drivers/remoteproc/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o obj-$(CONFIG_WKUP_M3_RPROC) += wkup_m3_rproc.o obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o obj-$(CONFIG_KEYSTONE_REMOTEPROC) += keystone_remoteproc.o +obj-$(CONFIG_QCOM_PIL_INFO) += qcom_pil_info.o obj-$(CONFIG_QCOM_RPROC_COMMON) += qcom_common.o obj-$(CONFIG_QCOM_Q6V5_COMMON) += qcom_q6v5.o obj-$(CONFIG_QCOM_Q6V5_ADSP) += qcom_q6v5_adsp.o diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c new file mode 100644 index 000000000000..0ddfb2639b7f --- /dev/null +++ b/drivers/remoteproc/qcom_pil_info.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2019-2020 Linaro Ltd. + */ +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define PIL_RELOC_NAME_LEN 8 + +struct pil_reloc_entry { + char name[PIL_RELOC_NAME_LEN]; + __le64 base; + __le32 size; +} __packed; + +struct pil_reloc { + struct device *dev; + struct regmap *map; + size_t offset; + size_t num_entries; + size_t entry_size; +}; + +static struct pil_reloc *_reloc __read_mostly; +static DEFINE_MUTEX(reloc_mutex); + +/** + * qcom_pil_info_store() - store PIL information of image in IMEM + * @image: name of the image + * @base: base address of the loaded image + * @size: size of the loaded image + * + * Return: 0 on success, negative errno on failure + */ +int qcom_pil_info_store(const char *image, phys_addr_t base, size_t size) +{ + struct pil_reloc_entry entry; + unsigned int offset; + int selected = -1; + int ret; + int i; + + offset = _reloc->offset; + + mutex_lock(&reloc_mutex); + for (i = 0; i < _reloc->num_entries; i++) { + ret = regmap_bulk_read(_reloc->map, offset, &entry, _reloc->entry_size); + if (ret < 0) + continue; + + if (!entry.name[0]) { + if (selected == -1) + selected = offset; + continue; + } + + if (!strncmp(entry.name, image, sizeof(entry.name))) { + selected = offset; + goto found; + } + + offset += sizeof(entry); + } + + if (selected == -1) { + dev_warn(_reloc->dev, "insufficient PIL info slots\n"); + ret = -ENOMEM; + goto unlock; + } + +found: + strscpy(entry.name, image, ARRAY_SIZE(entry.name)); + entry.base = base; + entry.size = size; + + ret = regmap_bulk_write(_reloc->map, selected, &entry, _reloc->entry_size); +unlock: + mutex_unlock(&reloc_mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(qcom_pil_info_store); + +/** + * qcom_pil_info_available() - query if the pil info is probed + * + * Return: boolean indicating if the pil info device is probed + */ +bool qcom_pil_info_available(void) +{ + return !!_reloc; +} +EXPORT_SYMBOL_GPL(qcom_pil_info_available); + +static int pil_reloc_probe(struct platform_device *pdev) +{ + struct pil_reloc_entry entry = {0}; + struct pil_reloc *reloc; + struct resource *res; + struct resource imem; + unsigned int offset; + int ret; + int i; + + reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL); + if (!reloc) + return -ENOMEM; + + reloc->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -EINVAL; + + /* reloc->offset is relative to parent (IMEM) base address */ + ret = of_address_to_resource(pdev->dev.of_node->parent, 0, &imem); + if (ret < 0) + return ret; + + reloc->offset = res->start - imem.start; + reloc->num_entries = resource_size(res) / sizeof(entry); + + reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node); + if (IS_ERR(reloc->map)) + return PTR_ERR(reloc->map); + + ret = regmap_get_val_bytes(reloc->map); + if (ret < 0) + return -EINVAL; + reloc->entry_size = sizeof(entry) / ret; + + offset = reloc->offset; + for (i = 0; i < reloc->num_entries; i++, offset += sizeof(entry)) { + ret = regmap_bulk_write(reloc->map, offset, &entry, + reloc->entry_size); + if (ret < 0) + return ret; + } + + mutex_lock(&reloc_mutex); + _reloc = reloc; + mutex_unlock(&reloc_mutex); + + return 0; +} + +static int pil_reloc_remove(struct platform_device *pdev) +{ + mutex_lock(&reloc_mutex); + _reloc = NULL; + mutex_unlock(&reloc_mutex); + + return 0; +} + +static const struct of_device_id pil_reloc_of_match[] = { + { .compatible = "qcom,pil-reloc-info" }, + {} +}; +MODULE_DEVICE_TABLE(of, pil_reloc_of_match); + +static struct platform_driver pil_reloc_driver = { + .probe = pil_reloc_probe, + .remove = pil_reloc_remove, + .driver = { + .name = "qcom-pil-reloc-info", + .of_match_table = pil_reloc_of_match, + }, +}; +module_platform_driver(pil_reloc_driver); + +MODULE_DESCRIPTION("Qualcomm PIL relocation info"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/remoteproc/qcom_pil_info.h b/drivers/remoteproc/qcom_pil_info.h new file mode 100644 index 000000000000..93aaaca8aed2 --- /dev/null +++ b/drivers/remoteproc/qcom_pil_info.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __QCOM_PIL_INFO_H__ +#define __QCOM_PIL_INFO_H__ + +int qcom_pil_info_store(const char *image, phys_addr_t base, size_t size); +bool qcom_pil_info_available(void); + +#endif -- 2.24.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM 2020-03-10 6:33 ` [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM Bjorn Andersson @ 2020-03-10 20:29 ` Stephen Boyd 2020-03-10 21:27 ` Bjorn Andersson 0 siblings, 1 reply; 20+ messages in thread From: Stephen Boyd @ 2020-03-10 20:29 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier Quoting Bjorn Andersson (2020-03-09 23:33:35) > A region in IMEM is used to communicate load addresses of remoteproc to > post mortem debug tools. Implement a driver that can be used to store > this information in order to enable these tools to process collected > ramdumps. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v3: > - Don't carry shadow of all entries > - Reworked indexing of entries in qcom_pil_info_store() > - Marked global __read_mostly > > Stephen requested, in v3, that the driver should be turned into a library that, > in the context of the remoteproc drivers, would resolve the pil info region and > update an appropriate entry, preferably a statically assigned one. > > Unfortunately the entries must be packed, so it's not possible to pre-assign > entries for each remoteproc, in case some of them aren't booted. Further more, > it turns out that the IMEM region must be zero-initialized in order to have a > reliable way to determining which entries are available. > > We therefor have the need for global mutual exclusion and a mechanism that is > guaranteed to run before any clients attempt to update the content - so the > previously proposed design is maintained. The library could have a static variable that tracks when it's been called once, holds a lock to protect concurrent access and then zero initializes on the first call. > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c > new file mode 100644 > index 000000000000..0ddfb2639b7f > --- /dev/null > +++ b/drivers/remoteproc/qcom_pil_info.c > @@ -0,0 +1,180 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2019-2020 Linaro Ltd. > + */ > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#define PIL_RELOC_NAME_LEN 8 > + > +struct pil_reloc_entry { > + char name[PIL_RELOC_NAME_LEN]; > + __le64 base; > + __le32 size; Why are these little endian? Isn't regmap doing endian swaps? > +} __packed; Is __packed necessary? It looks like things are naturally aligned already. > + > +struct pil_reloc { > + struct device *dev; > + struct regmap *map; > + size_t offset; > + size_t num_entries; > + size_t entry_size; > +}; > + > +static struct pil_reloc *_reloc __read_mostly; > +static DEFINE_MUTEX(reloc_mutex); > + > +/** > + * qcom_pil_info_store() - store PIL information of image in IMEM > + * @image: name of the image > + * @base: base address of the loaded image > + * @size: size of the loaded image > + * > + * Return: 0 on success, negative errno on failure > + */ > +int qcom_pil_info_store(const char *image, phys_addr_t base, size_t size) > +{ > + struct pil_reloc_entry entry; > + unsigned int offset; > + int selected = -1; > + int ret; > + int i; > + > + offset = _reloc->offset; > + > + mutex_lock(&reloc_mutex); > + for (i = 0; i < _reloc->num_entries; i++) { > + ret = regmap_bulk_read(_reloc->map, offset, &entry, _reloc->entry_size); > + if (ret < 0) > + continue; > + > + if (!entry.name[0]) { > + if (selected == -1) > + selected = offset; > + continue; Why not goto found? > + } > + > + if (!strncmp(entry.name, image, sizeof(entry.name))) { > + selected = offset; > + goto found; Why not goto found_again? And then jump over the strscpy() in this case? > + } > + > + offset += sizeof(entry); > + } > + > + if (selected == -1) { Do we need this check? It should have been jumped over if it found it? > + dev_warn(_reloc->dev, "insufficient PIL info slots\n"); > + ret = -ENOMEM; > + goto unlock; > + } > + > +found: > + strscpy(entry.name, image, ARRAY_SIZE(entry.name)); > + entry.base = base; > + entry.size = size; Sparse should complain here. > + > + ret = regmap_bulk_write(_reloc->map, selected, &entry, _reloc->entry_size); It would make a lot more sense to me if this was written with plain readl/writel logic. Then I don't have to think about structs being stored in I/O memory regions, and instead I can concentrate on there being two 64-bit registers for name and base and one 32-bit register for the size. > +unlock: > + mutex_unlock(&reloc_mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(qcom_pil_info_store); > + > +/** > + * qcom_pil_info_available() - query if the pil info is probed > + * > + * Return: boolean indicating if the pil info device is probed > + */ > +bool qcom_pil_info_available(void) > +{ > + return !!_reloc; > +} > +EXPORT_SYMBOL_GPL(qcom_pil_info_available); > + > +static int pil_reloc_probe(struct platform_device *pdev) > +{ > + struct pil_reloc_entry entry = {0}; > + struct pil_reloc *reloc; > + struct resource *res; > + struct resource imem; > + unsigned int offset; > + int ret; > + int i; > + > + reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL); > + if (!reloc) > + return -ENOMEM; > + > + reloc->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -EINVAL; > + > + /* reloc->offset is relative to parent (IMEM) base address */ > + ret = of_address_to_resource(pdev->dev.of_node->parent, 0, &imem); > + if (ret < 0) > + return ret; > + > + reloc->offset = res->start - imem.start; > + reloc->num_entries = resource_size(res) / sizeof(entry); > + > + reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node); > + if (IS_ERR(reloc->map)) > + return PTR_ERR(reloc->map); > + > + ret = regmap_get_val_bytes(reloc->map); > + if (ret < 0) > + return -EINVAL; > + reloc->entry_size = sizeof(entry) / ret; > + > + offset = reloc->offset; > + for (i = 0; i < reloc->num_entries; i++, offset += sizeof(entry)) { > + ret = regmap_bulk_write(reloc->map, offset, &entry, > + reloc->entry_size); > + if (ret < 0) > + return ret; > + } This would be a lot easier to read with a memset_io(). > + > + mutex_lock(&reloc_mutex); > + _reloc = reloc; > + mutex_unlock(&reloc_mutex); > + > + return 0; > +} > + > +static int pil_reloc_remove(struct platform_device *pdev) > +{ > + mutex_lock(&reloc_mutex); > + _reloc = NULL; > + mutex_unlock(&reloc_mutex); > + > + return 0; > +} > + > +static const struct of_device_id pil_reloc_of_match[] = { > + { .compatible = "qcom,pil-reloc-info" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, pil_reloc_of_match); > + > +static struct platform_driver pil_reloc_driver = { I still don't understand the need for a platform device driver and probe ordering. It's not a device in the usual ways, it's just some memory that's lying around and always there! Why can't we search in DT for the imem node and then find the pil reloc info compatible string on the first call to this library? Then we don't need an API to see if the device has probed yet (qcom_pil_info_available) and we can just ioremap some region of memory that's carved out for this reason. Forcing everything through the regmap is mostly adding pain. > + .probe = pil_reloc_probe, ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM 2020-03-10 20:29 ` Stephen Boyd @ 2020-03-10 21:27 ` Bjorn Andersson 0 siblings, 0 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 21:27 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier On Tue 10 Mar 13:29 PDT 2020, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-09 23:33:35) > > A region in IMEM is used to communicate load addresses of remoteproc to > > post mortem debug tools. Implement a driver that can be used to store > > this information in order to enable these tools to process collected > > ramdumps. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v3: > > - Don't carry shadow of all entries > > - Reworked indexing of entries in qcom_pil_info_store() > > - Marked global __read_mostly > > > > Stephen requested, in v3, that the driver should be turned into a library that, > > in the context of the remoteproc drivers, would resolve the pil info region and > > update an appropriate entry, preferably a statically assigned one. > > > > Unfortunately the entries must be packed, so it's not possible to pre-assign > > entries for each remoteproc, in case some of them aren't booted. Further more, > > it turns out that the IMEM region must be zero-initialized in order to have a > > reliable way to determining which entries are available. > > > > We therefor have the need for global mutual exclusion and a mechanism that is > > guaranteed to run before any clients attempt to update the content - so the > > previously proposed design is maintained. > > The library could have a static variable that tracks when it's been > called once, holds a lock to protect concurrent access and then zero > initializes on the first call. > For the benefit of not having the is_available call then? I presume we still want the compatible on the node, so the core will still allocate a struct platform_device for us... (I'm okay with reworking it that way) > > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c > > new file mode 100644 > > index 000000000000..0ddfb2639b7f > > --- /dev/null > > +++ b/drivers/remoteproc/qcom_pil_info.c > > @@ -0,0 +1,180 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2019-2020 Linaro Ltd. > > + */ > > +#include <linux/kernel.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define PIL_RELOC_NAME_LEN 8 > > + > > +struct pil_reloc_entry { > > + char name[PIL_RELOC_NAME_LEN]; > > + __le64 base; > > + __le32 size; > > Why are these little endian? Isn't regmap doing endian swaps? > Ugh, you're right. The regmap is created with "default" endian and 32-bit word size by syscon. So presumably this won't work and I must have missed it when I dumped imem to check the end result. > > +} __packed; > > Is __packed necessary? It looks like things are naturally aligned > already. > No, it should be fine. > > + > > +struct pil_reloc { > > + struct device *dev; > > + struct regmap *map; > > + size_t offset; > > + size_t num_entries; > > + size_t entry_size; > > +}; > > + > > +static struct pil_reloc *_reloc __read_mostly; > > +static DEFINE_MUTEX(reloc_mutex); > > + > > +/** > > + * qcom_pil_info_store() - store PIL information of image in IMEM > > + * @image: name of the image > > + * @base: base address of the loaded image > > + * @size: size of the loaded image > > + * > > + * Return: 0 on success, negative errno on failure > > + */ > > +int qcom_pil_info_store(const char *image, phys_addr_t base, size_t size) > > +{ > > + struct pil_reloc_entry entry; > > + unsigned int offset; > > + int selected = -1; > > + int ret; > > + int i; > > + > > + offset = _reloc->offset; > > + > > + mutex_lock(&reloc_mutex); > > + for (i = 0; i < _reloc->num_entries; i++) { > > + ret = regmap_bulk_read(_reloc->map, offset, &entry, _reloc->entry_size); > > + if (ret < 0) > > + continue; > > + > > + if (!entry.name[0]) { > > + if (selected == -1) > > + selected = offset; > > + continue; > > Why not goto found? > Didn't think hard enough about it, but you're right - per the need for packing of entries, if we hit a !name[0] that means there won't be any matches further down the list. > > + } > > + > > + if (!strncmp(entry.name, image, sizeof(entry.name))) { > > + selected = offset; > > + goto found; > > Why not goto found_again? And then jump over the strscpy() in this case? > +1 > > + } > > + > > + offset += sizeof(entry); > > + } > > + > > + if (selected == -1) { > > Do we need this check? It should have been jumped over if it found it? > Per above reasoning this can now go. > > + dev_warn(_reloc->dev, "insufficient PIL info slots\n"); > > + ret = -ENOMEM; > > + goto unlock; > > + } > > + > > +found: > > + strscpy(entry.name, image, ARRAY_SIZE(entry.name)); > > + entry.base = base; > > + entry.size = size; > > Sparse should complain here. > You're right. > > + > > + ret = regmap_bulk_write(_reloc->map, selected, &entry, _reloc->entry_size); > > It would make a lot more sense to me if this was written with plain > readl/writel logic. Then I don't have to think about structs being > stored in I/O memory regions, and instead I can concentrate on there > being two 64-bit registers for name and base and one 32-bit register for > the size. > Seems like this has to change, based on the per-"register" endian handling in the regmap. > > +unlock: > > + mutex_unlock(&reloc_mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(qcom_pil_info_store); > > + > > +/** > > + * qcom_pil_info_available() - query if the pil info is probed > > + * > > + * Return: boolean indicating if the pil info device is probed > > + */ > > +bool qcom_pil_info_available(void) > > +{ > > + return !!_reloc; > > +} > > +EXPORT_SYMBOL_GPL(qcom_pil_info_available); > > + > > +static int pil_reloc_probe(struct platform_device *pdev) > > +{ > > + struct pil_reloc_entry entry = {0}; > > + struct pil_reloc *reloc; > > + struct resource *res; > > + struct resource imem; > > + unsigned int offset; > > + int ret; > > + int i; > > + > > + reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL); > > + if (!reloc) > > + return -ENOMEM; > > + > > + reloc->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -EINVAL; > > + > > + /* reloc->offset is relative to parent (IMEM) base address */ > > + ret = of_address_to_resource(pdev->dev.of_node->parent, 0, &imem); > > + if (ret < 0) > > + return ret; > > + > > + reloc->offset = res->start - imem.start; > > + reloc->num_entries = resource_size(res) / sizeof(entry); > > + > > + reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node); > > + if (IS_ERR(reloc->map)) > > + return PTR_ERR(reloc->map); > > + > > + ret = regmap_get_val_bytes(reloc->map); > > + if (ret < 0) > > + return -EINVAL; > > + reloc->entry_size = sizeof(entry) / ret; > > + > > + offset = reloc->offset; > > + for (i = 0; i < reloc->num_entries; i++, offset += sizeof(entry)) { > > + ret = regmap_bulk_write(reloc->map, offset, &entry, > > + reloc->entry_size); > > + if (ret < 0) > > + return ret; > > + } > > This would be a lot easier to read with a memset_io(). > Yes, indeed. > > + > > + mutex_lock(&reloc_mutex); > > + _reloc = reloc; > > + mutex_unlock(&reloc_mutex); > > + > > + return 0; > > +} > > + > > +static int pil_reloc_remove(struct platform_device *pdev) > > +{ > > + mutex_lock(&reloc_mutex); > > + _reloc = NULL; > > + mutex_unlock(&reloc_mutex); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id pil_reloc_of_match[] = { > > + { .compatible = "qcom,pil-reloc-info" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, pil_reloc_of_match); > > + > > +static struct platform_driver pil_reloc_driver = { > > I still don't understand the need for a platform device driver and probe > ordering. It's not a device in the usual ways, it's just some memory > that's lying around and always there! I agree, but the device will be created regardless of us attaching to it or not - following the fact that it's used to deal with reboot reasons on some platforms. > Why can't we search in DT for the > imem node and then find the pil reloc info compatible string on the > first call to this library? Then we don't need an API to see if the > device has probed yet (qcom_pil_info_available) I think this sounds reasonable. > and we can just ioremap > some region of memory that's carved out for this reason. Forcing > everything through the regmap is mostly adding pain. > My concern here was simply that we'll end up ioremapping various small chunks of the imem region 10 (or so) times. But I agree that things would be cleaner here. Regards, Bjorn > > + .probe = pil_reloc_probe, ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM @ 2020-03-10 21:27 ` Bjorn Andersson 0 siblings, 0 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 21:27 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier On Tue 10 Mar 13:29 PDT 2020, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-09 23:33:35) > > A region in IMEM is used to communicate load addresses of remoteproc to > > post mortem debug tools. Implement a driver that can be used to store > > this information in order to enable these tools to process collected > > ramdumps. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v3: > > - Don't carry shadow of all entries > > - Reworked indexing of entries in qcom_pil_info_store() > > - Marked global __read_mostly > > > > Stephen requested, in v3, that the driver should be turned into a library that, > > in the context of the remoteproc drivers, would resolve the pil info region and > > update an appropriate entry, preferably a statically assigned one. > > > > Unfortunately the entries must be packed, so it's not possible to pre-assign > > entries for each remoteproc, in case some of them aren't booted. Further more, > > it turns out that the IMEM region must be zero-initialized in order to have a > > reliable way to determining which entries are available. > > > > We therefor have the need for global mutual exclusion and a mechanism that is > > guaranteed to run before any clients attempt to update the content - so the > > previously proposed design is maintained. > > The library could have a static variable that tracks when it's been > called once, holds a lock to protect concurrent access and then zero > initializes on the first call. > For the benefit of not having the is_available call then? I presume we still want the compatible on the node, so the core will still allocate a struct platform_device for us... (I'm okay with reworking it that way) > > diff --git a/drivers/remoteproc/qcom_pil_info.c b/drivers/remoteproc/qcom_pil_info.c > > new file mode 100644 > > index 000000000000..0ddfb2639b7f > > --- /dev/null > > +++ b/drivers/remoteproc/qcom_pil_info.c > > @@ -0,0 +1,180 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2019-2020 Linaro Ltd. > > + */ > > +#include <linux/kernel.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define PIL_RELOC_NAME_LEN 8 > > + > > +struct pil_reloc_entry { > > + char name[PIL_RELOC_NAME_LEN]; > > + __le64 base; > > + __le32 size; > > Why are these little endian? Isn't regmap doing endian swaps? > Ugh, you're right. The regmap is created with "default" endian and 32-bit word size by syscon. So presumably this won't work and I must have missed it when I dumped imem to check the end result. > > +} __packed; > > Is __packed necessary? It looks like things are naturally aligned > already. > No, it should be fine. > > + > > +struct pil_reloc { > > + struct device *dev; > > + struct regmap *map; > > + size_t offset; > > + size_t num_entries; > > + size_t entry_size; > > +}; > > + > > +static struct pil_reloc *_reloc __read_mostly; > > +static DEFINE_MUTEX(reloc_mutex); > > + > > +/** > > + * qcom_pil_info_store() - store PIL information of image in IMEM > > + * @image: name of the image > > + * @base: base address of the loaded image > > + * @size: size of the loaded image > > + * > > + * Return: 0 on success, negative errno on failure > > + */ > > +int qcom_pil_info_store(const char *image, phys_addr_t base, size_t size) > > +{ > > + struct pil_reloc_entry entry; > > + unsigned int offset; > > + int selected = -1; > > + int ret; > > + int i; > > + > > + offset = _reloc->offset; > > + > > + mutex_lock(&reloc_mutex); > > + for (i = 0; i < _reloc->num_entries; i++) { > > + ret = regmap_bulk_read(_reloc->map, offset, &entry, _reloc->entry_size); > > + if (ret < 0) > > + continue; > > + > > + if (!entry.name[0]) { > > + if (selected == -1) > > + selected = offset; > > + continue; > > Why not goto found? > Didn't think hard enough about it, but you're right - per the need for packing of entries, if we hit a !name[0] that means there won't be any matches further down the list. > > + } > > + > > + if (!strncmp(entry.name, image, sizeof(entry.name))) { > > + selected = offset; > > + goto found; > > Why not goto found_again? And then jump over the strscpy() in this case? > +1 > > + } > > + > > + offset += sizeof(entry); > > + } > > + > > + if (selected == -1) { > > Do we need this check? It should have been jumped over if it found it? > Per above reasoning this can now go. > > + dev_warn(_reloc->dev, "insufficient PIL info slots\n"); > > + ret = -ENOMEM; > > + goto unlock; > > + } > > + > > +found: > > + strscpy(entry.name, image, ARRAY_SIZE(entry.name)); > > + entry.base = base; > > + entry.size = size; > > Sparse should complain here. > You're right. > > + > > + ret = regmap_bulk_write(_reloc->map, selected, &entry, _reloc->entry_size); > > It would make a lot more sense to me if this was written with plain > readl/writel logic. Then I don't have to think about structs being > stored in I/O memory regions, and instead I can concentrate on there > being two 64-bit registers for name and base and one 32-bit register for > the size. > Seems like this has to change, based on the per-"register" endian handling in the regmap. > > +unlock: > > + mutex_unlock(&reloc_mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(qcom_pil_info_store); > > + > > +/** > > + * qcom_pil_info_available() - query if the pil info is probed > > + * > > + * Return: boolean indicating if the pil info device is probed > > + */ > > +bool qcom_pil_info_available(void) > > +{ > > + return !!_reloc; > > +} > > +EXPORT_SYMBOL_GPL(qcom_pil_info_available); > > + > > +static int pil_reloc_probe(struct platform_device *pdev) > > +{ > > + struct pil_reloc_entry entry = {0}; > > + struct pil_reloc *reloc; > > + struct resource *res; > > + struct resource imem; > > + unsigned int offset; > > + int ret; > > + int i; > > + > > + reloc = devm_kzalloc(&pdev->dev, sizeof(*reloc), GFP_KERNEL); > > + if (!reloc) > > + return -ENOMEM; > > + > > + reloc->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -EINVAL; > > + > > + /* reloc->offset is relative to parent (IMEM) base address */ > > + ret = of_address_to_resource(pdev->dev.of_node->parent, 0, &imem); > > + if (ret < 0) > > + return ret; > > + > > + reloc->offset = res->start - imem.start; > > + reloc->num_entries = resource_size(res) / sizeof(entry); > > + > > + reloc->map = syscon_node_to_regmap(pdev->dev.parent->of_node); > > + if (IS_ERR(reloc->map)) > > + return PTR_ERR(reloc->map); > > + > > + ret = regmap_get_val_bytes(reloc->map); > > + if (ret < 0) > > + return -EINVAL; > > + reloc->entry_size = sizeof(entry) / ret; > > + > > + offset = reloc->offset; > > + for (i = 0; i < reloc->num_entries; i++, offset += sizeof(entry)) { > > + ret = regmap_bulk_write(reloc->map, offset, &entry, > > + reloc->entry_size); > > + if (ret < 0) > > + return ret; > > + } > > This would be a lot easier to read with a memset_io(). > Yes, indeed. > > + > > + mutex_lock(&reloc_mutex); > > + _reloc = reloc; > > + mutex_unlock(&reloc_mutex); > > + > > + return 0; > > +} > > + > > +static int pil_reloc_remove(struct platform_device *pdev) > > +{ > > + mutex_lock(&reloc_mutex); > > + _reloc = NULL; > > + mutex_unlock(&reloc_mutex); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id pil_reloc_of_match[] = { > > + { .compatible = "qcom,pil-reloc-info" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, pil_reloc_of_match); > > + > > +static struct platform_driver pil_reloc_driver = { > > I still don't understand the need for a platform device driver and probe > ordering. It's not a device in the usual ways, it's just some memory > that's lying around and always there! I agree, but the device will be created regardless of us attaching to it or not - following the fact that it's used to deal with reboot reasons on some platforms. > Why can't we search in DT for the > imem node and then find the pil reloc info compatible string on the > first call to this library? Then we don't need an API to see if the > device has probed yet (qcom_pil_info_available) I think this sounds reasonable. > and we can just ioremap > some region of memory that's carved out for this reason. Forcing > everything through the regmap is mostly adding pain. > My concern here was simply that we'll end up ioremapping various small chunks of the imem region 10 (or so) times. But I agree that things would be cleaner here. Regards, Bjorn > > + .probe = pil_reloc_probe, ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM 2020-03-10 21:27 ` Bjorn Andersson (?) @ 2020-03-11 23:25 ` Stephen Boyd 2020-03-11 23:32 ` Bjorn Andersson -1 siblings, 1 reply; 20+ messages in thread From: Stephen Boyd @ 2020-03-11 23:25 UTC (permalink / raw) To: Bjorn Andersson Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier Quoting Bjorn Andersson (2020-03-10 14:27:28) > On Tue 10 Mar 13:29 PDT 2020, Stephen Boyd wrote: > > > Why can't we search in DT for the > > imem node and then find the pil reloc info compatible string on the > > first call to this library? Then we don't need an API to see if the > > device has probed yet (qcom_pil_info_available) > > I think this sounds reasonable. Great! > > > and we can just ioremap > > some region of memory that's carved out for this reason. Forcing > > everything through the regmap is mostly adding pain. > > > > My concern here was simply that we'll end up ioremapping various small > chunks of the imem region 10 (or so) times. But I agree that things > would be cleaner here. Alright. I'd like the ioremap() approach. ioremap() will "do the right thing" and reuse mappings if they're already there and overlap in the page. So it's OK that the syscon/simple-mfd exists and makes a device, etc. etc., but we don't need to care about it. We can just ioremap() the area and not worry that the regmap users may have a mapping to the same place. This is a dedicated carveout inside IMEM so we're safe from other meddling users. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM 2020-03-11 23:25 ` Stephen Boyd @ 2020-03-11 23:32 ` Bjorn Andersson 0 siblings, 0 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-11 23:32 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier On Wed 11 Mar 16:25 PDT 2020, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-10 14:27:28) > > On Tue 10 Mar 13:29 PDT 2020, Stephen Boyd wrote: > > > > > Why can't we search in DT for the > > > imem node and then find the pil reloc info compatible string on the > > > first call to this library? Then we don't need an API to see if the > > > device has probed yet (qcom_pil_info_available) > > > > I think this sounds reasonable. > > Great! > > > > > > and we can just ioremap > > > some region of memory that's carved out for this reason. Forcing > > > everything through the regmap is mostly adding pain. > > > > > > > My concern here was simply that we'll end up ioremapping various small > > chunks of the imem region 10 (or so) times. But I agree that things > > would be cleaner here. > > Alright. I'd like the ioremap() approach. ioremap() will "do the right > thing" and reuse mappings if they're already there and overlap in the > page. So it's OK that the syscon/simple-mfd exists and makes a device, > etc. etc., but we don't need to care about it. We can just ioremap() the > area and not worry that the regmap users may have a mapping to the same > place. This is a dedicated carveout inside IMEM so we're safe from other > meddling users. Agreed, thanks for the feedback! Regards, Bjorn ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM @ 2020-03-11 23:32 ` Bjorn Andersson 0 siblings, 0 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-11 23:32 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier On Wed 11 Mar 16:25 PDT 2020, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-10 14:27:28) > > On Tue 10 Mar 13:29 PDT 2020, Stephen Boyd wrote: > > > > > Why can't we search in DT for the > > > imem node and then find the pil reloc info compatible string on the > > > first call to this library? Then we don't need an API to see if the > > > device has probed yet (qcom_pil_info_available) > > > > I think this sounds reasonable. > > Great! > > > > > > and we can just ioremap > > > some region of memory that's carved out for this reason. Forcing > > > everything through the regmap is mostly adding pain. > > > > > > > My concern here was simply that we'll end up ioremapping various small > > chunks of the imem region 10 (or so) times. But I agree that things > > would be cleaner here. > > Alright. I'd like the ioremap() approach. ioremap() will "do the right > thing" and reuse mappings if they're already there and overlap in the > page. So it's OK that the syscon/simple-mfd exists and makes a device, > etc. etc., but we don't need to care about it. We can just ioremap() the > area and not worry that the regmap users may have a mapping to the same > place. This is a dedicated carveout inside IMEM so we're safe from other > meddling users. Agreed, thanks for the feedback! Regards, Bjorn ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load 2020-03-10 6:33 [PATCH v4 0/5] remoteproc: qcom: PIL info support Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM Bjorn Andersson @ 2020-03-10 6:33 ` Bjorn Andersson 2020-03-10 18:10 ` Stephen Boyd 2020-03-10 6:33 ` [PATCH v4 4/5] arm64: dts: qcom: qcs404: Add IMEM and PIL info region Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 5/5] arm64: dts: qcom: sdm845: " Bjorn Andersson 4 siblings, 1 reply; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 6:33 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd Update the PIL relocation information in IMEM with information about where the firmware for various remoteprocs are loaded. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - Added comment about the ignored return value - Added qcom_wcss.c hunk drivers/remoteproc/Kconfig | 3 +++ drivers/remoteproc/qcom_q6v5_adsp.c | 20 +++++++++++++++++--- drivers/remoteproc/qcom_q6v5_mss.c | 7 +++++++ drivers/remoteproc/qcom_q6v5_pas.c | 19 ++++++++++++++++--- drivers/remoteproc/qcom_q6v5_wcss.c | 14 +++++++++++--- drivers/remoteproc/qcom_wcnss.c | 18 +++++++++++++++--- 6 files changed, 69 insertions(+), 12 deletions(-) diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index 20c8194e610e..7f4834ab06c2 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -129,6 +129,7 @@ config QCOM_Q6V5_MSS depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n depends on QCOM_SYSMON || QCOM_SYSMON=n select MFD_SYSCON + select QCOM_PIL_INFO select QCOM_MDT_LOADER select QCOM_Q6V5_COMMON select QCOM_RPROC_COMMON @@ -145,6 +146,7 @@ config QCOM_Q6V5_PAS depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n depends on QCOM_SYSMON || QCOM_SYSMON=n select MFD_SYSCON + select QCOM_PIL_INFO select QCOM_MDT_LOADER select QCOM_Q6V5_COMMON select QCOM_RPROC_COMMON @@ -193,6 +195,7 @@ config QCOM_WCNSS_PIL depends on QCOM_SMEM depends on QCOM_SYSMON || QCOM_SYSMON=n select QCOM_MDT_LOADER + select QCOM_PIL_INFO select QCOM_RPROC_COMMON select QCOM_SCM help diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c index e953886b2eb7..d5cdff942535 100644 --- a/drivers/remoteproc/qcom_q6v5_adsp.c +++ b/drivers/remoteproc/qcom_q6v5_adsp.c @@ -26,6 +26,7 @@ #include <linux/soc/qcom/smem_state.h> #include "qcom_common.h" +#include "qcom_pil_info.h" #include "qcom_q6v5.h" #include "remoteproc_internal.h" @@ -82,6 +83,7 @@ struct qcom_adsp { unsigned int halt_lpass; int crash_reason_smem; + const char *info_name; struct completion start_done; struct completion stop_done; @@ -164,10 +166,18 @@ static int qcom_adsp_shutdown(struct qcom_adsp *adsp) static int adsp_load(struct rproc *rproc, const struct firmware *fw) { struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; + int ret; + + ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, + adsp->mem_region, adsp->mem_phys, + adsp->mem_size, &adsp->mem_reloc); + if (ret) + return ret; - return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, - adsp->mem_region, adsp->mem_phys, adsp->mem_size, - &adsp->mem_reloc); + /* Failures only affect post mortem debugging, so ignore return value */ + qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size); + + return 0; } static int adsp_start(struct rproc *rproc) @@ -413,6 +423,9 @@ static int adsp_probe(struct platform_device *pdev) struct rproc *rproc; int ret; + if (!qcom_pil_info_available()) + return -EPROBE_DEFER; + desc = of_device_get_match_data(&pdev->dev); if (!desc) return -EINVAL; @@ -427,6 +440,7 @@ static int adsp_probe(struct platform_device *pdev) adsp = (struct qcom_adsp *)rproc->priv; adsp->dev = &pdev->dev; adsp->rproc = rproc; + adsp->info_name = desc->sysmon_name; platform_set_drvdata(pdev, adsp); ret = adsp_alloc_memory_region(adsp); diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index a1cc9cbe038f..afebbe3c582c 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -28,6 +28,7 @@ #include "remoteproc_internal.h" #include "qcom_common.h" +#include "qcom_pil_info.h" #include "qcom_q6v5.h" #include <linux/qcom_scm.h> @@ -1166,6 +1167,9 @@ static int q6v5_mpss_load(struct q6v5 *qproc) else if (ret < 0) dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret); + /* Failures only affect post mortem debugging, so ignore return value */ + qcom_pil_info_store("modem", mpss_reloc, qproc->mpss_size); + release_firmware: release_firmware(fw); out: @@ -1555,6 +1559,9 @@ static int q6v5_probe(struct platform_device *pdev) if (desc->need_mem_protection && !qcom_scm_is_available()) return -EPROBE_DEFER; + if (!qcom_pil_info_available()) + return -EPROBE_DEFER; + mba_image = desc->hexagon_mba_image; ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name", 0, &mba_image); diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c index edf9d0e1a235..e64c268e6113 100644 --- a/drivers/remoteproc/qcom_q6v5_pas.c +++ b/drivers/remoteproc/qcom_q6v5_pas.c @@ -25,6 +25,7 @@ #include <linux/soc/qcom/smem_state.h> #include "qcom_common.h" +#include "qcom_pil_info.h" #include "qcom_q6v5.h" #include "remoteproc_internal.h" @@ -64,6 +65,7 @@ struct qcom_adsp { int pas_id; int crash_reason_smem; bool has_aggre2_clk; + const char *info_name; struct completion start_done; struct completion stop_done; @@ -117,11 +119,18 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds, static int adsp_load(struct rproc *rproc, const struct firmware *fw) { struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; + int ret; + + ret = qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id, + adsp->mem_region, adsp->mem_phys, adsp->mem_size, + &adsp->mem_reloc); + if (ret) + return ret; - return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id, - adsp->mem_region, adsp->mem_phys, adsp->mem_size, - &adsp->mem_reloc); + /* Failures only affect post mortem debugging, so ignore return value */ + qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size); + return 0; } static int adsp_start(struct rproc *rproc) @@ -376,6 +385,9 @@ static int adsp_probe(struct platform_device *pdev) if (!qcom_scm_is_available()) return -EPROBE_DEFER; + if (!qcom_pil_info_available()) + return -EPROBE_DEFER; + fw_name = desc->firmware_name; ret = of_property_read_string(pdev->dev.of_node, "firmware-name", &fw_name); @@ -396,6 +408,7 @@ static int adsp_probe(struct platform_device *pdev) adsp->rproc = rproc; adsp->pas_id = desc->pas_id; adsp->has_aggre2_clk = desc->has_aggre2_clk; + adsp->info_name = desc->sysmon_name; platform_set_drvdata(pdev, adsp); ret = adsp_alloc_memory_region(adsp); diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c index f93e1e4a1cc0..f76b7feccf25 100644 --- a/drivers/remoteproc/qcom_q6v5_wcss.c +++ b/drivers/remoteproc/qcom_q6v5_wcss.c @@ -421,10 +421,18 @@ static void *q6v5_wcss_da_to_va(struct rproc *rproc, u64 da, int len) static int q6v5_wcss_load(struct rproc *rproc, const struct firmware *fw) { struct q6v5_wcss *wcss = rproc->priv; + int ret; + + ret = qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware, + 0, wcss->mem_region, wcss->mem_phys, + wcss->mem_size, &wcss->mem_reloc); + if (ret) + return ret; + + /* Failures only affect post mortem debugging, so ignore return value */ + qcom_pil_info_store("wcnss", wcss->mem_reloc, wcss->mem_size); - return qcom_mdt_load_no_init(wcss->dev, fw, rproc->firmware, - 0, wcss->mem_region, wcss->mem_phys, - wcss->mem_size, &wcss->mem_reloc); + return ret; } static const struct rproc_ops q6v5_wcss_ops = { diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c index dc135754bb9c..c162cda1154f 100644 --- a/drivers/remoteproc/qcom_wcnss.c +++ b/drivers/remoteproc/qcom_wcnss.c @@ -27,6 +27,7 @@ #include "qcom_common.h" #include "remoteproc_internal.h" +#include "qcom_pil_info.h" #include "qcom_wcnss.h" #define WCNSS_CRASH_REASON_SMEM 422 @@ -145,10 +146,18 @@ void qcom_wcnss_assign_iris(struct qcom_wcnss *wcnss, static int wcnss_load(struct rproc *rproc, const struct firmware *fw) { struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv; + int ret; + + ret = qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID, + wcnss->mem_region, wcnss->mem_phys, + wcnss->mem_size, &wcnss->mem_reloc); + if (ret) + return ret; - return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID, - wcnss->mem_region, wcnss->mem_phys, - wcnss->mem_size, &wcnss->mem_reloc); + /* Failures only affect post mortem debugging, so ignore return value */ + qcom_pil_info_store("wcnss", wcnss->mem_reloc, wcnss->mem_size); + + return 0; } static void wcnss_indicate_nv_download(struct qcom_wcnss *wcnss) @@ -469,6 +478,9 @@ static int wcnss_probe(struct platform_device *pdev) if (!qcom_scm_is_available()) return -EPROBE_DEFER; + if (!qcom_pil_info_available()) + return -EPROBE_DEFER; + if (!qcom_scm_pas_supported(WCNSS_PAS_ID)) { dev_err(&pdev->dev, "PAS is not available for WCNSS\n"); return -ENXIO; -- 2.24.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load 2020-03-10 6:33 ` [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load Bjorn Andersson @ 2020-03-10 18:10 ` Stephen Boyd 2020-03-10 19:20 ` Bjorn Andersson 0 siblings, 1 reply; 20+ messages in thread From: Stephen Boyd @ 2020-03-10 18:10 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier Quoting Bjorn Andersson (2020-03-09 23:33:36) > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c > index e953886b2eb7..d5cdff942535 100644 > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > @@ -164,10 +166,18 @@ static int qcom_adsp_shutdown(struct qcom_adsp *adsp) > static int adsp_load(struct rproc *rproc, const struct firmware *fw) > { > struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; > + int ret; > + > + ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, > + adsp->mem_region, adsp->mem_phys, > + adsp->mem_size, &adsp->mem_reloc); > + if (ret) > + return ret; > > - return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, > - adsp->mem_region, adsp->mem_phys, adsp->mem_size, > - &adsp->mem_reloc); > + /* Failures only affect post mortem debugging, so ignore return value */ > + qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size); If the return value was void then the comment wouldn't be necessary and it would be self documenting as such. Can we do that? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load 2020-03-10 18:10 ` Stephen Boyd @ 2020-03-10 19:20 ` Bjorn Andersson 0 siblings, 0 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 19:20 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier On Tue 10 Mar 11:10 PDT 2020, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-09 23:33:36) > > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c > > index e953886b2eb7..d5cdff942535 100644 > > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > > @@ -164,10 +166,18 @@ static int qcom_adsp_shutdown(struct qcom_adsp *adsp) > > static int adsp_load(struct rproc *rproc, const struct firmware *fw) > > { > > struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; > > + int ret; > > + > > + ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, > > + adsp->mem_region, adsp->mem_phys, > > + adsp->mem_size, &adsp->mem_reloc); > > + if (ret) > > + return ret; > > > > - return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, > > - adsp->mem_region, adsp->mem_phys, adsp->mem_size, > > - &adsp->mem_reloc); > > + /* Failures only affect post mortem debugging, so ignore return value */ > > + qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size); > > If the return value was void then the comment wouldn't be necessary and > it would be self documenting as such. Can we do that? I started off with this in v1, but agreed with Mathieu to ignore the failures in the place where we actually don't care, rather than inside qcom_pil_info_store()... Regards, Bjorn ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load @ 2020-03-10 19:20 ` Bjorn Andersson 0 siblings, 0 replies; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 19:20 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Ohad Ben-Cohen, Rob Herring, linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier On Tue 10 Mar 11:10 PDT 2020, Stephen Boyd wrote: > Quoting Bjorn Andersson (2020-03-09 23:33:36) > > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c > > index e953886b2eb7..d5cdff942535 100644 > > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > > @@ -164,10 +166,18 @@ static int qcom_adsp_shutdown(struct qcom_adsp *adsp) > > static int adsp_load(struct rproc *rproc, const struct firmware *fw) > > { > > struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; > > + int ret; > > + > > + ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, > > + adsp->mem_region, adsp->mem_phys, > > + adsp->mem_size, &adsp->mem_reloc); > > + if (ret) > > + return ret; > > > > - return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0, > > - adsp->mem_region, adsp->mem_phys, adsp->mem_size, > > - &adsp->mem_reloc); > > + /* Failures only affect post mortem debugging, so ignore return value */ > > + qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, adsp->mem_size); > > If the return value was void then the comment wouldn't be necessary and > it would be self documenting as such. Can we do that? I started off with this in v1, but agreed with Mathieu to ignore the failures in the place where we actually don't care, rather than inside qcom_pil_info_store()... Regards, Bjorn ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 4/5] arm64: dts: qcom: qcs404: Add IMEM and PIL info region 2020-03-10 6:33 [PATCH v4 0/5] remoteproc: qcom: PIL info support Bjorn Andersson ` (2 preceding siblings ...) 2020-03-10 6:33 ` [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load Bjorn Andersson @ 2020-03-10 6:33 ` Bjorn Andersson 2020-03-10 18:11 ` Stephen Boyd 2020-03-10 6:33 ` [PATCH v4 5/5] arm64: dts: qcom: sdm845: " Bjorn Andersson 4 siblings, 1 reply; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 6:33 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd Add a simple-mfd representing IMEM on QCS404 and define the PIL relocation info region, so that post mortem tools will be able to locate the loaded remoteprocs. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - Added ranges - Made size in reg hex arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi index 1eea06435779..3dadcf1c968b 100644 --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi @@ -997,6 +997,21 @@ blsp2_spi0: spi@7af5000 { status = "disabled"; }; + imem@8600000 { + compatible = "syscon", "simple-mfd"; + reg = <0x08600000 0x1000>; + + #address-cells = <1>; + #size-cells = <1>; + + ranges = <0 0x08600000 0x1000>; + + pil-reloc@94c { + compatible = "qcom,pil-reloc-info"; + reg = <0x94c 0xc8>; + }; + }; + intc: interrupt-controller@b000000 { compatible = "qcom,msm-qgic2"; interrupt-controller; -- 2.24.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/5] arm64: dts: qcom: qcs404: Add IMEM and PIL info region 2020-03-10 6:33 ` [PATCH v4 4/5] arm64: dts: qcom: qcs404: Add IMEM and PIL info region Bjorn Andersson @ 2020-03-10 18:11 ` Stephen Boyd 0 siblings, 0 replies; 20+ messages in thread From: Stephen Boyd @ 2020-03-10 18:11 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier Quoting Bjorn Andersson (2020-03-09 23:33:37) > Add a simple-mfd representing IMEM on QCS404 and define the PIL > relocation info region, so that post mortem tools will be able to locate > the loaded remoteprocs. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 5/5] arm64: dts: qcom: sdm845: Add IMEM and PIL info region 2020-03-10 6:33 [PATCH v4 0/5] remoteproc: qcom: PIL info support Bjorn Andersson ` (3 preceding siblings ...) 2020-03-10 6:33 ` [PATCH v4 4/5] arm64: dts: qcom: qcs404: Add IMEM and PIL info region Bjorn Andersson @ 2020-03-10 6:33 ` Bjorn Andersson 2020-03-10 18:11 ` Stephen Boyd 4 siblings, 1 reply; 20+ messages in thread From: Bjorn Andersson @ 2020-03-10 6:33 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier, Stephen Boyd Add a simple-mfd representing IMEM on SDM845 and define the PIL relocation info region, so that post mortem tools will be able to locate the loaded remoteprocs. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - Added ranges - Made size in reg hex arch/arm64/boot/dts/qcom/sdm845.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index 061f49faab19..36ed6d8d0863 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -3254,6 +3254,21 @@ spmi_bus: spmi@c440000 { cell-index = <0>; }; + imem@146bf000 { + compatible = "syscon", "simple-mfd"; + reg = <0 0x146bf000 0 0x1000>; + + #address-cells = <1>; + #size-cells = <1>; + + ranges = <0 0 0x146bf000 0x1000>; + + pil-reloc@94c { + compatible = "qcom,pil-reloc-info"; + reg = <0x94c 0xc8>; + }; + }; + apps_smmu: iommu@15000000 { compatible = "qcom,sdm845-smmu-500", "arm,mmu-500"; reg = <0 0x15000000 0 0x80000>; -- 2.24.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 5/5] arm64: dts: qcom: sdm845: Add IMEM and PIL info region 2020-03-10 6:33 ` [PATCH v4 5/5] arm64: dts: qcom: sdm845: " Bjorn Andersson @ 2020-03-10 18:11 ` Stephen Boyd 0 siblings, 0 replies; 20+ messages in thread From: Stephen Boyd @ 2020-03-10 18:11 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Ohad Ben-Cohen, Rob Herring Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel, Mathieu Poirier Quoting Bjorn Andersson (2020-03-09 23:33:38) > Add a simple-mfd representing IMEM on SDM845 and define the PIL > relocation info region, so that post mortem tools will be able to locate > the loaded remoteprocs. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org> ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-03-11 23:33 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 6:33 [PATCH v4 0/5] remoteproc: qcom: PIL info support Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding Bjorn Andersson 2020-03-10 17:54 ` Stephen Boyd 2020-03-10 18:37 ` Rob Herring 2020-03-10 18:37 ` Rob Herring 2020-03-10 6:33 ` [PATCH v4 2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM Bjorn Andersson 2020-03-10 20:29 ` Stephen Boyd 2020-03-10 21:27 ` Bjorn Andersson 2020-03-10 21:27 ` Bjorn Andersson 2020-03-11 23:25 ` Stephen Boyd 2020-03-11 23:32 ` Bjorn Andersson 2020-03-11 23:32 ` Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 3/5] remoteproc: qcom: Update PIL relocation info on load Bjorn Andersson 2020-03-10 18:10 ` Stephen Boyd 2020-03-10 19:20 ` Bjorn Andersson 2020-03-10 19:20 ` Bjorn Andersson 2020-03-10 6:33 ` [PATCH v4 4/5] arm64: dts: qcom: qcs404: Add IMEM and PIL info region Bjorn Andersson 2020-03-10 18:11 ` Stephen Boyd 2020-03-10 6:33 ` [PATCH v4 5/5] arm64: dts: qcom: sdm845: " Bjorn Andersson 2020-03-10 18:11 ` Stephen Boyd
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.