On Thu, Jan 16, 2025 at 01:45:58PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 16, 2025 at 01:04:16PM -0500, Frank Li wrote:
> > On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
> > > On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
> > > > 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 └────────────────┘│
> > > > └───────────────────────────────────────────────┘
> > > >
> > > > Use 'ranges' property in DT to configure the iATU outbound window address.
> > > > The bus fabric generally passes the same address to the PCIe EP controller,
> > > > but some bus fabrics map the address before sending it to the PCIe EP
> > > > controller.
> > > >
> > > > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> > > > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> > > > input address and map to PCI address 0xA000_0000.
> > > >
> > > > 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 = <0x80000000 0x10000000>;
> > > > reg-names ="addr_space";
> > > > ...
> > > > };
> > > > ...
> > > > };
> > > >
> > > > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> > > > address.
> > >
> > > Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
> > > describe the translation for the window from bus addr 0x8000_0000 to
> > > PCI addr 0xA000_0000?
> >
> > Needn't 'ranges' under pcie-ep@5f010000 because history reason. DWC use
> > reg-names "addr_space" descript outbound windows space.
>
> If reg-name "addr_space" is used instead of 'ranges' for some
> historical reason, we should mention that in the commit log so people
> don't assume that this difference is the way it's *supposed* to be
> done.
How about add comments after
reg-names ="addr_space"; // Indicate EP outbound windows space instead use
ranges by histortical reason.
>
> I only see "addr_space" mentioned in
> Documentation/devicetree/bindings/pci/*-ep.yaml, so I assume
> this "addr_space" usage only applies to endpoints?
Yes, "addr_space" usage only applies to endpoints.
>
> > > > 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().
> > >
> > > Why is this different from [1], where parent_bus_addr comes from the
> > > 'ranges' property? Isn't this the same exact kind of address
> > > translation for both RC and EP mode?
> >
> > The method is the same, but space means is difference.
> >
> > RC side:
> > regs, 1: controller register, 2: config space, (although it should be
> > in ranges)
> > ranges, (IO range and Memory range).
> >
> > EP side:
> > regs, 1: controller register, 2: outbound windows space.("addr_space")
> >
> > All regs need call of_property_read_reg() to get untranslated address.
> > ranges: use "parent_bus_addr" in [1].
>
> I think we should at least use the same name ("parent_bus_addr", not
> "bus_addr_base") and probably also figure out a wrapper or similar way
> to use 'ranges' for future endpoint drivers and fall back to
> "addr_space" for DWC.
Okay for name parent_bus_addr.
Do you need me to respin it? Or you change it by yourself?
Frank
>
> > > [1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-1-c4bfa5193288@xxxxxxx
>
> > > > +++ 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];
