diff mbox series

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

Message ID 20201111054356.793390-6-ben.widawsky@intel.com (mailing list archive)
State RFC, archived
Headers show
Series CXL 2.0 Support | expand

Commit Message

Ben Widawsky Nov. 11, 2020, 5:43 a.m. UTC
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

Comments

Bjorn Helgaas Nov. 13, 2020, 6:26 p.m. UTC | #1
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
>
Ben Widawsky Nov. 14, 2020, 1:36 a.m. UTC | #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
> >
Jonathan Cameron Nov. 17, 2020, 3:15 p.m. UTC | #3
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;
> +};
Ben Widawsky Nov. 24, 2020, 12:17 a.m. UTC | #4
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;
> > +};
Jon Masters Nov. 26, 2020, 6:05 a.m. UTC | #5
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.
Ben Widawsky Nov. 26, 2020, 6:18 p.m. UTC | #6
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.
Dan Williams Dec. 4, 2020, 7:35 a.m. UTC | #7
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/
Dan Williams Dec. 4, 2020, 7:41 a.m. UTC | #8
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?
Ben Widawsky Dec. 7, 2020, 6:12 a.m. UTC | #9
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 mbox series

Patch

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;