[v2,06/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

  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).

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.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 10 deletions(-)
  

On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
>
> 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).
>
> 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.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>

What does this S-o-B mean?  If this person is your co-developer, you
need to add a Co-developed-by tag to clarify that.

> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 69 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..a92d5d7b7a92 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -9,6 +9,8 @@
>  #include "cxlpci.h"
>  #include "cxl.h"
>
> +#define CXL_RCRB_SIZE  SZ_8K
> +
>  static unsigned long cfmws_to_decoder_flags(int restrictions)
>  {
>         unsigned long flags = CXL_DECODER_F_ENABLE;
> @@ -229,27 +231,82 @@ 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 chbcr;
> +       struct acpi_cedt_chbs chbs;
>  };
>
> -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> -                        const unsigned long end)
> +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> +                       const unsigned long end)
>  {
>         struct cxl_chbs_context *ctx = arg;
>         struct acpi_cedt_chbs *chbs;
>
> -       if (ctx->chbcr)
> +       if (ctx->chbs.base)
>                 return 0;
>
>         chbs = (struct acpi_cedt_chbs *) header;
>
>         if (ctx->uid != chbs->uid)
>                 return 0;
> -       ctx->chbcr = chbs->base;
> +       ctx->chbs = *chbs;
>
>         return 0;
>  }
>
> +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> +{
> +       struct acpi_cedt_chbs *chbs = &ctx->chbs;
> +       resource_size_t component_reg_phys, rcrb;
> +       u32 bar0, bar1;
> +       void *addr;
> +
> +       if (!chbs->base)
> +               return CXL_RESOURCE_NONE;
> +
> +       if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> +               return chbs->base;
> +
> +       /* Extract RCRB */
> +
> +       if (chbs->length != CXL_RCRB_SIZE)
> +               return CXL_RESOURCE_NONE;
> +
> +       rcrb = chbs->base;
> +
> +       dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> +               ctx->uid, (u64)rcrb);
> +
> +       /*
> +        * 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).
> +        */
> +       addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> +       bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +       bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +       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).
> +        */
> +       if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> +               return CXL_RESOURCE_NONE;
> +
> +       return component_reg_phys;
> +}
> +
>  static int add_host_bridge_dport(struct device *match, void *arg)
>  {
>         acpi_status status;
> @@ -259,6 +316,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>         struct cxl_port *root_port = arg;
>         struct device *host = root_port->dev.parent;
>         struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> +       resource_size_t component_reg_phys;
>
>         if (!bridge)
>                 return 0;
> @@ -273,19 +331,20 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>         dev_dbg(match, "UID found: %lld\n", uid);
>
>         ctx = (struct cxl_chbs_context) {
> -               .dev = host,
> +               .dev = match,
>                 .uid = uid,
>         };
> -       acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx);
> +       acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx);
>
> -       if (ctx.chbcr == 0) {
> +       component_reg_phys = cxl_get_chbcr(&ctx);
> +       if (component_reg_phys == CXL_RESOURCE_NONE) {
>                 dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid);
>                 return 0;
>         }
>
> -       dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)ctx.chbcr);
> +       dev_dbg(match, "CHBCR found: 0x%08llx\n", (u64)component_reg_phys);
>
> -       dport = devm_cxl_add_dport(root_port, match, uid, ctx.chbcr);
> +       dport = devm_cxl_add_dport(root_port, match, uid, component_reg_phys);
>         if (IS_ERR(dport))
>                 return PTR_ERR(dport);
>
> --
> 2.30.2
>
  

On 18.10.22 15:31:16, Rafael J. Wysocki wrote:
> On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
> >
> > 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).
> >
> > 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.
> >
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> 
> What does this S-o-B mean?  If this person is your co-developer, you
> need to add a Co-developed-by tag to clarify that.
> 
> > Signed-off-by: Robert Richter <rrichter@amd.com>

I picked up an early patch and modified it significantly, so I just
left the S-o-B. I could change this to a Co-developed-by tag. IMO, the
S-o-B is ok, but could be wrong here.

-Robert
  

