| Message ID | 20240712-imx_rproc-v1-6-7bcf6732d328@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: imx_rproc: various patches for misc | expand |
On Fri, Jul 12, 2024 at 04:34:59PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > The i.MX7ULP Cortex-A7 is under control of Cortex-M4. The i.MX7ULP Linux > poweroff and restart rely on rpmsg driver to send a message to Cortex-M4 > firmware. Then Cortex-A7 could poweroff or restart by Cortex-M4 to > configure the i.MX7ULP power controller properly. > > However the reboot and restart kernel common code use atomic notifier, > so with blocking tx mailbox will trigger kernel dump, because of > blocking mailbox will use wait_for_completion_timeout. In such case, > linux no need to wait for completion. > > Current patch is to use non-blocking tx mailbox channel when system > is going to poweroff or restart. > > Reviewed-by: Jacky Bai <ping.bai@nxp.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/remoteproc/imx_rproc.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c > index 01cf1dfb2e87..e1abf110abc9 100644 > --- a/drivers/remoteproc/imx_rproc.c > +++ b/drivers/remoteproc/imx_rproc.c > @@ -18,6 +18,7 @@ > #include <linux/of_reserved_mem.h> > #include <linux/platform_device.h> > #include <linux/pm_domain.h> > +#include <linux/reboot.h> > #include <linux/regmap.h> > #include <linux/remoteproc.h> > #include <linux/workqueue.h> > @@ -114,6 +115,7 @@ struct imx_rproc { > u32 entry; /* cpu start address */ > u32 core_index; > struct dev_pm_domain_list *pd_list; > + struct sys_off_data data; > }; > > static const struct imx_rproc_att imx_rproc_att_imx93[] = { > @@ -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv) > return 0; > } > > +static int imx_rproc_sys_off_handler(struct sys_off_data *data) > +{ > + struct rproc *rproc = data->cb_data; > + int ret; > + > + imx_rproc_free_mbox(rproc); > + > + ret = imx_rproc_xtr_mbox_init(rproc, false); This is new functionality and has nothing to do with the rest of this set. Please introduce patches 5/6 and 6/6 as part of a new patchset. Thanks, Mathieu > + if (ret) { > + dev_err(&rproc->dev, "Failed to request non-blocking mbox\n"); > + return NOTIFY_BAD; > + } > + > + return NOTIFY_DONE; > +} > + > static int imx_rproc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev) > if (rproc->state != RPROC_DETACHED) > rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot"); > > + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) { > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE, > + SYS_OFF_PRIO_DEFAULT, > + imx_rproc_sys_off_handler, rproc); > + if (ret) { > + dev_err(dev, "register power off handler failure\n"); > + goto err_put_clk; > + } > + > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE, > + SYS_OFF_PRIO_DEFAULT, > + imx_rproc_sys_off_handler, rproc); > + if (ret) { > + dev_err(dev, "register restart handler failure\n"); > + goto err_put_clk; > + } > + } > + > ret = rproc_add(rproc); > if (ret) { > dev_err(dev, "rproc_add failed\n"); > > -- > 2.37.1 >
On Fri, Jul 12, 2024 at 04:34:59PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > The i.MX7ULP Cortex-A7 is under control of Cortex-M4. The i.MX7ULP Linux > poweroff and restart rely on rpmsg driver to send a message to Cortex-M4 > firmware. Then Cortex-A7 could poweroff or restart by Cortex-M4 to > configure the i.MX7ULP power controller properly. > > However the reboot and restart kernel common code use atomic notifier, > so with blocking tx mailbox will trigger kernel dump, because of > blocking mailbox will use wait_for_completion_timeout. In such case, > linux no need to wait for completion. > > Current patch is to use non-blocking tx mailbox channel when system > is going to poweroff or restart. > > Reviewed-by: Jacky Bai <ping.bai@nxp.com> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/remoteproc/imx_rproc.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c > index 01cf1dfb2e87..e1abf110abc9 100644 > --- a/drivers/remoteproc/imx_rproc.c > +++ b/drivers/remoteproc/imx_rproc.c > @@ -18,6 +18,7 @@ > #include <linux/of_reserved_mem.h> > #include <linux/platform_device.h> > #include <linux/pm_domain.h> > +#include <linux/reboot.h> > #include <linux/regmap.h> > #include <linux/remoteproc.h> > #include <linux/workqueue.h> > @@ -114,6 +115,7 @@ struct imx_rproc { > u32 entry; /* cpu start address */ > u32 core_index; > struct dev_pm_domain_list *pd_list; > + struct sys_off_data data; > }; > > static const struct imx_rproc_att imx_rproc_att_imx93[] = { > @@ -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv) > return 0; > } > > +static int imx_rproc_sys_off_handler(struct sys_off_data *data) > +{ > + struct rproc *rproc = data->cb_data; > + int ret; > + > + imx_rproc_free_mbox(rproc); > + > + ret = imx_rproc_xtr_mbox_init(rproc, false); > + if (ret) { > + dev_err(&rproc->dev, "Failed to request non-blocking mbox\n"); > + return NOTIFY_BAD; > + } > + > + return NOTIFY_DONE; > +} > + > static int imx_rproc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev) > if (rproc->state != RPROC_DETACHED) > rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot"); > > + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) { I don't suggest check compatible string. It'd better add a field in imx_rproc_dcfg, such as need_sys_off if (dcfg->need_sys_off) { ... } If there are new compatible string added, just need set need_sys_off to true in driver data. Frank > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE, > + SYS_OFF_PRIO_DEFAULT, > + imx_rproc_sys_off_handler, rproc); > + if (ret) { > + dev_err(dev, "register power off handler failure\n"); > + goto err_put_clk; > + } > + > + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE, > + SYS_OFF_PRIO_DEFAULT, > + imx_rproc_sys_off_handler, rproc); > + if (ret) { > + dev_err(dev, "register restart handler failure\n"); > + goto err_put_clk; > + } > + } > + > ret = rproc_add(rproc); > if (ret) { > dev_err(dev, "rproc_add failed\n"); > > -- > 2.37.1 >
> Subject: Re: [PATCH 6/6] remoteproc: imx_rproc: handle system off for > i.MX7ULP > > On Fri, Jul 12, 2024 at 04:34:59PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > The i.MX7ULP Cortex-A7 is under control of Cortex-M4. The > i.MX7ULP > > Linux poweroff and restart rely on rpmsg driver to send a message to > > Cortex-M4 firmware. Then Cortex-A7 could poweroff or restart by > > Cortex-M4 to configure the i.MX7ULP power controller properly. > > > > However the reboot and restart kernel common code use atomic > notifier, > > so with blocking tx mailbox will trigger kernel dump, because of > > blocking mailbox will use wait_for_completion_timeout. In such case, > > linux no need to wait for completion. > > > > Current patch is to use non-blocking tx mailbox channel when system > is > > going to poweroff or restart. > > > > Reviewed-by: Jacky Bai <ping.bai@nxp.com> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/remoteproc/imx_rproc.c | 36 > > ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/drivers/remoteproc/imx_rproc.c > > b/drivers/remoteproc/imx_rproc.c index > 01cf1dfb2e87..e1abf110abc9 > > 100644 > > --- a/drivers/remoteproc/imx_rproc.c > > +++ b/drivers/remoteproc/imx_rproc.c > > @@ -18,6 +18,7 @@ > > #include <linux/of_reserved_mem.h> > > #include <linux/platform_device.h> > > #include <linux/pm_domain.h> > > +#include <linux/reboot.h> > > #include <linux/regmap.h> > > #include <linux/remoteproc.h> > > #include <linux/workqueue.h> > > @@ -114,6 +115,7 @@ struct imx_rproc { > > u32 entry; /* cpu start > address */ > > u32 core_index; > > struct dev_pm_domain_list *pd_list; > > + struct sys_off_data data; > > }; > > > > static const struct imx_rproc_att imx_rproc_att_imx93[] = { @@ > > -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct > imx_rproc *priv) > > return 0; > > } > > > > +static int imx_rproc_sys_off_handler(struct sys_off_data *data) { > > + struct rproc *rproc = data->cb_data; > > + int ret; > > + > > + imx_rproc_free_mbox(rproc); > > + > > + ret = imx_rproc_xtr_mbox_init(rproc, false); > > + if (ret) { > > + dev_err(&rproc->dev, "Failed to request non-blocking > mbox\n"); > > + return NOTIFY_BAD; > > + } > > + > > + return NOTIFY_DONE; > > +} > > + > > static int imx_rproc_probe(struct platform_device *pdev) { > > struct device *dev = &pdev->dev; > > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct > platform_device *pdev) > > if (rproc->state != RPROC_DETACHED) > > rproc->auto_boot = of_property_read_bool(np, > "fsl,auto-boot"); > > > > + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) > { > > I don't suggest check compatible string. It'd better add a field in > imx_rproc_dcfg, such as need_sys_off > > if (dcfg->need_sys_off) { > ... > } > > If there are new compatible string added, just need set need_sys_off to > true in driver data. Could we delay the change when there is really new chips need this? The downstream commit time is " Date: Tue Dec 6 17:10:14 2022", In the past days, I not see other platforms require this. Mathieu, which do you prefer? add need_sys_off or keep current approach? Thanks, Peng. > > Frank > > > + ret = devm_register_sys_off_handler(dev, > SYS_OFF_MODE_POWER_OFF_PREPARE, > > + > SYS_OFF_PRIO_DEFAULT, > > + > imx_rproc_sys_off_handler, rproc); > > + if (ret) { > > + dev_err(dev, "register power off handler > failure\n"); > > + goto err_put_clk; > > + } > > + > > + ret = devm_register_sys_off_handler(dev, > SYS_OFF_MODE_RESTART_PREPARE, > > + > SYS_OFF_PRIO_DEFAULT, > > + > imx_rproc_sys_off_handler, rproc); > > + if (ret) { > > + dev_err(dev, "register restart handler > failure\n"); > > + goto err_put_clk; > > + } > > + } > > + > > ret = rproc_add(rproc); > > if (ret) { > > dev_err(dev, "rproc_add failed\n"); > > > > -- > > 2.37.1 > >
On Thu, Aug 08, 2024 at 02:56:20AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH 6/6] remoteproc: imx_rproc: handle system off for > > i.MX7ULP > > > > On Fri, Jul 12, 2024 at 04:34:59PM +0800, Peng Fan (OSS) wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > The i.MX7ULP Cortex-A7 is under control of Cortex-M4. The > > i.MX7ULP > > > Linux poweroff and restart rely on rpmsg driver to send a message to > > > Cortex-M4 firmware. Then Cortex-A7 could poweroff or restart by > > > Cortex-M4 to configure the i.MX7ULP power controller properly. > > > > > > However the reboot and restart kernel common code use atomic > > notifier, > > > so with blocking tx mailbox will trigger kernel dump, because of > > > blocking mailbox will use wait_for_completion_timeout. In such case, > > > linux no need to wait for completion. > > > > > > Current patch is to use non-blocking tx mailbox channel when system > > is > > > going to poweroff or restart. > > > > > > Reviewed-by: Jacky Bai <ping.bai@nxp.com> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > drivers/remoteproc/imx_rproc.c | 36 > > > ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/drivers/remoteproc/imx_rproc.c > > > b/drivers/remoteproc/imx_rproc.c index > > 01cf1dfb2e87..e1abf110abc9 > > > 100644 > > > --- a/drivers/remoteproc/imx_rproc.c > > > +++ b/drivers/remoteproc/imx_rproc.c > > > @@ -18,6 +18,7 @@ > > > #include <linux/of_reserved_mem.h> > > > #include <linux/platform_device.h> > > > #include <linux/pm_domain.h> > > > +#include <linux/reboot.h> > > > #include <linux/regmap.h> > > > #include <linux/remoteproc.h> > > > #include <linux/workqueue.h> > > > @@ -114,6 +115,7 @@ struct imx_rproc { > > > u32 entry; /* cpu start > > address */ > > > u32 core_index; > > > struct dev_pm_domain_list *pd_list; > > > + struct sys_off_data data; > > > }; > > > > > > static const struct imx_rproc_att imx_rproc_att_imx93[] = { @@ > > > -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct > > imx_rproc *priv) > > > return 0; > > > } > > > > > > +static int imx_rproc_sys_off_handler(struct sys_off_data *data) { > > > + struct rproc *rproc = data->cb_data; > > > + int ret; > > > + > > > + imx_rproc_free_mbox(rproc); > > > + > > > + ret = imx_rproc_xtr_mbox_init(rproc, false); > > > + if (ret) { > > > + dev_err(&rproc->dev, "Failed to request non-blocking > > mbox\n"); > > > + return NOTIFY_BAD; > > > + } > > > + > > > + return NOTIFY_DONE; > > > +} > > > + > > > static int imx_rproc_probe(struct platform_device *pdev) { > > > struct device *dev = &pdev->dev; > > > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct > > platform_device *pdev) > > > if (rproc->state != RPROC_DETACHED) > > > rproc->auto_boot = of_property_read_bool(np, > > "fsl,auto-boot"); > > > > > > + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) > > { > > > > I don't suggest check compatible string. It'd better add a field in > > imx_rproc_dcfg, such as need_sys_off > > > > if (dcfg->need_sys_off) { > > ... > > } > > > > If there are new compatible string added, just need set need_sys_off to > > true in driver data. > > Could we delay the change when there is really new chips need this? > The downstream commit time is " Date: Tue Dec 6 17:10:14 2022", > In the past days, I not see other platforms require this. > > Mathieu, which do you prefer? add need_sys_off or keep current > approach? > This driver is already making extensive use of device data and as such suggest to continue with that method. > Thanks, > Peng. > > > > > Frank > > > > > + ret = devm_register_sys_off_handler(dev, > > SYS_OFF_MODE_POWER_OFF_PREPARE, > > > + > > SYS_OFF_PRIO_DEFAULT, > > > + > > imx_rproc_sys_off_handler, rproc); > > > + if (ret) { > > > + dev_err(dev, "register power off handler > > failure\n"); > > > + goto err_put_clk; > > > + } > > > + > > > + ret = devm_register_sys_off_handler(dev, > > SYS_OFF_MODE_RESTART_PREPARE, > > > + > > SYS_OFF_PRIO_DEFAULT, > > > + > > imx_rproc_sys_off_handler, rproc); > > > + if (ret) { > > > + dev_err(dev, "register restart handler > > failure\n"); > > > + goto err_put_clk; > > > + } > > > + } > > > + > > > ret = rproc_add(rproc); > > > if (ret) { > > > dev_err(dev, "rproc_add failed\n"); > > > > > > -- > > > 2.37.1 > > >
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c index 01cf1dfb2e87..e1abf110abc9 100644 --- a/drivers/remoteproc/imx_rproc.c +++ b/drivers/remoteproc/imx_rproc.c @@ -18,6 +18,7 @@ #include <linux/of_reserved_mem.h> #include <linux/platform_device.h> #include <linux/pm_domain.h> +#include <linux/reboot.h> #include <linux/regmap.h> #include <linux/remoteproc.h> #include <linux/workqueue.h> @@ -114,6 +115,7 @@ struct imx_rproc { u32 entry; /* cpu start address */ u32 core_index; struct dev_pm_domain_list *pd_list; + struct sys_off_data data; }; static const struct imx_rproc_att imx_rproc_att_imx93[] = { @@ -1050,6 +1052,22 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv) return 0; } +static int imx_rproc_sys_off_handler(struct sys_off_data *data) +{ + struct rproc *rproc = data->cb_data; + int ret; + + imx_rproc_free_mbox(rproc); + + ret = imx_rproc_xtr_mbox_init(rproc, false); + if (ret) { + dev_err(&rproc->dev, "Failed to request non-blocking mbox\n"); + return NOTIFY_BAD; + } + + return NOTIFY_DONE; +} + static int imx_rproc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev) if (rproc->state != RPROC_DETACHED) rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot"); + if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) { + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE, + SYS_OFF_PRIO_DEFAULT, + imx_rproc_sys_off_handler, rproc); + if (ret) { + dev_err(dev, "register power off handler failure\n"); + goto err_put_clk; + } + + ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE, + SYS_OFF_PRIO_DEFAULT, + imx_rproc_sys_off_handler, rproc); + if (ret) { + dev_err(dev, "register restart handler failure\n"); + goto err_put_clk; + } + } + ret = rproc_add(rproc); if (ret) { dev_err(dev, "rproc_add failed\n");