| Message ID | 20230607221651.2454764-24-terry.bowman@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pci: Add support for RCH RAS error handling | expand |
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? > + > + /* > + * 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? > +} > + > 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.
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
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; + + /* + * 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); +} + 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); + cond_cxl_root_lock(port); rc = add_dport(port, dport); cond_cxl_root_unlock(port);
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(+)