On Thu, Oct 24, 2024 at 04:41:43PM -0400, Frank Li wrote: > Endpoint Root complex > ┌───────┐ ┌─────────┐ > ┌─────┐ │ EP │ │ │ ┌─────┐ > │ │ │ Ctrl │ │ │ │ CPU │ > │ DDR │ │ │ │ ┌────┐ │ └──┬──┘ > │ │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘ > │ │ │ │ │ └────┘ │ Outbound Transfer > └─────┘ │ │ │ │ > │ │ │ │ > │ │ │ │ > │ │ │ │ Inbound Transfer > │ │ │ │ ┌──▼──┐ > ┌───────┐ │ │ │ ┌───────┼─────►│DDR │ > │ │ outbound Transfer* │ │ │ └─────┘ > ┌─────┐ │ Bus ┼─────►│ ATU ─┬────────┼─┘ │ > │ │ │ Fabric│Bus │ │ PCI Addr │ > │ CPU ├───►│ │Addr │ │ 0xA000_0000 │ > │ │CPU │ │0x8000_0000 │ │ │ > └─────┘Addr└───────┘ │ │ │ │ > 0x7000_0000 └───────┘ └─────────┘ > > Add `bus_addr_base` to configure the outbound window address for CPU write. > The bus fabric generally passes the same address to the PCIe EP controller, > but some bus fabrics convert the address before sending it to the PCIe EP > controller. > > Above diagram, CPU write data to outbound windows address 0x7000_0000, > Bus fabric convert it to 0x8000_0000. ATU should use bus address > 0x8000_0000 as input address and convert to PCI address 0xA000_0000. Thanks for the diagram and description. I don't think the top half is relevant to *this* patch, is it? I think this patch is only concerned with the address translations between the CPU in the endpoint and the PCI bus address. In this case it happens in two steps: the bus fabric applies one offset, and the ATU applies a second offset. Unless the top half is relevant, I would omit it and simply use something like this: Endpoint ┌───────────────────────────────────────────────┐ │ pcie-ep@5f010000 │ │ ┌────────────────┐│ │ │ Endpoint ││ │ │ PCIe ││ │ │ Controller ││ │ bus@5f000000 │ ││ │ ┌──────────┐ │ ││ │ │ │ Outbound Transfer ││ │┌─────┐ │ Bus ┼─────►│ ATU ──────────┬┬─────► ││ │ │ Fabric │Bus │ ││PCI Addr ││ CPU ├───►│ │Addr │ ││0xA000_0000 ││ │CPU │ │0x8000_0000 ││ │└─────┘Addr└──────────┘ │ ││ │ 0x7000_0000 └────────────────┘│ └───────────────────────────────────────────────┘ If you don't want a big "Endpoint" box including the CPU and bus fabric, that's OK with me, too. I added it because everything on the PCI side only sees TLPs that contain PCI bus addresses, and can't tell anything about the internal implementation of the Endpoint. > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now, > the device tree provides this information, preferring a common method. > > bus@5f000000 { > compatible = "simple-bus"; > ranges = <0x80000000 0x0 0x70000000 0x10000000>; > > pcie-ep@5f010000 { > reg = <0x5f010000 0x00010000>, > <0x80000000 0x10000000>; > reg-names = "dbi", "addr_space"; > ... > }; > ... > }; I guess bus@5f000000 includes a "ranges" property because that translation from 0x7000_0000 -> 0x8000_0000 is fixed or at least not touched by Linux? And the pcie-ep@5f010000 address translation from 0x8000_0000 to 0xA000_0000 *is* programmed by Linux and therefore can't be described by a DT? But I guess Linux only programs the *PCI* side, and the parent side (0x8000_0000) is fixed? AFAICT, the "reg = <0x5f010000 0x00010000>" part is not relevant here. I guess this implementation assumes there's only a single aperture through the Bus Fabric, right? And also a single ATU aperture through the endpoint PCIe controller? And also that there's only one layer of Bus Fabric address translation? The fact that only a few DWC controllers have this translation suggests that this part of the picture might be external to the DWC IP and there could be more variation. But I guess there's no point in adding code for topologies that don't exist; we can deal with that if the need ever arises. > 'ranges' in bus@5f000000 descript how address convert from CPU address > to bus address. > > Use `of_property_read_reg()` to obtain the bus address and set it to the > ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup(). > > Add 'using_dtbus_info' to indicate device tree reflect correctly bus > address translation in case break compatibility. > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx> > --- > Change from v3 to v4 > - change bus_addr_base to u64 to fix 32bit build error > | Reported-by: kernel test robot <lkp@xxxxxxxxx> > | Closes: https://lore.kernel.org/oe-kbuild-all/202410230328.BTHareG1-lkp@xxxxxxxxx/ > > Change from v2 to v3 > - Add using_dtbus_info to control if use device tree bus ranges > information. > --- > drivers/pci/controller/dwc/pcie-designware-ep.c | 14 +++++++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 9 +++++++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 43ba5c6738df1..81b4057befa62 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -9,6 +9,7 @@ > #include <linux/align.h> > #include <linux/bitfield.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/platform_device.h> > > #include "pcie-designware.h" > @@ -294,7 +295,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > atu.func_no = func_no; > atu.type = PCIE_ATU_TYPE_MEM; > - atu.cpu_addr = addr; > + atu.cpu_addr = addr - ep->phys_base + ep->bus_addr_base; Tangent: Maybe dw_pcie_ob_atu_cfg.cpu_addr isn't exactly the right name, since it now contains an address that is not a CPU physical address. Not a question for *this* patch though. > atu.pci_addr = pci_addr; > atu.size = size; > ret = dw_pcie_ep_outbound_atu(ep, &atu); > @@ -861,6 +862,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > struct device *dev = pci->dev; > struct platform_device *pdev = to_platform_device(dev); > struct device_node *np = dev->of_node; > + int index; > > INIT_LIST_HEAD(&ep->func_list); > > @@ -873,6 +875,16 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > return -EINVAL; > > ep->phys_base = res->start; > + ep->bus_addr_base = ep->phys_base; > + > + if (pci->using_dtbus_info) { > + index = of_property_match_string(np, "reg-names", "addr_space"); > + if (index < 0) > + return -EINVAL; > + > + of_property_read_reg(np, index, &ep->bus_addr_base, NULL); > + } If this translation were fixed, I suppose we'd extract something from a "ranges" property that contains (child-bus-address, parent-bus-address) information. So I suppose "addr_space" contains a fixed parent-bus-address, and is setting the child (PCI) bus address, right? If so, I might add a comment here for other readers who come this way. (And me, because I won't remember the next time I read it :) > ep->addr_size = resource_size(res); > > if (ep->ops->pre_init) > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 347ab74ac35aa..f10b533b04f77 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -410,6 +410,7 @@ struct dw_pcie_ep { > struct list_head func_list; > const struct dw_pcie_ep_ops *ops; > phys_addr_t phys_base; > + u64 bus_addr_base; > size_t addr_size; > size_t page_size; > u8 bar_to_atu[PCI_STD_NUM_BARS]; > @@ -463,6 +464,14 @@ struct dw_pcie { > struct reset_control_bulk_data core_rsts[DW_PCIE_NUM_CORE_RSTS]; > struct gpio_desc *pe_rst; > bool suspended; > + /* > + * Use device tree 'ranges' property of bus node instead using > + * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not > + * reflect real hardware's behavior. In case break these platform back > + * compatibility, add below flags. Set it true if dts already correct > + * indicate bus fabric address convert. > + */ > + bool using_dtbus_info; > }; > > #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp) > > -- > 2.34.1 >