| Message ID | 20250313-pci_fixup_addr-v11-6-01d2313502ab@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: Use device bus range info to cleanup RC Host/EP pci_fixup_addr() | expand |
On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote: > The 'ranges' property at PCI controller parent bus can indicate address > translation information. 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. I think you've used reg["addr_space"] to get the offset for Endpoints forever. I just noticed that through v9, you used 'ranges' to get the offset for the Root Complex (with "Add parent_bus_offset to resource_entry"), and I think v10 switched to use reg["config"] instead. I think I originally proposed the idea of "Add parent_bus_offset to resource_entry" patch, but I think it turned out to be kind of an ugly approach. Anyway, IIUC this v11 patch actually uses reg["config"] to compute the offset, not 'ranges', so we should probably update the subject and commit log to reflect that, and maybe remove the now-unused bits of the devicetree example. I do worry a little bit about the assumption that the offset of reg["config"] is the same as the offset of the other pieces. The main place we use the offset on RCs is for the ATU, and isn't the ATU in the MemSpace area at 0x8000_0000 below? It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config" offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset, but I don't know that this is guaranteed for all designs. v9: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com [PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com v10: [PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com [PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com > 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 Intermediate address (IA) here means the address just before PCIe > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can > be removed. > > Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset > is the same for all kinds of address translation. > > Just set parent_bus_offset, but doesn't use it, so no functional change > intended yet. > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > pp->io_base = pci_pio_to_address(win->res->start); > } > > + /* > + * visconti_pcie_cpu_addr_fixup() use pp->io_base, > + * so have to call dw_pcie_init_parent_bus_offset() after init > + * pp->io_base. > + */ > + ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base); > + if (ret) > + return ret; > + > /* Set default bus ops */ > bridge->ops = &dw_pcie_ops; > bridge->child_ops = &dw_child_pcie_ops; > > -- > 2.34.1 >
On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote: > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote: > > The 'ranges' property at PCI controller parent bus can indicate address > > translation information. 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. > > I think you've used reg["addr_space"] to get the offset for Endpoints > forever. Yes, it still need ranges informaiton at parent bus. bus@000 { ranges = <...>; [1] /* still need this */ pcie { ranges = <...>;[2] }; pcie-ep {}; } > > I just noticed that through v9, you used 'ranges' to get the offset > for the Root Complex (with "Add parent_bus_offset to resource_entry"), > and I think v10 switched to use reg["config"] instead. > > I think I originally proposed the idea of "Add parent_bus_offset to > resource_entry" patch, but I think it turned out to be kind of an ugly > approach. > > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the > offset, not 'ranges', so we should probably update the subject and > commit log to reflect that, and maybe remove the now-unused bits of > the devicetree example. We use reg["config"] to detect offset, but still need parent dts's ranges. There are two ranges, one is at parent pci bus [1], the other is under 'pci bus' [2]. Although use reg["config"], but still need ranges [1]. And information at ranges [2] also need be correct. The whole devicetree example also validate to help write address translate informaiton. > > I do worry a little bit about the assumption that the offset of > reg["config"] is the same as the offset of the other pieces. The main > place we use the offset on RCs is for the ATU, and isn't the ATU in > the MemSpace area at 0x8000_0000 below? No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT". Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never happen. > > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config" > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset, > but I don't know that this is guaranteed for all designs. So far, it is the same for use dwc chips. If we meet difference, we can add later. reg["config"] only simplied our implement base on the offset is the same. But whole concept is unchanged. Frank > > v9: > [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry > https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com > [PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window > https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com > > v10: > [PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback > https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com > [PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window > https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com > > > 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 Intermediate address (IA) here means the address just before PCIe > > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can > > be removed. > > > > Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset > > is the same for all kinds of address translation. > > > > Just set parent_bus_offset, but doesn't use it, so no functional change > > intended yet. > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > pp->io_base = pci_pio_to_address(win->res->start); > > } > > > > + /* > > + * visconti_pcie_cpu_addr_fixup() use pp->io_base, > > + * so have to call dw_pcie_init_parent_bus_offset() after init > > + * pp->io_base. > > + */ > > + ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base); > > + if (ret) > > + return ret; > > + > > /* Set default bus ops */ > > bridge->ops = &dw_pcie_ops; > > bridge->child_ops = &dw_child_pcie_ops; > > > > -- > > 2.34.1 > >
On Thu, Mar 13, 2025 at 06:56:17PM -0400, Frank Li wrote: > On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote: > > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote: > > > The 'ranges' property at PCI controller parent bus can indicate address > > > translation information. 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. > > > > I think you've used reg["addr_space"] to get the offset for Endpoints > > forever. > > Yes, it still need ranges informaiton at parent bus. > > bus@000 > { > ranges = <...>; [1] /* still need this */ > pcie { > ranges = <...>;[2] > }; > pcie-ep {}; > } > > > > > I just noticed that through v9, you used 'ranges' to get the offset > > for the Root Complex (with "Add parent_bus_offset to resource_entry"), > > and I think v10 switched to use reg["config"] instead. > > > > I think I originally proposed the idea of "Add parent_bus_offset to > > resource_entry" patch, but I think it turned out to be kind of an ugly > > approach. > > > > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the > > offset, not 'ranges', so we should probably update the subject and > > commit log to reflect that, and maybe remove the now-unused bits of > > the devicetree example. > > We use reg["config"] to detect offset, but still need parent dts's ranges. > There are two ranges, one is at parent pci bus [1], the other is under > 'pci bus' [2]. Beside, luckly dwc use reg["config"] to indicate config space. but dt also define ranges [2] under pcie node, which can also include 'config's space. cadence also use reg["cfg"] to do that. res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); I am not sure why both choose use reg[] instead of [2]ranges under pcie node. But the result make our situation simpler. Frank > > Although use reg["config"], but still need ranges [1]. And information at > ranges [2] also need be correct. > > The whole devicetree example also validate to help write address translate > informaiton. > > > > > I do worry a little bit about the assumption that the offset of > > reg["config"] is the same as the offset of the other pieces. The main > > place we use the offset on RCs is for the ATU, and isn't the ATU in > > the MemSpace area at 0x8000_0000 below? > > No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT". > Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never > happen. > > > > > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config" > > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset, > > but I don't know that this is guaranteed for all designs. > > So far, it is the same for use dwc chips. If we meet difference, we can > add later. > > reg["config"] only simplied our implement base on the offset is the same. > But whole concept is unchanged. > > Frank > > > > > v9: > > [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry > > https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com > > [PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window > > https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com > > > > v10: > > [PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback > > https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com > > [PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window > > https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com > > > > > 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 Intermediate address (IA) here means the address just before PCIe > > > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can > > > be removed. > > > > > > Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset > > > is the same for all kinds of address translation. > > > > > > Just set parent_bus_offset, but doesn't use it, so no functional change > > > intended yet. > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > > pp->io_base = pci_pio_to_address(win->res->start); > > > } > > > > > > + /* > > > + * visconti_pcie_cpu_addr_fixup() use pp->io_base, > > > + * so have to call dw_pcie_init_parent_bus_offset() after init > > > + * pp->io_base. > > > + */ > > > + ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base); > > > + if (ret) > > > + return ret; > > > + > > > /* Set default bus ops */ > > > bridge->ops = &dw_pcie_ops; > > > bridge->child_ops = &dw_child_pcie_ops; > > > > > > -- > > > 2.34.1 > > >
On Fri, Mar 14, 2025 at 11:21:19AM -0400, Frank Li wrote: > On Thu, Mar 13, 2025 at 06:56:17PM -0400, Frank Li wrote: > > On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote: > > > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote: > > > > The 'ranges' property at PCI controller parent bus can indicate address > > > > translation information. 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. > > > > > > I think you've used reg["addr_space"] to get the offset for Endpoints > > > forever. > > > > Yes, it still need ranges informaiton at parent bus. > > > > bus@000 > > { > > ranges = <...>; [1] /* still need this */ > > pcie { > > ranges = <...>;[2] > > }; > > pcie-ep {}; > > } Yes, of course. I'm just making the point that the subject/commit log says this patch uses 'ranges' but in fact it uses 'reg'. > > > I just noticed that through v9, you used 'ranges' to get the offset > > > for the Root Complex (with "Add parent_bus_offset to resource_entry"), > > > and I think v10 switched to use reg["config"] instead. > > > > > > I think I originally proposed the idea of "Add parent_bus_offset to > > > resource_entry" patch, but I think it turned out to be kind of an ugly > > > approach. > > > > > > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the > > > offset, not 'ranges', so we should probably update the subject and > > > commit log to reflect that, and maybe remove the now-unused bits of > > > the devicetree example. > > > > We use reg["config"] to detect offset, but still need parent dts's ranges. > > There are two ranges, one is at parent pci bus [1], the other is under > > 'pci bus' [2]. > > Beside, luckly dwc use reg["config"] to indicate config space. but dt also > define ranges [2] under pcie node, which can also include 'config's space. > > cadence also use reg["cfg"] to do that. > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > > I am not sure why both choose use reg[] instead of [2]ranges under > pcie node. But the result make our situation simpler. > > > Although use reg["config"], but still need ranges [1]. And information at > > ranges [2] also need be correct. > > > > The whole devicetree example also validate to help write address translate > > informaiton. > > > > > I do worry a little bit about the assumption that the offset of > > > reg["config"] is the same as the offset of the other pieces. The main > > > place we use the offset on RCs is for the ATU, and isn't the ATU in > > > the MemSpace area at 0x8000_0000 below? > > > > No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT". > > Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never > > happen. Minor miscommunication, I think. I didn't mean there were addresses smaller than 0x8000_0000; I meant that in the picture, MemSpace at 0x8000_0000 is below CfgSpace at 0x8ff0_0000. The important point is that CfgSpace is a separate region from MemSpace, and we're applying the CfgSpace offset to the ATU in MemSpace. I think it's OK to assume that for now. AFAICS there is nothing in devicetree that explicitly mentions the ATU input address space; it's just implicitly part of the intermediate address space described by the bus@5f000000 'ranges'. > > > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config" > > > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset, > > > but I don't know that this is guaranteed for all designs. > > > > So far, it is the same for use dwc chips. If we meet difference, we can > > add later. > > > > reg["config"] only simplied our implement base on the offset is the same. > > But whole concept is unchanged. > > > > 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>; Of course we need this 'ranges' to describe the translation between intermediate addresses and PCI bus addresses. My point is that this is not relevant to the parent bus offset we're computing in this patch. So I think for purposes of this patch, we can omit pcie@5f010000 #address-cells and everything after it. Bjorn
On Fri, Mar 14, 2025 at 05:10:59PM -0500, Bjorn Helgaas wrote: > On Fri, Mar 14, 2025 at 11:21:19AM -0400, Frank Li wrote: > > On Thu, Mar 13, 2025 at 06:56:17PM -0400, Frank Li wrote: > > > On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote: > > > > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote: > > > > > The 'ranges' property at PCI controller parent bus can indicate address > > > > > translation information. 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. > > > > > > > > I think you've used reg["addr_space"] to get the offset for Endpoints > > > > forever. > > > > > > Yes, it still need ranges informaiton at parent bus. > > > > > > bus@000 > > > { > > > ranges = <...>; [1] /* still need this */ > > > pcie { > > > ranges = <...>;[2] > > > }; > > > pcie-ep {}; > > > } > > Yes, of course. I'm just making the point that the subject/commit log > says this patch uses 'ranges' but in fact it uses 'reg'. Actaully my means: 'ranges' in subject means parent bus's ranges. Anyway how about: Use reg['config'] dettect parent_bus_offset to get rid of cpu_addr_fixup() callback > > > > > I just noticed that through v9, you used 'ranges' to get the offset > > > > for the Root Complex (with "Add parent_bus_offset to resource_entry"), > > > > and I think v10 switched to use reg["config"] instead. > > > > > > > > I think I originally proposed the idea of "Add parent_bus_offset to > > > > resource_entry" patch, but I think it turned out to be kind of an ugly > > > > approach. > > > > > > > > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the > > > > offset, not 'ranges', so we should probably update the subject and > > > > commit log to reflect that, and maybe remove the now-unused bits of > > > > the devicetree example. > > > > > > We use reg["config"] to detect offset, but still need parent dts's ranges. > > > There are two ranges, one is at parent pci bus [1], the other is under > > > 'pci bus' [2]. > > > > Beside, luckly dwc use reg["config"] to indicate config space. but dt also > > define ranges [2] under pcie node, which can also include 'config's space. > > > > cadence also use reg["cfg"] to do that. > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); > > > > I am not sure why both choose use reg[] instead of [2]ranges under > > pcie node. But the result make our situation simpler. > > > > > Although use reg["config"], but still need ranges [1]. And information at > > > ranges [2] also need be correct. > > > > > > The whole devicetree example also validate to help write address translate > > > informaiton. > > > > > > > I do worry a little bit about the assumption that the offset of > > > > reg["config"] is the same as the offset of the other pieces. The main > > > > place we use the offset on RCs is for the ATU, and isn't the ATU in > > > > the MemSpace area at 0x8000_0000 below? > > > > > > No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT". > > > Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never > > > happen. > > Minor miscommunication, I think. I didn't mean there were addresses > smaller than 0x8000_0000; I meant that in the picture, MemSpace at > 0x8000_0000 is below CfgSpace at 0x8ff0_0000. The important point is > that CfgSpace is a separate region from MemSpace, and we're applying > the CfgSpace offset to the ATU in MemSpace. > > I think it's OK to assume that for now. AFAICS there is nothing in > devicetree that explicitly mentions the ATU input address space; it's > just implicitly part of the intermediate address space described by > the bus@5f000000 'ranges'. Actually 'ranges' means address tranlation from child node to parent node. that should means address to ATU input. It will be good if docuemnt somewhere. Do you think where should document? > > > > > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config" > > > > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset, > > > > but I don't know that this is guaranteed for all designs. > > > > > > So far, it is the same for use dwc chips. If we meet difference, we can > > > add later. > > > > > > reg["config"] only simplied our implement base on the offset is the same. > > > But whole concept is unchanged. > > > > > > 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>; > > Of course we need this 'ranges' to describe the translation between > intermediate addresses and PCI bus addresses. My point is that this > is not relevant to the parent bus offset we're computing in this > patch. > > So I think for purposes of this patch, we can omit pcie@5f010000 > #address-cells and everything after it. > Okay, we can remove it. Frank > Bjorn
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 52a441662cabe..482d8ff751526 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) pp->io_base = pci_pio_to_address(win->res->start); } + /* + * visconti_pcie_cpu_addr_fixup() use pp->io_base, + * so have to call dw_pcie_init_parent_bus_offset() after init + * pp->io_base. + */ + ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base); + if (ret) + return ret; + /* Set default bus ops */ bridge->ops = &dw_pcie_ops; bridge->child_ops = &dw_child_pcie_ops;
The 'ranges' property at PCI controller parent bus can indicate address translation information. 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 Intermediate address (IA) here means the address just before PCIe controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can be removed. Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset is the same for all kinds of address translation. Just set parent_bus_offset, but doesn't use it, so no functional change intended yet. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- change from v10 to v11 - update commit message's first paragraph because switch to use 'config' to get address translation. - move dw_pcie_init_parent_bus_offset() ahead of bridge->ops = ... change from v9 to v10 - call helper dw_pcie_init_parent_bus_offset() chagne from v8 to v9 - use resoure_entry parent_bus_offset to simple code logic - add check for use_parent_dt_ranges and cpu_addr_fixup to make sure only one set. Change from v7 to v8 - Add dev_warning_once at dw_pcie_iatu_detect() to reminder cpu_addr_fixup() user to correct their code - use 'use_parent_dt_ranges' control enable use dt parent bus node ranges. - rename dw_pcie_get_untranslate_addr to dw_pcie_get_parent_addr(). - of_property_read_reg() already have comments, so needn't add more. - return actual err code from function Change from v6 to v7 Add a resource_size_t parent_bus_addr local varible to fix 32bit build error. | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410291546.kvgEWJv7-lkp@intel.com/ Chagne from v5 to v6 -add comments for of_property_read_reg(). Change from v4 to v5 - remove confused 0x5f00_0000 range in sample dts. - reorder address at above diagram. Change from v3 to v4 - none Change from v2 to v3 - %s/cpu_untranslate_addr/parent_bus_addr/g - update diagram. - improve commit message. Change from v1 to v2 - update because patch1 change get untranslate address method. - add using_dtbus_info in case break back compatibility for exited platform. --- drivers/pci/controller/dwc/pcie-designware-host.c | 9 +++++++++ 1 file changed, 9 insertions(+)