| Message ID | 20220902142202.2437658-4-daire.mcnamara@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: microchip: apportion address translation between rootport and FPGA | expand |
On Fri, Sep 02, 2022 at 03:22:01PM +0100, daire.mcnamara@microchip.com wrote: > From: Daire McNamara <daire.mcnamara@microchip.com> > > On PolarFire SoC both in- & out-bound address translations occur in two > stages. The specific translations are tightly coupled to the FPGA > designs and supplement the {dma-,}ranges properties. The first stage of > the translation is done by the FPGA fabric & the second by the root > port. > Use outbound address translation information so that the translation > tables in the root port's bridge layer can be configured to account for > any translation done by the FPGA fabric, for example, on Icicle Kit > reference design. Can you please: - Make your subject follow previous convention, i.e., at least capitalize "Add". - Add a blank line between paragraphs. Patch 2/4 also lacks this blank line. Without the separator, it's just confusing because I can't tell whether it's supposed to be a single paragraph that you forgot to wrap correctly, or two paragraphs. > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > --- > drivers/pci/controller/pcie-microchip-host.c | 59 +++++++++++++++++--- > 1 file changed, 52 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c > index 7263d175b5ad..d78745eaa4b4 100644 > --- a/drivers/pci/controller/pcie-microchip-host.c > +++ b/drivers/pci/controller/pcie-microchip-host.c > @@ -269,6 +269,8 @@ struct mc_pcie { > struct irq_domain *event_domain; > raw_spinlock_t lock; > struct mc_msi msi; > + u32 num_outbound_ranges; > + u64 outbound_range_adjustments[32]; > }; > > struct cause { > @@ -964,6 +966,37 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > } > > +static void mc_pcie_parse_outbound_range_adjustments(struct mc_pcie *port, struct device_node *np) Wrap to fit in 80 columns like the rest of the file. > +{ > + const __be32 *range; > + int range_len, num_ranges, range_size, i; > + > + range = of_get_property(np, "microchip,outbound-fabric-translation-ranges", &range_len); > + if (!range) > + return; > + > + num_ranges = of_n_addr_cells(np); > + range_size = range_len / sizeof(__be32) / num_ranges; > + > + for (i = 0; i < num_ranges; i++, range += range_size) { > + u64 pcieaddr = of_read_number(range + 1, 2); > + u64 cpuaddr = of_read_number(range + 3, 2); > + > + port->outbound_range_adjustments[i] = cpuaddr - pcieaddr; > + port->num_outbound_ranges++; > + } > +} > + > +static inline u64 mc_pcie_adjust_axi(struct mc_pcie *port, int index, u64 axi_addr) No need for this to be inline; it's not a performance path so the "inline" annotation is just clutter and makes the line too long. > +{ > + u64 offset = 0; > + > + if (index < port->num_outbound_ranges) > + offset = port->outbound_range_adjustments[index]; > + > + return axi_addr - offset; if (index < port->num_outbound_ranges) return axi_addr - port->outbound_range_adjustments[index]; return axi_addr; > +} > + > static int mc_pcie_setup_windows(struct platform_device *pdev, > struct mc_pcie *port) > { > @@ -971,14 +1004,28 @@ static int mc_pcie_setup_windows(struct platform_device *pdev, > port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; > struct pci_host_bridge *bridge = platform_get_drvdata(pdev); > struct resource_entry *entry; > + u64 axi_addr; > u64 pci_addr; > - u32 index = 1; > + u32 index = 0; > + u32 num_outbound_ranges = 0; > + > + resource_list_for_each_entry(entry, &bridge->windows) { > + if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0) Rewrap. > + num_outbound_ranges++; > + } > + > + if (port->num_outbound_ranges && port->num_outbound_ranges != num_outbound_ranges) { Ditto. > + dev_err(&pdev->dev, "Mismatches in outbound range adjustment\n"); > + return -EINVAL; > + } > > resource_list_for_each_entry(entry, &bridge->windows) { > - if (resource_type(entry->res) == IORESOURCE_MEM) { > + if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0) { Ditto. I guess "resource_type() == 0" means config space? I assume these entries came from devm_of_pci_get_host_bridge_resources()? From gen_pci_init(), I guess there's an assumption that the resource at index 0 is ECAM space? > + axi_addr = entry->res->start; > + axi_addr = mc_pcie_adjust_axi(port, index, axi_addr); How does this address adjustment work given that pci_host_common_probe() has already called gen_pci_init() to map the config space? Hopefully you can use Rob's suggestion to just use two levels of ranges instead. > pci_addr = entry->res->start - entry->offset; > mc_pcie_setup_window(bridge_base_addr, index, > - entry->res->start, pci_addr, > + axi_addr, pci_addr, > resource_size(entry->res)); > index++; > } > @@ -1005,6 +1052,8 @@ static int mc_platform_init(struct pci_config_window *cfg) > return -ENOMEM; > port->dev = dev; > > + mc_pcie_parse_outbound_range_adjustments(port, dev->of_node); > + > ret = mc_pcie_init_clks(dev); > if (ret) { > dev_err(dev, "failed to get clock resources, error %d\n", ret); > @@ -1099,10 +1148,6 @@ static int mc_platform_init(struct pci_config_window *cfg) > writel_relaxed(0, bridge_base_addr + IMASK_HOST); > writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST); > > - /* Configure Address Translation Table 0 for PCIe config space */ > - mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff, > - cfg->res.start, resource_size(&cfg->res)); > - > return mc_pcie_setup_windows(pdev, port); > } Not specifically related to *this* patch, but microchip uses the pci_ecam_ops.init() method to do a whole bunch of init completely unrelated to ECAM, which makes things really hard to follow. It would be more readable to have an mc_pcie_probe() that does the mc-specific initialization and calls pci_host_common_probe() to do the generic stuff. This is what apple_pcie_probe() does. Bjorn
Thanks Bjorn for the feedback. I'll follow up Rob's suggestion and see how I get on! On Fri, 2022-09-02 at 11:49 -0500, Bjorn Helgaas wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, Sep 02, 2022 at 03:22:01PM +0100, > daire.mcnamara@microchip.com wrote: > > From: Daire McNamara <daire.mcnamara@microchip.com> > > > > On PolarFire SoC both in- & out-bound address translations occur in > > two > > stages. The specific translations are tightly coupled to the FPGA > > designs and supplement the {dma-,}ranges properties. The first > > stage of > > the translation is done by the FPGA fabric & the second by the root > > port. > > Use outbound address translation information so that the > > translation > > tables in the root port's bridge layer can be configured to account > > for > > any translation done by the FPGA fabric, for example, on Icicle > > Kit > > reference design. > > Can you please: > > - Make your subject follow previous convention, i.e., at least > capitalize "Add". > > - Add a blank line between paragraphs. Patch 2/4 also lacks this > blank line. Without the separator, it's just confusing because I > can't tell whether it's supposed to be a single paragraph that > you > forgot to wrap correctly, or two paragraphs. > > > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > > --- > > drivers/pci/controller/pcie-microchip-host.c | 59 > > +++++++++++++++++--- > > 1 file changed, 52 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-microchip-host.c > > b/drivers/pci/controller/pcie-microchip-host.c > > index 7263d175b5ad..d78745eaa4b4 100644 > > --- a/drivers/pci/controller/pcie-microchip-host.c > > +++ b/drivers/pci/controller/pcie-microchip-host.c > > @@ -269,6 +269,8 @@ struct mc_pcie { > > struct irq_domain *event_domain; > > raw_spinlock_t lock; > > struct mc_msi msi; > > + u32 num_outbound_ranges; > > + u64 outbound_range_adjustments[32]; > > }; > > > > struct cause { > > @@ -964,6 +966,37 @@ static void mc_pcie_setup_window(void __iomem > > *bridge_base_addr, u32 index, > > writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > > } > > > > +static void mc_pcie_parse_outbound_range_adjustments(struct > > mc_pcie *port, struct device_node *np) > > Wrap to fit in 80 columns like the rest of the file. > > > +{ > > + const __be32 *range; > > + int range_len, num_ranges, range_size, i; > > + > > + range = of_get_property(np, "microchip,outbound-fabric- > > translation-ranges", &range_len); > > + if (!range) > > + return; > > + > > + num_ranges = of_n_addr_cells(np); > > + range_size = range_len / sizeof(__be32) / num_ranges; > > + > > + for (i = 0; i < num_ranges; i++, range += range_size) { > > + u64 pcieaddr = of_read_number(range + 1, 2); > > + u64 cpuaddr = of_read_number(range + 3, 2); > > + > > + port->outbound_range_adjustments[i] = cpuaddr - > > pcieaddr; > > + port->num_outbound_ranges++; > > + } > > +} > > + > > +static inline u64 mc_pcie_adjust_axi(struct mc_pcie *port, int > > index, u64 axi_addr) > > No need for this to be inline; it's not a performance path so the > "inline" annotation is just clutter and makes the line too long. > > > +{ > > + u64 offset = 0; > > + > > + if (index < port->num_outbound_ranges) > > + offset = port->outbound_range_adjustments[index]; > > + > > + return axi_addr - offset; > > if (index < port->num_outbound_ranges) > return axi_addr - port->outbound_range_adjustments[index]; > > return axi_addr; > > > +} > > + > > static int mc_pcie_setup_windows(struct platform_device *pdev, > > struct mc_pcie *port) > > { > > @@ -971,14 +1004,28 @@ static int mc_pcie_setup_windows(struct > > platform_device *pdev, > > port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; > > struct pci_host_bridge *bridge = platform_get_drvdata(pdev); > > struct resource_entry *entry; > > + u64 axi_addr; > > u64 pci_addr; > > - u32 index = 1; > > + u32 index = 0; > > + u32 num_outbound_ranges = 0; > > + > > + resource_list_for_each_entry(entry, &bridge->windows) { > > + if (resource_type(entry->res) == IORESOURCE_MEM || > > resource_type(entry->res) == 0) > > Rewrap. > > > + num_outbound_ranges++; > > + } > > + > > + if (port->num_outbound_ranges && port->num_outbound_ranges != > > num_outbound_ranges) { > > Ditto. > > > + dev_err(&pdev->dev, "Mismatches in outbound range > > adjustment\n"); > > + return -EINVAL; > > + } > > > > resource_list_for_each_entry(entry, &bridge->windows) { > > - if (resource_type(entry->res) == IORESOURCE_MEM) { > > + if (resource_type(entry->res) == IORESOURCE_MEM || > > resource_type(entry->res) == 0) { > > Ditto. > > I guess "resource_type() == 0" means config space? I assume these > entries came from devm_of_pci_get_host_bridge_resources()? From > gen_pci_init(), I guess there's an assumption that the resource at > index 0 is ECAM space? > > > + axi_addr = entry->res->start; > > + axi_addr = mc_pcie_adjust_axi(port, index, > > axi_addr); > > How does this address adjustment work given that > pci_host_common_probe() has already called gen_pci_init() to map the > config space? > > Hopefully you can use Rob's suggestion to just use two levels of > ranges instead. > > > pci_addr = entry->res->start - entry->offset; > > mc_pcie_setup_window(bridge_base_addr, index, > > - entry->res->start, > > pci_addr, > > + axi_addr, pci_addr, > > resource_size(entry- > > >res)); > > index++; > > } > > @@ -1005,6 +1052,8 @@ static int mc_platform_init(struct > > pci_config_window *cfg) > > return -ENOMEM; > > port->dev = dev; > > > > + mc_pcie_parse_outbound_range_adjustments(port, dev->of_node); > > + > > ret = mc_pcie_init_clks(dev); > > if (ret) { > > dev_err(dev, "failed to get clock resources, error > > %d\n", ret); > > @@ -1099,10 +1148,6 @@ static int mc_platform_init(struct > > pci_config_window *cfg) > > writel_relaxed(0, bridge_base_addr + IMASK_HOST); > > writel_relaxed(GENMASK(31, 0), bridge_base_addr + > > ISTATUS_HOST); > > > > - /* Configure Address Translation Table 0 for PCIe config > > space */ > > - mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & > > 0xffffffff, > > - cfg->res.start, resource_size(&cfg- > > >res)); > > - > > return mc_pcie_setup_windows(pdev, port); > > } > > Not specifically related to *this* patch, but microchip uses the > pci_ecam_ops.init() method to do a whole bunch of init completely > unrelated to ECAM, which makes things really hard to follow. > > It would be more readable to have an mc_pcie_probe() that does the > mc-specific initialization and calls pci_host_common_probe() to do > the > generic stuff. This is what apple_pcie_probe() does. > > Bjorn
diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c index 7263d175b5ad..d78745eaa4b4 100644 --- a/drivers/pci/controller/pcie-microchip-host.c +++ b/drivers/pci/controller/pcie-microchip-host.c @@ -269,6 +269,8 @@ struct mc_pcie { struct irq_domain *event_domain; raw_spinlock_t lock; struct mc_msi msi; + u32 num_outbound_ranges; + u64 outbound_range_adjustments[32]; }; struct cause { @@ -964,6 +966,37 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); } +static void mc_pcie_parse_outbound_range_adjustments(struct mc_pcie *port, struct device_node *np) +{ + const __be32 *range; + int range_len, num_ranges, range_size, i; + + range = of_get_property(np, "microchip,outbound-fabric-translation-ranges", &range_len); + if (!range) + return; + + num_ranges = of_n_addr_cells(np); + range_size = range_len / sizeof(__be32) / num_ranges; + + for (i = 0; i < num_ranges; i++, range += range_size) { + u64 pcieaddr = of_read_number(range + 1, 2); + u64 cpuaddr = of_read_number(range + 3, 2); + + port->outbound_range_adjustments[i] = cpuaddr - pcieaddr; + port->num_outbound_ranges++; + } +} + +static inline u64 mc_pcie_adjust_axi(struct mc_pcie *port, int index, u64 axi_addr) +{ + u64 offset = 0; + + if (index < port->num_outbound_ranges) + offset = port->outbound_range_adjustments[index]; + + return axi_addr - offset; +} + static int mc_pcie_setup_windows(struct platform_device *pdev, struct mc_pcie *port) { @@ -971,14 +1004,28 @@ static int mc_pcie_setup_windows(struct platform_device *pdev, port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; struct pci_host_bridge *bridge = platform_get_drvdata(pdev); struct resource_entry *entry; + u64 axi_addr; u64 pci_addr; - u32 index = 1; + u32 index = 0; + u32 num_outbound_ranges = 0; + + resource_list_for_each_entry(entry, &bridge->windows) { + if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0) + num_outbound_ranges++; + } + + if (port->num_outbound_ranges && port->num_outbound_ranges != num_outbound_ranges) { + dev_err(&pdev->dev, "Mismatches in outbound range adjustment\n"); + return -EINVAL; + } resource_list_for_each_entry(entry, &bridge->windows) { - if (resource_type(entry->res) == IORESOURCE_MEM) { + if (resource_type(entry->res) == IORESOURCE_MEM || resource_type(entry->res) == 0) { + axi_addr = entry->res->start; + axi_addr = mc_pcie_adjust_axi(port, index, axi_addr); pci_addr = entry->res->start - entry->offset; mc_pcie_setup_window(bridge_base_addr, index, - entry->res->start, pci_addr, + axi_addr, pci_addr, resource_size(entry->res)); index++; } @@ -1005,6 +1052,8 @@ static int mc_platform_init(struct pci_config_window *cfg) return -ENOMEM; port->dev = dev; + mc_pcie_parse_outbound_range_adjustments(port, dev->of_node); + ret = mc_pcie_init_clks(dev); if (ret) { dev_err(dev, "failed to get clock resources, error %d\n", ret); @@ -1099,10 +1148,6 @@ static int mc_platform_init(struct pci_config_window *cfg) writel_relaxed(0, bridge_base_addr + IMASK_HOST); writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST); - /* Configure Address Translation Table 0 for PCIe config space */ - mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff, - cfg->res.start, resource_size(&cfg->res)); - return mc_pcie_setup_windows(pdev, port); }