Re: [RFC PATCH 2/2] PCI: Add basic Compute eXpress Link DVSEC decode

["Sean V Kelley" <sean.v.kelley@linux.intel.com>]

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