>
> On Wed, Sep 14, 2022 at 04:38:31AM +0000, Petlozu Pravareshwar wrote:
> > During PMC resume, translate tier2 SC7 wake sources back into irqs and
> > do generic_handle_irq() to invoke the interrupt handlers for edge
> > triggered wake events such as sw-wake.
> >
> > Signed-off-by: Prathamesh Shete <pshete@xxxxxxxxxx>
> > Signed-off-by: Petlozu Pravareshwar <petlozup@xxxxxxxxxx>
> > ---
> > drivers/soc/tegra/pmc.c | 44
> > +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index
> > 8c7b46ac6ad6..f275af15f2d0 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -27,6 +27,7 @@
> > #include <linux/iopoll.h>
> > #include <linux/irqdomain.h>
> > #include <linux/irq.h>
> > +#include <linux/interrupt.h>
>
> This should be sorted alphabetically.
>
> > #include <linux/kernel.h>
> > #include <linux/of_address.h>
> > #include <linux/of_clk.h>
> > @@ -3181,6 +3182,40 @@ static void wke_clear_wake_status(void)
> > WAKE_AOWAKE_STATUS_W((i * 32) +
> wake));
> > }
> > }
> > +
> > +/* translate sc7 wake sources back into irqs to catch edge triggered
> > +wakeups */ static void process_wake_event(int index, u32 status) {
> > + int irq;
>
> Interrupts are usually unsigned int. The index parameter can also be
> unsigned int because it will never be negative.
>
> > + irq_hw_number_t hwirq;
> > + int wake;
>
> Can be unsigned int as well. Also, it's usually best to define variables of the
> same time on a single line to make the code a bit shorter.
>
> > + unsigned long flags;
> > + struct irq_desc *desc;
> > + unsigned long ulong_status = (unsigned long)status;
>
> Why not just make the status parameter an unsigned long to avoid this
> clumsy construct?
>
> > +
> > + dev_info(pmc->dev, "Wake[%d:%d] status=0x%x\n", (index + 1) *
> 32,
> > + index * 32, status);
>
> At most these should be dev_dbg().
>
> > + for_each_set_bit(wake, &ulong_status, 32) {
> > + hwirq = wake + 32 * index;
> > +
> > +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> > + irq = irq_find_mapping(pmc->domain, hwirq);
>
> We already use irq_domain_add_hierarchy(), and so the assumption is that
> IRQ_DOMAIN_HIERARCHY will always be enabled. You can drop this check.
> We may even want to make it explicit by selecting it from SOC_TEGRA_PMC.
>
> > +#else
> > + irq = hwirq;
> > +#endif
> > + desc = irq_to_desc(irq);
> > + if (!desc || !desc->action || !desc->action->name) {
> > + dev_info(pmc->dev, "Resume caused by WAKE%d,
> irq %d\n",
> > + (wake + 32 * index), irq);
> > + continue;
> > + }
> > + dev_info(pmc->dev, "Resume caused by WAKE%d, %s\n",
> > + (wake + 32 * index), desc->action->name);
>
> Same here.
>
> > + local_irq_save(flags);
> > + generic_handle_irq(irq);
> > + local_irq_restore(flags);
> > + }
> > +}
> > #endif /* CONFIG_ARM64 */
> >
> > static int tegra_pmc_suspend(struct device *dev) @@ -3219,6 +3254,15
> > @@ static int tegra_pmc_resume(struct device *dev)
> > struct tegra_pmc *pmc = dev_get_drvdata(dev);
> >
> > tegra_pmc_writel(pmc, 0x0, PMC_SCRATCH41);
> > +#else /* CONFIG_ARM64 */
> > + int i;
>
> unsigned int, please.
>
> Thierry
Agree to the comments. Will be updating the patch.
Thanks.
>
> > + u32 status;
> > +
> > + for (i = 0; i < WAKE_NR_VECTORS; i++) {
> > + status = readl(pmc->wake + WAKE_AOWAKE_STATUS_R(i));
> > + status = status & readl(pmc->wake +
> WAKE_AOWAKE_TIER2_ROUTING(i));
> > + process_wake_event(i, status);
> > + }
> > #endif
> >
> > return 0;
> > --
> > 2.17.1
> >
