From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936658AbdAKKBW convert rfc822-to-8bit (ORCPT ); Wed, 11 Jan 2017 05:01:22 -0500 Received: from mga05.intel.com ([192.55.52.43]:56330 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934306AbdAKKBU (ORCPT ); Wed, 11 Jan 2017 05:01:20 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,345,1477983600"; d="scan'208";a="1081701405" From: "Winkler, Tomas" To: Lukas Wunner CC: Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , Andreas Noever , "linux-pci@vger.kernel.org" , "linux-pm@vger.kernel.org" , "Chen, Yu C" , "Levy, Amir (Jer)" , Bjorn Helgaas Subject: RE: [PATCH v4 1/8] PCI: Recognize Thunderbolt devices Thread-Topic: [PATCH v4 1/8] PCI: Recognize Thunderbolt devices Thread-Index: AQHSaYsiGVM8U1l34Uy4pkMdJEFVl6EuXYvQgAAZc4CABJjKMA== Date: Wed, 11 Jan 2017 10:01:14 +0000 Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B5437A15E@hasmsx108.ger.corp.intel.com> References: <5B8DA87D05A7694D9FA63FD143655C1B54376D27@hasmsx108.ger.corp.intel.com> <20170108134751.GA23120@wunner.de> In-Reply-To: <20170108134751.GA23120@wunner.de> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2U5OGU2NmYtNzA1MC00MzFlLTlhMjAtMTYzNTU4MGFkMGFhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ik1CTDlKMFA1VmVRdkRaU004QTBMXC9uMTh0KzQzTlpWSGVObkN5TTUwK3Z3PSJ9 x-originating-ip: [10.184.70.10] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > On Sun, Jan 08, 2017 at 10:23:03AM +0000, Winkler, Tomas wrote: > > > We're about to allow runtime PM on Thunderbolt ports in > > > pci_bridge_d3_possible() and unblock runtime PM for Thunderbolt host > > > hotplug ports in pci_dev_check_d3cold(). In both cases we need to > > > uniquely identify if a PCI device belongs to a Thunderbolt controller. > > > > > > We also have the need to detect presence of a Thunderbolt controller > > > in drivers/platform/x86/apple-gmux.c because dual GPU MacBook Pros > > > cannot switch external DP/HDMI ports between GPUs if they have > Thunderbolt. > > > > > > Furthermore, in multiple places in the DRM subsystem we need to > > > detect whether a GPU is on-board or attached with Thunderbolt. As > > > an example, Thunderbolt-attached GPUs shall not be registered with > vga_switcheroo. > > > > > > Intel uses a Vendor-Specific Extended Capability (VSEC) with ID > > > 0x1234 on devices belonging to a Thunderbolt controller which allows > > > us to recognize them. > > > > > > Detect presence of this VSEC on device probe and cache it in a newly > > > added is_thunderbolt bit in struct pci_dev which can then be queried > > > by pci_bridge_d3_possible(), pci_dev_check_d3cold(), apple-gmux and > others. > > > > > > Cc: Andreas Noever > > > Cc: Tomas Winkler > > > Cc: Amir Levy > > > Signed-off-by: Lukas Wunner > > > --- > > > drivers/pci/pci.h | 2 ++ > > > drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++++++++ > > > include/linux/pci.h | 1 + > > > 3 files changed, 37 insertions(+) > > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index > > > cb17db2..45c2b81 > > > 100644 > > > --- a/drivers/pci/pci.h > > > +++ b/drivers/pci/pci.h > > > @@ -3,6 +3,8 @@ > > > > > > #define PCI_FIND_CAP_TTL 48 > > > > > > +#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ > > > > Shouldn't bet this defined in pci_ids.h ? > > That file is solely intended for IDs used in multiple places, as explained in the > comment at its top. This particular ID is only used in the PCI core, hence it's in > the PCI core's private drivers/pci/pci.h. > > However the line is formatted such that it can just be moved to the global > include/linux/pci_ids.h should it be needed someplace else in the future. > > > > > extern const unsigned char pcie_link_speed[]; > > > > > > bool pcie_cap_has_lnkctl(const struct pci_dev *dev); diff --git > > > a/drivers/pci/probe.c b/drivers/pci/probe.c index e164b5c..891a8fa > > > 100644 > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -1206,6 +1206,37 @@ void set_pcie_hotplug_bridge(struct pci_dev > > > *pdev) > > > pdev->is_hotplug_bridge = 1; > > > } > > > > > > +static void set_pcie_vendor_specific(struct pci_dev *dev) { > > > > > > Why don't u implement this in quirk.c ? > > The is_thunderbolt bit signifies whether a given PCI device is part of a > Thunderbolt daisy chain. (As explained in the comment in include/linux/pci.h, > see below.) So it is set on all the PCI devices that comprise a Thunderbolt > controller, but also on all endpoint devices that are attached via Thunderbolt. > > Consequently this function needs to be executed for all PCI devices, not just > for a subset that could be identified by a vendor, device or class ID. Ok, god it. > > In the DRM drivers nouveau, radeon and amdgpu I need to prevent calls to > vga_switcheroo_register_client() and vga_switcheroo_init_domain_pm_ops() > if the device in question is attached via Thunderbolt. That's because external > GPUs can't drive the panel of a laptop or be powered down like an on-board > Optimus/PowerXpress GPU. We've got a bug there right now that manifests > itself once someone attaches an eGPU to a dual GPU laptop. > With this patch I'll be able to just skip registration with vga_switcheroo if > is_thunderbolt is set on a GPU's pci_dev, thereby fixing the bug. > > > BTW do you have information on the contents/meaning of the VSEC that you > would be able/allowed to share? What I've seen so far is that its length is 0x1c > bytes on older controllers (e.g. Light Ridge, Port Ridge) and its contents are > always the same, regardless if the controller is used in host mode or endpoint > mode: Sorry, currently nothing I can share. > 500: 0b 00 01 00 34 12 c1 01|50 09 00 00 39 00 00 00 > 510: 00 00 00 00 00 00 00 00 00 00 00 00 > > However on Alpine Ridge the size of the VSEC has increased significantly to > 0xd8 bytes, even though the version stayed at 1 as before: > > 500: 0b 00 01 60 34 12 81 0d 50 09 b0 0c b9 06 18 08 > 510: 00 38 00 01 00 00 00 00 00 00 00 00 32 02 10 00 > 520: ef d3 00 40 00 00 00 00 f0 f0 30 c1 00 02 30 00 > 530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 590: 00 00 00 08 00 00 00 00 00 00 00 00 00 00 00 00 > 5a0: 00 00 00 00 00 00 00 00 00 00 00 00 38 24 01 00 > 5b0: 08 40 00 1c 00 00 01 18 04 03 00 f0 06 00 00 00 > 5c0: 00 00 08 40 06 00 00 01 90 00 08 1b 00 08 80 08 > 5d0: 80 7f 08 00 00 00 00 00 > > Thanks, > > Lukas > > > > > > + int vsec = 0; > > > + u32 header; > > > + > > > + while ((vsec = pci_find_next_ext_capability(dev, vsec, > > > + PCI_EXT_CAP_ID_VNDR))) { > > > + pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, > > > &header); > > > + > > > + /* Is the device part of a Thunderbolt controller? */ > > > + if (dev->vendor == PCI_VENDOR_ID_INTEL && > > > + PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) > > > + dev->is_thunderbolt = 1; > > > + } > > > + > > > + /* > > > + * Is the device attached with Thunderbolt? Walk upwards and > > > +check > > > for > > > + * each encountered bridge if it's part of a Thunderbolt controller. > > > + * Reaching the host bridge means dev is soldered to the mainboard. > > > + */ > > > + if (!dev->is_thunderbolt) { > > > + struct pci_dev *parent = dev; > > > + > > > + while ((parent = pci_upstream_bridge(parent))) > > > + if (parent->is_thunderbolt) { > > > + dev->is_thunderbolt = 1; > > > + break; > > > + } > > > + } > > > +} > > > + > > > /** > > > * pci_ext_cfg_is_aliased - is ext config space just an alias of std config? > > > * @dev: PCI device > > > @@ -1358,6 +1389,9 @@ int pci_setup_device(struct pci_dev *dev) > > > /* need to have dev->class ready */ > > > dev->cfg_size = pci_cfg_space_size(dev); > > > > > > + /* need to have dev->cfg_size ready */ > > > + set_pcie_vendor_specific(dev); > > > + > > > /* "Unknown power state" */ > > > dev->current_state = PCI_UNKNOWN; > > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h index > > > e2d1a12..3c775e8 > > > 100644 > > > --- a/include/linux/pci.h > > > +++ b/include/linux/pci.h > > > @@ -358,6 +358,7 @@ struct pci_dev { > > > unsigned int is_virtfn:1; > > > unsigned int reset_fn:1; > > > unsigned int is_hotplug_bridge:1; > > > + unsigned int is_thunderbolt:1; /* part of Thunderbolt daisy chain > */ > > > unsigned int __aer_firmware_first_valid:1; > > > unsigned int __aer_firmware_first:1; > > > unsigned int broken_intx_masking:1; > > > -- > > > 2.11.0