On Thu, 1 Apr 2021 07:30:53 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> CXL MMIO register blocks are organized by device type and capabilities.
> There are Component registers, Device registers (yes, an ambiguous
> name), and Memory Device registers (a specific extension of Device
> registers).
>
> It is possible for a given device instance (endpoint or port) to
> implement register sets from multiple of the above categories.
>
> The driver code that enumerates and maps the registers is type specific
> so it is useful to have a dedicated type and helpers for each block
> type.
>
> At the same time, once the registers are mapped the origin type does not
> matter. It is overly pedantic to reference the register block type in
> code that is using the registers.
>
> In preparation for the endpoint driver to incorporate Component registers
> into its MMIO operations reorganize the registers to allow typed
> enumeration + mapping, but anonymous usage. With the end state of
> 'struct cxl_regs' to be:
>
> struct cxl_regs {
> union {
> struct {
> CXL_DEVICE_REGS();
> };
> struct cxl_device_regs device_regs;
> };
> union {
> struct {
> CXL_COMPONENT_REGS();
> };
> struct cxl_component_regs component_regs;
> };
> };
>
> With this arrangement the driver can share component init code with
> ports, but when using the registers it can directly reference the
> component register block type by name without the 'component_regs'
> prefix.
>
> So, map + enumerate can be shared across drivers of different CXL
> classes e.g.:
>
> void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> struct cxl_device_regs *regs);
>
> void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> struct cxl_component_regs *regs);
>
> ...while inline usage in the driver need not indicate where the
> registers came from:
>
> readl(cxlm->regs.mbox + MBOX_OFFSET);
> readl(cxlm->regs.hdm + HDM_OFFSET);
>
> ...instead of:
>
> readl(cxlm->regs.device_regs.mbox + MBOX_OFFSET);
> readl(cxlm->regs.component_regs.hdm + HDM_OFFSET);
>
> This complexity of the definition in .h yields improvement in code
> readability in .c while maintaining type-safety for organization of
> setup code. It prepares the implementation to maintain organization in
> the face of CXL devices that compose register interfaces consisting of
> multiple types.
>
> Reviewed-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
A few minor things inline.
> ---
> drivers/cxl/cxl.h | 33 +++++++++++++++++++++++++++++++++
> drivers/cxl/mem.c | 44 ++++++++++++++++++++++++--------------------
> drivers/cxl/mem.h | 13 +++++--------
> 3 files changed, 62 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2e3bdacb32e7..37325e504fb7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -34,5 +34,38 @@
> #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
> #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>
> +/* See note for 'struct cxl_regs' for the rationale of this organization */
> +#define CXL_DEVICE_REGS() \
> + void __iomem *status; \
> + void __iomem *mbox; \
> + void __iomem *memdev
> +
> +/**
> + * struct cxl_device_regs - Common container of CXL Device register
> + * block base pointers
> + * @status: CXL 2.0 8.2.8.3 Device Status Registers
> + * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
> + * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
kernel-doc script is not going to be happy with documenting fields it can't see
+ not documenting the CXL_DEVICE_REGS() field it can.
I've no idea what the right way to handle this might be.
> + */
> +struct cxl_device_regs {
> + CXL_DEVICE_REGS();
> +};
> +
> +/*
> + * Note, the anonymous union organization allows for per
> + * register-block-type helper routines, without requiring block-type
> + * agnostic code to include the prefix. I.e.
> + * cxl_setup_device_regs(&cxlm->regs.dev) vs readl(cxlm->regs.mbox).
> + * The specificity reads naturally from left-to-right.
> + */
> +struct cxl_regs {
> + union {
> + struct {
> + CXL_DEVICE_REGS();
> + };
> + struct cxl_device_regs device_regs;
> + };
> +};
> +
> extern struct bus_type cxl_bus_type;
> #endif /* __CXL_H__ */
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 45871ef65152..6951243d128e 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -31,7 +31,7 @@
> */
>
> #define cxl_doorbell_busy(cxlm) \
> - (readl((cxlm)->mbox_regs + CXLDEV_MBOX_CTRL_OFFSET) & \
> + (readl((cxlm)->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET) & \
> CXLDEV_MBOX_CTRL_DOORBELL)
>
> /* CXL 2.0 - 8.2.8.4 */
> @@ -271,7 +271,7 @@ static void cxl_mem_mbox_timeout(struct cxl_mem *cxlm,
> static int __cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> struct mbox_cmd *mbox_cmd)
> {
> - void __iomem *payload = cxlm->mbox_regs + CXLDEV_MBOX_PAYLOAD_OFFSET;
> + void __iomem *payload = cxlm->regs.mbox + CXLDEV_MBOX_PAYLOAD_OFFSET;
> u64 cmd_reg, status_reg;
> size_t out_len;
> int rc;
> @@ -314,12 +314,12 @@ static int __cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> }
>
> /* #2, #3 */
> - writeq(cmd_reg, cxlm->mbox_regs + CXLDEV_MBOX_CMD_OFFSET);
> + writeq(cmd_reg, cxlm->regs.mbox + CXLDEV_MBOX_CMD_OFFSET);
>
> /* #4 */
> dev_dbg(&cxlm->pdev->dev, "Sending command\n");
> writel(CXLDEV_MBOX_CTRL_DOORBELL,
> - cxlm->mbox_regs + CXLDEV_MBOX_CTRL_OFFSET);
> + cxlm->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
>
> /* #5 */
> rc = cxl_mem_wait_for_doorbell(cxlm);
> @@ -329,7 +329,7 @@ static int __cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> }
>
> /* #6 */
> - status_reg = readq(cxlm->mbox_regs + CXLDEV_MBOX_STATUS_OFFSET);
> + status_reg = readq(cxlm->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
> mbox_cmd->return_code =
> FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>
> @@ -339,7 +339,7 @@ static int __cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> }
>
> /* #7 */
> - cmd_reg = readq(cxlm->mbox_regs + CXLDEV_MBOX_CMD_OFFSET);
> + cmd_reg = readq(cxlm->regs.mbox + CXLDEV_MBOX_CMD_OFFSET);
> out_len = FIELD_GET(CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK, cmd_reg);
>
> /* #8 */
> @@ -400,7 +400,7 @@ static int cxl_mem_mbox_get(struct cxl_mem *cxlm)
> goto out;
> }
>
> - md_status = readq(cxlm->memdev_regs + CXLMDEV_STATUS_OFFSET);
> + md_status = readq(cxlm->regs.memdev + CXLMDEV_STATUS_OFFSET);
> if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
> dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
> rc = -EBUSY;
> @@ -868,7 +868,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> int cap, cap_count;
> u64 cap_array;
>
> - cap_array = readq(cxlm->regs + CXLDEV_CAP_ARRAY_OFFSET);
> + cap_array = readq(cxlm->base + CXLDEV_CAP_ARRAY_OFFSET);
> if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
> CXLDEV_CAP_ARRAY_CAP_ID)
> return -ENODEV;
> @@ -881,25 +881,25 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> u16 cap_id;
>
> cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
> - readl(cxlm->regs + cap * 0x10));
> - offset = readl(cxlm->regs + cap * 0x10 + 0x4);
> - register_block = cxlm->regs + offset;
> + readl(cxlm->base + cap * 0x10));
> + offset = readl(cxlm->base + cap * 0x10 + 0x4);
> + register_block = cxlm->base + offset;
>
> switch (cap_id) {
> case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
> dev_dbg(dev, "found Status capability (0x%x)\n", offset);
> - cxlm->status_regs = register_block;
> + cxlm->regs.status = register_block;
> break;
> case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
> dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
> - cxlm->mbox_regs = register_block;
> + cxlm->regs.mbox = register_block;
> break;
> case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
> dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
> break;
> case CXLDEV_CAP_CAP_ID_MEMDEV:
> dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
> - cxlm->memdev_regs = register_block;
> + cxlm->regs.memdev = register_block;
> break;
> default:
> dev_dbg(dev, "Unknown cap ID: %d (0x%x)\n", cap_id, offset);
> @@ -907,11 +907,11 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> }
> }
>
> - if (!cxlm->status_regs || !cxlm->mbox_regs || !cxlm->memdev_regs) {
> + if (!cxlm->regs.status || !cxlm->regs.mbox || !cxlm->regs.memdev) {
> dev_err(dev, "registers not found: %s%s%s\n",
> - !cxlm->status_regs ? "status " : "",
> - !cxlm->mbox_regs ? "mbox " : "",
> - !cxlm->memdev_regs ? "memdev" : "");
> + !cxlm->regs.status ? "status " : "",
> + !cxlm->regs.mbox ? "mbox " : "",
> + !cxlm->regs.memdev ? "memdev" : "");
> return -ENXIO;
> }
>
> @@ -920,7 +920,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>
> static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
> {
> - const int cap = readl(cxlm->mbox_regs + CXLDEV_MBOX_CAPS_OFFSET);
> + const int cap = readl(cxlm->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
>
> cxlm->payload_size =
> 1 << FIELD_GET(CXLDEV_MBOX_CAP_PAYLOAD_SIZE_MASK, cap);
> @@ -980,7 +980,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>
> mutex_init(&cxlm->mbox_mutex);
> cxlm->pdev = pdev;
> - cxlm->regs = regs + offset;
> + cxlm->base = regs + offset;
> cxlm->enabled_cmds =
> devm_kmalloc_array(dev, BITS_TO_LONGS(cxl_cmd_count),
> sizeof(unsigned long),
> @@ -1495,6 +1495,10 @@ static __init int cxl_mem_init(void)
> dev_t devt;
> int rc;
>
> + /* Double check the anonymous union trickery in struct cxl_regs */
> + BUILD_BUG_ON(offsetof(struct cxl_regs, memdev) !=
> + offsetof(struct cxl_regs, device_regs.memdev));
> +
> rc = alloc_chrdev_region(&devt, 0, CXL_MEM_MAX_DEVS, "cxl");
> if (rc)
> return rc;
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index daa9aba0e218..c247cf9c71af 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -53,10 +53,9 @@ struct cxl_memdev {
> /**
> * struct cxl_mem - A CXL memory device
> * @pdev: The PCI device associated with this CXL device.
> - * @regs: IO mappings to the device's MMIO
> - * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers
> - * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers
> - * @memdev_regs: CXL 2.0 8.2.8.5 Memory Device Registers
> + * @base: IO mappings to the device's MMIO
> + * @cxlmd: Logical memory device chardev / interface
Unrelated missing docs fix?
> + * @regs: Parsed register blocks
> * @payload_size: Size of space for payload
> * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register)
> * @mbox_mutex: Mutex to synchronize mailbox access.
> @@ -67,12 +66,10 @@ struct cxl_memdev {
> */
> struct cxl_mem {
> struct pci_dev *pdev;
> - void __iomem *regs;
> + void __iomem *base;
Whilst I have no problem with the rename and fact you want to free it
up for other uses, perhaps call it out in the patch description?
> struct cxl_memdev *cxlmd;
>
> - void __iomem *status_regs;
> - void __iomem *mbox_regs;
> - void __iomem *memdev_regs;
> + struct cxl_regs regs;
>
> size_t payload_size;
> struct mutex mbox_mutex; /* Protects device mailbox and firmware */
>
