On 6/24/2026 4:08 PM, Mukesh Ojha wrote: > On Tue, Jun 23, 2026 at 03:05:21AM -0700, Jingyi Wang wrote: >> Subsystems can be brought out of reset by entities such as bootloaders. >> As the irq enablement could be later than subsystem bring up, the state >> of subsystem should be checked by reading SMP2P bits. >> >> A new qcom_pas_attach() function is introduced. if crash state is detected >> for the subsystem, rproc_report_crash() is called. If the ready state is >> detected meanwhile stop state is not detected, it will be marked as >> "attached", otherwise it could be the early boot feature is not supported >> by other entities or it has already been stopped. In above cases, the >> state will be marked as RPROC_OFFLINE so that the PAS driver can load the >> firmware and start the remoteproc. >> >> Co-developed-by: Gokul Krishna Krishnakumar >> <[email protected]> >> Signed-off-by: Gokul Krishna Krishnakumar >> <[email protected]> >> Tested-by: Shawn Guo <[email protected]> >> Signed-off-by: Jingyi Wang <[email protected]> >> --- >> drivers/remoteproc/qcom_common.h | 6 ++++ >> drivers/remoteproc/qcom_q6v5.c | 3 +- >> drivers/remoteproc/qcom_q6v5_pas.c | 68 >> ++++++++++++++++++++++++++++++++++++++ >> drivers/remoteproc/qcom_sysmon.c | 19 +++++++++++ >> 4 files changed, 95 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/qcom_common.h >> b/drivers/remoteproc/qcom_common.h >> index b07fbaa091a0..b0e7e336d363 100644 >> --- a/drivers/remoteproc/qcom_common.h >> +++ b/drivers/remoteproc/qcom_common.h >> @@ -68,6 +68,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc >> *rproc, >> int ssctl_instance); >> void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon); >> bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon); >> +bool qcom_sysmon_shutdown_irq_state(struct qcom_sysmon *sysmon); >> #else >> static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc >> *rproc, >> const char *name, >> @@ -84,6 +85,11 @@ static inline bool qcom_sysmon_shutdown_acked(struct >> qcom_sysmon *sysmon) >> { >> return false; >> } >> + >> +static inline bool qcom_sysmon_shutdown_irq_state(struct qcom_sysmon >> *sysmon) >> +{ >> + return false; >> +} >> #endif >> >> #endif >> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c >> index 58d5b85e58cd..a11d8ace554b 100644 >> --- a/drivers/remoteproc/qcom_q6v5.c >> +++ b/drivers/remoteproc/qcom_q6v5.c >> @@ -202,7 +202,8 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5, >> struct qcom_sysmon *sysmon) >> q6v5->running = false; >> >> /* Don't perform SMP2P dance if remote isn't running */ >> - if (q6v5->rproc->state != RPROC_RUNNING || >> qcom_sysmon_shutdown_acked(sysmon)) >> + if ((q6v5->rproc->state != RPROC_RUNNING && q6v5->rproc->state != >> RPROC_ATTACHED) || >> + qcom_sysmon_shutdown_acked(sysmon)) >> return 0; >> >> qcom_smem_state_update_bits(q6v5->state, >> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c >> b/drivers/remoteproc/qcom_q6v5_pas.c >> index 808e9609988d..8a0bb4b2e71c 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pas.c >> +++ b/drivers/remoteproc/qcom_q6v5_pas.c >> @@ -60,6 +60,7 @@ struct qcom_pas_data { >> int region_assign_count; >> bool region_assign_shared; >> int region_assign_vmid; >> + bool early_boot; >> }; >> >> struct qcom_pas { >> @@ -507,6 +508,67 @@ static unsigned long qcom_pas_panic(struct rproc *rproc) >> return qcom_q6v5_panic(&pas->q6v5); >> } >> >> +static int qcom_pas_attach(struct rproc *rproc) >> +{ >> + struct qcom_pas *pas = rproc->priv; >> + bool ready_state; >> + bool crash_state; >> + bool stop_state; >> + int ret; >> + >> + pas->q6v5.handover_issued = true; >> + enable_irq(pas->q6v5.handover_irq); >> + >> + pas->q6v5.running = true; >> + ret = irq_get_irqchip_state(pas->q6v5.fatal_irq, >> + IRQCHIP_STATE_LINE_LEVEL, &crash_state); >> + if (ret) >> + goto disable_running; >> + >> + if (crash_state) { >> + dev_err(pas->dev, "Subsystem has crashed before driver >> probe\n"); >> + rproc_report_crash(rproc, RPROC_FATAL_ERROR); >> + ret = -EINVAL; >> + goto disable_running; >> + } >> + >> + ret = irq_get_irqchip_state(pas->q6v5.stop_irq, >> + IRQCHIP_STATE_LINE_LEVEL, &stop_state); >> + if (ret) >> + goto disable_running; >> + >> + if (stop_state || qcom_sysmon_shutdown_irq_state(pas->sysmon)) { >> + dev_info(pas->dev, "Subsystem found stop state set. Falling >> back to start.\n"); >> + goto unroll_attach; >> + } >> + >> + ret = irq_get_irqchip_state(pas->q6v5.ready_irq, >> + IRQCHIP_STATE_LINE_LEVEL, &ready_state); >> + if (ret) >> + goto disable_running; >> + >> + if (unlikely(!ready_state)) { >> + /* >> + * The bootloader may not support early boot, mark the state as >> + * RPROC_OFFLINE so that the PAS driver can load the firmware >> and >> + * start the remoteproc. >> + */ >> + dev_err(pas->dev, "Failed to get subsystem ready interrupt\n"); >> + goto unroll_attach; >> + } >> + >> + return 0; >> + >> +unroll_attach: >> + pas->rproc->state = RPROC_OFFLINE; >> + ret = -EINVAL; >> +disable_running: >> + disable_irq(pas->q6v5.handover_irq); >> + pas->q6v5.running = false; >> + >> + return ret; >> +} >> + >> static const struct rproc_ops qcom_pas_ops = { >> .unprepare = qcom_pas_unprepare, >> .start = qcom_pas_start, >> @@ -515,6 +577,7 @@ static const struct rproc_ops qcom_pas_ops = { >> .parse_fw = qcom_pas_parse_firmware, >> .load = qcom_pas_load, >> .panic = qcom_pas_panic, >> + .attach = qcom_pas_attach, >> }; >> >> static const struct rproc_ops qcom_pas_minidump_ops = { >> @@ -526,6 +589,7 @@ static const struct rproc_ops qcom_pas_minidump_ops = { >> .load = qcom_pas_load, >> .panic = qcom_pas_panic, >> .coredump = qcom_pas_minidump, >> + .attach = qcom_pas_attach, >> }; >> >> static int qcom_pas_init_clock(struct qcom_pas *pas) >> @@ -852,6 +916,10 @@ static int qcom_pas_probe(struct platform_device *pdev) >> >> pas->pas_ctx->use_tzmem = rproc->has_iommu; >> pas->dtb_pas_ctx->use_tzmem = rproc->has_iommu; >> + >> + if (desc->early_boot) >> + pas->rproc->state = RPROC_DETACHED; >> + >> ret = rproc_add(rproc); >> if (ret) >> goto remove_ssr_sysmon; >> diff --git a/drivers/remoteproc/qcom_sysmon.c >> b/drivers/remoteproc/qcom_sysmon.c >> index 913e3b750a86..a0830a48b1f4 100644 >> --- a/drivers/remoteproc/qcom_sysmon.c >> +++ b/drivers/remoteproc/qcom_sysmon.c >> @@ -736,6 +736,25 @@ bool qcom_sysmon_shutdown_acked(struct qcom_sysmon >> *sysmon) >> } >> EXPORT_SYMBOL_GPL(qcom_sysmon_shutdown_acked); >> >> +bool qcom_sysmon_shutdown_irq_state(struct qcom_sysmon *sysmon) >> +{ >> + bool shutdown_state; >> + int ret; >> + >> + if (!sysmon) >> + return false; >> + >> + ret = irq_get_irqchip_state(sysmon->shutdown_irq, >> + IRQCHIP_STATE_LINE_LEVEL, &shutdown_state); >> + if (ret) { >> + dev_warn(sysmon->dev, "failed to get shutdown_state: %d\n", >> ret); >> + return false; >> + } >> + >> + return shutdown_state; >> +} >> +EXPORT_SYMBOL_GPL(qcom_sysmon_shutdown_irq_state); >> + >> /** >> * sysmon_probe() - probe sys_mon channel >> * @rpdev: rpmsg device handle >> >> -- >> 2.34.1 >> > > I tested the series on Hawi., it works but unsure about newly introduced > stop and shutdown status checking why is it required ? I think, you are > checking this for sanity if it is left some random state by the boot > loader..
The stop and shutdown status checking is to address a valid scenario which Stephan brought out in previous comments [1]. The more improved robustness for rproc_attach fail cases including Ping-pong is at [2]. [1] https://lore.kernel.org/all/[email protected]/ [2] https://lore.kernel.org/all/[email protected]/ > > Tested-by: Mukesh Ojha <[email protected]> > -- Thx and BRs, Aiqun(Maria) Yu
