Message-ID: <20190805141621.GT2640@lahna.fi.intel.com>
Date:   Mon, 5 Aug 2019 17:16:21 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     linux-kernel@...r.kernel.org,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>, Mario.Limonciello@...l.com,
        Anthony Wong <anthony.wong@...onical.com>,
        linux-acpi@...r.kernel.org
Subject: Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake

On Sun, Aug 04, 2019 at 08:25:10PM +0200, Lukas Wunner wrote:
> On Fri, Jul 05, 2019 at 12:57:59PM +0300, Mika Westerberg wrote:
> > @@ -891,16 +1020,23 @@ static int nhi_resume_noirq(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct tb *tb = pci_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = nhi_power_up(tb->nhi);
> 
> Some general thoughts:
> 
> You're enabling Force Power during the resume_noirq phase of the NHI.
> 
> If TBT devices were already connected before system sleep, the PCI core
> walks down the hierarchy during resume_noirq, puts each device into D0 and
> restores config space.  For this to work, the NHI must have set up tunnels
> to attached devices.  But it can hardly do that until Force Power is
> enabled.  Yet I cannot see any provision here which causes the hotplug
> bridges to wait for the NHI to finish its resume_noirq phase.
> 
> Don't you need that?

For the FW CM we don't because there is some magic ASL as part of the
ACPI power resources (shared between root ports and NHI) that causes the
firmware to establish tunnels as part of _ON() method.

> On Macs we have quirk_apple_wait_for_thunderbolt() to resolve this issue.
> 
> And a commit has been rotting on my development branch for years
> which replaces quirk_apple_wait_for_thunderbolt() with device links:
> 
> https://github.com/l1k/linux/commit/8cbb1d589660
> 
> 
> Also, you recently posted a patch stating that on Ice Lake, the NHI and
> hotplug bridges share an ACPI Power Resource:
> 
> https://patchwork.kernel.org/patch/11015233/
> 
> How does that relate to the Force Power and Go2Sx dance implemented in
> the present patch?  Does the ACPI Power Resource toggle the same Force
> Power and Go2Sx registers as this patch here?  Or is that an additional
> power management mechanism?

It is an additional mechanism (but I think it also touches force power
at some point). Force power here is not the same as with the discrete
chips. AFAIK it pretty much only loads and starts the firmware whereas
in discrete it actually controls power to the chip.

> If Force Power is off on the NHI, are the hotplug bridges' config and
> mmio spaces accessible?

Yes, they are all accessible.

> All of this should be documented in code comments or at least the
> commit message.

Will do.

> There's a quirk for Macs with Cactus Ridge to perform a Go2Sx dance
> during the suspend_noirq phase, quirk_apple_poweroff_thunderbolt().
> Can this be reimplemented using the infrastructure you're adding
> in this patch?

It might be possible to implement ->force_power for Cactus Ridge and do
the ACPI magic there instead. I'll try and see what it looks like.

> On Macs with Thunderbolt 1 and 2, there are ACPI methods to toggle
> Force Power in the NHI's namespace.  However disabling Force Power
> not only cuts power to the NHI but also to the integrated PCIe switch.
> For this reason, I've implemented power management on these Macs using
> a struct dev_pm_domain which is assigned to the upstream bridge of
> the integrated PCIe switch.  That way, power isn't cut until the
> Thunderbolt controller's top-most device in the hierarchy is runtime
> suspended:
> 
> https://github.com/l1k/linux/commit/4db7f0b1f5c9
> 
> I'm wondering if it would make sense to similarly assign a dev_pm_domain
> to the NHI on Ice Lake.

Using PM domain here does not feel right to me. The force power thing
here is not really about power (but I'm calling it that because that's
the what we use internally) so I think it should be part of normal
driver flows.

> One thing that bothers me a bit with the present patch is that nhi.c
> is cluttered up with code specific to Ice Lake.  That doesn't seem
> sustainable going forward as I expect we'll have to add plenty of
> other quirks once USB4 chipsets become available.  Ideally the file
> should contain only (or mostly) generic code and quirks should be
> contained in separate files which need not be compiled in on
> unrelated arches.

I originally intented to have ICL support in icm.c but I realized that
the same steps are most likely needed on Apple systems as well (I don't
have any so can't tell if that's really the case) and since these flows
are for the NHI I figured that's the correct place for it.

I agree with you that moving chip specific things to separate files
makes sense (such as icl.c and so on) in the long run to keep the driver
maintainable.

> That's why in the above-linked commit to add
> Apple-specific power management, I put all the code in a pm_apple.c
> and nhi.c only calls tb_pm_apple_init() / _fini(), which become
> empty inline stubs if CONFIG_ACPI or CONFIG_PM isn't enabled.
> 
> 
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -38,6 +40,60 @@
> >  #define MSIX_MAX_VECS		16
> >  
> >  #define NHI_MAILBOX_TIMEOUT	500 /* ms */
> > +#define LC_MAILBOX_TIMEOUT	500 /* ms */
> > +
> > +enum lc_mailbox_cmd {
> > +	LC_GO2SX = 0x02,
> > +	LC_GO2SX_NO_WAKE = 0x03,
> > +	LC_PREPARE_FOR_RESET = 0x21,
> > +};
> 
> Shouldn't these live together with the other LC macros in tb_regs.h?

I've used tb_regs.h to keep registers and definitions that belong to
Thunderbolt fabric. The other LC macros are there because they are used
to access LC over TBT.

Here we access it through NHI and only use it in nhi.c.

No strong feelings moving it to tb_regs.h if you still think that's
more suitable.

> > +	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);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Why not:
> 
> 	if (ret)
> 		return 0;
> 
> and that way reduce the indentation of the code in your if-block by 1 tab?

Indeed. Will do.

Powered by blists - more mailing lists