| Message ID | 20240612112213.2734748-3-daire.mcnamara@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix address translations on MPFS PCIe controller | expand |
On Wed, 12 Jun 2024, daire.mcnamara@microchip.com wrote: > From: Daire McNamara <daire.mcnamara@microchip.com> > > On Microchip PolarFire SoC the PCIe Root Port can be behind one of three > general purpose Fabric Interface Controller (FIC) buses that encapsulates > an AXI-S bus. Depending on which FIC(s) the Root Port is connected > through to CPU space, and what address translation is done by that FIC, > the Root Port driver's inbound address translation may vary. > > For all current supported designs and all future expected designs, > inbound address translation done by a FIC on PolarFire SoC varies > depending on whether PolarFire SoC in operating in dma-coherent mode or > dma-noncoherent mode. > > The setup of the outbound address translation tables in the root port > driver only needs to handle these two cases. > > Setup the inbound address translation tables to one of two address > translations, depending on whether the rootport is marked as dma-coherent or > dma-noncoherent. > > Fixes: 6f15a9c9f941 ("PCI: microchip: Add Microchip Polarfire PCIe controller driver") > > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > --- > drivers/pci/controller/pcie-microchip-host.c | 97 +++++++++++++++++++- > 1 file changed, 92 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c > index 853adce24492..d5021333e2aa 100644 > --- a/drivers/pci/controller/pcie-microchip-host.c > +++ b/drivers/pci/controller/pcie-microchip-host.c > @@ -30,6 +30,9 @@ > #define MC_PCIE_BRIDGE_ADDR (MC_PCIE1_BRIDGE_ADDR) > #define MC_PCIE_CTRL_ADDR (MC_PCIE1_CTRL_ADDR) > > +#define MC_MAX_NUM_INBOUND_WINDOWS 8 > +#define MPFS_NC_BOUNCE_ADDR 0x80000000 > + > /* PCIe Bridge Phy Regs */ > #define PCIE_PCI_IRQ_DW0 0xa8 > #define MSIX_CAP_MASK BIT(31) > @@ -105,6 +108,7 @@ > #define ATR0_AXI4_SLV0_TRSL_PARAM 0x810u > #define PCIE_TX_RX_INTERFACE 0x00000000u > #define PCIE_CONFIG_INTERFACE 0x00000001u > +#define TRSL_ID_AXI4_MASTER_0 0x00000004u > > #define ATR_ENTRY_SIZE 32 > > @@ -931,6 +935,89 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port) > return mc_allocate_msi_domains(port); > } > > +static void mc_pcie_setup_inbound_atr(int window_index, u64 axi_addr, u64 pcie_addr, size_t size) > +{ > + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; > + u32 table_offset = window_index * ATR_ENTRY_SIZE; > + u32 atr_sz; > + u32 val; > + > + atr_sz = ilog2(size) - 1; > + atr_sz &= GENMASK(5, 0); > + val = lower_32_bits(pcie_addr) & GENMASK(31, 12); ALIGN_DOWN(, SZ_xx) ? > + val |= (atr_sz << ATR_SIZE_SHIFT); This looks like a named define + FIELD_PREP() would be more appropriate here. > + val |= ATR_IMPL_ENABLE; > + writel(val, bridge_base_addr + table_offset + ATR0_PCIE_WIN0_SRCADDR_PARAM); > + > + writel(upper_32_bits(pcie_addr), bridge_base_addr + table_offset + > + ATR0_PCIE_WIN0_SRC_ADDR); > + > + writel(lower_32_bits(axi_addr), bridge_base_addr + table_offset + > + ATR0_PCIE_WIN0_TRSL_ADDR_LSB); > + writel(upper_32_bits(axi_addr), bridge_base_addr + table_offset + > + ATR0_PCIE_WIN0_TRSL_ADDR_UDW); > + > + writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr + table_offset + > + ATR0_PCIE_WIN0_TRSL_PARAM); Having a table_addr local variable instead would make these much less repetitive and shorter. > +} > + > +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *dn = dev->of_node; > + struct of_range_parser parser; > + struct of_range range; > + int atr_index = 0; > + > + /* > + * MPFS PCIe root port is 32-bit only, behind a Fabric Interface > + * Controller FPGA logic block which contains the AXI-S interface. > + * > + * From the point of view of the PCIe root port, There are only > + * two supported Root Port configurations > + * > + * Configuration 1: for use with fully coherent designs; supports a > + * window from 0x0 (CPU space) to specified PCIe space. > + * > + * Configuration 2: for use with non-coherent designs; supports two > + * 1 Gb wide windows to CPU space; one mapping cpu space 0 to pcie > + * space 0x80000000 and mapping cpu space 0x40000000 to pcie > + * space 0xc0000000. This cfg needs two windows because of how > + * the MSI space is allocated in the AXI-S range on MPFS. > + * > + * The FIC interface outside the PCIe block *must* complete the inbound > + * address translation as per MCHP MPFS FPGA design guidelines. > + */ > + if (device_property_read_bool(dev, "dma-noncoherent")) { > + /* > + * Always need same two tables in this case. Need two tables > + * due to hardware interactions between address and size. > + */ > + mc_pcie_setup_inbound_atr(0, 0, MPFS_NC_BOUNCE_ADDR, SZ_1G); > + mc_pcie_setup_inbound_atr(1, SZ_1G, MPFS_NC_BOUNCE_ADDR + SZ_1G, SZ_1G); > + } else { > + /* Find any dma-ranges */ > + if (of_pci_dma_range_parser_init(&parser, dn)) { > + /* No dma-range property - setup default */ > + mc_pcie_setup_inbound_atr(0, 0, 0, SZ_4G); > + return 0; > + } > + > + for_each_of_range(&parser, &range) { > + if (atr_index >= MC_MAX_NUM_INBOUND_WINDOWS) { > + dev_err(dev, "too many inbound ranges; %d available tables\n", > + MC_MAX_NUM_INBOUND_WINDOWS); > + return -EINVAL; You don't need to rollback anything when this error is encountered? > + } > + mc_pcie_setup_inbound_atr(atr_index, 0, range.pci_addr, range.size); > + atr_index++; > + } > + } > + > + return 0; > +} > + > static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > phys_addr_t axi_addr, phys_addr_t pci_addr, > u64 size) > @@ -962,11 +1049,6 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > val = upper_32_bits(pci_addr); > writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > ATR0_AXI4_SLV0_TRSL_ADDR_UDW); > - > - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); > - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > } > > static int mc_pcie_setup_windows(struct platform_device *pdev, > @@ -1129,6 +1211,11 @@ static int mc_platform_init(struct pci_config_window *cfg) > if (ret) > return ret; > > + /* Configure inbound translation tables */ IMO, this comment adds 0 value over what the code tells all by itself so it would be best to drop it. > + ret = mc_pcie_setup_inbound_ranges(pdev, port); > + if (ret) > + return ret; > + > /* Address translation is up; safe to enable interrupts */ > ret = mc_init_interrupts(pdev, port); > if (ret) >
On Fri, Jun 14, 2024 at 04:29:47PM +0300, Ilpo Järvinen wrote: > On Wed, 12 Jun 2024, daire.mcnamara@microchip.com wrote: > > > From: Daire McNamara <daire.mcnamara@microchip.com> > > > > On Microchip PolarFire SoC the PCIe Root Port can be behind one of three > > general purpose Fabric Interface Controller (FIC) buses that encapsulates > > an AXI-S bus. Depending on which FIC(s) the Root Port is connected > > through to CPU space, and what address translation is done by that FIC, > > the Root Port driver's inbound address translation may vary. > > > > For all current supported designs and all future expected designs, > > inbound address translation done by a FIC on PolarFire SoC varies > > depending on whether PolarFire SoC in operating in dma-coherent mode or > > dma-noncoherent mode. > > > > The setup of the outbound address translation tables in the root port > > driver only needs to handle these two cases. > > > > Setup the inbound address translation tables to one of two address > > translations, depending on whether the rootport is marked as dma-coherent or > > dma-noncoherent. > > > > Fixes: 6f15a9c9f941 ("PCI: microchip: Add Microchip Polarfire PCIe controller driver") > > > > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > > --- > > drivers/pci/controller/pcie-microchip-host.c | 97 +++++++++++++++++++- > > 1 file changed, 92 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c > > index 853adce24492..d5021333e2aa 100644 > > --- a/drivers/pci/controller/pcie-microchip-host.c > > +++ b/drivers/pci/controller/pcie-microchip-host.c > > @@ -30,6 +30,9 @@ > > #define MC_PCIE_BRIDGE_ADDR (MC_PCIE1_BRIDGE_ADDR) > > #define MC_PCIE_CTRL_ADDR (MC_PCIE1_CTRL_ADDR) > > > > +#define MC_MAX_NUM_INBOUND_WINDOWS 8 > > +#define MPFS_NC_BOUNCE_ADDR 0x80000000 > > + > > /* PCIe Bridge Phy Regs */ > > #define PCIE_PCI_IRQ_DW0 0xa8 > > #define MSIX_CAP_MASK BIT(31) > > @@ -105,6 +108,7 @@ > > #define ATR0_AXI4_SLV0_TRSL_PARAM 0x810u > > #define PCIE_TX_RX_INTERFACE 0x00000000u > > #define PCIE_CONFIG_INTERFACE 0x00000001u > > +#define TRSL_ID_AXI4_MASTER_0 0x00000004u > > > > #define ATR_ENTRY_SIZE 32 > > > > @@ -931,6 +935,89 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port) > > return mc_allocate_msi_domains(port); > > } > > > > +static void mc_pcie_setup_inbound_atr(int window_index, u64 axi_addr, u64 pcie_addr, size_t size) > > +{ > > + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; > > + u32 table_offset = window_index * ATR_ENTRY_SIZE; > > + u32 atr_sz; > > + u32 val; > > + > > + atr_sz = ilog2(size) - 1; > > + atr_sz &= GENMASK(5, 0); > > + val = lower_32_bits(pcie_addr) & GENMASK(31, 12); > > ALIGN_DOWN(, SZ_xx) ? Nice, I'll pop that into v4 > > > + val |= (atr_sz << ATR_SIZE_SHIFT); > > This looks like a named define + FIELD_PREP() would be more appropriate > here. And, also nice. Again, I'll pop that into v4. > > > + val |= ATR_IMPL_ENABLE; > > + writel(val, bridge_base_addr + table_offset + ATR0_PCIE_WIN0_SRCADDR_PARAM); > > + > > + writel(upper_32_bits(pcie_addr), bridge_base_addr + table_offset + > > + ATR0_PCIE_WIN0_SRC_ADDR); > > + > > + writel(lower_32_bits(axi_addr), bridge_base_addr + table_offset + > > + ATR0_PCIE_WIN0_TRSL_ADDR_LSB); > > + writel(upper_32_bits(axi_addr), bridge_base_addr + table_offset + > > + ATR0_PCIE_WIN0_TRSL_ADDR_UDW); > > + > > + writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr + table_offset + > > + ATR0_PCIE_WIN0_TRSL_PARAM); > > Having a table_addr local variable instead would make these much less > repetitive and shorter. Agreed. v4 > > > +} > > + > > +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *dn = dev->of_node; > > + struct of_range_parser parser; > > + struct of_range range; > > + int atr_index = 0; > > + > > + /* > > + * MPFS PCIe root port is 32-bit only, behind a Fabric Interface > > + * Controller FPGA logic block which contains the AXI-S interface. > > + * > > + * From the point of view of the PCIe root port, There are only > > + * two supported Root Port configurations > > + * > > + * Configuration 1: for use with fully coherent designs; supports a > > + * window from 0x0 (CPU space) to specified PCIe space. > > + * > > + * Configuration 2: for use with non-coherent designs; supports two > > + * 1 Gb wide windows to CPU space; one mapping cpu space 0 to pcie > > + * space 0x80000000 and mapping cpu space 0x40000000 to pcie > > + * space 0xc0000000. This cfg needs two windows because of how > > + * the MSI space is allocated in the AXI-S range on MPFS. > > + * > > + * The FIC interface outside the PCIe block *must* complete the inbound > > + * address translation as per MCHP MPFS FPGA design guidelines. > > + */ > > + if (device_property_read_bool(dev, "dma-noncoherent")) { > > + /* > > + * Always need same two tables in this case. Need two tables > > + * due to hardware interactions between address and size. > > + */ > > + mc_pcie_setup_inbound_atr(0, 0, MPFS_NC_BOUNCE_ADDR, SZ_1G); > > + mc_pcie_setup_inbound_atr(1, SZ_1G, MPFS_NC_BOUNCE_ADDR + SZ_1G, SZ_1G); > > + } else { > > + /* Find any dma-ranges */ > > + if (of_pci_dma_range_parser_init(&parser, dn)) { > > + /* No dma-range property - setup default */ > > + mc_pcie_setup_inbound_atr(0, 0, 0, SZ_4G); > > + return 0; > > + } > > + > > + for_each_of_range(&parser, &range) { > > + if (atr_index >= MC_MAX_NUM_INBOUND_WINDOWS) { > > + dev_err(dev, "too many inbound ranges; %d available tables\n", > > + MC_MAX_NUM_INBOUND_WINDOWS); > > + return -EINVAL; > > You don't need to rollback anything when this error is encountered? This one, I can't see how the driver can do anything more useful than just give up - the address translation tables are now no better nor no worse than they were before; i.e. the root port won't work properly. I can't see any deploment-time recovery of the root port from this. For general end-users of PolarFire systems, I'd expect they're going to see a working system that has been brought up/debugged by someone doing a bsp. So, in my mind, this is a case only a developer bringing up a new bsp on PolarFire SoC needs to know about and all the driver can do is a notify that their proposed address translations won't work as they imagine (as they've run out of tables). > > > + } > > + mc_pcie_setup_inbound_atr(atr_index, 0, range.pci_addr, range.size); > > + atr_index++; > > + } > > + } > > + > > + return 0; > > +} > > + > > static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > > phys_addr_t axi_addr, phys_addr_t pci_addr, > > u64 size) > > @@ -962,11 +1049,6 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > > val = upper_32_bits(pci_addr); > > writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > > ATR0_AXI4_SLV0_TRSL_ADDR_UDW); > > - > > - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > > - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); > > - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > > - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > > } > > > > static int mc_pcie_setup_windows(struct platform_device *pdev, > > @@ -1129,6 +1211,11 @@ static int mc_platform_init(struct pci_config_window *cfg) > > if (ret) > > return ret; > > > > + /* Configure inbound translation tables */ > > IMO, this comment adds 0 value over what the code tells all by itself so > it would be best to drop it. Dropped > > > + ret = mc_pcie_setup_inbound_ranges(pdev, port); > > + if (ret) > > + return ret; > > + > > /* Address translation is up; safe to enable interrupts */ > > ret = mc_init_interrupts(pdev, port); > > if (ret) > > > > -- > i. > >
diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c index 853adce24492..d5021333e2aa 100644 --- a/drivers/pci/controller/pcie-microchip-host.c +++ b/drivers/pci/controller/pcie-microchip-host.c @@ -30,6 +30,9 @@ #define MC_PCIE_BRIDGE_ADDR (MC_PCIE1_BRIDGE_ADDR) #define MC_PCIE_CTRL_ADDR (MC_PCIE1_CTRL_ADDR) +#define MC_MAX_NUM_INBOUND_WINDOWS 8 +#define MPFS_NC_BOUNCE_ADDR 0x80000000 + /* PCIe Bridge Phy Regs */ #define PCIE_PCI_IRQ_DW0 0xa8 #define MSIX_CAP_MASK BIT(31) @@ -105,6 +108,7 @@ #define ATR0_AXI4_SLV0_TRSL_PARAM 0x810u #define PCIE_TX_RX_INTERFACE 0x00000000u #define PCIE_CONFIG_INTERFACE 0x00000001u +#define TRSL_ID_AXI4_MASTER_0 0x00000004u #define ATR_ENTRY_SIZE 32 @@ -931,6 +935,89 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port) return mc_allocate_msi_domains(port); } +static void mc_pcie_setup_inbound_atr(int window_index, u64 axi_addr, u64 pcie_addr, size_t size) +{ + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; + u32 table_offset = window_index * ATR_ENTRY_SIZE; + u32 atr_sz; + u32 val; + + atr_sz = ilog2(size) - 1; + atr_sz &= GENMASK(5, 0); + + val = lower_32_bits(pcie_addr) & GENMASK(31, 12); + val |= (atr_sz << ATR_SIZE_SHIFT); + val |= ATR_IMPL_ENABLE; + writel(val, bridge_base_addr + table_offset + ATR0_PCIE_WIN0_SRCADDR_PARAM); + + writel(upper_32_bits(pcie_addr), bridge_base_addr + table_offset + + ATR0_PCIE_WIN0_SRC_ADDR); + + writel(lower_32_bits(axi_addr), bridge_base_addr + table_offset + + ATR0_PCIE_WIN0_TRSL_ADDR_LSB); + writel(upper_32_bits(axi_addr), bridge_base_addr + table_offset + + ATR0_PCIE_WIN0_TRSL_ADDR_UDW); + + writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr + table_offset + + ATR0_PCIE_WIN0_TRSL_PARAM); +} + +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port) +{ + struct device *dev = &pdev->dev; + struct device_node *dn = dev->of_node; + struct of_range_parser parser; + struct of_range range; + int atr_index = 0; + + /* + * MPFS PCIe root port is 32-bit only, behind a Fabric Interface + * Controller FPGA logic block which contains the AXI-S interface. + * + * From the point of view of the PCIe root port, There are only + * two supported Root Port configurations + * + * Configuration 1: for use with fully coherent designs; supports a + * window from 0x0 (CPU space) to specified PCIe space. + * + * Configuration 2: for use with non-coherent designs; supports two + * 1 Gb wide windows to CPU space; one mapping cpu space 0 to pcie + * space 0x80000000 and mapping cpu space 0x40000000 to pcie + * space 0xc0000000. This cfg needs two windows because of how + * the MSI space is allocated in the AXI-S range on MPFS. + * + * The FIC interface outside the PCIe block *must* complete the inbound + * address translation as per MCHP MPFS FPGA design guidelines. + */ + if (device_property_read_bool(dev, "dma-noncoherent")) { + /* + * Always need same two tables in this case. Need two tables + * due to hardware interactions between address and size. + */ + mc_pcie_setup_inbound_atr(0, 0, MPFS_NC_BOUNCE_ADDR, SZ_1G); + mc_pcie_setup_inbound_atr(1, SZ_1G, MPFS_NC_BOUNCE_ADDR + SZ_1G, SZ_1G); + } else { + /* Find any dma-ranges */ + if (of_pci_dma_range_parser_init(&parser, dn)) { + /* No dma-range property - setup default */ + mc_pcie_setup_inbound_atr(0, 0, 0, SZ_4G); + return 0; + } + + for_each_of_range(&parser, &range) { + if (atr_index >= MC_MAX_NUM_INBOUND_WINDOWS) { + dev_err(dev, "too many inbound ranges; %d available tables\n", + MC_MAX_NUM_INBOUND_WINDOWS); + return -EINVAL; + } + mc_pcie_setup_inbound_atr(atr_index, 0, range.pci_addr, range.size); + atr_index++; + } + } + + return 0; +} + static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, phys_addr_t axi_addr, phys_addr_t pci_addr, u64 size) @@ -962,11 +1049,6 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, val = upper_32_bits(pci_addr); writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + ATR0_AXI4_SLV0_TRSL_ADDR_UDW); - - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); } static int mc_pcie_setup_windows(struct platform_device *pdev, @@ -1129,6 +1211,11 @@ static int mc_platform_init(struct pci_config_window *cfg) if (ret) return ret; + /* Configure inbound translation tables */ + ret = mc_pcie_setup_inbound_ranges(pdev, port); + if (ret) + return ret; + /* Address translation is up; safe to enable interrupts */ ret = mc_init_interrupts(pdev, port); if (ret)