On 20-11-17 15:15:19, Jonathan Cameron wrote: > On Tue, 10 Nov 2020 21:43:52 -0800 > Ben Widawsky <ben.widawsky@xxxxxxxxx> 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@xxxxxxxxx> > > 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; > > +};