| Message ID | 20250304-axis-v1-2-ed475ab3a3ed@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: artpec6: Try to clean up artpec6_pcie_cpu_addr_fixup() | expand |
On Tue, Mar 04, 2025 at 12:49:36PM -0500, Frank Li wrote: > Remove artpec6_pcie_cpu_addr_fixup() as the DT bus fabric should provide correct > address translation. Set use_parent_dt_ranges to allow the DWC core driver to > fetch address translation from the device tree. Shouldn't we be able to detect platforms where DT doesn't describe the translation correctly? E.g., by running .cpu_addr_fixup() on a res.start value and comparing the result to the parent_bus_addr()? Then we could complain about it if they don't match. Bjorn
On Tue, Mar 04, 2025 at 01:08:16PM -0600, Bjorn Helgaas wrote: > On Tue, Mar 04, 2025 at 12:49:36PM -0500, Frank Li wrote: > > Remove artpec6_pcie_cpu_addr_fixup() as the DT bus fabric should provide correct > > address translation. Set use_parent_dt_ranges to allow the DWC core driver to > > fetch address translation from the device tree. > > Shouldn't we be able to detect platforms where DT doesn't describe the > translation correctly? E.g., by running .cpu_addr_fixup() on a > res.start value and comparing the result to the parent_bus_addr()? > Then we could complain about it if they don't match. Can't detect because: There are case, driver have not provide .cpu_addr_fixup, but dts still be wrong. such as bus@10000000 { ranges = <0xdeaddead 0x1000000 size>; pci@90000000 { reg = <...>, <0xdeaddead>; reg-names = <...>, <config>; } }; above dts can work with current driver, but parent bus address 0xdeaddead is totally fake address. We can't detect this case because no .cpu_addr_fixup() at all. Frank > > Bjorn
On Tue, Mar 04, 2025 at 03:12:11PM -0500, Frank Li wrote: > On Tue, Mar 04, 2025 at 01:08:16PM -0600, Bjorn Helgaas wrote: > > On Tue, Mar 04, 2025 at 12:49:36PM -0500, Frank Li wrote: > > > Remove artpec6_pcie_cpu_addr_fixup() as the DT bus fabric should provide correct > > > address translation. Set use_parent_dt_ranges to allow the DWC core driver to > > > fetch address translation from the device tree. > > > > Shouldn't we be able to detect platforms where DT doesn't describe the > > translation correctly? E.g., by running .cpu_addr_fixup() on a > > res.start value and comparing the result to the parent_bus_addr()? > > Then we could complain about it if they don't match. > > Can't detect because: > > There are case, driver have not provide .cpu_addr_fixup, but dts still be > wrong. such as > > bus@10000000 > { > ranges = <0xdeaddead 0x1000000 size>; > pci@90000000 { > > reg = <...>, <0xdeaddead>; > reg-names = <...>, <config>; > } > > }; > > above dts can work with current driver, but parent bus address 0xdeaddead > is totally fake address. We can't detect this case because no > .cpu_addr_fixup() at all. If there's no .cpu_addr_fixup(), primary-side ATU addresses must be identical to CPU addresses. If the DT parent bus address is different, can't we assume the DT is broken?
On Tue, Mar 04, 2025 at 02:31:46PM -0600, Bjorn Helgaas wrote: > On Tue, Mar 04, 2025 at 03:12:11PM -0500, Frank Li wrote: > > On Tue, Mar 04, 2025 at 01:08:16PM -0600, Bjorn Helgaas wrote: > > > On Tue, Mar 04, 2025 at 12:49:36PM -0500, Frank Li wrote: > > > > Remove artpec6_pcie_cpu_addr_fixup() as the DT bus fabric should provide correct > > > > address translation. Set use_parent_dt_ranges to allow the DWC core driver to > > > > fetch address translation from the device tree. > > > > > > Shouldn't we be able to detect platforms where DT doesn't describe the > > > translation correctly? E.g., by running .cpu_addr_fixup() on a > > > res.start value and comparing the result to the parent_bus_addr()? > > > Then we could complain about it if they don't match. > > > > Can't detect because: > > > > There are case, driver have not provide .cpu_addr_fixup, but dts still be > > wrong. such as > > > > bus@10000000 > > { > > ranges = <0xdeaddead 0x1000000 size>; > > pci@90000000 { > > > > reg = <...>, <0xdeaddead>; > > reg-names = <...>, <config>; > > } > > > > }; > > > > above dts can work with current driver, but parent bus address 0xdeaddead > > is totally fake address. We can't detect this case because no > > .cpu_addr_fixup() at all. > > If there's no .cpu_addr_fixup(), primary-side ATU addresses must be > identical to CPU addresses. If the DT parent bus address is > different, can't we assume the DT is broken? I think so. Frank
diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c index 234c8cbcae3af..d2a628a0fdc17 100644 --- a/drivers/pci/controller/dwc/pcie-artpec6.c +++ b/drivers/pci/controller/dwc/pcie-artpec6.c @@ -94,23 +94,6 @@ static void artpec6_pcie_writel(struct artpec6_pcie *artpec6_pcie, u32 offset, u regmap_write(artpec6_pcie->regmap, offset, val); } -static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr) -{ - struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci); - struct dw_pcie_rp *pp = &pci->pp; - struct dw_pcie_ep *ep = &pci->ep; - - switch (artpec6_pcie->mode) { - case DW_PCIE_RC_TYPE: - return cpu_addr - pp->cfg0_base; - case DW_PCIE_EP_TYPE: - return cpu_addr - ep->phys_base; - default: - dev_err(pci->dev, "UNKNOWN device type\n"); - } - return cpu_addr; -} - static int artpec6_pcie_establish_link(struct dw_pcie *pci) { struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci); @@ -134,7 +117,6 @@ static void artpec6_pcie_stop_link(struct dw_pcie *pci) } static const struct dw_pcie_ops dw_pcie_ops = { - .cpu_addr_fixup = artpec6_pcie_cpu_addr_fixup, .start_link = artpec6_pcie_establish_link, .stop_link = artpec6_pcie_stop_link, }; @@ -433,6 +415,8 @@ static int artpec6_pcie_probe(struct platform_device *pdev) platform_set_drvdata(pdev, artpec6_pcie); + pci->use_parent_dt_ranges = true; + switch (artpec6_pcie->mode) { case DW_PCIE_RC_TYPE: if (!IS_ENABLED(CONFIG_PCIE_ARTPEC6_HOST))
Remove artpec6_pcie_cpu_addr_fixup() as the DT bus fabric should provide correct address translation. Set use_parent_dt_ranges to allow the DWC core driver to fetch address translation from the device tree. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/pci/controller/dwc/pcie-artpec6.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-)