On Thu, Feb 15, 2024 at 01:40:45PM -0600, Ben Cheatham wrote:
> Add programming of CXL.mem transaction timeout range (CXL 3.0 8.2.4.23.1
> bits 4 & 11:8) through sysfs. To program the device, read the ranges
> supported from "available_timeout_ranges" in the PCIe service device
> directory, then write the desired value to "timeout_range". The current
> value can be found by reading from "timeout_range".
>
> Example for CXL-enabled PCIe port 0000:0c:00.0, with CXL timeout
> service as 020:
> # cd /sys/bus/pci_express/devices/0000:0c:00.0:pcie020
I would really, really like to avoid adding sysfs dependences on
portdrv. Is there any chance these files could go in the normal
/sys/bus/pci/devices/ hierarchy instead?
> +/* CXL 3.0 8.2.4.23.2 CXL Timeout and Isolation Control Register, bits 3:0 */
> +#define CXL_TIMEOUT_TIMEOUT_RANGE_DEFAULT 0x0
> +#define CXL_TIMEOUT_TIMEOUT_RANGE_D2 0xE
Looks like the single other example in this file of hex constants
using A-F uses lower-case.
Does "TIMEOUT_TIMEOUT" add information over just "TIMEOUT"?
> +#define NUM_CXL_TIMEOUT_RANGES 9
I don't think we actually need this constant, do we?
> +static bool cxl_validate_timeout_range(struct cxl_timeout *cxlt, u8 range)
"validate" is not a very name for a function returning "bool" because
you can't tell what true/false means from the name. "valid" would be
fine.
> + pci_dbg(cxlt->dev->port,
> + "Timeout & isolation timeout set to range 0x%x\n", range);
I don't know CXL, but "timeout ... timeout" reads sort of strange. Is
it actually a timeout for a timeout? Maybe it was supposed to be
"transaction timeout"?
> +const struct cxl_timeout_range {
> + const char *str;
> + u8 range_val;
> +} cxl_timeout_ranges[NUM_CXL_TIMEOUT_RANGES] = {
Static?
Bjorn
