lkml.org 
[lkml]   [2019]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend
    From
    Date

    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...

    \
     
     \ /
      Last update: 2019-07-25 01:13    [W:5.336 / U:0.648 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site