| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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