[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1eeea670-4599-cd13-1d2b-7989c2544338@amd.com>
Date: Fri, 27 Dec 2024 07:00:50 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v8 02/27] sfc: add cxl support using new CXL API
On 12/24/24 17:04, Jonathan Cameron wrote:
> On Mon, 16 Dec 2024 16:10:17 +0000
> <alejandro.lucero-palau@....com> wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Add CXL initialization based on new CXL API for accel drivers and make
>> it dependent on kernel CXL configuration.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@...il.com>
>> Acked-by: Edward Cree <ecree.xilinx@...il.com>
> Hi Alejandro
>
> A few minor comments inline. Assuming those are tidied up.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> new file mode 100644
>> index 000000000000..356d7a977e1c
>> --- /dev/null
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -0,0 +1,87 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/****************************************************************************
>> + *
>> + * Driver for AMD network controllers and boards
>> + * Copyright (C) 2024, Advanced Micro Devices, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation, incorporated herein by reference.
>> + */
>> +
>> +#include <cxl/cxl.h>
>> +#include <cxl/pci.h>
>> +#include <linux/pci.h>
>> +
>> +#include "net_driver.h"
>> +#include "efx_cxl.h"
>> +
>> +#define EFX_CTPIO_BUFFER_SIZE SZ_256M
>> +
>> +int efx_cxl_init(struct efx_probe_data *probe_data)
>> +{
>> + struct efx_nic *efx = &probe_data->efx;
>> + struct pci_dev *pci_dev;
>> + struct efx_cxl *cxl;
>> + struct resource res;
>> + u16 dvsec;
>> + int rc;
>> +
>> + pci_dev = efx->pci_dev;
> Trivial, but maybe put that one inline at the declaration above.
Sure.
>> + probe_data->cxl_pio_initialised = false;
>> +
>> + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL,
>> + CXL_DVSEC_PCIE_DEVICE);
>> + if (!dvsec)
>> + return 0;
>> +
>> + pci_dbg(pci_dev, "CXL_DVSEC_PCIE_DEVICE capability found\n");
>> +
>> + cxl = kzalloc(sizeof(*cxl), GFP_KERNEL);
>> + if (!cxl)
>> + return -ENOMEM;
>> +
>> + cxl->cxlds = cxl_accel_state_create(&pci_dev->dev);
>> + if (IS_ERR(cxl->cxlds)) {
>> + pci_err(pci_dev, "CXL accel device state failed");
>> + rc = -ENOMEM;
>> + goto err_state;
>> + }
>> +
>> + cxl_set_dvsec(cxl->cxlds, dvsec);
>> + cxl_set_serial(cxl->cxlds, pci_dev->dev.id);
>> +
>> + res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE);
>> + if (cxl_set_resource(cxl->cxlds, res, CXL_RES_DPA)) {
>> + pci_err(pci_dev, "cxl_set_resource DPA failed\n");
>> + rc = -EINVAL;
>> + goto err_resource_set;
>> + }
>> +
>> + res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>> + if (cxl_set_resource(cxl->cxlds, res, CXL_RES_RAM)) {
>> + pci_err(pci_dev, "cxl_set_resource RAM failed\n");
>> + rc = -EINVAL;
>> + goto err_resource_set;
>> + }
>> +
>> + probe_data->cxl = cxl;
>> +
>> + return 0;
>> +
>> +err_resource_set:
>> + kfree(cxl->cxlds);
>> +err_state:
>> + kfree(cxl);
>> + return rc;
>> +}
>> +
>> +void efx_cxl_exit(struct efx_probe_data *probe_data)
>> +{
>> + if (probe_data->cxl) {
>> + kfree(probe_data->cxl->cxlds);
>> + kfree(probe_data->cxl);
>> + }
>> +}
>> +
>> +MODULE_IMPORT_NS("CXL");
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.h b/drivers/net/ethernet/sfc/efx_cxl.h
>> new file mode 100644
>> index 000000000000..90fa46bc94db
>> --- /dev/null
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/****************************************************************************
>> + * Driver for AMD network controllers and boards
>> + * Copyright (C) 2024, Advanced Micro Devices, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation, incorporated herein by reference.
>> + */
>> +
>> +#ifndef EFX_CXL_H
>> +#define EFX_CXL_H
>> +
>> +struct efx_nic;
> Not sure why you need this one, but...
I forgot to remove it after using efx_probe_data instead of efx_nic.
Good catch. I'll remove it.
> struct efx_probe_data;
> struct cxl_dev_state;
> struct cxl_memdev;
>
> etc.
>
> are probably a good idea to avoid potential issues with
> include reorderings in the future.
Yes.
Thanks
>
>> +
>> +struct efx_cxl {
>> + struct cxl_dev_state *cxlds;
>> + struct cxl_memdev *cxlmd;
>> + struct cxl_root_decoder *cxlrd;
>> + struct cxl_port *endpoint;
>> + struct cxl_endpoint_decoder *cxled;
>> + struct cxl_region *efx_region;
>> + void __iomem *ctpio_cxl;
>> +};
>> +
>> +int efx_cxl_init(struct efx_probe_data *probe_data);
>> +void efx_cxl_exit(struct efx_probe_data *probe_data);
>> +#endif
Powered by blists - more mailing lists