On Tue, Oct 18, 2022 at 8:42 PM Robert Richter <rrichter@amd.com> wrote:
>
> On 18.10.22 15:31:16, Rafael J. Wysocki wrote:
> > On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
> > >
> > > 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).
> > >
> > > 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.
> > >
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> >
> > What does this S-o-B mean?  If this person is your co-developer, you
> > need to add a Co-developed-by tag to clarify that.
> >
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
>
> I picked up an early patch and modified it significantly, so I just
> left the S-o-B.

In that case the right thing to do is to mention the original author
in the changelog instead of retaining the S-o-b.

> I could change this to a Co-developed-by tag.

Co-developed-by should be used in addition to and not instead of S-o-b
when one of the authors is sending a patch.  However, all of the
authors need to be familiar with the patch in the form in which it is
being sent then.

> IMO, the S-o-B is ok, but could be wrong here.

It isn't, at least not without a Co-developed-by tag.

There are 3 cases in which S-o-b is OK AFAICS:

1. When it matches the From: address.
2. When there is a matching Co-developed-by.
3. When maintainers pick up patches and add their own S-o-b.

This case is none of the above.
  

On 18.10.22 20:57:02, Rafael J. Wysocki wrote:
> On Tue, Oct 18, 2022 at 8:42 PM Robert Richter <rrichter@amd.com> wrote:
> >
> > On 18.10.22 15:31:16, Rafael J. Wysocki wrote:
> > > On Tue, Oct 18, 2022 at 3:24 PM Robert Richter <rrichter@amd.com> wrote:
> > > >
> > > > 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).
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > >
> > > What does this S-o-B mean?  If this person is your co-developer, you
> > > need to add a Co-developed-by tag to clarify that.
> > >
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> >
> > I picked up an early patch and modified it significantly, so I just
> > left the S-o-B.
> 
> In that case the right thing to do is to mention the original author
> in the changelog instead of retaining the S-o-b.
> 
> > I could change this to a Co-developed-by tag.
> 
> Co-developed-by should be used in addition to and not instead of S-o-b
> when one of the authors is sending a patch.  However, all of the
> authors need to be familiar with the patch in the form in which it is
> being sent then.
> 
> > IMO, the S-o-B is ok, but could be wrong here.
> 
> It isn't, at least not without a Co-developed-by tag.
> 
> There are 3 cases in which S-o-b is OK AFAICS:
> 
> 1. When it matches the From: address.
> 2. When there is a matching Co-developed-by.
> 3. When maintainers pick up patches and add their own S-o-b.
> 
> This case is none of the above.

Will add a Co-developed-by tag in my next version. Thanks for pointing
that out.

-Robert
  

Robert Richter wrote:
> 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).
> 
> 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.
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..a92d5d7b7a92 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -9,6 +9,8 @@
>  #include "cxlpci.h"
>  #include "cxl.h"
>  
> +#define CXL_RCRB_SIZE	SZ_8K
> +
>  static unsigned long cfmws_to_decoder_flags(int restrictions)
>  {
>  	unsigned long flags = CXL_DECODER_F_ENABLE;
> @@ -229,27 +231,82 @@ 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 chbcr;
> +	struct acpi_cedt_chbs chbs;
>  };
>  
> -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> -			 const unsigned long end)
> +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> +			const unsigned long end)
>  {
>  	struct cxl_chbs_context *ctx = arg;
>  	struct acpi_cedt_chbs *chbs;
>  
> -	if (ctx->chbcr)
> +	if (ctx->chbs.base)
>  		return 0;
>  
>  	chbs = (struct acpi_cedt_chbs *) header;
>  
>  	if (ctx->uid != chbs->uid)
>  		return 0;
> -	ctx->chbcr = chbs->base;
> +	ctx->chbs = *chbs;
>  
>  	return 0;
>  }
>  
> +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> +{

The core logic of this looks good, but this wants to be shared with the
upstream port component register discovery.

Full disclosure I am reconciling these patches with an attempt that Dave
Jiang made at this topic. Since your series hit the list first I will
let it take the lead, but then fill it in with comments and learnings
from Dave's effort.

So in this case Dave moved this into the drivers/cxl/core/regs.c with a
function signature like:

enum cxl_rcrb {
       CXL_RCRB_DOWNSTREAM,
       CXL_RCRB_UPSTREAM,
};

resource_size_t cxl_rcrb_to_component(struct device *dev,
                                      resource_size_t rcrb_base, int len,
                                      enum cxl_rcrb which);

...where @which alternates when called by cxl_acpi for the downstream
case, or cxl_mem for the upstream case.


> +	struct acpi_cedt_chbs *chbs = &ctx->chbs;
> +	resource_size_t component_reg_phys, rcrb;
> +	u32 bar0, bar1;
> +	void *addr;
> +
> +	if (!chbs->base)
> +		return CXL_RESOURCE_NONE;
> +
> +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> +		return chbs->base;
> +
> +	/* Extract RCRB */
> +
> +	if (chbs->length != CXL_RCRB_SIZE)
> +		return CXL_RESOURCE_NONE;
> +
> +	rcrb = chbs->base;
> +
> +	dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> +		ctx->uid, (u64)rcrb);
> +
> +	/*
> +	 * 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).
> +	 */
> +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);

