| Message ID | 20200324214603.14979-7-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: Add support for synchronisation with MCU | expand |
Hi Mathieu, > -----Original Message----- > From: Mathieu Poirier <mathieu.poirier@linaro.org> > Sent: mardi 24 mars 2020 22:46 > To: bjorn.andersson@linaro.org > Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s- > anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN > <arnaud.pouliquen@st.com>; Fabien DESSENNE > <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org > Subject: [PATCH v2 06/17] remoteproc: Introduce function > rproc_alloc_internals() > > In preparation to allocate the synchronisation operation and state > machine, spin off a new function in order to keep rproc_alloc() as > clean as possible. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++--- > - > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index ee277bc5556c..9da245734db6 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc, > const struct rproc_ops *ops) > return 0; > } > > +static int rproc_alloc_internals(struct rproc *rproc, const char *name, > + const struct rproc_ops *boot_ops, > + const char *firmware, int len) len argument is not used in the patch nor in the following, maybe removed from my pov. Regards, Loic > +{ > + int ret; > + > + /* We have a boot_ops so allocate firmware name and operations */ > + if (boot_ops) { > + ret = rproc_alloc_firmware(rproc, name, firmware); > + if (ret) > + return ret; > + > + ret = rproc_alloc_ops(rproc, boot_ops); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > /** > * rproc_alloc() - allocate a remote processor handle > * @dev: the underlying device > @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const > char *name, > rproc->dev.class = &rproc_class; > rproc->dev.driver_data = rproc; > > - if (rproc_alloc_firmware(rproc, name, firmware)) > - goto out; > - > - if (rproc_alloc_ops(rproc, ops)) > + if (rproc_alloc_internals(rproc, name, ops, > + firmware, len)) > goto out; > > /* Assign a unique device index and name */ > -- > 2.20.1
Hi Mathieu, On 3/27/20 6:10 AM, Loic PALLARDY wrote: > Hi Mathieu, > >> >> In preparation to allocate the synchronisation operation and state >> machine, spin off a new function in order to keep rproc_alloc() as >> clean as possible. >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++--- >> - >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index ee277bc5556c..9da245734db6 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc, >> const struct rproc_ops *ops) >> return 0; >> } >> >> +static int rproc_alloc_internals(struct rproc *rproc, const char *name, >> + const struct rproc_ops *boot_ops, >> + const char *firmware, int len) > > len argument is not used in the patch nor in the following, maybe removed from my pov. Indeed. > > Regards, > Loic >> +{ >> + int ret; >> + >> + /* We have a boot_ops so allocate firmware name and operations */ >> + if (boot_ops) { >> + ret = rproc_alloc_firmware(rproc, name, firmware); >> + if (ret) >> + return ret; So, can you explain why firmware allocation now becomes conditional on this boot_ops? Perhaps, continue to call this as ops following the field name in struct rproc. regards Suman >> + >> + ret = rproc_alloc_ops(rproc, boot_ops); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> /** >> * rproc_alloc() - allocate a remote processor handle >> * @dev: the underlying device >> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const >> char *name, >> rproc->dev.class = &rproc_class; >> rproc->dev.driver_data = rproc; >> >> - if (rproc_alloc_firmware(rproc, name, firmware)) >> - goto out; >> - >> - if (rproc_alloc_ops(rproc, ops)) >> + if (rproc_alloc_internals(rproc, name, ops, >> + firmware, len)) >> goto out; >> >> /* Assign a unique device index and name */ >> -- >> 2.20.1 >
On Fri, Mar 27, 2020 at 11:10:20AM +0000, Loic PALLARDY wrote: > Hi Mathieu, > > > -----Original Message----- > > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > Sent: mardi 24 mars 2020 22:46 > > To: bjorn.andersson@linaro.org > > Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s- > > anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN > > <arnaud.pouliquen@st.com>; Fabien DESSENNE > > <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org > > Subject: [PATCH v2 06/17] remoteproc: Introduce function > > rproc_alloc_internals() > > > > In preparation to allocate the synchronisation operation and state > > machine, spin off a new function in order to keep rproc_alloc() as > > clean as possible. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++--- > > - > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c > > b/drivers/remoteproc/remoteproc_core.c > > index ee277bc5556c..9da245734db6 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc, > > const struct rproc_ops *ops) > > return 0; > > } > > > > +static int rproc_alloc_internals(struct rproc *rproc, const char *name, > > + const struct rproc_ops *boot_ops, > > + const char *firmware, int len) > > len argument is not used in the patch nor in the following, maybe removed from my pov. I debated over this one... It is either introduce the function signature as a whole or incrementally as parameters are needed. I'm fine with both and will adopt the latter on the next revision. > > Regards, > Loic > > +{ > > + int ret; > > + > > + /* We have a boot_ops so allocate firmware name and operations */ > > + if (boot_ops) { > > + ret = rproc_alloc_firmware(rproc, name, firmware); > > + if (ret) > > + return ret; > > + > > + ret = rproc_alloc_ops(rproc, boot_ops); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > /** > > * rproc_alloc() - allocate a remote processor handle > > * @dev: the underlying device > > @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const > > char *name, > > rproc->dev.class = &rproc_class; > > rproc->dev.driver_data = rproc; > > > > - if (rproc_alloc_firmware(rproc, name, firmware)) > > - goto out; > > - > > - if (rproc_alloc_ops(rproc, ops)) > > + if (rproc_alloc_internals(rproc, name, ops, > > + firmware, len)) > > goto out; > > > > /* Assign a unique device index and name */ > > -- > > 2.20.1 >
Hi Suman, On Mon, Mar 30, 2020 at 03:38:14PM -0500, Suman Anna wrote: > Hi Mathieu, > > On 3/27/20 6:10 AM, Loic PALLARDY wrote: > > Hi Mathieu, > > > >> > >> In preparation to allocate the synchronisation operation and state > >> machine, spin off a new function in order to keep rproc_alloc() as > >> clean as possible. > >> > >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >> --- > >> drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++--- > >> - > >> 1 file changed, 22 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/remoteproc/remoteproc_core.c > >> b/drivers/remoteproc/remoteproc_core.c > >> index ee277bc5556c..9da245734db6 100644 > >> --- a/drivers/remoteproc/remoteproc_core.c > >> +++ b/drivers/remoteproc/remoteproc_core.c > >> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc, > >> const struct rproc_ops *ops) > >> return 0; > >> } > >> > >> +static int rproc_alloc_internals(struct rproc *rproc, const char *name, > >> + const struct rproc_ops *boot_ops, > >> + const char *firmware, int len) > > > > len argument is not used in the patch nor in the following, maybe removed from my pov. > > Indeed. > > > > > Regards, > > Loic > > >> +{ > >> + int ret; > >> + > >> + /* We have a boot_ops so allocate firmware name and operations */ > >> + if (boot_ops) { > >> + ret = rproc_alloc_firmware(rproc, name, firmware); > >> + if (ret) > >> + return ret; > > So, can you explain why firmware allocation now becomes conditional on > this boot_ops? There is no point in allocating a firmware name in a scenario where the remoteproc core is only synchronising with the MCU. As soon as a boot_ops (to be renamed ops as per your comment below) is present I assume firmware loading will be involved at some point. Do you see a scenario where that wouldn't be be case? > > Perhaps, continue to call this as ops following the field name in struct > rproc. Ok > > regards > Suman > > >> + > >> + ret = rproc_alloc_ops(rproc, boot_ops); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> /** > >> * rproc_alloc() - allocate a remote processor handle > >> * @dev: the underlying device > >> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const > >> char *name, > >> rproc->dev.class = &rproc_class; > >> rproc->dev.driver_data = rproc; > >> > >> - if (rproc_alloc_firmware(rproc, name, firmware)) > >> - goto out; > >> - > >> - if (rproc_alloc_ops(rproc, ops)) > >> + if (rproc_alloc_internals(rproc, name, ops, > >> + firmware, len)) > >> goto out; > >> > >> /* Assign a unique device index and name */ > >> -- > >> 2.20.1 > > >
On 4/1/20 3:29 PM, Mathieu Poirier wrote: > Hi Suman, > > On Mon, Mar 30, 2020 at 03:38:14PM -0500, Suman Anna wrote: >> Hi Mathieu, >> >> On 3/27/20 6:10 AM, Loic PALLARDY wrote: >>> Hi Mathieu, >>> >>>> >>>> In preparation to allocate the synchronisation operation and state >>>> machine, spin off a new function in order to keep rproc_alloc() as >>>> clean as possible. >>>> >>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>> --- >>>> drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++--- >>>> - >>>> 1 file changed, 22 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c >>>> b/drivers/remoteproc/remoteproc_core.c >>>> index ee277bc5556c..9da245734db6 100644 >>>> --- a/drivers/remoteproc/remoteproc_core.c >>>> +++ b/drivers/remoteproc/remoteproc_core.c >>>> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc, >>>> const struct rproc_ops *ops) >>>> return 0; >>>> } >>>> >>>> +static int rproc_alloc_internals(struct rproc *rproc, const char *name, >>>> + const struct rproc_ops *boot_ops, >>>> + const char *firmware, int len) >>> >>> len argument is not used in the patch nor in the following, maybe removed from my pov. >> >> Indeed. >> >>> >>> Regards, >>> Loic >> >>>> +{ >>>> + int ret; >>>> + >>>> + /* We have a boot_ops so allocate firmware name and operations */ >>>> + if (boot_ops) { >>>> + ret = rproc_alloc_firmware(rproc, name, firmware); >>>> + if (ret) >>>> + return ret; >> >> So, can you explain why firmware allocation now becomes conditional on >> this boot_ops? > > There is no point in allocating a firmware name in a scenario where the > remoteproc core is only synchronising with the MCU. As soon as a boot_ops (to > be renamed ops as per your comment below) is present I assume firmware loading > will be involved at some point. Do you see a scenario where that wouldn't be > be case? No. But that isn't until the whole sync stuff is introduced. As of this patch, it it still code refactoring. And I would think that the cases where only sync_ops will be used will be the minority. regards Suman > >> >> Perhaps, continue to call this as ops following the field name in struct >> rproc. > > Ok > >> >> regards >> Suman >> >>>> + >>>> + ret = rproc_alloc_ops(rproc, boot_ops); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> /** >>>> * rproc_alloc() - allocate a remote processor handle >>>> * @dev: the underlying device >>>> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const >>>> char *name, >>>> rproc->dev.class = &rproc_class; >>>> rproc->dev.driver_data = rproc; >>>> >>>> - if (rproc_alloc_firmware(rproc, name, firmware)) >>>> - goto out; >>>> - >>>> - if (rproc_alloc_ops(rproc, ops)) >>>> + if (rproc_alloc_internals(rproc, name, ops, >>>> + firmware, len)) >>>> goto out; >>>> >>>> /* Assign a unique device index and name */ >>>> -- >>>> 2.20.1 >>> >>
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index ee277bc5556c..9da245734db6 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) return 0; } +static int rproc_alloc_internals(struct rproc *rproc, const char *name, + const struct rproc_ops *boot_ops, + const char *firmware, int len) +{ + int ret; + + /* We have a boot_ops so allocate firmware name and operations */ + if (boot_ops) { + ret = rproc_alloc_firmware(rproc, name, firmware); + if (ret) + return ret; + + ret = rproc_alloc_ops(rproc, boot_ops); + if (ret) + return ret; + } + + return 0; +} + /** * rproc_alloc() - allocate a remote processor handle * @dev: the underlying device @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, rproc->dev.class = &rproc_class; rproc->dev.driver_data = rproc; - if (rproc_alloc_firmware(rproc, name, firmware)) - goto out; - - if (rproc_alloc_ops(rproc, ops)) + if (rproc_alloc_internals(rproc, name, ops, + firmware, len)) goto out; /* Assign a unique device index and name */
In preparation to allocate the synchronisation operation and state machine, spin off a new function in order to keep rproc_alloc() as clean as possible. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)