Message-ID: <YyHvfuCGLifv9OX+@orome>
Date:   Wed, 14 Sep 2022 17:13:02 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Petlozu Pravareshwar <petlozup@...dia.com>
Cc:     jonathanh@...dia.com, p.zabel@...gutronix.de,
        dmitry.osipenko@...labora.com, ulf.hansson@...aro.org,
        kkartik@...dia.com, cai.huoqing@...ux.dev, spatra@...dia.com,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stefan Kristiansson <stefank@...dia.com>
Subject: Re: [PATCH] soc/tegra: pmc: Fix dual edge triggered wakes

On Tue, Sep 06, 2022 at 01:44:17PM +0000, Petlozu Pravareshwar wrote:
> When a wake event is defined to be triggered on both positive
> and negative edge of the input wake signal, it is crucial to
> know the current state of the signal when going into suspend.
> The intended way to obtain the current state of the wake
> signals is to read the WAKE_AOWAKE_SW_STATUS register,
> which should contains the raw state of the wake signals.
> 
> However, this register is edge triggered, an edge will not
> be generated for signals that are already asserted prior to
> the assertion of WAKE_LATCH_SW.
> 
> To workaround this, change the polarity of the wake level
> from '0' to '1' while latching the signals, as this will
> generate an edge for signals that are set to '1'.
> 
> Signed-off-by: Stefan Kristiansson <stefank@...dia.com>
> Signed-off-by: Petlozu Pravareshwar <petlozup@...dia.com>
> ---
>  drivers/soc/tegra/pmc.c | 141 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 136 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 495d16a4732c..6a86961477e8 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -182,6 +182,9 @@
>  #define WAKE_AOWAKE_TIER0_ROUTING(x) (0x4b4 + ((x) << 2))
>  #define WAKE_AOWAKE_TIER1_ROUTING(x) (0x4c0 + ((x) << 2))
>  #define WAKE_AOWAKE_TIER2_ROUTING(x) (0x4cc + ((x) << 2))
> +#define WAKE_AOWAKE_SW_STATUS_W_0	0x49c
> +#define WAKE_AOWAKE_SW_STATUS(x)	(0x4a0 + ((x) << 2))
> +#define WAKE_LATCH_SW			0x498
>  
>  #define WAKE_AOWAKE_CTRL 0x4f4
>  #define  WAKE_AOWAKE_CTRL_INTR_POLARITY BIT(0)
> @@ -191,6 +194,12 @@
>  #define  TEGRA_SMC_PMC_READ	0xaa
>  #define  TEGRA_SMC_PMC_WRITE	0xbb
>  
> +#define WAKE_NR_EVENTS		96
> +#define WAKE_NR_VECTORS		(WAKE_NR_EVENTS / 32)

Are these always 96? Can they not vary by SoC generation?

> +
> +DECLARE_BITMAP(wake_level_map, WAKE_NR_EVENTS);
> +DECLARE_BITMAP(wake_dual_edge_map, WAKE_NR_EVENTS);

These should not be global variables but rather part of struct pmc.
Might be worth to allocate them dynamically based on a parameterized
per-SoC num_events.

