Messages in this thread | | | Date | Tue, 13 Jun 2023 10:28:19 -0500 | Subject | Re: [PATCH v5 23/26] cxl/pci: Disable root port interrupts in RCH mode | From | Terry Bowman <> |
| |
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
| |