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;
IronPort-SDR: uyzgEvSHXpWmhmMiruhGtvG5t21v36jwwuG/m/OUG0pAli2ba9knnlFNBqPwjHGn72CrmnlCO8
 oCE0Ecdi7jPg==
IronPort-SDR: IhVOBDmjxlDK9Sd8+sJGPG+B4+FkqFiJ1cAgWab3U9jtR3iv4eNAqfxXRYeZUF8b1LxiPs5Z1P
 Rqps0i5b8W/A==
From:   "Sean V Kelley" <sean.v.kelley@linux.intel.com>
To:     "Bjorn Helgaas" <helgaas@kernel.org>
Cc:     bhelgaas@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] PCI: Add basic Compute eXpress Link DVSEC decode
Date:   Fri, 15 May 2020 15:28:17 -0700
Message-ID: <55BAB3D3-5C7F-455A-96E6-7C5672510021@linux.intel.com>
In-Reply-To: <20200515210435.GA544190@bjorn-Precision-5520>
References: <20200515210435.GA544190@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 15 May 2020, at 14:04, Bjorn Helgaas wrote:

> 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.
>

Correct.  Understood. I’ve additional changes for CXL.mem/.cache.  
I’ll queue those up.

>> +	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().

Will do.

>
>> +	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.

Will do.

Thanks,

Sean