From: Robert Richter <rrichter@amd.com>

Now, that the Component Register mappings are stored, use them to
enable and map the HDM decoder capabilities. The Component Registers
do not need to be probed again for this, remove probing code.

The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
Endpoint's component register mappings are located in the cxlds and
else in the port's structure. Provide a helper function
cxl_port_get_comp_map() to locate the mappings depending on the
component's type.

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/core/hdm.c | 65 +++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 17c8ba8c75e0..892a1fb5e4c6 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 		cxlhdm->interleave_mask |= GENMASK(14, 12);
 }
 
-static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
-				struct cxl_component_regs *regs)
-{
-	struct cxl_register_map map = {
-		.dev = &port->dev,
-		.resource = port->component_reg_phys,
-		.base = crb,
-		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
-	};
-
-	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
-	if (!map.component_map.hdm_decoder.valid) {
-		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
-		/* unique error code to indicate no HDM decoder capability */
-		return -ENODEV;
-	}
-
-	return cxl_map_component_regs(&map, &port->dev, regs,
-				      BIT(CXL_CM_CAP_CAP_ID_HDM));
-}
-
 static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
 {
 	struct cxl_hdm *cxlhdm;
@@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
 	return true;
 }
 
+static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
+{
+	/*
+	 * HDM capability applies to Endpoints, USPs and VH Host
+	 * Bridges. The Endpoint's component register mappings are
+	 * located in the cxlds.
+	 */
+	if (is_cxl_endpoint(port)) {
+		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
+
+		return &memdev->cxlds->comp_map;
+	}
+
+	return &port->comp_map;
+}
+
 /**
  * devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
@@ -156,7 +151,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 {
 	struct device *dev = &port->dev;
 	struct cxl_hdm *cxlhdm;
-	void __iomem *crb;
+	struct cxl_register_map *comp_map;
 	int rc;
 
 	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
@@ -165,19 +160,29 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 	cxlhdm->port = port;
 	dev_set_drvdata(dev, cxlhdm);
 
-	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
-	if (!crb && info && info->mem_enabled) {
-		cxlhdm->decoder_count = info->ranges;
-		return cxlhdm;
-	} else if (!crb) {
+	comp_map = cxl_port_get_comp_map(port);
+
+	if (comp_map->resource == CXL_RESOURCE_NONE) {
+		if (info && info->mem_enabled) {
+			cxlhdm->decoder_count = info->ranges;
+			return cxlhdm;
+		}
 		dev_err(dev, "No component registers mapped\n");
 		return ERR_PTR(-ENXIO);
 	}
 
-	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
-	iounmap(crb);
-	if (rc)
+	if (!comp_map->component_map.hdm_decoder.valid) {
+		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
+		/* unique error code to indicate no HDM decoder capability */
+		return ERR_PTR(-ENODEV);
+	}
+
+	rc = cxl_map_component_regs(comp_map, dev, &cxlhdm->regs,
+				    BIT(CXL_CM_CAP_CAP_ID_HDM));
+	if (rc) {
+		dev_dbg(dev, "Failed to map HDM capability.\n");
 		return ERR_PTR(rc);
+	}
 
 	parse_hdm_decoder_caps(cxlhdm);
 	if (cxlhdm->decoder_count == 0) {
-- 
2.34.1

Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> Now, that the Component Register mappings are stored, use them to
> enable and map the HDM decoder capabilities. The Component Registers
> do not need to be probed again for this, remove probing code.
> 
> The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> Endpoint's component register mappings are located in the cxlds and
> else in the port's structure. Provide a helper function
> cxl_port_get_comp_map() to locate the mappings depending on the
> component's type.
> 
> 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/core/hdm.c | 65 +++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 17c8ba8c75e0..892a1fb5e4c6 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->interleave_mask |= GENMASK(14, 12);
>  }
>  
> -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> -				struct cxl_component_regs *regs)
> -{
> -	struct cxl_register_map map = {
> -		.dev = &port->dev,
> -		.resource = port->component_reg_phys,
> -		.base = crb,
> -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> -	};
> -
> -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> -	if (!map.component_map.hdm_decoder.valid) {
> -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> -		/* unique error code to indicate no HDM decoder capability */
> -		return -ENODEV;
> -	}
> -
> -	return cxl_map_component_regs(&map, &port->dev, regs,
> -				      BIT(CXL_CM_CAP_CAP_ID_HDM));
> -}
> -
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_hdm *cxlhdm;
> @@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  	return true;
>  }
>  
> +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> +{
> +	/*
> +	 * HDM capability applies to Endpoints, USPs and VH Host
> +	 * Bridges. The Endpoint's component register mappings are
> +	 * located in the cxlds.
> +	 */
> +	if (is_cxl_endpoint(port)) {
> +		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> +
> +		return &memdev->cxlds->comp_map;
> +	}
> +
> +	return &port->comp_map;
> +}

