[v4,2/5] remoteproc: qcom: Introduce driver to store pil info in IMEM
diff mbox series

Message ID 20200310063338.3344582-3-bjorn.andersson@linaro.org
State Superseded
Headers show
Series
  • remoteproc: qcom: PIL info support
Related show

Commit Message

Bjorn Andersson March 10, 2020, 6:33 a.m. UTC
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

Comments

Stephen Boyd March 10, 2020, 8:29 p.m. UTC | #1
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,
Bjorn Andersson March 10, 2020, 9:27 p.m. UTC | #2
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,
Stephen Boyd March 11, 2020, 11:25 p.m. UTC | #3
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.
Bjorn Andersson March 11, 2020, 11:32 p.m. UTC | #4
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

Patch
diff mbox series

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