Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190705095800.43534-1-mika.westerberg@linux.intel.com> <20190705095800.43534-8-mika.westerberg@linux.intel.com>
In-Reply-To: <20190705095800.43534-8-mika.westerberg@linux.intel.com>
From:   Yehezkel Bernat <yehezkelshb@gmail.com>
Date:   Fri, 5 Jul 2019 17:44:01 +0300
Message-ID: <CA+CmpXsak9Rvkq_RNzoxRecMercUPKqdK+KzbHv_fJC59inaHA@mail.gmail.com>
Subject: Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
To:     Mika Westerberg <mika.westerberg@linux.intel.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Len Brown <lenb@kernel.org>, Lukas Wunner <lukas@wunner.de>,
        Mario Limonciello <Mario.Limonciello@dell.com>,
        Anthony Wong <anthony.wong@canonical.com>,
        linux-acpi@vger.kernel.org, raanan.avargil@intel.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> +static void icm_icl_rtd3_veto(struct tb *tb, const struct icm_pkg_header *hdr)
> +{
> +       const struct icm_icl_event_rtd3_veto *pkg =
> +               (const struct icm_icl_event_rtd3_veto *)hdr;
> +       struct icm *icm = tb_priv(tb);
> +
> +       tb_dbg(tb, "ICM rtd3 veto=0x%08x\n", pkg->veto_reason);
> +
> +       if (pkg->veto_reason) {
> +               if (!icm->veto) {
> +                       icm->veto = true;
> +                       /* Keep the domain powered while veto is in effect */
> +                       pm_runtime_get(&tb->dev);
> +               }
> +       } else {
> +               if (icm->veto) {
> +                       icm->veto = false;
> +                       /* Allow the domain suspend now */
> +                       pm_runtime_mark_last_busy(&tb->dev);
> +                       pm_runtime_put_autosuspend(&tb->dev);

Handling the removal of the veto is duplicated below. Worth introducing as a
helper function?

> +               }
> +       }
> +}
> +

...

> @@ -1853,6 +1943,18 @@ static void icm_complete(struct tb *tb)
>         if (tb->nhi->going_away)
>                 return;
>
> +       /*
> +        * If RTD3 was vetoed before we entered system suspend allow it
> +        * again now before driver ready is sent. Firmware sends a new RTD3
> +        * veto if it is still the case after we have sent it driver ready
> +        * command.
> +        */
> +       if (icm->veto) {
> +               icm->veto = false;
> +               pm_runtime_mark_last_busy(&tb->dev);
> +               pm_runtime_put_autosuspend(&tb->dev);
> +       }
> +

Here is the duplication.


> +static int nhi_suspend_power_down(struct tb *tb)
> +{
> +       int ret;
> +
> +       /*
> +        * If there is no device connected we need to perform an additional
> +        * handshake through LC mailbox and force power down before
> +        * entering D3.
> +        */
> +       ret = device_for_each_child(&tb->root_switch->dev, NULL,
> +                                   nhi_device_connected);
> +       if (!ret) {
> +               lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> +               ret = lc_mailbox_cmd_complete(tb->nhi,
> +                                             LC_MAILBOX_TIMEOUT);
> +               if (ret)
> +                       return ret;
> +
> +               return nhi_power_down(tb->nhi);

Just to be sure: unforce power is done only if no device is connected?
My understanding of the comment above was that unforce power should be done
anyway (so it should be outside of this if block), and the difference between
the cases is only about the additional LC mailbox message. I guess I misread it.

> +       }
> +
> +       return 0;
> +}
> +