On Fri, Jan 17, 2025 at 10:42:37AM -0500, Frank Li wrote:
> On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 16, 2025 at 05:14:00PM -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?
>
> All should use "intermediate bus address", RC side only call it here. EP
> side is here
> https://lore.kernel.org/imx/Z4p0fUAK1ONNjLst@lizhi-Precision-Tower-5810/T/#t
Currently dw_pcie_prog_outbound_atu() uses atu->cpu_addr, calls
ops->cpu_addr_fixup() if defined, and writes cpu_addr to
PCIE_ATU_LOWER_BASE/PCIE_ATU_UPPER_BASE.
The callers of dw_pcie_prog_outbound_atu() I see are:
dw_pcie_ep_outbound_atu
atu.cpu_addr came from dw_pcie_ep_map_addr(), so it's a CPU
address. Fixed by [1], which reads ep->bus_addr_base from the
"addr_space" 'reg' property.
dw_pcie_other_conf_map_bus
atu.cpu_addr came from pp->cfg0_base, set by dw_pcie_host_init()
to a CPU address (the "config" resource). Fixed by [2], which
overwrites pp->cfg0_base with the address from the "config" 'reg'
property if 'use_parent_dt_ranges' is set.
dw_pcie_rd_other_conf
dw_pcie_wr_other_conf
dw_pcie_prog_outbound_atu() only called if pp->cfg0_io_shared,
after an ECAM map via dw_pcie_other_conf_map_bus() and subsequent
successful access; atu.cpu_addr came from pp->io_base, set by
dw_pcie_host_init() from pci_pio_to_address(), which I'm pretty
sure returns a CPU address.
So this still looks wrong to me. In addition, I think doing the
ATU setup in *_map() and restore in *rd/wr_other_conf() is ugly
and looks unreliable.
dw_pcie_iatu_setup
atu.cpu_addr came from the struct resource in pp->bridge->windows,
so it's a CPU address. Also fixed by [2], which overwrites
atu.cpu_addr with the address from dw_pcie_get_parent_addr(), which
iterates through the PCI controller's 'ranges' property and
returns the range.parent_bus_addr.
dw_pcie_pme_turn_off
atu.cpu_addr came from pp.msg_res, set by
dw_pcie_host_request_msg_tlp_res() to a CPU address at the end of
the first MMIO bridge window. This one also looks wrong to me.
[1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-3-c4bfa5193288@xxxxxxx
[2] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-2-c4bfa5193288@xxxxxxx
> > Since dw_pcie_prog_outbound_atu() is the only dwc caller, maybe the
> > parent_bus_addr change should go *there* instead of in the callers?
>
> I am not sure I understand your means.
>
> EP and RC parts need call dw_pcie_prog_outbound_atu(). EP and RC use
> difference method to get outbound windows information. So can't move it
> into dw_pcie_prog_outbound_atu().
Yes, I think I see what you mean. In the Endpoint case, the iATU
input comes from the "addr_space" 'reg' property. In the Root Complex
case, it comes from the parent bus address of the 'ranges' property.
Bjorn
