| Message ID | 20171226203832.14928-3-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
> -----Original Message----- > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > Sent: Tuesday, December 26, 2017 9:39 PM > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson > <bjorn.andersson@linaro.org> > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; Suman Anna <s- > anna@ti.com>; Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > Subject: [PATCH v2 2/4] remoteproc: Rename "load_rsc_table" to > "parse_fw" > > The resource table is just one possible source of information that can > be extracted from the firmware file. Generalize this interface to allow > drivers to override this with parsers of other types of information. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v1: > - New patch > > drivers/remoteproc/remoteproc_core.c | 6 +++--- > drivers/remoteproc/remoteproc_internal.h | 7 +++---- > include/linux/remoteproc.h | 2 +- > 3 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index 5af7547b9d8d..6a72daa94673 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -944,8 +944,8 @@ static int rproc_fw_boot(struct rproc *rproc, const > struct firmware *fw) > > rproc->bootaddr = rproc_get_boot_addr(rproc, fw); > > - /* load resource table */ > - ret = rproc_load_rsc_table(rproc, fw); > + /* parse firmware resources */ > + ret = rproc_parse_fw(rproc, fw); Hi Bjorn, I think it will be good to keep resource (aka rsc) in function name. only "parse_fw" is not enough explicit and we don't know why rproc should parse firmware. Regards, Loic > if (ret) > goto disable_iommu; > > @@ -1555,7 +1555,7 @@ struct rproc *rproc_alloc(struct device *dev, const > char *name, > /* Default to ELF loader if no load function is specified */ > if (!rproc->ops->load) { > rproc->ops->load = rproc_elf_load_segments; > - rproc->ops->load_rsc_table = rproc_elf_load_rsc_table; > + rproc->ops->parse_fw = rproc_elf_load_rsc_table; > rproc->ops->find_loaded_rsc_table = > rproc_elf_find_loaded_rsc_table; > rproc->ops->sanity_check = rproc_elf_sanity_check; > rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; > diff --git a/drivers/remoteproc/remoteproc_internal.h > b/drivers/remoteproc/remoteproc_internal.h > index 55a2950c5cb7..7570beb035b5 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -88,11 +88,10 @@ int rproc_load_segments(struct rproc *rproc, const > struct firmware *fw) > return -EINVAL; > } > > -static inline int rproc_load_rsc_table(struct rproc *rproc, > - const struct firmware *fw) > +static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware > *fw) > { > - if (rproc->ops->load_rsc_table) > - return rproc->ops->load_rsc_table(rproc, fw); > + if (rproc->ops->parse_fw) > + return rproc->ops->parse_fw(rproc, fw); > > return 0; > } > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index de6e20a3f061..dc93ac3d1692 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -343,7 +343,7 @@ struct rproc_ops { > int (*stop)(struct rproc *rproc); > void (*kick)(struct rproc *rproc, int vqid); > void * (*da_to_va)(struct rproc *rproc, u64 da, int len); > - int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw); > + int (*parse_fw)(struct rproc *rproc, const struct firmware *fw); > struct resource_table *(*find_loaded_rsc_table)( > struct rproc *rproc, const struct firmware > *fw); > int (*load)(struct rproc *rproc, const struct firmware *fw); > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > owner@vger.kernel.org] On Behalf Of Loic PALLARDY > Sent: Wednesday, January 03, 2018 11:27 AM > To: Bjorn Andersson <bjorn.andersson@linaro.org>; Ohad Ben-Cohen > <ohad@wizery.com> > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; Suman Anna <s- > anna@ti.com>; Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > Subject: RE: [PATCH v2 2/4] remoteproc: Rename "load_rsc_table" to > "parse_fw" > > > > > -----Original Message----- > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux- > remoteproc- > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson > > Sent: Tuesday, December 26, 2017 9:39 PM > > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson > > <bjorn.andersson@linaro.org> > > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; > linux- > > arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; Suman Anna <s- > > anna@ti.com>; Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > > Subject: [PATCH v2 2/4] remoteproc: Rename "load_rsc_table" to > > "parse_fw" > > > > The resource table is just one possible source of information that can > > be extracted from the firmware file. Generalize this interface to allow > > drivers to override this with parsers of other types of information. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v1: > > - New patch > > > > drivers/remoteproc/remoteproc_core.c | 6 +++--- > > drivers/remoteproc/remoteproc_internal.h | 7 +++---- > > include/linux/remoteproc.h | 2 +- > > 3 files changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c > > b/drivers/remoteproc/remoteproc_core.c > > index 5af7547b9d8d..6a72daa94673 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -944,8 +944,8 @@ static int rproc_fw_boot(struct rproc *rproc, const > > struct firmware *fw) > > > > rproc->bootaddr = rproc_get_boot_addr(rproc, fw); > > > > - /* load resource table */ > > - ret = rproc_load_rsc_table(rproc, fw); > > + /* parse firmware resources */ > > + ret = rproc_parse_fw(rproc, fw); > Hi Bjorn, > > I think it will be good to keep resource (aka rsc) in function name. only > "parse_fw" is not enough explicit and we don't know why rproc should parse > firmware. > > Regards, > Loic Forgot my previous remark, better understanding thanks to the rest of the series. Anyway, will be nice to have a comment here as it is not only parsing the firmware, you collect some information like copy of the resource table, list of elf segment to dump... I think it is important to be clear about resource table management as it is a key element of the remoteproc core, where it is loaded, where it is copied back in memory... Regards, Loic > > > if (ret) > > goto disable_iommu; > > > > @@ -1555,7 +1555,7 @@ struct rproc *rproc_alloc(struct device *dev, > const > > char *name, > > /* Default to ELF loader if no load function is specified */ > > if (!rproc->ops->load) { > > rproc->ops->load = rproc_elf_load_segments; > > - rproc->ops->load_rsc_table = rproc_elf_load_rsc_table; > > + rproc->ops->parse_fw = rproc_elf_load_rsc_table; > > rproc->ops->find_loaded_rsc_table = > > rproc_elf_find_loaded_rsc_table; > > rproc->ops->sanity_check = rproc_elf_sanity_check; > > rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; > > diff --git a/drivers/remoteproc/remoteproc_internal.h > > b/drivers/remoteproc/remoteproc_internal.h > > index 55a2950c5cb7..7570beb035b5 100644 > > --- a/drivers/remoteproc/remoteproc_internal.h > > +++ b/drivers/remoteproc/remoteproc_internal.h > > @@ -88,11 +88,10 @@ int rproc_load_segments(struct rproc *rproc, const > > struct firmware *fw) > > return -EINVAL; > > } > > > > -static inline int rproc_load_rsc_table(struct rproc *rproc, > > - const struct firmware *fw) > > +static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware > > *fw) > > { > > - if (rproc->ops->load_rsc_table) > > - return rproc->ops->load_rsc_table(rproc, fw); > > + if (rproc->ops->parse_fw) > > + return rproc->ops->parse_fw(rproc, fw); > > > > return 0; > > } > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > index de6e20a3f061..dc93ac3d1692 100644 > > --- a/include/linux/remoteproc.h > > +++ b/include/linux/remoteproc.h > > @@ -343,7 +343,7 @@ struct rproc_ops { > > int (*stop)(struct rproc *rproc); > > void (*kick)(struct rproc *rproc, int vqid); > > void * (*da_to_va)(struct rproc *rproc, u64 da, int len); > > - int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw); > > + int (*parse_fw)(struct rproc *rproc, const struct firmware *fw); > > struct resource_table *(*find_loaded_rsc_table)( > > struct rproc *rproc, const struct firmware > > *fw); > > int (*load)(struct rproc *rproc, const struct firmware *fw); > > -- > > 2.15.0 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" > in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 03 Jan 05:15 PST 2018, Loic PALLARDY wrote: > > -----Original Message----- > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc- > > owner@vger.kernel.org] On Behalf Of Loic PALLARDY > > Sent: Wednesday, January 03, 2018 11:27 AM > > To: Bjorn Andersson <bjorn.andersson@linaro.org>; Ohad Ben-Cohen > > <ohad@wizery.com> > > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > > arm-msm@vger.kernel.org; linux-soc@vger.kernel.org; Suman Anna <s- > > anna@ti.com>; Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > > Subject: RE: [PATCH v2 2/4] remoteproc: Rename "load_rsc_table" to > > "parse_fw" > > > -----Original Message----- > > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux- > > remoteproc- > > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson [..] > > > - /* load resource table */ > > > - ret = rproc_load_rsc_table(rproc, fw); > > > + /* parse firmware resources */ > > > + ret = rproc_parse_fw(rproc, fw); > > Hi Bjorn, > > > > I think it will be good to keep resource (aka rsc) in function name. only > > "parse_fw" is not enough explicit and we don't know why rproc should parse > > firmware. > > > > Regards, > > Loic > Forgot my previous remark, better understanding thanks to the rest of > the series. > Anyway, will be nice to have a comment here as it is not only parsing > the firmware, you collect some information like copy of the resource > table, list of elf segment to dump... > I think it is important to be clear about resource table management as > it is a key element of the remoteproc core, where it is loaded, where > it is copied back in memory... I didn't manage to come up with a better name, but adding a comment to capture this makes a lot of sense. I will respin this patch! Thanks for reviewing this! Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 5af7547b9d8d..6a72daa94673 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -944,8 +944,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) rproc->bootaddr = rproc_get_boot_addr(rproc, fw); - /* load resource table */ - ret = rproc_load_rsc_table(rproc, fw); + /* parse firmware resources */ + ret = rproc_parse_fw(rproc, fw); if (ret) goto disable_iommu; @@ -1555,7 +1555,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, /* Default to ELF loader if no load function is specified */ if (!rproc->ops->load) { rproc->ops->load = rproc_elf_load_segments; - rproc->ops->load_rsc_table = rproc_elf_load_rsc_table; + rproc->ops->parse_fw = rproc_elf_load_rsc_table; rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table; rproc->ops->sanity_check = rproc_elf_sanity_check; rproc->ops->get_boot_addr = rproc_elf_get_boot_addr; diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h index 55a2950c5cb7..7570beb035b5 100644 --- a/drivers/remoteproc/remoteproc_internal.h +++ b/drivers/remoteproc/remoteproc_internal.h @@ -88,11 +88,10 @@ int rproc_load_segments(struct rproc *rproc, const struct firmware *fw) return -EINVAL; } -static inline int rproc_load_rsc_table(struct rproc *rproc, - const struct firmware *fw) +static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) { - if (rproc->ops->load_rsc_table) - return rproc->ops->load_rsc_table(rproc, fw); + if (rproc->ops->parse_fw) + return rproc->ops->parse_fw(rproc, fw); return 0; } diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index de6e20a3f061..dc93ac3d1692 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -343,7 +343,7 @@ struct rproc_ops { int (*stop)(struct rproc *rproc); void (*kick)(struct rproc *rproc, int vqid); void * (*da_to_va)(struct rproc *rproc, u64 da, int len); - int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw); + int (*parse_fw)(struct rproc *rproc, const struct firmware *fw); struct resource_table *(*find_loaded_rsc_table)( struct rproc *rproc, const struct firmware *fw); int (*load)(struct rproc *rproc, const struct firmware *fw);
The resource table is just one possible source of information that can be extracted from the firmware file. Generalize this interface to allow drivers to override this with parsers of other types of information. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v1: - New patch drivers/remoteproc/remoteproc_core.c | 6 +++--- drivers/remoteproc/remoteproc_internal.h | 7 +++---- include/linux/remoteproc.h | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-)