| Message ID | 20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dwc: opitimaze RC Host/EP pci_fixup_addr() | expand |
On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote: > Introduce `parent_bus_offset` in `resource_entry` and a new API, > `pci_add_resource_parent_bus_offset()`, to provide necessary information > for PCI controllers with address translation units. > > Typical PCI data flow involves: > CPU (CPU address) -> Bus Fabric (Intermediate address) -> > PCI Controller (PCI bus address) -> PCI Bus. > > While most bus fabrics preserve address consistency, some modify addresses > to intermediate values. The `parent_bus_offset` enables PCI controllers to > translate these intermediate addresses correctly to PCI bus addresses. > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in > PCI controller drivers. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- Bjorn: How do you think this method to handle parent_bus_offset? so other pci host rc driver should be easy to use it. Frank > change from v9 to v10 > - new patch > --- > drivers/pci/bus.c | 11 +++++++++-- > drivers/pci/of.c | 12 +++++++++++- > include/linux/pci.h | 2 ++ > include/linux/resource_ext.h | 1 + > 4 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > index 98910bc0fcc4e..52e88c391e256 100644 > --- a/drivers/pci/bus.c > +++ b/drivers/pci/bus.c > @@ -31,8 +31,8 @@ struct pci_bus_resource { > struct resource *res; > }; > > -void pci_add_resource_offset(struct list_head *resources, struct resource *res, > - resource_size_t offset) > +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res, > + resource_size_t offset, resource_size_t parent_bus_offset) > { > struct resource_entry *entry; > > @@ -43,8 +43,15 @@ void pci_add_resource_offset(struct list_head *resources, struct resource *res, > } > > entry->offset = offset; > + entry->parent_bus_offset = parent_bus_offset; > resource_list_add_tail(entry, resources); > } > + > +void pci_add_resource_offset(struct list_head *resources, struct resource *res, > + resource_size_t offset) > +{ > + pci_add_resource_parent_bus_offset(resources, res, offset, 0); > +} > EXPORT_SYMBOL(pci_add_resource_offset); > > void pci_add_resource(struct list_head *resources, struct resource *res) > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 7a806f5c0d201..aa4a2e266c55e 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, > res->flags &= ~IORESOURCE_MEM_64; > } > > - pci_add_resource_offset(resources, res, res->start - range.pci_addr); > + /* > + * IORESOURCE_IO res->start is io space start address. > + * IORESOURCE_MEM res->start is cpu start address, which is the > + * same as range.cpu_addr. > + * > + * Use (range.cpu_addr - range.parent_bus_addr) to align both > + * IO and MEM's parent_bus_offset always offset to cpu address. > + */ > + > + pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr, > + range.cpu_addr - range.parent_bus_addr); > } > > /* Check for dma-ranges property */ > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 47b31ad724fa5..0d7e67b47be47 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev, > void pci_add_resource(struct list_head *resources, struct resource *res); > void pci_add_resource_offset(struct list_head *resources, struct resource *res, > resource_size_t offset); > +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res, > + resource_size_t offset, resource_size_t parent_bus_offset); > void pci_free_resource_list(struct list_head *resources); > void pci_bus_add_resource(struct pci_bus *bus, struct resource *res); > struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n); > diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h > index ff0339df56afc..b6ec6cc318203 100644 > --- a/include/linux/resource_ext.h > +++ b/include/linux/resource_ext.h > @@ -24,6 +24,7 @@ struct resource_entry { > struct list_head node; > struct resource *res; /* In master (CPU) address space */ > resource_size_t offset; /* Translation offset for bridge */ > + resource_size_t parent_bus_offset; /* Parent bus address offset for bridge */ > struct resource __res; /* Default storage for res */ > }; > > > -- > 2.34.1 >
On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote: > Introduce `parent_bus_offset` in `resource_entry` and a new API, > `pci_add_resource_parent_bus_offset()`, to provide necessary information > for PCI controllers with address translation units. > > Typical PCI data flow involves: > CPU (CPU address) -> Bus Fabric (Intermediate address) -> > PCI Controller (PCI bus address) -> PCI Bus. > > While most bus fabrics preserve address consistency, some modify addresses > to intermediate values. s/modify/translate/ Specifically, they *translate* addresses, which means the same offset is added to every address in the range, as opposed to masking or some other transformation. I think we can take advantage of this to simplify the callers of .cpu_addr_fixup() later. Ironically, most of the .cpu_addr_fixup() implementations *do* mask the address, e.g., cdns_plat_cpu_addr_fixup() masks with 0x0fffffff. But I think this is actually incorrect because masking results in a many-to-one mapping, e.g., 0x42000000 & 0x0fffffff == 0x02000000 0x52000000 & 0x0fffffff == 0x02000000 But presumably the addresses we pass to cdns_plat_cpu_addr_fixup() don't cross a 256MB (0x10000000) boundary, so we could accomplish the same by subtracting 0x40000000: 0x42000000 - 0x40000000 == 0x02000000 > +++ b/drivers/pci/of.c > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, > res->flags &= ~IORESOURCE_MEM_64; > } > > - pci_add_resource_offset(resources, res, res->start - range.pci_addr); > + /* > + * IORESOURCE_IO res->start is io space start address. > + * IORESOURCE_MEM res->start is cpu start address, which is the > + * same as range.cpu_addr. > + * > + * Use (range.cpu_addr - range.parent_bus_addr) to align both > + * IO and MEM's parent_bus_offset always offset to cpu address. > + */ > + > + pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr, > + range.cpu_addr - range.parent_bus_addr); Wrap to fit in 80 columns like the rest of the file. Will have to unindent the two lines of arguments to make this work.
On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote: > Introduce `parent_bus_offset` in `resource_entry` and a new API, > `pci_add_resource_parent_bus_offset()`, to provide necessary information > for PCI controllers with address translation units. > > Typical PCI data flow involves: > CPU (CPU address) -> Bus Fabric (Intermediate address) -> > PCI Controller (PCI bus address) -> PCI Bus. > > While most bus fabrics preserve address consistency, some modify addresses > to intermediate values. The `parent_bus_offset` enables PCI controllers to > translate these intermediate addresses correctly to PCI bus addresses. > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in > PCI controller drivers. > ... > +++ b/drivers/pci/of.c > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, > res->flags &= ~IORESOURCE_MEM_64; > } > > - pci_add_resource_offset(resources, res, res->start - range.pci_addr); > + /* > + * IORESOURCE_IO res->start is io space start address. > + * IORESOURCE_MEM res->start is cpu start address, which is the > + * same as range.cpu_addr. > + * > + * Use (range.cpu_addr - range.parent_bus_addr) to align both > + * IO and MEM's parent_bus_offset always offset to cpu address. > + */ > + > + pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr, > + range.cpu_addr - range.parent_bus_addr); I don't know exactly where it needs to go, but I think we can call .cpu_addr_fixup() once at startup on the base of the region. This will tell us the offset that applies to the entire region, i.e., parent_bus_offset. Then we can remove all the .cpu_addr_fixup() calls in cdns_pcie_host_init_address_translation(), cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu(). Until we can get rid of all the .cpu_addr_fixup() implementations, We'll still have that single call at startup (I guess once for cadence and another for designware), but it should simplify the current callers quite a bit. > } > > /* Check for dma-ranges property */ > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 47b31ad724fa5..0d7e67b47be47 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev, > void pci_add_resource(struct list_head *resources, struct resource *res); > void pci_add_resource_offset(struct list_head *resources, struct resource *res, > resource_size_t offset); > +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res, > + resource_size_t offset, resource_size_t parent_bus_offset); > void pci_free_resource_list(struct list_head *resources); > void pci_bus_add_resource(struct pci_bus *bus, struct resource *res); > struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n); > diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h > index ff0339df56afc..b6ec6cc318203 100644 > --- a/include/linux/resource_ext.h > +++ b/include/linux/resource_ext.h > @@ -24,6 +24,7 @@ struct resource_entry { > struct list_head node; > struct resource *res; /* In master (CPU) address space */ > resource_size_t offset; /* Translation offset for bridge */ > + resource_size_t parent_bus_offset; /* Parent bus address offset for bridge */ > struct resource __res; /* Default storage for res */ > }; > > > -- > 2.34.1 >
On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote: > On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote: > > Introduce `parent_bus_offset` in `resource_entry` and a new API, > > `pci_add_resource_parent_bus_offset()`, to provide necessary information > > for PCI controllers with address translation units. > > > > Typical PCI data flow involves: > > CPU (CPU address) -> Bus Fabric (Intermediate address) -> > > PCI Controller (PCI bus address) -> PCI Bus. > > > > While most bus fabrics preserve address consistency, some modify addresses > > to intermediate values. The `parent_bus_offset` enables PCI controllers to > > translate these intermediate addresses correctly to PCI bus addresses. > > > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in > > PCI controller drivers. > > ... > > > +++ b/drivers/pci/of.c > > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, > > res->flags &= ~IORESOURCE_MEM_64; > > } > > > > - pci_add_resource_offset(resources, res, res->start - range.pci_addr); > > + /* > > + * IORESOURCE_IO res->start is io space start address. > > + * IORESOURCE_MEM res->start is cpu start address, which is the > > + * same as range.cpu_addr. > > + * > > + * Use (range.cpu_addr - range.parent_bus_addr) to align both > > + * IO and MEM's parent_bus_offset always offset to cpu address. > > + */ > > + > > + pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr, > > + range.cpu_addr - range.parent_bus_addr); > > I don't know exactly where it needs to go, but I think we can call > .cpu_addr_fixup() once at startup on the base of the region. This > will tell us the offset that applies to the entire region, i.e., > parent_bus_offset. > > Then we can remove all the .cpu_addr_fixup() calls in > cdns_pcie_host_init_address_translation(), > cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu(). > > Until we can get rid of all the .cpu_addr_fixup() implementations, > We'll still have that single call at startup (I guess once for cadence > and another for designware), but it should simplify the current > callers quite a bit. I don't think it can simple code. cdns_pcie_set_outbound_region() and dw_pcie_prog_outbound_atu() are called by EP functions, which have not use "resource" to manage outbound windows. And there are IO/CFG/MEM space at host side, It will involve more .cpu_addr_fixup() calls to setup these range. Frank > > > } > > > > /* Check for dma-ranges property */ > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 47b31ad724fa5..0d7e67b47be47 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev, > > void pci_add_resource(struct list_head *resources, struct resource *res); > > void pci_add_resource_offset(struct list_head *resources, struct resource *res, > > resource_size_t offset); > > +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res, > > + resource_size_t offset, resource_size_t parent_bus_offset); > > void pci_free_resource_list(struct list_head *resources); > > void pci_bus_add_resource(struct pci_bus *bus, struct resource *res); > > struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n); > > diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h > > index ff0339df56afc..b6ec6cc318203 100644 > > --- a/include/linux/resource_ext.h > > +++ b/include/linux/resource_ext.h > > @@ -24,6 +24,7 @@ struct resource_entry { > > struct list_head node; > > struct resource *res; /* In master (CPU) address space */ > > resource_size_t offset; /* Translation offset for bridge */ > > + resource_size_t parent_bus_offset; /* Parent bus address offset for bridge */ > > struct resource __res; /* Default storage for res */ > > }; > > > > > > -- > > 2.34.1 > >
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 98910bc0fcc4e..52e88c391e256 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -31,8 +31,8 @@ struct pci_bus_resource { struct resource *res; }; -void pci_add_resource_offset(struct list_head *resources, struct resource *res, - resource_size_t offset) +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res, + resource_size_t offset, resource_size_t parent_bus_offset) { struct resource_entry *entry; @@ -43,8 +43,15 @@ void pci_add_resource_offset(struct list_head *resources, struct resource *res, } entry->offset = offset; + entry->parent_bus_offset = parent_bus_offset; resource_list_add_tail(entry, resources); } + +void pci_add_resource_offset(struct list_head *resources, struct resource *res, + resource_size_t offset) +{ + pci_add_resource_parent_bus_offset(resources, res, offset, 0); +} EXPORT_SYMBOL(pci_add_resource_offset); void pci_add_resource(struct list_head *resources, struct resource *res) diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 7a806f5c0d201..aa4a2e266c55e 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, res->flags &= ~IORESOURCE_MEM_64; } - pci_add_resource_offset(resources, res, res->start - range.pci_addr); + /* + * IORESOURCE_IO res->start is io space start address. + * IORESOURCE_MEM res->start is cpu start address, which is the + * same as range.cpu_addr. + * + * Use (range.cpu_addr - range.parent_bus_addr) to align both + * IO and MEM's parent_bus_offset always offset to cpu address. + */ + + pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr, + range.cpu_addr - range.parent_bus_addr); } /* Check for dma-ranges property */ diff --git a/include/linux/pci.h b/include/linux/pci.h index 47b31ad724fa5..0d7e67b47be47 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev, void pci_add_resource(struct list_head *resources, struct resource *res); void pci_add_resource_offset(struct list_head *resources, struct resource *res, resource_size_t offset); +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res, + resource_size_t offset, resource_size_t parent_bus_offset); void pci_free_resource_list(struct list_head *resources); void pci_bus_add_resource(struct pci_bus *bus, struct resource *res); struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n); diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h index ff0339df56afc..b6ec6cc318203 100644 --- a/include/linux/resource_ext.h +++ b/include/linux/resource_ext.h @@ -24,6 +24,7 @@ struct resource_entry { struct list_head node; struct resource *res; /* In master (CPU) address space */ resource_size_t offset; /* Translation offset for bridge */ + resource_size_t parent_bus_offset; /* Parent bus address offset for bridge */ struct resource __res; /* Default storage for res */ };
Introduce `parent_bus_offset` in `resource_entry` and a new API, `pci_add_resource_parent_bus_offset()`, to provide necessary information for PCI controllers with address translation units. Typical PCI data flow involves: CPU (CPU address) -> Bus Fabric (Intermediate address) -> PCI Controller (PCI bus address) -> PCI Bus. While most bus fabrics preserve address consistency, some modify addresses to intermediate values. The `parent_bus_offset` enables PCI controllers to translate these intermediate addresses correctly to PCI bus addresses. Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in PCI controller drivers. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- change from v9 to v10 - new patch --- drivers/pci/bus.c | 11 +++++++++-- drivers/pci/of.c | 12 +++++++++++- include/linux/pci.h | 2 ++ include/linux/resource_ext.h | 1 + 4 files changed, 23 insertions(+), 3 deletions(-)