This was the function I was hoping would disappear in the new version.
cxl_pci and cxl_port care about different register blocks and have
different mapping lifetimes. I think that justifies having the
endpoint->comp_map be valid for everything that the cxl_port driver
cares about even though it duplicates some informatiom from
cxlds->comp_map.

Dan Williams wrote:
> Terry Bowman wrote:
> > From: Robert Richter <rrichter@amd.com>
> > 
> > Now, that the Component Register mappings are stored, use them to
> > enable and map the HDM decoder capabilities. The Component Registers
> > do not need to be probed again for this, remove probing code.
> > 
> > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > Endpoint's component register mappings are located in the cxlds and
> > else in the port's structure. Provide a helper function
> > cxl_port_get_comp_map() to locate the mappings depending on the
> > component's type.
> > 
> > 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/core/hdm.c | 65 +++++++++++++++++++++++-------------------
> >  1 file changed, 35 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 17c8ba8c75e0..892a1fb5e4c6 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> >  		cxlhdm->interleave_mask |= GENMASK(14, 12);
> >  }
> >  
> > -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> > -				struct cxl_component_regs *regs)
> > -{
> > -	struct cxl_register_map map = {
> > -		.dev = &port->dev,
> > -		.resource = port->component_reg_phys,
> > -		.base = crb,
> > -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> > -	};
> > -
> > -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> > -	if (!map.component_map.hdm_decoder.valid) {
> > -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> > -		/* unique error code to indicate no HDM decoder capability */
> > -		return -ENODEV;
> > -	}
> > -
> > -	return cxl_map_component_regs(&map, &port->dev, regs,
> > -				      BIT(CXL_CM_CAP_CAP_ID_HDM));
> > -}
> > -
> >  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> >  {
> >  	struct cxl_hdm *cxlhdm;
> > @@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> >  	return true;
> >  }
> >  
> > +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> > +{
> > +	/*
> > +	 * HDM capability applies to Endpoints, USPs and VH Host
> > +	 * Bridges. The Endpoint's component register mappings are
> > +	 * located in the cxlds.
> > +	 */
> > +	if (is_cxl_endpoint(port)) {
> > +		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> > +
> > +		return &memdev->cxlds->comp_map;
> > +	}
> > +
> > +	return &port->comp_map;
> > +}
> 
> This was the function I was hoping would disappear in the new version.
> cxl_pci and cxl_port care about different register blocks and have
> different mapping lifetimes. I think that justifies having the
> endpoint->comp_map be valid for everything that the cxl_port driver
> cares about even though it duplicates some informatiom from
> cxlds->comp_map.

In the interest of time I went ahead and reflowed this patch to the
below and it is passing my tests. I also noticed some other @dev vs
@host confusion in some of the previous register conversion so perhaps I
should just send out v11 with this all rolled together...

-- >8 --
Subject: cxl/hdm: Use stored Component Register mappings to map HDM decoder capability

From: Robert Richter <rrichter@amd.com>

Now, that the Component Register mappings are stored, use them to
enable and map the HDM decoder capabilities. The Component Registers
do not need to be probed again for this, remove probing code.

The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
Endpoint's component register mappings are located in the cxlds and
else in the port's structure. Duplicate the cxlds->comp_map in
port->comp_map for endpoint ports.

Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
[rework to drop cxl_port_get_comp_map()]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c  |   48 +++++++++++++++++++----------------------------
 drivers/cxl/core/port.c |   29 ++++++++++++++++++++++------
 drivers/cxl/mem.c       |    5 ++---
 3 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 11d9971f3e8c..ced7801516d2 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 		cxlhdm->interleave_mask |= GENMASK(14, 12);
 }
 
