Re: [PATCH v5 23/26] cxl/pci: Disable root port interrupts in RCH mode

Terry Bowman
2023-06-13T15:28:31Z

Hi Dan,

Thanks for the review.

On 6/12/23 15:29, Dan Williams wrote:
> Terry Bowman wrote:
>> The RCH root port contains root command AER registers that should not be
>> enabled.[1] Disable these to prevent root port interrupts.
>>
>> [1] CXL 3.0 - 12.2.1.1 RCH Downstream Port-detected Errors
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> ---
>> drivers/cxl/core/port.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index bc5d0ee9da54..828ae69086c4 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -981,6 +981,30 @@ static int cxl_dport_map_regs(struct cxl_dport *dport)
>> return cxl_dport_map_rch_aer(dport);
>> }
>>
>> +static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>> +{
>> + void __iomem *aer_base = dport->regs.dport_aer;
>> + u32 aer_cmd_mask, aer_cmd;
>> +
>> + if (!dport->rch || !aer_base)
>> + return;
>
> Does this need to check ->rch? There is no path that sets
> ->regs.dport_aer in the VH case, right?
>

Correct. This can be simplified to only check !aer_base because
aer_base is only set if dport->rch.

>> +
>> + /*
>> + * Disable RCH root port command interrupts.
>> + * CXL 3.0 12.2.1.1 - RCH Downstream Port-detected Errors
>> + *
>> + * This sequnce may not be necessary. CXL spec states disabling
>> + * the root cmd register's interrupts is required. But, PCI spec
>> + * shows these are disabled by default on reset.
>> + */
>> + aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
>> + PCI_ERR_ROOT_CMD_NONFATAL_EN |
>> + PCI_ERR_ROOT_CMD_FATAL_EN);
>> + aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
>> + aer_cmd &= ~aer_cmd_mask;
>> + writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
>
> What about the scenario where @host_bridge->native_cxl_error is false?
> Should the driver unconditionally touch AER registers?
>

Right. The driver should only touch the AER registers if the AER control is OSC negotiated to OS.
I'll add check for @host_bridge->native_cxl_error.

>> +}
>> +
>> static struct cxl_dport *
>> __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>> int port_id, resource_size_t component_reg_phys,
>> @@ -1038,6 +1062,8 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>> if (rc && rc != -ENODEV)
>> return ERR_PTR(rc);
>>
>> + cxl_disable_rch_root_ints(dport);
>> +
>
> It feels odd to modify hardware state within an object setup helper. Is
> there a reason this needs to be settled before __devm_cxl_add_dport()
> returns? If not this feels like a helper that cxl_acpi should call to
> change the state of the @dport instance it allocated.
Sure. This can be moved into the caller, add_host_bridge_dport(), after calling devm_cxl_add_rch_dport().

Regards,
Terry