Re: [PATCH v6 02/27] cxl/acpi: Probe RCRB later during RCH downstream port creation

[Dave Jiang <dave.jiang@intel.com>]

On 6/21/23 20:51, Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> The RCRB is extracted already during ACPI CEDT table parsing while the
> data of this is needed not earlier than dport creation. This
> implementation comes with drawbacks: During ACPI table scan there is
> already MMIO access including mapping and unmapping, but only ACPI
> data should be collected here. The collected data must be transferred
> through a couple of interfaces until it is finally consumed when
> creating the dport. This causes complex data structures and function
> interfaces. Additionally, RCRB parsing will be extended to also
> extract AER data, it would be much easier do this at a later point
> during port and dport creation when the data structures are available
> to hold that data.
> 
> To simplify all that, probe the RCRB at a later point during RCH
> downstream port creation. Change ACPI table parser to only extract the
> base address of either the component registers or the RCRB. Parse and
> extract the RCRB in devm_cxl_add_rch_dport().
> 
> This is in preparation to centralize all RCRB scanning.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/acpi.c      | 51 +++++++++++++++++------------------------
>   drivers/cxl/core/port.c | 21 +++++++++++++----
>   drivers/cxl/cxl.h       |  1 -
>   3 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 7e1765b09e04..70cd9ac73a8b 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -372,21 +372,21 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>   	return 0;
>   }
>   
> +/* Note, @dev is used by mock_acpi_table_parse_cedt() */
>   struct cxl_chbs_context {
>   	struct device *dev;
>   	unsigned long long uid;
> -	resource_size_t rcrb;
> -	resource_size_t chbcr;
> +	resource_size_t base;
>   	u32 cxl_version;
>   };
>   
> -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> +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->base)
>   		return 0;
>   
>   	chbs = (struct acpi_cedt_chbs *) header;
> @@ -395,23 +395,16 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>   		return 0;
>   
>   	ctx->cxl_version = chbs->cxl_version;
> -	ctx->rcrb = CXL_RESOURCE_NONE;
> -	ctx->chbcr = CXL_RESOURCE_NONE;
> +	ctx->base = CXL_RESOURCE_NONE;
>   
>   	if (!chbs->base)
>   		return 0;
>   
> -	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11) {
> -		ctx->chbcr = chbs->base;
> -		return 0;
> -	}
> -
> -	if (chbs->length != CXL_RCRB_SIZE)
> +	if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11 &&
> +	    chbs->length != CXL_RCRB_SIZE)
>   		return 0;
>   
> -	ctx->rcrb = chbs->base;
> -	ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
> -					   CXL_RCRB_DOWNSTREAM);
> +	ctx->base = chbs->base;
>   
>   	return 0;
>   }
> @@ -443,33 +436,31 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>   		.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) {
> +	if (!ctx.base) {
>   		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n",
>   			 uid);
>   		return 0;
>   	}
>   
> -	if (ctx.rcrb != CXL_RESOURCE_NONE)
> -		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> -
> -	if (ctx.chbcr == CXL_RESOURCE_NONE) {
> -		dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n",
> +	if (ctx.base == CXL_RESOURCE_NONE) {
> +		dev_warn(match, "CHBS invalid for Host Bridge (UID %lld)\n",
>   			 uid);
>   		return 0;
>   	}
>   
> -	dev_dbg(match, "CHBCR found: %pa\n", &ctx.chbcr);
> -
>   	pci_root = acpi_pci_find_root(hb->handle);
>   	bridge = pci_root->bus->bridge;
> -	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
> -		dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
> -					       ctx.chbcr, ctx.rcrb);
> -	else
> -		dport = devm_cxl_add_dport(root_port, bridge, uid,
> -					   ctx.chbcr);
> +
> +	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);
> +	}
> +
>   	if (IS_ERR(dport))
>   		return PTR_ERR(dport);
>   
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e7c284c890bc..1a3f8729a616 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -938,12 +938,25 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>   	if (!dport)
>   		return ERR_PTR(-ENOMEM);
>   
> +	if (rcrb != CXL_RESOURCE_NONE) {
> +		component_reg_phys = cxl_rcrb_to_component(dport_dev,
> +						rcrb, CXL_RCRB_DOWNSTREAM);
> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> +			return ERR_PTR(-ENXIO);
> +		}
> +
> +		dport->rch = true;
> +	}
> +
> +	if (component_reg_phys != CXL_RESOURCE_NONE)
> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> +			&component_reg_phys);
> +
>   	dport->dport = dport_dev;
>   	dport->port_id = port_id;
>   	dport->component_reg_phys = component_reg_phys;
>   	dport->port = port;
> -	if (rcrb != CXL_RESOURCE_NONE)
> -		dport->rch = true;
>   	dport->rcrb = rcrb;
>   
>   	cond_cxl_root_lock(port);
> @@ -1004,14 +1017,12 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
>    * @port: the cxl_port that references this dport
>    * @dport_dev: firmware or PCI device representing the dport
>    * @port_id: identifier for this dport in a decoder's target list
> - * @component_reg_phys: optional location of CXL component registers
>    * @rcrb: mandatory location of a Root Complex Register Block
>    *
>    * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
>    */
>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>   					 struct device *dport_dev, int port_id,
> -					 resource_size_t component_reg_phys,
>   					 resource_size_t rcrb)
>   {
>   	struct cxl_dport *dport;
> @@ -1022,7 +1033,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>   	}
>   
>   	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> -				     component_reg_phys, rcrb);
> +				     CXL_RESOURCE_NONE, rcrb);
>   	if (IS_ERR(dport)) {
>   		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
>   			dev_name(&port->dev), PTR_ERR(dport));
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index f93a28538962..a5cd661face2 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -671,7 +671,6 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>   				     resource_size_t component_reg_phys);
>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>   					 struct device *dport_dev, int port_id,
> -					 resource_size_t component_reg_phys,
>   					 resource_size_t rcrb);
>   
>   struct cxl_decoder *to_cxl_decoder(struct device *dev);