No failure check? This also only needs to map 4K at a time.

> +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +	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).
> +	 */
> +	if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> +		return CXL_RESOURCE_NONE;

This is open-coding the IS_ALIGNED() macro. More importantly, why is it
using RCRB size for the component register block alignment? The
component lock is 64K, and at least for CXL 2.0 devices it is 64K
aligned (8.1.9.1 Register Block Offset Low), so I am not sure what this
check is for?

---

Given that there are actual CXL RCH platforms in the wild I want this
topic branch to be the first thing queued for v6.2. To help us
coordinate I pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=rch

...with the patches from this set accepted so far. You can use that as
the baseline for the next spin.
  

On 20.10.22 22:17:07, Dan Williams wrote:
> Robert Richter wrote:
> > 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).
> > 
> > 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.
> > 
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 69 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index fb9f72813067..a92d5d7b7a92 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -9,6 +9,8 @@
> >  #include "cxlpci.h"
> >  #include "cxl.h"
> >  
> > +#define CXL_RCRB_SIZE	SZ_8K
> > +
> >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> >  {
> >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > @@ -229,27 +231,82 @@ 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 chbcr;
> > +	struct acpi_cedt_chbs chbs;
> >  };
> >  
> > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > -			 const unsigned long end)
> > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > +			const unsigned long end)
> >  {
> >  	struct cxl_chbs_context *ctx = arg;
> >  	struct acpi_cedt_chbs *chbs;
> >  
> > -	if (ctx->chbcr)
> > +	if (ctx->chbs.base)
> >  		return 0;
> >  
> >  	chbs = (struct acpi_cedt_chbs *) header;
> >  
> >  	if (ctx->uid != chbs->uid)
> >  		return 0;
> > -	ctx->chbcr = chbs->base;
> > +	ctx->chbs = *chbs;
> >  
> >  	return 0;
> >  }
> >  
> > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > +{
> 
> The core logic of this looks good, but this wants to be shared with the
> upstream port component register discovery.
> 
> Full disclosure I am reconciling these patches with an attempt that Dave
> Jiang made at this topic. Since your series hit the list first I will
> let it take the lead, but then fill it in with comments and learnings
> from Dave's effort.
> 
> So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> function signature like:
> 
> enum cxl_rcrb {
>        CXL_RCRB_DOWNSTREAM,
>        CXL_RCRB_UPSTREAM,
> };
> 
> resource_size_t cxl_rcrb_to_component(struct device *dev,
>                                       resource_size_t rcrb_base, int len,
>                                       enum cxl_rcrb which);
> 
> ...where @which alternates when called by cxl_acpi for the downstream
> case, or cxl_mem for the upstream case.

Ok, I see where to go here. Could you point me to Dave's postings you
are referring to? I checked linux-cxl and could not find anything
related to RCRB or that changes regs.c.

> 
> 
> > +	struct acpi_cedt_chbs *chbs = &ctx->chbs;
> > +	resource_size_t component_reg_phys, rcrb;
> > +	u32 bar0, bar1;
> > +	void *addr;
> > +
> > +	if (!chbs->base)
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> > +		return chbs->base;
> > +
> > +	/* Extract RCRB */
> > +
> > +	if (chbs->length != CXL_RCRB_SIZE)
> > +		return CXL_RESOURCE_NONE;
> > +
> > +	rcrb = chbs->base;
> > +
> > +	dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> > +		ctx->uid, (u64)rcrb);
> > +
> > +	/*
> > +	 * 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).
> > +	 */
> > +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> 
> No failure check? This also only needs to map 4K at a time.

