| Message ID | 20240611055254.61203-2-kobayashi.da-06@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Export cxl1.1 device link status register value to pci device sysfs. | expand |
On Tue, 11 Jun 2024 14:52:53 +0900 "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote: > Add rcd_pcie_cap and its initialization at __rcrb_to_component() to cache > the offset of cxl1.1 device link status information. By caching it, avoid > the walking memory map area to find the offset when output the register > value. > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> Hi. The basic functionality now looks good, but I'm not convinced by the 'where' of the calls. Even though it will require an additional ioremap/iounmap() pair I don't think you should bury this in code doing something largely unrelated. > --- > drivers/cxl/core/core.h | 5 +++++ > drivers/cxl/core/regs.c | 27 ++++++++++++++++++++++++++- > drivers/cxl/cxl.h | 9 +++++++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 3b64fb1b9ed0..66778c3ce3b7 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -75,6 +75,11 @@ resource_size_t __rcrb_to_component(struct device *dev, > enum cxl_rcrb which); > u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb); > > +#define PCI_RCRB_CAP_LIST_ID_MASK GENMASK(7, 0) > +#define PCI_RCRB_CAP_HDR_ID_MASK GENMASK(7, 0) > +#define PCI_RCRB_CAP_HDR_NEXT_MASK GENMASK(15, 8) > +#define RCRB_PCIECAP_LEN 0x3c > + > extern struct rw_semaphore cxl_dpa_rwsem; > extern struct rw_semaphore cxl_region_rwsem; > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index 372786f80955..5ce831ca05ca 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -514,6 +514,8 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri > u32 bar0, bar1; > u16 cmd; > u32 id; > + u32 cap_hdr; > + u16 offset; > > if (which == CXL_RCRB_UPSTREAM) > rcrb += SZ_4K; > @@ -537,6 +539,19 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri > cmd = readw(addr + PCI_COMMAND); > bar0 = readl(addr + PCI_BASE_ADDRESS_0); > bar1 = readl(addr + PCI_BASE_ADDRESS_1); > + offset = FIELD_GET(PCI_RCRB_CAP_LIST_ID_MASK, readw(addr + PCI_CAPABILITY_LIST)); Similarly to below, this is putting unrelated functionality in __rcrb_to_component() I think you need to be more smilar to cxl_rcrb_to_aer() which does it's own iomap of the rcrb. That allows it to keep to doing just one thing. So I would have a cxl_rcrb_to_lnkcap() and call that from cxl_pci_setup_regs() > + cap_hdr = readl(addr + offset); > + while ((FIELD_GET(PCI_RCRB_CAP_HDR_ID_MASK, cap_hdr)) != PCI_CAP_ID_EXP) { > + offset = FIELD_GET(PCI_RCRB_CAP_HDR_NEXT_MASK, cap_hdr); > + if (offset == 0 || offset > SZ_4K) { > + offset = 0; > + break; > + } > + cap_hdr = readl(addr + offset); > + } > + if (offset) > + ri->rcd_pcie_cap = offset; > + > iounmap(addr); > release_mem_region(rcrb, SZ_4K); > > @@ -572,8 +587,18 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri > resource_size_t cxl_rcd_component_reg_phys(struct device *dev, > struct cxl_dport *dport) This has gone from being a 'find me one of these' function to one that does rather more inside. Doesn't feel like the right place to locate this capability. I would wrap the mapping code up in a function similar to the aer_cap code in cxl_dport_map_rch_aer() called something like cxl_dport_map_rcd_lnkcap(), and call that directly from cxl_pci_setup_regs() under the same check as is use for calling cxl_rcrb_get_comp_regs() > { > + resource_size_t rcd_pcie_offset, ret; > + > if (!dport->rch) > return CXL_RESOURCE_NONE; > - return __rcrb_to_component(dev, &dport->rcrb, CXL_RCRB_UPSTREAM); > + > + ret = __rcrb_to_component(dev, &dport->rcrb, CXL_RCRB_UPSTREAM); > + if (dport->rcrb.rcd_pcie_cap) { > + rcd_pcie_offset = dport->rcrb.base + dport->rcrb.rcd_pcie_cap; > + dport->regs.rcd_pcie_cap = devm_cxl_iomap_block(dev, rcd_pcie_offset, > + sizeof(u8) * RCRB_PCIECAP_LEN); > + } > + > + return ret; > } > EXPORT_SYMBOL_NS_GPL(cxl_rcd_component_reg_phys, CXL); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 003feebab79b..fc9e0dbd5932 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -230,6 +230,14 @@ struct cxl_regs { > struct_group_tagged(cxl_rch_regs, rch_regs, > void __iomem *dport_aer; > ); > + > + /* > + * RCD upstream port specific PCIe cap register > + * @pcie_cap: CXL 3.0 8.2.1.2 RCD Upstream Port RCRB > + */ > + struct_group_tagged(cxl_rcd_regs, rcd_regs, > + void __iomem *rcd_pcie_cap; > + ); > }; > > struct cxl_reg_map { > @@ -646,6 +654,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) > > struct cxl_rcrb_info { > resource_size_t base; > + u16 rcd_pcie_cap; > u16 aer_cap; > }; >
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 3b64fb1b9ed0..66778c3ce3b7 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -75,6 +75,11 @@ resource_size_t __rcrb_to_component(struct device *dev, enum cxl_rcrb which); u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb); +#define PCI_RCRB_CAP_LIST_ID_MASK GENMASK(7, 0) +#define PCI_RCRB_CAP_HDR_ID_MASK GENMASK(7, 0) +#define PCI_RCRB_CAP_HDR_NEXT_MASK GENMASK(15, 8) +#define RCRB_PCIECAP_LEN 0x3c + extern struct rw_semaphore cxl_dpa_rwsem; extern struct rw_semaphore cxl_region_rwsem; diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 372786f80955..5ce831ca05ca 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -514,6 +514,8 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri u32 bar0, bar1; u16 cmd; u32 id; + u32 cap_hdr; + u16 offset; if (which == CXL_RCRB_UPSTREAM) rcrb += SZ_4K; @@ -537,6 +539,19 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri cmd = readw(addr + PCI_COMMAND); bar0 = readl(addr + PCI_BASE_ADDRESS_0); bar1 = readl(addr + PCI_BASE_ADDRESS_1); + offset = FIELD_GET(PCI_RCRB_CAP_LIST_ID_MASK, readw(addr + PCI_CAPABILITY_LIST)); + cap_hdr = readl(addr + offset); + while ((FIELD_GET(PCI_RCRB_CAP_HDR_ID_MASK, cap_hdr)) != PCI_CAP_ID_EXP) { + offset = FIELD_GET(PCI_RCRB_CAP_HDR_NEXT_MASK, cap_hdr); + if (offset == 0 || offset > SZ_4K) { + offset = 0; + break; + } + cap_hdr = readl(addr + offset); + } + if (offset) + ri->rcd_pcie_cap = offset; + iounmap(addr); release_mem_region(rcrb, SZ_4K); @@ -572,8 +587,18 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri resource_size_t cxl_rcd_component_reg_phys(struct device *dev, struct cxl_dport *dport) { + resource_size_t rcd_pcie_offset, ret; + if (!dport->rch) return CXL_RESOURCE_NONE; - return __rcrb_to_component(dev, &dport->rcrb, CXL_RCRB_UPSTREAM); + + ret = __rcrb_to_component(dev, &dport->rcrb, CXL_RCRB_UPSTREAM); + if (dport->rcrb.rcd_pcie_cap) { + rcd_pcie_offset = dport->rcrb.base + dport->rcrb.rcd_pcie_cap; + dport->regs.rcd_pcie_cap = devm_cxl_iomap_block(dev, rcd_pcie_offset, + sizeof(u8) * RCRB_PCIECAP_LEN); + } + + return ret; } EXPORT_SYMBOL_NS_GPL(cxl_rcd_component_reg_phys, CXL); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 003feebab79b..fc9e0dbd5932 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -230,6 +230,14 @@ struct cxl_regs { struct_group_tagged(cxl_rch_regs, rch_regs, void __iomem *dport_aer; ); + + /* + * RCD upstream port specific PCIe cap register + * @pcie_cap: CXL 3.0 8.2.1.2 RCD Upstream Port RCRB + */ + struct_group_tagged(cxl_rcd_regs, rcd_regs, + void __iomem *rcd_pcie_cap; + ); }; struct cxl_reg_map { @@ -646,6 +654,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) struct cxl_rcrb_info { resource_size_t base; + u16 rcd_pcie_cap; u16 aer_cap; };
Add rcd_pcie_cap and its initialization at __rcrb_to_component() to cache the offset of cxl1.1 device link status information. By caching it, avoid the walking memory map area to find the offset when output the register value. Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> --- drivers/cxl/core/core.h | 5 +++++ drivers/cxl/core/regs.c | 27 ++++++++++++++++++++++++++- drivers/cxl/cxl.h | 9 +++++++++ 3 files changed, 40 insertions(+), 1 deletion(-)