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)
+{
+ 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.2Robert 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 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
© 2016 - 2026 Red Hat, Inc.