Date:   Thu, 1 Sep 2022 09:38:13 +0200
From:   Robert Richter <rrichter@....com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Terry Bowman <terry.bowman@....com>
Subject: Re: [PATCH 11/15] cxl/acpi: Extract the host's component register
 base address from RCRB

On 31.08.22 12:56:56, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:59 +0200
> Robert Richter <rrichter@....com> wrote:

> A few comments inline.  Mostly requests for references for things
> I couldn't find in the specs.

Most of it comes from the pci base spec (5 or 6).

> 
> > ---
> >  drivers/cxl/acpi.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 439df9df2741..88bbd2bb61fc 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -401,12 +401,84 @@ static resource_size_t cxl_get_rcrb(u32 uid)
> >  	return ctx.chbcr;
> >  }
> >  
> > +static resource_size_t cxl_get_component_reg_phys(resource_size_t rcrb)
> > +{
> > +	resource_size_t component_reg_phys;
> > +	u32 bar0, bar1;
> > +	void *addr;
> > +
> > +	/*
> > +	 * RCRB's BAR[0..1] point to component block containing CXL subsystem
> > +	 * component registers.
> > +	 * CXL 8.2.4 - Component Register Layout Definition.
> 
> For references include spec version.  Based on discussion other day,
> preference is always latest version. So r3.0 8.2.3
> is probably right. I think your references are CXL r2.0?

Right will update the comment.

> 
> 
> > +	 *
> > +	 * Also, RCRB accesses must use MMIO readl()/readq() to guarantee
> > +	 * 32/64-bit access.
> > +	 * CXL 8.2.2 - CXL 1.1 Upstream and Downstream Port Subsystem Component
> > +	 * Registers
> > +	 */
> > +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> > +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> 
> The spec is a bit confusing on this, but I think you are reading into
> MEMBAR0 of the RCRB, for which there isn't a lot of description other than
> it being an address. It's referred to as a 64-bit BAR in places so you
> might be right - or it might be intended to be a bare address..
> 
> We might want a clarification on this...
> 
> Also it's a 64 bit address so we need to read it in one go. However it's
> referred to as a being a 64 bit address at 0x10 and 0x14 so who knows...

This is part of the pci base spec and clearly defined there. There are
also some similar implementation in the kernel already.

> 
> 
> > +	iounmap(addr);
> > +
> > +	/* sanity check */
> > +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> > +		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;
> > +
> > +	/*
> > +	 * Must be 8k aligned (size of combined CXL 1.1 Downstream and
> > +	 * Upstream Port RCRBs).
> 
> The rcrb is 8k though I'm not immediately spotting an alignment requirement,
> but I'm not sure the component regs have any restrictions do... Add a reference perhaps?
> For non RCD devices there is a 64K alignment requirement, but I can't find
> anything for RCDs (might just be missing it).

This are the requirements of the pci base spec to membar ranges. The
range size is power of 2 and base must be aligned to its size.

> 
> > +	 */
> > +	if (component_reg_phys & (SZ_8K - 1))
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	return component_reg_phys;
> > +}
> > +
> > +static int cxl_setup_component_reg(struct device *parent,
> > +				   resource_size_t component_reg_phys)
> > +{
> > +	struct cxl_component_reg_map comp_map;
> > +	void __iomem *base;
> > +
> > +	if (component_reg_phys == CXL_RESOURCE_NONE)
> > +		return -EINVAL;
> > +
> > +	base = ioremap(component_reg_phys, SZ_64K);
> 
> Add a spec reference for the size. Table 8-21 perhaps?

Can add a comment here.

> 
> > +	if (!base) {
> > +		dev_err(parent, "failed to map registers\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	cxl_probe_component_regs(parent, base, &comp_map);
> > +	iounmap(base);
> > +
> > +	if (!comp_map.hdm_decoder.valid) {
> > +		dev_err(parent, "HDM decoder registers not found\n");
> > +		return -ENXIO;
> 
> Hmm. HDM decoder capability is optional for RCDs - might be using the
> range registers.  Seems like we'd really want to handle that for
> RCDs.  Future work I guess.

I used the same message as for the non-RCD code path. HDM decoding is
just a subset of features handled with component regs. We need to
generalize the code for this in the future.

> 
> > +	}
> > +
> > +	dev_dbg(parent, "Set up component registers\n");
> > +
> > +	return 0;
> > +}
> > +
> >  static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> >  {
> >  	struct pci_host_bridge *host = NULL;
> >  	struct acpi_device *adev;
> >  	unsigned long long uid = ~0;
> >  	resource_size_t rcrb;
> > +	resource_size_t component_reg_phys;
> Trivial: As before, if we can reduce scope to inside the while loop, slightly cleaner
> and more local.
> > +	int rc;
> >  
> >  	while ((host = cxl_find_next_rch(host)) != NULL) {
> >  		adev = ACPI_COMPANION(&host->dev);
> > @@ -425,10 +497,18 @@ static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> >  
> >  		dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)rcrb);
> >  
> > +		component_reg_phys = cxl_get_component_reg_phys(rcrb);
> > +		rc = cxl_setup_component_reg(&host->dev, component_reg_phys);
> 
> Perhaps rename to make it clear this is getting the DSP component registers.
> 
> Future work would be to add support for the CXL 3.0 feature that lets even an
> RCD just put some of these registers in a BAR as per CXL 2.0 devices.

Yes, this is left out atm.

> 
> > +		if (rc)
> > +			goto fail;
> 
> > +
> >  		dev_info(&host->dev, "host supports CXL\n");
> >  	}
> >  
> >  	return 0;
> > +fail:
> 
> Better to have a more specific error message and return directly above.
> Note that so far vast majority of CXL error messages are dev_dbg,
> so for consistency perhaps this should be as well.
> (I prefer dev_err() but not my subsystem ;)

There is more verbosity on the errors with dbg enabled. Note there are
only a few dev_info/dev_err messages to not polute the logs. dev_err()
is only used if something unexpected happens (e.g. the device exists
but component regs are broken).

> 
> > +	dev_err(&host->dev, "failed to initialize CXL host: %d\n", rc);
> dev_err_probe() is slightly nicer to use if things can only happen in
> probe() paths.

Will consider that.

-Robert

> 
> > +	return rc;
> >  }
> >  
> >  static struct lock_class_key cxl_root_key;
> 

Powered by blists - more mailing lists