Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755918AbeAHIO7 convert rfc822-to-8bit (ORCPT + 1 other); Mon, 8 Jan 2018 03:14:59 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:45304 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755009AbeAHIO5 (ORCPT ); Mon, 8 Jan 2018 03:14:57 -0500 From: Loic PALLARDY To: Bjorn Andersson CC: Ohad Ben-Cohen , "linux-remoteproc@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies Thread-Topic: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies Thread-Index: AQHTdGOoMpJrZ3yX6UGTPcAnhITdSqNli9BwgAAm2gCABBVOAA== Date: Mon, 8 Jan 2018 08:14:53 +0000 Message-ID: References: <20171213224111.17864-1-bjorn.andersson@linaro.org> <20171213224111.17864-8-bjorn.andersson@linaro.org> <20180105185050.GD478@tuxbook> In-Reply-To: <20180105185050.GD478@tuxbook> Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.47] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-08_05:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: > -----Original Message----- > From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org] > Sent: Friday, January 05, 2018 7:51 PM > To: Loic PALLARDY > Cc: Ohad Ben-Cohen ; linux- > remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies > > 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 ; Bjorn Andersson > > > > > > 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 > > > --- > > > 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. OK would be nice to document somewhere what's framework responsibilities and what's driver ones. > > > 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). You're right > > 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. No it is OK. I'll review v3. Regards, Loic > > Regards, > Bjorn