From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
Alison Schofield <alison.schofield@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/5] cxl/mem: Map registers based on capabilities
Date: Thu, 27 May 2021 10:53:05 -0700 [thread overview]
Message-ID: <20210527175305.GF3697498@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20210525105214.00005e54@Huawei.com>
On Tue, May 25, 2021 at 10:52:14AM +0100, Jonathan Cameron wrote:
> On Fri, 21 May 2021 17:11:52 -0700
> <ira.weiny@intel.com> wrote:
>
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > The information required to map registers based on capabilities is
> > contained within the bars themselves. This means the bar must be mapped
> > to read the information needed and then unmapped to map the individual
> > parts of the BAR based on capabilities.
> >
> > Change cxl_setup_device_regs() to return a new cxl_register_map, change
> > the name to cxl_probe_device_regs(). Allocate and place
> > cxl_register_maps on a list while processing all of the specified
> > register blocks.
> >
> > After probing all the register blocks go back and map smaller registers
> > blocks based on their capabilities and dispose of the cxl_register_maps.
> >
> > NOTE: pci_iomap() is not managed automatically via pcim_enable_device()
> > so be careful to call pci_iounmap() correctly.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> A couple of really minor queries inline, but otherwise looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Thanks!
>
> >
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 38979c97158d..add66a6ec875 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -3,6 +3,7 @@
> > #include <linux/io-64-nonatomic-lo-hi.h>
> > #include <linux/device.h>
> > #include <linux/module.h>
> > +#include <linux/pci.h>
> > #include "cxl.h"
> >
> > /**
> > @@ -12,19 +13,13 @@
> > * point for cross-device interleave coordination through cxl ports.
> > */
> >
> > -/**
> > - * cxl_setup_device_regs() - Detect CXL Device register blocks
> > - * @dev: Host device of the @base mapping
> > - * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
> > - * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS())
> > - */
>
> Nice to keep docs given this is an exported function.
I can write something better but the above does not add much IMO. The
parameter explanations are unnecessary IMO.
> >
> > @@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> > dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
> > bar, offset, reg_type);
> >
> > - if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > - base = cxl_mem_map_regblock(cxlm, bar, offset);
> > - if (!base)
> > - return -ENOMEM;
> > - break;
> > + base = cxl_mem_map_regblock(cxlm, bar, offset);
> > + if (!base) {
> > + ret = -ENOMEM;
> > + goto free_maps;
> > }
> > - }
> >
> > - if (i == regblocks) {
> > - dev_err(dev, "Missing register locator for device registers\n");
> > - return -ENXIO;
>
> Do we have or need an equivalent of this check somewhere else?
Yes agreed! I moved the check to cxl_probe_regs which returns -ENXIO if the
register sets expected are not found. A check is also added to RBI_COMPONENT
register type in the following patch. This was moved mainly because each
register type is going to know better what it needs for proper operation.
cxl_probe_device_regs() really can't know that after this series.
Ira
next prev parent reply other threads:[~2021-05-27 17:53 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-22 0:11 [PATCH v2 0/5] Map register blocks individually ira.weiny
2021-05-22 0:11 ` [PATCH v2 1/5] cxl/mem: Introduce cxl_decode_register_block() ira.weiny
2021-05-25 9:53 ` Jonathan Cameron
2021-05-22 0:11 ` [PATCH v2 2/5] cxl/mem: Reserve all device regions at once ira.weiny
2021-05-25 9:54 ` Jonathan Cameron
2021-05-22 0:11 ` [PATCH v2 3/5] cxl/mem: Map registers based on capabilities ira.weiny
2021-05-25 9:52 ` Jonathan Cameron
2021-05-27 17:53 ` Ira Weiny [this message]
2021-05-22 0:11 ` [PATCH v2 4/5] cxl/mem: Reserve individual register block regions ira.weiny
2021-05-25 9:59 ` Jonathan Cameron
2021-05-22 0:11 ` [PATCH v2 5/5] cxl: Add HDM decoder capbilities ira.weiny
2021-05-25 14:28 ` Jonathan Cameron
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=20210527175305.GF3697498@iweiny-DESK2.sc.intel.com \
--to=ira.weiny@intel.com \
--cc=Jonathan.Cameron@Huawei.com \
--cc=alison.schofield@intel.com \
--cc=ben.widawsky@intel.com \
--cc=dan.j.williams@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).