| Message ID | 20200424200135.28825-6-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: Add support for synchronisaton with rproc | expand |
On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > Refactor function rproc_fw_boot() in order to better reflect the work > that is done when supporting scenarios where the remoteproc core is > synchronising with a remote processor. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index a02593b75bec..e90a21de9de1 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) > } > > /* > - * take a firmware and boot a remote processor with it. > + * boot or synchronise with a remote processor. > */ > -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > +static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw) Per patch 4 this function will if rproc_needs_syncing() be called with fw == NULL, it's not obvious to me that the various operations on "fw" in this function are valid anymore. > { > struct device *dev = &rproc->dev; > const char *name = rproc->firmware; > @@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > if (ret) > return ret; > > - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > + if (!rproc_needs_syncing(rproc)) Can't we make this check on fw, to make the relationship "if we where passed a firmware object, we're going to load and boot that firmware"? Regards, Bjorn > + dev_info(dev, "Booting fw image %s, size %zd\n", > + name, fw->size); > > /* > * if enabling an IOMMU isn't relevant for this rproc, this is > @@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc) > } > } > > - ret = rproc_fw_boot(rproc, firmware_p); > + ret = rproc_actuate_device(rproc, firmware_p); > > release_firmware(firmware_p); > > -- > 2.20.1 >
On Tue, May 05, 2020 at 05:33:41PM -0700, Bjorn Andersson wrote: > On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > > > Refactor function rproc_fw_boot() in order to better reflect the work > > that is done when supporting scenarios where the remoteproc core is > > synchronising with a remote processor. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/remoteproc_core.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index a02593b75bec..e90a21de9de1 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) > > } > > > > /* > > - * take a firmware and boot a remote processor with it. > > + * boot or synchronise with a remote processor. > > */ > > -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > +static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw) > > Per patch 4 this function will if rproc_needs_syncing() be called with > fw == NULL, it's not obvious to me that the various operations on "fw" > in this function are valid anymore. That is right, all firmware related operations in this function are found in remoteproc_internal.h where the value of rproc->sync_with_mcu is checked before moving forward. That allows us to avoid introducing a new function similar to rproc_fw_boot() but without firmware operations or peppering the code with if statements. > > > { > > struct device *dev = &rproc->dev; > > const char *name = rproc->firmware; > > @@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > if (ret) > > return ret; > > > > - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > > + if (!rproc_needs_syncing(rproc)) > > Can't we make this check on fw, to make the relationship "if we where > passed a firmware object, we're going to load and boot that firmware"? It can but I specifically decided to use rproc_needs_syncing() to be consistent with the rest of the patchset. That way all we need to do is grep for rproc_needs_syncing to get all the places where a decision about synchronising with a remote processor is made. > > Regards, > Bjorn > > > + dev_info(dev, "Booting fw image %s, size %zd\n", > > + name, fw->size); > > > > /* > > * if enabling an IOMMU isn't relevant for this rproc, this is > > @@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc) > > } > > } > > > > - ret = rproc_fw_boot(rproc, firmware_p); > > + ret = rproc_actuate_device(rproc, firmware_p); > > > > release_firmware(firmware_p); > > > > -- > > 2.20.1 > >
On Fri 08 May 14:27 PDT 2020, Mathieu Poirier wrote: > On Tue, May 05, 2020 at 05:33:41PM -0700, Bjorn Andersson wrote: > > On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > > > > > Refactor function rproc_fw_boot() in order to better reflect the work > > > that is done when supporting scenarios where the remoteproc core is > > > synchronising with a remote processor. > > > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > --- > > > drivers/remoteproc/remoteproc_core.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > index a02593b75bec..e90a21de9de1 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) > > > } > > > > > > /* > > > - * take a firmware and boot a remote processor with it. > > > + * boot or synchronise with a remote processor. > > > */ > > > -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > > +static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw) > > > > Per patch 4 this function will if rproc_needs_syncing() be called with > > fw == NULL, it's not obvious to me that the various operations on "fw" > > in this function are valid anymore. > > That is right, all firmware related operations in this function are found in > remoteproc_internal.h where the value of rproc->sync_with_mcu is checked before > moving forward. That allows us to avoid introducing a new function similar to > rproc_fw_boot() but without firmware operations or peppering the code with if > statements. > As I wrote in my other reply, the two mechanisms seems to consist of the following steps: boot the core: 1) request firmware 2) prepare device 3) parse fw 4) handle resources 5) allocate carveouts 6) load segments 7) find resource table 8) prepare subdevices 9) power on 10) start subdevices sync: 1) prepare device (?) 2) handle resources 3) allocate carveouts (?) 4) prepare subdevices 5) attach 6) start subdevices Rather than relying on the state flag and missing ops will turn the first list into the second list I conceptually prefer having two separate functions that are easy to reason about. But I haven't done any refactoring or implemented this, so in practice the two might just be a lot of duplication(?) > > > > > { > > > struct device *dev = &rproc->dev; > > > const char *name = rproc->firmware; > > > @@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > > if (ret) > > > return ret; > > > > > > - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > > > + if (!rproc_needs_syncing(rproc)) > > > > Can't we make this check on fw, to make the relationship "if we where > > passed a firmware object, we're going to load and boot that firmware"? > > It can but I specifically decided to use rproc_needs_syncing() to be consistent > with the rest of the patchset. That way all we need to do is grep for > rproc_needs_syncing to get all the places where a decision about synchronising > with a remote processor is made. > Conceptually we have a single "to sync or not to sync", but I think we're invoking rproc_needs_syncing() 8 times during rproc_fw_boot() and each of those operations may or may not do anything. There are certain operations where I see it makes sense for a driver to either implement or not, but I think that e.g. for a rproc in OFFLINE state we should just require ops->start to be specified - because it doesn't make sense to enter rproc_start() if ops->start is a nop. Regards, Bjorn > > > > Regards, > > Bjorn > > > > > + dev_info(dev, "Booting fw image %s, size %zd\n", > > > + name, fw->size); > > > > > > /* > > > * if enabling an IOMMU isn't relevant for this rproc, this is > > > @@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc) > > > } > > > } > > > > > > - ret = rproc_fw_boot(rproc, firmware_p); > > > + ret = rproc_actuate_device(rproc, firmware_p); > > > > > > release_firmware(firmware_p); > > > > > > -- > > > 2.20.1 > > >
On Wed, May 13, 2020 at 07:10:55PM -0700, Bjorn Andersson wrote: > On Fri 08 May 14:27 PDT 2020, Mathieu Poirier wrote: > > > On Tue, May 05, 2020 at 05:33:41PM -0700, Bjorn Andersson wrote: > > > On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > > > > > > > Refactor function rproc_fw_boot() in order to better reflect the work > > > > that is done when supporting scenarios where the remoteproc core is > > > > synchronising with a remote processor. > > > > > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > --- > > > > drivers/remoteproc/remoteproc_core.c | 10 ++++++---- > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > > index a02593b75bec..e90a21de9de1 100644 > > > > --- a/drivers/remoteproc/remoteproc_core.c > > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > > @@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) > > > > } > > > > > > > > /* > > > > - * take a firmware and boot a remote processor with it. > > > > + * boot or synchronise with a remote processor. > > > > */ > > > > -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > > > +static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw) > > > > > > Per patch 4 this function will if rproc_needs_syncing() be called with > > > fw == NULL, it's not obvious to me that the various operations on "fw" > > > in this function are valid anymore. > > > > That is right, all firmware related operations in this function are found in > > remoteproc_internal.h where the value of rproc->sync_with_mcu is checked before > > moving forward. That allows us to avoid introducing a new function similar to > > rproc_fw_boot() but without firmware operations or peppering the code with if > > statements. > > > > As I wrote in my other reply, the two mechanisms seems to consist of the > following steps: > > boot the core: > 1) request firmware > 2) prepare device > 3) parse fw > 4) handle resources > 5) allocate carveouts > 6) load segments > 7) find resource table > 8) prepare subdevices > 9) power on > 10) start subdevices > > sync: > 1) prepare device (?) > 2) handle resources > 3) allocate carveouts (?) > 4) prepare subdevices > 5) attach > 6) start subdevices > > Rather than relying on the state flag and missing ops will turn the > first list into the second list I conceptually prefer having two > separate functions that are easy to reason about. I reflected long and hard about doing just that... > > But I haven't done any refactoring or implemented this, so in practice > the two might just be a lot of duplication(?) Exactly - duplication and maintenance are my prime concern. Right now some functions in the OFFLINE -> RUNNING are clearly not needed when dealing with a DETACHED -> RUNNING scenarios, but with I am convinced people will find ways to do something creative with the callbacks. In the end I fear the new functions we spin off to deal with DETACHED -> RUNNING scenarios will end up looking very similar to the current implementation. With that in mind I simply did all the work in remoteproc_internal.h and left the core functions intact. We can try spinning off new functions in the next revision, just to test my theory and see how much gets duplicated. > > > > > > > > { > > > > struct device *dev = &rproc->dev; > > > > const char *name = rproc->firmware; > > > > @@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > > > if (ret) > > > > return ret; > > > > > > > > - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > > > > + if (!rproc_needs_syncing(rproc)) > > > > > > Can't we make this check on fw, to make the relationship "if we where > > > passed a firmware object, we're going to load and boot that firmware"? > > > > It can but I specifically decided to use rproc_needs_syncing() to be consistent > > with the rest of the patchset. That way all we need to do is grep for > > rproc_needs_syncing to get all the places where a decision about synchronising > > with a remote processor is made. > > > > Conceptually we have a single "to sync or not to sync", but I think > we're invoking rproc_needs_syncing() 8 times during rproc_fw_boot() and > each of those operations may or may not do anything. As I said above, I'll try spinning off new functions in the next revision. From there we can decide how best to move forward. > > There are certain operations where I see it makes sense for a driver to > either implement or not, but I think that e.g. for a rproc in OFFLINE > state we should just require ops->start to be specified - because it > doesn't make sense to enter rproc_start() if ops->start is a nop. At this time ops->start() doesn't have to be specified... But as you say it won't do much good and this is something we can easily spot when reviewing patches. Thanks for the review, Mathieu > > Regards, > Bjorn > > > > > > > Regards, > > > Bjorn > > > > > > > + dev_info(dev, "Booting fw image %s, size %zd\n", > > > > + name, fw->size); > > > > > > > > /* > > > > * if enabling an IOMMU isn't relevant for this rproc, this is > > > > @@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc) > > > > } > > > > } > > > > > > > > - ret = rproc_fw_boot(rproc, firmware_p); > > > > + ret = rproc_actuate_device(rproc, firmware_p); > > > > > > > > release_firmware(firmware_p); > > > > > > > > -- > > > > 2.20.1 > > > >
On Fri 15 May 12:46 PDT 2020, Mathieu Poirier wrote: > On Wed, May 13, 2020 at 07:10:55PM -0700, Bjorn Andersson wrote: > > On Fri 08 May 14:27 PDT 2020, Mathieu Poirier wrote: > > > > > On Tue, May 05, 2020 at 05:33:41PM -0700, Bjorn Andersson wrote: > > > > On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > > > > > > > > > Refactor function rproc_fw_boot() in order to better reflect the work > > > > > that is done when supporting scenarios where the remoteproc core is > > > > > synchronising with a remote processor. > > > > > > > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > > --- > > > > > drivers/remoteproc/remoteproc_core.c | 10 ++++++---- > > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > > > index a02593b75bec..e90a21de9de1 100644 > > > > > --- a/drivers/remoteproc/remoteproc_core.c > > > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > > > @@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) > > > > > } > > > > > > > > > > /* > > > > > - * take a firmware and boot a remote processor with it. > > > > > + * boot or synchronise with a remote processor. > > > > > */ > > > > > -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > > > > +static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw) > > > > > > > > Per patch 4 this function will if rproc_needs_syncing() be called with > > > > fw == NULL, it's not obvious to me that the various operations on "fw" > > > > in this function are valid anymore. > > > > > > That is right, all firmware related operations in this function are found in > > > remoteproc_internal.h where the value of rproc->sync_with_mcu is checked before > > > moving forward. That allows us to avoid introducing a new function similar to > > > rproc_fw_boot() but without firmware operations or peppering the code with if > > > statements. > > > > > > > As I wrote in my other reply, the two mechanisms seems to consist of the > > following steps: > > > > boot the core: > > 1) request firmware > > 2) prepare device > > 3) parse fw > > 4) handle resources > > 5) allocate carveouts > > 6) load segments > > 7) find resource table > > 8) prepare subdevices > > 9) power on > > 10) start subdevices > > > > sync: > > 1) prepare device (?) > > 2) handle resources > > 3) allocate carveouts (?) > > 4) prepare subdevices > > 5) attach > > 6) start subdevices > > > > Rather than relying on the state flag and missing ops will turn the > > first list into the second list I conceptually prefer having two > > separate functions that are easy to reason about. > > I reflected long and hard about doing just that... > > > > > But I haven't done any refactoring or implemented this, so in practice > > the two might just be a lot of duplication(?) > > Exactly - duplication and maintenance are my prime concern. Right now some > functions in the OFFLINE -> RUNNING are clearly not needed when dealing with a > DETACHED -> RUNNING scenarios, but with I am convinced people will find ways to > do something creative with the callbacks. I'm sure there are problems out there that will require creative solutions, but I would prefer that we keep things easy to reason about and ensure that as new problems arise we can evolve the framework. > In the end I fear the new functions > we spin off to deal with DETACHED -> RUNNING scenarios will end up looking very > similar to the current implementation. > In those scenarios I don't see a problem with the platform drivers having functions of common code shared between ops->start and ops->attach. > With that in mind I simply did all the work in remoteproc_internal.h and left > the core functions intact. > > We can try spinning off new functions in the next revision, just to test my > theory and see how much gets duplicated. > Looking forward to it! > > > > > > > > > > > { > > > > > struct device *dev = &rproc->dev; > > > > > const char *name = rproc->firmware; > > > > > @@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) > > > > > if (ret) > > > > > return ret; > > > > > > > > > > - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); > > > > > + if (!rproc_needs_syncing(rproc)) > > > > > > > > Can't we make this check on fw, to make the relationship "if we where > > > > passed a firmware object, we're going to load and boot that firmware"? > > > > > > It can but I specifically decided to use rproc_needs_syncing() to be consistent > > > with the rest of the patchset. That way all we need to do is grep for > > > rproc_needs_syncing to get all the places where a decision about synchronising > > > with a remote processor is made. > > > > > > > Conceptually we have a single "to sync or not to sync", but I think > > we're invoking rproc_needs_syncing() 8 times during rproc_fw_boot() and > > each of those operations may or may not do anything. > > As I said above, I'll try spinning off new functions in the next revision. From > there we can decide how best to move forward. > > > > > There are certain operations where I see it makes sense for a driver to > > either implement or not, but I think that e.g. for a rproc in OFFLINE > > state we should just require ops->start to be specified - because it > > doesn't make sense to enter rproc_start() if ops->start is a nop. > > At this time ops->start() doesn't have to be specified... But as you say it > won't do much good and this is something we can easily spot when reviewing > patches. > Presumably after implementing this support we should check during registration that there's either a start or an attach ops specified. And if there's no start we shouldn't allow the RUNNING->OFFLINE transition. > Thanks for the review, Thanks for working on this and sorry that it took me time really digest this. Regards, Bjorn > Mathieu > > > > > Regards, > > Bjorn > > > > > > > > > > Regards, > > > > Bjorn > > > > > > > > > + dev_info(dev, "Booting fw image %s, size %zd\n", > > > > > + name, fw->size); > > > > > > > > > > /* > > > > > * if enabling an IOMMU isn't relevant for this rproc, this is > > > > > @@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc) > > > > > } > > > > > } > > > > > > > > > > - ret = rproc_fw_boot(rproc, firmware_p); > > > > > + ret = rproc_actuate_device(rproc, firmware_p); > > > > > > > > > > release_firmware(firmware_p); > > > > > > > > > > -- > > > > > 2.20.1 > > > > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index a02593b75bec..e90a21de9de1 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1370,9 +1370,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw) } /* - * take a firmware and boot a remote processor with it. + * boot or synchronise with a remote processor. */ -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) +static int rproc_actuate_device(struct rproc *rproc, const struct firmware *fw) { struct device *dev = &rproc->dev; const char *name = rproc->firmware; @@ -1382,7 +1382,9 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw) if (ret) return ret; - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size); + if (!rproc_needs_syncing(rproc)) + dev_info(dev, "Booting fw image %s, size %zd\n", + name, fw->size); /* * if enabling an IOMMU isn't relevant for this rproc, this is @@ -1818,7 +1820,7 @@ int rproc_boot(struct rproc *rproc) } } - ret = rproc_fw_boot(rproc, firmware_p); + ret = rproc_actuate_device(rproc, firmware_p); release_firmware(firmware_p);
Refactor function rproc_fw_boot() in order to better reflect the work that is done when supporting scenarios where the remoteproc core is synchronising with a remote processor. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)