Re: [PATCH v4 10/23] cxl: Add helpers to calculate pci latency for the CXL device

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

On 4/24/23 5:30 PM, Dan Williams wrote:
> 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> ---
>> v2:
>> - Fix commit log issues. (Jonathan)
>> - Fix var declaration issues. (Jonathan)
>> ---
>>   drivers/cxl/core/pci.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxlpci.h   |   15 +++++++++++
>>   drivers/cxl/pci.c      |   13 ---------
>>   3 files changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 1c415b26e866..bb58296b3e56 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -712,3 +712,71 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>>   	return PCI_ERS_RESULT_NEED_RESET;
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_error_detected, CXL);
>> +
>> +static int pci_bus_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 = pci_bus_speed_to_mbps(pdev->bus->cur_bus_speed);
>> +	if (mbits < 0)
>> +		return mbits;
>> +
>> +	return mbits >> 3;
> 
> Why not just return mbits directly and skip the conversion? Otherwise a
> "/ 8" requires bit less cleverness to read than ">> 3".

You mean just move the math to the caller()?
> 
>> +}
>> +
>> +static int cxl_flit_size(struct pci_dev *pdev)
> 
> This like something that might be worth caching in 'struct cxl_port'
> rather than re-reading the configuration register each call. Depends on
> how often it is used.

You mean we just calculate it during cxl_port creation? I think the 
calculations for a switch upstream segment towards the root complex may 
be used multiple times. Downstream towards device, 1 or more depends on 
how many partitions there are. But probably not a big deal to just cache 
it.

> 
>> +{
>> +	if (cxl_pci_flit_256(pdev))
>> +		return 256;
>> +
>> +	return 68;
>> +}
>> +
>> +/**
>> + * cxl_pci_get_latency - calculate the link latency for the PCIe link
>> + * @pdev - PCI device
>> + *
>> + * return: calculated latency or -errno
>> + *
>> + * 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 negligible and 0 will be used
>> + * FlitLatency = FlitSize / LinkBandwidth
>> + * FlitSize is defined by spec. CXL rev3.0 4.2.1.
>> + * 68B flit is used up to 32GT/s. >32GT/s, 256B flit size is used.
>> + * The FlitLatency is converted to picoseconds.
>> + */
>> +long cxl_pci_get_latency(struct pci_dev *pdev)
>> +{
>> +	long bw;
>> +
>> +	bw = cxl_pci_mbits_to_mbytes(pdev);
> 
> This function looks misnamed when I read it here, it's retrieving the
> bus speed in MiBs not doing a conversion.
> 
>> +	if (bw < 0)
>> +		return bw;
>> +
>> +	return cxl_flit_size(pdev) * 1000000L / bw;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_get_latency, CXL);
>> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
>> index 1bca1c0e4b40..795eba31fe29 100644
>> --- a/drivers/cxl/cxlpci.h
>> +++ b/drivers/cxl/cxlpci.h
>> @@ -167,6 +167,19 @@ struct cdat_sslbis {
>>   #define SSLBIS_US_PORT		0x0100
>>   #define SSLBIS_ANY_PORT		0xffff
>>   
>> +/*
>> + * CXL v3.0 6.2.3 Table 6-4
>> + * 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)
>> +{
>> +	u16 lnksta2;
>> +
>> +	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA2, &lnksta2);
>> +	return lnksta2 & PCI_EXP_LNKSTA2_FLIT;
>> +}
>> +
>>   int devm_cxl_port_enumerate_dports(struct cxl_port *port);
>>   struct cxl_dev_state;
>>   int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>> @@ -189,4 +202,6 @@ int cxl_##x##_parse_entry(struct cdat_entry_header *header, void *arg)
>>   cxl_parse_entry(dsmas);
>>   cxl_parse_entry(dslbis);
>>   cxl_parse_entry(sslbis);
>> +
>> +long cxl_pci_get_latency(struct pci_dev *pdev);
>>   #endif /* __CXL_PCI_H__ */
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index ea38bd49b0cf..ed39d133b70d 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -365,19 +365,6 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
>>   	return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END;
>>   }
>>   
>> -/*
>> - * CXL v3.0 6.2.3 Table 6-4
>> - * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits
>> - * mode, otherwise it's 68B flits mode.
>> - */
>> -static bool cxl_pci_flit_256(struct pci_dev *pdev)
>> -{
>> -	u16 lnksta2;
>> -
>> -	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA2, &lnksta2);
>> -	return lnksta2 & PCI_EXP_LNKSTA2_FLIT;
>> -}
>> -
>>   static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>   {
>>   	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
>>
>>
> 
>