Re: [PATCH v2] soc/tegra: pmc: Support software wake-up for SPE

[Thierry Reding]

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