On Mon, Apr 03, 2023 at 07:06:12PM +0300, Dmitry Osipenko wrote:
> On 3/30/23 20:06, Petlozu Pravareshwar wrote:
> > The Sensor Processing Engine(SPE) can trigger a software wake-up of
> > the device. To support this wake-up for the SPE, set SR_CAPTURE_EN
> > bit in WAKE_AOWAKE_CNTRL register associated with the wake-up for
> > the SPE. This SR capturing logic is expected to be enabled for wakes
> > with short pulse signalling requirements.
> >
> > Signed-off-by: Viswanath L <viswanathl@xxxxxxxxxx>
> > Signed-off-by: Petlozu Pravareshwar <petlozup@xxxxxxxxxx>
> > ---
> > v1->v2:
> > * Rebase the change on latest code.
> > ---
> > drivers/soc/tegra/pmc.c | 24 +++++++++++++++++++++++-
> > 1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index cf4cfbf9f7c5..2a2342eff622 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -3,7 +3,7 @@
> > * drivers/soc/tegra/pmc.c
> > *
> > * Copyright (c) 2010 Google, Inc
> > - * Copyright (c) 2018-2022, NVIDIA CORPORATION. All rights reserved.
> > + * Copyright (c) 2018-2023, NVIDIA CORPORATION. All rights reserved.
> > *
> > * Author:
> > * Colin Cross <ccross@xxxxxxxxxx>
> > @@ -177,6 +177,7 @@
> > /* Tegra186 and later */
> > #define WAKE_AOWAKE_CNTRL(x) (0x000 + ((x) << 2))
> > #define WAKE_AOWAKE_CNTRL_LEVEL (1 << 3)
> > +#define WAKE_AOWAKE_CNTRL_SR_CAPTURE_EN (1 << 1)
> > #define WAKE_AOWAKE_MASK_W(x) (0x180 + ((x) << 2))
> > #define WAKE_AOWAKE_MASK_R(x) (0x300 + ((x) << 2))
> > #define WAKE_AOWAKE_STATUS_W(x) (0x30c + ((x) << 2))
> > @@ -191,6 +192,8 @@
> > #define WAKE_AOWAKE_CTRL 0x4f4
> > #define WAKE_AOWAKE_CTRL_INTR_POLARITY BIT(0)
> >
> > +#define SW_WAKE_ID 83 /* wake83 */
> > +
> > /* for secure PMC */
> > #define TEGRA_SMC_PMC 0xc2fffe00
> > #define TEGRA_SMC_PMC_READ 0xaa
> > @@ -355,6 +358,7 @@ struct tegra_pmc_soc {
> > void (*setup_irq_polarity)(struct tegra_pmc *pmc,
> > struct device_node *np,
> > bool invert);
> > + void (*set_wake_filters)(struct tegra_pmc *pmc);
> > int (*irq_set_wake)(struct irq_data *data, unsigned int on);
> > int (*irq_set_type)(struct irq_data *data, unsigned int type);
> > int (*powergate_set)(struct tegra_pmc *pmc, unsigned int id,
> > @@ -2416,6 +2420,17 @@ static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
> > return 0;
> > }
> >
> > +static void tegra186_pmc_set_wake_filters(struct tegra_pmc *pmc)
> > +{
> > + u32 value;
> > +
> > + /* SW Wake (wake83) needs SR_CAPTURE filter to be enabled */
> > + value = readl(pmc->wake + WAKE_AOWAKE_CNTRL(SW_WAKE_ID));
> > + value |= WAKE_AOWAKE_CNTRL_SR_CAPTURE_EN;
> > + writel(value, pmc->wake + WAKE_AOWAKE_CNTRL(SW_WAKE_ID));
> > + dev_dbg(pmc->dev, "WAKE_AOWAKE_CNTRL_83 = 0x%x\n", value);
> > +}
>
> To me this needs to be moved to the SPE driver, which should get the PMC
> regmap handle and enable wake only when needed, similarly how it's done
> by USB Tegra drivers that also need to configure PMC. Otherwise this
> looks like a hack/workaround.
Wake is still only enabled when needed via the irq_set_wake() callback.
And this is slightly different in that it pretty has no side-effects. It
isn't a configuration option that needs to be switched every now and
then but rather just a bit in a register that happens to have the wrong
default value (or an inconvenient default value).
For USB there are actual side-effects and some of the settings only make
sense if a given USB port is actually used, etc. So doing the regmap
dance is actually worth it.
For SPE it just doesn't seem worth it to have all of these inter-
dependencies just for the sake of statically setting one bit once during
boot.
Thierry
Attachment:
signature.asc
Description: PGP signature
