On Tue, Nov 10, 2020 at 09:43:48PM -0800, Ben Widawsky wrote:
> +menuconfig CXL_BUS
> + tristate "CXL (Compute Express Link) Devices Support"
> + help
> + CXL is a bus that is electrically compatible with PCI-E, but layers
> + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The
> + CXL.cache protocol allows devices to hold cachelines locally, the
> + CXL.mem protocol allows devices to be fully coherent memory targets, the
> + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for
> + the configuration and management of devices supporting these protocols.
> +
Please fix the overly long lines.
> +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev)
Another overly long line.
> +{
> + dev_set_drvdata(dev, acpi_desc);
> + acpi_desc->dev = dev;
> +}
But this helper seems pretty pointless to start with.
> +static int acpi_cxl_remove(struct acpi_device *adev)
> +{
> + return 0;
> +}
The emptry remove callback is not needed.
> +/*
> + * If/when CXL support is defined by other platform firmware the kernel
> + * will need a mechanism to select between the platform specific version
> + * of this routine, until then, hard-code ACPI assumptions
> + */
> +int cxl_bus_prepared(struct pci_dev *pdev)
> +{
> + struct acpi_device *adev;
> + struct pci_dev *root_port;
> + struct device *root;
> +
> + root_port = pcie_find_root_port(pdev);
> + if (!root_port)
> + return -ENXIO;
> +
> + root = root_port->dev.parent;
> + if (!root)
> + return -ENXIO;
> +
> + adev = ACPI_COMPANION(root);
> + if (!adev)
> + return -ENXIO;
> +
> + /* TODO: OSC enabling */
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_bus_prepared);
What is the point of this function? I doesn't realy do anything,
not even a CXL specific check.
>
> +/*******************************************************************************
> + *
> + * CEDT - CXL Early Discovery Table (ACPI 6.4)
> + * Version 1
> + *
> + ******************************************************************************/
> +
Pleae use the normal Linux comment style.
> +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0)
> +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1)
> +
> +/* Values for length field above */
> +
> +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000)
> +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000)
No need for the braces.
