| Message ID | 20241230214445.27602-4-alejandro.lucero-palau@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On Mon, 30 Dec 2024 21:44:21 +0000 <alejandro.lucero-palau@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Type2 devices have some Type3 functionalities as optional like an mbox > or an hdm decoder, and CXL core needs a way to know what an CXL accelerator > implements. > > Add a new field to cxl_dev_state for keeping device capabilities as > discovered during initialization. Add same field to cxl_port as registers > discovery is also used during port initialization. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> > Reviewed-by: Fan Ni <fan.ni@samsung.com> Comment in thread on v8. I don't see a reason to have any specific bitmap length - just use a final entry in the enum without a value set to let us know how long it actually is. Using the bit / bitmap functions should work fine without constraining that to any particular value - also allowing for greater than 64 entries with no need to fix up call sites etc. > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index 59cb35b40c7e..144ae9eb6253 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -4,6 +4,7 @@ > +enum cxl_dev_cap { > + /* capabilities from Component Registers */ > + CXL_DEV_CAP_RAS, > + CXL_DEV_CAP_HDM, > + /* capabilities from Device Registers */ > + CXL_DEV_CAP_DEV_STATUS, > + CXL_DEV_CAP_MAILBOX_PRIMARY, > + CXL_DEV_CAP_MEMDEV, > + CXL_MAX_CAPS = 64 As in v8. I'm not seeing any reason for this. If you need a bitmap to be a particular number of unsigned longs, then that code should be fixed. (only exception being compile time constant bitmaps where this is tricky to do!) Obviously I replied with that to v8 after you posted this so time machines aside no way you could have acted on it yet. Jonathan > +}; > + > struct cxl_dev_state; > struct device; >
On 1/2/25 14:36, Jonathan Cameron wrote: > On Mon, 30 Dec 2024 21:44:21 +0000 > <alejandro.lucero-palau@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Type2 devices have some Type3 functionalities as optional like an mbox >> or an hdm decoder, and CXL core needs a way to know what an CXL accelerator >> implements. >> >> Add a new field to cxl_dev_state for keeping device capabilities as >> discovered during initialization. Add same field to cxl_port as registers >> discovery is also used during port initialization. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> >> Reviewed-by: Fan Ni <fan.ni@samsung.com> > Comment in thread on v8. I don't see a reason to have any specific > bitmap length - just use a final entry in the enum without a value set > to let us know how long it actually is. I could do this but it implies to clear/zeroing the bitmaps with the final entry value and to mask bitmaps with that when comparing them. I tried to avoid the masking, and it led to that use of sizeof and then CXL_MAX_CAPS=64. > Using the bit / bitmap functions should work fine without constraining > that to any particular value - also allowing for greater than 64 entries > with no need to fix up call sites etc. > > >> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c >> index 59cb35b40c7e..144ae9eb6253 100644 >> --- a/drivers/cxl/core/regs.c >> +++ b/drivers/cxl/core/regs.c >> @@ -4,6 +4,7 @@ >> +enum cxl_dev_cap { >> + /* capabilities from Component Registers */ >> + CXL_DEV_CAP_RAS, >> + CXL_DEV_CAP_HDM, >> + /* capabilities from Device Registers */ >> + CXL_DEV_CAP_DEV_STATUS, >> + CXL_DEV_CAP_MAILBOX_PRIMARY, >> + CXL_DEV_CAP_MEMDEV, >> + CXL_MAX_CAPS = 64 > As in v8. I'm not seeing any reason for this. If you need > a bitmap to be a particular number of unsigned longs, then that > code should be fixed. (only exception being compile time constant > bitmaps where this is tricky to do!) > > Obviously I replied with that to v8 after you posted this > so time machines aside no way you could have acted on it yet. > > > Jonathan > >> +}; >> + >> struct cxl_dev_state; >> struct device; >>
On Fri, 3 Jan 2025 07:20:48 +0000 Alejandro Lucero Palau <alucerop@amd.com> wrote: > On 1/2/25 14:36, Jonathan Cameron wrote: > > On Mon, 30 Dec 2024 21:44:21 +0000 > > <alejandro.lucero-palau@amd.com> wrote: > > > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> Type2 devices have some Type3 functionalities as optional like an mbox > >> or an hdm decoder, and CXL core needs a way to know what an CXL accelerator > >> implements. > >> > >> Add a new field to cxl_dev_state for keeping device capabilities as > >> discovered during initialization. Add same field to cxl_port as registers > >> discovery is also used during port initialization. > >> > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> > >> Reviewed-by: Fan Ni <fan.ni@samsung.com> > > Comment in thread on v8. I don't see a reason to have any specific > > bitmap length - just use a final entry in the enum without a value set > > to let us know how long it actually is. > > > I could do this but it implies to clear/zeroing the bitmaps with the > final entry value and to mask bitmaps with that when comparing them. Yes but that is automatic if you use the bitmap functions throughout. > > I tried to avoid the masking, and it led to that use of sizeof and then > CXL_MAX_CAPS=64. Don't avoid it. You are creating maintenance pain for a bit of unnecessary micro optimization. Just make sure to treat this bitmap as a bitmap in all paths and there will be not reason for a reviewer to ever have to care what this value is and whether enough bits are zero etc. Jonathan > > > > Using the bit / bitmap functions should work fine without constraining > > that to any particular value - also allowing for greater than 64 entries > > with no need to fix up call sites etc. > > > > > >> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > >> index 59cb35b40c7e..144ae9eb6253 100644 > >> --- a/drivers/cxl/core/regs.c > >> +++ b/drivers/cxl/core/regs.c > >> @@ -4,6 +4,7 @@ > >> +enum cxl_dev_cap { > >> + /* capabilities from Component Registers */ > >> + CXL_DEV_CAP_RAS, > >> + CXL_DEV_CAP_HDM, > >> + /* capabilities from Device Registers */ > >> + CXL_DEV_CAP_DEV_STATUS, > >> + CXL_DEV_CAP_MAILBOX_PRIMARY, > >> + CXL_DEV_CAP_MEMDEV, > >> + CXL_MAX_CAPS = 64 > > As in v8. I'm not seeing any reason for this. If you need > > a bitmap to be a particular number of unsigned longs, then that > > code should be fixed. (only exception being compile time constant > > bitmaps where this is tricky to do!) > > > > Obviously I replied with that to v8 after you posted this > > so time machines aside no way you could have acted on it yet. > > > > > > Jonathan > > > >> +}; > >> + > >> struct cxl_dev_state; > >> struct device; > >>
On 1/3/25 10:50, Jonathan Cameron wrote: > On Fri, 3 Jan 2025 07:20:48 +0000 > Alejandro Lucero Palau <alucerop@amd.com> wrote: > >> On 1/2/25 14:36, Jonathan Cameron wrote: >>> On Mon, 30 Dec 2024 21:44:21 +0000 >>> <alejandro.lucero-palau@amd.com> wrote: >>> >>>> From: Alejandro Lucero <alucerop@amd.com> >>>> >>>> Type2 devices have some Type3 functionalities as optional like an mbox >>>> or an hdm decoder, and CXL core needs a way to know what an CXL accelerator >>>> implements. >>>> >>>> Add a new field to cxl_dev_state for keeping device capabilities as >>>> discovered during initialization. Add same field to cxl_port as registers >>>> discovery is also used during port initialization. >>>> >>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> >>>> Reviewed-by: Fan Ni <fan.ni@samsung.com> >>> Comment in thread on v8. I don't see a reason to have any specific >>> bitmap length - just use a final entry in the enum without a value set >>> to let us know how long it actually is. >> >> I could do this but it implies to clear/zeroing the bitmaps with the >> final entry value and to mask bitmaps with that when comparing them. > Yes but that is automatic if you use the bitmap functions throughout. > Oh, that is true. >> I tried to avoid the masking, and it led to that use of sizeof and then >> CXL_MAX_CAPS=64. > Don't avoid it. You are creating maintenance pain for a bit of unnecessary > micro optimization. Just make sure to treat this bitmap as a bitmap > in all paths and there will be not reason for a reviewer to ever have > to care what this value is and whether enough bits are zero etc. I'm afraid I have been in the wrong path regarding the use of the bitmap API. I even initially implemented my own bitmap_subset because I did not notice there was one already there, and I think that was the starting point of this chain of bad decisions. Because I did implement it poorly it led to keep with the wrong assumptions when I was told to use the existing one. Happy to have you pointing this out. I'll fix it. Thank you! > Jonathan > > > > >> >>> Using the bit / bitmap functions should work fine without constraining >>> that to any particular value - also allowing for greater than 64 entries >>> with no need to fix up call sites etc. >>> >>> >>>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c >>>> index 59cb35b40c7e..144ae9eb6253 100644 >>>> --- a/drivers/cxl/core/regs.c >>>> +++ b/drivers/cxl/core/regs.c >>>> @@ -4,6 +4,7 @@ >>>> +enum cxl_dev_cap { >>>> + /* capabilities from Component Registers */ >>>> + CXL_DEV_CAP_RAS, >>>> + CXL_DEV_CAP_HDM, >>>> + /* capabilities from Device Registers */ >>>> + CXL_DEV_CAP_DEV_STATUS, >>>> + CXL_DEV_CAP_MAILBOX_PRIMARY, >>>> + CXL_DEV_CAP_MEMDEV, >>>> + CXL_MAX_CAPS = 64 >>> As in v8. I'm not seeing any reason for this. If you need >>> a bitmap to be a particular number of unsigned longs, then that >>> code should be fixed. (only exception being compile time constant >>> bitmaps where this is tricky to do!) >>> >>> Obviously I replied with that to v8 after you posted this >>> so time machines aside no way you could have acted on it yet. >>> >>> >>> Jonathan >>> >>>> +}; >>>> + >>>> struct cxl_dev_state; >>>> struct device; >>>>
alejandro.lucero-palau@ wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Type2 devices have some Type3 functionalities as optional like an mbox > or an hdm decoder, and CXL core needs a way to know what an CXL accelerator > implements. > > Add a new field to cxl_dev_state for keeping device capabilities as > discovered during initialization. Add same field to cxl_port as registers > discovery is also used during port initialization. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> > Reviewed-by: Fan Ni <fan.ni@samsung.com> > --- > drivers/cxl/core/port.c | 11 +++++++---- > drivers/cxl/core/regs.c | 23 ++++++++++++++++------- > drivers/cxl/cxl.h | 9 ++++++--- > drivers/cxl/cxlmem.h | 2 ++ > drivers/cxl/pci.c | 10 ++++++---- > include/cxl/cxl.h | 19 +++++++++++++++++++ > 6 files changed, 56 insertions(+), 18 deletions(-) > [..] > @@ -113,11 +118,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, "CXL"); > * @dev: Host device of the @base mapping > * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface > * @map: Map object describing the register block information found > + * @caps: capabilities to be set when discovered > * > * Probe for device register information and return it in map object. > */ > void cxl_probe_device_regs(struct device *dev, void __iomem *base, > - struct cxl_device_reg_map *map) > + struct cxl_device_reg_map *map, unsigned long *caps) > { > int cap, cap_count; > u64 cap_array; > @@ -146,10 +152,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, > case CXLDEV_CAP_CAP_ID_DEVICE_STATUS: > dev_dbg(dev, "found Status capability (0x%x)\n", offset); > rmap = &map->status; > + set_bit(CXL_DEV_CAP_DEV_STATUS, caps); > break; > case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX: > dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset); > rmap = &map->mbox; > + set_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, caps); > break; > case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX: > dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset); > @@ -157,6 +165,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, > case CXLDEV_CAP_CAP_ID_MEMDEV: > dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset); > rmap = &map->memdev; > + set_bit(CXL_DEV_CAP_MEMDEV, caps); I do not understand the rationale for a capability bitmap. There is already a 'valid' flag in 'struct cxl_reg_map' for all register blocks. Any optional core functionality should key off those existing flags.
On 1/18/25 01:37, Dan Williams wrote: > alejandro.lucero-palau@ wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Type2 devices have some Type3 functionalities as optional like an mbox >> or an hdm decoder, and CXL core needs a way to know what an CXL accelerator >> implements. >> >> Add a new field to cxl_dev_state for keeping device capabilities as >> discovered during initialization. Add same field to cxl_port as registers >> discovery is also used during port initialization. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> >> Reviewed-by: Fan Ni <fan.ni@samsung.com> >> --- >> drivers/cxl/core/port.c | 11 +++++++---- >> drivers/cxl/core/regs.c | 23 ++++++++++++++++------- >> drivers/cxl/cxl.h | 9 ++++++--- >> drivers/cxl/cxlmem.h | 2 ++ >> drivers/cxl/pci.c | 10 ++++++---- >> include/cxl/cxl.h | 19 +++++++++++++++++++ >> 6 files changed, 56 insertions(+), 18 deletions(-) >> > [..] >> @@ -113,11 +118,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, "CXL"); >> * @dev: Host device of the @base mapping >> * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface >> * @map: Map object describing the register block information found >> + * @caps: capabilities to be set when discovered >> * >> * Probe for device register information and return it in map object. >> */ >> void cxl_probe_device_regs(struct device *dev, void __iomem *base, >> - struct cxl_device_reg_map *map) >> + struct cxl_device_reg_map *map, unsigned long *caps) >> { >> int cap, cap_count; >> u64 cap_array; >> @@ -146,10 +152,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, >> case CXLDEV_CAP_CAP_ID_DEVICE_STATUS: >> dev_dbg(dev, "found Status capability (0x%x)\n", offset); >> rmap = &map->status; >> + set_bit(CXL_DEV_CAP_DEV_STATUS, caps); >> break; >> case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX: >> dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset); >> rmap = &map->mbox; >> + set_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, caps); >> break; >> case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX: >> dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset); >> @@ -157,6 +165,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, >> case CXLDEV_CAP_CAP_ID_MEMDEV: >> dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset); >> rmap = &map->memdev; >> + set_bit(CXL_DEV_CAP_MEMDEV, caps); > I do not understand the rationale for a capability bitmap. There is > already a 'valid' flag in 'struct cxl_reg_map' for all register blocks. > Any optional core functionality should key off those existing flags. The current code is based on Type3 and the registers and capabilities are defined as mandatory, I think except RAS. With Type2 we have optional capabilities like mailbox and hdm, and the code probing the regs should not make any assumption about what should be there. With this patchset the capabilities to expect are set by the accel driver and compared with those discovered when probing CXL regs. Although the capabilities check could use the cxl_reg_map, I consider it is convenient to have a capability bitmap for keeping those discovered and easily checking them against those expected by the accel driver, and reporting them (if necessary) as well without further processing.
Alejandro Lucero Palau wrote: [..] > > I do not understand the rationale for a capability bitmap. There is > > already a 'valid' flag in 'struct cxl_reg_map' for all register blocks. > > Any optional core functionality should key off those existing flags. > > > The current code is based on Type3 and the registers and capabilities > are defined as mandatory, I think except RAS. > > With Type2 we have optional capabilities like mailbox and hdm, and the > code probing the regs should not make any assumption about what should > be there. > > With this patchset the capabilities to expect are set by the accel > driver and compared with those discovered when probing CXL regs. > Although the capabilities check could use the cxl_reg_map, I consider it > is convenient to have a capability bitmap for keeping those discovered > and easily checking them against those expected by the accel driver, and > reporting them (if necessary) as well without further processing. That is just on-loading redundancy to the core data structure for a workalike way of checking the available register blocks. The 'struct cxl_reg_map' already tracks this, the only needed change is to move the responsibility for validating those bits to the driver. That work is nearly identical to teaching the driver to inject a capability bitmask, but more flexible for the case where the driver wants to optionally enable functionality. In that latter case it will end up checking the valid-bits anyway.
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 78a5c2c25982..831bc35c2083 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -749,7 +749,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev, } static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map, - resource_size_t component_reg_phys) + resource_size_t component_reg_phys, unsigned long *caps) { *map = (struct cxl_register_map) { .host = host, @@ -763,7 +763,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map map->reg_type = CXL_REGLOC_RBI_COMPONENT; map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE; - return cxl_setup_regs(map); + return cxl_setup_regs(map, caps); } static int cxl_port_setup_regs(struct cxl_port *port, @@ -772,7 +772,7 @@ static int cxl_port_setup_regs(struct cxl_port *port, if (dev_is_platform(port->uport_dev)) return 0; return cxl_setup_comp_regs(&port->dev, &port->reg_map, - component_reg_phys); + component_reg_phys, port->capabilities); } static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport, @@ -789,7 +789,8 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport, * NULL. */ rc = cxl_setup_comp_regs(dport->dport_dev, &dport->reg_map, - component_reg_phys); + component_reg_phys, + dport->port->capabilities); dport->reg_map.host = host; return rc; } @@ -851,6 +852,8 @@ static int cxl_port_add(struct cxl_port *port, port->reg_map = cxlds->reg_map; port->reg_map.host = &port->dev; cxlmd->endpoint = port; + bitmap_copy(port->capabilities, cxlds->capabilities, + CXL_MAX_CAPS); } else if (parent_dport) { rc = dev_set_name(dev, "port%d", port->id); if (rc) diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 59cb35b40c7e..144ae9eb6253 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -4,6 +4,7 @@ #include <linux/device.h> #include <linux/slab.h> #include <linux/pci.h> +#include <cxl/cxl.h> #include <cxlmem.h> #include <cxlpci.h> #include <pmu.h> @@ -29,6 +30,7 @@ * @dev: Host device of the @base mapping * @base: Mapping containing the HDM Decoder Capability Header * @map: Map object describing the register block information found + * @caps: capabilities to be set when discovered * * See CXL 2.0 8.2.4 Component Register Layout and Definition * See CXL 2.0 8.2.5.5 CXL Device Register Interface @@ -36,7 +38,8 @@ * Probe for component register information and return it in map object. */ void cxl_probe_component_regs(struct device *dev, void __iomem *base, - struct cxl_component_reg_map *map) + struct cxl_component_reg_map *map, + unsigned long *caps) { int cap, cap_count; u32 cap_array; @@ -84,6 +87,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, decoder_cnt = cxl_hdm_decoder_count(hdr); length = 0x20 * decoder_cnt + 0x10; rmap = &map->hdm_decoder; + set_bit(CXL_DEV_CAP_HDM, caps); break; } case CXL_CM_CAP_CAP_ID_RAS: @@ -91,6 +95,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, offset); length = CXL_RAS_CAPABILITY_LENGTH; rmap = &map->ras; + set_bit(CXL_DEV_CAP_RAS, caps); break; default: dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id, @@ -113,11 +118,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, "CXL"); * @dev: Host device of the @base mapping * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface * @map: Map object describing the register block information found + * @caps: capabilities to be set when discovered * * Probe for device register information and return it in map object. */ void cxl_probe_device_regs(struct device *dev, void __iomem *base, - struct cxl_device_reg_map *map) + struct cxl_device_reg_map *map, unsigned long *caps) { int cap, cap_count; u64 cap_array; @@ -146,10 +152,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, case CXLDEV_CAP_CAP_ID_DEVICE_STATUS: dev_dbg(dev, "found Status capability (0x%x)\n", offset); rmap = &map->status; + set_bit(CXL_DEV_CAP_DEV_STATUS, caps); break; case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX: dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset); rmap = &map->mbox; + set_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, caps); break; case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX: dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset); @@ -157,6 +165,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, case CXLDEV_CAP_CAP_ID_MEMDEV: dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset); rmap = &map->memdev; + set_bit(CXL_DEV_CAP_MEMDEV, caps); break; default: if (cap_id >= 0x8000) @@ -421,7 +430,7 @@ static void cxl_unmap_regblock(struct cxl_register_map *map) map->base = NULL; } -static int cxl_probe_regs(struct cxl_register_map *map) +static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps) { struct cxl_component_reg_map *comp_map; struct cxl_device_reg_map *dev_map; @@ -431,12 +440,12 @@ static int cxl_probe_regs(struct cxl_register_map *map) switch (map->reg_type) { case CXL_REGLOC_RBI_COMPONENT: comp_map = &map->component_map; - cxl_probe_component_regs(host, base, comp_map); + cxl_probe_component_regs(host, base, comp_map, caps); dev_dbg(host, "Set up component registers\n"); break; case CXL_REGLOC_RBI_MEMDEV: dev_map = &map->device_map; - cxl_probe_device_regs(host, base, dev_map); + cxl_probe_device_regs(host, base, dev_map, caps); if (!dev_map->status.valid || !dev_map->mbox.valid || !dev_map->memdev.valid) { dev_err(host, "registers not found: %s%s%s\n", @@ -455,7 +464,7 @@ static int cxl_probe_regs(struct cxl_register_map *map) return 0; } -int cxl_setup_regs(struct cxl_register_map *map) +int cxl_setup_regs(struct cxl_register_map *map, unsigned long *caps) { int rc; @@ -463,7 +472,7 @@ int cxl_setup_regs(struct cxl_register_map *map) if (rc) return rc; - rc = cxl_probe_regs(map); + rc = cxl_probe_regs(map, caps); cxl_unmap_regblock(map); return rc; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index fdac3ddb8635..a662b1b88408 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -4,6 +4,7 @@ #ifndef __CXL_H__ #define __CXL_H__ +#include <cxl/cxl.h> #include <linux/libnvdimm.h> #include <linux/bitfield.h> #include <linux/notifier.h> @@ -292,9 +293,9 @@ struct cxl_register_map { }; void cxl_probe_component_regs(struct device *dev, void __iomem *base, - struct cxl_component_reg_map *map); + struct cxl_component_reg_map *map, unsigned long *caps); void cxl_probe_device_regs(struct device *dev, void __iomem *base, - struct cxl_device_reg_map *map); + struct cxl_device_reg_map *map, unsigned long *caps); int cxl_map_component_regs(const struct cxl_register_map *map, struct cxl_component_regs *regs, unsigned long map_mask); @@ -308,7 +309,7 @@ int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map, int index); int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map); -int cxl_setup_regs(struct cxl_register_map *map); +int cxl_setup_regs(struct cxl_register_map *map, unsigned long *caps); struct cxl_dport; resource_size_t cxl_rcd_component_reg_phys(struct device *dev, struct cxl_dport *dport); @@ -609,6 +610,7 @@ struct cxl_dax_region { * @cdat: Cached CDAT data * @cdat_available: Should a CDAT attribute be available in sysfs * @pci_latency: Upstream latency in picoseconds + * @capabilities: those capabilities as defined in device mapped registers */ struct cxl_port { struct device dev; @@ -632,6 +634,7 @@ struct cxl_port { } cdat; bool cdat_available; long pci_latency; + DECLARE_BITMAP(capabilities, CXL_MAX_CAPS); }; /** diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 2a25d1957ddb..4c1c53c29544 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -428,6 +428,7 @@ struct cxl_dpa_perf { * @serial: PCIe Device Serial Number * @type: Generic Memory Class device or Vendor Specific Memory device * @cxl_mbox: CXL mailbox context + * @capabilities: those capabilities as defined in device mapped registers */ struct cxl_dev_state { struct device *dev; @@ -443,6 +444,7 @@ struct cxl_dev_state { u64 serial; enum cxl_devtype type; struct cxl_mailbox cxl_mbox; + DECLARE_BITMAP(capabilities, CXL_MAX_CAPS); }; static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox) diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 36098e2b4235..dbc1cd9bec09 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -504,7 +504,8 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, } static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, - struct cxl_register_map *map) + struct cxl_register_map *map, + unsigned long *caps) { int rc; @@ -534,7 +535,7 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, return rc; } - return cxl_setup_regs(map); + return cxl_setup_regs(map, caps); } static int cxl_pci_ras_unmask(struct pci_dev *pdev) @@ -938,7 +939,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) cxl_set_dvsec(cxlds, dvsec); - rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map); + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, + cxlds->capabilities); if (rc) return rc; @@ -951,7 +953,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) * still be useful for management functions so don't return an error. */ rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, - &cxlds->reg_map); + &cxlds->reg_map, cxlds->capabilities); if (rc) dev_warn(&pdev->dev, "No component registers (%d)\n", rc); else if (!cxlds->reg_map.component_map.ras.valid) diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h index aa4480d49e48..c5e4b6233baa 100644 --- a/include/cxl/cxl.h +++ b/include/cxl/cxl.h @@ -12,6 +12,25 @@ enum cxl_resource { CXL_RES_PMEM, }; +/* Capabilities as defined for: + * + * Component Registers (Table 8-22 CXL 3.1 specification) + * Device Registers (8.2.8.2.1 CXL 3.1 specification) + * + * and currently being used for kernel CXL support. + */ + +enum cxl_dev_cap { + /* capabilities from Component Registers */ + CXL_DEV_CAP_RAS, + CXL_DEV_CAP_HDM, + /* capabilities from Device Registers */ + CXL_DEV_CAP_DEV_STATUS, + CXL_DEV_CAP_MAILBOX_PRIMARY, + CXL_DEV_CAP_MEMDEV, + CXL_MAX_CAPS = 64 +}; + struct cxl_dev_state; struct device;