Re: [PATCH 12/18] cxl: Add helpers to calculate pci latency for the CXL device

[Dave Jiang <dave.jiang@intel.com>]

On 2/6/23 3:39 PM, Bjorn Helgaas wrote:
> On Mon, Feb 06, 2023 at 01:51:10PM -0700, Dave Jiang wrote:
>> The latency is calculated by dividing the FLIT size over the bandwidth. Add
>> support to retrieve the FLIT size for the CXL device and calculate the
>> latency of the downstream link.
> 
> s/FLIT/flit/ to match spec usage.

ok will fix.

> 
> Most of this looks like PCIe, not necessarily CXL-specific.
> 
> I guess you only care about the latency of a single link, not the
> entire path?

I am adding each of the link individually together in the next patch. 
Are you suggesting a similar function like pcie_bandwidth_available() 
but for latency for the entire path?
> 
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/pci.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxlpci.h   |   14 ++++++++++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index a24dac36bedd..54ac6f8825ff 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -633,3 +633,70 @@ void read_cdat_data(struct cxl_port *port)
>>   	}
>>   }
>>   EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>> +
>> +static int pcie_speed_to_mbps(enum pci_bus_speed speed)
>> +{
>> +	switch (speed) {
>> +	case PCIE_SPEED_2_5GT:
>> +		return 2500;
>> +	case PCIE_SPEED_5_0GT:
>> +		return 5000;
>> +	case PCIE_SPEED_8_0GT:
>> +		return 8000;
>> +	case PCIE_SPEED_16_0GT:
>> +		return 16000;
>> +	case PCIE_SPEED_32_0GT:
>> +		return 32000;
>> +	case PCIE_SPEED_64_0GT:
>> +		return 64000;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int cxl_pci_mbits_to_mbytes(struct pci_dev *pdev)
>> +{
>> +	int mbits;
>> +
>> +	mbits = pcie_speed_to_mbps(pcie_get_speed(pdev));
>> +	if (mbits < 0)
>> +		return mbits;
>> +
>> +	return mbits >> 3;
>> +}
>> +
>> +static int cxl_get_flit_size(struct pci_dev *pdev)
>> +{
>> +	if (cxl_pci_flit_256(pdev))
>> +		return 256;
>> +
>> +	return 66;
> 
> I don't know about the 66-byte flit format, maybe this part is
> CXL-specific?

68-byte flit format. Looks like this is a typo from me.

> 
>> + * cxl_pci_get_latency - calculate the link latency for the PCIe link
>> + * @pdev - PCI device
>> + *
>> + * CXL Memory Device SW Guide v1.0 2.11.4 Link latency calculation
>> + * Link latency = LinkPropagationLatency + FlitLatency + RetimerLatency
>> + * LinkProgationLatency is negligible, so 0 will be used
>> + * RetimerLatency is assumed to be neglibible and 0 will be used
> 
> s/neglibible/negligible/

thank you will fix.
> 
>> + * FlitLatency = FlitSize / LinkBandwidth
>> + * FlitSize is defined by spec. CXL v3.0 4.2.1.
>> + * 68B flit is used up to 32GT/s. >32GT/s, 256B flit size is used.
>> + * The FlitLatency is converted to pico-seconds.
> 
> I guess this means cxl_pci_get_latency() actually *returns* a value in
> picoseconds?

yes

> 
> There are a couple instances of this written as "pico-seconds", but
> most are "picoseconds".

ok will fix.

> 
>> +long cxl_pci_get_latency(struct pci_dev *pdev)
>> +{
>> +	long bw, flit_size;
>> +
>> +	bw = cxl_pci_mbits_to_mbytes(pdev);
>> +	if (bw < 0)
>> +		return bw;
>> +
>> +	flit_size = cxl_get_flit_size(pdev);
>> +	return flit_size * 1000000L / bw;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_get_latency, CXL);
>> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
>> index 920909791bb9..d64a3e0458ab 100644
>> --- a/drivers/cxl/cxlpci.h
>> +++ b/drivers/cxl/cxlpci.h
>> @@ -62,8 +62,22 @@ enum cxl_regloc_type {
>>   	CXL_REGLOC_RBI_TYPES
>>   };
>>   
>> +/*
>> + * CXL v3.0 6.2.3 Table 6-4
> 
> The copy I have refers to *Revision 3.0, Version 1.0*, i.e.,
> "Revision" is the major level and "Version" is the minor.  So I would
> cite this as "CXL r3.0", not "CXL v3.0".  I suppose the same for CXL
> Memory Device above, but I don't have that spec.

Ok will fix.

> 
>> + * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits
>> + * mode, otherwise it's 68B flits mode.
>> + */
>> +static inline bool cxl_pci_flit_256(struct pci_dev *pdev)
>> +{
>> +	u32 lnksta2;
>> +
>> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKSTA2, &lnksta2);
>> +	return lnksta2 & BIT(10);
> 
> Add a #define for the bit.

ok will add.

> 
> AFAICT, the PCIe spec defines this bit, and it only indicates the link
> is or will be operating in Flit Mode; it doesn't actually say anything
> about how large the flits are.  I suppose that's because PCIe only
> talks about 256B flits, not 66B ones?

Looking at CXL v1.0 rev3.0 6.2.3 "256B Flit Mode", table 6-4, it shows 
that when PCIe Flit Mode is set, then CXL is in 256B flits mode, 
otherwise, it is 68B flits. So an assumption is made here regarding the 
flit side based on the table.

> 
> Bjorn