Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:34:05 -0800
> Dan Williams <dan.j.willi...@intel.com> wrote:
>
> > From: Robert Richter <rrich...@amd.com>
> >
> > A downstream port must be connected to a component register block.
> > For restricted hosts the base address is determined from the RCRB. The
> > RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> > get the RCRB and add code to extract the component register block from
> > it.
> >
> > RCRB's BAR[0..1] point to the component block containing CXL subsystem
> > component registers. MEMBAR extraction follows the PCI base spec here,
> > esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1). The
> > RCRB base address is cached in the cxl_dport per-host bridge so that the
> > upstream port component registers can be retrieved later by an RCD
> > (RCIEP) associated with the host bridge.
> >
> > Note: Right now the component register block is used for HDM decoder
> > capability only which is optional for RCDs. If unsupported by the RCD,
> > the HDM init will fail. It is future work to bypass it in this case.
> >
> > Co-developed-by: Terry Bowman <terry.bow...@amd.com>
> > Signed-off-by: Terry Bowman <terry.bow...@amd.com>
> > Signed-off-by: Robert Richter <rrich...@amd.com>
> > Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> > [djbw: introduce devm_cxl_add_rch_dport()]
> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
>
> Trivial moans that may have something to do with it being near going home time
> on a Friday.
>
> Otherwise looks sensible though this was a fairly superficial look.
>
> Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
>
>
>
> > ---
> > drivers/cxl/acpi.c | 51 ++++++++++++++++++++++++++++-----
> > drivers/cxl/core/port.c | 53 ++++++++++++++++++++++++++++++----
> > drivers/cxl/core/regs.c | 64
> > +++++++++++++++++++++++++++++++++++++++++
> > drivers/cxl/cxl.h | 16 ++++++++++
> > tools/testing/cxl/Kbuild | 1 +
> > tools/testing/cxl/test/cxl.c | 10 ++++++
> > tools/testing/cxl/test/mock.c | 19 ++++++++++++
> > tools/testing/cxl/test/mock.h | 3 ++
> > 8 files changed, 203 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 50d82376097c..db8173f3ee10 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
>
> > struct cxl_chbs_context {
> > - struct device *dev;
> > - unsigned long long uid;
> > - resource_size_t chbcr;
> > + struct device *dev;
> > + unsigned long long uid;
> > + resource_size_t rcrb;
> > + resource_size_t chbcr;
> > + u32 cxl_version;
> > };
>
> I'm not keen on this style change because it slightly obscures the meaningful
> changes in this diff + I suspect it's not consistent with rest of the file.
Copy and pasted from Robert's update. Looks much better after hitting it
with clang-format:
@@ -232,7 +239,9 @@ static int add_host_bridge_uport(struct device *match, void
*arg)
struct cxl_chbs_context {
struct device *dev;
unsigned long long uid;
+ resource_size_t rcrb;
resource_size_t chbcr;
+ u32 cxl_version;
};
static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>
>
>
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index ec178e69b18f..28ed0ec8ee3e 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -307,3 +307,67 @@ int cxl_find_regblock(struct pci_dev *pdev, enum
> > cxl_regloc_type type,
> > return -ENODEV;
> > }
> > EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> > +
> > +resource_size_t cxl_rcrb_to_component(struct device *dev,
> > + resource_size_t rcrb,
> > + enum cxl_rcrb which)
> > +{
> > + resource_size_t component_reg_phys;
> > + u32 bar0, bar1;
> > + void *addr;
> > + u16 cmd;
> > + u32 id;
> > +
> > + if (which == CXL_RCRB_UPSTREAM)
> > + rcrb += SZ_4K;
> > +
> > + /*
> > + * RCRB's BAR[0..1] point to component block containing CXL
> > + * subsystem component registers. MEMBAR extraction follows
> > + * the PCI Base spec here, esp. 64 bit extraction and memory
> > + * ranges alignment (6.0, 7.5.1.2.1).
> > + */
> > + if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> > + return CXL_RESOURCE_NONE;
> > + addr = ioremap(rcrb, SZ_4K);
> > + if (!addr) {
> > + dev_err(dev, "Failed to map region %pr\n", addr);
> > + release_mem_region(rcrb, SZ_4K);
> > + return CXL_RESOURCE_NONE;
> > + }
> > +
> > + id = readl(addr + PCI_VENDOR_ID);
> > + cmd = readw(addr + PCI_COMMAND);
> > + bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > + bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> > + iounmap(addr);
> > + release_mem_region(rcrb, SZ_4K);
> > +
> > + /*
> > + * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> > + * Remap Upstream Port and Component Registers
> > + */
> > + if (id == U32_MAX) {
> > + if (which == CXL_RCRB_DOWNSTREAM)
> > + dev_err(dev, "Failed to access Downstream Port RCRB\n");
> > + return CXL_RESOURCE_NONE;
> > + }
> > + if (!(cmd & PCI_COMMAND_MEMORY))
> > + return CXL_RESOURCE_NONE;
> > + if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
>
> Trivial: A positive match on what we do want might be better...
>
> I had to got look up MEM_TYPE_1M to find out what on earth it was (marked
> obsolete which
> I guess isn't surprising.... )
>
> Up to you though...
The polarity switch is not any prettier, but a comment would save
someone searching what PCI_BASE_ADDRESS_MEM_TYPE_1M is though. I
actually looked that up myself when I first read it.
>
> > + return CXL_RESOURCE_NONE;
> > +
> > + component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> > + if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> > + component_reg_phys |= ((u64)bar1) << 32;
> > +
> > + if (!component_reg_phys)
> > + return CXL_RESOURCE_NONE;
> > +
> > + /* MEMBAR is block size (64k) aligned. */
> > + if (!IS_ALIGNED(component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE))
> > + return CXL_RESOURCE_NONE;
> > +
> > + return component_reg_phys;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 281b1db5a271..1342e4e61537 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
>
>
>
> > #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> > #define CXL_TARGET_STRLEN 20
> >
> > @@ -486,12 +494,16 @@ cxl_find_dport_by_dev(struct cxl_port *port, const
> > struct device *dport_dev)
> > * @dport: PCI bridge or firmware device representing the downstream link
> > * @port_id: unique hardware identifier for dport in decoder target list
> > * @component_reg_phys: downstream port component registers
> > + * @rcrb: base address for the Root Complex Register Block
> > + * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>
> Clarify this as
> Indicate this dport was enumerated in RCH rather than VH mode.
>
> a boolean with an or in the comment is confusing!
>
> > * @port: reference to cxl_port that contains this downstream port
> > */
> > struct cxl_dport {
> > struct device *dport;
> > int port_id;
> > resource_size_t component_reg_phys;
> > + resource_size_t rcrb;
> > + bool rch;
> > struct cxl_port *port;
> > };
>
