| Message ID | 20210407222625.320177-8-ben.widawsky@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Enumerate HDM Decoder registers | expand |
On Wed, 7 Apr 2021 15:26:25 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > An HDM decoder is defined in the CXL 2.0 specification as a mechanism > that allow devices and upstream ports to claim memory address ranges and > participate in interleave sets. HDM decoder registers are within the > component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component > Registers as part of the CXL.cache and CXL.mem subregion. > > The Component Register Block is found via the Register Locator DVSEC > in a similar fashion to how the CXL Device Register Block is found. The > primary difference is the capability id size of the Component Register > Block is a single DWORD instead of 4 DWORDS. > > It's now possible to configure a CXL type 3 device's HDM decoder. Such > programming is expected for CXL devices with persistent memory, and hot > plugged CXL devices that participate in CXL.mem with volatile memory. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> Some register field naming suggestions. Otherwise looks fine to me. > --- > drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 48 ++++++++++++++++++++++++++++++ > drivers/cxl/mem.c | 37 ++++++++++++++++++++--- > drivers/cxl/pci.h | 1 + > 4 files changed, 155 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > index 65cd704581bc..db6a83eed0a2 100644 > --- a/drivers/cxl/core.c > +++ b/drivers/cxl/core.c > @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host, > } > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > + struct cxl_component_regs *regs) > +{ > + int cap, cap_count; > + u64 cap_array; > + > + *regs = (struct cxl_component_regs) { 0 }; > + > + /* > + * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in > + * CXL 2.0 8.2.4 Table 141. > + * > + * TODO: Map other registers as needed. > + */ > + base += CXL_CM_OFFSET; > + > + cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET); > + > + if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) { > + dev_err(dev, > + "Couldn't locate the CXL.cache and CXL.mem capability array header./n"); > + return; > + } > + > + /* It's assumed that future versions will be backward compatible */ > +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg) \ > + do { \ > + if (FIELD_GET(mask, data) < expected) { \ > + dev_err(dev, \ > + capability_msg \ > + " version %ld is below expected %d", \ /n > + FIELD_GET(mask, data), expected); \ > + return; \ > + } \ > + } while (0) > + > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK, > + CM_CAP_HDR_CAP_VERSION, "Capability array header"); > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK, > + CM_CAP_HDR_CACHE_MEM_VERSION, > + "Capability array header CXL.cache CXL.mem"); Is that macro worth bothering with? Particularly as it will make the string harder to grep for. ver = FIELD_GET(CXL_CM_CAP_HDR_VERSION_MASK, cap_array); if (ver < CM_CAP_HDR_CAP_VERSION)) { dev_err(dev, "Capability array header version %ld is below expected %d./n", ver, CM_CAP_HDER_CAP_VERSION); etc seems better to me given we only have two instances. > + > + cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array); > + > + for (cap = 1; cap <= cap_count; cap++) { > + void __iomem *register_block; > + u32 hdr; > + u16 cap_id, offset; > + > + hdr = readl(base + cap * 0x4); > + > + cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr); See below, but I'd suggest some name changes for defines. Whilst it's the same value, this is in a different type of entry to where you use CXL_CM_CAP_HDR_ID_MASK above. Confused me so may confuse others :) > + offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr); > + register_block = base + offset; > + > + switch (cap_id) { > + case CXL_CM_CAP_CAP_ID_HDM: > + CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK, > + CXL_CM_CAP_CAP_HDM_VERSION, > + "HDM decoder capability"); > + dev_dbg(dev, "found HDM decoder capability (0x%x)\n", > + offset); > + regs->hdm_decoder = register_block; > + break; > + default: > + dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id, > + offset); > + break; > + } > + } > +} > +EXPORT_SYMBOL_GPL(cxl_setup_component_regs); > + > /* > * cxl_setup_device_regs() - Detect CXL Device register blocks > * @dev: Host device of the @base mapping > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 0211f44c95a2..a4ad1176dc5a 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -8,6 +8,31 @@ > #include <linux/bitops.h> > #include <linux/io.h> > > +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/ > +#define CXL_CM_OFFSET 0x1000 > +#define CXL_CM_CAP_HDR_OFFSET 0x0 > +#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0) > +#define CM_CAP_HDR_CAP_ID 1 > +#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16) > +#define CM_CAP_HDR_CAP_VERSION 1 > +#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20) > +#define CM_CAP_HDR_CACHE_MEM_VERSION 1 > +#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24) > +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20) I'd separate the field naming for those in CXL Capability Header Register and those for the individual headers. Naming tricky though. If you don't do that it looks like you have overlapping fields because of the reuse above.. e.g also have #define CXL_CM_CAP_X_HDR_ID_MASK GENMASK(15, 0) #define CXL_CM_CAP_X_HDR_CAP_MASK GENMASK(19, 16) and rename to #define CXL_CM_CAP_X_HDR_POINTER_MASK GENMASK(31, 20) > + > +#define CXL_CM_CAP_CAP_ID_HDM 0x5 > +#define CXL_CM_CAP_CAP_HDM_VERSION 1 > + > +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */ > +#define CXL_HDM_DECODER_CAP_OFFSET 0x0 > +#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0) > +#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10 > +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14 > +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18 > +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c > +#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20 > + > /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ > #define CXLDEV_CAP_ARRAY_OFFSET 0x0 > #define CXLDEV_CAP_ARRAY_CAP_ID 0 > @@ -35,11 +60,26 @@ > #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 > > /* See note for 'struct cxl_regs' for the rationale of this organization */ > +#define CXL_COMPONENT_REGS() \ > + void __iomem *hdm_decoder > + > #define CXL_DEVICE_REGS() \ > void __iomem *status; \ > void __iomem *mbox; \ > void __iomem *memdev > > +/** > + * struct cxl_component_regs - Common container of CXL component register block > + * base pointers. kernel-doc script won't like this. Your best bet may be to just drop the /** to /* and have it ignored. > + * > + * The only component registers that we care about are the CXL.cache and CXL.mem > + * registers which are at offset 0x1000 from the component register base (CXL > + * 2.0 8.2.4) > + */ > +struct cxl_component_regs { > + CXL_COMPONENT_REGS(); > +}; > + > /** > * struct cxl_device_regs - Common container of CXL Device register > * block base pointers > @@ -59,6 +99,12 @@ struct cxl_device_regs { > * The specificity reads naturally from left-to-right. > */ > struct cxl_regs { > + union { > + struct { > + CXL_COMPONENT_REGS(); > + }; > + struct cxl_component_regs component; > + }; > union { > struct { > CXL_DEVICE_REGS(); > @@ -67,6 +113,8 @@ struct cxl_regs { > }; > }; > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > + struct cxl_component_regs *regs); > void cxl_setup_device_regs(struct device *dev, void __iomem *base, > struct cxl_device_regs *regs); > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index b7342aaf38c4..e915e3743b76 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > return 0; > } > > +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base) > +{ > + struct cxl_regs *regs = &cxlm->regs; > + struct pci_dev *pdev = cxlm->pdev; > + struct device *dev = &pdev->dev; > + > + cxl_setup_component_regs(dev, base, ®s->component); > + if (!regs->hdm_decoder) { > + dev_err(dev, "HDM decoder registers not found\n"); > + return -ENXIO; > + } > + > + return 0; > +} > + > static int __cxl_setup_device_regs(struct cxl_mem *cxlm, void __iomem *base) > { > struct cxl_regs *regs = &cxlm->regs; > @@ -1032,16 +1047,30 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > pci_read_config_dword(pdev, regloc + 4, ®_hi); > > reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); > + if (reg_type == CXL_REGLOC_RBI_EMPTY || > + reg_type > CXL_REGLOC_RBI_MAX) > + continue; > + > + base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); > + if (IS_ERR(base)) > + return PTR_ERR(base); > > - if (reg_type == CXL_REGLOC_RBI_MEMDEV) { > - base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); > - if (IS_ERR(base)) > - return PTR_ERR(base); > + switch (reg_type) { > + case CXL_REGLOC_RBI_COMPONENT: > + rc = __cxl_setup_component_regs(cxlm, base); > + if (rc) > + return rc; > > + dev_dbg(dev, "Set up component registers\n"); > + break; > + case CXL_REGLOC_RBI_MEMDEV: > rc = __cxl_setup_device_regs(cxlm, base); > if (rc) > return rc; > > + dev_dbg(dev, "Set up device registers\n"); > + break; > + default: > break; > } > } > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > index af3ec078cf6c..8b8c6afbe605 100644 > --- a/drivers/cxl/pci.h > +++ b/drivers/cxl/pci.h > @@ -25,6 +25,7 @@ > #define CXL_REGLOC_RBI_COMPONENT 1 > #define CXL_REGLOC_RBI_VIRT 2 > #define CXL_REGLOC_RBI_MEMDEV 3 > +#define CXL_REGLOC_RBI_MAX CXL_REGLOC_RBI_MEMDEV > > #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16) >
On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > An HDM decoder is defined in the CXL 2.0 specification as a mechanism > that allow devices and upstream ports to claim memory address ranges and > participate in interleave sets. HDM decoder registers are within the > component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component > Registers as part of the CXL.cache and CXL.mem subregion. > > The Component Register Block is found via the Register Locator DVSEC > in a similar fashion to how the CXL Device Register Block is found. The > primary difference is the capability id size of the Component Register > Block is a single DWORD instead of 4 DWORDS. > > It's now possible to configure a CXL type 3 device's HDM decoder. It's always been possible and this patch just prepares for programming them, but does not actually touch them yet. So let's drop this as I'm not sure what it is saying. > Such > programming is expected for CXL devices with persistent memory, and hot > plugged CXL devices that participate in CXL.mem with volatile memory. Let's drop this as well because the state of what the OS is expected to program is platform and topology specific. The OS needs to be prepared for the HDMs to be unprogrammed, validly programmed in a state the OS will leave untouched, and/or validly programmed in a way the OS wants to redo. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 48 ++++++++++++++++++++++++++++++ > drivers/cxl/mem.c | 37 ++++++++++++++++++++--- > drivers/cxl/pci.h | 1 + > 4 files changed, 155 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > index 65cd704581bc..db6a83eed0a2 100644 > --- a/drivers/cxl/core.c > +++ b/drivers/cxl/core.c > @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host, > } > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > + struct cxl_component_regs *regs) > +{ > + int cap, cap_count; > + u64 cap_array; > + > + *regs = (struct cxl_component_regs) { 0 }; > + > + /* > + * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in > + * CXL 2.0 8.2.4 Table 141. > + * > + * TODO: Map other registers as needed. That TODO goes without saying. > + */ > + base += CXL_CM_OFFSET; > + > + cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET); > + > + if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) { > + dev_err(dev, > + "Couldn't locate the CXL.cache and CXL.mem capability array header./n"); > + return; > + } > + > + /* It's assumed that future versions will be backward compatible */ If future versions are backwards compatible then why check? If someone wants a driver that supports new capabilities then update the driver, but validating that the version is non-zero seems to be a pedantic check for no good reason to me. > +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg) \ > + do { \ > + if (FIELD_GET(mask, data) < expected) { \ > + dev_err(dev, \ > + capability_msg \ > + " version %ld is below expected %d", \ > + FIELD_GET(mask, data), expected); \ > + return; \ Ugh, "return" in a macro, please no. > + } \ > + } while (0) > + > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK, > + CM_CAP_HDR_CAP_VERSION, "Capability array header"); > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK, > + CM_CAP_HDR_CACHE_MEM_VERSION, > + "Capability array header CXL.cache CXL.mem"); > + > + cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array); > + > + for (cap = 1; cap <= cap_count; cap++) { > + void __iomem *register_block; > + u32 hdr; > + u16 cap_id, offset; > + > + hdr = readl(base + cap * 0x4); > + > + cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr); > + offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr); > + register_block = base + offset; > + > + switch (cap_id) { > + case CXL_CM_CAP_CAP_ID_HDM: > + CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK, > + CXL_CM_CAP_CAP_HDM_VERSION, > + "HDM decoder capability"); > + dev_dbg(dev, "found HDM decoder capability (0x%x)\n", > + offset); > + regs->hdm_decoder = register_block; > + break; > + default: > + dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id, Hex cap id? Perhaps rather than "Unknown" this can just say "Skipping" because some of these are technically it's just the kernel does not care. > + offset); > + break; > + } > + } > +} > +EXPORT_SYMBOL_GPL(cxl_setup_component_regs); > + > /* > * cxl_setup_device_regs() - Detect CXL Device register blocks > * @dev: Host device of the @base mapping > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 0211f44c95a2..a4ad1176dc5a 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -8,6 +8,31 @@ > #include <linux/bitops.h> > #include <linux/io.h> > > +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/ > +#define CXL_CM_OFFSET 0x1000 > +#define CXL_CM_CAP_HDR_OFFSET 0x0 > +#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0) > +#define CM_CAP_HDR_CAP_ID 1 > +#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16) > +#define CM_CAP_HDR_CAP_VERSION 1 > +#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20) > +#define CM_CAP_HDR_CACHE_MEM_VERSION 1 > +#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24) > +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20) > + > +#define CXL_CM_CAP_CAP_ID_HDM 0x5 > +#define CXL_CM_CAP_CAP_HDM_VERSION 1 > + > +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */ > +#define CXL_HDM_DECODER_CAP_OFFSET 0x0 > +#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0) > +#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10 > +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14 > +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18 > +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c > +#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20 > + > /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ > #define CXLDEV_CAP_ARRAY_OFFSET 0x0 > #define CXLDEV_CAP_ARRAY_CAP_ID 0 > @@ -35,11 +60,26 @@ > #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 > > /* See note for 'struct cxl_regs' for the rationale of this organization */ > +#define CXL_COMPONENT_REGS() \ > + void __iomem *hdm_decoder > + > #define CXL_DEVICE_REGS() \ > void __iomem *status; \ > void __iomem *mbox; \ > void __iomem *memdev > > +/** > + * struct cxl_component_regs - Common container of CXL component register block > + * base pointers. > + * > + * The only component registers that we care about are the CXL.cache and CXL.mem > + * registers which are at offset 0x1000 from the component register base (CXL > + * 2.0 8.2.4) > + */ I don't think kernel-doc will be happy about this, so probably follow the same fixup I did when Jonathan pointed this out for the cxl_port patches. > +struct cxl_component_regs { > + CXL_COMPONENT_REGS(); > +}; > + > /** > * struct cxl_device_regs - Common container of CXL Device register > * block base pointers > @@ -59,6 +99,12 @@ struct cxl_device_regs { > * The specificity reads naturally from left-to-right. > */ > struct cxl_regs { > + union { > + struct { > + CXL_COMPONENT_REGS(); > + }; > + struct cxl_component_regs component; > + }; > union { > struct { > CXL_DEVICE_REGS(); > @@ -67,6 +113,8 @@ struct cxl_regs { > }; > }; > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > + struct cxl_component_regs *regs); > void cxl_setup_device_regs(struct device *dev, void __iomem *base, > struct cxl_device_regs *regs); > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index b7342aaf38c4..e915e3743b76 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > return 0; > } > > +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base) > +{ > + struct cxl_regs *regs = &cxlm->regs; > + struct pci_dev *pdev = cxlm->pdev; > + struct device *dev = &pdev->dev; > + > + cxl_setup_component_regs(dev, base, ®s->component); > + if (!regs->hdm_decoder) { > + dev_err(dev, "HDM decoder registers not found\n"); > + return -ENXIO; I was hoping this patch would show the justification of the organization proposed in patch-6, but it doesn't and I think we're better off open coding the error check in cxl_mem_setup_regs(). All the other patches in this series I did not comment on look good to me.
Thanks for looking. Mostly trivial agreements or disagreements which I don't care much about, but I'd really like you to focus on the last point, please. On 21-04-15 16:27:14, Dan Williams wrote: > On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > An HDM decoder is defined in the CXL 2.0 specification as a mechanism > > that allow devices and upstream ports to claim memory address ranges and > > participate in interleave sets. HDM decoder registers are within the > > component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component > > Registers as part of the CXL.cache and CXL.mem subregion. > > > > The Component Register Block is found via the Register Locator DVSEC > > in a similar fashion to how the CXL Device Register Block is found. The > > primary difference is the capability id size of the Component Register > > Block is a single DWORD instead of 4 DWORDS. > > > > It's now possible to configure a CXL type 3 device's HDM decoder. > > It's always been possible and this patch just prepares for programming > them, but does not actually touch them yet. So let's drop this as I'm > not sure what it is saying. > > > Such > > programming is expected for CXL devices with persistent memory, and hot > > plugged CXL devices that participate in CXL.mem with volatile memory. > > Let's drop this as well because the state of what the OS is expected > to program is platform and topology specific. The OS needs to be > prepared for the HDMs to be unprogrammed, validly programmed in a > state the OS will leave untouched, and/or validly programmed in a way > the OS wants to redo. > Okay to both. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/cxl.h | 48 ++++++++++++++++++++++++++++++ > > drivers/cxl/mem.c | 37 ++++++++++++++++++++--- > > drivers/cxl/pci.h | 1 + > > 4 files changed, 155 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > > index 65cd704581bc..db6a83eed0a2 100644 > > --- a/drivers/cxl/core.c > > +++ b/drivers/cxl/core.c > > @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host, > > } > > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > > + struct cxl_component_regs *regs) > > +{ > > + int cap, cap_count; > > + u64 cap_array; > > + > > + *regs = (struct cxl_component_regs) { 0 }; > > + > > + /* > > + * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in > > + * CXL 2.0 8.2.4 Table 141. > > + * > > + * TODO: Map other registers as needed. > > That TODO goes without saying. > I don't agree with this. The device register implementation is complete as is, and this one is not. > > + */ > > + base += CXL_CM_OFFSET; > > + > > + cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET); > > + > > + if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) { > > + dev_err(dev, > > + "Couldn't locate the CXL.cache and CXL.mem capability array header./n"); > > + return; > > + } > > + > > + /* It's assumed that future versions will be backward compatible */ > > If future versions are backwards compatible then why check? If someone > wants a driver that supports new capabilities then update the driver, > but validating that the version is non-zero seems to be a pedantic > check for no good reason to me. > This was specifically meant as a way to prevent the driver being used for 1.1 without explicit support being added (and then dropping this check). You're right though , it should be !=, it should have been <. It's sounding like you'd rather I remove it entirely rather than fix it though. Is that correct? > > +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg) \ > > + do { \ > > + if (FIELD_GET(mask, data) < expected) { \ > > + dev_err(dev, \ > > + capability_msg \ > > + " version %ld is below expected %d", \ > > + FIELD_GET(mask, data), expected); \ > > + return; \ > > Ugh, "return" in a macro, please no. > Okay. If you're okay with the macro, I'll rewrite it to bool and then return based on that. > > + } \ > > + } while (0) > > + > > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK, > > + CM_CAP_HDR_CAP_VERSION, "Capability array header"); > > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK, > > + CM_CAP_HDR_CACHE_MEM_VERSION, > > + "Capability array header CXL.cache CXL.mem"); > > + > > + cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array); > > + > > + for (cap = 1; cap <= cap_count; cap++) { > > + void __iomem *register_block; > > + u32 hdr; > > + u16 cap_id, offset; > > + > > + hdr = readl(base + cap * 0x4); > > + > > + cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr); > > + offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr); > > + register_block = base + offset; > > + > > + switch (cap_id) { > > + case CXL_CM_CAP_CAP_ID_HDM: > > + CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK, > > + CXL_CM_CAP_CAP_HDM_VERSION, > > + "HDM decoder capability"); > > + dev_dbg(dev, "found HDM decoder capability (0x%x)\n", > > + offset); > > + regs->hdm_decoder = register_block; > > + break; > > + default: > > + dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id, > > Hex cap id? Perhaps rather than "Unknown" this can just say "Skipping" > because some of these are technically it's just the kernel does not > care. > I just sent some patches for devices that I'd like to include for 5.12 to this effect. I'll fix this here too. > > + offset); > > + break; > > + } > > + } > > +} > > +EXPORT_SYMBOL_GPL(cxl_setup_component_regs); > > + > > /* > > * cxl_setup_device_regs() - Detect CXL Device register blocks > > * @dev: Host device of the @base mapping > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 0211f44c95a2..a4ad1176dc5a 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -8,6 +8,31 @@ > > #include <linux/bitops.h> > > #include <linux/io.h> > > > > +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/ > > +#define CXL_CM_OFFSET 0x1000 > > +#define CXL_CM_CAP_HDR_OFFSET 0x0 > > +#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0) > > +#define CM_CAP_HDR_CAP_ID 1 > > +#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16) > > +#define CM_CAP_HDR_CAP_VERSION 1 > > +#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20) > > +#define CM_CAP_HDR_CACHE_MEM_VERSION 1 > > +#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24) > > +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20) > > + > > +#define CXL_CM_CAP_CAP_ID_HDM 0x5 > > +#define CXL_CM_CAP_CAP_HDM_VERSION 1 > > + > > +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */ > > +#define CXL_HDM_DECODER_CAP_OFFSET 0x0 > > +#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0) > > +#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > > +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10 > > +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14 > > +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18 > > +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c > > +#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20 > > + > > /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ > > #define CXLDEV_CAP_ARRAY_OFFSET 0x0 > > #define CXLDEV_CAP_ARRAY_CAP_ID 0 > > @@ -35,11 +60,26 @@ > > #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 > > > > /* See note for 'struct cxl_regs' for the rationale of this organization */ > > +#define CXL_COMPONENT_REGS() \ > > + void __iomem *hdm_decoder > > + > > #define CXL_DEVICE_REGS() \ > > void __iomem *status; \ > > void __iomem *mbox; \ > > void __iomem *memdev > > > > +/** > > + * struct cxl_component_regs - Common container of CXL component register block > > + * base pointers. > > + * > > + * The only component registers that we care about are the CXL.cache and CXL.mem > > + * registers which are at offset 0x1000 from the component register base (CXL > > + * 2.0 8.2.4) > > + */ > > I don't think kernel-doc will be happy about this, so probably follow > the same fixup I did when Jonathan pointed this out for the cxl_port > patches. > I must have missed something on this. I thought the issue with yours was the way in which you were calling out the structure members. I skipped this completely in mine. Is the ask here to add "@hdm" to the documentation? I don't see any other issue here. > > +struct cxl_component_regs { > > + CXL_COMPONENT_REGS(); > > +}; > > + > > /** > > * struct cxl_device_regs - Common container of CXL Device register > > * block base pointers > > @@ -59,6 +99,12 @@ struct cxl_device_regs { > > * The specificity reads naturally from left-to-right. > > */ > > struct cxl_regs { > > + union { > > + struct { > > + CXL_COMPONENT_REGS(); > > + }; > > + struct cxl_component_regs component; > > + }; > > union { > > struct { > > CXL_DEVICE_REGS(); > > @@ -67,6 +113,8 @@ struct cxl_regs { > > }; > > }; > > > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > > + struct cxl_component_regs *regs); > > void cxl_setup_device_regs(struct device *dev, void __iomem *base, > > struct cxl_device_regs *regs); > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index b7342aaf38c4..e915e3743b76 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > return 0; > > } > > > > +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base) > > +{ > > + struct cxl_regs *regs = &cxlm->regs; > > + struct pci_dev *pdev = cxlm->pdev; > > + struct device *dev = &pdev->dev; > > + > > + cxl_setup_component_regs(dev, base, ®s->component); > > + if (!regs->hdm_decoder) { > > + dev_err(dev, "HDM decoder registers not found\n"); > > + return -ENXIO; > > I was hoping this patch would show the justification of the > organization proposed in patch-6, but it doesn't and I think we're > better off open coding the error check in cxl_mem_setup_regs(). Open coding is functionally equivalent and I think much less tidy, in particular if you consider adding more register block types in the future (and specifically, more component register types in the blocks, in the future). I would predict that this kind of cleanup will happen at some point, however if you'd prefer to kick the can down the road, we can do that. If the heartburn is over the naming specifically, it would be my preference to fix that rather than open-coding. Addressing the feedback from patch 6 here, since they're tied together, "__" prefix is, I want to call the core function but I have certain expectations about the results of that function, and thus the wrapper (sounds like you got that). TBF, the naming is backward as I'd prefer the core function be __cxl_setup.., and this one be something like cxl_mem_setup_component_regs(). I just didn't want to touch the thing you'd previously named. Perhaps that naming more appropriately construes what I'm trying to do? > > All the other patches in this series I did not comment on look good to me.
On Thu, Apr 15, 2021 at 4:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Thanks for looking. Mostly trivial agreements or disagreements which I don't > care much about, but I'd really like you to focus on the last point, please. > > On 21-04-15 16:27:14, Dan Williams wrote: > > On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > An HDM decoder is defined in the CXL 2.0 specification as a mechanism > > > that allow devices and upstream ports to claim memory address ranges and > > > participate in interleave sets. HDM decoder registers are within the > > > component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component > > > Registers as part of the CXL.cache and CXL.mem subregion. > > > > > > The Component Register Block is found via the Register Locator DVSEC > > > in a similar fashion to how the CXL Device Register Block is found. The > > > primary difference is the capability id size of the Component Register > > > Block is a single DWORD instead of 4 DWORDS. > > > > > > It's now possible to configure a CXL type 3 device's HDM decoder. > > > > It's always been possible and this patch just prepares for programming > > them, but does not actually touch them yet. So let's drop this as I'm > > not sure what it is saying. > > > > > Such > > > programming is expected for CXL devices with persistent memory, and hot > > > plugged CXL devices that participate in CXL.mem with volatile memory. > > > > Let's drop this as well because the state of what the OS is expected > > to program is platform and topology specific. The OS needs to be > > prepared for the HDMs to be unprogrammed, validly programmed in a > > state the OS will leave untouched, and/or validly programmed in a way > > the OS wants to redo. > > > > Okay to both. > > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > --- > > > drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > > > drivers/cxl/cxl.h | 48 ++++++++++++++++++++++++++++++ > > > drivers/cxl/mem.c | 37 ++++++++++++++++++++--- > > > drivers/cxl/pci.h | 1 + > > > 4 files changed, 155 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > > > index 65cd704581bc..db6a83eed0a2 100644 > > > --- a/drivers/cxl/core.c > > > +++ b/drivers/cxl/core.c > > > @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host, > > > } > > > EXPORT_SYMBOL_GPL(devm_cxl_add_port); > > > > > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > > > + struct cxl_component_regs *regs) > > > +{ > > > + int cap, cap_count; > > > + u64 cap_array; > > > + > > > + *regs = (struct cxl_component_regs) { 0 }; > > > + > > > + /* > > > + * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in > > > + * CXL 2.0 8.2.4 Table 141. > > > + * > > > + * TODO: Map other registers as needed. > > > > That TODO goes without saying. > > > > I don't agree with this. The device register implementation is complete as is, > and this one is not. ...but there's a pile of stuff in the component registers that the kernel will never care about. So if the bar is "remove when all capabilities are mapped" then the TODO never gets removed. > > > > + */ > > > + base += CXL_CM_OFFSET; > > > + > > > + cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET); > > > + > > > + if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) { > > > + dev_err(dev, > > > + "Couldn't locate the CXL.cache and CXL.mem capability array header./n"); > > > + return; > > > + } > > > + > > > + /* It's assumed that future versions will be backward compatible */ > > > > If future versions are backwards compatible then why check? If someone > > wants a driver that supports new capabilities then update the driver, > > but validating that the version is non-zero seems to be a pedantic > > check for no good reason to me. > > > > This was specifically meant as a way to prevent the driver being used for 1.1 > without explicit support being added (and then dropping this check). Seems premature to assume this code path would be reused for 1.1, also CXL 1.1 also puts version1 here. The difference between 1.1 and 2.0 component registers is more potential capability ids, not a different capability version number if I'm reading the specs correctly. > You're > right though , it should be !=, it should have been <. It's sounding like you'd > rather I remove it entirely rather than fix it though. Is that correct? > > > > +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg) \ > > > + do { \ > > > + if (FIELD_GET(mask, data) < expected) { \ > > > + dev_err(dev, \ > > > + capability_msg \ > > > + " version %ld is below expected %d", \ > > > + FIELD_GET(mask, data), expected); \ > > > + return; \ > > > > Ugh, "return" in a macro, please no. > > > > Okay. If you're okay with the macro, I'll rewrite it to bool and then return > based on that. No, per above, I still don't think this is adding any value. > > > > + } \ > > > + } while (0) > > > + > > > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK, > > > + CM_CAP_HDR_CAP_VERSION, "Capability array header"); > > > + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK, > > > + CM_CAP_HDR_CACHE_MEM_VERSION, > > > + "Capability array header CXL.cache CXL.mem"); > > > + > > > + cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array); > > > + > > > + for (cap = 1; cap <= cap_count; cap++) { > > > + void __iomem *register_block; > > > + u32 hdr; > > > + u16 cap_id, offset; > > > + > > > + hdr = readl(base + cap * 0x4); > > > + > > > + cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr); > > > + offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr); > > > + register_block = base + offset; > > > + > > > + switch (cap_id) { > > > + case CXL_CM_CAP_CAP_ID_HDM: > > > + CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK, > > > + CXL_CM_CAP_CAP_HDM_VERSION, > > > + "HDM decoder capability"); > > > + dev_dbg(dev, "found HDM decoder capability (0x%x)\n", > > > + offset); > > > + regs->hdm_decoder = register_block; > > > + break; > > > + default: > > > + dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id, > > > > Hex cap id? Perhaps rather than "Unknown" this can just say "Skipping" > > because some of these are technically it's just the kernel does not > > care. > > > > I just sent some patches for devices that I'd like to include for 5.12 to this > effect. I'll fix this here too. > > > > + offset); > > > + break; > > > + } > > > + } > > > +} > > > +EXPORT_SYMBOL_GPL(cxl_setup_component_regs); > > > + > > > /* > > > * cxl_setup_device_regs() - Detect CXL Device register blocks > > > * @dev: Host device of the @base mapping > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > > index 0211f44c95a2..a4ad1176dc5a 100644 > > > --- a/drivers/cxl/cxl.h > > > +++ b/drivers/cxl/cxl.h > > > @@ -8,6 +8,31 @@ > > > #include <linux/bitops.h> > > > #include <linux/io.h> > > > > > > +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/ > > > +#define CXL_CM_OFFSET 0x1000 > > > +#define CXL_CM_CAP_HDR_OFFSET 0x0 > > > +#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0) > > > +#define CM_CAP_HDR_CAP_ID 1 > > > +#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16) > > > +#define CM_CAP_HDR_CAP_VERSION 1 > > > +#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20) > > > +#define CM_CAP_HDR_CACHE_MEM_VERSION 1 > > > +#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24) > > > +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20) > > > + > > > +#define CXL_CM_CAP_CAP_ID_HDM 0x5 > > > +#define CXL_CM_CAP_CAP_HDM_VERSION 1 > > > + > > > +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */ > > > +#define CXL_HDM_DECODER_CAP_OFFSET 0x0 > > > +#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0) > > > +#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > > > +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10 > > > +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14 > > > +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18 > > > +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c > > > +#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20 > > > + > > > /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ > > > #define CXLDEV_CAP_ARRAY_OFFSET 0x0 > > > #define CXLDEV_CAP_ARRAY_CAP_ID 0 > > > @@ -35,11 +60,26 @@ > > > #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 > > > > > > /* See note for 'struct cxl_regs' for the rationale of this organization */ > > > +#define CXL_COMPONENT_REGS() \ > > > + void __iomem *hdm_decoder > > > + > > > #define CXL_DEVICE_REGS() \ > > > void __iomem *status; \ > > > void __iomem *mbox; \ > > > void __iomem *memdev > > > > > > +/** > > > + * struct cxl_component_regs - Common container of CXL component register block > > > + * base pointers. > > > + * > > > + * The only component registers that we care about are the CXL.cache and CXL.mem > > > + * registers which are at offset 0x1000 from the component register base (CXL > > > + * 2.0 8.2.4) > > > + */ > > > > I don't think kernel-doc will be happy about this, so probably follow > > the same fixup I did when Jonathan pointed this out for the cxl_port > > patches. > > > > I must have missed something on this. I thought the issue with yours was the way > in which you were calling out the structure members. I skipped this completely > in mine. Is the ask here to add "@hdm" to the documentation? I don't see any > other issue here. The issue is that this is a struct entry with no description of the members. I ended up doing this for device registers: /* * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers * @status: CXL 2.0 8.2.8.3 Device Status Registers * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers */ #define CXL_DEVICE_REGS() \ void __iomem *status; \ void __iomem *mbox; \ void __iomem *memdev /* See note for 'struct cxl_regs' for the rationale of this organization */ struct cxl_device_regs { CXL_DEVICE_REGS(); }; ...i.e. just abandon kernel-doc for this organization, but this might be more evidence that this arrangement is more clever than its worth. > > > > +struct cxl_component_regs { > > > + CXL_COMPONENT_REGS(); > > > +}; > > > + > > > /** > > > * struct cxl_device_regs - Common container of CXL Device register > > > * block base pointers > > > @@ -59,6 +99,12 @@ struct cxl_device_regs { > > > * The specificity reads naturally from left-to-right. > > > */ > > > struct cxl_regs { > > > + union { > > > + struct { > > > + CXL_COMPONENT_REGS(); > > > + }; > > > + struct cxl_component_regs component; > > > + }; > > > union { > > > struct { > > > CXL_DEVICE_REGS(); > > > @@ -67,6 +113,8 @@ struct cxl_regs { > > > }; > > > }; > > > > > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base, > > > + struct cxl_component_regs *regs); > > > void cxl_setup_device_regs(struct device *dev, void __iomem *base, > > > struct cxl_device_regs *regs); > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > index b7342aaf38c4..e915e3743b76 100644 > > > --- a/drivers/cxl/mem.c > > > +++ b/drivers/cxl/mem.c > > > @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > > return 0; > > > } > > > > > > +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base) > > > +{ > > > + struct cxl_regs *regs = &cxlm->regs; > > > + struct pci_dev *pdev = cxlm->pdev; > > > + struct device *dev = &pdev->dev; > > > + > > > + cxl_setup_component_regs(dev, base, ®s->component); > > > + if (!regs->hdm_decoder) { > > > + dev_err(dev, "HDM decoder registers not found\n"); > > > + return -ENXIO; > > > > I was hoping this patch would show the justification of the > > organization proposed in patch-6, but it doesn't and I think we're > > better off open coding the error check in cxl_mem_setup_regs(). > > Open coding is functionally equivalent and I think much less tidy, in particular > if you consider adding more register block types in the future I'm not seeing much register block type support being added in the near term beyond HDMs. > (and > specifically, more component register types in the blocks, in the future). I > would predict that this kind of cleanup will happen at some point, however if > you'd prefer to kick the can down the road, we can do that. > > If the heartburn is over the naming specifically, it would be my preference to > fix that rather than open-coding. It just feels like the helpers here are too tiny to provide any significant cleanup. > > Addressing the feedback from patch 6 here, since they're tied together, "__" > prefix is, I want to call the core function but I have certain expectations > about the results of that function, and thus the wrapper (sounds like you got > that). TBF, the naming is backward as I'd prefer the core function be > __cxl_setup.., and this one be something like cxl_mem_setup_component_regs(). I > just didn't want to touch the thing you'd previously named. Perhaps that naming > more appropriately construes what I'm trying to do? I'm open to better naming, but not __func() wrapping func().
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c index 65cd704581bc..db6a83eed0a2 100644 --- a/drivers/cxl/core.c +++ b/drivers/cxl/core.c @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host, } EXPORT_SYMBOL_GPL(devm_cxl_add_port); +void cxl_setup_component_regs(struct device *dev, void __iomem *base, + struct cxl_component_regs *regs) +{ + int cap, cap_count; + u64 cap_array; + + *regs = (struct cxl_component_regs) { 0 }; + + /* + * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in + * CXL 2.0 8.2.4 Table 141. + * + * TODO: Map other registers as needed. + */ + base += CXL_CM_OFFSET; + + cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET); + + if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) { + dev_err(dev, + "Couldn't locate the CXL.cache and CXL.mem capability array header./n"); + return; + } + + /* It's assumed that future versions will be backward compatible */ +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg) \ + do { \ + if (FIELD_GET(mask, data) < expected) { \ + dev_err(dev, \ + capability_msg \ + " version %ld is below expected %d", \ + FIELD_GET(mask, data), expected); \ + return; \ + } \ + } while (0) + + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK, + CM_CAP_HDR_CAP_VERSION, "Capability array header"); + CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK, + CM_CAP_HDR_CACHE_MEM_VERSION, + "Capability array header CXL.cache CXL.mem"); + + cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array); + + for (cap = 1; cap <= cap_count; cap++) { + void __iomem *register_block; + u32 hdr; + u16 cap_id, offset; + + hdr = readl(base + cap * 0x4); + + cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr); + offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr); + register_block = base + offset; + + switch (cap_id) { + case CXL_CM_CAP_CAP_ID_HDM: + CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK, + CXL_CM_CAP_CAP_HDM_VERSION, + "HDM decoder capability"); + dev_dbg(dev, "found HDM decoder capability (0x%x)\n", + offset); + regs->hdm_decoder = register_block; + break; + default: + dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id, + offset); + break; + } + } +} +EXPORT_SYMBOL_GPL(cxl_setup_component_regs); + /* * cxl_setup_device_regs() - Detect CXL Device register blocks * @dev: Host device of the @base mapping diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 0211f44c95a2..a4ad1176dc5a 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -8,6 +8,31 @@ #include <linux/bitops.h> #include <linux/io.h> +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/ +#define CXL_CM_OFFSET 0x1000 +#define CXL_CM_CAP_HDR_OFFSET 0x0 +#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0) +#define CM_CAP_HDR_CAP_ID 1 +#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16) +#define CM_CAP_HDR_CAP_VERSION 1 +#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20) +#define CM_CAP_HDR_CACHE_MEM_VERSION 1 +#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24) +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20) + +#define CXL_CM_CAP_CAP_ID_HDM 0x5 +#define CXL_CM_CAP_CAP_HDM_VERSION 1 + +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */ +#define CXL_HDM_DECODER_CAP_OFFSET 0x0 +#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0) +#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10 +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14 +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18 +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c +#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20 + /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */ #define CXLDEV_CAP_ARRAY_OFFSET 0x0 #define CXLDEV_CAP_ARRAY_CAP_ID 0 @@ -35,11 +60,26 @@ #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 /* See note for 'struct cxl_regs' for the rationale of this organization */ +#define CXL_COMPONENT_REGS() \ + void __iomem *hdm_decoder + #define CXL_DEVICE_REGS() \ void __iomem *status; \ void __iomem *mbox; \ void __iomem *memdev +/** + * struct cxl_component_regs - Common container of CXL component register block + * base pointers. + * + * The only component registers that we care about are the CXL.cache and CXL.mem + * registers which are at offset 0x1000 from the component register base (CXL + * 2.0 8.2.4) + */ +struct cxl_component_regs { + CXL_COMPONENT_REGS(); +}; + /** * struct cxl_device_regs - Common container of CXL Device register * block base pointers @@ -59,6 +99,12 @@ struct cxl_device_regs { * The specificity reads naturally from left-to-right. */ struct cxl_regs { + union { + struct { + CXL_COMPONENT_REGS(); + }; + struct cxl_component_regs component; + }; union { struct { CXL_DEVICE_REGS(); @@ -67,6 +113,8 @@ struct cxl_regs { }; }; +void cxl_setup_component_regs(struct device *dev, void __iomem *base, + struct cxl_component_regs *regs); void cxl_setup_device_regs(struct device *dev, void __iomem *base, struct cxl_device_regs *regs); diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index b7342aaf38c4..e915e3743b76 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) return 0; } +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base) +{ + struct cxl_regs *regs = &cxlm->regs; + struct pci_dev *pdev = cxlm->pdev; + struct device *dev = &pdev->dev; + + cxl_setup_component_regs(dev, base, ®s->component); + if (!regs->hdm_decoder) { + dev_err(dev, "HDM decoder registers not found\n"); + return -ENXIO; + } + + return 0; +} + static int __cxl_setup_device_regs(struct cxl_mem *cxlm, void __iomem *base) { struct cxl_regs *regs = &cxlm->regs; @@ -1032,16 +1047,30 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) pci_read_config_dword(pdev, regloc + 4, ®_hi); reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); + if (reg_type == CXL_REGLOC_RBI_EMPTY || + reg_type > CXL_REGLOC_RBI_MAX) + continue; + + base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); + if (IS_ERR(base)) + return PTR_ERR(base); - if (reg_type == CXL_REGLOC_RBI_MEMDEV) { - base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); - if (IS_ERR(base)) - return PTR_ERR(base); + switch (reg_type) { + case CXL_REGLOC_RBI_COMPONENT: + rc = __cxl_setup_component_regs(cxlm, base); + if (rc) + return rc; + dev_dbg(dev, "Set up component registers\n"); + break; + case CXL_REGLOC_RBI_MEMDEV: rc = __cxl_setup_device_regs(cxlm, base); if (rc) return rc; + dev_dbg(dev, "Set up device registers\n"); + break; + default: break; } } diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h index af3ec078cf6c..8b8c6afbe605 100644 --- a/drivers/cxl/pci.h +++ b/drivers/cxl/pci.h @@ -25,6 +25,7 @@ #define CXL_REGLOC_RBI_COMPONENT 1 #define CXL_REGLOC_RBI_VIRT 2 #define CXL_REGLOC_RBI_MEMDEV 3 +#define CXL_REGLOC_RBI_MAX CXL_REGLOC_RBI_MEMDEV #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
An HDM decoder is defined in the CXL 2.0 specification as a mechanism that allow devices and upstream ports to claim memory address ranges and participate in interleave sets. HDM decoder registers are within the component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component Registers as part of the CXL.cache and CXL.mem subregion. The Component Register Block is found via the Register Locator DVSEC in a similar fashion to how the CXL Device Register Block is found. The primary difference is the capability id size of the Component Register Block is a single DWORD instead of 4 DWORDS. It's now possible to configure a CXL type 3 device's HDM decoder. Such programming is expected for CXL devices with persistent memory, and hot plugged CXL devices that participate in CXL.mem with volatile memory. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 48 ++++++++++++++++++++++++++++++ drivers/cxl/mem.c | 37 ++++++++++++++++++++--- drivers/cxl/pci.h | 1 + 4 files changed, 155 insertions(+), 4 deletions(-)