Messages in this thread | | | Subject | Re: [PATCH v2] mailbox: qcom-ipcc: Log the pending interrupt during resume | From | Sibi Sankar <> | Date | Thu, 12 May 2022 15:08:43 +0530 |
| |
Hey Mani,
Thanks for taking time to review the patch.
On 5/12/22 1:13 PM, Manivannan Sadhasivam wrote: > On Wed, May 11, 2022 at 12:13:24PM +0530, Sibi Sankar wrote: >> From: Prasad Sodagudi <quic_psodagud@quicinc.com> >> >> Enable logging of the pending interrupt that triggered device wakeup. This >> logging information helps to debug IRQs that cause periodic device wakeups >> and prints the detailed information of pending IPCC interrupts instead of >> the generic "Resume caused by IRQ 17, ipcc". >> >> Scenario: Device wakeup caused by Modem crash >> Logs: >> qcom-ipcc mailbox: virq: 182 triggered client-id: 2; signal-id: 2 >> >> From the IPCC bindings it can further understood that the client here is >> IPCC_CLIENT_MPSS and the signal was IPCC_MPROC_SIGNAL_SMP2P. >> >> Signed-off-by: Prasad Sodagudi <quic_psodagud@quicinc.com> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> >> V2: >> * Fix build error when ipcc is a module [Kernel Test Bot] >> >> drivers/mailbox/qcom-ipcc.c | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c >> index c5d963222014..21c071ec119c 100644 >> --- a/drivers/mailbox/qcom-ipcc.c >> +++ b/drivers/mailbox/qcom-ipcc.c >> @@ -254,6 +254,28 @@ static int qcom_ipcc_setup_mbox(struct qcom_ipcc *ipcc, >> return devm_mbox_controller_register(dev, mbox); >> } >> >> +#ifdef CONFIG_PM_SLEEP > > You don't need this guard anymore. Please see below.
ack
> >> +static int qcom_ipcc_pm_resume(struct device *dev) >> +{ >> + struct qcom_ipcc *ipcc = dev_get_drvdata(dev); >> + u32 hwirq; >> + int virq; >> + >> + hwirq = readl(ipcc->base + IPCC_REG_RECV_ID); >> + if (hwirq == IPCC_NO_PENDING_IRQ) >> + return 0; >> + >> + virq = irq_find_mapping(ipcc->irq_domain, hwirq); >> + >> + dev_info(dev, "virq: %d triggered client-id: %ld; signal-id: %ld\n", virq, >> + FIELD_GET(IPCC_CLIENT_ID_MASK, hwirq), FIELD_GET(IPCC_SIGNAL_ID_MASK, hwirq)); >> + > > Does this really need to be dev_info? This looks like a dev_dbg() material to > me.
The whole point of the log is to catch sporadic issues like random wakeups caused by remoteprocs through ipcc. We would just end up with a single line identifying the client id during resume if ipcc had indeed caused the wakeup else it wouldn't print anything.
-Sibi > >> + return 0; >> +} >> +#else >> +#define qcom_ipcc_pm_resume NULL >> +#endif >> + >> static int qcom_ipcc_probe(struct platform_device *pdev) >> { >> struct qcom_ipcc *ipcc; >> @@ -324,6 +346,10 @@ static const struct of_device_id qcom_ipcc_of_match[] = { >> }; >> MODULE_DEVICE_TABLE(of, qcom_ipcc_of_match); >> >> +static const struct dev_pm_ops qcom_ipcc_dev_pm_ops = { >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, qcom_ipcc_pm_resume) >> +}; >> + >> static struct platform_driver qcom_ipcc_driver = { >> .probe = qcom_ipcc_probe, >> .remove = qcom_ipcc_remove, >> @@ -331,6 +357,7 @@ static struct platform_driver qcom_ipcc_driver = { >> .name = "qcom-ipcc", >> .of_match_table = qcom_ipcc_of_match, >> .suppress_bind_attrs = true, >> + .pm = &qcom_ipcc_dev_pm_ops, > > You can use the new pm_sleep_ptr() macro to avoid the PM_SLEEP guard. > > .pm = pm_sleep_ptr(&qcom_ipcc_dev_pm_ops),
ack
> > Thanks, > Mani > >> }, >> }; >> >> -- >> 2.7.4 >> >
| |