Right, will add that.

> 
> > +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> > +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> > +	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).
> > +	 */
> > +	if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> > +		return CXL_RESOURCE_NONE;
> 
> This is open-coding the IS_ALIGNED() macro. More importantly, why is it
> using RCRB size for the component register block alignment? The
> component lock is 64K, and at least for CXL 2.0 devices it is 64K
> aligned (8.1.9.1 Register Block Offset Low), so I am not sure what this
> check is for?

True, this is a mistake and needs to be corrected. It is the component
reg range which is 64k. Will also use IS_ALIGNED().

> 
> ---
> 
> Given that there are actual CXL RCH platforms in the wild I want this
> topic branch to be the first thing queued for v6.2. To help us
> coordinate I pushed:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=rch
> 
> ...with the patches from this set accepted so far. You can use that as
> the baseline for the next spin.

Yes, thanks for that branch and applying the first part.

-Robert
  

Robert Richter wrote:
> On 20.10.22 22:17:07, Dan Williams wrote:
> > Robert Richter wrote:
> > > 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).
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > ---
> > >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 69 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index fb9f72813067..a92d5d7b7a92 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -9,6 +9,8 @@
> > >  #include "cxlpci.h"
> > >  #include "cxl.h"
> > >  
> > > +#define CXL_RCRB_SIZE	SZ_8K
> > > +
> > >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> > >  {
> > >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > > @@ -229,27 +231,82 @@ 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 chbcr;
> > > +	struct acpi_cedt_chbs chbs;
> > >  };
> > >  
> > > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > > -			 const unsigned long end)
> > > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > > +			const unsigned long end)
> > >  {
> > >  	struct cxl_chbs_context *ctx = arg;
> > >  	struct acpi_cedt_chbs *chbs;
> > >  
> > > -	if (ctx->chbcr)
> > > +	if (ctx->chbs.base)
> > >  		return 0;
> > >  
> > >  	chbs = (struct acpi_cedt_chbs *) header;
> > >  
> > >  	if (ctx->uid != chbs->uid)
> > >  		return 0;
> > > -	ctx->chbcr = chbs->base;
> > > +	ctx->chbs = *chbs;
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > > +{
> > 
> > The core logic of this looks good, but this wants to be shared with the
> > upstream port component register discovery.
> > 
> > Full disclosure I am reconciling these patches with an attempt that Dave
> > Jiang made at this topic. Since your series hit the list first I will
> > let it take the lead, but then fill it in with comments and learnings
> > from Dave's effort.
> > 
> > So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> > function signature like:
> > 
> > enum cxl_rcrb {
> >        CXL_RCRB_DOWNSTREAM,
> >        CXL_RCRB_UPSTREAM,
> > };
> > 
> > resource_size_t cxl_rcrb_to_component(struct device *dev,
> >                                       resource_size_t rcrb_base, int len,
> >                                       enum cxl_rcrb which);
> > 
> > ...where @which alternates when called by cxl_acpi for the downstream
> > case, or cxl_mem for the upstream case.
> 
> Ok, I see where to go here. Could you point me to Dave's postings you
> are referring to? I checked linux-cxl and could not find anything
> related to RCRB or that changes regs.c.

He was in the middle of tidying them when you posted your series, but I
think it would not hurt to push them to a git tree so you can grab the
bits and pieces you want.

Dave?
  

Dan Williams wrote:
> Robert Richter wrote:
> > On 20.10.22 22:17:07, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > 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).
> > > > 
> > > > 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.
> > > > 
> > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > ---
> > > >  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 69 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > > index fb9f72813067..a92d5d7b7a92 100644
> > > > --- a/drivers/cxl/acpi.c
> > > > +++ b/drivers/cxl/acpi.c
> > > > @@ -9,6 +9,8 @@
> > > >  #include "cxlpci.h"
> > > >  #include "cxl.h"
> > > >  
> > > > +#define CXL_RCRB_SIZE	SZ_8K
> > > > +
> > > >  static unsigned long cfmws_to_decoder_flags(int restrictions)
> > > >  {
> > > >  	unsigned long flags = CXL_DECODER_F_ENABLE;
> > > > @@ -229,27 +231,82 @@ 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 chbcr;
> > > > +	struct acpi_cedt_chbs chbs;
> > > >  };
> > > >  
> > > > -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> > > > -			 const unsigned long end)
> > > > +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> > > > +			const unsigned long end)
> > > >  {
> > > >  	struct cxl_chbs_context *ctx = arg;
> > > >  	struct acpi_cedt_chbs *chbs;
> > > >  
> > > > -	if (ctx->chbcr)
> > > > +	if (ctx->chbs.base)
> > > >  		return 0;
> > > >  
> > > >  	chbs = (struct acpi_cedt_chbs *) header;
> > > >  
> > > >  	if (ctx->uid != chbs->uid)
> > > >  		return 0;
> > > > -	ctx->chbcr = chbs->base;
> > > > +	ctx->chbs = *chbs;
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> > > > +{
> > > 
> > > The core logic of this looks good, but this wants to be shared with the
> > > upstream port component register discovery.
> > > 
> > > Full disclosure I am reconciling these patches with an attempt that Dave
> > > Jiang made at this topic. Since your series hit the list first I will
> > > let it take the lead, but then fill it in with comments and learnings
> > > from Dave's effort.
> > > 
> > > So in this case Dave moved this into the drivers/cxl/core/regs.c with a
> > > function signature like:
> > > 
> > > enum cxl_rcrb {
> > >        CXL_RCRB_DOWNSTREAM,
> > >        CXL_RCRB_UPSTREAM,
> > > };
> > > 
> > > resource_size_t cxl_rcrb_to_component(struct device *dev,
> > >                                       resource_size_t rcrb_base, int len,
> > >                                       enum cxl_rcrb which);
> > > 
> > > ...where @which alternates when called by cxl_acpi for the downstream
> > > case, or cxl_mem for the upstream case.
> > 
> > Ok, I see where to go here. Could you point me to Dave's postings you
> > are referring to? I checked linux-cxl and could not find anything
> > related to RCRB or that changes regs.c.
> 
> He was in the middle of tidying them when you posted your series, but I
> think it would not hurt to push them to a git tree so you can grab the
> bits and pieces you want.
> 
> Dave?

Looks like the list delivery is backed up, so I added Dave to the Cc:.

He pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch

...which was his original attempt and:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert

...which was an attempt to rebase on top of your bits.

The common RCRB mapping function is here:

https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49

I think the path forward is to build on that common RCRB code, fix
cxl_acpi to register the pci host bridge device instead of the APCI
device as the dport device, and then rely on a flag to skip over
devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
directly in the RCIEP / RCH case. Then we can figure out what to do
about RCDs that choose not to implement the HDM decoder capability which
was forbidden in CXL 2.0, but now allowed in CXL 3.0.
  

On 24.10.22 16:23:39, Dave Jiang wrote:
> On 10/24/2022 3:37 PM, Dan Williams wrote:
>     Dan Williams wrote:
>         Robert Richter wrote:

>             Ok, I see where to go here. Could you point me to Dave's postings you
>             are referring to? I checked linux-cxl and could not find anything
>             related to RCRB or that changes regs.c.
> 
>         He was in the middle of tidying them when you posted your series, but I
>         think it would not hurt to push them to a git tree so you can grab the
>         bits and pieces you want.
> 
>         Dave?
> 
>     Looks like the list delivery is backed up, so I added Dave to the Cc:.
> 
>     He pushed:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch
> 
>     ...which was his original attempt and:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert
> 
>     ...which was an attempt to rebase on top of your bits.
> 
>     The common RCRB mapping function is here:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49

Thanks for the pointers.

> 
>     I think the path forward is to build on that common RCRB code, fix
>     cxl_acpi to register the pci host bridge device instead of the APCI
>     device as the dport device, and then rely on a flag to skip over
>     devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
>     directly in the RCIEP / RCH case.

Yes, we can completely skip devm_enumerate_cxl_ports() now. Though, I
am not convinced on using the pci host bridge as dport_dev as RCD and
non-RCD mode will diverge too much then. Looking into details here.

>     Then we can figure out what to do
>     about RCDs that choose not to implement the HDM decoder capability which
>     was forbidden in CXL 2.0, but now allowed in CXL 3.0.
> 
> Hi Robert. As follow on to your work, I'm also working on reworking the hdm
> decoder enumeration. I have this table from Dan where rr - range register exist
> but not setup, RR - range register exist and setup, hdm - HDM decoder exist but
> not programmed, HDM - HDM decoders exist and programmed. And I'm trying to
> refactor the current code to cover all those scenarios:
> 
>          rr             RR               rr hdm          rr HDM          RR hdm         RR HDM
> D2       unsupported    emulate RR       enable HDM      keep HDM        enable HDM     keep HDM
> D1       unsupported    emulate RR       enable HDM      keep HDM        enable HDM     keep HDM
> 
> The current test device I have that's attached to RCH host, I'm seeing the RR
> has setup a single range, but none of the HDM decoders are programmed.
> 

Right, HDM decoder init need to be changed next.

Thanks,

-Robert
  

Robert Richter wrote:
> On 24.10.22 16:23:39, Dave Jiang wrote:
> > On 10/24/2022 3:37 PM, Dan Williams wrote:
> >     Dan Williams wrote:
> >         Robert Richter wrote:
> 
> >             Ok, I see where to go here. Could you point me to Dave's postings you
> >             are referring to? I checked linux-cxl and could not find anything
> >             related to RCRB or that changes regs.c.
> > 
> >         He was in the middle of tidying them when you posted your series, but I
> >         think it would not hurt to push them to a git tree so you can grab the
> >         bits and pieces you want.
> > 
> >         Dave?
> > 
> >     Looks like the list delivery is backed up, so I added Dave to the Cc:.
> > 
> >     He pushed:
> > 
> >     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch
> > 
> >     ...which was his original attempt and:
> > 
> >     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-rch-robert
> > 
> >     ...which was an attempt to rebase on top of your bits.
> > 
> >     The common RCRB mapping function is here:
> > 
> >     https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl-rch-robert&id=5be44cad37972517dae6a79001080ccfbdb67c49
> 
> Thanks for the pointers.
> 
> > 
> >     I think the path forward is to build on that common RCRB code, fix
> >     cxl_acpi to register the pci host bridge device instead of the APCI
> >     device as the dport device, and then rely on a flag to skip over
> >     devm_enumerate_cxl_ports() in favor of just calling cxl_mem_find_port()
> >     directly in the RCIEP / RCH case.
> 
> Yes, we can completely skip devm_enumerate_cxl_ports() now. Though, I
> am not convinced on using the pci host bridge as dport_dev as RCD and
> non-RCD mode will diverge too much then. Looking into details here.

Oh, I disagree with the initial implementation Dave had here. Both cases
should be specifying the bridge device as the dport. That's a fixup that
can go in now even without the RCD support.

As it is the tooling needs to jump through the physical_node attribute
to provide the useful information in cxl list:

# cxl list -BTu -b ACPI.CXL
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"ACPI0016:00",
      "alias":"pci0000:34",
      "id":"0x34"
    }
  ]
}

...and I think that should just swap to this in all cases:

# cxl list -BTu -b ACPI.CXL
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"pci0000:34",
      "alias":"ACPI0016:00",
      "id":"0x34"
    }
  ]
}