Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li <Frank.li@nxp.com>]

On Fri, Jan 17, 2025 at 08:05:16PM +0530, Manivannan Sadhasivam wrote:
>
>
> On January 17, 2025 4:19:02 AM GMT+05:30, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >On Thu, Jan 16, 2025 at 03:02:44PM -0500, Frank Li wrote:
> >> 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.
> >
> >OK, that seems reasonable.
> >
>
> Unfortunately not. Please see below.

Yes, you are right.

>
> >Where does the 0xA000_0000 PCI address come from?  I assume that's in
> >DT somewhere too?
> >
>
> No, PCI address is from host address space, hence it cannot be described in DT. That's the reason why 'addr_space' reg property is used to define outbound window region. iATU will map the host PCI address to endpoint outbound window region dynamically based on usecase (like mapping the buffer in host address space).
>
> The translation between CPU:BUS address space is a hardware property, hence using 'ranges' to describe them makes sense.
>
> But the same cannot be applied for BUS:PCI address space. Maybe the diagram had misled you thinking that PCI address is also static.

Yes, 0xA000_0000 come from PCI host side, generally allocate by dma API.
Use other channel (like registers) to told EP side how to map it.

>
> - Mani
>
> >Is there a binding in the tree that would take advantage of this patch
> >that I can look at?  arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
> >has bus@5f000000 that does this translation, but I don't see any
> >endpoint mode that uses it.

Not upstream yet. I plan do this after this patch merged.

Frank

> >
> >> > > 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?
> >
> >I can do that.
> >
> >Bjorn
> >
> >> > > > [1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-1-c4bfa5193288@nxp.com
>
> மணிவண்ணன் சதாசிவம்