Re: [PATCH v11 07/20] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability

Terry Bowman
2023-10-09T14:35:24Z

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) {
>