lkml.org 
[lkml]   [2023]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 07/23] cxl/acpi: Directly bind the CEDT detected CHBCR to the Host Bridge's port
    On Tue, 23 May 2023 18:21:58 -0500
    Terry Bowman <terry.bowman@amd.com> wrote:

    > From: Robert Richter <rrichter@amd.com>
    >
    > During a Host Bridge's downstream port enumeration the CHBS entries in
    > the CEDT table are parsed, its Component Register base address
    > extracted and then stored in struct cxl_dport. The CHBS may contain
    > either the RCRB (RCH mode) or the Host Bridge's Component Registers
    > (CHBCR, VH mode). The RCRB further contains the CXL downstream port
    > register base address, while in VH mode the CXL Downstream Switch
    > Ports are visible in the PCI hierarchy and the DP's component regs are
    > disovered using the CXL DVSEC register locator capability. The
    > Component Registers derived from the CHBS for both modes are different
    > and thus also must be treated differently. That is, in RCH mode, the
    > component regs base should be bound to the dport, but in VH mode to
    > the CXL host bridge's port object.
    >
    > The current implementation stores the CHBCR in addition in struct
    > cxl_dport and copies it later from there to struct cxl_port. As a
    > result, the dport contains the wrong Component Registers base address
    > and, e.g. the RAS capability of a CXL Root Port cannot be detected.
    >
    > To fix the CHBCR binding, attach it directly to the Host Bridge's
    > @cxl_port structure. Do this during port creation of the Host Bridge
    > in add_host_bridge_uport(). Factor out CHBS parsing code in
    > add_host_bridge_dport() and use it in both functions.
    >
    > Signed-off-by: Robert Richter <rrichter@amd.com>
    > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
    A few trivial formatting things. With those tidied up or
    reason given for why not,

    Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

    > ---
    > drivers/cxl/acpi.c | 65 +++++++++++++++++++++++++++++++++++-----------
    > 1 file changed, 50 insertions(+), 15 deletions(-)
    >
    > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
    > index 4fd9fe32f830..78a24b2ca923 100644
    > --- a/drivers/cxl/acpi.c
    > +++ b/drivers/cxl/acpi.c
    > @@ -333,8 +333,8 @@ struct cxl_chbs_context {
    > u32 cxl_version;
    > };
    >
    > -static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
    > - const unsigned long end)
    > +static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
    > + const unsigned long end)
    > {
    > struct cxl_chbs_context *ctx = arg;
    > struct acpi_cedt_chbs *chbs;
    > @@ -362,6 +362,22 @@ static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
    > return 0;
    > }
    >
    > +static int cxl_get_chbs(struct acpi_device *hb, struct cxl_chbs_context *ctx)
    > +{
    > + unsigned long long uid;
    > + int rc;
    > +
    > + rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
    > + if (rc != AE_OK)
    > + return -ENOENT;
    > +
    > + memset(ctx, 0, sizeof(*ctx));
    > + ctx->uid = uid;

    For consistency with original code better to use

    *ctx = (struct cxl_chbs_context) {
    .uid = uid,
    };

    > + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx);
    > +
    > + return 0;
    > +}
    > +
    > static int add_host_bridge_dport(struct device *match, void *arg)
    > {
    > acpi_status rc;
    > @@ -377,19 +393,15 @@ static int add_host_bridge_dport(struct device *match, void *arg)
    > if (!hb)
    > return 0;
    >
    > - rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
    > - if (rc != AE_OK) {
    > + rc = cxl_get_chbs(hb, &ctx);
    > + if (rc == -ENOENT)
    > dev_err(match, "unable to retrieve _UID\n");

    Why not push that down into the cxl_get_chbs() where no special handling
    of error code is needed?

    > - return -ENODEV;
    > - }
    > + if (rc)
    > + return rc;
    >
    > + uid = ctx.uid;
    > dev_dbg(match, "UID found: %lld\n", uid);
    >
    > - ctx = (struct cxl_chbs_context) {
    > - .uid = uid,
    > - };
    > - acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx);
    > -
    > if (!ctx.base) {
    > dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n",
    > uid);
    > @@ -405,12 +417,17 @@ static int add_host_bridge_dport(struct device *match, void *arg)
    > pci_root = acpi_pci_find_root(hb->handle);
    > bridge = pci_root->bus->bridge;
    >
    > + /*
    > + * In RCH mode, bind the component regs base to the dport. In
    > + * VH mode it will be bound to the CXL host bridge's port
    > + * object later in add_host_bridge_uport().
    > + */
    > if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
    > dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.base);
    > dport = devm_cxl_add_rch_dport(root_port, bridge, uid, ctx.base);
    > } else {
    > - dev_dbg(match, "CHBCR found for UID %lld: %pa\n", uid, &ctx.base);
    > - dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.base);
    > + dport = devm_cxl_add_dport(root_port, bridge, uid,
    > + CXL_RESOURCE_NONE);
    > }
    >
    > if (IS_ERR(dport))
    > @@ -432,6 +449,8 @@ static int add_host_bridge_uport(struct device *match, void *arg)
    > struct cxl_dport *dport;
    > struct cxl_port *port;
    > struct device *bridge;
    > + struct cxl_chbs_context ctx;
    > + resource_size_t component_reg_phys;
    > int rc;
    >
    > if (!hb)
    > @@ -450,12 +469,28 @@ static int add_host_bridge_uport(struct device *match, void *arg)
    > return 0;
    > }
    >
    > + rc = cxl_get_chbs(hb, &ctx);
    > + if (rc)
    > + return rc;
    > +
    > + if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
    > + /* RCH mode, should never happen */
    > + return 0;
    > +
    > + if (ctx.base)
    > + component_reg_phys = ctx.base;
    > + else
    > + component_reg_phys = CXL_RESOURCE_NONE;
    > +
    > + if (component_reg_phys != CXL_RESOURCE_NONE)
    > + dev_dbg(match, "CHBCR found for UID %lld: %pa\n",
    > + ctx.uid, &component_reg_phys);

    Why not put that in the block above? Fine leaving it here if this
    makes sense after further refactoring.

    > +
    > rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
    > if (rc)
    > return rc;
    >
    > - port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
    > - dport);
    > + port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
    > if (IS_ERR(port))
    > return PTR_ERR(port);
    >

    \
     
     \ /
      Last update: 2023-06-01 14:46    [W:4.226 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site