Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 4 Apr 2023 13:15:29 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc:     Petlozu Pravareshwar <petlozup@nvidia.com>, jonathanh@nvidia.com,
        p.zabel@pengutronix.de, ulf.hansson@linaro.org, kkartik@nvidia.com,
        cai.huoqing@linux.dev, spatra@nvidia.com,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
        Viswanath L <viswanathl@nvidia.com>
Subject: Re: [PATCH v2] soc/tegra: pmc: Support software wake-up for SPE
Message-ID: <ZCwG0SM9szT6toGG@orome>
References: <20230330170621.258068-1-petlozup@nvidia.com>
 <f70c6d62-a7fd-c153-932f-16886e46329b@collabora.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="HWENIabzcbjHRvPD"
Content-Disposition: inline
In-Reply-To: <f70c6d62-a7fd-c153-932f-16886e46329b@collabora.com>
User-Agent: Mutt/2.2.10 (2023-03-25)
Precedence: bulk


--HWENIabzcbjHRvPD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
> >=20
> > Signed-off-by: Viswanath L <viswanathl@nvidia.com>
> > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com>
> > ---
> > v1->v2:
> > * Rebase the change on latest code.
> > ---
> >  drivers/soc/tegra/pmc.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >=20
> > 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@google.com>
> > @@ -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)
> > =20
> > +#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;
> >  }
> > =20
> > +static void tegra186_pmc_set_wake_filters(struct tegra_pmc *pmc)
> > +{
> > +	u32 value;
> > +
> > +	/* SW Wake (wake83) needs SR_CAPTURE filter to be enabled */
> > +	value =3D readl(pmc->wake + WAKE_AOWAKE_CNTRL(SW_WAKE_ID));
> > +	value |=3D WAKE_AOWAKE_CNTRL_SR_CAPTURE_EN;
> > +	writel(value, pmc->wake + WAKE_AOWAKE_CNTRL(SW_WAKE_ID));
> > +	dev_dbg(pmc->dev, "WAKE_AOWAKE_CNTRL_83 =3D 0x%x\n", value);
> > +}
>=20
> 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

--HWENIabzcbjHRvPD
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmQsBs4ACgkQ3SOs138+
s6En+RAAvRL37e270TWQhnw+WuuJchdJ3i+kJ9KFpCRbWXBoHv/4JB+WQHHiOpI9
LYi231NgMEDgQvmgyRgB5sPJIGA4+9aLVqhitOHU3wm3tZBQ1lrMaymPALSOGcQs
pHpXPFd8FO+vYdWYjVdh7K2mGT0V4eDkgNdPzdFFfMdxFSqs3taoMxUCnsKOGEZg
JScNPs6bws+5+5qmvgkm0Uei7OuKW8UTsJOdHyma66CHF4oql2WEDB84/a2reKUB
fIqwEI+jfHH80ilXuf755Q3zssgcePK4b3A86LqlXfgIqB0Gw+25W3a5S1pfMosk
8mYoVIpwsXRK0jOazims3KZIMA3ILwJBgm4uC5Vj3bzcmsqlvEEoVzSXTkhdGE7I
0cvXkXXlpx59w370QvvPPxS0yiiU/V3xBA4WUl+p3mI7MuQx/26cZ2PSIPT+VuK4
WnyblJ9ssV7OQCSzebeR/oxmDhfsB2lqLFNCfMt4f7GUd9idkHqitr11W6rYclCp
AJ2ZDqXLHDSlmrL3MIBd9CIpwCPVav8N3Ez7FNFj1/aly7wuqL9EW9lgIqj+7ZI+
hCE4Txl8XaJ0GdTUWC4Dh00QqaRCBHtgeVvgGGbg/fC0tyuwYqGlvTb4Zg8MHQrD
VxXGDdyqOtysE6dT9wRIAhDOEgzWYLYGO4GhjIhBpbDQ3TYJWQ8=
=YpjT
-----END PGP SIGNATURE-----

--HWENIabzcbjHRvPD--