On Thu, Jan 16, 2025 at 05:13:58PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > parent_bus_addr in struct of_range can indicate address information just
> > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > map. See below diagram:
> >
> > ┌─────────┐ ┌────────────┐
> > ┌─────┐ │ │ IA: 0x8ff8_0000 │ │
> > │ CPU ├───►│ ┌────►├─────────────────┐ │ PCI │
> > └─────┘ │ │ │ IA: 0x8ff0_0000 │ │ │
> > CPU Addr │ │ ┌─►├─────────────┐ │ │ Controller │
> > 0x7ff8_0000─┼───┘ │ │ │ │ │ │
> > │ │ │ │ │ │ │ PCI Addr
> > 0x7ff0_0000─┼──────┘ │ │ └──► IOSpace ─┼────────────►
> > │ │ │ │ │ 0
> > 0x7000_0000─┼────────►├─────────┐ │ │ │
> > └─────────┘ │ └──────► CfgSpace ─┼────────────►
> > BUS Fabric │ │ │ 0
> > │ │ │
> > └──────────► MemSpace ─┼────────────►
> > IA: 0x8000_0000 │ │ 0x8000_0000
> > └────────────┘
> >
> > bus@5f000000 {
> > compatible = "simple-bus";
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >
> > pcie@5f010000 {
> > compatible = "fsl,imx8q-pcie";
> > reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > reg-names = "dbi", "config";
> > #address-cells = <3>;
> > #size-cells = <2>;
> > device_type = "pci";
> > bus-range = <0x00 0xff>;
> > ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > ...
> > };
> > };
> >
> > Term internal address (IA) here means the address just before PCIe
> > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > be removed.
>
> > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >
> > atu.index = i;
> > atu.type = PCIE_ATU_TYPE_MEM;
> > - atu.cpu_addr = entry->res->start;
> > + parent_bus_addr = entry->res->start;
> > atu.pci_addr = entry->res->start - entry->offset;
> >
> > + ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > + if (ret)
> > + return ret;
> > +
> > + atu.cpu_addr = parent_bus_addr;
>
> Here you set atu.cpu_addr to the intermediate bus address instead
> of the CPU physical address before calling
> dw_pcie_prog_outbound_atu().
>
> But what about other callers of dw_pcie_prog_outbound_atu()? Don't
> all of them need to use the intermediate bus address?
EP side change in follow patches. RC side only here call
dw_pcie_prog_outbound_atu()
>
> Maybe struct dw_pcie_ob_atu_cfg.cpu_addr should be renamed since it is
> not necessarily a CPU physical address?
Yes, we can improve it later.
>
> > + if (pci->ops && pci->ops->cpu_addr_fixup) {
> > + /*
> > + * If the parent 'ranges' property in DT correctly describes
> > + * the address translation, cpu_addr_fixup() callback is not
> > + * needed.
> > + */
> > + dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n");
> > + }
>
> I kinda wish this warning were in a separate patch because it will be
> a little cleaner if we ever want to revert or remove the warning.
I don't think it is possible to only revert one print warning patch in
future. I think only two path: one revert whole solution, the other is fix
all dts and remove whole cpu_addr_fixup.
Frank
