RE: [PATCH v2 2/4] remoteproc: Rename "load_rsc_table" to "parse_fw"

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@xxxxxxxxxxxxxxx [mailto:linux-remoteproc-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Loic PALLARDY
> Sent: Wednesday, January 03, 2018 11:27 AM
> To: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>; Ohad Ben-Cohen
> <ohad@xxxxxxxxxx>
> Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> arm-msm@xxxxxxxxxxxxxxx; linux-soc@xxxxxxxxxxxxxxx; Suman Anna <s-
> anna@xxxxxx>; Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
> Subject: RE: [PATCH v2 2/4] remoteproc: Rename "load_rsc_table" to
> "parse_fw"
> 
> 
> 
> > -----Original Message-----
> > From: linux-remoteproc-owner@xxxxxxxxxxxxxxx [mailto:linux-
> remoteproc-
> > owner@xxxxxxxxxxxxxxx] On Behalf Of Bjorn Andersson
> > Sent: Tuesday, December 26, 2017 9:39 PM
> > To: Ohad Ben-Cohen <ohad@xxxxxxxxxx>; Bjorn Andersson
> > <bjorn.andersson@xxxxxxxxxx>
> > Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-
> > arm-msm@xxxxxxxxxxxxxxx; linux-soc@xxxxxxxxxxxxxxx; Suman Anna <s-
> > anna@xxxxxx>; Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html