On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> 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.
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.
