On Fri, Oct 25, 2024 at 05:31:02PM -0500, Bjorn Helgaas wrote:
> 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?
Yes, it is fixed by hardware.
>
> 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?
0x8000_0000 -> 0xA000_0000 is programmed by Linux RC side host driver tell
Linux EP side driver how to map it. 0x8000_0000 is fixed MDIO space.
>
> AFAICT, the "reg = <0x5f010000 0x00010000>" part is not relevant here.
Yes.
>
> 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?
>
There are 8 ATU provide by DWC PCI controller. Bus only provide a big
PCI map windows for example, 0x8000_0000..0x9000_0000.
we can slip it up to 8 small region, such as each 64K, each region can
map to any PCI address by ATU.
0x8001_0000 -> 0xA000_00000
0x8002_0000 -> 0xB100_00000
0x8003_0000 -> 0xD000_00000
...
This part EPC driver already handle it. EPC driver only need know
"addr_space" information by above sample dts.
> And also that there's only one layer of Bus Fabric address
> translation?
Doesn't metter, but PCI controller only care about closest one. Others
translation is tranparent to drivers. As above example, PCI EP controller
only care about input address is 0x8000_0000, and cpu send out address is
0x7000_0000. Even there are more translation,
0x7000_0000-> 0x9000_0000->0x8000_0000,
^^^^^^ ^^^^
PCI EP controller only care input (0x7000_0000) and output (0x9000_0000),
don't care any internal translation (0x9000_0000).
> 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.
Some cdns also have the same situation.
>
> > '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.
yes, cpu_addr is not good name altough it is correct for most system.
>
> > 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.
Yes, see below
ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> So I suppose "addr_space" contains a
> fixed parent-bus-address, and is setting the child (PCI) bus address,
> right?
"addr_space" hold PCI EP outbound MDIO space, which is parent-bus-address.
it is confused if called as PCI bus address, which most likely the address
after ATU covert.
bus@5f000000 {
compatible = "simple-bus";
ranges = <0x80000000 0x0 0x70000000 0x10000000>;
pcie-ep@5f010000 {
reg = <0x5f010000 0x00010000>,
<0x80000000 0x10000000>;
reg-names = "dbi", "addr_space";
...
};
History reasion, PCI EP use reg-names "addr_space" indicate outbound
windows informaiton.
>
> 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
> >
