| Message ID | 20240715172835.24757-2-alejandro.lucero-palau@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add Type2 device support | expand |
> +++ b/include/linux/cxl_accel_mem.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#include <linux/cdev.h> That is generally a red flag that something not good is about to be found. But it does not appear to be used in this patch.... Andrew
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on cxl/pending v6.10 next-20240715]
[cannot apply to cxl/next horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/alejandro-lucero-palau-amd-com/cxl-add-type2-device-basic-support/20240716-015920
base: linus/master
patch link: https://lore.kernel.org/r/20240715172835.24757-2-alejandro.lucero-palau%40amd.com
patch subject: [PATCH v2 01/15] cxl: add type2 device basic support
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240716/202407160957.L4mIOUtI-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240716/202407160957.L4mIOUtI-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407160957.L4mIOUtI-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/ethernet/sfc/efx.c:8:
In file included from include/linux/filter.h:9:
In file included from include/linux/bpf.h:20:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2258:
include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
500 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
501 | item];
| ~~~~
include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
507 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
508 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
519 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
520 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
528 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
529 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/ethernet/sfc/efx.c:8:
In file included from include/linux/filter.h:12:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/net/ethernet/sfc/efx.c:8:
In file included from include/linux/filter.h:12:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/net/ethernet/sfc/efx.c:8:
In file included from include/linux/filter.h:12:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from drivers/net/ethernet/sfc/efx.c:36:
>> drivers/net/ethernet/sfc/efx_cxl.h:11:9: warning: 'EFX_CXL_H' is used as a header guard here, followed by #define of a different macro [-Wheader-guard]
11 | #ifndef EFX_CXL_H
| ^~~~~~~~~
drivers/net/ethernet/sfc/efx_cxl.h:12:9: note: 'EFX_CLX_H' is defined here; did you mean 'EFX_CXL_H'?
12 | #define EFX_CLX_H
| ^~~~~~~~~
| EFX_CXL_H
18 warnings generated.
vim +/EFX_CXL_H +11 drivers/net/ethernet/sfc/efx_cxl.h
> 11 #ifndef EFX_CXL_H
12 #define EFX_CLX_H
13
On 7/15/24 19:48, Andrew Lunn wrote: >> +++ b/include/linux/cxl_accel_mem.h >> @@ -0,0 +1,22 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ >> + >> +#include <linux/cdev.h> > That is generally a red flag that something not good is about to be > found. But it does not appear to be used in this patch.... > > Andrew > I have no explanation about how it ended up there. I suspect it comes from V1 --> V2 transition. cxlmem.h includes it and V1 was moving that file to include/linux. Anyway, I'll get rid of it. Thanks
On 7/15/24 10:28 AM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Differientiate Type3, aka memory expanders, from Type2, aka device > accelerators, with a new function for initializing cxl_dev_state. > > Create opaque struct to be used by accelerators relying on new access > functions in following patches. > > Add SFC ethernet network driver as the client. > > Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Co-developed-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/memdev.c | 52 ++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/Makefile | 2 +- > drivers/net/ethernet/sfc/efx.c | 4 ++ > drivers/net/ethernet/sfc/efx_cxl.c | 53 +++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/efx_cxl.h | 29 +++++++++++++++ > drivers/net/ethernet/sfc/net_driver.h | 4 ++ > include/linux/cxl_accel_mem.h | 22 +++++++++++ > include/linux/cxl_accel_pci.h | 23 ++++++++++++ Maybe create an include/linux/cxl and then we can put headers in there. > 8 files changed, 188 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c > create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h > create mode 100644 include/linux/cxl_accel_mem.h > create mode 100644 include/linux/cxl_accel_pci.h > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index 0277726afd04..61b5d35b49e7 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -8,6 +8,7 @@ > #include <linux/idr.h> > #include <linux/pci.h> > #include <cxlmem.h> > +#include <linux/cxl_accel_mem.h> > #include "trace.h" > #include "core.h" > > @@ -615,6 +616,25 @@ static void detach_memdev(struct work_struct *work) > > static struct lock_class_key cxl_memdev_key; > > +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) > +{ > + struct cxl_dev_state *cxlds; > + > + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); Naked cxlds. Do you think you'll need an accel_dev_state to wrap around cxl_dev_state similar to cxl_memdev_state in order to store accel related information? I also wonder if 'struct cxl_dev_state' should be a public definition. Need to look at the rest of the patchset to circle back. > + if (!cxlds) > + return ERR_PTR(-ENOMEM); > + > + cxlds->dev = dev; > + cxlds->type = CXL_DEVTYPE_DEVMEM; > + > + cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa"); > + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram"); > + cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem"); > + > + return cxlds; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); I do wonder if we should have a common device state init helper function to init all the common bits: int cxlds_init(struct *dev, enum cxl_devtype devtype) > + > static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds, > const struct file_operations *fops) > { > @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode *inode, struct file *file) > return 0; > } > > + > +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > +{ > + cxlds->cxl_dvsec = dvsec; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); > + > +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) > +{ > + cxlds->serial= serial; Missing space before '=' > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); > + > +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, > + enum accel_resource type) > +{ > + switch (type) { > + case CXL_ACCEL_RES_DPA: > + cxlds->dpa_res = res; > + return; > + case CXL_ACCEL_RES_RAM: > + cxlds->ram_res = res; > + return; > + case CXL_ACCEL_RES_PMEM: > + cxlds->pmem_res = res; > + return; > + default: > + dev_err(cxlds->dev, "unkown resource type (%u)\n", type); > + } > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); > + > static int cxl_memdev_release_file(struct inode *inode, struct file *file) > { > struct cxl_memdev *cxlmd = > diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile > index 8f446b9bd5ee..e80c713c3b0c 100644 > --- a/drivers/net/ethernet/sfc/Makefile > +++ b/drivers/net/ethernet/sfc/Makefile > @@ -7,7 +7,7 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ > mcdi_functions.o mcdi_filters.o mcdi_mon.o \ > ef100.o ef100_nic.o ef100_netdev.o \ > ef100_ethtool.o ef100_rx.o ef100_tx.o \ > - efx_devlink.o > + efx_devlink.o efx_cxl.o > sfc-$(CONFIG_SFC_MTD) += mtd.o > sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ > mae.o tc.o tc_bindings.o tc_counters.o \ > diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c > index e9d9de8e648a..cb3f74d30852 100644 > --- a/drivers/net/ethernet/sfc/efx.c > +++ b/drivers/net/ethernet/sfc/efx.c > @@ -33,6 +33,7 @@ > #include "selftest.h" > #include "sriov.h" > #include "efx_devlink.h" > +#include "efx_cxl.h" > > #include "mcdi_port_common.h" > #include "mcdi_pcol.h" > @@ -899,6 +900,7 @@ static void efx_pci_remove(struct pci_dev *pci_dev) > efx_pci_remove_main(efx); > > efx_fini_io(efx); > + stray blank line > pci_dbg(efx->pci_dev, "shutdown successful\n"); > > efx_fini_devlink_and_unlock(efx); > @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev *pci_dev, > if (rc) > goto fail2; > > + efx_cxl_init(efx); No error checks? Does the device expect to work whether CXL is setup or not? > + > rc = efx_pci_probe_post_io(efx); > if (rc) { > /* On failure, retry once immediately. > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > new file mode 100644 > index 000000000000..4554dd7cca76 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -0,0 +1,53 @@ > +// 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 <linux/pci.h> > +#include <linux/cxl_accel_mem.h> > +#include <linux/cxl_accel_pci.h> > + > +#include "net_driver.h" > +#include "efx_cxl.h" > + > +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256) > + > +void efx_cxl_init(struct efx_nic *efx) > +{ > + struct pci_dev *pci_dev = efx->pci_dev; > + struct efx_cxl *cxl = efx->cxl; > + struct resource res; > + u16 dvsec; > + > + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL, > + CXL_DVSEC_PCIE_DEVICE); > + > + if (!dvsec) > + return; > + > + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); Seem like unnecessary kern log emission > + > + cxl->cxlds = cxl_accel_state_create(&pci_dev->dev); > + if (IS_ERR(cxl->cxlds)) { > + pci_info(pci_dev, "CXL accel device state failed"); pci_err()? or maybe pci_warn() given it's ignoring error returns. > + return; > + } > + > + cxl_accel_set_dvsec(cxl->cxlds, dvsec); > + cxl_accel_set_serial(cxl->cxlds, pci_dev->dev.id); > + > + res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE); > + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_DPA); > + > + res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram"); > + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM); > +} > + > + > +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..76c6794c20d8 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_cxl.h > @@ -0,0 +1,29 @@ > +// 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_CLX_H > + > +#include <linux/cxl_accel_mem.h> > + > +struct efx_nic; > + > +struct efx_cxl { > + cxl_accel_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; > +}; > + > +void efx_cxl_init(struct efx_nic *efx); > +#endif > diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h > index f2dd7feb0e0c..58b7517afea4 100644 > --- a/drivers/net/ethernet/sfc/net_driver.h > +++ b/drivers/net/ethernet/sfc/net_driver.h > @@ -814,6 +814,8 @@ enum efx_xdp_tx_queues_mode { > > struct efx_mae; > > +struct efx_cxl; > + > /** > * struct efx_nic - an Efx NIC > * @name: Device name (net device name or bus id before net device registered) > @@ -962,6 +964,7 @@ struct efx_mae; > * @tc: state for TC offload (EF100). > * @devlink: reference to devlink structure owned by this device > * @dl_port: devlink port associated with the PF > + * @cxl: details of related cxl objects > * @mem_bar: The BAR that is mapped into membase. > * @reg_base: Offset from the start of the bar to the function control window. > * @monitor_work: Hardware monitor workitem > @@ -1148,6 +1151,7 @@ struct efx_nic { > > struct devlink *devlink; > struct devlink_port *dl_port; > + struct efx_cxl *cxl; > unsigned int mem_bar; > u32 reg_base; > > diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h > new file mode 100644 > index 000000000000..daf46d41f59c > --- /dev/null > +++ b/include/linux/cxl_accel_mem.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#include <linux/cdev.h> Don't think this header is needed? > + > +#ifndef __CXL_ACCEL_MEM_H > +#define __CXL_ACCEL_MEM_H > + > +enum accel_resource{ > + CXL_ACCEL_RES_DPA, > + CXL_ACCEL_RES_RAM, > + CXL_ACCEL_RES_PMEM, > +}; > + > +typedef struct cxl_dev_state cxl_accel_state; Please use 'struct cxl_dev_state' directly. There's no good reason to hide the type. > +cxl_accel_state *cxl_accel_state_create(struct device *dev); > + > +void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec); > +void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial); > +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, > + enum accel_resource); > +#endif > diff --git a/include/linux/cxl_accel_pci.h b/include/linux/cxl_accel_pci.h > new file mode 100644 > index 000000000000..c337ae8797e6 > --- /dev/null > +++ b/include/linux/cxl_accel_pci.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#ifndef __CXL_ACCEL_PCI_H > +#define __CXL_ACCEL_PCI_H > + > +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ > +#define CXL_DVSEC_PCIE_DEVICE 0 > +#define CXL_DVSEC_CAP_OFFSET 0xA > +#define CXL_DVSEC_MEM_CAPABLE BIT(2) > +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) > +#define CXL_DVSEC_CTRL_OFFSET 0xC > +#define CXL_DVSEC_MEM_ENABLE BIT(2) > +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) > +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) > +#define CXL_DVSEC_MEM_INFO_VALID BIT(0) > +#define CXL_DVSEC_MEM_ACTIVE BIT(1) > +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28) > +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10)) > +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) > +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) This looks like a copy/paste of drivers/cxl/cxlpci.h definition. I suggest create a include/linux/cxl/pci.h and stick it in there and delete the copy in cxlpci.h. Also update the CXL spec version to latest (3.1) if you don't mind if we are going to move it. > + > +#endif
On 7/19/24 00:12, Dave Jiang wrote: > > On 7/15/24 10:28 AM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Differientiate Type3, aka memory expanders, from Type2, aka device >> accelerators, with a new function for initializing cxl_dev_state. >> >> Create opaque struct to be used by accelerators relying on new access >> functions in following patches. >> >> Add SFC ethernet network driver as the client. >> >> Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/cxl/core/memdev.c | 52 ++++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/Makefile | 2 +- >> drivers/net/ethernet/sfc/efx.c | 4 ++ >> drivers/net/ethernet/sfc/efx_cxl.c | 53 +++++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/efx_cxl.h | 29 +++++++++++++++ >> drivers/net/ethernet/sfc/net_driver.h | 4 ++ >> include/linux/cxl_accel_mem.h | 22 +++++++++++ >> include/linux/cxl_accel_pci.h | 23 ++++++++++++ > Maybe create an include/linux/cxl and then we can put headers in there. > >> 8 files changed, 188 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c >> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h >> create mode 100644 include/linux/cxl_accel_mem.h >> create mode 100644 include/linux/cxl_accel_pci.h >> >> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c >> index 0277726afd04..61b5d35b49e7 100644 >> --- a/drivers/cxl/core/memdev.c >> +++ b/drivers/cxl/core/memdev.c >> @@ -8,6 +8,7 @@ >> #include <linux/idr.h> >> #include <linux/pci.h> >> #include <cxlmem.h> >> +#include <linux/cxl_accel_mem.h> >> #include "trace.h" >> #include "core.h" >> >> @@ -615,6 +616,25 @@ static void detach_memdev(struct work_struct *work) >> >> static struct lock_class_key cxl_memdev_key; >> >> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) >> +{ >> + struct cxl_dev_state *cxlds; >> + >> + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); > Naked cxlds. Do you think you'll need an accel_dev_state to wrap around cxl_dev_state similar to cxl_memdev_state in order to store accel related information? I also wonder if 'struct cxl_dev_state' should be a public definition. Need to look at the rest of the patchset to circle back. > Not sure I understand your concern. Are you saying we need to introduce an cxl_accel_state struct? Fro my work and I guess from Dan's original patch, it seems it is not needed, although I have already raised my concerns about, maybe, current structs requiring a refactoring due to the optional capabilities for Type2. Regarding if cxl_dev_state needs to be public, this patchet version defines it as opaque for addressing the concerns about accel drivers need to be "controlled". >> + if (!cxlds) >> + return ERR_PTR(-ENOMEM); >> + >> + cxlds->dev = dev; >> + cxlds->type = CXL_DEVTYPE_DEVMEM; >> + >> + cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa"); >> + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram"); >> + cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem"); >> + >> + return cxlds; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); > I do wonder if we should have a common device state init helper function to init all the common bits: > int cxlds_init(struct *dev, enum cxl_devtype devtype) > > >> + >> static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds, >> const struct file_operations *fops) >> { >> @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode *inode, struct file *file) >> return 0; >> } >> >> + >> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >> +{ >> + cxlds->cxl_dvsec = dvsec; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); >> + >> +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) >> +{ >> + cxlds->serial= serial; > Missing space before '=' >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); >> + >> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, >> + enum accel_resource type) >> +{ >> + switch (type) { >> + case CXL_ACCEL_RES_DPA: >> + cxlds->dpa_res = res; >> + return; >> + case CXL_ACCEL_RES_RAM: >> + cxlds->ram_res = res; >> + return; >> + case CXL_ACCEL_RES_PMEM: >> + cxlds->pmem_res = res; >> + return; >> + default: >> + dev_err(cxlds->dev, "unkown resource type (%u)\n", type); >> + } >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); >> + >> static int cxl_memdev_release_file(struct inode *inode, struct file *file) >> { >> struct cxl_memdev *cxlmd = >> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile >> index 8f446b9bd5ee..e80c713c3b0c 100644 >> --- a/drivers/net/ethernet/sfc/Makefile >> +++ b/drivers/net/ethernet/sfc/Makefile >> @@ -7,7 +7,7 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ >> mcdi_functions.o mcdi_filters.o mcdi_mon.o \ >> ef100.o ef100_nic.o ef100_netdev.o \ >> ef100_ethtool.o ef100_rx.o ef100_tx.o \ >> - efx_devlink.o >> + efx_devlink.o efx_cxl.o >> sfc-$(CONFIG_SFC_MTD) += mtd.o >> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ >> mae.o tc.o tc_bindings.o tc_counters.o \ >> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c >> index e9d9de8e648a..cb3f74d30852 100644 >> --- a/drivers/net/ethernet/sfc/efx.c >> +++ b/drivers/net/ethernet/sfc/efx.c >> @@ -33,6 +33,7 @@ >> #include "selftest.h" >> #include "sriov.h" >> #include "efx_devlink.h" >> +#include "efx_cxl.h" >> >> #include "mcdi_port_common.h" >> #include "mcdi_pcol.h" >> @@ -899,6 +900,7 @@ static void efx_pci_remove(struct pci_dev *pci_dev) >> efx_pci_remove_main(efx); >> >> efx_fini_io(efx); >> + > stray blank line > >> pci_dbg(efx->pci_dev, "shutdown successful\n"); >> >> efx_fini_devlink_and_unlock(efx); >> @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev *pci_dev, >> if (rc) >> goto fail2; >> >> + efx_cxl_init(efx); > No error checks? Does the device expect to work whether CXL is setup or not? > Right. The netdev functionality will not be jeopardized because CXL initialization errors. If it is all fine, the PIO buffers will be mapped using the created CXL region, if not, PIO buffers will be used mapping at specific BAR offset. >> + >> rc = efx_pci_probe_post_io(efx); >> if (rc) { >> /* On failure, retry once immediately. >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> new file mode 100644 >> index 000000000000..4554dd7cca76 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -0,0 +1,53 @@ >> +// 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 <linux/pci.h> >> +#include <linux/cxl_accel_mem.h> >> +#include <linux/cxl_accel_pci.h> >> + >> +#include "net_driver.h" >> +#include "efx_cxl.h" >> + >> +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256) >> + >> +void efx_cxl_init(struct efx_nic *efx) >> +{ >> + struct pci_dev *pci_dev = efx->pci_dev; >> + struct efx_cxl *cxl = efx->cxl; >> + struct resource res; >> + u16 dvsec; >> + >> + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL, >> + CXL_DVSEC_PCIE_DEVICE); >> + >> + if (!dvsec) >> + return; >> + >> + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); > Seem like unnecessary kern log emission > Uhmm, yes, maybe something more linked to how PIO buffer end up being used at a later time. >> + >> + cxl->cxlds = cxl_accel_state_create(&pci_dev->dev); >> + if (IS_ERR(cxl->cxlds)) { >> + pci_info(pci_dev, "CXL accel device state failed"); > pci_err()? or maybe pci_warn() given it's ignoring error returns. Right. I will change this and other similar ones. >> + return; >> + } >> + >> + cxl_accel_set_dvsec(cxl->cxlds, dvsec); >> + cxl_accel_set_serial(cxl->cxlds, pci_dev->dev.id); >> + >> + res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE); >> + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_DPA); >> + >> + res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram"); >> + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM); >> +} >> + >> + >> +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..76c6794c20d8 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_cxl.h >> @@ -0,0 +1,29 @@ >> +// 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_CLX_H >> + >> +#include <linux/cxl_accel_mem.h> >> + >> +struct efx_nic; >> + >> +struct efx_cxl { >> + cxl_accel_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; >> +}; >> + >> +void efx_cxl_init(struct efx_nic *efx); >> +#endif >> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h >> index f2dd7feb0e0c..58b7517afea4 100644 >> --- a/drivers/net/ethernet/sfc/net_driver.h >> +++ b/drivers/net/ethernet/sfc/net_driver.h >> @@ -814,6 +814,8 @@ enum efx_xdp_tx_queues_mode { >> >> struct efx_mae; >> >> +struct efx_cxl; >> + >> /** >> * struct efx_nic - an Efx NIC >> * @name: Device name (net device name or bus id before net device registered) >> @@ -962,6 +964,7 @@ struct efx_mae; >> * @tc: state for TC offload (EF100). >> * @devlink: reference to devlink structure owned by this device >> * @dl_port: devlink port associated with the PF >> + * @cxl: details of related cxl objects >> * @mem_bar: The BAR that is mapped into membase. >> * @reg_base: Offset from the start of the bar to the function control window. >> * @monitor_work: Hardware monitor workitem >> @@ -1148,6 +1151,7 @@ struct efx_nic { >> >> struct devlink *devlink; >> struct devlink_port *dl_port; >> + struct efx_cxl *cxl; >> unsigned int mem_bar; >> u32 reg_base; >> >> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h >> new file mode 100644 >> index 000000000000..daf46d41f59c >> --- /dev/null >> +++ b/include/linux/cxl_accel_mem.h >> @@ -0,0 +1,22 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ >> + >> +#include <linux/cdev.h> > Don't think this header is needed? > >> + >> +#ifndef __CXL_ACCEL_MEM_H >> +#define __CXL_ACCEL_MEM_H >> + >> +enum accel_resource{ >> + CXL_ACCEL_RES_DPA, >> + CXL_ACCEL_RES_RAM, >> + CXL_ACCEL_RES_PMEM, >> +}; >> + >> +typedef struct cxl_dev_state cxl_accel_state; > Please use 'struct cxl_dev_state' directly. There's no good reason to hide the type. That is what I think I was told to do although not explicitly. There were concerns in the RFC about accel drivers too loose for doing things regarding CXL and somehow CXL core should keep control as much as possible. I was even thought I was being asked to implement auxbus with the CXL part of an accel as an auxiliar device which should be bound to a CXL core driver. Then Jonathan Cameron the only one explicitly giving the possibility of the opaque approach and disadvising the auxbus idea. Maybe I need an explicit action here. >> +cxl_accel_state *cxl_accel_state_create(struct device *dev); >> + >> +void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec); >> +void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial); >> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, >> + enum accel_resource); >> +#endif >> diff --git a/include/linux/cxl_accel_pci.h b/include/linux/cxl_accel_pci.h >> new file mode 100644 >> index 000000000000..c337ae8797e6 >> --- /dev/null >> +++ b/include/linux/cxl_accel_pci.h >> @@ -0,0 +1,23 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ >> + >> +#ifndef __CXL_ACCEL_PCI_H >> +#define __CXL_ACCEL_PCI_H >> + >> +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ >> +#define CXL_DVSEC_PCIE_DEVICE 0 >> +#define CXL_DVSEC_CAP_OFFSET 0xA >> +#define CXL_DVSEC_MEM_CAPABLE BIT(2) >> +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) >> +#define CXL_DVSEC_CTRL_OFFSET 0xC >> +#define CXL_DVSEC_MEM_ENABLE BIT(2) >> +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) >> +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) >> +#define CXL_DVSEC_MEM_INFO_VALID BIT(0) >> +#define CXL_DVSEC_MEM_ACTIVE BIT(1) >> +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28) >> +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10)) >> +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) >> +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) > This looks like a copy/paste of drivers/cxl/cxlpci.h definition. I suggest create a include/linux/cxl/pci.h and stick it in there and delete the copy in cxlpci.h. Also update the CXL spec version to latest (3.1) if you don't mind if we are going to move it. That makes sense. I'll do it. Thanks >> + >> +#endif
> >> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h > >> new file mode 100644 > >> index 000000000000..daf46d41f59c > >> --- /dev/null > >> +++ b/include/linux/cxl_accel_mem.h > >> @@ -0,0 +1,22 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > >> + > >> +#include <linux/cdev.h> > > Don't think this header is needed? > > > >> + > >> +#ifndef __CXL_ACCEL_MEM_H > >> +#define __CXL_ACCEL_MEM_H > >> + > >> +enum accel_resource{ > >> + CXL_ACCEL_RES_DPA, > >> + CXL_ACCEL_RES_RAM, > >> + CXL_ACCEL_RES_PMEM, > >> +}; > >> + > >> +typedef struct cxl_dev_state cxl_accel_state; > > Please use 'struct cxl_dev_state' directly. There's no good reason to hide the type. > > > That is what I think I was told to do although not explicitly. There > were concerns in the RFC about accel drivers too loose for doing things > regarding CXL and somehow CXL core should keep control as much as > possible. I was even thought I was being asked to implement auxbus with > the CXL part of an accel as an auxiliar device which should be bound to > a CXL core driver. Then Jonathan Cameron the only one explicitly giving > the possibility of the opaque approach and disadvising the auxbus idea. I wasn't thinking a typedef to hide it. More making all state accesses that are needed through accessor functions so that from the 'internals' become opaque to the accelerator code and we can radically change how things are structured internally with no impact to the (hopefully large number of) CXL accelerator drivers. So here, I'd just expect a struct cxl_device_state; forwards declaration. Or potentially one to a a different structure after refactors etc. > > > Maybe I need an explicit action here. J
On Mon, 15 Jul 2024 18:28:21 +0100 <alejandro.lucero-palau@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Differientiate Type3, aka memory expanders, from Type2, aka device > accelerators, with a new function for initializing cxl_dev_state. > > Create opaque struct to be used by accelerators relying on new access > functions in following patches. > > Add SFC ethernet network driver as the client. > > Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Co-developed-by: Dan Williams <dan.j.williams@intel.com> Hi Alejandro, Various comments inline. Mostly minor detail as I need to get my head around the whole thing which will take a while yet! Some will seem very fussy given the stage we are at (and fairly long way to go), but cleaner code will generally be easier to read so may help move the bigger stuff forwards quicker. + I had my review brain in gear so couldn't ignore things. Jonathan > --- > drivers/cxl/core/memdev.c | 52 ++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/Makefile | 2 +- > drivers/net/ethernet/sfc/efx.c | 4 ++ > drivers/net/ethernet/sfc/efx_cxl.c | 53 +++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/efx_cxl.h | 29 +++++++++++++++ > drivers/net/ethernet/sfc/net_driver.h | 4 ++ > include/linux/cxl_accel_mem.h | 22 +++++++++++ > include/linux/cxl_accel_pci.h | 23 ++++++++++++ > 8 files changed, 188 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c > create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h > create mode 100644 include/linux/cxl_accel_mem.h > create mode 100644 include/linux/cxl_accel_pci.h > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index 0277726afd04..61b5d35b49e7 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode *inode, struct file *file) > return 0; > } > > + > +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > +{ > + cxlds->cxl_dvsec = dvsec; Nothing to do with accel. If these make sense promote to cxl core and a linux/cxl/ header. Also we may want the type3 driver to switch to them long term. If nothing else, making that handle the cxl_dev_state as more opaque will show up what is still directly accessed and may need to be wrapped up for a future accelerator driver to use. > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); > + > +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) > +{ > + cxlds->serial= serial; Run checkpatch over this series before v3 with --strict and fix the warnings. Probably would have spotted missing space before = Sure it's a series that is kind of RFC ish at the moment but clean code means you don't get nitpickers like me pointing this stuff out! > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); > + > +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, > + enum accel_resource type) > +{ > + switch (type) { > + case CXL_ACCEL_RES_DPA: > + cxlds->dpa_res = res; > + return; > + case CXL_ACCEL_RES_RAM: > + cxlds->ram_res = res; > + return; > + case CXL_ACCEL_RES_PMEM: > + cxlds->pmem_res = res; > + return; > + default: > + dev_err(cxlds->dev, "unkown resource type (%u)\n", type); typo. Plus I'd let this return an error as we may well have more types in future and not handle them all. > + } > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); > + > static int cxl_memdev_release_file(struct inode *inode, struct file *file) > { > struct cxl_memdev *cxlmd = > diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c > index e9d9de8e648a..cb3f74d30852 100644 > --- a/drivers/net/ethernet/sfc/efx.c > +++ b/drivers/net/ethernet/sfc/efx.c > @@ -33,6 +33,7 @@ > #include "selftest.h" > #include "sriov.h" > #include "efx_devlink.h" > +#include "efx_cxl.h" > > #include "mcdi_port_common.h" > #include "mcdi_pcol.h" > @@ -899,6 +900,7 @@ static void efx_pci_remove(struct pci_dev *pci_dev) > efx_pci_remove_main(efx); > > efx_fini_io(efx); > + Make sure you don't add noisy whitespace changes in v3. Slows down review and makes a patch set look bigger than it is. > pci_dbg(efx->pci_dev, "shutdown successful\n"); > > efx_fini_devlink_and_unlock(efx); > @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev *pci_dev, > if (rc) > goto fail2; > > + efx_cxl_init(efx); > + As below, have an error code. This is not something we want to fail and have the driver carry on. > rc = efx_pci_probe_post_io(efx); > if (rc) { > /* On failure, retry once immediately. > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > new file mode 100644 > index 000000000000..4554dd7cca76 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -0,0 +1,53 @@ > +// 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 <linux/pci.h> > +#include <linux/cxl_accel_mem.h> > +#include <linux/cxl_accel_pci.h> > + > +#include "net_driver.h" > +#include "efx_cxl.h" > + > +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256) > + > +void efx_cxl_init(struct efx_nic *efx) > +{ > + struct pci_dev *pci_dev = efx->pci_dev; > + struct efx_cxl *cxl = efx->cxl; > + struct resource res; > + u16 dvsec; > + > + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL, > + CXL_DVSEC_PCIE_DEVICE); > + > + if (!dvsec) > + return; > + > + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); pci_dbg(); > + > + cxl->cxlds = cxl_accel_state_create(&pci_dev->dev); > + if (IS_ERR(cxl->cxlds)) { > + pci_info(pci_dev, "CXL accel device state failed"); > + return; Return an error. A driver calling CXL stuff that fails is going to want to know > + } > + > + cxl_accel_set_dvsec(cxl->cxlds, dvsec); > + cxl_accel_set_serial(cxl->cxlds, pci_dev->dev.id); > + > + res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE); > + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_DPA); > + > + res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram"); > + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM); > +} > + > + > +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..76c6794c20d8 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_cxl.h > @@ -0,0 +1,29 @@ > +// 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_CLX_H > + > +#include <linux/cxl_accel_mem.h> Maybe, or maybe just some more forward defines to keep all the header nice an separate. > + > +struct efx_nic; > + > +struct efx_cxl { > + cxl_accel_state *cxlds; There are other ways to keep this opaque that let you embed the structure into one you do know about. Usually involve allocating a cxl_device_state + your structure and some cxl_devstate_private() accessors to get to the data placed after the cxlds part. May not be worth bothering here though, particularly as the CXL-ness of the device may not be the most important part and you may well be doing similar tricks anyway to hid some other subsystem specific driver. So for now this looks like a sensible approach to me. > + 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; > +}; > + > +void efx_cxl_init(struct efx_nic *efx); > +#endif > diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h > new file mode 100644 > index 000000000000..daf46d41f59c > --- /dev/null > +++ b/include/linux/cxl_accel_mem.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#include <linux/cdev.h> > + > +#ifndef __CXL_ACCEL_MEM_H > +#define __CXL_ACCEL_MEM_H > + > +enum accel_resource{ > + CXL_ACCEL_RES_DPA, > + CXL_ACCEL_RES_RAM, > + CXL_ACCEL_RES_PMEM, > +}; > + > +typedef struct cxl_dev_state cxl_accel_state; A forwards def would work like you do for struct efx_cxl above. Keeps the structure opaque unless code actually needs to know what is in it. That code can including the header that defines it. In many cases it will be an opaque pointer passed to code in the CXL core. struct cxl_dev_state; Then use pointers to that in these functions. > +cxl_accel_state *cxl_accel_state_create(struct device *dev); > + > +void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec); > +void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial); > +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, > + enum accel_resource); > +#endif > diff --git a/include/linux/cxl_accel_pci.h b/include/linux/cxl_accel_pci.h > new file mode 100644 > index 000000000000..c337ae8797e6 > --- /dev/null > +++ b/include/linux/cxl_accel_pci.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#ifndef __CXL_ACCEL_PCI_H > +#define __CXL_ACCEL_PCI_H > + > +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ > +#define CXL_DVSEC_PCIE_DEVICE 0 > +#define CXL_DVSEC_CAP_OFFSET 0xA > +#define CXL_DVSEC_MEM_CAPABLE BIT(2) > +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) > +#define CXL_DVSEC_CTRL_OFFSET 0xC > +#define CXL_DVSEC_MEM_ENABLE BIT(2) > +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) > +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) > +#define CXL_DVSEC_MEM_INFO_VALID BIT(0) > +#define CXL_DVSEC_MEM_ACTIVE BIT(1) > +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28) > +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10)) > +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) > +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) As I think Dave suggested, pull any defs you need to linux/cxl/pci.h or whatever makes sense and make the exiting code look for them there. Ideally do that in a patch that does nothing else as simple moves are easier to review quickly than ones mixed with real changes. > + > +#endif
On 8/4/24 17:44, Jonathan Cameron wrote: >>>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h >>>> new file mode 100644 >>>> index 000000000000..daf46d41f59c >>>> --- /dev/null >>>> +++ b/include/linux/cxl_accel_mem.h >>>> @@ -0,0 +1,22 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ >>>> + >>>> +#include <linux/cdev.h> >>> Don't think this header is needed? >>> >>>> + >>>> +#ifndef __CXL_ACCEL_MEM_H >>>> +#define __CXL_ACCEL_MEM_H >>>> + >>>> +enum accel_resource{ >>>> + CXL_ACCEL_RES_DPA, >>>> + CXL_ACCEL_RES_RAM, >>>> + CXL_ACCEL_RES_PMEM, >>>> +}; >>>> + >>>> +typedef struct cxl_dev_state cxl_accel_state; >>> Please use 'struct cxl_dev_state' directly. There's no good reason to hide the type. >> >> That is what I think I was told to do although not explicitly. There >> were concerns in the RFC about accel drivers too loose for doing things >> regarding CXL and somehow CXL core should keep control as much as >> possible. I was even thought I was being asked to implement auxbus with >> the CXL part of an accel as an auxiliar device which should be bound to >> a CXL core driver. Then Jonathan Cameron the only one explicitly giving >> the possibility of the opaque approach and disadvising the auxbus idea. > I wasn't thinking a typedef to hide it. > More making all state accesses that are needed through accessor functions so > that from the 'internals' become opaque to the accelerator code and > we can radically change how things are structured internally with > no impact to the (hopefully large number of) CXL accelerator drivers. > > So here, I'd just expect a > struct cxl_device_state; forwards declaration. > > Or potentially one to a a different structure after refactors etc. OK. It makes sense. I thought the concern was about external driver modules using the internal cxl structs. This is the main point in this second patchset version, so if none else says the opposite during the next days, I will take it as the right move forward and send a new version 3 soon. Thank you >> >> Maybe I need an explicit action here. > J
On Mon, 15 Jul 2024 18:28:21 +0100 <alejandro.lucero-palau@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Differientiate Type3, aka memory expanders, from Type2, aka device > accelerators, with a new function for initializing cxl_dev_state. > > Create opaque struct to be used by accelerators relying on new access > functions in following patches. > > Add SFC ethernet network driver as the client. > > Based on > https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Co-developed-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/memdev.c | 52 ++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/Makefile | 2 +- > drivers/net/ethernet/sfc/efx.c | 4 ++ > drivers/net/ethernet/sfc/efx_cxl.c | 53 > +++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.h | > 29 +++++++++++++++ drivers/net/ethernet/sfc/net_driver.h | 4 ++ > include/linux/cxl_accel_mem.h | 22 +++++++++++ > include/linux/cxl_accel_pci.h | 23 ++++++++++++ > 8 files changed, 188 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c > create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h > create mode 100644 include/linux/cxl_accel_mem.h > create mode 100644 include/linux/cxl_accel_pci.h > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index 0277726afd04..61b5d35b49e7 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -8,6 +8,7 @@ > #include <linux/idr.h> > #include <linux/pci.h> > #include <cxlmem.h> > +#include <linux/cxl_accel_mem.h> Let's keep the header inclusion in an alphabetical order. The same in efx_cxl.c > #include "trace.h" > #include "core.h" > > @@ -615,6 +616,25 @@ static void detach_memdev(struct work_struct > *work) > static struct lock_class_key cxl_memdev_key; > > +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) > +{ > + struct cxl_dev_state *cxlds; > + > + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); > + if (!cxlds) > + return ERR_PTR(-ENOMEM); > + > + cxlds->dev = dev; > + cxlds->type = CXL_DEVTYPE_DEVMEM; > + > + cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa"); > + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram"); > + cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem"); > + > + return cxlds; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); > + > static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state > *cxlds, const struct file_operations *fops) > { > @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode *inode, > struct file *file) return 0; > } > > + > +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > +{ > + cxlds->cxl_dvsec = dvsec; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); > + > +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) > +{ > + cxlds->serial= serial; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); > + It would be nice to explain about how the cxl core is using these in the patch comments, as we just saw the stuff got promoted into the core. > +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct > resource res, > + enum accel_resource type) > +{ > + switch (type) { > + case CXL_ACCEL_RES_DPA: > + cxlds->dpa_res = res; > + return; > + case CXL_ACCEL_RES_RAM: > + cxlds->ram_res = res; > + return; > + case CXL_ACCEL_RES_PMEM: > + cxlds->pmem_res = res; > + return; > + default: > + dev_err(cxlds->dev, "unkown resource type (%u)\n", > type); > + } > +} > +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); > + I wonder in which situation this error can be triggered. One can be a newer out-of-tree type-2 driver tries to work on an older kernel. Other situations should be the coding problem of an in-tree driver. I prefer to WARN_ONCE() here. > static int cxl_memdev_release_file(struct inode *inode, struct file > *file) { > struct cxl_memdev *cxlmd = > diff --git a/drivers/net/ethernet/sfc/Makefile > b/drivers/net/ethernet/sfc/Makefile index 8f446b9bd5ee..e80c713c3b0c > 100644 --- a/drivers/net/ethernet/sfc/Makefile > +++ b/drivers/net/ethernet/sfc/Makefile > @@ -7,7 +7,7 @@ sfc-y += efx.o efx_common.o > efx_channels.o nic.o \ mcdi_functions.o mcdi_filters.o mcdi_mon.o \ > ef100.o ef100_nic.o ef100_netdev.o \ > ef100_ethtool.o ef100_rx.o ef100_tx.o \ > - efx_devlink.o > + efx_devlink.o efx_cxl.o > sfc-$(CONFIG_SFC_MTD) += mtd.o > sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o > ef100_rep.o \ mae.o tc.o tc_bindings.o tc_counters.o \ > diff --git a/drivers/net/ethernet/sfc/efx.c > b/drivers/net/ethernet/sfc/efx.c index e9d9de8e648a..cb3f74d30852 > 100644 --- a/drivers/net/ethernet/sfc/efx.c > +++ b/drivers/net/ethernet/sfc/efx.c > @@ -33,6 +33,7 @@ > #include "selftest.h" > #include "sriov.h" > #include "efx_devlink.h" > +#include "efx_cxl.h" > > #include "mcdi_port_common.h" > #include "mcdi_pcol.h" > @@ -899,6 +900,7 @@ static void efx_pci_remove(struct pci_dev > *pci_dev) efx_pci_remove_main(efx); > > efx_fini_io(efx); > + > pci_dbg(efx->pci_dev, "shutdown successful\n"); > > efx_fini_devlink_and_unlock(efx); > @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev > *pci_dev, if (rc) > goto fail2; > > + efx_cxl_init(efx); > + > rc = efx_pci_probe_post_io(efx); > if (rc) { > /* On failure, retry once immediately. > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c > b/drivers/net/ethernet/sfc/efx_cxl.c new file mode 100644 > index 000000000000..4554dd7cca76 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -0,0 +1,53 @@ > +// 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 <linux/pci.h> > +#include <linux/cxl_accel_mem.h> > +#include <linux/cxl_accel_pci.h> > + Let's keep them in alphabetical order. :) > +#include "net_driver.h" > +#include "efx_cxl.h" > + > +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256) > + > +void efx_cxl_init(struct efx_nic *efx) > +{ > + struct pci_dev *pci_dev = efx->pci_dev; > + struct efx_cxl *cxl = efx->cxl; > + struct resource res; > + u16 dvsec; > + > + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL, > + CXL_DVSEC_PCIE_DEVICE); > + > + if (!dvsec) > + return; > + > + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability > found"); + > + cxl->cxlds = cxl_accel_state_create(&pci_dev->dev); > + if (IS_ERR(cxl->cxlds)) { > + pci_info(pci_dev, "CXL accel device state failed"); > + return; > + } > + > + cxl_accel_set_dvsec(cxl->cxlds, dvsec); > + cxl_accel_set_serial(cxl->cxlds, pci_dev->dev.id); > + > + res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE); > + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_DPA); > + > + res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram"); > + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM); > +} > + > + > +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..76c6794c20d8 > --- /dev/null > +++ b/drivers/net/ethernet/sfc/efx_cxl.h > @@ -0,0 +1,29 @@ > +// 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_CLX_H > + > +#include <linux/cxl_accel_mem.h> > + > +struct efx_nic; > + > +struct efx_cxl { > + cxl_accel_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; > +}; > + > +void efx_cxl_init(struct efx_nic *efx); > +#endif > diff --git a/drivers/net/ethernet/sfc/net_driver.h > b/drivers/net/ethernet/sfc/net_driver.h index > f2dd7feb0e0c..58b7517afea4 100644 --- > a/drivers/net/ethernet/sfc/net_driver.h +++ > b/drivers/net/ethernet/sfc/net_driver.h @@ -814,6 +814,8 @@ enum > efx_xdp_tx_queues_mode { > struct efx_mae; > > +struct efx_cxl; > + > /** > * struct efx_nic - an Efx NIC > * @name: Device name (net device name or bus id before net device > registered) @@ -962,6 +964,7 @@ struct efx_mae; > * @tc: state for TC offload (EF100). > * @devlink: reference to devlink structure owned by this device > * @dl_port: devlink port associated with the PF > + * @cxl: details of related cxl objects > * @mem_bar: The BAR that is mapped into membase. > * @reg_base: Offset from the start of the bar to the function > control window. > * @monitor_work: Hardware monitor workitem > @@ -1148,6 +1151,7 @@ struct efx_nic { > > struct devlink *devlink; > struct devlink_port *dl_port; > + struct efx_cxl *cxl; > unsigned int mem_bar; > u32 reg_base; > > diff --git a/include/linux/cxl_accel_mem.h > b/include/linux/cxl_accel_mem.h new file mode 100644 > index 000000000000..daf46d41f59c > --- /dev/null > +++ b/include/linux/cxl_accel_mem.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#include <linux/cdev.h> > + > +#ifndef __CXL_ACCEL_MEM_H > +#define __CXL_ACCEL_MEM_H > + > +enum accel_resource{ > + CXL_ACCEL_RES_DPA, > + CXL_ACCEL_RES_RAM, > + CXL_ACCEL_RES_PMEM, > +}; > + > +typedef struct cxl_dev_state cxl_accel_state; The case of using typedef in kernel coding is very rare (quite many of them are still there due to history reason, you can also spot that there is only one typedef in driver/cxl). Be sure to double check the coding style bible [1] when deciding to use one. :) [1] https://www.kernel.org/doc/html/v4.14/process/coding-style.html > +cxl_accel_state *cxl_accel_state_create(struct device *dev); > + > +void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec); > +void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial); > +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct > resource res, > + enum accel_resource); > +#endif > diff --git a/include/linux/cxl_accel_pci.h > b/include/linux/cxl_accel_pci.h new file mode 100644 > index 000000000000..c337ae8797e6 > --- /dev/null > +++ b/include/linux/cxl_accel_pci.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > + > +#ifndef __CXL_ACCEL_PCI_H > +#define __CXL_ACCEL_PCI_H > + > +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ > +#define CXL_DVSEC_PCIE_DEVICE > 0 +#define CXL_DVSEC_CAP_OFFSET 0xA > +#define CXL_DVSEC_MEM_CAPABLE BIT(2) > +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) > +#define CXL_DVSEC_CTRL_OFFSET 0xC > +#define CXL_DVSEC_MEM_ENABLE BIT(2) > +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) > +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) > +#define CXL_DVSEC_MEM_INFO_VALID BIT(0) > +#define CXL_DVSEC_MEM_ACTIVE BIT(1) > +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28) > +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10)) > +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) > +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) > + > +#endif
On 8/4/24 18:10, Jonathan Cameron wrote: > On Mon, 15 Jul 2024 18:28:21 +0100 > <alejandro.lucero-palau@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Differientiate Type3, aka memory expanders, from Type2, aka device >> accelerators, with a new function for initializing cxl_dev_state. >> >> Create opaque struct to be used by accelerators relying on new access >> functions in following patches. >> >> Add SFC ethernet network driver as the client. >> >> Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Co-developed-by: Dan Williams <dan.j.williams@intel.com> > >> + >> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >> +{ >> + cxlds->cxl_dvsec = dvsec; > Nothing to do with accel. If these make sense promote to cxl > core and a linux/cxl/ header. Also we may want the type3 driver to > switch to them long term. If nothing else, making that handle the > cxl_dev_state as more opaque will show up what is still directly > accessed and may need to be wrapped up for a future accelerator driver > to use. > I will change the function name then, but not sure I follow the comment about more opaque ... >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); >> + >> +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) >> +{ >> + cxlds->serial= serial; > Run checkpatch over this series before v3 with --strict and fix the > warnings. Probably would have spotted missing space before = > > Sure it's a series that is kind of RFC ish at the moment but clean > code means you don't get nitpickers like me pointing this stuff out! > Sure. Thanks. >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); >> + >> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, >> + enum accel_resource type) >> +{ >> + switch (type) { >> + case CXL_ACCEL_RES_DPA: >> + cxlds->dpa_res = res; >> + return; >> + case CXL_ACCEL_RES_RAM: >> + cxlds->ram_res = res; >> + return; >> + case CXL_ACCEL_RES_PMEM: >> + cxlds->pmem_res = res; >> + return; >> + default: >> + dev_err(cxlds->dev, "unkown resource type (%u)\n", type); > typo. Plus I'd let this return an error as we may well have more types > in future and not handle them all. > OK. >> pci_dbg(efx->pci_dev, "shutdown successful\n"); >> >> efx_fini_devlink_and_unlock(efx); >> @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev *pci_dev, >> if (rc) >> goto fail2; >> >> + efx_cxl_init(efx); >> + > As below, have an error code. This is not something we want to fail > and have the driver carry on. As you have seen in another patch when CXL initialization is taken into account, the driver can keep going if this fails. Those pci_warn/err inside CXL core should be enough. >> rc = efx_pci_probe_post_io(efx); >> if (rc) { >> /* On failure, retry once immediately. >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> new file mode 100644 >> index 000000000000..4554dd7cca76 >> --- /dev/null >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -0,0 +1,53 @@ >> +// 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 <linux/pci.h> >> +#include <linux/cxl_accel_mem.h> >> +#include <linux/cxl_accel_pci.h> >> + >> +#include "net_driver.h" >> +#include "efx_cxl.h" >> + >> +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256) >> + >> +void efx_cxl_init(struct efx_nic *efx) >> +{ >> + struct pci_dev *pci_dev = efx->pci_dev; >> + struct efx_cxl *cxl = efx->cxl; >> + struct resource res; >> + u16 dvsec; >> + >> + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL, >> + CXL_DVSEC_PCIE_DEVICE); >> + >> + if (!dvsec) >> + return; >> + >> + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); > pci_dbg(); Right. > >> diff --git a/include/linux/cxl_accel_pci.h b/include/linux/cxl_accel_pci.h >> new file mode 100644 >> index 000000000000..c337ae8797e6 >> --- /dev/null >> +++ b/include/linux/cxl_accel_pci.h >> @@ -0,0 +1,23 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ >> + >> +#ifndef __CXL_ACCEL_PCI_H >> +#define __CXL_ACCEL_PCI_H >> + >> +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ >> +#define CXL_DVSEC_PCIE_DEVICE 0 >> +#define CXL_DVSEC_CAP_OFFSET 0xA >> +#define CXL_DVSEC_MEM_CAPABLE BIT(2) >> +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) >> +#define CXL_DVSEC_CTRL_OFFSET 0xC >> +#define CXL_DVSEC_MEM_ENABLE BIT(2) >> +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) >> +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) >> +#define CXL_DVSEC_MEM_INFO_VALID BIT(0) >> +#define CXL_DVSEC_MEM_ACTIVE BIT(1) >> +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28) >> +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10)) >> +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) >> +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) > As I think Dave suggested, pull any defs you need to linux/cxl/pci.h or whatever > makes sense and make the exiting code look for them there. > > Ideally do that in a patch that does nothing else as simple > moves are easier to review quickly than ones mixed with real changes. I'll do. Thanks > > >> + >> +#endif
On 8/9/24 09:34, Zhi Wang wrote: > On Mon, 15 Jul 2024 18:28:21 +0100 > <alejandro.lucero-palau@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Differientiate Type3, aka memory expanders, from Type2, aka device >> accelerators, with a new function for initializing cxl_dev_state. >> >> Create opaque struct to be used by accelerators relying on new access >> functions in following patches. >> >> Add SFC ethernet network driver as the client. >> >> Based on >> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/cxl/core/memdev.c | 52 ++++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/Makefile | 2 +- >> drivers/net/ethernet/sfc/efx.c | 4 ++ >> drivers/net/ethernet/sfc/efx_cxl.c | 53 >> +++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.h | >> 29 +++++++++++++++ drivers/net/ethernet/sfc/net_driver.h | 4 ++ >> include/linux/cxl_accel_mem.h | 22 +++++++++++ >> include/linux/cxl_accel_pci.h | 23 ++++++++++++ >> 8 files changed, 188 insertions(+), 1 deletion(-) >> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c >> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h >> create mode 100644 include/linux/cxl_accel_mem.h >> create mode 100644 include/linux/cxl_accel_pci.h >> >> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c >> index 0277726afd04..61b5d35b49e7 100644 >> --- a/drivers/cxl/core/memdev.c >> +++ b/drivers/cxl/core/memdev.c >> @@ -8,6 +8,7 @@ >> #include <linux/idr.h> >> #include <linux/pci.h> >> #include <cxlmem.h> >> +#include <linux/cxl_accel_mem.h> > Let's keep the header inclusion in an alphabetical order. The same in > efx_cxl.c The headers seem to follow a reverse Christmas tree order here rather than an alphabetical one. Should I rearrange them all? >> #include "trace.h" >> #include "core.h" >> >> @@ -615,6 +616,25 @@ static void detach_memdev(struct work_struct >> *work) >> static struct lock_class_key cxl_memdev_key; >> >> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) >> +{ >> + struct cxl_dev_state *cxlds; >> + >> + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); >> + if (!cxlds) >> + return ERR_PTR(-ENOMEM); >> + >> + cxlds->dev = dev; >> + cxlds->type = CXL_DEVTYPE_DEVMEM; >> + >> + cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa"); >> + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram"); >> + cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem"); >> + >> + return cxlds; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); >> + >> static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state >> *cxlds, const struct file_operations *fops) >> { >> @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode *inode, >> struct file *file) return 0; >> } >> >> + >> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >> +{ >> + cxlds->cxl_dvsec = dvsec; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); >> + >> +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) >> +{ >> + cxlds->serial= serial; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); >> + > It would be nice to explain about how the cxl core is using these in > the patch comments, as we just saw the stuff got promoted into the core. As far as I can see, it is for info/debugging purposes. I will add such explanation in next version. > >> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct >> resource res, >> + enum accel_resource type) >> +{ >> + switch (type) { >> + case CXL_ACCEL_RES_DPA: >> + cxlds->dpa_res = res; >> + return; >> + case CXL_ACCEL_RES_RAM: >> + cxlds->ram_res = res; >> + return; >> + case CXL_ACCEL_RES_PMEM: >> + cxlds->pmem_res = res; >> + return; >> + default: >> + dev_err(cxlds->dev, "unkown resource type (%u)\n", >> type); >> + } >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); >> + > I wonder in which situation this error can be triggered. > One can be a newer out-of-tree type-2 driver tries to work on an older > kernel. Other situations should be the coding problem of an in-tree > driver. I guess that would point to an extension not updating this function. > I prefer to WARN_ONCE() here. I agree after your previous concern. > >> >> diff --git a/include/linux/cxl_accel_mem.h >> b/include/linux/cxl_accel_mem.h new file mode 100644 >> index 000000000000..daf46d41f59c >> --- /dev/null >> +++ b/include/linux/cxl_accel_mem.h >> @@ -0,0 +1,22 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ >> + >> +#include <linux/cdev.h> >> + >> +#ifndef __CXL_ACCEL_MEM_H >> +#define __CXL_ACCEL_MEM_H >> + >> +enum accel_resource{ >> + CXL_ACCEL_RES_DPA, >> + CXL_ACCEL_RES_RAM, >> + CXL_ACCEL_RES_PMEM, >> +}; >> + >> +typedef struct cxl_dev_state cxl_accel_state; > The case of using typedef in kernel coding is very rare (quite many > of them are still there due to history reason, you can also spot that > there is only one typedef in driver/cxl). Be sure to double check the > coding style bible [1] when deciding to use one. :) > > [1] https://www.kernel.org/doc/html/v4.14/process/coding-style.html Right. I think there is an agreement now in not using typedef but struct cxl_dev_state so problem solved. Thanks!
On 8/12/24 12:16, Alejandro Lucero Palau wrote: > > On 8/4/24 18:10, Jonathan Cameron wrote: >> On Mon, 15 Jul 2024 18:28:21 +0100 >> <alejandro.lucero-palau@amd.com> wrote: >> >>> From: Alejandro Lucero <alucerop@amd.com> >>> >>> Differientiate Type3, aka memory expanders, from Type2, aka device >>> accelerators, with a new function for initializing cxl_dev_state. >>> >>> Create opaque struct to be used by accelerators relying on new access >>> functions in following patches. >>> >>> Add SFC ethernet network driver as the client. >>> >>> Based on >>> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >>> >>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >> > >>> + >>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >>> +{ >>> + cxlds->cxl_dvsec = dvsec; >> Nothing to do with accel. If these make sense promote to cxl >> core and a linux/cxl/ header. Also we may want the type3 driver to >> switch to them long term. If nothing else, making that handle the >> cxl_dev_state as more opaque will show up what is still directly >> accessed and may need to be wrapped up for a future accelerator driver >> to use. >> > > I will change the function name then, but not sure I follow the > comment about more opaque ... > > > I have second thoughts about this. I consider this as an accessor for, as you said in a previous exchange, facilitating changes to the core structs without touching those accel drivers using it. Type3 driver is part of the CXL core and easy to change for these kind of updates since it will only be one driver supporting all Type3, and an accessor is not required then. Let me know what you think.
On Mon, 12 Aug 2024 12:16:02 +0100 Alejandro Lucero Palau <alucerop@amd.com> wrote: > On 8/4/24 18:10, Jonathan Cameron wrote: > > On Mon, 15 Jul 2024 18:28:21 +0100 > > <alejandro.lucero-palau@amd.com> wrote: > > > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> Differientiate Type3, aka memory expanders, from Type2, aka device > >> accelerators, with a new function for initializing cxl_dev_state. > >> > >> Create opaque struct to be used by accelerators relying on new access > >> functions in following patches. > >> > >> Add SFC ethernet network driver as the client. > >> > >> Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > >> > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >> Co-developed-by: Dan Williams <dan.j.williams@intel.com> > > > > >> + > >> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > >> +{ > >> + cxlds->cxl_dvsec = dvsec; > > Nothing to do with accel. If these make sense promote to cxl > > core and a linux/cxl/ header. Also we may want the type3 driver to > > switch to them long term. If nothing else, making that handle the > > cxl_dev_state as more opaque will show up what is still directly > > accessed and may need to be wrapped up for a future accelerator driver > > to use. > > > > I will change the function name then, but not sure I follow the comment > about more opaque ... If most code can't see the internals of cxl_dev_state because it doesn't include the header that defines it, then we will generally spot data that may not belong in that state structure in the first place or where it is appropriate to have an accessor function mediating that access. Jonathan
On Tue, 13 Aug 2024 09:30:08 +0100 Alejandro Lucero Palau <alucerop@amd.com> wrote: > On 8/12/24 12:16, Alejandro Lucero Palau wrote: > > > > On 8/4/24 18:10, Jonathan Cameron wrote: > >> On Mon, 15 Jul 2024 18:28:21 +0100 > >> <alejandro.lucero-palau@amd.com> wrote: > >> > >>> From: Alejandro Lucero <alucerop@amd.com> > >>> > >>> Differientiate Type3, aka memory expanders, from Type2, aka device > >>> accelerators, with a new function for initializing cxl_dev_state. > >>> > >>> Create opaque struct to be used by accelerators relying on new access > >>> functions in following patches. > >>> > >>> Add SFC ethernet network driver as the client. > >>> > >>> Based on > >>> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > >>> > >>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> > >> > > > >>> + > >>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > >>> +{ > >>> + cxlds->cxl_dvsec = dvsec; > >> Nothing to do with accel. If these make sense promote to cxl > >> core and a linux/cxl/ header. Also we may want the type3 driver to > >> switch to them long term. If nothing else, making that handle the > >> cxl_dev_state as more opaque will show up what is still directly > >> accessed and may need to be wrapped up for a future accelerator driver > >> to use. > >> > > > > I will change the function name then, but not sure I follow the > > comment about more opaque ... > > > > > > > > I have second thoughts about this. > > > I consider this as an accessor for, as you said in a previous exchange, > facilitating changes to the core structs without touching those accel > drivers using it. > > Type3 driver is part of the CXL core and easy to change for these kind > of updates since it will only be one driver supporting all Type3, and an > accessor is not required then. > > Let me know what you think. It's less critical, but longer term I'd expect any stuff that makes sense for accelerators and the type 3 driver to use the same approaches and code paths. Makes it easier to see where they are related than opencoding the accesses in the type 3 driver will do. In the very long term, I'd expect the type 3 driver to just be another CXL driver alongside many others. Jonathan > >
On Mon, 12 Aug 2024 12:34:55 +0100 Alejandro Lucero Palau <alucerop@amd.com> wrote: > > On 8/9/24 09:34, Zhi Wang wrote: > > On Mon, 15 Jul 2024 18:28:21 +0100 > > <alejandro.lucero-palau@amd.com> wrote: > > > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> Differientiate Type3, aka memory expanders, from Type2, aka device > >> accelerators, with a new function for initializing cxl_dev_state. > >> > >> Create opaque struct to be used by accelerators relying on new > >> access functions in following patches. > >> > >> Add SFC ethernet network driver as the client. > >> > >> Based on > >> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > >> > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >> Co-developed-by: Dan Williams <dan.j.williams@intel.com> > >> --- > >> drivers/cxl/core/memdev.c | 52 > >> ++++++++++++++++++++++++++ drivers/net/ethernet/sfc/Makefile | > >> 2 +- drivers/net/ethernet/sfc/efx.c | 4 ++ > >> drivers/net/ethernet/sfc/efx_cxl.c | 53 > >> +++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.h | > >> 29 +++++++++++++++ drivers/net/ethernet/sfc/net_driver.h | 4 ++ > >> include/linux/cxl_accel_mem.h | 22 +++++++++++ > >> include/linux/cxl_accel_pci.h | 23 ++++++++++++ > >> 8 files changed, 188 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c > >> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h > >> create mode 100644 include/linux/cxl_accel_mem.h > >> create mode 100644 include/linux/cxl_accel_pci.h > >> > >> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > >> index 0277726afd04..61b5d35b49e7 100644 > >> --- a/drivers/cxl/core/memdev.c > >> +++ b/drivers/cxl/core/memdev.c > >> @@ -8,6 +8,7 @@ > >> #include <linux/idr.h> > >> #include <linux/pci.h> > >> #include <cxlmem.h> > >> +#include <linux/cxl_accel_mem.h> > > Let's keep the header inclusion in an alphabetical order. The same > > in efx_cxl.c > > > The headers seem to follow a reverse Christmas tree order here rather > than an alphabetical one. > > Should I rearrange them all? > Let's fix them. > > >> #include "trace.h" > >> #include "core.h" > >> > >> @@ -615,6 +616,25 @@ static void detach_memdev(struct work_struct > >> *work) > >> static struct lock_class_key cxl_memdev_key; > >> > >> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) > >> +{ > >> + struct cxl_dev_state *cxlds; > >> + > >> + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); > >> + if (!cxlds) > >> + return ERR_PTR(-ENOMEM); > >> + > >> + cxlds->dev = dev; > >> + cxlds->type = CXL_DEVTYPE_DEVMEM; > >> + > >> + cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa"); > >> + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram"); > >> + cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem"); > >> + > >> + return cxlds; > >> +} > >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); > >> + > >> static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state > >> *cxlds, const struct file_operations *fops) > >> { > >> @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode > >> *inode, struct file *file) return 0; > >> } > >> > >> + > >> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > >> +{ > >> + cxlds->cxl_dvsec = dvsec; > >> +} > >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); > >> + > >> +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) > >> +{ > >> + cxlds->serial= serial; > >> +} > >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); > >> + > > It would be nice to explain about how the cxl core is using these in > > the patch comments, as we just saw the stuff got promoted into the > > core. > > > As far as I can see, it is for info/debugging purposes. I will add > such explanation in next version. > > > > > >> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct > >> resource res, > >> + enum accel_resource type) > >> +{ > >> + switch (type) { > >> + case CXL_ACCEL_RES_DPA: > >> + cxlds->dpa_res = res; > >> + return; > >> + case CXL_ACCEL_RES_RAM: > >> + cxlds->ram_res = res; > >> + return; > >> + case CXL_ACCEL_RES_PMEM: > >> + cxlds->pmem_res = res; > >> + return; > >> + default: > >> + dev_err(cxlds->dev, "unkown resource type (%u)\n", > >> type); > >> + } > >> +} > >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); > >> + > > I wonder in which situation this error can be triggered. > > One can be a newer out-of-tree type-2 driver tries to work on an > > older kernel. Other situations should be the coding problem of an > > in-tree driver. > > > I guess that would point to an extension not updating this function. > > > > I prefer to WARN_ONCE() here. > > > I agree after your previous concern. > > > > > >> > >> diff --git a/include/linux/cxl_accel_mem.h > >> b/include/linux/cxl_accel_mem.h new file mode 100644 > >> index 000000000000..daf46d41f59c > >> --- /dev/null > >> +++ b/include/linux/cxl_accel_mem.h > >> @@ -0,0 +1,22 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ > >> + > >> +#include <linux/cdev.h> > >> + > >> +#ifndef __CXL_ACCEL_MEM_H > >> +#define __CXL_ACCEL_MEM_H > >> + > >> +enum accel_resource{ > >> + CXL_ACCEL_RES_DPA, > >> + CXL_ACCEL_RES_RAM, > >> + CXL_ACCEL_RES_PMEM, > >> +}; > >> + > >> +typedef struct cxl_dev_state cxl_accel_state; > > The case of using typedef in kernel coding is very rare (quite many > > of them are still there due to history reason, you can also spot > > that there is only one typedef in driver/cxl). Be sure to double > > check the coding style bible [1] when deciding to use one. :) > > > > [1] https://www.kernel.org/doc/html/v4.14/process/coding-style.html > > > Right. > > I think there is an agreement now in not using typedef but struct > cxl_dev_state so problem solved. > > > Thanks! > > >
On 8/15/24 17:35, Jonathan Cameron wrote: > On Mon, 12 Aug 2024 12:16:02 +0100 > Alejandro Lucero Palau <alucerop@amd.com> wrote: > >> On 8/4/24 18:10, Jonathan Cameron wrote: >>> On Mon, 15 Jul 2024 18:28:21 +0100 >>> <alejandro.lucero-palau@amd.com> wrote: >>> >>>> From: Alejandro Lucero <alucerop@amd.com> >>>> >>>> Differientiate Type3, aka memory expanders, from Type2, aka device >>>> accelerators, with a new function for initializing cxl_dev_state. >>>> >>>> Create opaque struct to be used by accelerators relying on new access >>>> functions in following patches. >>>> >>>> Add SFC ethernet network driver as the client. >>>> >>>> Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >>>> >>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >>> >>>> + >>>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >>>> +{ >>>> + cxlds->cxl_dvsec = dvsec; >>> Nothing to do with accel. If these make sense promote to cxl >>> core and a linux/cxl/ header. Also we may want the type3 driver to >>> switch to them long term. If nothing else, making that handle the >>> cxl_dev_state as more opaque will show up what is still directly >>> accessed and may need to be wrapped up for a future accelerator driver >>> to use. >>> >> I will change the function name then, but not sure I follow the comment >> about more opaque ... > If most code can't see the internals of cxl_dev_state because it > doesn't include the header that defines it, then we will generally > spot data that may not belong in that state structure in the first place > or where it is appropriate to have an accessor function mediating that > access. I follow that but I do not know if you are suggesting here to make it opaque which conflicts with a previous comment stating it does not need to be. > Jonathan > >
On 8/15/24 17:38, Jonathan Cameron wrote: > On Tue, 13 Aug 2024 09:30:08 +0100 > Alejandro Lucero Palau <alucerop@amd.com> wrote: > >> On 8/12/24 12:16, Alejandro Lucero Palau wrote: >>> On 8/4/24 18:10, Jonathan Cameron wrote: >>>> On Mon, 15 Jul 2024 18:28:21 +0100 >>>> <alejandro.lucero-palau@amd.com> wrote: >>>> >>>>> From: Alejandro Lucero <alucerop@amd.com> >>>>> >>>>> Differientiate Type3, aka memory expanders, from Type2, aka device >>>>> accelerators, with a new function for initializing cxl_dev_state. >>>>> >>>>> Create opaque struct to be used by accelerators relying on new access >>>>> functions in following patches. >>>>> >>>>> Add SFC ethernet network driver as the client. >>>>> >>>>> Based on >>>>> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >>>>> >>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >>>> >>> >>>>> + >>>>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >>>>> +{ >>>>> + cxlds->cxl_dvsec = dvsec; >>>> Nothing to do with accel. If these make sense promote to cxl >>>> core and a linux/cxl/ header. Also we may want the type3 driver to >>>> switch to them long term. If nothing else, making that handle the >>>> cxl_dev_state as more opaque will show up what is still directly >>>> accessed and may need to be wrapped up for a future accelerator driver >>>> to use. >>>> >>> I will change the function name then, but not sure I follow the >>> comment about more opaque ... >>> >>> >>> >> I have second thoughts about this. >> >> >> I consider this as an accessor for, as you said in a previous exchange, >> facilitating changes to the core structs without touching those accel >> drivers using it. >> >> Type3 driver is part of the CXL core and easy to change for these kind >> of updates since it will only be one driver supporting all Type3, and an >> accessor is not required then. >> >> Let me know what you think. > It's less critical, but longer term I'd expect any stuff that makes > sense for accelerators and the type 3 driver to use the same > approaches and code paths. Makes it easier to see where they > are related than opencoding the accesses in the type 3 driver will > do. In the very long term, I'd expect the type 3 driver to just be > another CXL driver alongside many others. It makes sense, so I will change the name. A following patchset when this is hopefully going through will be to use the accessors in the CXL PCI driver. Thanks! > Jonathan > >>
On 8/17/24 21:32, Zhi Wang wrote: > On Mon, 12 Aug 2024 12:34:55 +0100 > Alejandro Lucero Palau <alucerop@amd.com> wrote: > >> On 8/9/24 09:34, Zhi Wang wrote: >>> On Mon, 15 Jul 2024 18:28:21 +0100 >>> <alejandro.lucero-palau@amd.com> wrote: >>> >>>> From: Alejandro Lucero <alucerop@amd.com> >>>> >>>> Differientiate Type3, aka memory expanders, from Type2, aka device >>>> accelerators, with a new function for initializing cxl_dev_state. >>>> >>>> Create opaque struct to be used by accelerators relying on new >>>> access functions in following patches. >>>> >>>> Add SFC ethernet network driver as the client. >>>> >>>> Based on >>>> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >>>> >>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >>>> --- >>>> drivers/cxl/core/memdev.c | 52 >>>> ++++++++++++++++++++++++++ drivers/net/ethernet/sfc/Makefile | >>>> 2 +- drivers/net/ethernet/sfc/efx.c | 4 ++ >>>> drivers/net/ethernet/sfc/efx_cxl.c | 53 >>>> +++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.h | >>>> 29 +++++++++++++++ drivers/net/ethernet/sfc/net_driver.h | 4 ++ >>>> include/linux/cxl_accel_mem.h | 22 +++++++++++ >>>> include/linux/cxl_accel_pci.h | 23 ++++++++++++ >>>> 8 files changed, 188 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c >>>> create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h >>>> create mode 100644 include/linux/cxl_accel_mem.h >>>> create mode 100644 include/linux/cxl_accel_pci.h >>>> >>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c >>>> index 0277726afd04..61b5d35b49e7 100644 >>>> --- a/drivers/cxl/core/memdev.c >>>> +++ b/drivers/cxl/core/memdev.c >>>> @@ -8,6 +8,7 @@ >>>> #include <linux/idr.h> >>>> #include <linux/pci.h> >>>> #include <cxlmem.h> >>>> +#include <linux/cxl_accel_mem.h> >>> Let's keep the header inclusion in an alphabetical order. The same >>> in efx_cxl.c >> >> The headers seem to follow a reverse Christmas tree order here rather >> than an alphabetical one. >> >> Should I rearrange them all? >> > Let's fix them. > I'll do. Thanks!
On 8/19/24 12:12, Alejandro Lucero Palau wrote: > > On 8/15/24 17:38, Jonathan Cameron wrote: >> On Tue, 13 Aug 2024 09:30:08 +0100 >> Alejandro Lucero Palau <alucerop@amd.com> wrote: >> >>> On 8/12/24 12:16, Alejandro Lucero Palau wrote: >>>> On 8/4/24 18:10, Jonathan Cameron wrote: >>>>> On Mon, 15 Jul 2024 18:28:21 +0100 >>>>> <alejandro.lucero-palau@amd.com> wrote: >>>>>> From: Alejandro Lucero <alucerop@amd.com> >>>>>> >>>>>> Differientiate Type3, aka memory expanders, from Type2, aka device >>>>>> accelerators, with a new function for initializing cxl_dev_state. >>>>>> >>>>>> Create opaque struct to be used by accelerators relying on new >>>>>> access >>>>>> functions in following patches. >>>>>> >>>>>> Add SFC ethernet network driver as the client. >>>>>> >>>>>> Based on >>>>>> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e >>>>>> >>>>>> >>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> >>>>>> + >>>>>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) >>>>>> +{ >>>>>> + cxlds->cxl_dvsec = dvsec; >>>>> Nothing to do with accel. If these make sense promote to cxl >>>>> core and a linux/cxl/ header. Also we may want the type3 driver to >>>>> switch to them long term. If nothing else, making that handle the >>>>> cxl_dev_state as more opaque will show up what is still directly >>>>> accessed and may need to be wrapped up for a future accelerator >>>>> driver >>>>> to use. >>>> I will change the function name then, but not sure I follow the >>>> comment about more opaque ... >>>> >>>> >>> I have second thoughts about this. >>> >>> >>> I consider this as an accessor for, as you said in a previous >>> exchange, >>> facilitating changes to the core structs without touching those accel >>> drivers using it. >>> >>> Type3 driver is part of the CXL core and easy to change for these kind >>> of updates since it will only be one driver supporting all Type3, >>> and an >>> accessor is not required then. >>> >>> Let me know what you think. >> It's less critical, but longer term I'd expect any stuff that makes >> sense for accelerators and the type 3 driver to use the same >> approaches and code paths. Makes it easier to see where they >> are related than opencoding the accesses in the type 3 driver will >> do. In the very long term, I'd expect the type 3 driver to just be >> another CXL driver alongside many others. > > > It makes sense, so I will change the name. > > A following patchset when this is hopefully going through will be to > use the accessors in the CXL PCI driver. > > Thanks! > I realize you likely mean all the accessors and not just the dvsec one. Right? Also, I think I could add the changes to the pci driver for using them within this patchset. > >> Jonathan >> >>>
On Mon, 19 Aug 2024 12:10:34 +0100 Alejandro Lucero Palau <alucerop@amd.com> wrote: > On 8/15/24 17:35, Jonathan Cameron wrote: > > On Mon, 12 Aug 2024 12:16:02 +0100 > > Alejandro Lucero Palau <alucerop@amd.com> wrote: > > > >> On 8/4/24 18:10, Jonathan Cameron wrote: > >>> On Mon, 15 Jul 2024 18:28:21 +0100 > >>> <alejandro.lucero-palau@amd.com> wrote: > >>> > >>>> From: Alejandro Lucero <alucerop@amd.com> > >>>> > >>>> Differientiate Type3, aka memory expanders, from Type2, aka device > >>>> accelerators, with a new function for initializing cxl_dev_state. > >>>> > >>>> Create opaque struct to be used by accelerators relying on new access > >>>> functions in following patches. > >>>> > >>>> Add SFC ethernet network driver as the client. > >>>> > >>>> Based on https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e > >>>> > >>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >>>> Co-developed-by: Dan Williams <dan.j.williams@intel.com> > >>> > >>>> + > >>>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) > >>>> +{ > >>>> + cxlds->cxl_dvsec = dvsec; > >>> Nothing to do with accel. If these make sense promote to cxl > >>> core and a linux/cxl/ header. Also we may want the type3 driver to > >>> switch to them long term. If nothing else, making that handle the > >>> cxl_dev_state as more opaque will show up what is still directly > >>> accessed and may need to be wrapped up for a future accelerator driver > >>> to use. > >>> > >> I will change the function name then, but not sure I follow the comment > >> about more opaque ... > > If most code can't see the internals of cxl_dev_state because it > > doesn't include the header that defines it, then we will generally > > spot data that may not belong in that state structure in the first place > > or where it is appropriate to have an accessor function mediating that > > access. > > > I follow that but I do not know if you are suggesting here to make it > opaque which conflicts with a previous comment stating it does not need > to be. > Different potential approaches. I'm not totally sure we 'yet' care about making it opaque as we don't have that many drivers so review for misuse is enough. Longer term I think we want to get there - maybe now is the convenient moment to do so. Jonathan > > > Jonathan > > > >
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index 0277726afd04..61b5d35b49e7 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -8,6 +8,7 @@ #include <linux/idr.h> #include <linux/pci.h> #include <cxlmem.h> +#include <linux/cxl_accel_mem.h> #include "trace.h" #include "core.h" @@ -615,6 +616,25 @@ static void detach_memdev(struct work_struct *work) static struct lock_class_key cxl_memdev_key; +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) +{ + struct cxl_dev_state *cxlds; + + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); + if (!cxlds) + return ERR_PTR(-ENOMEM); + + cxlds->dev = dev; + cxlds->type = CXL_DEVTYPE_DEVMEM; + + cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa"); + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram"); + cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem"); + + return cxlds; +} +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); + static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds, const struct file_operations *fops) { @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode *inode, struct file *file) return 0; } + +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec) +{ + cxlds->cxl_dvsec = dvsec; +} +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL); + +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial) +{ + cxlds->serial= serial; +} +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL); + +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, + enum accel_resource type) +{ + switch (type) { + case CXL_ACCEL_RES_DPA: + cxlds->dpa_res = res; + return; + case CXL_ACCEL_RES_RAM: + cxlds->ram_res = res; + return; + case CXL_ACCEL_RES_PMEM: + cxlds->pmem_res = res; + return; + default: + dev_err(cxlds->dev, "unkown resource type (%u)\n", type); + } +} +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL); + static int cxl_memdev_release_file(struct inode *inode, struct file *file) { struct cxl_memdev *cxlmd = diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile index 8f446b9bd5ee..e80c713c3b0c 100644 --- a/drivers/net/ethernet/sfc/Makefile +++ b/drivers/net/ethernet/sfc/Makefile @@ -7,7 +7,7 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \ mcdi_functions.o mcdi_filters.o mcdi_mon.o \ ef100.o ef100_nic.o ef100_netdev.o \ ef100_ethtool.o ef100_rx.o ef100_tx.o \ - efx_devlink.o + efx_devlink.o efx_cxl.o sfc-$(CONFIG_SFC_MTD) += mtd.o sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \ mae.o tc.o tc_bindings.o tc_counters.o \ diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c index e9d9de8e648a..cb3f74d30852 100644 --- a/drivers/net/ethernet/sfc/efx.c +++ b/drivers/net/ethernet/sfc/efx.c @@ -33,6 +33,7 @@ #include "selftest.h" #include "sriov.h" #include "efx_devlink.h" +#include "efx_cxl.h" #include "mcdi_port_common.h" #include "mcdi_pcol.h" @@ -899,6 +900,7 @@ static void efx_pci_remove(struct pci_dev *pci_dev) efx_pci_remove_main(efx); efx_fini_io(efx); + pci_dbg(efx->pci_dev, "shutdown successful\n"); efx_fini_devlink_and_unlock(efx); @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev *pci_dev, if (rc) goto fail2; + efx_cxl_init(efx); + rc = efx_pci_probe_post_io(efx); if (rc) { /* On failure, retry once immediately. diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c new file mode 100644 index 000000000000..4554dd7cca76 --- /dev/null +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -0,0 +1,53 @@ +// 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 <linux/pci.h> +#include <linux/cxl_accel_mem.h> +#include <linux/cxl_accel_pci.h> + +#include "net_driver.h" +#include "efx_cxl.h" + +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256) + +void efx_cxl_init(struct efx_nic *efx) +{ + struct pci_dev *pci_dev = efx->pci_dev; + struct efx_cxl *cxl = efx->cxl; + struct resource res; + u16 dvsec; + + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL, + CXL_DVSEC_PCIE_DEVICE); + + if (!dvsec) + return; + + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); + + cxl->cxlds = cxl_accel_state_create(&pci_dev->dev); + if (IS_ERR(cxl->cxlds)) { + pci_info(pci_dev, "CXL accel device state failed"); + return; + } + + cxl_accel_set_dvsec(cxl->cxlds, dvsec); + cxl_accel_set_serial(cxl->cxlds, pci_dev->dev.id); + + res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE); + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_DPA); + + res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram"); + cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM); +} + + +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..76c6794c20d8 --- /dev/null +++ b/drivers/net/ethernet/sfc/efx_cxl.h @@ -0,0 +1,29 @@ +// 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_CLX_H + +#include <linux/cxl_accel_mem.h> + +struct efx_nic; + +struct efx_cxl { + cxl_accel_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; +}; + +void efx_cxl_init(struct efx_nic *efx); +#endif diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index f2dd7feb0e0c..58b7517afea4 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -814,6 +814,8 @@ enum efx_xdp_tx_queues_mode { struct efx_mae; +struct efx_cxl; + /** * struct efx_nic - an Efx NIC * @name: Device name (net device name or bus id before net device registered) @@ -962,6 +964,7 @@ struct efx_mae; * @tc: state for TC offload (EF100). * @devlink: reference to devlink structure owned by this device * @dl_port: devlink port associated with the PF + * @cxl: details of related cxl objects * @mem_bar: The BAR that is mapped into membase. * @reg_base: Offset from the start of the bar to the function control window. * @monitor_work: Hardware monitor workitem @@ -1148,6 +1151,7 @@ struct efx_nic { struct devlink *devlink; struct devlink_port *dl_port; + struct efx_cxl *cxl; unsigned int mem_bar; u32 reg_base; diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h new file mode 100644 index 000000000000..daf46d41f59c --- /dev/null +++ b/include/linux/cxl_accel_mem.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ + +#include <linux/cdev.h> + +#ifndef __CXL_ACCEL_MEM_H +#define __CXL_ACCEL_MEM_H + +enum accel_resource{ + CXL_ACCEL_RES_DPA, + CXL_ACCEL_RES_RAM, + CXL_ACCEL_RES_PMEM, +}; + +typedef struct cxl_dev_state cxl_accel_state; +cxl_accel_state *cxl_accel_state_create(struct device *dev); + +void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec); +void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial); +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res, + enum accel_resource); +#endif diff --git a/include/linux/cxl_accel_pci.h b/include/linux/cxl_accel_pci.h new file mode 100644 index 000000000000..c337ae8797e6 --- /dev/null +++ b/include/linux/cxl_accel_pci.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */ + +#ifndef __CXL_ACCEL_PCI_H +#define __CXL_ACCEL_PCI_H + +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ +#define CXL_DVSEC_PCIE_DEVICE 0 +#define CXL_DVSEC_CAP_OFFSET 0xA +#define CXL_DVSEC_MEM_CAPABLE BIT(2) +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) +#define CXL_DVSEC_CTRL_OFFSET 0xC +#define CXL_DVSEC_MEM_ENABLE BIT(2) +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) +#define CXL_DVSEC_MEM_INFO_VALID BIT(0) +#define CXL_DVSEC_MEM_ACTIVE BIT(1) +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28) +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10)) +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10)) +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28) + +#endif