-static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
-				struct cxl_component_regs *regs)
-{
-	struct cxl_register_map map = {
-		.host = &port->dev,
-		.resource = port->component_reg_phys,
-		.base = crb,
-		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
-	};
-
-	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
-	if (!map.component_map.hdm_decoder.valid) {
-		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
-		/* unique error code to indicate no HDM decoder capability */
-		return -ENODEV;
-	}
-
-	return cxl_map_component_regs(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
-}
-
 static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
 {
 	struct cxl_hdm *cxlhdm;
@@ -155,7 +135,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 {
 	struct device *dev = &port->dev;
 	struct cxl_hdm *cxlhdm;
-	void __iomem *crb;
+	struct cxl_register_map *comp_map;
 	int rc;
 
 	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
@@ -164,19 +144,29 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 	cxlhdm->port = port;
 	dev_set_drvdata(dev, cxlhdm);
 
-	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
-	if (!crb && info && info->mem_enabled) {
-		cxlhdm->decoder_count = info->ranges;
-		return cxlhdm;
-	} else if (!crb) {
+	comp_map = &port->comp_map;
+	if (comp_map->resource == CXL_RESOURCE_NONE) {
+		if (info && info->mem_enabled) {
+			cxlhdm->decoder_count = info->ranges;
+			return cxlhdm;
+		}
+		WARN_ON(1);
 		dev_err(dev, "No component registers mapped\n");
 		return ERR_PTR(-ENXIO);
 	}
 
-	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
-	iounmap(crb);
-	if (rc)
+	if (!comp_map->component_map.hdm_decoder.valid) {
+		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
+		/* unique error code to indicate no HDM decoder capability */
+		return ERR_PTR(-ENODEV);
+	}
+
+	rc = cxl_map_component_regs(comp_map, &cxlhdm->regs,
+				    BIT(CXL_CM_CAP_CAP_ID_HDM));
+	if (rc) {
+		dev_dbg(dev, "Failed to map HDM capability.\n");
 		return ERR_PTR(rc);
+	}
 
 	parse_hdm_decoder_caps(cxlhdm);
 	if (cxlhdm->decoder_count == 0) {
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 3732925162e4..64fcb5b22372 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -741,16 +741,31 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
 		return port;
 
 	dev = &port->dev;
-	if (is_cxl_memdev(uport_dev))
+	if (is_cxl_memdev(uport_dev)) {
+		struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
+		struct cxl_dev_state *cxlds = cxlmd->cxlds;
+
 		rc = dev_set_name(dev, "endpoint%d", port->id);
-	else if (parent_dport)
+		if (rc)
+			goto err;
+
+		/*
+		 * The endpoint driver already enumerated the component and RAS
+		 * registers. Reuse that enumeration while prepping them to be
+		 * mapped by the cxl_port driver.
+		 */
+		port->comp_map = cxlds->comp_map;
+		port->comp_map.host = &port->dev;
+	} else if (parent_dport) {
 		rc = dev_set_name(dev, "port%d", port->id);
-	else
-		rc = dev_set_name(dev, "root%d", port->id);
-	if (rc)
-		goto err;
+		if (rc)
+			goto err;
 
-	rc = cxl_port_setup_regs(port, component_reg_phys);
+		rc = cxl_port_setup_regs(port, component_reg_phys);
+		if (rc)
+			goto err;
+	} else
+		rc = dev_set_name(dev, "root%d", port->id);
 	if (rc)
 		goto err;
 
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 317c7548e4e9..04107058739b 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -49,7 +49,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
 				 struct cxl_dport *parent_dport)
 {
 	struct cxl_port *parent_port = parent_dport->port;
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *endpoint, *iter, *down;
 	int rc;
 
@@ -65,8 +64,8 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
 		ep->next = down;
 	}
 
-	endpoint = devm_cxl_add_port(host, &cxlmd->dev,
-				     cxlds->component_reg_phys,
+	/* Note: endpoint port component registers are derived from @cxlds */
+	endpoint = devm_cxl_add_port(host, &cxlmd->dev, CXL_RESOURCE_NONE,
 				     parent_dport);
 	if (IS_ERR(endpoint))
 		return PTR_ERR(endpoint);

Dan,

On 14.09.23 17:15:44, Dan Williams wrote:
> Dan Williams wrote:
> > Terry Bowman wrote:
> > > From: Robert Richter <rrichter@amd.com>
> > > 
> > > Now, that the Component Register mappings are stored, use them to
> > > enable and map the HDM decoder capabilities. The Component Registers
> > > do not need to be probed again for this, remove probing code.
> > > 
> > > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > > Endpoint's component register mappings are located in the cxlds and
> > > else in the port's structure. Provide a helper function
> > > cxl_port_get_comp_map() to locate the mappings depending on the
> > > component's type.
> > > 
> > > 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/core/hdm.c | 65 +++++++++++++++++++++++-------------------
> > >  1 file changed, 35 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index 17c8ba8c75e0..892a1fb5e4c6 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> > >  		cxlhdm->interleave_mask |= GENMASK(14, 12);
> > >  }
> > >  
> > > -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> > > -				struct cxl_component_regs *regs)
> > > -{
> > > -	struct cxl_register_map map = {
> > > -		.dev = &port->dev,
> > > -		.resource = port->component_reg_phys,
> > > -		.base = crb,
> > > -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> > > -	};
> > > -
> > > -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> > > -	if (!map.component_map.hdm_decoder.valid) {
> > > -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> > > -		/* unique error code to indicate no HDM decoder capability */
> > > -		return -ENODEV;
> > > -	}
> > > -
> > > -	return cxl_map_component_regs(&map, &port->dev, regs,
> > > -				      BIT(CXL_CM_CAP_CAP_ID_HDM));
> > > -}
> > > -
> > >  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > >  {
> > >  	struct cxl_hdm *cxlhdm;
> > > @@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > >  	return true;
> > >  }
> > >  
> > > +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> > > +{
> > > +	/*
> > > +	 * HDM capability applies to Endpoints, USPs and VH Host
> > > +	 * Bridges. The Endpoint's component register mappings are
> > > +	 * located in the cxlds.
> > > +	 */
> > > +	if (is_cxl_endpoint(port)) {
> > > +		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> > > +
> > > +		return &memdev->cxlds->comp_map;
> > > +	}
> > > +
> > > +	return &port->comp_map;
> > > +}
> > 
> > This was the function I was hoping would disappear in the new version.
> > cxl_pci and cxl_port care about different register blocks and have
> > different mapping lifetimes. I think that justifies having the
> > endpoint->comp_map be valid for everything that the cxl_port driver
> > cares about even though it duplicates some informatiom from
> > cxlds->comp_map.
> 
> In the interest of time I went ahead and reflowed this patch to the
> below and it is passing my tests. I also noticed some other @dev vs
> @host confusion in some of the previous register conversion so perhaps I
> should just send out v11 with this all rolled together...

just a quick update here.

We were going to send v11 a couple of days ago but I found an issue
during testing, see below. If you don't mind I will send it out next
week with a fix for that included.

> 
> -- >8 --
> Subject: cxl/hdm: Use stored Component Register mappings to map HDM decoder capability
> 
> From: Robert Richter <rrichter@amd.com>
> 
> Now, that the Component Register mappings are stored, use them to
> enable and map the HDM decoder capabilities. The Component Registers
> do not need to be probed again for this, remove probing code.
> 
> The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> Endpoint's component register mappings are located in the cxlds and
> else in the port's structure. Duplicate the cxlds->comp_map in
> port->comp_map for endpoint ports.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> [rework to drop cxl_port_get_comp_map()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/hdm.c  |   48 +++++++++++++++++++----------------------------
>  drivers/cxl/core/port.c |   29 ++++++++++++++++++++++------
>  drivers/cxl/mem.c       |    5 ++---
>  3 files changed, 43 insertions(+), 39 deletions(-)

Patch look good to me.

I have a similar implementation, but did that with a
cxl_port_clone_regs() function in cxl_endpoint_port_probe(). I can
take this version instead.

During testing I found an issue freeing IO resources with devm for RCH
mode. The endpoint is not removed if the cxl_mem driver is
unbound. Then, the resources of the endpoint that also holds the IO
mappings are not freed. A subsequent IO map fails when rebinding the
driver again. It looks like cxl_mem_find_port() is broken for RCDs
preventing the port from being autoremoved. I am working on a fix for
this and will test the whole series again.

Thanks,

-Robert

> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 11d9971f3e8c..ced7801516d2 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->interleave_mask |= GENMASK(14, 12);
>  }
>  
> -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> -				struct cxl_component_regs *regs)
> -{
> -	struct cxl_register_map map = {
> -		.host = &port->dev,
> -		.resource = port->component_reg_phys,
> -		.base = crb,
> -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> -	};
> -
> -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> -	if (!map.component_map.hdm_decoder.valid) {
> -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> -		/* unique error code to indicate no HDM decoder capability */
> -		return -ENODEV;
> -	}
> -
> -	return cxl_map_component_regs(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
> -}
> -
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_hdm *cxlhdm;
> @@ -155,7 +135,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  {
>  	struct device *dev = &port->dev;
>  	struct cxl_hdm *cxlhdm;
> -	void __iomem *crb;
> +	struct cxl_register_map *comp_map;
>  	int rc;
>  
>  	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
> @@ -164,19 +144,29 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  	cxlhdm->port = port;
>  	dev_set_drvdata(dev, cxlhdm);
>  
> -	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
> -	if (!crb && info && info->mem_enabled) {
> -		cxlhdm->decoder_count = info->ranges;
> -		return cxlhdm;
> -	} else if (!crb) {
> +	comp_map = &port->comp_map;
> +	if (comp_map->resource == CXL_RESOURCE_NONE) {
> +		if (info && info->mem_enabled) {
> +			cxlhdm->decoder_count = info->ranges;
> +			return cxlhdm;
> +		}
> +		WARN_ON(1);
>  		dev_err(dev, "No component registers mapped\n");
>  		return ERR_PTR(-ENXIO);
>  	}
>  
> -	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
> -	iounmap(crb);
> -	if (rc)
> +	if (!comp_map->component_map.hdm_decoder.valid) {
> +		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> +		/* unique error code to indicate no HDM decoder capability */
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	rc = cxl_map_component_regs(comp_map, &cxlhdm->regs,
> +				    BIT(CXL_CM_CAP_CAP_ID_HDM));
> +	if (rc) {
> +		dev_dbg(dev, "Failed to map HDM capability.\n");
>  		return ERR_PTR(rc);
> +	}
>  
>  	parse_hdm_decoder_caps(cxlhdm);
>  	if (cxlhdm->decoder_count == 0) {
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 3732925162e4..64fcb5b22372 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -741,16 +741,31 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>  		return port;
>  
>  	dev = &port->dev;
> -	if (is_cxl_memdev(uport_dev))
> +	if (is_cxl_memdev(uport_dev)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
>  		rc = dev_set_name(dev, "endpoint%d", port->id);
> -	else if (parent_dport)
> +		if (rc)
> +			goto err;
> +
> +		/*
> +		 * The endpoint driver already enumerated the component and RAS
> +		 * registers. Reuse that enumeration while prepping them to be
> +		 * mapped by the cxl_port driver.
> +		 */
> +		port->comp_map = cxlds->comp_map;
> +		port->comp_map.host = &port->dev;
> +	} else if (parent_dport) {
>  		rc = dev_set_name(dev, "port%d", port->id);
> -	else
> -		rc = dev_set_name(dev, "root%d", port->id);
> -	if (rc)
> -		goto err;
> +		if (rc)
> +			goto err;
>  
> -	rc = cxl_port_setup_regs(port, component_reg_phys);
> +		rc = cxl_port_setup_regs(port, component_reg_phys);
> +		if (rc)
> +			goto err;
> +	} else
> +		rc = dev_set_name(dev, "root%d", port->id);
>  	if (rc)
>  		goto err;
>  
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 317c7548e4e9..04107058739b 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -49,7 +49,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  				 struct cxl_dport *parent_dport)
>  {
>  	struct cxl_port *parent_port = parent_dport->port;
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_port *endpoint, *iter, *down;
>  	int rc;
>  
> @@ -65,8 +64,8 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  		ep->next = down;
>  	}
>  
> -	endpoint = devm_cxl_add_port(host, &cxlmd->dev,
> -				     cxlds->component_reg_phys,
> +	/* Note: endpoint port component registers are derived from @cxlds */
> +	endpoint = devm_cxl_add_port(host, &cxlmd->dev, CXL_RESOURCE_NONE,
>  				     parent_dport);
>  	if (IS_ERR(endpoint))
>  		return PTR_ERR(endpoint);

Robert Richter wrote:
> Dan,
> 
> On 14.09.23 17:15:44, Dan Williams wrote:
> > Dan Williams wrote:
> > > Terry Bowman wrote:
> > > > From: Robert Richter <rrichter@amd.com>
> > > > 
> > > > Now, that the Component Register mappings are stored, use them to
> > > > enable and map the HDM decoder capabilities. The Component Registers
> > > > do not need to be probed again for this, remove probing code.
> > > > 
> > > > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > > > Endpoint's component register mappings are located in the cxlds and
> > > > else in the port's structure. Provide a helper function
> > > > cxl_port_get_comp_map() to locate the mappings depending on the
> > > > component's type.
> > > > 
> > > > 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/core/hdm.c | 65 +++++++++++++++++++++++-------------------
> > > >  1 file changed, 35 insertions(+), 30 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > index 17c8ba8c75e0..892a1fb5e4c6 100644
> > > > --- a/drivers/cxl/core/hdm.c
> > > > +++ b/drivers/cxl/core/hdm.c
> > > > @@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> > > >  		cxlhdm->interleave_mask |= GENMASK(14, 12);
> > > >  }
> > > >  
> > > > -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> > > > -				struct cxl_component_regs *regs)
> > > > -{
> > > > -	struct cxl_register_map map = {
> > > > -		.dev = &port->dev,
> > > > -		.resource = port->component_reg_phys,
> > > > -		.base = crb,
> > > > -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> > > > -	};
> > > > -
> > > > -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> > > > -	if (!map.component_map.hdm_decoder.valid) {
> > > > -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> > > > -		/* unique error code to indicate no HDM decoder capability */
> > > > -		return -ENODEV;
> > > > -	}
> > > > -
> > > > -	return cxl_map_component_regs(&map, &port->dev, regs,
> > > > -				      BIT(CXL_CM_CAP_CAP_ID_HDM));
> > > > -}
> > > > -
> > > >  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > > >  {
> > > >  	struct cxl_hdm *cxlhdm;
> > > > @@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > > >  	return true;
> > > >  }
> > > >  
> > > > +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> > > > +{
> > > > +	/*
> > > > +	 * HDM capability applies to Endpoints, USPs and VH Host
> > > > +	 * Bridges. The Endpoint's component register mappings are
> > > > +	 * located in the cxlds.
> > > > +	 */
> > > > +	if (is_cxl_endpoint(port)) {
> > > > +		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> > > > +
> > > > +		return &memdev->cxlds->comp_map;
> > > > +	}
> > > > +
> > > > +	return &port->comp_map;
> > > > +}
> > > 
> > > This was the function I was hoping would disappear in the new version.
> > > cxl_pci and cxl_port care about different register blocks and have
> > > different mapping lifetimes. I think that justifies having the
> > > endpoint->comp_map be valid for everything that the cxl_port driver
> > > cares about even though it duplicates some informatiom from
> > > cxlds->comp_map.
> > 
> > In the interest of time I went ahead and reflowed this patch to the
> > below and it is passing my tests. I also noticed some other @dev vs
> > @host confusion in some of the previous register conversion so perhaps I
> > should just send out v11 with this all rolled together...
> 
> just a quick update here.
> 
> We were going to send v11 a couple of days ago but I found an issue
> during testing, see below. If you don't mind I will send it out next
> week with a fix for that included.

Sounds good, I will wait for that update. I had found a couple more
issues as well, like come some confusion on the @host for cxl_dport
register mapping. I will send just that out so you can incorporate.

There was also the feedback on moving register mapping calls out of the
__devm_cxl_add_dport() and into the driver that makes use of them.
Similar for root port interrupt enabling.

> 
> > 
> > -- >8 --
> > Subject: cxl/hdm: Use stored Component Register mappings to map HDM decoder capability
> > 
> > From: Robert Richter <rrichter@amd.com>
> > 
> > Now, that the Component Register mappings are stored, use them to
> > enable and map the HDM decoder capabilities. The Component Registers
> > do not need to be probed again for this, remove probing code.
> > 
> > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > Endpoint's component register mappings are located in the cxlds and
> > else in the port's structure. Duplicate the cxlds->comp_map in
> > port->comp_map for endpoint ports.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > [rework to drop cxl_port_get_comp_map()]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/hdm.c  |   48 +++++++++++++++++++----------------------------
> >  drivers/cxl/core/port.c |   29 ++++++++++++++++++++++------
> >  drivers/cxl/mem.c       |    5 ++---
> >  3 files changed, 43 insertions(+), 39 deletions(-)
> 
> Patch look good to me.
> 
> I have a similar implementation, but did that with a
> cxl_port_clone_regs() function in cxl_endpoint_port_probe(). I can
> take this version instead.
> 
> During testing I found an issue freeing IO resources with devm for RCH
> mode. The endpoint is not removed if the cxl_mem driver is
> unbound. Then, the resources of the endpoint that also holds the IO
> mappings are not freed. A subsequent IO map fails when rebinding the
> driver again. It looks like cxl_mem_find_port() is broken for RCDs
> preventing the port from being autoremoved. I am working on a fix for
> this and will test the whole series again.

Oh, I see that bug, yes, good find. The cxl-topology.sh test is only
checking for endpoint port removal on parent port removal, but RCDs do
not have the same port interaction when the cxl_mem driver detaches.

I expect you found that delete_enpoint() is the problem in that it
assumes the parent_port has a driver, which for RCH it does not because
that's the CXL root port.

In any event, glad to see this moving forward, and glad to let you
drive the next version.