| Message ID | 1481846632-4778-1-git-send-email-spjoshi@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Sarang, On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: > The function wkup_m3_rproc_boot_thread waits for asynchronous > firmware loading to complete successfully before calling > rproc_boot(). The same can be achieved by just setting > rproc->auto_boot flag. Change this. As a result this change > removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete > initialization to the wkup_m3_ipc_probe(). > > Other than the current usage, the firmware_loading_complete is > only used in rproc_del() where it's no longer needed. This > change is in preparation for removing firmware_loading_complete > completely. Based on the comments so far, I am assuming that you are dropping this series. In any case, this series did break our PM stack. We definitely don't want to auto-boot the wkup_m3_rproc device, that responsibility will need to stay with the wkup_m3_ipc driver. regards Suman > > CC: Dave Gerlach <d-gerlach@ti.com> > CC: Suman Anna <s-anna@ti.com> > CC: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org> > --- > > Hi Suman, > > Unfortunately, I don't have a TI device and couldn't test this > change. Is it possible for you to test this change on TI device? > > Thanks in advance. > > Regards, > Sarang > > drivers/remoteproc/wkup_m3_rproc.c | 2 +- > drivers/soc/ti/wkup_m3_ipc.c | 35 ++--------------------------------- > 2 files changed, 3 insertions(+), 34 deletions(-) > > diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c > index 18175d0..79ea022 100644 > --- a/drivers/remoteproc/wkup_m3_rproc.c > +++ b/drivers/remoteproc/wkup_m3_rproc.c > @@ -167,7 +167,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev) > goto err; > } > > - rproc->auto_boot = false; > + rproc->auto_boot = true; > > wkupm3 = rproc->priv; > wkupm3->rproc = rproc; > diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c > index 8823cc8..31090d70 100644 > --- a/drivers/soc/ti/wkup_m3_ipc.c > +++ b/drivers/soc/ti/wkup_m3_ipc.c > @@ -17,7 +17,6 @@ > > #include <linux/err.h> > #include <linux/kernel.h> > -#include <linux/kthread.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > #include <linux/module.h> > @@ -365,22 +364,6 @@ void wkup_m3_ipc_put(struct wkup_m3_ipc *m3_ipc) > } > EXPORT_SYMBOL_GPL(wkup_m3_ipc_put); > > -static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc) > -{ > - struct device *dev = m3_ipc->dev; > - int ret; > - > - wait_for_completion(&m3_ipc->rproc->firmware_loading_complete); > - > - init_completion(&m3_ipc->sync_complete); > - > - ret = rproc_boot(m3_ipc->rproc); > - if (ret) > - dev_err(dev, "rproc_boot failed\n"); > - > - do_exit(0); > -} > - > static int wkup_m3_ipc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -388,7 +371,6 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) > phandle rproc_phandle; > struct rproc *m3_rproc; > struct resource *res; > - struct task_struct *task; > struct wkup_m3_ipc *m3_ipc; > > m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL); > @@ -402,6 +384,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) > return PTR_ERR(m3_ipc->ipc_mem_base); > } > > + init_completion(&m3_ipc->sync_complete); > + > irq = platform_get_irq(pdev, 0); > if (!irq) { > dev_err(&pdev->dev, "no irq resource\n"); > @@ -449,25 +433,10 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) > > m3_ipc->ops = &ipc_ops; > > - /* > - * Wait for firmware loading completion in a thread so we > - * can boot the wkup_m3 as soon as it's ready without holding > - * up kernel boot > - */ > - task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_ipc, > - "wkup_m3_rproc_loader"); > - > - if (IS_ERR(task)) { > - dev_err(dev, "can't create rproc_boot thread\n"); > - goto err_put_rproc; > - } > - > m3_ipc_state = m3_ipc; > > return 0; > > -err_put_rproc: > - rproc_put(m3_rproc); > err_free_mbox: > mbox_free_channel(m3_ipc->mbox); > return ret; > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 21 Dec 19:16 PST 2016, Suman Anna wrote: > Hi Sarang, > > On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: > > The function wkup_m3_rproc_boot_thread waits for asynchronous > > firmware loading to complete successfully before calling > > rproc_boot(). The same can be achieved by just setting > > rproc->auto_boot flag. Change this. As a result this change > > removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete > > initialization to the wkup_m3_ipc_probe(). > > > > Other than the current usage, the firmware_loading_complete is > > only used in rproc_del() where it's no longer needed. This > > change is in preparation for removing firmware_loading_complete > > completely. > > Based on the comments so far, I am assuming that you are dropping this > series. > Following up on those comments only revealed that we have several other similar race conditions, so I'm hoping that Sarangdhar will continue to work on fixing those - and in this process get rid of this completion. > In any case, this series did break our PM stack. We definitely don't > want to auto-boot the wkup_m3_rproc device, that responsibility will > need to stay with the wkup_m3_ipc driver. > Reviewing the wkup_m3 situation again I see that as we have moved the resource table parsing to the rproc_boot() path there's no longer any need for the wkup_m3_ipc driver to wait for the remoteproc-core-internal completion. If rproc_get_by_phandle() returns non-NULL it is initialized. We still don't want to call rproc_boot() from wkup_m3_ipc_probe(), so let's keep the wkup_m3_rproc_boot_thread(). Sarangdhar, could you update the wkup_m3_ipc patch to just drop the wait_for_completion() call? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suman, On 12/21/2016 7:16 PM, Suman Anna wrote: > Hi Sarang, > > On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: >> The function wkup_m3_rproc_boot_thread waits for asynchronous >> firmware loading to complete successfully before calling >> rproc_boot(). The same can be achieved by just setting >> rproc->auto_boot flag. Change this. As a result this change >> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete >> initialization to the wkup_m3_ipc_probe(). >> >> Other than the current usage, the firmware_loading_complete is >> only used in rproc_del() where it's no longer needed. This >> change is in preparation for removing firmware_loading_complete >> completely. > > Based on the comments so far, I am assuming that you are dropping this > series. No, may not be dropping this. Will try to handle it differently in rproc_del() (probably by making use of some state flag). > > In any case, this series did break our PM stack. We definitely don't > want to auto-boot the wkup_m3_rproc device, that responsibility will > need to stay with the wkup_m3_ipc driver. Which scenario did it break? Booting up rproc device before wkup_m3_ipc_probe() causes issues? Nevertheless, I think Bjorn's suggestion of just dropping the call to wait_for_completion() and keeping kthread looks good, also because of the fact that rproc_boot() anyways calls request_firmware() and no longer needed to wait on asynchronous loading of firmware. Regards, Sarang > > regards > Suman > >> >> CC: Dave Gerlach <d-gerlach@ti.com> >> CC: Suman Anna <s-anna@ti.com> >> CC: Bjorn Andersson <bjorn.andersson@linaro.org> >> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org> >> --- >> >> Hi Suman, >> >> Unfortunately, I don't have a TI device and couldn't test this >> change. Is it possible for you to test this change on TI device? >> >> Thanks in advance. >> >> Regards, >> Sarang >> >> drivers/remoteproc/wkup_m3_rproc.c | 2 +- >> drivers/soc/ti/wkup_m3_ipc.c | 35 ++--------------------------------- >> 2 files changed, 3 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c >> index 18175d0..79ea022 100644 >> --- a/drivers/remoteproc/wkup_m3_rproc.c >> +++ b/drivers/remoteproc/wkup_m3_rproc.c >> @@ -167,7 +167,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev) >> goto err; >> } >> >> - rproc->auto_boot = false; >> + rproc->auto_boot = true; >> >> wkupm3 = rproc->priv; >> wkupm3->rproc = rproc; >> diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c >> index 8823cc8..31090d70 100644 >> --- a/drivers/soc/ti/wkup_m3_ipc.c >> +++ b/drivers/soc/ti/wkup_m3_ipc.c >> @@ -17,7 +17,6 @@ >> >> #include <linux/err.h> >> #include <linux/kernel.h> >> -#include <linux/kthread.h> >> #include <linux/interrupt.h> >> #include <linux/irq.h> >> #include <linux/module.h> >> @@ -365,22 +364,6 @@ void wkup_m3_ipc_put(struct wkup_m3_ipc *m3_ipc) >> } >> EXPORT_SYMBOL_GPL(wkup_m3_ipc_put); >> >> -static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc) >> -{ >> - struct device *dev = m3_ipc->dev; >> - int ret; >> - >> - wait_for_completion(&m3_ipc->rproc->firmware_loading_complete); >> - >> - init_completion(&m3_ipc->sync_complete); >> - >> - ret = rproc_boot(m3_ipc->rproc); >> - if (ret) >> - dev_err(dev, "rproc_boot failed\n"); >> - >> - do_exit(0); >> -} >> - >> static int wkup_m3_ipc_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -388,7 +371,6 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) >> phandle rproc_phandle; >> struct rproc *m3_rproc; >> struct resource *res; >> - struct task_struct *task; >> struct wkup_m3_ipc *m3_ipc; >> >> m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL); >> @@ -402,6 +384,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) >> return PTR_ERR(m3_ipc->ipc_mem_base); >> } >> >> + init_completion(&m3_ipc->sync_complete); >> + >> irq = platform_get_irq(pdev, 0); >> if (!irq) { >> dev_err(&pdev->dev, "no irq resource\n"); >> @@ -449,25 +433,10 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) >> >> m3_ipc->ops = &ipc_ops; >> >> - /* >> - * Wait for firmware loading completion in a thread so we >> - * can boot the wkup_m3 as soon as it's ready without holding >> - * up kernel boot >> - */ >> - task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_ipc, >> - "wkup_m3_rproc_loader"); >> - >> - if (IS_ERR(task)) { >> - dev_err(dev, "can't create rproc_boot thread\n"); >> - goto err_put_rproc; >> - } >> - >> m3_ipc_state = m3_ipc; >> >> return 0; >> >> -err_put_rproc: >> - rproc_put(m3_rproc); >> err_free_mbox: >> mbox_free_channel(m3_ipc->mbox); >> return ret; >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 12/22/2016 5:02 AM, Bjorn Andersson wrote: > On Wed 21 Dec 19:16 PST 2016, Suman Anna wrote: > >> Hi Sarang, >> >> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: >>> The function wkup_m3_rproc_boot_thread waits for asynchronous >>> firmware loading to complete successfully before calling >>> rproc_boot(). The same can be achieved by just setting >>> rproc->auto_boot flag. Change this. As a result this change >>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete >>> initialization to the wkup_m3_ipc_probe(). >>> >>> Other than the current usage, the firmware_loading_complete is >>> only used in rproc_del() where it's no longer needed. This >>> change is in preparation for removing firmware_loading_complete >>> completely. >> >> Based on the comments so far, I am assuming that you are dropping this >> series. >> > > Following up on those comments only revealed that we have several other > similar race conditions, so I'm hoping that Sarangdhar will continue to > work on fixing those - and in this process get rid of this completion. > >> In any case, this series did break our PM stack. We definitely don't >> want to auto-boot the wkup_m3_rproc device, that responsibility will >> need to stay with the wkup_m3_ipc driver. >> > > Reviewing the wkup_m3 situation again I see that as we have moved the > resource table parsing to the rproc_boot() path there's no longer any > need for the wkup_m3_ipc driver to wait for the remoteproc-core-internal > completion. > > If rproc_get_by_phandle() returns non-NULL it is initialized. We still > don't want to call rproc_boot() from wkup_m3_ipc_probe(), so let's keep > the wkup_m3_rproc_boot_thread(). Did you mean it's okay to call rproc_boot() from wkup_m3_ipc_probe()? rproc_boot() calls request_firmware() anyways and so there is no need to wait for completion of asynchronous firmware loading. > > Sarangdhar, could you update the wkup_m3_ipc patch to just drop the > wait_for_completion() call? Sure, assuming we should still keep the rproc_boot() call in the kthread. Regards, Sarang > > Regards, > Bjorn > > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi Sarang, On 12/22/2016 06:07 PM, Sarangdhar Joshi wrote: > On 12/22/2016 5:02 AM, Bjorn Andersson wrote: >> On Wed 21 Dec 19:16 PST 2016, Suman Anna wrote: >> >>> Hi Sarang, >>> >>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: >>>> The function wkup_m3_rproc_boot_thread waits for asynchronous >>>> firmware loading to complete successfully before calling >>>> rproc_boot(). The same can be achieved by just setting >>>> rproc->auto_boot flag. Change this. As a result this change >>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete >>>> initialization to the wkup_m3_ipc_probe(). >>>> >>>> Other than the current usage, the firmware_loading_complete is >>>> only used in rproc_del() where it's no longer needed. This >>>> change is in preparation for removing firmware_loading_complete >>>> completely. >>> >>> Based on the comments so far, I am assuming that you are dropping this >>> series. >>> >> >> Following up on those comments only revealed that we have several other >> similar race conditions, so I'm hoping that Sarangdhar will continue to >> work on fixing those - and in this process get rid of this completion. >> >>> In any case, this series did break our PM stack. We definitely don't >>> want to auto-boot the wkup_m3_rproc device, that responsibility will >>> need to stay with the wkup_m3_ipc driver. >>> >> >> Reviewing the wkup_m3 situation again I see that as we have moved the >> resource table parsing to the rproc_boot() path there's no longer any >> need for the wkup_m3_ipc driver to wait for the remoteproc-core-internal >> completion. >> >> If rproc_get_by_phandle() returns non-NULL it is initialized. We still >> don't want to call rproc_boot() from wkup_m3_ipc_probe(), so let's keep >> the wkup_m3_rproc_boot_thread(). > > Did you mean it's okay to call rproc_boot() from wkup_m3_ipc_probe()? > rproc_boot() calls request_firmware() anyways and so there is no need to > wait for completion of asynchronous firmware loading. No, please retain the kthread. I think you miss the purpose of this wait for completion originally. Before all the core changes in 4.9, the resource table is parsed in the first asynchronous loading step, and we had to wait for that step to complete. Now that there's no table parsing during the request_firmware_no_wait() for non auto-boot, we can get away from the need for a synchronzition call. > >> >> Sarangdhar, could you update the wkup_m3_ipc patch to just drop the >> wait_for_completion() call? > > Sure, assuming we should still keep the rproc_boot() call in the kthread. Yep. regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sarang, >> >> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: >>> The function wkup_m3_rproc_boot_thread waits for asynchronous >>> firmware loading to complete successfully before calling >>> rproc_boot(). The same can be achieved by just setting >>> rproc->auto_boot flag. Change this. As a result this change >>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete >>> initialization to the wkup_m3_ipc_probe(). >>> >>> Other than the current usage, the firmware_loading_complete is >>> only used in rproc_del() where it's no longer needed. This >>> change is in preparation for removing firmware_loading_complete >>> completely. >> >> Based on the comments so far, I am assuming that you are dropping this >> series. > > No, may not be dropping this. Will try to handle it differently in > rproc_del() (probably by making use of some state flag). >> >> In any case, this series did break our PM stack. We definitely don't >> want to auto-boot the wkup_m3_rproc device, that responsibility will >> need to stay with the wkup_m3_ipc driver. > > Which scenario did it break? Booting up rproc device before > wkup_m3_ipc_probe() causes issues? Yes, our state machine requires the wkup_m3_ipc driver to control the boot of the wkup_m3 remoteproc. The wkup_m3 is not a typical remoteproc, it is our PM master and is responsible for putting the host processor into suspend and waking it up in system suspend/cpuidle paths. The remoteproc infrastructure is only used for load/boot, but the Linux PM state machine and communication is all controlled by the wkup_m3_ipc driver. > > Nevertheless, I think Bjorn's suggestion of just dropping the call to > wait_for_completion() and keeping kthread looks good, also because of > the fact that rproc_boot() anyways calls request_firmware() and no > longer needed to wait on asynchronous loading of firmware. Yeah, I will have to test this, but looking at current code, this should mostly be ok because of the remoteproc core changes w.r.t resource table parsing. regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/23/2016 11:05 AM, Suman Anna wrote: > Hi Sarang, > >>> >>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: >>>> The function wkup_m3_rproc_boot_thread waits for asynchronous >>>> firmware loading to complete successfully before calling >>>> rproc_boot(). The same can be achieved by just setting >>>> rproc->auto_boot flag. Change this. As a result this change >>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete >>>> initialization to the wkup_m3_ipc_probe(). >>>> >>>> Other than the current usage, the firmware_loading_complete is >>>> only used in rproc_del() where it's no longer needed. This >>>> change is in preparation for removing firmware_loading_complete >>>> completely. >>> >>> Based on the comments so far, I am assuming that you are dropping this >>> series. >> >> No, may not be dropping this. Will try to handle it differently in >> rproc_del() (probably by making use of some state flag). >>> >>> In any case, this series did break our PM stack. We definitely don't >>> want to auto-boot the wkup_m3_rproc device, that responsibility will >>> need to stay with the wkup_m3_ipc driver. >> >> Which scenario did it break? Booting up rproc device before >> wkup_m3_ipc_probe() causes issues? > > Yes, our state machine requires the wkup_m3_ipc driver to control the > boot of the wkup_m3 remoteproc. The wkup_m3 is not a typical remoteproc, > it is our PM master and is responsible for putting the host processor > into suspend and waking it up in system suspend/cpuidle paths. > The remoteproc infrastructure is only used for load/boot, but the Linux > PM state machine and communication is all controlled by the wkup_m3_ipc > driver. > >> >> Nevertheless, I think Bjorn's suggestion of just dropping the call to >> wait_for_completion() and keeping kthread looks good, also because of >> the fact that rproc_boot() anyways calls request_firmware() and no >> longer needed to wait on asynchronous loading of firmware. > > Yeah, I will have to test this, but looking at current code, this should > mostly be ok because of the remoteproc core changes w.r.t resource table > parsing. Tested with just the wait_for_completion() removed and it works fine. I can send a patch for the same if you prefer me to send it. regards Suman > > regards > Suman > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/23/2016 03:57 PM, Suman Anna wrote: > On 12/23/2016 11:05 AM, Suman Anna wrote: >> Hi Sarang, >> >>>> >>>> On 12/15/2016 06:03 PM, Sarangdhar Joshi wrote: >>>>> The function wkup_m3_rproc_boot_thread waits for asynchronous >>>>> firmware loading to complete successfully before calling >>>>> rproc_boot(). The same can be achieved by just setting >>>>> rproc->auto_boot flag. Change this. As a result this change >>>>> removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete >>>>> initialization to the wkup_m3_ipc_probe(). >>>>> >>>>> Other than the current usage, the firmware_loading_complete is >>>>> only used in rproc_del() where it's no longer needed. This >>>>> change is in preparation for removing firmware_loading_complete >>>>> completely. >>>> >>>> Based on the comments so far, I am assuming that you are dropping this >>>> series. >>> >>> No, may not be dropping this. Will try to handle it differently in >>> rproc_del() (probably by making use of some state flag). >>>> >>>> In any case, this series did break our PM stack. We definitely don't >>>> want to auto-boot the wkup_m3_rproc device, that responsibility will >>>> need to stay with the wkup_m3_ipc driver. >>> >>> Which scenario did it break? Booting up rproc device before >>> wkup_m3_ipc_probe() causes issues? >> >> Yes, our state machine requires the wkup_m3_ipc driver to control the >> boot of the wkup_m3 remoteproc. The wkup_m3 is not a typical remoteproc, >> it is our PM master and is responsible for putting the host processor >> into suspend and waking it up in system suspend/cpuidle paths. >> The remoteproc infrastructure is only used for load/boot, but the Linux >> PM state machine and communication is all controlled by the wkup_m3_ipc >> driver. Thanks for explaining. I was missing the fact that the resource table parsing during asynchronous call and the one in rproc_boot() are different. >> >>> >>> Nevertheless, I think Bjorn's suggestion of just dropping the call to >>> wait_for_completion() and keeping kthread looks good, also because of >>> the fact that rproc_boot() anyways calls request_firmware() and no >>> longer needed to wait on asynchronous loading of firmware. >> >> Yeah, I will have to test this, but looking at current code, this should >> mostly be ok because of the remoteproc core changes w.r.t resource table >> parsing. > > Tested with just the wait_for_completion() removed and it works fine. I > can send a patch for the same if you prefer me to send it. Thanks for testing it. I have sent the patch. Regards, Sarang > > regards > Suman > >> >> regards >> Suman >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in >> the body of a message to majordomo@vger.kernel.org >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c index 18175d0..79ea022 100644 --- a/drivers/remoteproc/wkup_m3_rproc.c +++ b/drivers/remoteproc/wkup_m3_rproc.c @@ -167,7 +167,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev) goto err; } - rproc->auto_boot = false; + rproc->auto_boot = true; wkupm3 = rproc->priv; wkupm3->rproc = rproc; diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c index 8823cc8..31090d70 100644 --- a/drivers/soc/ti/wkup_m3_ipc.c +++ b/drivers/soc/ti/wkup_m3_ipc.c @@ -17,7 +17,6 @@ #include <linux/err.h> #include <linux/kernel.h> -#include <linux/kthread.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/module.h> @@ -365,22 +364,6 @@ void wkup_m3_ipc_put(struct wkup_m3_ipc *m3_ipc) } EXPORT_SYMBOL_GPL(wkup_m3_ipc_put); -static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc) -{ - struct device *dev = m3_ipc->dev; - int ret; - - wait_for_completion(&m3_ipc->rproc->firmware_loading_complete); - - init_completion(&m3_ipc->sync_complete); - - ret = rproc_boot(m3_ipc->rproc); - if (ret) - dev_err(dev, "rproc_boot failed\n"); - - do_exit(0); -} - static int wkup_m3_ipc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -388,7 +371,6 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) phandle rproc_phandle; struct rproc *m3_rproc; struct resource *res; - struct task_struct *task; struct wkup_m3_ipc *m3_ipc; m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL); @@ -402,6 +384,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) return PTR_ERR(m3_ipc->ipc_mem_base); } + init_completion(&m3_ipc->sync_complete); + irq = platform_get_irq(pdev, 0); if (!irq) { dev_err(&pdev->dev, "no irq resource\n"); @@ -449,25 +433,10 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev) m3_ipc->ops = &ipc_ops; - /* - * Wait for firmware loading completion in a thread so we - * can boot the wkup_m3 as soon as it's ready without holding - * up kernel boot - */ - task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_ipc, - "wkup_m3_rproc_loader"); - - if (IS_ERR(task)) { - dev_err(dev, "can't create rproc_boot thread\n"); - goto err_put_rproc; - } - m3_ipc_state = m3_ipc; return 0; -err_put_rproc: - rproc_put(m3_rproc); err_free_mbox: mbox_free_channel(m3_ipc->mbox); return ret;
The function wkup_m3_rproc_boot_thread waits for asynchronous firmware loading to complete successfully before calling rproc_boot(). The same can be achieved by just setting rproc->auto_boot flag. Change this. As a result this change removes wkup_m3_rproc_boot_thread and moves m3_ipc->sync_complete initialization to the wkup_m3_ipc_probe(). Other than the current usage, the firmware_loading_complete is only used in rproc_del() where it's no longer needed. This change is in preparation for removing firmware_loading_complete completely. CC: Dave Gerlach <d-gerlach@ti.com> CC: Suman Anna <s-anna@ti.com> CC: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org> --- Hi Suman, Unfortunately, I don't have a TI device and couldn't test this change. Is it possible for you to test this change on TI device? Thanks in advance. Regards, Sarang drivers/remoteproc/wkup_m3_rproc.c | 2 +- drivers/soc/ti/wkup_m3_ipc.c | 35 ++--------------------------------- 2 files changed, 3 insertions(+), 34 deletions(-)