Linux-CXL Archive on lore.kernel.org
 help / color / Atom feed
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
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.

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01 14:30 [PATCH v2 0/8] CXL Port Enumeration Dan Williams
2021-04-01 14:30 ` [PATCH v2 1/8] cxl/mem: Move some definitions to mem.h Dan Williams
2021-04-06 16:38   ` Jonathan Cameron
2021-04-14  0:18     ` Dan Williams
2021-04-14  0:42       ` Dan Williams
2021-04-14  9:21         ` Jonathan Cameron
2021-04-01 14:30 ` [PATCH v2 2/8] cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices Dan Williams
2021-04-06 17:00   ` Jonathan Cameron
2021-04-14  0:40     ` Dan Williams [this message]
2021-04-01 14:30 ` [PATCH v2 3/8] cxl/core: Rename bus.c to core.c Dan Williams
2021-04-01 14:31 ` [PATCH v2 4/8] cxl/core: Refactor CXL register lookup for bridge reuse Dan Williams
2021-04-06 17:00   ` Jonathan Cameron
2021-04-15 20:53     ` Dan Williams
2021-04-01 14:31 ` [PATCH v2 5/8] cxl/acpi: Introduce ACPI0017 driver and cxl_root Dan Williams
2021-04-01 21:34   ` kernel test robot
2021-04-06 17:32   ` Jonathan Cameron
2021-04-15 15:00     ` Dan Williams
2021-04-01 14:31 ` [PATCH v2 6/8] cxl/Kconfig: Default drivers to CONFIG_CXL_BUS Dan Williams
2021-04-01 14:31 ` [PATCH v2 7/8] cxl/port: Introduce cxl_port objects Dan Williams
2021-04-06 17:44   ` Jonathan Cameron
2021-04-08 22:42   ` Bjorn Helgaas
2021-04-09  2:13     ` Dan Williams
2021-04-13 17:18       ` Dan Williams
2021-04-14  1:14       ` Bjorn Helgaas
2021-04-15  5:21         ` Dan Williams
2021-04-01 14:31 ` [PATCH v2 8/8] cxl/acpi: Add module parameters to stand in for ACPI tables Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPcyv4gJx+xt0sirQ3LUz2nJ_6ybek7EzL0E9QkjdYZwqN2vzg@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-CXL Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cxl/0 linux-cxl/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cxl linux-cxl/ https://lore.kernel.org/linux-cxl \
		linux-cxl@vger.kernel.org
	public-inbox-index linux-cxl

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cxl


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git