> +
>  struct pmc_clk {
>  	struct clk_hw	hw;
>  	unsigned long	offs;
> @@ -2469,29 +2478,37 @@ static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>  {
>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>  	u32 value;
> +	unsigned long wake_id;

The usefulness of that local variable is questionable. Might as well
just keep using data->hwirq.

>  
> -	value = readl(pmc->wake + WAKE_AOWAKE_CNTRL(data->hwirq));
> +	wake_id = data->hwirq;
> +	value = readl(pmc->wake + WAKE_AOWAKE_CNTRL(wake_id));
>  
>  	switch (type) {
>  	case IRQ_TYPE_EDGE_RISING:
>  	case IRQ_TYPE_LEVEL_HIGH:
>  		value |= WAKE_AOWAKE_CNTRL_LEVEL;
> +		set_bit(wake_id, wake_level_map);
> +		clear_bit(wake_id, wake_dual_edge_map);
>  		break;
>  
>  	case IRQ_TYPE_EDGE_FALLING:
>  	case IRQ_TYPE_LEVEL_LOW:
>  		value &= ~WAKE_AOWAKE_CNTRL_LEVEL;
> +		clear_bit(wake_id, wake_level_map);
> +		clear_bit(wake_id, wake_dual_edge_map);
>  		break;
>  
>  	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>  		value ^= WAKE_AOWAKE_CNTRL_LEVEL;
> +		clear_bit(wake_id, wake_level_map);
> +		set_bit(wake_id, wake_dual_edge_map);
>  		break;
>  
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	writel(value, pmc->wake + WAKE_AOWAKE_CNTRL(data->hwirq));
> +	writel(value, pmc->wake + WAKE_AOWAKE_CNTRL(wake_id));
>  
>  	return 0;
>  }
> @@ -3074,28 +3091,142 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> -#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
> +#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_ARM64
> +/*
> + * Ensures that sufficient time is passed for a register write to
> + * serialize into the 32KHz domain.
> + */
> +static void wke_32kwritel(u32 val, u32 reg)
> +{
> +	writel(val, pmc->wake + reg);
> +	udelay(130);
> +}
> +
> +static void wke_write_wake_level(int wake, int level)
> +{
> +	u32 val;
> +	u32 reg = WAKE_AOWAKE_CNTRL(wake);
> +
> +	val = readl(pmc->wake + reg);
> +	if (level)
> +		val |= WAKE_AOWAKE_CNTRL_LEVEL;
> +	else
> +		val &= ~WAKE_AOWAKE_CNTRL_LEVEL;
> +	writel(val, pmc->wake + reg);
> +}
> +
> +static void wke_write_wake_levels(unsigned long *lvl)
> +{
> +	int i;
> +
> +	for (i = 0; i < WAKE_NR_EVENTS; i++)
> +		wke_write_wake_level(i, test_bit(i, lvl));
> +}
> +
> +static void wke_clear_sw_wake_status(void)
> +{
> +	wke_32kwritel(1, WAKE_AOWAKE_SW_STATUS_W_0);
> +}
> +
> +static void wke_read_sw_wake_status(unsigned long *status_map)
> +{
> +	u32 status[WAKE_NR_VECTORS];
> +	int i;
> +
> +	for (i = 0; i < WAKE_NR_EVENTS; i++)
> +		wke_write_wake_level(i, 0);
> +
> +	wke_clear_sw_wake_status();
> +
> +	wke_32kwritel(1, WAKE_LATCH_SW);
> +
> +	/*
> +	 * WAKE_AOWAKE_SW_STATUS is edge triggered, so in order to
> +	 * obtain the current status of the input wake signals, change
> +	 * the polarity of the wake level from 0->1 while latching to force
> +	 * a positive edge if the sampled signal is '1'.
> +	 */
> +	for (i = 0; i < WAKE_NR_EVENTS; i++)
> +		wke_write_wake_level(i, 1);
> +
> +	/*
> +	 * Wait for the update to be synced into the 32kHz domain,
> +	 * and let enough time lapse, so that the wake signals have time to
> +	 * be sampled.
> +	 */
> +	udelay(300);
> +
> +	wke_32kwritel(0, WAKE_LATCH_SW);
> +
> +	for (i = 0; i < WAKE_NR_VECTORS; i++)
> +		status[i] = readl(pmc->wake + WAKE_AOWAKE_SW_STATUS(i));
> +
> +	bitmap_from_arr32(status_map, status, WAKE_NR_EVENTS);
> +}
> +
> +static void wke_clear_wake_status(void)
> +{
> +	u32 status;
> +	int i, wake;
> +	unsigned long ulong_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));
> +		ulong_status = (unsigned long)status;
> +		for_each_set_bit(wake, &ulong_status, 32)
> +			wke_32kwritel(0x1,
> +				WAKE_AOWAKE_STATUS_W((i * 32) + wake));
> +	}
> +}
> +#endif /* CONFIG_ARM64 */
> +
>  static int tegra_pmc_suspend(struct device *dev)
>  {
> +#ifdef CONFIG_ARM
>  	struct tegra_pmc *pmc = dev_get_drvdata(dev);
>  
>  	tegra_pmc_writel(pmc, virt_to_phys(tegra_resume), PMC_SCRATCH41);

If this code is really no longer needed on 64-bit ARM, this should be a
separate patch. Also, these conditionals are becoming a bit unwieldy, so
I wonder if we should instead move to C conditionals using IS_ENABLED()
to make this a bit more readable and get improved code coverage at
compile time.

> +#else /* CONFIG_ARM64 */

The implied condition here seems a bit broad. I think it might be better
to conditionalize this code on the setting of tegra_pmc_soc.wake_events.
If that's != NULL, then execute all this context store/restore,
otherwise don't.

> +	DECLARE_BITMAP(status, WAKE_NR_EVENTS);
> +	DECLARE_BITMAP(lvl, WAKE_NR_EVENTS);
> +	DECLARE_BITMAP(wake_level, WAKE_NR_EVENTS);
> +	int i;
> +
> +	wke_read_sw_wake_status(status);
> +
> +	/* flip the wakeup trigger for dual-edge triggered pads
> +	 * which are currently asserting as wakeups
> +	 */
> +	for (i = 0; i < BITS_TO_LONGS(WAKE_NR_EVENTS); i++) {
> +		lvl[i] = ~status[i] & wake_dual_edge_map[i];
> +		wake_level[i] = lvl[i] | wake_level_map[i];
> +	}

I think these can be done using:

	bitmap_andnot(lvl, wake_dual_edge_map, status, WAKE_NR_EVENTS);
	bitmap_or(wake_level, lvl, wake_level_map, WAKE_NR_EVENTS);

> +
> +	/* Clear PMC Wake Status registers while going to suspend */
> +	wke_clear_wake_status();
>  
> +	wke_write_wake_levels(wake_level);
> +#endif
>  	return 0;
>  }
>  
>  static int tegra_pmc_resume(struct device *dev)
>  {
> +#ifdef CONFIG_ARM
>  	struct tegra_pmc *pmc = dev_get_drvdata(dev);
>  
>  	tegra_pmc_writel(pmc, 0x0, PMC_SCRATCH41);
> +#endif
>  
>  	return 0;
>  }
>  
>  static SIMPLE_DEV_PM_OPS(tegra_pmc_pm_ops, tegra_pmc_suspend, tegra_pmc_resume);
>  
> -#endif
> +#endif /*CONFIG_PM_SLEEP */

Perhaps we should then also drop the CONFIG_PM_SLEEP conditional and
instead mark tegra_pmc_suspend()/tegra_pmc_resume() as __maybe_unused.
Could also be a separate patch.

Thierry

>  
>  static const char * const tegra20_powergates[] = {
>  	[TEGRA_POWERGATE_CPU] = "cpu",
> @@ -4069,7 +4200,7 @@ static struct platform_driver tegra_pmc_driver = {
>  		.name = "tegra-pmc",
>  		.suppress_bind_attrs = true,
>  		.of_match_table = tegra_pmc_match,
> -#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
> +#if defined(CONFIG_PM_SLEEP)
>  		.pm = &tegra_pmc_pm_ops,
>  #endif
>  		.sync_state = tegra_pmc_sync_state,
> -- 
> 2.17.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists