Re: [PATCH v18 04/20] cxl: allow Type2 drivers to map cxl component regs

[Alejandro Lucero Palau <alucerop@amd.com>]

On 9/24/25 09:25, Alejandro Lucero Palau wrote:
>
> On 9/18/25 12:03, Jonathan Cameron wrote:
>> On Thu, 18 Sep 2025 10:17:30 +0100
>> alejandro.lucero-palau@amd.com wrote:
>>
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Export cxl core functions for a Type2 driver being able to discover and
>>> map the device component registers.
>>>
>>> Use it in sfc driver cxl initialization.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c 
>>> b/drivers/net/ethernet/sfc/efx_cxl.c
>>> index 56d148318636..cdfbe546d8d8 100644
>>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>>> @@ -5,6 +5,7 @@
>>>    * Copyright (C) 2025, Advanced Micro Devices, Inc.
>>>    */
>>>   +#include <cxl/cxl.h>
>>>   #include <cxl/pci.h>
>>>   #include <linux/pci.h>
>>>   @@ -19,6 +20,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>>       struct pci_dev *pci_dev = efx->pci_dev;
>>>       struct efx_cxl *cxl;
>>>       u16 dvsec;
>>> +    int rc;
>>>         probe_data->cxl_pio_initialised = false;
>>>   @@ -45,6 +47,37 @@ int efx_cxl_init(struct efx_probe_data 
>>> *probe_data)
>>>       if (!cxl)
>>>           return -ENOMEM;
>>>   +    rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_COMPONENT,
>>> +                &cxl->cxlds.reg_map);
>>> +    if (rc) {
>>> +        dev_err(&pci_dev->dev, "No component registers (err=%d)\n", 
>>> rc);
>>> +        return rc;
>>> +    }
>>> +
>>> +    if (!cxl->cxlds.reg_map.component_map.hdm_decoder.valid) {
>>> +        dev_err(&pci_dev->dev, "Expected HDM component register not 
>>> found\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if (!cxl->cxlds.reg_map.component_map.ras.valid)
>>> +        return dev_err_probe(&pci_dev->dev, -ENODEV,
>>> +                     "Expected RAS component register not found\n");
>> Why the mix of dev_err() and dev_err_probe()?
>> I'd prefer dev_err_probe() for all these, but we definitely don't
>> want a mix.
>
>
> I'll use dev_err_probe here and in following patches extending sfc cxl 
> functionality.
>

FWIW, I have decided to drop patch 8 what was something problematic 
coming from v17, and after Dan's patches, neither its original purpose 
nor why I was still using it, remain. So no EPROBE_DEFER possible for 
sfc driver initialization, therefore no dev_err_probe will be used.


This along with sfc driver using pci_err makes me to update the code for 
using always pci_err. Nevetheless, I do not think mixing pci_err and 
dev_err should be seen as inconsistent error reporting since some errors 
could be clearly related to the pci device and other to those devices 
created during cxl driver initialization. For those errors reported in 
this patch makes sense pci_err though.


Thanks


>
>>> +
>>> +    rc = cxl_map_component_regs(&cxl->cxlds.reg_map,
>>> +                    &cxl->cxlds.regs.component,
>>> +                    BIT(CXL_CM_CAP_CAP_ID_RAS));
>>> +    if (rc) {
>>> +        dev_err(&pci_dev->dev, "Failed to map RAS capability.\n");
>>> +        return rc;
>>> +    }
>>> +
>>> +    /*
>>> +     * Set media ready explicitly as there are neither mailbox for 
>>> checking
>>> +     * this state nor the CXL register involved, both not mandatory 
>>> for
>>> +     * type2.
>>> +     */
>>> +    cxl->cxlds.media_ready = true;
>>> +
>>>       probe_data->cxl = cxl;
>>>         return 0;
>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>> index 13d448686189..3b9c8cb187a3 100644
>>> --- a/include/cxl/cxl.h
>>> +++ b/include/cxl/cxl.h
>>> +/**
>>> + * cxl_map_component_regs - map cxl component registers
>>> + *
>> Why 2 blank lines?
>
>
> I'll fix it.
>
>
> Thanks!
>
>
>>
>>> + *
>>> + * @map: cxl register map to update with the mappings
>>> + * @regs: cxl component registers to work with
>>> + * @map_mask: cxl component regs to map
>>> + *
>>> + * Returns integer: success (0) or error (-ENOMEM)
>>> + *
>>> + * Made public for Type2 driver support.
>>> + */
>>> +int cxl_map_component_regs(const struct cxl_register_map *map,
>>> +               struct cxl_component_regs *regs,
>>> +               unsigned long map_mask);
>>>   #endif /* __CXL_CXL_H__ */
>>                struct cxl_register_map *map);
>>