Extend parse_fw callback to include SMC call to get resource
table from TrustZone to leverage resource table parse and
mapping and unmapping code reuse from the framework.
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
drivers/remoteproc/qcom_q6v5_pas.c | 33 +++++++++++++++++++++++++++--
drivers/soc/qcom/mdt_loader.c | 1 -
include/linux/soc/qcom/mdt_loader.h | 2 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 09cada92dfd5..1e0f09bf1ef2 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -408,6 +408,35 @@ static void *qcom_pas_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is
return pas->mem_region + offset;
}
+static int qcom_pas_parse_firmware(struct rproc *rproc, const struct firmware *fw)
+{
+ struct qcom_pas *pas = rproc->priv;
+ size_t output_rt_size = MAX_RSCTABLE_SIZE;
+ void *output_rt;
+ int ret;
+
+ ret = qcom_register_dump_segments(rproc, fw);
+ if (ret) {
+ dev_err(pas->dev, "Error in registering dump segments\n");
+ return ret;
+ }
+
+ if (!rproc->has_iommu)
+ return ret;
+
+ ret = qcom_scm_pas_get_rsc_table(pas->pas_id, NULL, 0, &output_rt, &output_rt_size);
+ if (ret) {
+ dev_err(pas->dev, "error %d getting resource_table\n", ret);
+ return ret;
+ }
+
+ rproc->cached_table = output_rt;
+ rproc->table_ptr = rproc->cached_table;
+ rproc->table_sz = output_rt_size;
+
+ return ret;
+}
+
static unsigned long qcom_pas_panic(struct rproc *rproc)
{
struct qcom_pas *pas = rproc->priv;
@@ -420,7 +449,7 @@ static const struct rproc_ops qcom_pas_ops = {
.start = qcom_pas_start,
.stop = qcom_pas_stop,
.da_to_va = qcom_pas_da_to_va,
- .parse_fw = qcom_register_dump_segments,
+ .parse_fw = qcom_pas_parse_firmware,
.load = qcom_pas_load,
.panic = qcom_pas_panic,
};
@@ -430,7 +459,7 @@ static const struct rproc_ops qcom_pas_minidump_ops = {
.start = qcom_pas_start,
.stop = qcom_pas_stop,
.da_to_va = qcom_pas_da_to_va,
- .parse_fw = qcom_register_dump_segments,
+ .parse_fw = qcom_pas_parse_firmware,
.load = qcom_pas_load,
.panic = qcom_pas_panic,
.coredump = qcom_pas_minidump,
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index ea7034c4b996..8456cca3f3e0 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -22,7 +22,6 @@
#include <linux/slab.h>
#include <linux/soc/qcom/mdt_loader.h>
-#define MAX_RSCTABLE_SIZE SZ_16K;
#define RSC_TABLE_HASH_BITS 5 // 32 buckets
DEFINE_HASHTABLE(qcom_pas_rsc_table_map, RSC_TABLE_HASH_BITS);
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index 62f239f64dfb..92ad862e733e 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -8,6 +8,8 @@
#define QCOM_MDT_TYPE_HASH (2 << 24)
#define QCOM_MDT_RELOCATABLE BIT(27)
+#define MAX_RSCTABLE_SIZE SZ_16K
+
struct device;
struct firmware;
struct qcom_scm_pas_ctx;
--
2.50.1On Tue, Aug 19, 2025 at 10:24:44PM +0530, Mukesh Ojha wrote:
> Extend parse_fw callback to include SMC call to get resource
> table from TrustZone to leverage resource table parse and
> mapping and unmapping code reuse from the framework.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
> drivers/remoteproc/qcom_q6v5_pas.c | 33 +++++++++++++++++++++++++++--
> drivers/soc/qcom/mdt_loader.c | 1 -
> include/linux/soc/qcom/mdt_loader.h | 2 ++
> 3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 09cada92dfd5..1e0f09bf1ef2 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -408,6 +408,35 @@ static void *qcom_pas_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is
> return pas->mem_region + offset;
> }
>
> +static int qcom_pas_parse_firmware(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct qcom_pas *pas = rproc->priv;
> + size_t output_rt_size = MAX_RSCTABLE_SIZE;
> + void *output_rt;
> + int ret;
> +
> + ret = qcom_register_dump_segments(rproc, fw);
> + if (ret) {
> + dev_err(pas->dev, "Error in registering dump segments\n");
> + return ret;
> + }
> +
> + if (!rproc->has_iommu)
> + return ret;
> +
> + ret = qcom_scm_pas_get_rsc_table(pas->pas_id, NULL, 0, &output_rt, &output_rt_size);
In PATCH 07/11 you have support for "static" resources that can be part
of the firmware binary, but then you never make use of it. Like in the
iris patch you just give in NULL, 0 for input_rt, even though,
(presumably?) the remoteproc framework has support for parsing the
resource table from the ELF firmware image.
I would suggest adding a comment here justifying this and perhaps
something to the commit message. I do see value in having the
qcom_scm_pas_get_rsc_table() properly defined with input RT support, but
it's not obvious from the description of your patches that this is
effectively dead code right now(?).
> + if (ret) {
> + dev_err(pas->dev, "error %d getting resource_table\n", ret);
> + return ret;
> + }
> +
> + rproc->cached_table = output_rt;
> + rproc->table_ptr = rproc->cached_table;
> + rproc->table_sz = output_rt_size;
> +
> + return ret;
> +}
> +
> static unsigned long qcom_pas_panic(struct rproc *rproc)
> {
> struct qcom_pas *pas = rproc->priv;
> @@ -420,7 +449,7 @@ static const struct rproc_ops qcom_pas_ops = {
> .start = qcom_pas_start,
> .stop = qcom_pas_stop,
> .da_to_va = qcom_pas_da_to_va,
> - .parse_fw = qcom_register_dump_segments,
> + .parse_fw = qcom_pas_parse_firmware,
> .load = qcom_pas_load,
> .panic = qcom_pas_panic,
> };
> @@ -430,7 +459,7 @@ static const struct rproc_ops qcom_pas_minidump_ops = {
> .start = qcom_pas_start,
> .stop = qcom_pas_stop,
> .da_to_va = qcom_pas_da_to_va,
> - .parse_fw = qcom_register_dump_segments,
> + .parse_fw = qcom_pas_parse_firmware,
> .load = qcom_pas_load,
> .panic = qcom_pas_panic,
> .coredump = qcom_pas_minidump,
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index ea7034c4b996..8456cca3f3e0 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -22,7 +22,6 @@
> #include <linux/slab.h>
> #include <linux/soc/qcom/mdt_loader.h>
>
> -#define MAX_RSCTABLE_SIZE SZ_16K;
I'm confused why there is a semicolon here suddenly. Did you edit this
patch by hand?
Applying: remoteproc: pas: Extend parse_fw callback to parse resource table
Patch failed at 0009 remoteproc: pas: Extend parse_fw callback to parse resource table
error: patch failed: drivers/soc/qcom/mdt_loader.c:22
error: drivers/soc/qcom/mdt_loader.c: patch does not apply
> #define RSC_TABLE_HASH_BITS 5 // 32 buckets
>
> DEFINE_HASHTABLE(qcom_pas_rsc_table_map, RSC_TABLE_HASH_BITS);
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index 62f239f64dfb..92ad862e733e 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -8,6 +8,8 @@
> #define QCOM_MDT_TYPE_HASH (2 << 24)
> #define QCOM_MDT_RELOCATABLE BIT(27)
>
> +#define MAX_RSCTABLE_SIZE SZ_16K
> +
> struct device;
> struct firmware;
> struct qcom_scm_pas_ctx;
You added this define yourself in PATCH 08/11, so just add it in the
right place directly. Make sure you scroll through your patch set before
sending to make sure all changes are in the right commit. :-)
Thanks,
Stephan
On 20/08/2025 10:36, Stephan Gerhold wrote: >> #include <linux/slab.h> >> #include <linux/soc/qcom/mdt_loader.h> >> >> -#define MAX_RSCTABLE_SIZE SZ_16K; > > I'm confused why there is a semicolon here suddenly. Did you edit this > patch by hand? > > Applying: remoteproc: pas: Extend parse_fw callback to parse resource table > Patch failed at 0009 remoteproc: pas: Extend parse_fw callback to parse resource table > error: patch failed: drivers/soc/qcom/mdt_loader.c:22 > error: drivers/soc/qcom/mdt_loader.c: patch does not apply This is very, very odd process. Editing patches POST format-patch or post b4 (wut?) is a serious warning sign. Few commit msgs also bring attention to possibility of AI, therefore please clarify: Did you use AI tools (qcom internal, external, any LLM/AI related tools) when writing that code, formatting it or creating this patchset? This is very important, as it create might create legal risk and everyone should be aware of it. Best regards, Krzysztof
On Thu, Aug 21, 2025 at 04:49:39PM +0200, Krzysztof Kozlowski wrote: > On 20/08/2025 10:36, Stephan Gerhold wrote: > >> #include <linux/slab.h> > >> #include <linux/soc/qcom/mdt_loader.h> > >> > >> -#define MAX_RSCTABLE_SIZE SZ_16K; > > > > I'm confused why there is a semicolon here suddenly. Did you edit this > > patch by hand? > > > > Applying: remoteproc: pas: Extend parse_fw callback to parse resource table > > Patch failed at 0009 remoteproc: pas: Extend parse_fw callback to parse resource table > > error: patch failed: drivers/soc/qcom/mdt_loader.c:22 > > error: drivers/soc/qcom/mdt_loader.c: patch does not apply > > > This is very, very odd process. Editing patches POST format-patch or > post b4 (wut?) is a serious warning sign. > > Few commit msgs also bring attention to possibility of AI, therefore > please clarify: > > Did you use AI tools (qcom internal, external, any LLM/AI related tools) > when writing that code, formatting it or creating this patchset? > > This is very important, as it create might create legal risk and > everyone should be aware of it. Again, I already accepted my mistakes of editing(removing ;) 8/11 patch once I checked minor checkpatch warning on it and will not do it again. And again, I don't use any of such tools, and hopeful it clarifies. > > Best regards, > Krzysztof > -- -Mukesh Ojha
On Wed, Aug 20, 2025 at 10:36:10AM +0200, Stephan Gerhold wrote:
> On Tue, Aug 19, 2025 at 10:24:44PM +0530, Mukesh Ojha wrote:
> > Extend parse_fw callback to include SMC call to get resource
> > table from TrustZone to leverage resource table parse and
> > mapping and unmapping code reuse from the framework.
> >
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > ---
> > drivers/remoteproc/qcom_q6v5_pas.c | 33 +++++++++++++++++++++++++++--
> > drivers/soc/qcom/mdt_loader.c | 1 -
> > include/linux/soc/qcom/mdt_loader.h | 2 ++
> > 3 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > index 09cada92dfd5..1e0f09bf1ef2 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > @@ -408,6 +408,35 @@ static void *qcom_pas_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is
> > return pas->mem_region + offset;
> > }
> >
> > +static int qcom_pas_parse_firmware(struct rproc *rproc, const struct firmware *fw)
> > +{
> > + struct qcom_pas *pas = rproc->priv;
> > + size_t output_rt_size = MAX_RSCTABLE_SIZE;
> > + void *output_rt;
> > + int ret;
> > +
> > + ret = qcom_register_dump_segments(rproc, fw);
> > + if (ret) {
> > + dev_err(pas->dev, "Error in registering dump segments\n");
> > + return ret;
> > + }
> > +
> > + if (!rproc->has_iommu)
> > + return ret;
> > +
> > + ret = qcom_scm_pas_get_rsc_table(pas->pas_id, NULL, 0, &output_rt, &output_rt_size);
>
> In PATCH 07/11 you have support for "static" resources that can be part
> of the firmware binary, but then you never make use of it. Like in the
> iris patch you just give in NULL, 0 for input_rt, even though,
> (presumably?) the remoteproc framework has support for parsing the
> resource table from the ELF firmware image.
Should have added a check here
ret = rproc_elf_load_rsc_table(rproc, fw);
if (ret)
dev_info(&rproc->dev, "Error in loading resource table in firmware\n");
ret = qcom_scm_pas_get_rsc_table(pas->pas_id, rproc->table_ptr, rproc->table_sz, &output_rt, &output_rt_size);
...
..
return ret;
..
}
> I would suggest adding a comment here justifying this and perhaps
> something to the commit message. I do see value in having the
> qcom_scm_pas_get_rsc_table() properly defined with input RT support, but
> it's not obvious from the description of your patches that this is
> effectively dead code right now(?).
Sure, will add the comment where ever, I am going to pass NULL, 0, which
is for like IRIS. You rightly said, remoteproc can have its input_rt
by checking it in firmware binary have resource table while for others like
iris/venus etc. support is not there now but can be added in future.
-Mukesh
>
> > + if (ret) {
> > + dev_err(pas->dev, "error %d getting resource_table\n", ret);
> > + return ret;
> > + }
> > +
> > + rproc->cached_table = output_rt;
> > + rproc->table_ptr = rproc->cached_table;
> > + rproc->table_sz = output_rt_size;
> > +
> > + return ret;
> > +}
> > +
> > static unsigned long qcom_pas_panic(struct rproc *rproc)
> > {
> > struct qcom_pas *pas = rproc->priv;
> > @@ -420,7 +449,7 @@ static const struct rproc_ops qcom_pas_ops = {
> > .start = qcom_pas_start,
> > .stop = qcom_pas_stop,
> > .da_to_va = qcom_pas_da_to_va,
> > - .parse_fw = qcom_register_dump_segments,
> > + .parse_fw = qcom_pas_parse_firmware,
> > .load = qcom_pas_load,
> > .panic = qcom_pas_panic,
> > };
> > @@ -430,7 +459,7 @@ static const struct rproc_ops qcom_pas_minidump_ops = {
> > .start = qcom_pas_start,
> > .stop = qcom_pas_stop,
> > .da_to_va = qcom_pas_da_to_va,
> > - .parse_fw = qcom_register_dump_segments,
> > + .parse_fw = qcom_pas_parse_firmware,
> > .load = qcom_pas_load,
> > .panic = qcom_pas_panic,
> > .coredump = qcom_pas_minidump,
> > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > index ea7034c4b996..8456cca3f3e0 100644
> > --- a/drivers/soc/qcom/mdt_loader.c
> > +++ b/drivers/soc/qcom/mdt_loader.c
> > @@ -22,7 +22,6 @@
> > #include <linux/slab.h>
> > #include <linux/soc/qcom/mdt_loader.h>
> >
> > -#define MAX_RSCTABLE_SIZE SZ_16K;
>
> I'm confused why there is a semicolon here suddenly. Did you edit this
> patch by hand?
>
> Applying: remoteproc: pas: Extend parse_fw callback to parse resource table
> Patch failed at 0009 remoteproc: pas: Extend parse_fw callback to parse resource table
> error: patch failed: drivers/soc/qcom/mdt_loader.c:22
> error: drivers/soc/qcom/mdt_loader.c: patch does not apply
Yes, I did this edit just before sending when checkpatch caught this.
Will avoid this in future.
>
> > #define RSC_TABLE_HASH_BITS 5 // 32 buckets
> >
> > DEFINE_HASHTABLE(qcom_pas_rsc_table_map, RSC_TABLE_HASH_BITS);
> > diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> > index 62f239f64dfb..92ad862e733e 100644
> > --- a/include/linux/soc/qcom/mdt_loader.h
> > +++ b/include/linux/soc/qcom/mdt_loader.h
> > @@ -8,6 +8,8 @@
> > #define QCOM_MDT_TYPE_HASH (2 << 24)
> > #define QCOM_MDT_RELOCATABLE BIT(27)
> >
> > +#define MAX_RSCTABLE_SIZE SZ_16K
> > +
> > struct device;
> > struct firmware;
> > struct qcom_scm_pas_ctx;
>
> You added this define yourself in PATCH 08/11, so just add it in the
> right place directly. Make sure you scroll through your patch set before
> sending to make sure all changes are in the right commit. :-)
I did this intentionally, because there is outside user of this macro
with this patch.
>
> Thanks,
> Stephan
--
-Mukesh Ojha
On Wed, Aug 20, 2025 at 04:44:48PM +0530, Mukesh Ojha wrote: > On Wed, Aug 20, 2025 at 10:36:10AM +0200, Stephan Gerhold wrote: > > On Tue, Aug 19, 2025 at 10:24:44PM +0530, Mukesh Ojha wrote: > [...] > > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c > > > index ea7034c4b996..8456cca3f3e0 100644 > > > --- a/drivers/soc/qcom/mdt_loader.c > > > +++ b/drivers/soc/qcom/mdt_loader.c > > > @@ -22,7 +22,6 @@ > > > #include <linux/slab.h> > > > #include <linux/soc/qcom/mdt_loader.h> > > > > > > -#define MAX_RSCTABLE_SIZE SZ_16K; > > > > I'm confused why there is a semicolon here suddenly. Did you edit this > > patch by hand? > > > > Applying: remoteproc: pas: Extend parse_fw callback to parse resource table > > Patch failed at 0009 remoteproc: pas: Extend parse_fw callback to parse resource table > > error: patch failed: drivers/soc/qcom/mdt_loader.c:22 > > error: drivers/soc/qcom/mdt_loader.c: patch does not apply > > Yes, I did this edit just before sending when checkpatch caught this. > Will avoid this in future. > Please don't edit patches manually, fix the commit instead. ;) > > > > > #define RSC_TABLE_HASH_BITS 5 // 32 buckets > > > > > > DEFINE_HASHTABLE(qcom_pas_rsc_table_map, RSC_TABLE_HASH_BITS); > > > diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h > > > index 62f239f64dfb..92ad862e733e 100644 > > > --- a/include/linux/soc/qcom/mdt_loader.h > > > +++ b/include/linux/soc/qcom/mdt_loader.h > > > @@ -8,6 +8,8 @@ > > > #define QCOM_MDT_TYPE_HASH (2 << 24) > > > #define QCOM_MDT_RELOCATABLE BIT(27) > > > > > > +#define MAX_RSCTABLE_SIZE SZ_16K > > > + > > > struct device; > > > struct firmware; > > > struct qcom_scm_pas_ctx; > > > > You added this define yourself in PATCH 08/11, so just add it in the > > right place directly. Make sure you scroll through your patch set before > > sending to make sure all changes are in the right commit. :-) > > I did this intentionally, because there is outside user of this macro > with this patch. > I don't think putting it in the header without an immediate user is a problem, as long as a user appears somewhere in the same series. Right now this patch touches multiple subsystems at once (qcom soc and remoteproc), which should be avoided when possible. Thanks, Stpehan
© 2016 - 2025 Red Hat, Inc.