Re: [PATCH v7 5/6] remoteproc: qcom: pas: Add late attach support for subsystems

[Aiqun(Maria) Yu]

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 <gokul.krishnakumar@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Gokul Krishna Krishnakumar <gokul.krishnakumar@xxxxxxxxxxxxxxxx>
>> Tested-by: Shawn Guo <shengchao.guo@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Jingyi Wang <jingyi.wang@xxxxxxxxxxxxxxxx>
>> ---
>>  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/ahBG6jKYdSAboWjs@xxxxxxxxxx/
[2]
https://lore.kernel.org/all/20260623-rproc-attach-issue-v3-0-8e24310707ce@xxxxxxxxxxxxxxxx/

> 
> Tested-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> 


-- 
Thx and BRs,
Aiqun(Maria) Yu