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

[Thierry Reding]

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

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

Attachment: signature.asc
Description: PGP signature