| Message ID | 20201111054356.793390-6-ben.widawsky@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | CXL 2.0 Support | expand |
On Tue, Nov 10, 2020 at 09:43:52PM -0800, Ben Widawsky wrote: > CXL devices contain an array of capabilities that describe the > interactions software can interact with the device, or firmware running > on the device. A CXL compliant device must implement the device status > and the mailbox capability. A CXL compliant memory device must implement > the memory device capability. > > Each of the capabilities can [will] provide an offset within the MMIO > region for interacting with the CXL device. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- > 2 files changed, 143 insertions(+), 4 deletions(-) > create mode 100644 drivers/cxl/cxl.h > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > new file mode 100644 > index 000000000000..02858ae63d6d > --- /dev/null > +++ b/drivers/cxl/cxl.h > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. Fix comment usage (I think SPDX in .h needs "/* */") > +#ifndef __CXL_H__ > +#define __CXL_H__ > + > +/* Device */ > +#define CXLDEV_CAP_ARRAY_REG 0x0 > +#define CXLDEV_CAP_ARRAY_CAP_ID 0 > +#define CXLDEV_CAP_ARRAY_ID(x) ((x) & 0xffff) > +#define CXLDEV_CAP_ARRAY_COUNT(x) (((x) >> 32) & 0xffff) > + > +#define CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS 1 > +#define CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX 2 > +#define CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX 3 > +#define CXL_CAPABILITIES_CAP_ID_MEMDEV 0x4000 Strange that the first three are decimal and the last is hex. > +/* Mailbox */ > +#define CXLDEV_MB_CAPS 0x00 > +#define CXLDEV_MB_CAP_PAYLOAD_SIZE(cap) ((cap) & 0x1F) Use upper- or lower-case hex consistently. Add tabs to line things up. > +#define CXLDEV_MB_CTRL 0x04 > +#define CXLDEV_MB_CMD 0x08 > +#define CXLDEV_MB_STATUS 0x10 > +#define CXLDEV_MB_BG_CMD_STATUS 0x18 > + > +struct cxl_mem { > + struct pci_dev *pdev; > + void __iomem *regs; > + > + /* Cap 0000h */ > + struct { > + void __iomem *regs; > + } status; > + > + /* Cap 0002h */ > + struct { > + void __iomem *regs; > + size_t payload_size; > + } mbox; > + > + /* Cap 0040h */ > + struct { > + void __iomem *regs; > + } mem; > +}; Maybe a note about why READ_ONCE() is required? > +#define cxl_reg(type) \ > + static inline void cxl_write_##type##_reg32(struct cxl_mem *cxlm, \ > + u32 reg, u32 value) \ > + { \ > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > + writel(value, reg_addr + reg); \ > + } \ > + static inline void cxl_write_##type##_reg64(struct cxl_mem *cxlm, \ > + u32 reg, u64 value) \ > + { \ > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > + writeq(value, reg_addr + reg); \ > + } \ > + static inline u32 cxl_read_##type##_reg32(struct cxl_mem *cxlm, \ > + u32 reg) \ > + { \ > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > + return readl(reg_addr + reg); \ > + } \ > + static inline u64 cxl_read_##type##_reg64(struct cxl_mem *cxlm, \ > + u32 reg) \ > + { \ > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > + return readq(reg_addr + reg); \ > + } > + > +cxl_reg(status) > +cxl_reg(mbox) > + > +static inline u32 __cxl_raw_read_reg32(struct cxl_mem *cxlm, u32 reg) > +{ > + void __iomem *reg_addr = READ_ONCE(cxlm->regs); > + > + return readl(reg_addr + reg); > +} > + > +static inline u64 __cxl_raw_read_reg64(struct cxl_mem *cxlm, u32 reg) > +{ > + void __iomem *reg_addr = READ_ONCE(cxlm->regs); > + > + return readq(reg_addr + reg); > +} Are the "__" prefixes here to leave space for something else in the future? "__" typically means something like "raw", so right now it sort of reads like "raw cxl raw read". So if you don't *need* the "__" prefix, I'd drop it. > +#endif /* __CXL_H__ */ > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 8d9b9ab6c5ea..4109ef7c3ecb 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -5,11 +5,57 @@ > #include <linux/io.h> > #include "acpi.h" > #include "pci.h" > +#include "cxl.h" > > -struct cxl_mem { > - struct pci_dev *pdev; > - void __iomem *regs; > -}; Probably nicer if you put "struct cxl_mem" in its ultimate destination (drivers/cxl/cxl.h) from the beginning. Then it's easier to see what this patch adds because it's not moving at the same time. > +static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > +{ > + u64 cap_array; > + int cap; > + > + cap_array = __cxl_raw_read_reg64(cxlm, CXLDEV_CAP_ARRAY_REG); > + if (CXLDEV_CAP_ARRAY_ID(cap_array) != CXLDEV_CAP_ARRAY_CAP_ID) > + return -ENODEV; > + > + for (cap = 1; cap <= CXLDEV_CAP_ARRAY_COUNT(cap_array); cap++) { > + void *__iomem register_block; > + u32 offset; > + u16 cap_id; > + > + cap_id = __cxl_raw_read_reg32(cxlm, cap * 0x10) & 0xffff; > + offset = __cxl_raw_read_reg32(cxlm, cap * 0x10 + 0x4); > + register_block = cxlm->regs + offset; > + > + switch (cap_id) { > + case CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS: > + dev_dbg(&cxlm->pdev->dev, "found Status capability\n"); Consider including the address or offset in these messages to help debug? Printing a completely constant string always seems like a missed opportunity to me. > + cxlm->status.regs = register_block; > + break; > + case CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX: > + dev_dbg(&cxlm->pdev->dev, > + "found Mailbox capability\n"); > + cxlm->mbox.regs = register_block; > + cxlm->mbox.payload_size = CXLDEV_MB_CAP_PAYLOAD_SIZE(cap_id); > + break; > + case CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX: > + dev_dbg(&cxlm->pdev->dev, > + "found UNSUPPORTED Secondary Mailbox capability\n"); > + break; > + case CXL_CAPABILITIES_CAP_ID_MEMDEV: > + dev_dbg(&cxlm->pdev->dev, > + "found Memory Device capability\n"); > + cxlm->mem.regs = register_block; > + break; > + default: > + dev_err(&cxlm->pdev->dev, "Unknown cap ID: %d\n", cap_id); > + return -ENXIO; > + } > + } > + > + if (!cxlm->status.regs || !cxlm->mbox.regs || !cxlm->mem.regs) > + return -ENXIO; > + > + return 0; > +} > > static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi) > { > @@ -110,6 +156,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (IS_ERR(cxlm)) > return -ENXIO; > > + rc = cxl_mem_setup_regs(cxlm); > + if (rc) > + return rc; > + > pci_set_drvdata(pdev, cxlm); > > return 0; > -- > 2.29.2 >
On 20-11-13 12:26:03, Bjorn Helgaas wrote: > On Tue, Nov 10, 2020 at 09:43:52PM -0800, Ben Widawsky wrote: > > CXL devices contain an array of capabilities that describe the > > interactions software can interact with the device, or firmware running > > on the device. A CXL compliant device must implement the device status > > and the mailbox capability. A CXL compliant memory device must implement > > the memory device capability. > > > > Each of the capabilities can [will] provide an offset within the MMIO > > region for interacting with the CXL device. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- > > 2 files changed, 143 insertions(+), 4 deletions(-) > > create mode 100644 drivers/cxl/cxl.h > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > new file mode 100644 > > index 000000000000..02858ae63d6d > > --- /dev/null > > +++ b/drivers/cxl/cxl.h > > @@ -0,0 +1,89 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > Fix comment usage (I think SPDX in .h needs "/* */") > > > +#ifndef __CXL_H__ > > +#define __CXL_H__ > > + > > +/* Device */ > > +#define CXLDEV_CAP_ARRAY_REG 0x0 > > +#define CXLDEV_CAP_ARRAY_CAP_ID 0 > > +#define CXLDEV_CAP_ARRAY_ID(x) ((x) & 0xffff) > > +#define CXLDEV_CAP_ARRAY_COUNT(x) (((x) >> 32) & 0xffff) > > + > > +#define CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS 1 > > +#define CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX 2 > > +#define CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX 3 > > +#define CXL_CAPABILITIES_CAP_ID_MEMDEV 0x4000 > > Strange that the first three are decimal and the last is hex. > > > +/* Mailbox */ > > +#define CXLDEV_MB_CAPS 0x00 > > +#define CXLDEV_MB_CAP_PAYLOAD_SIZE(cap) ((cap) & 0x1F) > > Use upper- or lower-case hex consistently. Add tabs to line things > up. > > > +#define CXLDEV_MB_CTRL 0x04 > > +#define CXLDEV_MB_CMD 0x08 > > +#define CXLDEV_MB_STATUS 0x10 > > +#define CXLDEV_MB_BG_CMD_STATUS 0x18 > > + > > +struct cxl_mem { > > + struct pci_dev *pdev; > > + void __iomem *regs; > > + > > + /* Cap 0000h */ > > + struct { > > + void __iomem *regs; > > + } status; > > + > > + /* Cap 0002h */ > > + struct { > > + void __iomem *regs; > > + size_t payload_size; > > + } mbox; > > + > > + /* Cap 0040h */ > > + struct { > > + void __iomem *regs; > > + } mem; > > +}; > > Maybe a note about why READ_ONCE() is required? > I don't believe it's actually necessary. I will drop it. > > +#define cxl_reg(type) \ > > + static inline void cxl_write_##type##_reg32(struct cxl_mem *cxlm, \ > > + u32 reg, u32 value) \ > > + { \ > > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > > + writel(value, reg_addr + reg); \ > > + } \ > > + static inline void cxl_write_##type##_reg64(struct cxl_mem *cxlm, \ > > + u32 reg, u64 value) \ > > + { \ > > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > > + writeq(value, reg_addr + reg); \ > > + } \ > > + static inline u32 cxl_read_##type##_reg32(struct cxl_mem *cxlm, \ > > + u32 reg) \ > > + { \ > > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > > + return readl(reg_addr + reg); \ > > + } \ > > + static inline u64 cxl_read_##type##_reg64(struct cxl_mem *cxlm, \ > > + u32 reg) \ > > + { \ > > + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ > > + return readq(reg_addr + reg); \ > > + } > > + > > +cxl_reg(status) > > +cxl_reg(mbox) > > + > > +static inline u32 __cxl_raw_read_reg32(struct cxl_mem *cxlm, u32 reg) > > +{ > > + void __iomem *reg_addr = READ_ONCE(cxlm->regs); > > + > > + return readl(reg_addr + reg); > > +} > > + > > +static inline u64 __cxl_raw_read_reg64(struct cxl_mem *cxlm, u32 reg) > > +{ > > + void __iomem *reg_addr = READ_ONCE(cxlm->regs); > > + > > + return readq(reg_addr + reg); > > +} > > Are the "__" prefixes here to leave space for something else in the > future? "__" typically means something like "raw", so right now it > sort of reads like "raw cxl raw read". So if you don't *need* the > "__" prefix, I'd drop it. > The "__" prefix is because those functions really shouldn't be used except in early initialization and perhaps for debugfs kinds of things. I can remove the 'raw' from the name, but I do consider this a raw read as it isn't going to read/write to any particular function of a CXL device. So unless you're deeply offended by it, I'd like to make it __cxl_read/write_reg64() > > +#endif /* __CXL_H__ */ > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index 8d9b9ab6c5ea..4109ef7c3ecb 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -5,11 +5,57 @@ > > #include <linux/io.h> > > #include "acpi.h" > > #include "pci.h" > > +#include "cxl.h" > > > > -struct cxl_mem { > > - struct pci_dev *pdev; > > - void __iomem *regs; > > -}; > > Probably nicer if you put "struct cxl_mem" in its ultimate destination > (drivers/cxl/cxl.h) from the beginning. Then it's easier to see what > this patch adds because it's not moving at the same time. > Yes, this is sort of the wart again of 3 of us all working on the code at the same time. Dan, you want to squash it into yours? > > +static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > +{ > > + u64 cap_array; > > + int cap; > > + > > + cap_array = __cxl_raw_read_reg64(cxlm, CXLDEV_CAP_ARRAY_REG); > > + if (CXLDEV_CAP_ARRAY_ID(cap_array) != CXLDEV_CAP_ARRAY_CAP_ID) > > + return -ENODEV; > > + > > + for (cap = 1; cap <= CXLDEV_CAP_ARRAY_COUNT(cap_array); cap++) { > > + void *__iomem register_block; > > + u32 offset; > > + u16 cap_id; > > + > > + cap_id = __cxl_raw_read_reg32(cxlm, cap * 0x10) & 0xffff; > > + offset = __cxl_raw_read_reg32(cxlm, cap * 0x10 + 0x4); > > + register_block = cxlm->regs + offset; > > + > > + switch (cap_id) { > > + case CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS: > > + dev_dbg(&cxlm->pdev->dev, "found Status capability\n"); > > Consider including the address or offset in these messages to help > debug? Printing a completely constant string always seems like a > missed opportunity to me. > Sure. The main thing the debug message is trying to help notify is textual versions of the caps, compared to what one might expect. I don't see offsets as immediately useful, but they definitely do not hurt. > > + cxlm->status.regs = register_block; > > + break; > > + case CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX: > > + dev_dbg(&cxlm->pdev->dev, > > + "found Mailbox capability\n"); > > + cxlm->mbox.regs = register_block; > > + cxlm->mbox.payload_size = CXLDEV_MB_CAP_PAYLOAD_SIZE(cap_id); > > + break; > > + case CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX: > > + dev_dbg(&cxlm->pdev->dev, > > + "found UNSUPPORTED Secondary Mailbox capability\n"); > > + break; > > + case CXL_CAPABILITIES_CAP_ID_MEMDEV: > > + dev_dbg(&cxlm->pdev->dev, > > + "found Memory Device capability\n"); > > + cxlm->mem.regs = register_block; > > + break; > > + default: > > + dev_err(&cxlm->pdev->dev, "Unknown cap ID: %d\n", cap_id); > > + return -ENXIO; > > + } > > + } > > + > > + if (!cxlm->status.regs || !cxlm->mbox.regs || !cxlm->mem.regs) > > + return -ENXIO; > > + > > + return 0; > > +} > > > > static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi) > > { > > @@ -110,6 +156,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (IS_ERR(cxlm)) > > return -ENXIO; > > > > + rc = cxl_mem_setup_regs(cxlm); > > + if (rc) > > + return rc; > > + > > pci_set_drvdata(pdev, cxlm); > > > > return 0; > > -- > > 2.29.2 > >
On Tue, 10 Nov 2020 21:43:52 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > CXL devices contain an array of capabilities that describe the > interactions software can interact with the device, or firmware running > on the device. A CXL compliant device must implement the device status > and the mailbox capability. A CXL compliant memory device must implement > the memory device capability. > > Each of the capabilities can [will] provide an offset within the MMIO > region for interacting with the CXL device. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> A few really minor things in this one. Jonathan > --- > drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- > 2 files changed, 143 insertions(+), 4 deletions(-) > create mode 100644 drivers/cxl/cxl.h > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > new file mode 100644 > index 000000000000..02858ae63d6d > --- /dev/null > +++ b/drivers/cxl/cxl.h > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > + > +#ifndef __CXL_H__ > +#define __CXL_H__ > + > +/* Device */ > +#define CXLDEV_CAP_ARRAY_REG 0x0 > +#define CXLDEV_CAP_ARRAY_CAP_ID 0 > +#define CXLDEV_CAP_ARRAY_ID(x) ((x) & 0xffff) > +#define CXLDEV_CAP_ARRAY_COUNT(x) (((x) >> 32) & 0xffff) > + > +#define CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS 1 I'm not sure what you can do about consistent naming, but perhaps this really does need to be CXLDEV_CAP_CAP_ID_x however silly that looks. It's a funny exercise I've only seen done once in a spec, but I wish everyone would try working out what their fully expanded field names will end up as and make sure the long form naming shortens to something sensible. It definitely helps with clarity! > +#define CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX 2 > +#define CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX 3 > +#define CXL_CAPABILITIES_CAP_ID_MEMDEV 0x4000 > + > +/* Mailbox */ > +#define CXLDEV_MB_CAPS 0x00 Elsewhere you've used _OFFSET postfix. That's useful so I'd do it here as well. Cross references to the spec section always appreciated as well! > +#define CXLDEV_MB_CAP_PAYLOAD_SIZE(cap) ((cap) & 0x1F) > +#define CXLDEV_MB_CTRL 0x04 > +#define CXLDEV_MB_CMD 0x08 > +#define CXLDEV_MB_STATUS 0x10 > +#define CXLDEV_MB_BG_CMD_STATUS 0x18 > + > +struct cxl_mem { > + struct pci_dev *pdev; > + void __iomem *regs; > + > + /* Cap 0000h */ What are the numbers here? These capabilities have too many indexes associated with them in different ways for it to be obvious, so perhaps more detail in the comments? > + struct { > + void __iomem *regs; > + } status; > + > + /* Cap 0002h */ > + struct { > + void __iomem *regs; > + size_t payload_size; > + } mbox; > + > + /* Cap 0040h */ > + struct { > + void __iomem *regs; > + } mem; > +};
On 20-11-17 15:15:19, Jonathan Cameron wrote: > On Tue, 10 Nov 2020 21:43:52 -0800 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > CXL devices contain an array of capabilities that describe the > > interactions software can interact with the device, or firmware running > > on the device. A CXL compliant device must implement the device status > > and the mailbox capability. A CXL compliant memory device must implement > > the memory device capability. > > > > Each of the capabilities can [will] provide an offset within the MMIO > > region for interacting with the CXL device. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > A few really minor things in this one. > > Jonathan > > > --- > > drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- > > 2 files changed, 143 insertions(+), 4 deletions(-) > > create mode 100644 drivers/cxl/cxl.h > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > new file mode 100644 > > index 000000000000..02858ae63d6d > > --- /dev/null > > +++ b/drivers/cxl/cxl.h > > @@ -0,0 +1,89 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > + > > +#ifndef __CXL_H__ > > +#define __CXL_H__ > > + > > +/* Device */ > > +#define CXLDEV_CAP_ARRAY_REG 0x0 > > +#define CXLDEV_CAP_ARRAY_CAP_ID 0 > > +#define CXLDEV_CAP_ARRAY_ID(x) ((x) & 0xffff) > > +#define CXLDEV_CAP_ARRAY_COUNT(x) (((x) >> 32) & 0xffff) > > + > > +#define CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS 1 > > I'm not sure what you can do about consistent naming, but > perhaps this really does need to be > CXLDEV_CAP_CAP_ID_x however silly that looks. > > It's a funny exercise I've only seen done once in a spec, but > I wish everyone would try working out what their fully expanded > field names will end up as and make sure the long form naming shortens > to something sensible. It definitely helps with clarity! > > > +#define CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX 2 > > +#define CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX 3 > > +#define CXL_CAPABILITIES_CAP_ID_MEMDEV 0x4000 > > + > > +/* Mailbox */ > > +#define CXLDEV_MB_CAPS 0x00 > > Elsewhere you've used _OFFSET postfix. That's useful so I'd do it here > as well. Cross references to the spec section always appreciated as well! > > > +#define CXLDEV_MB_CAP_PAYLOAD_SIZE(cap) ((cap) & 0x1F) > > +#define CXLDEV_MB_CTRL 0x04 > > +#define CXLDEV_MB_CMD 0x08 > > +#define CXLDEV_MB_STATUS 0x10 > > +#define CXLDEV_MB_BG_CMD_STATUS 0x18 > > + > > +struct cxl_mem { > > + struct pci_dev *pdev; > > + void __iomem *regs; > > + > > + /* Cap 0000h */ > > What are the numbers here? These capabilities have too > many indexes associated with them in different ways for it > to be obvious, so perhaps more detail in the comments? This comment was a bug. The cap is 0001h actually. I've added the hash define for its cap id as part of the comment. Everything else is accepted. > > > + struct { > > + void __iomem *regs; > > + } status; > > + > > + /* Cap 0002h */ > > + struct { > > + void __iomem *regs; > > + size_t payload_size; > > + } mbox; > > + > > + /* Cap 0040h */ > > + struct { > > + void __iomem *regs; > > + } mem; > > +};
On 11/11/20 12:43 AM, Ben Widawsky wrote: > + case CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX: > + dev_dbg(&cxlm->pdev->dev, > + "found UNSUPPORTED Secondary Mailbox capability\n"); Per spec, the secondary mailbox is intended for use by platform firmware, so Linux should never be using it anyway. Maybe that message is slightly misleading? Jon. P.S. Related - I've severe doubts about the mailbox approach being proposed by CXL and have begun to push back through the spec org.
On 20-11-26 01:05:56, Jon Masters wrote: > On 11/11/20 12:43 AM, Ben Widawsky wrote: > > > + case CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX: > > + dev_dbg(&cxlm->pdev->dev, > > + "found UNSUPPORTED Secondary Mailbox capability\n"); > > Per spec, the secondary mailbox is intended for use by platform firmware, so > Linux should never be using it anyway. Maybe that message is slightly > misleading? Yeah, I think the message could be reworded, but it is dev_dbg, so I wasn't too worried about the wording in the first place. I think it is a mistake in this case for the spec to describe the intended purpose. If the expectation is for platform firmware to use it, but there is no negotiation mechanism in place, it's essentially useless. > > Jon. > > P.S. Related - I've severe doubts about the mailbox approach being proposed > by CXL and have begun to push back through the spec org. Any reason not to articulate that here? Now that the spec is public, I don't see any reason not to disclose that publicly.
On Wed, Nov 25, 2020 at 10:06 PM Jon Masters <jcm@jonmasters.org> wrote: > > On 11/11/20 12:43 AM, Ben Widawsky wrote: > > > + case CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX: > > + dev_dbg(&cxlm->pdev->dev, > > + "found UNSUPPORTED Secondary Mailbox capability\n"); > > Per spec, the secondary mailbox is intended for use by platform > firmware, so Linux should never be using it anyway. Maybe that message > is slightly misleading? > > Jon. > > P.S. Related - I've severe doubts about the mailbox approach being > proposed by CXL and have begun to push back through the spec org. The more Linux software voices the better. At the same time the spec is released so we're into xkcd territory [1] of what the driver will be expected to support for any future improvements. [1]: https://xkcd.com/927/
On Tue, Nov 10, 2020 at 9:44 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > CXL devices contain an array of capabilities that describe the > interactions software can interact with the device, or firmware running > on the device. A CXL compliant device must implement the device status > and the mailbox capability. A CXL compliant memory device must implement > the memory device capability. > > Each of the capabilities can [will] provide an offset within the MMIO > region for interacting with the CXL device. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- > 2 files changed, 143 insertions(+), 4 deletions(-) > create mode 100644 drivers/cxl/cxl.h > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > new file mode 100644 > index 000000000000..02858ae63d6d > --- /dev/null > +++ b/drivers/cxl/cxl.h [..] > +static inline u32 __cxl_raw_read_reg32(struct cxl_mem *cxlm, u32 reg) Going through my reworks and the "raw" jumped out at me. My typical interpretation of "raw" in respect to register access macros is the difference between readl() and __raw_readl() which means "don't do bus endian swizzling, and don't do a memory clobber barrier". Any heartburn to drop the "raw"? ...is it only me that reacts that way?
On 20-12-03 23:41:16, Dan Williams wrote: > On Tue, Nov 10, 2020 at 9:44 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > CXL devices contain an array of capabilities that describe the > > interactions software can interact with the device, or firmware running > > on the device. A CXL compliant device must implement the device status > > and the mailbox capability. A CXL compliant memory device must implement > > the memory device capability. > > > > Each of the capabilities can [will] provide an offset within the MMIO > > region for interacting with the CXL device. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- > > 2 files changed, 143 insertions(+), 4 deletions(-) > > create mode 100644 drivers/cxl/cxl.h > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > new file mode 100644 > > index 000000000000..02858ae63d6d > > --- /dev/null > > +++ b/drivers/cxl/cxl.h > [..] > > +static inline u32 __cxl_raw_read_reg32(struct cxl_mem *cxlm, u32 reg) > > Going through my reworks and the "raw" jumped out at me. My typical > interpretation of "raw" in respect to register access macros is the > difference between readl() and __raw_readl() which means "don't do > bus endian swizzling, and don't do a memory clobber barrier". Any > heartburn to drop the "raw"? > > ...is it only me that reacts that way? I will drop "raw". Especially given that I intend to reuse the word in v2 for something entirely different, it makes sense. My idea of "raw" was that it's just unfettered access to the device's MMIO space. No offsets, no checks. I'm not sure of a better adjective to describe that, but if you have any in mind, I'd like to add it.
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h new file mode 100644 index 000000000000..02858ae63d6d --- /dev/null +++ b/drivers/cxl/cxl.h @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright(c) 2020 Intel Corporation. All rights reserved. + +#ifndef __CXL_H__ +#define __CXL_H__ + +/* Device */ +#define CXLDEV_CAP_ARRAY_REG 0x0 +#define CXLDEV_CAP_ARRAY_CAP_ID 0 +#define CXLDEV_CAP_ARRAY_ID(x) ((x) & 0xffff) +#define CXLDEV_CAP_ARRAY_COUNT(x) (((x) >> 32) & 0xffff) + +#define CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS 1 +#define CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX 2 +#define CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX 3 +#define CXL_CAPABILITIES_CAP_ID_MEMDEV 0x4000 + +/* Mailbox */ +#define CXLDEV_MB_CAPS 0x00 +#define CXLDEV_MB_CAP_PAYLOAD_SIZE(cap) ((cap) & 0x1F) +#define CXLDEV_MB_CTRL 0x04 +#define CXLDEV_MB_CMD 0x08 +#define CXLDEV_MB_STATUS 0x10 +#define CXLDEV_MB_BG_CMD_STATUS 0x18 + +struct cxl_mem { + struct pci_dev *pdev; + void __iomem *regs; + + /* Cap 0000h */ + struct { + void __iomem *regs; + } status; + + /* Cap 0002h */ + struct { + void __iomem *regs; + size_t payload_size; + } mbox; + + /* Cap 0040h */ + struct { + void __iomem *regs; + } mem; +}; + +#define cxl_reg(type) \ + static inline void cxl_write_##type##_reg32(struct cxl_mem *cxlm, \ + u32 reg, u32 value) \ + { \ + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ + writel(value, reg_addr + reg); \ + } \ + static inline void cxl_write_##type##_reg64(struct cxl_mem *cxlm, \ + u32 reg, u64 value) \ + { \ + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ + writeq(value, reg_addr + reg); \ + } \ + static inline u32 cxl_read_##type##_reg32(struct cxl_mem *cxlm, \ + u32 reg) \ + { \ + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ + return readl(reg_addr + reg); \ + } \ + static inline u64 cxl_read_##type##_reg64(struct cxl_mem *cxlm, \ + u32 reg) \ + { \ + void __iomem *reg_addr = READ_ONCE(cxlm->type.regs); \ + return readq(reg_addr + reg); \ + } + +cxl_reg(status) +cxl_reg(mbox) + +static inline u32 __cxl_raw_read_reg32(struct cxl_mem *cxlm, u32 reg) +{ + void __iomem *reg_addr = READ_ONCE(cxlm->regs); + + return readl(reg_addr + reg); +} + +static inline u64 __cxl_raw_read_reg64(struct cxl_mem *cxlm, u32 reg) +{ + void __iomem *reg_addr = READ_ONCE(cxlm->regs); + + return readq(reg_addr + reg); +} +#endif /* __CXL_H__ */ diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 8d9b9ab6c5ea..4109ef7c3ecb 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -5,11 +5,57 @@ #include <linux/io.h> #include "acpi.h" #include "pci.h" +#include "cxl.h" -struct cxl_mem { - struct pci_dev *pdev; - void __iomem *regs; -}; +static int cxl_mem_setup_regs(struct cxl_mem *cxlm) +{ + u64 cap_array; + int cap; + + cap_array = __cxl_raw_read_reg64(cxlm, CXLDEV_CAP_ARRAY_REG); + if (CXLDEV_CAP_ARRAY_ID(cap_array) != CXLDEV_CAP_ARRAY_CAP_ID) + return -ENODEV; + + for (cap = 1; cap <= CXLDEV_CAP_ARRAY_COUNT(cap_array); cap++) { + void *__iomem register_block; + u32 offset; + u16 cap_id; + + cap_id = __cxl_raw_read_reg32(cxlm, cap * 0x10) & 0xffff; + offset = __cxl_raw_read_reg32(cxlm, cap * 0x10 + 0x4); + register_block = cxlm->regs + offset; + + switch (cap_id) { + case CXL_CAPABILITIES_CAP_ID_DEVICE_STATUS: + dev_dbg(&cxlm->pdev->dev, "found Status capability\n"); + cxlm->status.regs = register_block; + break; + case CXL_CAPABILITIES_CAP_ID_PRIMARY_MAILBOX: + dev_dbg(&cxlm->pdev->dev, + "found Mailbox capability\n"); + cxlm->mbox.regs = register_block; + cxlm->mbox.payload_size = CXLDEV_MB_CAP_PAYLOAD_SIZE(cap_id); + break; + case CXL_CAPABILITIES_CAP_ID_SECONDARY_MAILBOX: + dev_dbg(&cxlm->pdev->dev, + "found UNSUPPORTED Secondary Mailbox capability\n"); + break; + case CXL_CAPABILITIES_CAP_ID_MEMDEV: + dev_dbg(&cxlm->pdev->dev, + "found Memory Device capability\n"); + cxlm->mem.regs = register_block; + break; + default: + dev_err(&cxlm->pdev->dev, "Unknown cap ID: %d\n", cap_id); + return -ENXIO; + } + } + + if (!cxlm->status.regs || !cxlm->mbox.regs || !cxlm->mem.regs) + return -ENXIO; + + return 0; +} static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi) { @@ -110,6 +156,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (IS_ERR(cxlm)) return -ENXIO; + rc = cxl_mem_setup_regs(cxlm); + if (rc) + return rc; + pci_set_drvdata(pdev, cxlm); return 0;
CXL devices contain an array of capabilities that describe the interactions software can interact with the device, or firmware running on the device. A CXL compliant device must implement the device status and the mailbox capability. A CXL compliant memory device must implement the memory device capability. Each of the capabilities can [will] provide an offset within the MMIO region for interacting with the CXL device. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/cxl.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/cxl/mem.c | 58 +++++++++++++++++++++++++++--- 2 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 drivers/cxl/cxl.h