Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 May 2020 16:04:35 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Sean V Kelley <sean.v.kelley@linux.intel.com>
Cc:     bhelgaas@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Felipe Balbi <felipe.balbi@linux.intel.com>
Subject: Re: [RFC PATCH 2/2] PCI: Add basic Compute eXpress Link DVSEC decode
Message-ID: <20200515210435.GA544190@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200515175528.980103-3-sean.v.kelley@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 15, 2020 at 10:55:28AM -0700, Sean V Kelley wrote:
> Compute eXpress Link is a new CPU interconnect created with
> workload accelerators in mind. The interconnect relies on PCIe Electrical
> and Physical interconnect for communication. CXL devices enumerate to the
> OS as an ACPI-described PCIe Root Complex Integrated Endpoint.
> 
> This patch introduces the bare minimum support by simply looking for and
> caching the DVSEC CXL Extended Capability. Currently, only CXL.io (which
> is mandatory to be configured by BIOS) is enabled. In future, we will
> also add support for CXL.cache and CXL.mem.

This looks fine, but AFAICT, it doesn't *do* anything yet (except
print a few things to dmesg).  We don't normally merge code until it
adds some new functionality.  So just FYI that I'll wait until that
new functionality comes along and then merge this as part of that
series.  But let me know if I'm missing something.

> +	dev_info(&dev->dev, "CXL: Cache%c IO%c Mem%c Viral%c HDMCount %d\n",
> +		 (cap & PCI_CXL_CACHE) ? '+' : '-',
> +		 (cap & PCI_CXL_IO) ? '+' : '-',
> +		 (cap & PCI_CXL_MEM) ? '+' : '-',
> +		 (cap & PCI_CXL_VIRAL) ? '+' : '-',
> +		 PCI_CXL_HDM_COUNT(cap));

These could use pci_info() and FLAG(), as in pcie_init().

> +	dev_info(&dev->dev, "CXL: cap ctrl status ctrl2 status2 lock\n");
> +	dev_info(&dev->dev, "CXL: %04x %04x %04x %04x %04x %04x\n",
> +		 cap, ctrl, status, ctrl2, status2, lock);
> +}

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -315,6 +315,7 @@ struct pci_dev {
>  	u16		aer_cap;	/* AER capability offset */
>  	struct aer_stats *aer_stats;	/* AER stats for this device */
>  #endif
> +	u16		cxl_cap;	/* CXL capability offset */

Wrap in #ifdef PCI_CXL.