From: Dmitry Osipenko <digetx@gmail.com>
To: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>,
thierry.reding@gmail.com, jonathanh@nvidia.com,
tglx@linutronix.de, jason@lakedaemon.net,
linus.walleij@linaro.org, stefan@agner.ch, mark.rutland@arm.com,
pdeschrijver@nvidia.com, pgaikwad@nvidia.com, sboyd@kernel.org,
linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
jckuo@nvidia.com, josephl@nvidia.com, talho@nvidia.com,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
mperttunen@nvidia.com, spatra@nvidia.com, robh+dt@kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend
Date: Fri, 26 Jul 2019 07:48:11 +0300 [thread overview]
Message-ID: <20190726074811.39e0e6cb@dimatab> (raw)
In-Reply-To: <78d5af07-2556-b60d-01d7-3684ebe7040b@nvidia.com>
В Wed, 24 Jul 2019 16:09:53 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> On 7/22/19 4:35 PM, Dmitry Osipenko wrote:
> > 22.07.2019 21:38, Marc Zyngier пишет:
> >> On Mon, 22 Jul 2019 09:21:21 -0700
> >> Sowjanya Komatineni <skomatineni@nvidia.com> wrote:
> >>
> >>> On 7/22/19 3:57 AM, Dmitry Osipenko wrote:
> >>>> 22.07.2019 13:13, Marc Zyngier пишет:
> >>>>> On 22/07/2019 10:54, Dmitry Osipenko wrote:
> >>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
> >>>>>>> Tegra210 platforms use sc7 entry firmware to program Tegra
> >>>>>>> LP0/SC7 entry sequence and sc7 entry firmware is run from
> >>>>>>> COP/BPMP-Lite.
> >>>>>>>
> >>>>>>> So, COP/BPMP-Lite still need IRQ function to finish SC7
> >>>>>>> suspend sequence for Tegra210.
> >>>>>>>
> >>>>>>> This patch has fix for leaving the COP IRQ enabled for
> >>>>>>> Tegra210 during interrupt controller suspend operation.
> >>>>>>>
> >>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
> >>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> >>>>>>> ---
> >>>>>>> drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
> >>>>>>> 1 file changed, 18 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/irqchip/irq-tegra.c
> >>>>>>> b/drivers/irqchip/irq-tegra.c index
> >>>>>>> e1f771c72fc4..851f88cef508 100644 ---
> >>>>>>> a/drivers/irqchip/irq-tegra.c +++
> >>>>>>> b/drivers/irqchip/irq-tegra.c @@ -44,6 +44,7 @@ static
> >>>>>>> unsigned int num_ictlrs;
> >>>>>>> struct tegra_ictlr_soc {
> >>>>>>> unsigned int num_ictlrs;
> >>>>>>> + bool supports_sc7;
> >>>>>>> };
> >>>>>>>
> >>>>>>> static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
> >>>>>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc
> >>>>>>> tegra30_ictlr_soc = {
> >>>>>>> static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
> >>>>>>> .num_ictlrs = 6,
> >>>>>>> + .supports_sc7 = true,
> >>>>>>> };
> >>>>>>>
> >>>>>>> static const struct of_device_id ictlr_matches[] = {
> >>>>>>> @@ -67,6 +69,7 @@ static const struct of_device_id
> >>>>>>> ictlr_matches[] = {
> >>>>>>> struct tegra_ictlr_info {
> >>>>>>> void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
> >>>>>>> + const struct tegra_ictlr_soc *soc;
> >>>>>>> #ifdef CONFIG_PM_SLEEP
> >>>>>>> u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
> >>>>>>> u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
> >>>>>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
> >>>>>>> lic->cop_ier[i] = readl_relaxed(ictlr +
> >>>>>>> ICTLR_COP_IER); lic->cop_iep[i] = readl_relaxed(ictlr +
> >>>>>>> ICTLR_COP_IEP_CLASS);
> >>>>>>> - /* Disable COP interrupts */
> >>>>>>> - writel_relaxed(~0ul, ictlr +
> >>>>>>> ICTLR_COP_IER_CLR);
> >>>>>>> + /*
> >>>>>>> + * AVP/COP/BPMP-Lite is the Tegra boot
> >>>>>>> processor.
> >>>>>>> + *
> >>>>>>> + * Tegra210 system suspend flow uses
> >>>>>>> sc7entry firmware which
> >>>>>>> + * is executed by COP/BPMP and it includes
> >>>>>>> disabling COP IRQ,
> >>>>>>> + * clamping CPU rail, turning off VDD_CPU,
> >>>>>>> and preparing the
> >>>>>>> + * system to go to SC7/LP0.
> >>>>>>> + *
> >>>>>>> + * COP/BPMP wakes up when COP IRQ is
> >>>>>>> triggered and runs
> >>>>>>> + * sc7entry-firmware. So need to keep COP
> >>>>>>> interrupt enabled.
> >>>>>>> + */
> >>>>>>> + if (!lic->soc->supports_sc7)
> >>>>>>> + /* Disable COP interrupts if SC7 is
> >>>>>>> not supported */
> >>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the
> >>>>>> comment doesn't sound correct to me. Something like
> >>>>>> 'firmware_sc7' should suit better here.
> >>>>> If what you're saying is true, then the whole patch is wrong,
> >>>>> and the SC7 property should come from DT.
> >>>> It should be safe to assume that all of existing Tegra210
> >>>> devices use the firmware for SC7, hence I wouldn't say that the
> >>>> patch is entirely wrong. To me it's not entirely correct.
> >>> Yes, all existing Tegra210 platforms uses sc7 entry firmware for
> >>> SC7 and AVP/COP IRQ need to be kept enabled as during suspend ATF
> >>> triggers IRQ to COP for SC7 entry fw execution.
> > Okay, as I already wrote before, it looks to me that a more proper
> > solution should be to just remove everything related to COP from
> > this driver instead of adding custom quirks for T210.
> >
> > The disabling / restoring of COP interrupts should be relevant only
> > for the multimedia firmware on older Tegra SoCs. That firmware
> > won't be ever supported in the upstream simply because NVIDIA
> > abandoned the support for older hardware in the downstream and
> > because it is not possible due to some legal weirdness (IIUC). The
> > only variant for upstream is reverse-engineering of hardware (not
> > the firmware BLOB) and writing proper opensource drivers for the
> > upstream kernel, which we're already doing and have success to a
> > some extent.
> >> That's not the question. Dmitry says that the SC7 support is not a
> >> property of the SoC, but mostly a platform decision on whether the
> >> firmware supports SC7 or not.
> >>
> >> To me, that's a clear indication that this should not be hardcoded
> >> in the driver, but instead obtained dynamically, via DT or
> >> otherwise.
> > We already have an nvidia,suspend-mode property in the device-tree
> > of the Power Management Controller node (all Tegra SoCs) which
> > defines what suspending type is supported by a particular board.
> >
> >>>>>>> + writel_relaxed(~0ul, ictlr +
> >>>>>>> ICTLR_COP_IER_CLR);
> >>>>>> Secondly, I'm also not sure why COP interrupts need to be
> >>>>>> disabled for pre-T210 at all, since COP is unused. This looks
> >>>>>> to me like it was cut-n-pasted from downstream kernel without
> >>>>>> a good reason and could be simply removed.
> >>>>> Please verify that this is actually the case. Tegra-2
> >>>>> definitely needed some level of poking, and I'm not keen on
> >>>>> changing anything there until you (or someone else) has
> >>>>> verified it on actual HW (see e307cc8941fc).
> >>>> Tested on Tegra20 and Tegra30, LP1 suspend-resume works
> >>>> perfectly fine with all COP bits removed from the driver.
> >>>>
> >>>> AFAIK, the reason why downstream needed that disabling is that
> >>>> it uses proprietary firmware which is running on the COP and
> >>>> that firmware is usually a BLOB audio/video DEC-ENC driver which
> >>>> doesn't cleanup interrupts after itself. That firmware is not
> >>>> applicable for the upstream kernel, hence there is no need to
> >>>> care about it.
> >>>>> Joseph, can you please shed some light here?
> >>> SC7 entry flow uses 3rd party ATF (arm-trusted FW) blob which is
> >>> the one that actually loads SC7 entry firmware and triggers IRQ to
> >>> AVP/COP which causes COP to wakeup and run SC7 entry FW.
> >>>
> >>> So when SC7 support is enabled, IRQ need to be kept enabled and
> >>> when SC7 FW starts execution, it will disable COP IRQ.
> >> This looks like a lot of undocumented assumptions on what firmware
> >> does, as well as what firmware *is*. What I gather from this
> >> thread is that there is at least two versions of firmware (a
> >> "proprietary firmware" for "downstream kernels", and another one
> >> for mainline), and that they do different things.
> >>
> >> Given that we cannot know what people actually run, I don't think
> >> we can safely remove anything unless this gets tested on the full
> >> spectrum of HW/FW combination.
> > I'm not sure whether multiple firmware variations exist in the wild
> > for Tegra210. Maybe Sowjanya or somebody else from NVIDIA could
> > clarify. I think there should be some efforts in regards to a fully
> > opensource firmware on Tegra210, but I'm not following it and have
> > no idea about the status.
> >
> > You're right that there are multiple variants of suspend-resuming
> > flow on Tegra SoCs. The older 32bit Tegra SoC generations have a
> > variety of options in regards to suspend-resuming, including
> > firmware-less variants on platforms that are having kernel running
> > in secure mode (dev boards, most of Tegra20 consumer devices) and
> > Trusted-Foundations firmware variant for insecure platforms
> > (consumer devices). And yes, vendor firmware creates a lot of
> > headache in regards to bringing support into upstream because it
> > usually does a lot of odd undocumented things which may also vary
> > depending on a firmware version (bootloader, etc) and it also
> > usually difficult to replace it with an opensource alternative due
> > to a crypto signing.
>
> Tried without this patch which keeps COP IRQ disabled and I see SC7
> entry FW execution happens still.
>
> Digging through the ATF FW code, I see on SC7 entry firmware loading
> into IRAM, COP processor is reset with RESET VECTOR set to SC7 entry
> firmware location in IRAM and on reset de-assert & unhalt COP, SC7
> firmware starts execution.
>
> Will remove this patch in next version...
>
Good, sounds like you also verified that SC7 COP firmware doesn't use
interrupts.
next prev parent reply other threads:[~2019-07-26 4:48 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-21 19:40 [PATCH V6 00/21] SC7 entry and exit support for Tegra210 Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend Sowjanya Komatineni
2019-07-21 20:24 ` Marc Zyngier
2019-07-22 9:54 ` Dmitry Osipenko
2019-07-22 10:13 ` Marc Zyngier
2019-07-22 10:57 ` Dmitry Osipenko
2019-07-22 16:21 ` Sowjanya Komatineni
2019-07-22 18:38 ` Marc Zyngier
2019-07-22 23:35 ` Dmitry Osipenko
2019-07-24 23:09 ` Sowjanya Komatineni
2019-07-26 4:48 ` Dmitry Osipenko [this message]
2019-07-25 9:55 ` Peter De Schrijver
2019-07-25 10:05 ` Dmitry Osipenko
2019-07-25 10:33 ` Peter De Schrijver
2019-07-25 10:38 ` Peter De Schrijver
2019-07-25 10:59 ` Dmitry Osipenko
2019-08-02 13:05 ` Peter De Schrijver
2019-08-02 17:35 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 02/21] pinctrl: tegra: Add suspend and resume support Sowjanya Komatineni
2019-07-21 22:03 ` Dmitry Osipenko
2019-07-21 22:09 ` Dmitry Osipenko
2019-07-21 22:48 ` Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 03/21] pinctrl: tegra210: Add Tegra210 pinctrl pm ops Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 04/21] clk: tegra: Save and restore divider rate Sowjanya Komatineni
2019-07-21 22:14 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 05/21] clk: tegra: pllout: Save and restore pllout context Sowjanya Komatineni
2019-07-21 22:18 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 06/21] clk: tegra: pll: Save and restore pll context Sowjanya Komatineni
2019-07-21 21:44 ` Dmitry Osipenko
2019-07-21 22:47 ` Sowjanya Komatineni
2019-07-21 22:21 ` Dmitry Osipenko
2019-07-22 3:22 ` Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 07/21] clk: tegra: Support for OSC context save and restore Sowjanya Komatineni
2019-07-22 10:12 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 08/21] clk: tegra: clk-periph: Add save and restore support Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 09/21] clk: tegra: clk-super: Fix to enable PLLP branches to CPU Sowjanya Komatineni
2019-07-21 21:16 ` Dmitry Osipenko
2019-07-21 22:39 ` Sowjanya Komatineni
2019-07-22 3:17 ` Sowjanya Komatineni
2019-07-22 6:32 ` Dmitry Osipenko
2019-07-22 7:12 ` Sowjanya Komatineni
2019-07-22 7:17 ` Dmitry Osipenko
2019-07-22 7:24 ` Sowjanya Komatineni
2019-07-22 7:30 ` Dmitry Osipenko
2019-07-22 7:36 ` Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 10/21] clk: tegra: clk-super: Add save and restore support Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 11/21] clk: tegra: clk-dfll: Add suspend and resume support Sowjanya Komatineni
2019-07-21 21:32 ` Dmitry Osipenko
2019-07-21 22:42 ` Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 12/21] cpufreq: tegra124: " Sowjanya Komatineni
2019-07-21 21:04 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 13/21] clk: tegra210: Use fence_udelay during PLLU init Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 14/21] clk: tegra210: Add suspend and resume support Sowjanya Komatineni
2019-07-21 21:38 ` Dmitry Osipenko
2019-07-21 22:45 ` Sowjanya Komatineni
2019-07-22 6:10 ` Dmitry Osipenko
2019-07-22 6:52 ` Sowjanya Komatineni
2019-07-22 7:09 ` Dmitry Osipenko
2019-07-22 7:12 ` Dmitry Osipenko
2019-08-02 17:51 ` Stephen Boyd
2019-08-02 20:39 ` Sowjanya Komatineni
2019-08-07 21:22 ` Stephen Boyd
2019-07-21 19:40 ` [PATCH V6 15/21] soc/tegra: pmc: Allow to support more tegras wake Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 16/21] soc/tegra: pmc: Add pmc wake support for tegra210 Sowjanya Komatineni
2019-07-23 0:58 ` Dmitry Osipenko
2019-07-23 1:08 ` Dmitry Osipenko
2019-07-23 1:41 ` Dmitry Osipenko
2019-07-23 1:52 ` Sowjanya Komatineni
2019-07-23 3:03 ` Dmitry Osipenko
2019-07-23 3:09 ` Sowjanya Komatineni
2019-07-23 3:25 ` Dmitry Osipenko
2019-07-23 3:31 ` Sowjanya Komatineni
2019-07-23 3:43 ` Dmitry Osipenko
2019-07-23 14:27 ` Dmitry Osipenko
2019-07-23 23:39 ` Sowjanya Komatineni
2019-07-24 9:31 ` Dmitry Osipenko
2019-07-23 1:52 ` Dmitry Osipenko
2019-07-23 2:10 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 17/21] arm64: tegra: Enable wake from deep sleep on RTC alarm Sowjanya Komatineni
2019-07-26 6:30 ` Dmitry Osipenko
2019-07-21 19:40 ` [PATCH V6 18/21] soc/tegra: pmc: Configure core power request polarity Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 19/21] soc/tegra: pmc: Configure deep sleep control settings Sowjanya Komatineni
2019-07-21 19:40 ` [PATCH V6 20/21] arm64: dts: tegra210-p2180: Jetson TX1 SC7 timings Sowjanya Komatineni
2019-07-21 19:41 ` [PATCH V6 21/21] arm64: dts: tegra210-p3450: Jetson nano " Sowjanya Komatineni
2019-07-21 22:25 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190726074811.39e0e6cb@dimatab \
--to=digetx@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jason@lakedaemon.net \
--cc=jckuo@nvidia.com \
--cc=jonathanh@nvidia.com \
--cc=josephl@nvidia.com \
--cc=linus.walleij@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=mperttunen@nvidia.com \
--cc=pdeschrijver@nvidia.com \
--cc=pgaikwad@nvidia.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=skomatineni@nvidia.com \
--cc=spatra@nvidia.com \
--cc=stefan@agner.ch \
--cc=talho@nvidia.com \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).