From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Loic PALLARDY <loic.pallardy@st.com>
Cc: Ohad Ben-Cohen <ohad@wizery.com>,
"linux-remoteproc@vger.kernel.org"
<linux-remoteproc@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
Date: Fri, 5 Jan 2018 10:50:50 -0800 [thread overview]
Message-ID: <20180105185050.GD478@tuxbook> (raw)
In-Reply-To: <c3e80239c8d84645919713897fcf9e46@SFHDAG7NODE2.st.com>
On Fri 05 Jan 08:53 PST 2018, Loic PALLARDY wrote:
>
>
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> > Sent: Wednesday, December 13, 2017 11:41 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
> > Subject: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> >
> > As the core now deals with the lack of a resource table, remove the
> > dangling custom dummy implementations of find_rsc_table from drivers.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > drivers/remoteproc/qcom_adsp_pil.c | 1 -
> > drivers/remoteproc/qcom_common.c | 19 -------------------
> > drivers/remoteproc/qcom_common.h | 4 ----
> > drivers/remoteproc/qcom_q6v5_pil.c | 11 -----------
> > drivers/remoteproc/qcom_wcnss.c | 1 -
> > drivers/remoteproc/st_slim_rproc.c | 18 ------------------
> > include/linux/remoteproc.h | 4 ----
> > 7 files changed, 58 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_adsp_pil.c
> > b/drivers/remoteproc/qcom_adsp_pil.c
> > index 7b9d810b23f1..b0b0d5ca1ca0 100644
> > --- a/drivers/remoteproc/qcom_adsp_pil.c
> > +++ b/drivers/remoteproc/qcom_adsp_pil.c
> > @@ -181,7 +181,6 @@ static const struct rproc_ops adsp_ops = {
> > .start = adsp_start,
> > .stop = adsp_stop,
> > .da_to_va = adsp_da_to_va,
> > - .find_rsc_table = qcom_mdt_find_rsc_table,
> > .load = adsp_load,
> > };
> >
> > diff --git a/drivers/remoteproc/qcom_common.c
> > b/drivers/remoteproc/qcom_common.c
> > index 818ee3657043..ce2dcc4f7de7 100644
> > --- a/drivers/remoteproc/qcom_common.c
> > +++ b/drivers/remoteproc/qcom_common.c
> > @@ -32,25 +32,6 @@
> >
> > static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
> >
> > -/**
> > - * qcom_mdt_find_rsc_table() - provide dummy resource table for
> > remoteproc
> > - * @rproc: remoteproc handle
> > - * @fw: firmware header
> > - * @tablesz: outgoing size of the table
> > - *
> > - * Returns a dummy table.
> > - */
> > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz)
> > -{
> > - static struct resource_table table = { .ver = 1, };
> > -
> > - *tablesz = sizeof(table);
> > - return &table;
> > -}
> > -EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
> > -
> > static int glink_subdev_probe(struct rproc_subdev *subdev)
> > {
> > struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
> > diff --git a/drivers/remoteproc/qcom_common.h
> > b/drivers/remoteproc/qcom_common.h
> > index 541586e528b3..73efed969bfd 100644
> > --- a/drivers/remoteproc/qcom_common.h
> > +++ b/drivers/remoteproc/qcom_common.h
> > @@ -30,10 +30,6 @@ struct qcom_rproc_ssr {
> > const char *name;
> > };
> >
> > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz);
> > -
> > void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink
> > *glink);
> > void qcom_remove_glink_subdev(struct rproc *rproc, struct
> > qcom_rproc_glink *glink);
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> > b/drivers/remoteproc/qcom_q6v5_pil.c
> > index fbff5d842581..6f6ea0414366 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pil.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> > @@ -304,16 +304,6 @@ static void q6v5_clk_disable(struct device *dev,
> > clk_disable_unprepare(clks[i]);
> > }
> >
> > -static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz)
> > -{
> > - static struct resource_table table = { .ver = 1, };
> > -
> > - *tablesz = sizeof(table);
> > - return &table;
> > -}
> > -
> > static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int
> > *current_perm,
> > bool remote_owner, phys_addr_t addr,
> > size_t size)
> > @@ -927,7 +917,6 @@ static const struct rproc_ops q6v5_ops = {
> > .start = q6v5_start,
> > .stop = q6v5_stop,
> > .da_to_va = q6v5_da_to_va,
> > - .find_rsc_table = q6v5_find_rsc_table,
> > .load = q6v5_load,
> > };
> >
> > diff --git a/drivers/remoteproc/qcom_wcnss.c
> > b/drivers/remoteproc/qcom_wcnss.c
> > index cc44ec598522..1fa5253020dd 100644
> > --- a/drivers/remoteproc/qcom_wcnss.c
> > +++ b/drivers/remoteproc/qcom_wcnss.c
> > @@ -310,7 +310,6 @@ static const struct rproc_ops wcnss_ops = {
> > .start = wcnss_start,
> > .stop = wcnss_stop,
> > .da_to_va = wcnss_da_to_va,
> > - .find_rsc_table = qcom_mdt_find_rsc_table,
> > .load = wcnss_load,
> > };
> >
> > diff --git a/drivers/remoteproc/st_slim_rproc.c
> > b/drivers/remoteproc/st_slim_rproc.c
> > index 1538ea915c49..c6a2a8b68c7a 100644
> > --- a/drivers/remoteproc/st_slim_rproc.c
> > +++ b/drivers/remoteproc/st_slim_rproc.c
> > @@ -200,28 +200,10 @@ static void *slim_rproc_da_to_va(struct rproc
> > *rproc, u64 da, int len)
> > return va;
> > }
> >
> > -/*
> > - * Firmware handler operations: sanity, boot address, load ...
> > - */
> > -
> > -static struct resource_table empty_rsc_tbl = {
> > - .ver = 1,
> > - .num = 0,
> > -};
> > -
> > -static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz)
> > -{
> > - *tablesz = sizeof(empty_rsc_tbl);
> > - return &empty_rsc_tbl;
> > -}
> > -
> > static const struct rproc_ops slim_rproc_ops = {
> > .start = slim_rproc_start,
> > .stop = slim_rproc_stop,
> > .da_to_va = slim_rproc_da_to_va,
> > - .find_rsc_table = slim_rproc_find_rsc_table,
> Hi Bjorn,
> Your patch is not complete for st_slim_rproc and so not working.
> In your patch 6/8, .load_rsc_table is define to default rproc_elf_load_rsc_table if no load ops defined.
>
> /* Default to ELF loader if no load function is specified */
> if (!rproc->ops->load) {
> rproc->ops->load = rproc_elf_load_segments;
> - rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
> + rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
>
> As st_slim_rproc has no load ops, it will inherit from all default ops including rproc_elf_load_rsc_table.
> As no resource table present in firmware, an error will be returned and st_slim_rproc will failed.
Thanks for catching my mistake, Loic! The expected outcome would be that
the slim_rproc_ops does point the "load" to the now exported ELF loader
symbol and by that won't get a find_rsc_table entry from the default
set..
> In case of Qualcom, load ops is defined...
Right, in the end the st_slim driver should have been defined just as
the Qualcomm one - but referencing rproc_elf_load_segments().
> See below B2260 log (with additional error message in rproc_load_rsc_table function.
>
> [ 10.201079] remoteproc remoteproc2: st231-delta is available
> [ 10.258121] remoteproc remoteproc2: powering up st231-delta
> [ 10.258143] remoteproc remoteproc2: Booting fw image rproc-st231-delta-fw, size 44416
> [ 10.258151] rproc_load_rsc_table: error -22
>
> Moreover with your proposal, as the choice to support or not a
> resource table is based on the fact rproc->ops->load_rsc_table is set
> or not, it is not possible with one unique driver to support firmware
> with resource table and firmware without resource table.
This is retaining the previous behaviour of failing to load/start a
remoteproc if no resource table is found, when the driver expects one.
The new scheme would allow you to specify a custom load_rsc_table that
calls rproc_elf_load_rsc_table() and ignores the return value to support
your use case.
> Will be better from my pov to consider that no resource table found
> in rproc_elf_load_rsc_table function as a normal case and not an
> error.
This would be a change in behavior and I can see how this could be
annoying to people (by not catching their mistakes during development).
If you think this is the preferred implementation then please submit a
separate patch for this so we can get some feedback from other users.
Regards,
Bjorn
next prev parent reply other threads:[~2018-01-05 18:50 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-13 22:41 [PATCH 0/8] Remoteproc cleanups Bjorn Andersson
2017-12-13 22:41 ` [PATCH 1/8] remoteproc: Remove depricated crash completion Bjorn Andersson
2017-12-13 22:41 ` [PATCH 2/8] remoteproc: Cache resource table size Bjorn Andersson
2017-12-13 22:41 ` [PATCH 3/8] remoteproc: Clone rproc_ops in rproc_alloc() Bjorn Andersson
2017-12-13 22:41 ` [PATCH 4/8] remoteproc: Merge rproc_ops and rproc_fw_ops Bjorn Andersson
2017-12-13 22:41 ` [PATCH 5/8] remoteproc: Don't handle empty resource table Bjorn Andersson
2017-12-13 22:41 ` [PATCH 6/8] remoteproc: Move resource table load logic to find Bjorn Andersson
2017-12-14 11:25 ` Loic PALLARDY
2017-12-14 19:25 ` Bjorn Andersson
2017-12-14 12:00 ` Loic PALLARDY
2017-12-14 19:47 ` Bjorn Andersson
2017-12-14 20:12 ` Loic PALLARDY
2017-12-13 22:41 ` [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies Bjorn Andersson
2018-01-05 16:53 ` Loic PALLARDY
2018-01-05 18:50 ` Bjorn Andersson [this message]
2018-01-08 8:14 ` Loic PALLARDY
2017-12-13 22:41 ` [PATCH 8/8] remoteproc: Reset table_ptr on stop Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180105185050.GD478@tuxbook \
--to=bjorn.andersson@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=loic.pallardy@st.com \
--cc=ohad@wizery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox