RAS registers are not currently mapped for CXL root ports, CXL downstream
switch ports, and CXL upstream switch ports. Update the driver to map the
ports' RAS registers in preparation for RAS logging and handling to be
added in the future.
Add a 'struct cxl_regs' variable to 'struct cxl_port'. This will be used
to store a pointer to the upstream port's mapped RAS registers.
Invoke the RAS mapping logic from the CXL memory device probe routine
after the endpoint is added. This ensures the ports have completed
training and the RAS registers are present in CXL.cachemem.
Refactor the cxl_dport_map_regs() function to support mapping the CXL
PCIe ports. Also, check for previously mapped registers in the topology
including CXL switch. Endpoints under a CXL switch share a CXL root port
and will be iterated for each endpoint. Only map once.
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
drivers/cxl/core/pci.c | 30 +++++++++++++++++++++++++-----
drivers/cxl/cxl.h | 5 +++++
drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++--
3 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0df09bd79408..e6c91b3dfccf 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -787,16 +787,26 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
dport->regs.dport_aer = dport_aer;
}
-static void cxl_dport_map_regs(struct cxl_dport *dport)
+static void cxl_port_map_regs(struct device *dev,
+ struct cxl_register_map *map,
+ struct cxl_regs *regs)
{
- struct cxl_register_map *map = &dport->reg_map;
- struct device *dev = dport->dport_dev;
-
if (!map->component_map.ras.valid)
dev_dbg(dev, "RAS registers not found\n");
- else if (cxl_map_component_regs(map, &dport->regs.component,
+ else if (regs->ras)
+ dev_dbg(dev, "RAS registers already initialized\n");
+ else if (cxl_map_component_regs(map, ®s->component,
BIT(CXL_CM_CAP_CAP_ID_RAS)))
dev_dbg(dev, "Failed to map RAS capability.\n");
+}
+
+static void cxl_dport_map_regs(struct cxl_dport *dport)
+{
+ struct cxl_register_map *map = &dport->reg_map;
+ struct cxl_regs *regs = &dport->regs;
+ struct device *dev = dport->dport_dev;
+
+ cxl_port_map_regs(dev, map, regs);
if (dport->rch)
cxl_dport_map_rch_aer(dport);
@@ -831,6 +841,16 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
}
}
+void cxl_setup_parent_uport(struct device *host, struct cxl_port *port)
+{
+ struct cxl_register_map *map = &port->reg_map;
+ struct cxl_regs *regs = &port->regs;
+ struct device *uport_dev = port->uport_dev;
+
+ cxl_port_map_regs(uport_dev, map, regs);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_uport, CXL);
+
void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
{
struct device *dport_dev = dport->dport_dev;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 036d17db68e0..7cee678fdb75 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -587,6 +587,7 @@ struct cxl_dax_region {
* @parent_dport: dport that points to this port in the parent
* @decoder_ida: allocator for decoder ids
* @reg_map: component and ras register mapping parameters
+ * @regs: mapped component registers
* @nr_dports: number of entries in @dports
* @hdm_end: track last allocated HDM decoder instance for allocation ordering
* @commit_end: cursor to track highest committed decoder for commit ordering
@@ -607,6 +608,7 @@ struct cxl_port {
struct cxl_dport *parent_dport;
struct ida decoder_ida;
struct cxl_register_map reg_map;
+ struct cxl_regs regs;
int nr_dports;
int hdm_end;
int commit_end;
@@ -757,9 +759,12 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
#ifdef CONFIG_PCIEAER_CXL
void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
+void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
#else
static inline void cxl_setup_parent_dport(struct device *host,
struct cxl_dport *dport) { }
+static inline void cxl_setup_parent_uport(struct device *host,
+ struct cxl_port *port) { }
#endif
struct cxl_decoder *to_cxl_decoder(struct device *dev);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 0c79d9ce877c..51a4641fc9a6 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -45,10 +45,39 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
return 0;
}
+static bool cxl_dev_is_pci_type(struct device *dev, u32 pcie_type)
+{
+ struct pci_dev *pdev;
+
+ if (!dev_is_pci(dev))
+ return false;
+
+ pdev = to_pci_dev(dev);
+ if (pci_pcie_type(pdev) != pcie_type)
+ return false;
+
+ return pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+ CXL_DVSEC_REG_LOCATOR);
+}
+
+static void cxl_setup_ep_parent_ports(struct cxl_ep *ep, struct device *host)
+{
+ struct cxl_dport *dport = ep->dport;
+
+ if (cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_DOWNSTREAM) ||
+ cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_ROOT_PORT))
+ cxl_setup_parent_dport(host, ep->dport);
+
+ if (cxl_dev_is_pci_type(dport->port->uport_dev, PCI_EXP_TYPE_UPSTREAM))
+ cxl_setup_parent_uport(host, ep->dport->port);
+}
+
static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
struct cxl_dport *parent_dport)
{
struct cxl_port *parent_port = parent_dport->port;
+ struct cxl_dev_state *cxlds = cxlmd->cxlds;
+ struct pci_dev *pdev = to_pci_dev(cxlds->dev);
struct cxl_port *endpoint, *iter, *down;
int rc;
@@ -62,6 +91,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
ep = cxl_ep_load(iter, cxlmd);
ep->next = down;
+ cxl_setup_ep_parent_ports(ep, &pdev->dev);
}
/* Note: endpoint port component registers are derived from @cxlds */
@@ -157,8 +187,6 @@ static int cxl_mem_probe(struct device *dev)
else
endpoint_parent = &parent_port->dev;
- cxl_setup_parent_dport(dev, dport);
-
device_lock(endpoint_parent);
if (!endpoint_parent->driver) {
dev_err(dev, "CXL port topology %s not enabled\n",
--
2.34.1On 6/18/2024 4:04 AM, Terry Bowman wrote:
> RAS registers are not currently mapped for CXL root ports, CXL downstream
> switch ports, and CXL upstream switch ports. Update the driver to map the
> ports' RAS registers in preparation for RAS logging and handling to be
> added in the future.
>
> Add a 'struct cxl_regs' variable to 'struct cxl_port'. This will be used
> to store a pointer to the upstream port's mapped RAS registers.
>
> Invoke the RAS mapping logic from the CXL memory device probe routine
> after the endpoint is added. This ensures the ports have completed
> training and the RAS registers are present in CXL.cachemem.
>
> Refactor the cxl_dport_map_regs() function to support mapping the CXL
> PCIe ports. Also, check for previously mapped registers in the topology
> including CXL switch. Endpoints under a CXL switch share a CXL root port
> and will be iterated for each endpoint. Only map once.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
> drivers/cxl/core/pci.c | 30 +++++++++++++++++++++++++-----
> drivers/cxl/cxl.h | 5 +++++
> drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++--
> 3 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0df09bd79408..e6c91b3dfccf 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -787,16 +787,26 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
> dport->regs.dport_aer = dport_aer;
> }
>
> -static void cxl_dport_map_regs(struct cxl_dport *dport)
> +static void cxl_port_map_regs(struct device *dev,
> + struct cxl_register_map *map,
> + struct cxl_regs *regs)
> {
> - struct cxl_register_map *map = &dport->reg_map;
> - struct device *dev = dport->dport_dev;
> -
> if (!map->component_map.ras.valid)
> dev_dbg(dev, "RAS registers not found\n");
> - else if (cxl_map_component_regs(map, &dport->regs.component,
> + else if (regs->ras)
> + dev_dbg(dev, "RAS registers already initialized\n");
> + else if (cxl_map_component_regs(map, ®s->component,
> BIT(CXL_CM_CAP_CAP_ID_RAS)))
> dev_dbg(dev, "Failed to map RAS capability.\n");
> +}
> +
> +static void cxl_dport_map_regs(struct cxl_dport *dport)
> +{
> + struct cxl_register_map *map = &dport->reg_map;
> + struct cxl_regs *regs = &dport->regs;
> + struct device *dev = dport->dport_dev;
> +
> + cxl_port_map_regs(dev, map, regs);
>
> if (dport->rch)
> cxl_dport_map_rch_aer(dport);
> @@ -831,6 +841,16 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
> }
> }
>
> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port)
> +{
> + struct cxl_register_map *map = &port->reg_map;
> + struct cxl_regs *regs = &port->regs;
> + struct device *uport_dev = port->uport_dev;
> +
> + cxl_port_map_regs(uport_dev, map, regs);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_uport, CXL);
> +
> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
> {
> struct device *dport_dev = dport->dport_dev;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 036d17db68e0..7cee678fdb75 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -587,6 +587,7 @@ struct cxl_dax_region {
> * @parent_dport: dport that points to this port in the parent
> * @decoder_ida: allocator for decoder ids
> * @reg_map: component and ras register mapping parameters
> + * @regs: mapped component registers
> * @nr_dports: number of entries in @dports
> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> * @commit_end: cursor to track highest committed decoder for commit ordering
> @@ -607,6 +608,7 @@ struct cxl_port {
> struct cxl_dport *parent_dport;
> struct ida decoder_ida;
> struct cxl_register_map reg_map;
> + struct cxl_regs regs;
> int nr_dports;
> int hdm_end;
> int commit_end;
> @@ -757,9 +759,12 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>
> #ifdef CONFIG_PCIEAER_CXL
> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
> #else
> static inline void cxl_setup_parent_dport(struct device *host,
> struct cxl_dport *dport) { }
> +static inline void cxl_setup_parent_uport(struct device *host,
> + struct cxl_port *port) { }
> #endif
>
> struct cxl_decoder *to_cxl_decoder(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 0c79d9ce877c..51a4641fc9a6 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -45,10 +45,39 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> return 0;
> }
>
> +static bool cxl_dev_is_pci_type(struct device *dev, u32 pcie_type)
> +{
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(dev))
> + return false;
> +
> + pdev = to_pci_dev(dev);
> + if (pci_pcie_type(pdev) != pcie_type)
> + return false;
> +
> + return pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> + CXL_DVSEC_REG_LOCATOR);
> +}
> +
> +static void cxl_setup_ep_parent_ports(struct cxl_ep *ep, struct device *host)
> +{
> + struct cxl_dport *dport = ep->dport;
> +
> + if (cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_DOWNSTREAM) ||
> + cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_ROOT_PORT))
> + cxl_setup_parent_dport(host, ep->dport);
you reuse cxl_setup_parent_dport() for root ports in this case, and I noticed that cxl_setup_parent_dport() will update dport->reg_map.host. So the host of dport's reg_map is the first cxl device trying to map the registers on the dport, the mapping of registers will be released during the device removal, but the mapping should still be available for other devices/switches under the root port after the device removal.
> +
> + if (cxl_dev_is_pci_type(dport->port->uport_dev, PCI_EXP_TYPE_UPSTREAM))
> + cxl_setup_parent_uport(host, ep->dport->port);
> +}
> +
> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> struct cxl_dport *parent_dport)
> {
> struct cxl_port *parent_port = parent_dport->port;
> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> struct cxl_port *endpoint, *iter, *down;
> int rc;
>
> @@ -62,6 +91,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>
> ep = cxl_ep_load(iter, cxlmd);
> ep->next = down;
> + cxl_setup_ep_parent_ports(ep, &pdev->dev);
> }
>
> /* Note: endpoint port component registers are derived from @cxlds */
> @@ -157,8 +187,6 @@ static int cxl_mem_probe(struct device *dev)
> else
> endpoint_parent = &parent_port->dev;
>
> - cxl_setup_parent_dport(dev, dport);
> -
> device_lock(endpoint_parent);
> if (!endpoint_parent->driver) {
> dev_err(dev, "CXL port topology %s not enabled\n",
On Mon, 17 Jun 2024 15:04:06 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> RAS registers are not currently mapped for CXL root ports, CXL downstream
> switch ports, and CXL upstream switch ports. Update the driver to map the
> ports' RAS registers in preparation for RAS logging and handling to be
> added in the future.
>
> Add a 'struct cxl_regs' variable to 'struct cxl_port'. This will be used
> to store a pointer to the upstream port's mapped RAS registers.
>
> Invoke the RAS mapping logic from the CXL memory device probe routine
> after the endpoint is added. This ensures the ports have completed
> training and the RAS registers are present in CXL.cachemem.
>
> Refactor the cxl_dport_map_regs() function to support mapping the CXL
> PCIe ports. Also, check for previously mapped registers in the topology
> including CXL switch. Endpoints under a CXL switch share a CXL root port
> and will be iterated for each endpoint. Only map once.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Hi Terry,
A few minor comments inline.
Thanks,
Jonathan
> ---
> drivers/cxl/core/pci.c | 30 +++++++++++++++++++++++++-----
> drivers/cxl/cxl.h | 5 +++++
> drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++--
> 3 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0df09bd79408..e6c91b3dfccf 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -787,16 +787,26 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
> dport->regs.dport_aer = dport_aer;
> }
>
> -static void cxl_dport_map_regs(struct cxl_dport *dport)
> +static void cxl_port_map_regs(struct device *dev,
> + struct cxl_register_map *map,
> + struct cxl_regs *regs)
> {
> - struct cxl_register_map *map = &dport->reg_map;
> - struct device *dev = dport->dport_dev;
> -
> if (!map->component_map.ras.valid)
> dev_dbg(dev, "RAS registers not found\n");
Maybe return here as nothing useful is going to occur after this any more.
> - else if (cxl_map_component_regs(map, &dport->regs.component,
> + else if (regs->ras)
> + dev_dbg(dev, "RAS registers already initialized\n");
likewise, return if this condition happened.
> + else if (cxl_map_component_regs(map, ®s->component,
> BIT(CXL_CM_CAP_CAP_ID_RAS)))
> dev_dbg(dev, "Failed to map RAS capability.\n");
> +}
> +
> +static void cxl_dport_map_regs(struct cxl_dport *dport)
> +{
> + struct cxl_register_map *map = &dport->reg_map;
> + struct cxl_regs *regs = &dport->regs;
> + struct device *dev = dport->dport_dev;
> +
> + cxl_port_map_regs(dev, map, regs);
>
> if (dport->rch)
> cxl_dport_map_rch_aer(dport);
> @@ -831,6 +841,16 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
> }
> }
>
> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port)
> +{
> + struct cxl_register_map *map = &port->reg_map;
> + struct cxl_regs *regs = &port->regs;
> + struct device *uport_dev = port->uport_dev;
> +
> + cxl_port_map_regs(uport_dev, map, regs);
Maybe it will be used later, but based on this patch alone.
cxl_port_map_regs(port->uport_dev, &port->reg_map,
&port->regs);
is more compact and I don't think looses anything on readability front.
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_uport, CXL);
> +
> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
> {
> struct device *dport_dev = dport->dport_dev;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 036d17db68e0..7cee678fdb75 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -587,6 +587,7 @@ struct cxl_dax_region {
> * @parent_dport: dport that points to this port in the parent
> * @decoder_ida: allocator for decoder ids
> * @reg_map: component and ras register mapping parameters
> + * @regs: mapped component registers
> * @nr_dports: number of entries in @dports
> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> * @commit_end: cursor to track highest committed decoder for commit ordering
> @@ -607,6 +608,7 @@ struct cxl_port {
> struct cxl_dport *parent_dport;
> struct ida decoder_ida;
> struct cxl_register_map reg_map;
> + struct cxl_regs regs;
Does mapping the whole cxl_regs in make sense?
At least currently we can't use the pmu regs in there from here
for instance - the mess with interrupts means that has to bind
via the port driver (for now anyway).
Maybe struct cxl_component_regs is more appropriate here?
> int nr_dports;
> int hdm_end;
> int commit_end;
> @@ -757,9 +759,12 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>
> #ifdef CONFIG_PCIEAER_CXL
> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
> #else
> static inline void cxl_setup_parent_dport(struct device *host,
> struct cxl_dport *dport) { }
> +static inline void cxl_setup_parent_uport(struct device *host,
> + struct cxl_port *port) { }
> #endif
>
> struct cxl_decoder *to_cxl_decoder(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 0c79d9ce877c..51a4641fc9a6 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -45,10 +45,39 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> return 0;
> }
>
> +static bool cxl_dev_is_pci_type(struct device *dev, u32 pcie_type)
Naming perhaps needs work to make it clear this is checking if
it's a CXL device of that type.
Also, seems like general functionality that belongs in core/pci.c or
similar.
> +{
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(dev))
> + return false;
> +
> + pdev = to_pci_dev(dev);
> + if (pci_pcie_type(pdev) != pcie_type)
> + return false;
> +
> + return pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> + CXL_DVSEC_REG_LOCATOR);
> +}
> +
> +static void cxl_setup_ep_parent_ports(struct cxl_ep *ep, struct device *host)
> +{
> + struct cxl_dport *dport = ep->dport;
> +
> + if (cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_DOWNSTREAM) ||
> + cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_ROOT_PORT))
> + cxl_setup_parent_dport(host, ep->dport);
> +
> + if (cxl_dev_is_pci_type(dport->port->uport_dev, PCI_EXP_TYPE_UPSTREAM))
> + cxl_setup_parent_uport(host, ep->dport->port);
> +}
> +
> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> struct cxl_dport *parent_dport)
> {
> struct cxl_port *parent_port = parent_dport->port;
> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> struct cxl_port *endpoint, *iter, *down;
> int rc;
>
> @@ -62,6 +91,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>
> ep = cxl_ep_load(iter, cxlmd);
> ep->next = down;
> + cxl_setup_ep_parent_ports(ep, &pdev->dev);
> }
>
> /* Note: endpoint port component registers are derived from @cxlds */
> @@ -157,8 +187,6 @@ static int cxl_mem_probe(struct device *dev)
> else
> endpoint_parent = &parent_port->dev;
>
> - cxl_setup_parent_dport(dev, dport);
> -
> device_lock(endpoint_parent);
> if (!endpoint_parent->driver) {
> dev_err(dev, "CXL port topology %s not enabled\n",
Hi Jonathan,
I added responses inline below.
On 6/20/24 07:46, Jonathan Cameron wrote:
> On Mon, 17 Jun 2024 15:04:06 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
>
>> RAS registers are not currently mapped for CXL root ports, CXL downstream
>> switch ports, and CXL upstream switch ports. Update the driver to map the
>> ports' RAS registers in preparation for RAS logging and handling to be
>> added in the future.
>>
>> Add a 'struct cxl_regs' variable to 'struct cxl_port'. This will be used
>> to store a pointer to the upstream port's mapped RAS registers.
>>
>> Invoke the RAS mapping logic from the CXL memory device probe routine
>> after the endpoint is added. This ensures the ports have completed
>> training and the RAS registers are present in CXL.cachemem.
>>
>> Refactor the cxl_dport_map_regs() function to support mapping the CXL
>> PCIe ports. Also, check for previously mapped registers in the topology
>> including CXL switch. Endpoints under a CXL switch share a CXL root port
>> and will be iterated for each endpoint. Only map once.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Hi Terry,
>
> A few minor comments inline.
>
> Thanks,
>
> Jonathan
>
>> ---
>> drivers/cxl/core/pci.c | 30 +++++++++++++++++++++++++-----
>> drivers/cxl/cxl.h | 5 +++++
>> drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++--
>> 3 files changed, 60 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 0df09bd79408..e6c91b3dfccf 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -787,16 +787,26 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
>> dport->regs.dport_aer = dport_aer;
>> }
>>
>> -static void cxl_dport_map_regs(struct cxl_dport *dport)
>> +static void cxl_port_map_regs(struct device *dev,
>> + struct cxl_register_map *map,
>> + struct cxl_regs *regs)
>> {
>> - struct cxl_register_map *map = &dport->reg_map;
>> - struct device *dev = dport->dport_dev;
>> -
>> if (!map->component_map.ras.valid)
>> dev_dbg(dev, "RAS registers not found\n");
>
> Maybe return here as nothing useful is going to occur after this any more.
>
Ok
>> - else if (cxl_map_component_regs(map, &dport->regs.component,
>> + else if (regs->ras)
>> + dev_dbg(dev, "RAS registers already initialized\n");
>
> likewise, return if this condition happened.
>
Ok
>> + else if (cxl_map_component_regs(map, ®s->component,
>> BIT(CXL_CM_CAP_CAP_ID_RAS)))
>> dev_dbg(dev, "Failed to map RAS capability.\n");
>> +}
>> +
>> +static void cxl_dport_map_regs(struct cxl_dport *dport)
>> +{
>> + struct cxl_register_map *map = &dport->reg_map;
>> + struct cxl_regs *regs = &dport->regs;
>> + struct device *dev = dport->dport_dev;
>> +
>> + cxl_port_map_regs(dev, map, regs);
>>
>> if (dport->rch)
>> cxl_dport_map_rch_aer(dport);
>> @@ -831,6 +841,16 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>> }
>> }
>>
>> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port)
>> +{
>> + struct cxl_register_map *map = &port->reg_map;
>> + struct cxl_regs *regs = &port->regs;
>> + struct device *uport_dev = port->uport_dev;
>> +
>> + cxl_port_map_regs(uport_dev, map, regs);
>
> Maybe it will be used later, but based on this patch alone.
> cxl_port_map_regs(port->uport_dev, &port->reg_map,
> &port->regs);
>> is more compact and I don't think looses anything on readability front.
>
>
Good point.
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_uport, CXL);
>> +
>> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
>> {
>> struct device *dport_dev = dport->dport_dev;
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 036d17db68e0..7cee678fdb75 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -587,6 +587,7 @@ struct cxl_dax_region {
>> * @parent_dport: dport that points to this port in the parent
>> * @decoder_ida: allocator for decoder ids
>> * @reg_map: component and ras register mapping parameters
>> + * @regs: mapped component registers
>> * @nr_dports: number of entries in @dports
>> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
>> * @commit_end: cursor to track highest committed decoder for commit ordering
>> @@ -607,6 +608,7 @@ struct cxl_port {
>> struct cxl_dport *parent_dport;
>> struct ida decoder_ida;
>> struct cxl_register_map reg_map;
>> + struct cxl_regs regs;
>
> Does mapping the whole cxl_regs in make sense?
> At least currently we can't use the pmu regs in there from here
> for instance - the mess with interrupts means that has to bind
> via the port driver (for now anyway).
> Maybe struct cxl_component_regs is more appropriate here?
>
>
Only the RAS is mapped. But, as you point out this can be changed to
be cxl_component_regs and it will be more precise in how it's used.
>> int nr_dports;
>> int hdm_end;
>> int commit_end;
>> @@ -757,9 +759,12 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>>
>> #ifdef CONFIG_PCIEAER_CXL
>> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
>> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
>> #else
>> static inline void cxl_setup_parent_dport(struct device *host,
>> struct cxl_dport *dport) { }
>> +static inline void cxl_setup_parent_uport(struct device *host,
>> + struct cxl_port *port) { }
>> #endif
>>
>> struct cxl_decoder *to_cxl_decoder(struct device *dev);
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 0c79d9ce877c..51a4641fc9a6 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -45,10 +45,39 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>> return 0;
>> }
>>
>> +static bool cxl_dev_is_pci_type(struct device *dev, u32 pcie_type)
> Naming perhaps needs work to make it clear this is checking if
> it's a CXL device of that type.
> Also, seems like general functionality that belongs in core/pci.c or
> similar.
Any suggestions on what to use for the rename?
Regards,
Terry
>
>> +{
>> + struct pci_dev *pdev;
>> +
>> + if (!dev_is_pci(dev))
>> + return false;
>> +
>> + pdev = to_pci_dev(dev);
>> + if (pci_pcie_type(pdev) != pcie_type)
>> + return false;
>> +
>> + return pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
>> + CXL_DVSEC_REG_LOCATOR);
>> +}
>> +
>> +static void cxl_setup_ep_parent_ports(struct cxl_ep *ep, struct device *host)
>> +{
>> + struct cxl_dport *dport = ep->dport;
>> +
>> + if (cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_DOWNSTREAM) ||
>> + cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_ROOT_PORT))
>> + cxl_setup_parent_dport(host, ep->dport);
>> +
>> + if (cxl_dev_is_pci_type(dport->port->uport_dev, PCI_EXP_TYPE_UPSTREAM))
>> + cxl_setup_parent_uport(host, ep->dport->port);
>> +}
>> +
>> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>> struct cxl_dport *parent_dport)
>> {
>> struct cxl_port *parent_port = parent_dport->port;
>> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>> struct cxl_port *endpoint, *iter, *down;
>> int rc;
>>
>> @@ -62,6 +91,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>>
>> ep = cxl_ep_load(iter, cxlmd);
>> ep->next = down;
>> + cxl_setup_ep_parent_ports(ep, &pdev->dev);
>> }
>>
>> /* Note: endpoint port component registers are derived from @cxlds */
>> @@ -157,8 +187,6 @@ static int cxl_mem_probe(struct device *dev)
>> else
>> endpoint_parent = &parent_port->dev;
>>
>> - cxl_setup_parent_dport(dev, dport);
>> -
>> device_lock(endpoint_parent);
>> if (!endpoint_parent->driver) {
>> dev_err(dev, "CXL port topology %s not enabled\n",
>
> >> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > >> index 0c79d9ce877c..51a4641fc9a6 100644 > >> --- a/drivers/cxl/mem.c > >> +++ b/drivers/cxl/mem.c > >> @@ -45,10 +45,39 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data) > >> return 0; > >> } > >> > >> +static bool cxl_dev_is_pci_type(struct device *dev, u32 pcie_type) > > Naming perhaps needs work to make it clear this is checking if > > it's a CXL device of that type. > > Also, seems like general functionality that belongs in core/pci.c or > > similar. > > Any suggestions on what to use for the rename? dev_is_pcie_of_type() perhaps? The dvsec bit obviously is less general but might be bandled separately with if ((dev_is_pcie_of_type(dport->dport_dev, PCI_EXP_TYPE_DOWNSTREAM) || dev_is_pcie_of_type(dport->dport_dev, PCI_EXP_TYPE_ROOT_PORT)) && cxl_dev_regloc(dport->dport_dev)) where cxl_dev_regloc() is that lookup and is also used in core/regs.c Or something along those lines.
© 2016 - 2025 Red Hat, Inc.