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->reg_map in
port->reg_map for endpoint ports.
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Robert Richter <rrichter@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..14a0d0017df3 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 *reg_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) {
+ reg_map = &port->reg_map;
+ if (reg_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 (!reg_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(reg_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 28ba8922d0a4..f69484d3c93c 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -751,16 +751,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->reg_map = cxlds->reg_map;
+ port->reg_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);
--
2.30.2On Wed, 27 Sep 2023 17:43:26 +0200
Robert Richter <rrichter@amd.com> wrote:
> 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->reg_map in
> port->reg_map for endpoint ports.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@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>
A few comments inline.
Also, Dan's SoB doesn't make sense if you are the Author and he's not
the one sending the email. The fun of patches bounced back and forwards
is sometimes you have to tweak this stuff on each posting... :(
> ---
> 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..14a0d0017df3 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 *reg_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) {
> + reg_map = &port->reg_map;
Could you set this where it's defined above?
> + if (reg_map->resource == CXL_RESOURCE_NONE) {
A reminder comment on why/when this happens might be a good addition.
> + if (info && info->mem_enabled) {
> + cxlhdm->decoder_count = info->ranges;
> + return cxlhdm;
> + }
Trivial (and true before this patch) but I'd rather see the error path out of line
if (!info || !info->mem_enabled) {
WARN_ON(1);
dev_err(dev, "No ...
...
}
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 (!reg_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(reg_map, &cxlhdm->regs,
> + BIT(CXL_CM_CAP_CAP_ID_HDM));
> + if (rc) {
> + dev_dbg(dev, "Failed to map HDM capability.\n");
dev_err() seems appropriate here.
> return ERR_PTR(rc);
> + }
>
> parse_hdm_decoder_caps(cxlhdm);
> if (cxlhdm->decoder_count == 0) {
On 02.10.23 15:43:51, Jonathan Cameron wrote: > On Wed, 27 Sep 2023 17:43:26 +0200 > Robert Richter <rrichter@amd.com> wrote: > > > 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->reg_map in > > port->reg_map for endpoint ports. > > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > > Signed-off-by: Robert Richter <rrichter@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> > A few comments inline. > > Also, Dan's SoB doesn't make sense if you are the Author and he's not > the one sending the email. The fun of patches bounced back and forwards > is sometimes you have to tweak this stuff on each posting... :( Yeah, there was some back and forth with the code going on: Terry changed a little, Dan was taking it and reposting, Terry was also submitting the previous series, Dave reviewed it. So I decided here to just swap Terry and my SOBs as I was posting the series now and left the remaining to Dan the way as it will look like when he will apply the patch, which means updating the SOB chain again. This keeps the comment for the change he made to the patch. -Robert > > > --- > > drivers/cxl/core/hdm.c | 48 ++++++++++++++++------------------------- > > drivers/cxl/core/port.c | 29 +++++++++++++++++++------ > > drivers/cxl/mem.c | 5 ++--- > > 3 files changed, 43 insertions(+), 39 deletions(-)
Hi Jonathan,
I added responses below.
On 10/2/23 09:43, Jonathan Cameron wrote:
> On Wed, 27 Sep 2023 17:43:26 +0200
> Robert Richter <rrichter@amd.com> wrote:
>
>> 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->reg_map in
>> port->reg_map for endpoint ports.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> Signed-off-by: Robert Richter <rrichter@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>
> A few comments inline.
>
> Also, Dan's SoB doesn't make sense if you are the Author and he's not
> the one sending the email. The fun of patches bounced back and forwards
> is sometimes you have to tweak this stuff on each posting... :(
>
>> ---
>> 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..14a0d0017df3 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 *reg_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) {
>> + reg_map = &port->reg_map;
>
> Could you set this where it's defined above?
>
Yes.
>> + if (reg_map->resource == CXL_RESOURCE_NONE) {
>
> A reminder comment on why/when this happens might be a good addition.
>
Yes, we will add.
>> + if (info && info->mem_enabled) {
>> + cxlhdm->decoder_count = info->ranges;
>> + return cxlhdm;
>> + }
>
> Trivial (and true before this patch) but I'd rather see the error path out of line
>
Ok, we will change the conditional check to be for the error case and the default path
for success.
>
> if (!info || !info->mem_enabled) {
> WARN_ON(1);
> dev_err(dev, "No ...
> ...
> }
>
> 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 (!reg_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(reg_map, &cxlhdm->regs,
>> + BIT(CXL_CM_CAP_CAP_ID_HDM));
>> + if (rc) {
>> + dev_dbg(dev, "Failed to map HDM capability.\n");
> dev_err() seems appropriate here.
>
Yes, we will change dev_dbg() here to use dev_err().
Regards,
Terry
>> return ERR_PTR(rc);
>> + }
>>
>> parse_hdm_decoder_caps(cxlhdm);
>> if (cxlhdm->decoder_count == 0) {
>
© 2016 - 2026 Red Hat, Inc.