Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake

[Yehezkel Bernat]

On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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;
> +}
> +