Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] mailbox: qcom-ipcc: Log the pending interrupt during resume | From | Sibi Sankar <> | Date | Thu, 12 May 2022 16:49:30 +0530 |
| |
On 5/12/22 3:29 PM, Manivannan Sadhasivam wrote: > On Thu, May 12, 2022 at 03:08:43PM +0530, Sibi Sankar wrote: >> 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. >> > > Right but that information is only required for debugging the periodic wakeups. > And that's not going to be useful for an end user.
I would consider this an extension to "Resume caused by IRQ xx, xxxx" print that we get to identify the wake up source. That's the reasoning behind marking it as dev_info (being able to nail down random wakeups is just an added advantage). That said I'll re-spin it with dbg if that's the consensus.
> > Thanks, > Mani > >> -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 >>>> >>> >
| |