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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>