Re: [PATCH v2 2/8] cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: linux-cxl@vger.kernel.org, Linux PCI <linux-pci@vger.kernel.org>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	"Weiny, Ira" <ira.weiny@intel.com>,
	Vishal L Verma <vishal.l.verma@intel.com>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	Ben Widawsky <ben.widawsky@intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/8] cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices
Date: Tue, 13 Apr 2021 17:40:16 -0700	[thread overview]
Message-ID: <CAPcyv4gJx+xt0sirQ3LUz2nJ_6ybek7EzL0E9QkjdYZwqN2vzg@mail.gmail.com> (raw)
In-Reply-To: <20210406180037.00000474@Huawei.com>

On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 1 Apr 2021 07:30:53 -0700
> Dan Williams <dan.j.williams@intel.com> 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@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> 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.

Sure, I'll at least check that the tool does not complain, I might
just make this not a kernel-doc and change the /** to plain /*.

[..]
> > 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?

Yeah, I'll declare that in the changelog.

>
> > + * @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?

Sure.