RE: [PATCH] soc/tegra: pmc: Process wake events during resume

[Petlozu Pravareshwar]

> 
> 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
> >