Re: [RFC PATCH 5/9] cxl/mem: Find device capabilities

[Ben Widawsky <ben.widawsky@intel.com>]

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;
> > +};