[PATCH v1 0/9] PCI: microchip: Partition address translations

[PATCH v1 0/9] PCI: microchip: Partition address translations

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

Microchip PolarFire SoC is a 64-bit device and has DDR starting at
0x80000000 and 0x1000000000. Its PCIe rootport is connected to the CPU
Coreplex via an FPGA fabric. The AXI connections between the Coreplex and
the fabric are 64-bit and the AXI connections between the fabric and the
rootport are 32-bit.  For the CPU CorePlex to act as an AXI-Master to the
PCIe devices and for the PCIe devices to act as bus masters to DDR at these
base addresses, the fabric can be customised to add/remove offsets for bits
38-32 in each direction. These offsets, if present, vary with each
customer's design.

To support this variety, the rootport driver must know how much address
translation (both inbound and outbound) is performed by a particular
customer design and how much address translation must be provided by the
rootport.

This patchset contains a parent/child dma-ranges scheme suggested by Rob
Herring. It creates an FPGA PCIe parent bus which wraps the PCIe rootport
and implements a parsing scheme where the root port identifies what address
translations are performed by the FPGA fabric parent bus, and what
address translations must be done by the rootport itself.

See https://lore.kernel.org/linux-pci/20220902142202.2437658-1-daire.mcnamara@microchip.com/
for the relevant previous patch submission discussion.

It also re-partitions the probe() and init() functions as suggested by
Bjorn Helgaas to make them more maintainable as the init() function had
become too large.

It also contains some minor fixes and clean-ups that are pre-requisites:
- to align register, offset, and mask names with the hardware documentation
  and to have the register definitions appear in the same order as in the
  hardware documentation;
- to harvest the MSI information from the hardware configuration register
  as these depend on the FPGA fabric design and can vary with different
  customer designs;
- to clean up interrupt initialisation to make it more maintainable;
- to fix SEC and DED interrupt handling.

I expect Conor will take the dts patch via the soc tree once the PCIe parts
of the series are accepted.

Conor Dooley (1):
  riscv: dts: microchip: add parent ranges and dma-ranges for IKRD
    v2022.09

Daire McNamara (8):
  PCI: microchip: Align register, offset, and mask names with hw docs
  PCI: microchip: Correct the DED and SEC interrupt bit offsets
  PCI: microchip: Enable event handlers to access bridge and ctrl ptrs
  PCI: microchip: Clean up initialisation of interrupts
  PCI: microchip: Gather MSI information from hardware config registers
  PCI: microchip: Re-partition code between probe() and init()
  PCI: microchip: Partition outbound address translation
  PCI: microchip: Partition inbound address translation

 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  62 +-
 drivers/pci/controller/pcie-microchip-host.c  | 676 +++++++++++++-----
 2 files changed, 522 insertions(+), 216 deletions(-)


base-commit: 3c1f24109dfc4fb1a3730ed237e50183c6bb26b3
-- 
2.25.1

[PATCH v1 1/9] PCI: microchip: Align register, offset, and mask names with hw docs

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

Minor re-organisation so that macros representing registers ascend in
numerical order and use the same names as their hardware documentation.
Removed registers not used by the driver.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 122 +++++++++----------
 1 file changed, 60 insertions(+), 62 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 0ebf7015e9af..80e7554722ca 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -30,66 +30,7 @@
 #define MC_PCIE_BRIDGE_ADDR			(MC_PCIE1_BRIDGE_ADDR)
 #define MC_PCIE_CTRL_ADDR			(MC_PCIE1_CTRL_ADDR)
 
-/* PCIe Controller Phy Regs */
-#define SEC_ERROR_CNT				0x20
-#define DED_ERROR_CNT				0x24
-#define SEC_ERROR_INT				0x28
-#define  SEC_ERROR_INT_TX_RAM_SEC_ERR_INT	GENMASK(3, 0)
-#define  SEC_ERROR_INT_RX_RAM_SEC_ERR_INT	GENMASK(7, 4)
-#define  SEC_ERROR_INT_PCIE2AXI_RAM_SEC_ERR_INT	GENMASK(11, 8)
-#define  SEC_ERROR_INT_AXI2PCIE_RAM_SEC_ERR_INT	GENMASK(15, 12)
-#define  NUM_SEC_ERROR_INTS			(4)
-#define SEC_ERROR_INT_MASK			0x2c
-#define DED_ERROR_INT				0x30
-#define  DED_ERROR_INT_TX_RAM_DED_ERR_INT	GENMASK(3, 0)
-#define  DED_ERROR_INT_RX_RAM_DED_ERR_INT	GENMASK(7, 4)
-#define  DED_ERROR_INT_PCIE2AXI_RAM_DED_ERR_INT	GENMASK(11, 8)
-#define  DED_ERROR_INT_AXI2PCIE_RAM_DED_ERR_INT	GENMASK(15, 12)
-#define  NUM_DED_ERROR_INTS			(4)
-#define DED_ERROR_INT_MASK			0x34
-#define ECC_CONTROL				0x38
-#define  ECC_CONTROL_TX_RAM_INJ_ERROR_0		BIT(0)
-#define  ECC_CONTROL_TX_RAM_INJ_ERROR_1		BIT(1)
-#define  ECC_CONTROL_TX_RAM_INJ_ERROR_2		BIT(2)
-#define  ECC_CONTROL_TX_RAM_INJ_ERROR_3		BIT(3)
-#define  ECC_CONTROL_RX_RAM_INJ_ERROR_0		BIT(4)
-#define  ECC_CONTROL_RX_RAM_INJ_ERROR_1		BIT(5)
-#define  ECC_CONTROL_RX_RAM_INJ_ERROR_2		BIT(6)
-#define  ECC_CONTROL_RX_RAM_INJ_ERROR_3		BIT(7)
-#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_0	BIT(8)
-#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_1	BIT(9)
-#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_2	BIT(10)
-#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_3	BIT(11)
-#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_0	BIT(12)
-#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_1	BIT(13)
-#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_2	BIT(14)
-#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_3	BIT(15)
-#define  ECC_CONTROL_TX_RAM_ECC_BYPASS		BIT(24)
-#define  ECC_CONTROL_RX_RAM_ECC_BYPASS		BIT(25)
-#define  ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS	BIT(26)
-#define  ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS	BIT(27)
-#define LTSSM_STATE				0x5c
-#define  LTSSM_L0_STATE				0x10
-#define PCIE_EVENT_INT				0x14c
-#define  PCIE_EVENT_INT_L2_EXIT_INT		BIT(0)
-#define  PCIE_EVENT_INT_HOTRST_EXIT_INT		BIT(1)
-#define  PCIE_EVENT_INT_DLUP_EXIT_INT		BIT(2)
-#define  PCIE_EVENT_INT_MASK			GENMASK(2, 0)
-#define  PCIE_EVENT_INT_L2_EXIT_INT_MASK	BIT(16)
-#define  PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK	BIT(17)
-#define  PCIE_EVENT_INT_DLUP_EXIT_INT_MASK	BIT(18)
-#define  PCIE_EVENT_INT_ENB_MASK		GENMASK(18, 16)
-#define  PCIE_EVENT_INT_ENB_SHIFT		16
-#define  NUM_PCIE_EVENTS			(3)
-
 /* PCIe Bridge Phy Regs */
-#define PCIE_PCI_IDS_DW1			0x9c
-
-/* PCIe Config space MSI capability structure */
-#define MC_MSI_CAP_CTRL_OFFSET			0xe0u
-#define  MC_MSI_MAX_Q_AVAIL			(MC_NUM_MSI_IRQS_CODED << 1)
-#define  MC_MSI_Q_SIZE				(MC_NUM_MSI_IRQS_CODED << 4)
-
 #define IMASK_LOCAL				0x180
 #define  DMA_END_ENGINE_0_MASK			0x00000000u
 #define  DMA_END_ENGINE_0_SHIFT			0
@@ -137,7 +78,8 @@
 #define ISTATUS_LOCAL				0x184
 #define IMASK_HOST				0x188
 #define ISTATUS_HOST				0x18c
-#define MSI_ADDR				0x190
+#define IMSI_ADDR				0x190
+#define  MSI_ADDR				0x190
 #define ISTATUS_MSI				0x194
 
 /* PCIe Master table init defines */
@@ -162,6 +104,62 @@
 
 #define ATR_ENTRY_SIZE				32
 
+/* PCIe Controller Phy Regs */
+#define SEC_ERROR_EVENT_CNT			0x20
+#define DED_ERROR_EVENT_CNT			0x24
+#define SEC_ERROR_INT				0x28
+#define  SEC_ERROR_INT_TX_RAM_SEC_ERR_INT	GENMASK(3, 0)
+#define  SEC_ERROR_INT_RX_RAM_SEC_ERR_INT	GENMASK(7, 4)
+#define  SEC_ERROR_INT_PCIE2AXI_RAM_SEC_ERR_INT	GENMASK(11, 8)
+#define  SEC_ERROR_INT_AXI2PCIE_RAM_SEC_ERR_INT	GENMASK(15, 12)
+#define  NUM_SEC_ERROR_INTS			(4)
+#define SEC_ERROR_INT_MASK			0x2c
+#define DED_ERROR_INT				0x30
+#define  DED_ERROR_INT_TX_RAM_DED_ERR_INT	GENMASK(3, 0)
+#define  DED_ERROR_INT_RX_RAM_DED_ERR_INT	GENMASK(7, 4)
+#define  DED_ERROR_INT_PCIE2AXI_RAM_DED_ERR_INT	GENMASK(11, 8)
+#define  DED_ERROR_INT_AXI2PCIE_RAM_DED_ERR_INT	GENMASK(15, 12)
+#define  NUM_DED_ERROR_INTS			(4)
+#define DED_ERROR_INT_MASK			0x34
+#define ECC_CONTROL				0x38
+#define  ECC_CONTROL_TX_RAM_INJ_ERROR_0		BIT(0)
+#define  ECC_CONTROL_TX_RAM_INJ_ERROR_1		BIT(1)
+#define  ECC_CONTROL_TX_RAM_INJ_ERROR_2		BIT(2)
+#define  ECC_CONTROL_TX_RAM_INJ_ERROR_3		BIT(3)
+#define  ECC_CONTROL_RX_RAM_INJ_ERROR_0		BIT(4)
+#define  ECC_CONTROL_RX_RAM_INJ_ERROR_1		BIT(5)
+#define  ECC_CONTROL_RX_RAM_INJ_ERROR_2		BIT(6)
+#define  ECC_CONTROL_RX_RAM_INJ_ERROR_3		BIT(7)
+#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_0	BIT(8)
+#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_1	BIT(9)
+#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_2	BIT(10)
+#define  ECC_CONTROL_PCIE2AXI_RAM_INJ_ERROR_3	BIT(11)
+#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_0	BIT(12)
+#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_1	BIT(13)
+#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_2	BIT(14)
+#define  ECC_CONTROL_AXI2PCIE_RAM_INJ_ERROR_3	BIT(15)
+#define  ECC_CONTROL_TX_RAM_ECC_BYPASS		BIT(24)
+#define  ECC_CONTROL_RX_RAM_ECC_BYPASS		BIT(25)
+#define  ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS	BIT(26)
+#define  ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS	BIT(27)
+#define PCIE_EVENT_INT				0x14c
+#define  PCIE_EVENT_INT_L2_EXIT_INT		BIT(0)
+#define  PCIE_EVENT_INT_HOTRST_EXIT_INT		BIT(1)
+#define  PCIE_EVENT_INT_DLUP_EXIT_INT		BIT(2)
+#define  PCIE_EVENT_INT_MASK			GENMASK(2, 0)
+#define  PCIE_EVENT_INT_L2_EXIT_INT_MASK	BIT(16)
+#define  PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK	BIT(17)
+#define  PCIE_EVENT_INT_DLUP_EXIT_INT_MASK	BIT(18)
+#define  PCIE_EVENT_INT_ENB_MASK		GENMASK(18, 16)
+#define  PCIE_EVENT_INT_ENB_SHIFT		16
+#define  NUM_PCIE_EVENTS			(3)
+
+/* PCIe Config space MSI capability structure */
+#define MC_MSI_CAP_CTRL_OFFSET			0xe0u
+#define  MC_MSI_MAX_Q_AVAIL			(MC_NUM_MSI_IRQS_CODED << 1)
+#define  MC_MSI_Q_SIZE				(MC_NUM_MSI_IRQS_CODED << 4)
+
+/* Events */
 #define EVENT_PCIE_L2_EXIT			0
 #define EVENT_PCIE_HOTRST_EXIT			1
 #define EVENT_PCIE_DLUP_EXIT			2
@@ -1086,7 +1084,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	      SEC_ERROR_INT_AXI2PCIE_RAM_SEC_ERR_INT;
 	writel_relaxed(val, ctrl_base_addr + SEC_ERROR_INT);
 	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_INT_MASK);
-	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_CNT);
+	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
 
 	val = DED_ERROR_INT_TX_RAM_DED_ERR_INT |
 	      DED_ERROR_INT_RX_RAM_DED_ERR_INT |
@@ -1094,7 +1092,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	      DED_ERROR_INT_AXI2PCIE_RAM_DED_ERR_INT;
 	writel_relaxed(val, ctrl_base_addr + DED_ERROR_INT);
 	writel_relaxed(0, ctrl_base_addr + DED_ERROR_INT_MASK);
-	writel_relaxed(0, ctrl_base_addr + DED_ERROR_CNT);
+	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
 
 	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
 	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
-- 
2.25.1

Re: [PATCH v1 1/9] PCI: microchip: Align register, offset, and mask names with hw docs

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:54:56PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Minor re-organisation so that macros representing registers ascend in
> numerical order and use the same names as their hardware documentation.
> Removed registers not used by the driver.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 122 +++++++++----------
>  1 file changed, 60 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 0ebf7015e9af..80e7554722ca 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c

> @@ -137,7 +78,8 @@
>  #define ISTATUS_LOCAL				0x184
>  #define IMASK_HOST				0x188
>  #define ISTATUS_HOST				0x18c
> -#define MSI_ADDR				0x190
> +#define IMSI_ADDR				0x190
> +#define  MSI_ADDR				0x190

Trivial, trivial comment - I think it would look more intentional as:
#define  MSI_ADDR				IMSI_ADDR

Otherwise this seems grand to me, modulo the SoB issue.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[PATCH v1 2/9] PCI: microchip: Correct the DED and SEC interrupt bit offsets

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

The SEC and DED interrupt bits were the wrong way round so the SEC
interrupt handler attempted to mask, unmask, and clear the DED interrupt
and vice versa. Correct the bit offsets so each interrupt handler
operates properly.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 80e7554722ca..30153fd1a2b3 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -165,12 +165,12 @@
 #define EVENT_PCIE_DLUP_EXIT			2
 #define EVENT_SEC_TX_RAM_SEC_ERR		3
 #define EVENT_SEC_RX_RAM_SEC_ERR		4
-#define EVENT_SEC_AXI2PCIE_RAM_SEC_ERR		5
-#define EVENT_SEC_PCIE2AXI_RAM_SEC_ERR		6
+#define EVENT_SEC_PCIE2AXI_RAM_SEC_ERR		5
+#define EVENT_SEC_AXI2PCIE_RAM_SEC_ERR		6
 #define EVENT_DED_TX_RAM_DED_ERR		7
 #define EVENT_DED_RX_RAM_DED_ERR		8
-#define EVENT_DED_AXI2PCIE_RAM_DED_ERR		9
-#define EVENT_DED_PCIE2AXI_RAM_DED_ERR		10
+#define EVENT_DED_PCIE2AXI_RAM_DED_ERR		9
+#define EVENT_DED_AXI2PCIE_RAM_DED_ERR		10
 #define EVENT_LOCAL_DMA_END_ENGINE_0		11
 #define EVENT_LOCAL_DMA_END_ENGINE_1		12
 #define EVENT_LOCAL_DMA_ERROR_ENGINE_0		13
-- 
2.25.1

Re: [PATCH v1 2/9] PCI: microchip: Correct the DED and SEC interrupt bit offsets

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:54:57PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> The SEC and DED interrupt bits were the wrong way round so the SEC
> interrupt handler attempted to mask, unmask, and clear the DED interrupt
> and vice versa. Correct the bit offsets so each interrupt handler
> operates properly.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Hey Daire,
I assume my SoB here is a hang over from me applying to our tree?
It'll need dropping for whenever you send a v2, sorry for not noticing
when you sent it to me before sending here.
Conor.

> ---
>  drivers/pci/controller/pcie-microchip-host.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 80e7554722ca..30153fd1a2b3 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -165,12 +165,12 @@
>  #define EVENT_PCIE_DLUP_EXIT			2
>  #define EVENT_SEC_TX_RAM_SEC_ERR		3
>  #define EVENT_SEC_RX_RAM_SEC_ERR		4
> -#define EVENT_SEC_AXI2PCIE_RAM_SEC_ERR		5
> -#define EVENT_SEC_PCIE2AXI_RAM_SEC_ERR		6
> +#define EVENT_SEC_PCIE2AXI_RAM_SEC_ERR		5
> +#define EVENT_SEC_AXI2PCIE_RAM_SEC_ERR		6
>  #define EVENT_DED_TX_RAM_DED_ERR		7
>  #define EVENT_DED_RX_RAM_DED_ERR		8
> -#define EVENT_DED_AXI2PCIE_RAM_DED_ERR		9
> -#define EVENT_DED_PCIE2AXI_RAM_DED_ERR		10
> +#define EVENT_DED_PCIE2AXI_RAM_DED_ERR		9
> +#define EVENT_DED_AXI2PCIE_RAM_DED_ERR		10
>  #define EVENT_LOCAL_DMA_END_ENGINE_0		11
>  #define EVENT_LOCAL_DMA_END_ENGINE_1		12
>  #define EVENT_LOCAL_DMA_ERROR_ENGINE_0		13
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 2/9] PCI: microchip: Correct the DED and SEC interrupt bit offsets

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:54:57PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> The SEC and DED interrupt bits were the wrong way round so the SEC
> interrupt handler attempted to mask, unmask, and clear the DED interrupt
> and vice versa. Correct the bit offsets so each interrupt handler
> operates properly.

Firstly, does this need a fixes tag for backporting?
If it does, then you should probably put this as patch #1 in the series.


> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 80e7554722ca..30153fd1a2b3 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -165,12 +165,12 @@
>  #define EVENT_PCIE_DLUP_EXIT			2
>  #define EVENT_SEC_TX_RAM_SEC_ERR		3
>  #define EVENT_SEC_RX_RAM_SEC_ERR		4
> -#define EVENT_SEC_AXI2PCIE_RAM_SEC_ERR		5
> -#define EVENT_SEC_PCIE2AXI_RAM_SEC_ERR		6
> +#define EVENT_SEC_PCIE2AXI_RAM_SEC_ERR		5
> +#define EVENT_SEC_AXI2PCIE_RAM_SEC_ERR		6

The order in the registers is:
TX_RAM, RX_RAM, PCIE2AXI_RAM, AXI2PCIE_RAM
Fix looks correct to me on that basis:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

>  #define EVENT_DED_TX_RAM_DED_ERR		7
>  #define EVENT_DED_RX_RAM_DED_ERR		8
> -#define EVENT_DED_AXI2PCIE_RAM_DED_ERR		9
> -#define EVENT_DED_PCIE2AXI_RAM_DED_ERR		10
> +#define EVENT_DED_PCIE2AXI_RAM_DED_ERR		9
> +#define EVENT_DED_AXI2PCIE_RAM_DED_ERR		10
>  #define EVENT_LOCAL_DMA_END_ENGINE_0		11
>  #define EVENT_LOCAL_DMA_END_ENGINE_1		12
>  #define EVENT_LOCAL_DMA_ERROR_ENGINE_0		13
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 3/9] PCI: microchip: Enable event handlers to access bridge and ctrl ptrs

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

Minor re-organisation so that event handlers can access both a pointer
to the bridge area of the PCIe rootport and the ctrl area of the PCIe
rootport.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 31 ++++++++++----------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 30153fd1a2b3..a81e6d25e347 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -654,9 +654,10 @@ static inline u32 reg_to_event(u32 reg, struct event_map field)
 	return (reg & field.reg_mask) ? BIT(field.event_bit) : 0;
 }
 
-static u32 pcie_events(void __iomem *addr)
+static u32 pcie_events(struct mc_pcie *port)
 {
-	u32 reg = readl_relaxed(addr);
+	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+	u32 reg = readl_relaxed(ctrl_base_addr + PCIE_EVENT_INT);
 	u32 val = 0;
 	int i;
 
@@ -666,9 +667,10 @@ static u32 pcie_events(void __iomem *addr)
 	return val;
 }
 
-static u32 sec_errors(void __iomem *addr)
+static u32 sec_errors(struct mc_pcie *port)
 {
-	u32 reg = readl_relaxed(addr);
+	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+	u32 reg = readl_relaxed(ctrl_base_addr + SEC_ERROR_INT);
 	u32 val = 0;
 	int i;
 
@@ -678,9 +680,10 @@ static u32 sec_errors(void __iomem *addr)
 	return val;
 }
 
-static u32 ded_errors(void __iomem *addr)
+static u32 ded_errors(struct mc_pcie *port)
 {
-	u32 reg = readl_relaxed(addr);
+	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+	u32 reg = readl_relaxed(ctrl_base_addr + DED_ERROR_INT);
 	u32 val = 0;
 	int i;
 
@@ -690,9 +693,10 @@ static u32 ded_errors(void __iomem *addr)
 	return val;
 }
 
-static u32 local_events(void __iomem *addr)
+static u32 local_events(struct mc_pcie *port)
 {
-	u32 reg = readl_relaxed(addr);
+	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
+	u32 reg = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
 	u32 val = 0;
 	int i;
 
@@ -704,15 +708,12 @@ static u32 local_events(void __iomem *addr)
 
 static u32 get_events(struct mc_pcie *port)
 {
-	void __iomem *bridge_base_addr =
-		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
-	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
 	u32 events = 0;
 
-	events |= pcie_events(ctrl_base_addr + PCIE_EVENT_INT);
-	events |= sec_errors(ctrl_base_addr + SEC_ERROR_INT);
-	events |= ded_errors(ctrl_base_addr + DED_ERROR_INT);
-	events |= local_events(bridge_base_addr + ISTATUS_LOCAL);
+	events |= pcie_events(port);
+	events |= sec_errors(port);
+	events |= ded_errors(port);
+	events |= local_events(port);
 
 	return events;
 }
-- 
2.25.1

Re: [PATCH v1 3/9] PCI: microchip: Enable event handlers to access bridge and ctrl ptrs

[Conor Dooley]

Hey Daire,

On Wed, Nov 16, 2022 at 01:54:58PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Minor re-organisation so that event handlers can access both a pointer
> to the bridge area of the PCIe rootport and the ctrl area of the PCIe
> rootport.

Perhaps explaining why we would want to access both, when we've not
needed to so far, would be helpful so that this commit message will make
sense in isolation would be nice. The mechanics of the change seem good
to me though:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 31 ++++++++++----------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 30153fd1a2b3..a81e6d25e347 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -654,9 +654,10 @@ static inline u32 reg_to_event(u32 reg, struct event_map field)
>  	return (reg & field.reg_mask) ? BIT(field.event_bit) : 0;
>  }
>  
> -static u32 pcie_events(void __iomem *addr)
> +static u32 pcie_events(struct mc_pcie *port)
>  {
> -	u32 reg = readl_relaxed(addr);
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +	u32 reg = readl_relaxed(ctrl_base_addr + PCIE_EVENT_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -666,9 +667,10 @@ static u32 pcie_events(void __iomem *addr)
>  	return val;
>  }
>  
> -static u32 sec_errors(void __iomem *addr)
> +static u32 sec_errors(struct mc_pcie *port)
>  {
> -	u32 reg = readl_relaxed(addr);
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +	u32 reg = readl_relaxed(ctrl_base_addr + SEC_ERROR_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -678,9 +680,10 @@ static u32 sec_errors(void __iomem *addr)
>  	return val;
>  }
>  
> -static u32 ded_errors(void __iomem *addr)
> +static u32 ded_errors(struct mc_pcie *port)
>  {
> -	u32 reg = readl_relaxed(addr);
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +	u32 reg = readl_relaxed(ctrl_base_addr + DED_ERROR_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -690,9 +693,10 @@ static u32 ded_errors(void __iomem *addr)
>  	return val;
>  }
>  
> -static u32 local_events(void __iomem *addr)
> +static u32 local_events(struct mc_pcie *port)
>  {
> -	u32 reg = readl_relaxed(addr);
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	u32 reg = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
>  	u32 val = 0;
>  	int i;
>  
> @@ -704,15 +708,12 @@ static u32 local_events(void __iomem *addr)
>  
>  static u32 get_events(struct mc_pcie *port)
>  {
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
>  	u32 events = 0;
>  
> -	events |= pcie_events(ctrl_base_addr + PCIE_EVENT_INT);
> -	events |= sec_errors(ctrl_base_addr + SEC_ERROR_INT);
> -	events |= ded_errors(ctrl_base_addr + DED_ERROR_INT);
> -	events |= local_events(bridge_base_addr + ISTATUS_LOCAL);
> +	events |= pcie_events(port);
> +	events |= sec_errors(port);
> +	events |= ded_errors(port);
> +	events |= local_events(port);
>  
>  	return events;
>  }
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 4/9] PCI: microchip: Clean up initialisation of interrupts

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

Refactor interrupt handling in _init() function into
disable_interrupts(), init_interrupts(), clear_sec_errors() and clear
ded_errors().  It was unwieldy and prone to bugs. Then clearly disable
interrupts as soon as possible and only enable interrupts after address
translation errors are setup to prevent spurious axi2pcie and pcie2axi
translation errors being reported

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 148 ++++++++++++-------
 1 file changed, 92 insertions(+), 56 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index a81e6d25e347..ecd4d3f3e3d4 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -986,39 +986,65 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
 	return 0;
 }
 
-static int mc_platform_init(struct pci_config_window *cfg)
+static inline void mc_clear_secs(struct mc_pcie *port)
 {
-	struct device *dev = cfg->parent;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct mc_pcie *port;
-	void __iomem *bridge_base_addr;
-	void __iomem *ctrl_base_addr;
-	int ret;
-	int irq;
-	int i, intx_irq, msi_irq, event_irq;
+	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+
+	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + SEC_ERROR_INT);
+	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
+}
+
+static inline void mc_clear_deds(struct mc_pcie *port)
+{
+	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+
+	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + DED_ERROR_INT);
+	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
+}
+
+static void mc_disable_interrupts(struct mc_pcie *port)
+{
+	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
+	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
 	u32 val;
-	int err;
 
-	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
-	if (!port)
-		return -ENOMEM;
-	port->dev = dev;
+	/* ensure ecc bypass is enabled */
+	val = ECC_CONTROL_TX_RAM_ECC_BYPASS | ECC_CONTROL_RX_RAM_ECC_BYPASS |
+		ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS | ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;
+	writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
 
-	ret = mc_pcie_init_clks(dev);
-	if (ret) {
-		dev_err(dev, "failed to get clock resources, error %d\n", ret);
-		return -ENODEV;
-	}
+	/* disable sec errors and clear any outstanding */
+	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + SEC_ERROR_INT_MASK);
+	mc_clear_secs(port);
 
-	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(port->axi_base_addr))
-		return PTR_ERR(port->axi_base_addr);
+	/* disable ded errors and clear any outstanding */
+	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + DED_ERROR_INT_MASK);
+	mc_clear_deds(port);
 
-	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
-	ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+	/* disable local interrupts and clear any outstanding */
+	writel_relaxed(0, bridge_base_addr + IMASK_LOCAL);
+	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_LOCAL);
+	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_MSI);
+
+	/* disable PCIe events and clear any outstanding */
+	val = PCIE_EVENT_INT_L2_EXIT_INT | PCIE_EVENT_INT_HOTRST_EXIT_INT |
+	      PCIE_EVENT_INT_DLUP_EXIT_INT | PCIE_EVENT_INT_L2_EXIT_INT_MASK |
+	      PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK |
+	      PCIE_EVENT_INT_DLUP_EXIT_INT_MASK;
+	writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
+
+	/* disable host interrupts and clear any outstanding */
+	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
+	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
+}
+
+static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port)
+{
+	struct device *dev = &pdev->dev;
+	int irq;
+	int i, intx_irq, msi_irq, event_irq;
+	int ret;
 
-	port->msi.vector_phy = MSI_ADDR;
-	port->msi.num_vectors = MC_NUM_MSI_IRQS;
 	ret = mc_pcie_init_irq_domains(port);
 	if (ret) {
 		dev_err(dev, "failed creating IRQ domains\n");
@@ -1036,11 +1062,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
 			return -ENXIO;
 		}
 
-		err = devm_request_irq(dev, event_irq, mc_event_handler,
+		ret = devm_request_irq(dev, event_irq, mc_event_handler,
 				       0, event_cause[i].sym, port);
-		if (err) {
+		if (ret) {
 			dev_err(dev, "failed to request IRQ %d\n", event_irq);
-			return err;
+			return ret;
 		}
 	}
 
@@ -1065,44 +1091,54 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	/* Plug the main event chained handler */
 	irq_set_chained_handler_and_data(irq, mc_handle_event, port);
 
-	/* Hardware doesn't setup MSI by default */
-	mc_pcie_enable_msi(port, cfg->win);
+	return 0;
+}
 
-	val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
-	val |= PM_MSI_INT_INTX_MASK;
-	writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
+static int mc_platform_init(struct pci_config_window *cfg)
+{
+	struct device *dev = cfg->parent;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mc_pcie *port;
+	void __iomem *bridge_base_addr;
+	void __iomem *ctrl_base_addr;
+	int ret;
 
-	writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+	port->dev = dev;
 
-	val = PCIE_EVENT_INT_L2_EXIT_INT |
-	      PCIE_EVENT_INT_HOTRST_EXIT_INT |
-	      PCIE_EVENT_INT_DLUP_EXIT_INT;
-	writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
+	ret = mc_pcie_init_clks(dev);
+	if (ret) {
+		dev_err(dev, "failed to get clock resources, error %d\n", ret);
+		return -ENODEV;
+	}
 
-	val = SEC_ERROR_INT_TX_RAM_SEC_ERR_INT |
-	      SEC_ERROR_INT_RX_RAM_SEC_ERR_INT |
-	      SEC_ERROR_INT_PCIE2AXI_RAM_SEC_ERR_INT |
-	      SEC_ERROR_INT_AXI2PCIE_RAM_SEC_ERR_INT;
-	writel_relaxed(val, ctrl_base_addr + SEC_ERROR_INT);
-	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_INT_MASK);
-	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
+	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(port->axi_base_addr))
+		return PTR_ERR(port->axi_base_addr);
 
-	val = DED_ERROR_INT_TX_RAM_DED_ERR_INT |
-	      DED_ERROR_INT_RX_RAM_DED_ERR_INT |
-	      DED_ERROR_INT_PCIE2AXI_RAM_DED_ERR_INT |
-	      DED_ERROR_INT_AXI2PCIE_RAM_DED_ERR_INT;
-	writel_relaxed(val, ctrl_base_addr + DED_ERROR_INT);
-	writel_relaxed(0, ctrl_base_addr + DED_ERROR_INT_MASK);
-	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
+	mc_disable_interrupts(port);
 
-	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
-	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
+	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
+	ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
+
+	port->msi.vector_phy = MSI_ADDR;
+	port->msi.num_vectors = MC_NUM_MSI_IRQS;
+
+	/* Hardware doesn't setup MSI by default */
+	mc_pcie_enable_msi(port, cfg->win);
 
 	/* 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);
+	ret = mc_pcie_setup_windows(pdev, port);
+	if (ret)
+		return ret;
+
+	/* address translation is up; safe to enable interrupts */
+	return mc_init_interrupts(pdev, port);
 }
 
 static const struct pci_ecam_ops mc_ecam_ops = {
-- 
2.25.1

Re: [PATCH v1 4/9] PCI: microchip: Clean up initialisation of interrupts

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:54:59PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Refactor interrupt handling in _init() function into
> disable_interrupts(), init_interrupts(), clear_sec_errors() and clear
> ded_errors().  It was unwieldy and prone to bugs. Then clearly disable
> interrupts as soon as possible and only enable interrupts after address
> translation errors are setup to prevent spurious axi2pcie and pcie2axi
              ^^^^^^
Is that meant to read "after address translation is" or "after address
translation handling is"?

> translation errors being reported
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 148 ++++++++++++-------
>  1 file changed, 92 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index a81e6d25e347..ecd4d3f3e3d4 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -986,39 +986,65 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
>  	return 0;
>  }
>  
> -static int mc_platform_init(struct pci_config_window *cfg)
> +static inline void mc_clear_secs(struct mc_pcie *port)
>  {
> -	struct device *dev = cfg->parent;
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct mc_pcie *port;
> -	void __iomem *bridge_base_addr;
> -	void __iomem *ctrl_base_addr;
> -	int ret;
> -	int irq;
> -	int i, intx_irq, msi_irq, event_irq;
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +
> +	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + SEC_ERROR_INT);

I think it'd be nice if the GENMASK()s in this function and below were
#defined above somewhere.

> +	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> +}
> +
> +static inline void mc_clear_deds(struct mc_pcie *port)
> +{
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +
> +	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + DED_ERROR_INT);
> +	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> +}
> +
> +static void mc_disable_interrupts(struct mc_pcie *port)
> +{
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
>  	u32 val;
> -	int err;
>  
> -	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> -	if (!port)
> -		return -ENOMEM;
> -	port->dev = dev;
> +	/* ensure ecc bypass is enabled */
> +	val = ECC_CONTROL_TX_RAM_ECC_BYPASS | ECC_CONTROL_RX_RAM_ECC_BYPASS |
> +		ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS | ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;

Pedantic maybe, but could we format this as:
		ECC_CONTROL_TX_RAM_ECC_BYPASS |
		ECC_CONTROL_RX_RAM_ECC_BYPASS |
		ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS |
		ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;
And the same below for the PCIE_EVENT_FOO stuff, I think it'd make
things a bit easier on the eye.

Anyways, SEC and DED stuff that I usually see on startup are gone - at
least on the setup I have :)
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> +	writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
>  
> -	ret = mc_pcie_init_clks(dev);
> -	if (ret) {
> -		dev_err(dev, "failed to get clock resources, error %d\n", ret);
> -		return -ENODEV;
> -	}
> +	/* disable sec errors and clear any outstanding */
> +	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + SEC_ERROR_INT_MASK);
> +	mc_clear_secs(port);
>  
> -	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> -	if (IS_ERR(port->axi_base_addr))
> -		return PTR_ERR(port->axi_base_addr);
> +	/* disable ded errors and clear any outstanding */
> +	writel_relaxed(GENMASK(15, 0), ctrl_base_addr + DED_ERROR_INT_MASK);
> +	mc_clear_deds(port);
>  
> -	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -	ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +	/* disable local interrupts and clear any outstanding */
> +	writel_relaxed(0, bridge_base_addr + IMASK_LOCAL);
> +	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_LOCAL);
> +	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_MSI);
> +
> +	/* disable PCIe events and clear any outstanding */
> +	val = PCIE_EVENT_INT_L2_EXIT_INT | PCIE_EVENT_INT_HOTRST_EXIT_INT |
> +	      PCIE_EVENT_INT_DLUP_EXIT_INT | PCIE_EVENT_INT_L2_EXIT_INT_MASK |
> +	      PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK |
> +	      PCIE_EVENT_INT_DLUP_EXIT_INT_MASK;
> +	writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
> +
> +	/* disable host interrupts and clear any outstanding */
> +	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> +	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
> +}
> +
> +static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port)
> +{
> +	struct device *dev = &pdev->dev;
> +	int irq;
> +	int i, intx_irq, msi_irq, event_irq;
> +	int ret;
>  
> -	port->msi.vector_phy = MSI_ADDR;
> -	port->msi.num_vectors = MC_NUM_MSI_IRQS;
>  	ret = mc_pcie_init_irq_domains(port);
>  	if (ret) {
>  		dev_err(dev, "failed creating IRQ domains\n");
> @@ -1036,11 +1062,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  			return -ENXIO;
>  		}
>  
> -		err = devm_request_irq(dev, event_irq, mc_event_handler,
> +		ret = devm_request_irq(dev, event_irq, mc_event_handler,
>  				       0, event_cause[i].sym, port);
> -		if (err) {
> +		if (ret) {
>  			dev_err(dev, "failed to request IRQ %d\n", event_irq);
> -			return err;
> +			return ret;
>  		}
>  	}
>  
> @@ -1065,44 +1091,54 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	/* Plug the main event chained handler */
>  	irq_set_chained_handler_and_data(irq, mc_handle_event, port);
>  
> -	/* Hardware doesn't setup MSI by default */
> -	mc_pcie_enable_msi(port, cfg->win);
> +	return 0;
> +}
>  
> -	val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> -	val |= PM_MSI_INT_INTX_MASK;
> -	writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> +static int mc_platform_init(struct pci_config_window *cfg)
> +{
> +	struct device *dev = cfg->parent;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct mc_pcie *port;
> +	void __iomem *bridge_base_addr;
> +	void __iomem *ctrl_base_addr;
> +	int ret;
>  
> -	writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +	port->dev = dev;
>  
> -	val = PCIE_EVENT_INT_L2_EXIT_INT |
> -	      PCIE_EVENT_INT_HOTRST_EXIT_INT |
> -	      PCIE_EVENT_INT_DLUP_EXIT_INT;
> -	writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
> +	ret = mc_pcie_init_clks(dev);
> +	if (ret) {
> +		dev_err(dev, "failed to get clock resources, error %d\n", ret);
> +		return -ENODEV;
> +	}
>  
> -	val = SEC_ERROR_INT_TX_RAM_SEC_ERR_INT |
> -	      SEC_ERROR_INT_RX_RAM_SEC_ERR_INT |
> -	      SEC_ERROR_INT_PCIE2AXI_RAM_SEC_ERR_INT |
> -	      SEC_ERROR_INT_AXI2PCIE_RAM_SEC_ERR_INT;
> -	writel_relaxed(val, ctrl_base_addr + SEC_ERROR_INT);
> -	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_INT_MASK);
> -	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> +	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(port->axi_base_addr))
> +		return PTR_ERR(port->axi_base_addr);
>  
> -	val = DED_ERROR_INT_TX_RAM_DED_ERR_INT |
> -	      DED_ERROR_INT_RX_RAM_DED_ERR_INT |
> -	      DED_ERROR_INT_PCIE2AXI_RAM_DED_ERR_INT |
> -	      DED_ERROR_INT_AXI2PCIE_RAM_DED_ERR_INT;
> -	writel_relaxed(val, ctrl_base_addr + DED_ERROR_INT);
> -	writel_relaxed(0, ctrl_base_addr + DED_ERROR_INT_MASK);
> -	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> +	mc_disable_interrupts(port);
>  
> -	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> -	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
> +	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> +
> +	port->msi.vector_phy = MSI_ADDR;
> +	port->msi.num_vectors = MC_NUM_MSI_IRQS;
> +
> +	/* Hardware doesn't setup MSI by default */
> +	mc_pcie_enable_msi(port, cfg->win);
>  
>  	/* 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);
> +	ret = mc_pcie_setup_windows(pdev, port);
> +	if (ret)
> +		return ret;
> +
> +	/* address translation is up; safe to enable interrupts */
> +	return mc_init_interrupts(pdev, port);
>  }
>  
>  static const struct pci_ecam_ops mc_ecam_ops = {
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 5/9] PCI: microchip: Gather MSI information from hardware config registers

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

The PCIe root complex on PolarFire SoC is configured at bitstream creation
time using Libero.  Key MSI-related parameters include the number of
MSIs (1/2/4/8/16/32) and the MSI address. In the device driver, extract
this information from hw registers at init time, and use it to configure
MSI system, including configuring MSI capability structure correctly in
configuration space.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 73 +++++++++++---------
 1 file changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index ecd4d3f3e3d4..faecf419ad6f 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -20,8 +20,7 @@
 #include "../pci.h"
 
 /* Number of MSI IRQs */
-#define MC_NUM_MSI_IRQS				32
-#define MC_NUM_MSI_IRQS_CODED			5
+#define MC_MAX_NUM_MSI_IRQS			32
 
 /* PCIe Bridge Phy and Controller Phy offsets */
 #define MC_PCIE1_BRIDGE_ADDR			0x00008000u
@@ -31,6 +30,11 @@
 #define MC_PCIE_CTRL_ADDR			(MC_PCIE1_CTRL_ADDR)
 
 /* PCIe Bridge Phy Regs */
+#define PCIE_PCI_IRQ_DW0			0xa8
+#define  MSIX_CAP_MASK				BIT(31)
+#define  NUM_MSI_MSGS_MASK			GENMASK(6, 4)
+#define  NUM_MSI_MSGS_SHIFT			4
+
 #define IMASK_LOCAL				0x180
 #define  DMA_END_ENGINE_0_MASK			0x00000000u
 #define  DMA_END_ENGINE_0_SHIFT			0
@@ -79,7 +83,6 @@
 #define IMASK_HOST				0x188
 #define ISTATUS_HOST				0x18c
 #define IMSI_ADDR				0x190
-#define  MSI_ADDR				0x190
 #define ISTATUS_MSI				0x194
 
 /* PCIe Master table init defines */
@@ -156,8 +159,6 @@
 
 /* PCIe Config space MSI capability structure */
 #define MC_MSI_CAP_CTRL_OFFSET			0xe0u
-#define  MC_MSI_MAX_Q_AVAIL			(MC_NUM_MSI_IRQS_CODED << 1)
-#define  MC_MSI_Q_SIZE				(MC_NUM_MSI_IRQS_CODED << 4)
 
 /* Events */
 #define EVENT_PCIE_L2_EXIT			0
@@ -257,7 +258,7 @@ struct mc_msi {
 	struct irq_domain *dev_domain;
 	u32 num_vectors;
 	u64 vector_phy;
-	DECLARE_BITMAP(used, MC_NUM_MSI_IRQS);
+	DECLARE_BITMAP(used, MC_MAX_NUM_MSI_IRQS);
 };
 
 struct mc_pcie {
@@ -380,25 +381,29 @@ static struct {
 
 static char poss_clks[][5] = { "fic0", "fic1", "fic2", "fic3" };
 
-static void mc_pcie_enable_msi(struct mc_pcie *port, void __iomem *base)
+static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)
 {
 	struct mc_msi *msi = &port->msi;
-	u32 cap_offset = MC_MSI_CAP_CTRL_OFFSET;
-	u16 msg_ctrl = readw_relaxed(base + cap_offset + PCI_MSI_FLAGS);
-
-	msg_ctrl |= PCI_MSI_FLAGS_ENABLE;
-	msg_ctrl &= ~PCI_MSI_FLAGS_QMASK;
-	msg_ctrl |= MC_MSI_MAX_Q_AVAIL;
-	msg_ctrl &= ~PCI_MSI_FLAGS_QSIZE;
-	msg_ctrl |= MC_MSI_Q_SIZE;
-	msg_ctrl |= PCI_MSI_FLAGS_64BIT;
-
-	writew_relaxed(msg_ctrl, base + cap_offset + PCI_MSI_FLAGS);
-
+	u16 reg;
+	u8 queue_size;
+
+	/* fixup msi enable flag */
+	reg = readw_relaxed(ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
+	reg |= PCI_MSI_FLAGS_ENABLE;
+	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
+
+	/* fixup msi queue flags */
+	queue_size = reg & PCI_MSI_FLAGS_QMASK;
+	queue_size >>= 1;
+	reg &= ~PCI_MSI_FLAGS_QSIZE;
+	reg |= queue_size << 4;
+	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
+
+	/* fixup msi addr fields */
 	writel_relaxed(lower_32_bits(msi->vector_phy),
-		       base + cap_offset + PCI_MSI_ADDRESS_LO);
+		       ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_ADDRESS_LO);
 	writel_relaxed(upper_32_bits(msi->vector_phy),
-		       base + cap_offset + PCI_MSI_ADDRESS_HI);
+		       ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_ADDRESS_HI);
 }
 
 static void mc_handle_msi(struct irq_desc *desc)
@@ -471,10 +476,7 @@ static int mc_irq_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
 {
 	struct mc_pcie *port = domain->host_data;
 	struct mc_msi *msi = &port->msi;
-	void __iomem *bridge_base_addr =
-		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
 	unsigned long bit;
-	u32 val;
 
 	mutex_lock(&msi->lock);
 	bit = find_first_zero_bit(msi->used, msi->num_vectors);
@@ -488,11 +490,6 @@ static int mc_irq_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	irq_domain_set_info(domain, virq, bit, &mc_msi_bottom_irq_chip,
 			    domain->host_data, handle_edge_irq, NULL, NULL);
 
-	/* Enable MSI interrupts */
-	val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
-	val |= PM_MSI_INT_MSI_MASK;
-	writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
-
 	mutex_unlock(&msi->lock);
 
 	return 0;
@@ -1102,6 +1099,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	void __iomem *bridge_base_addr;
 	void __iomem *ctrl_base_addr;
 	int ret;
+	u32 val;
 
 	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
 	if (!port)
@@ -1123,11 +1121,20 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
 	ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
 
-	port->msi.vector_phy = MSI_ADDR;
-	port->msi.num_vectors = MC_NUM_MSI_IRQS;
+	/* allow enabling msi by disabling msi-x */
+	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
+	val &= ~MSIX_CAP_MASK;
+	writel(val, bridge_base_addr + PCIE_PCI_IRQ_DW0);
+
+	/* pick num vectors from design */
+	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
+	val &= NUM_MSI_MSGS_MASK;
+	val >>= NUM_MSI_MSGS_SHIFT;
+
+	port->msi.num_vectors = 1 << val;
 
-	/* Hardware doesn't setup MSI by default */
-	mc_pcie_enable_msi(port, cfg->win);
+	/* pick vector address from design */
+	port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
 
 	/* Configure Address Translation Table 0 for PCIe config space */
 	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff,
-- 
2.25.1

Re: [PATCH v1 5/9] PCI: microchip: Gather MSI information from hardware config registers

[Bjorn Helgaas]

On Wed, Nov 16, 2022 at 01:55:00PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> The PCIe root complex on PolarFire SoC is configured at bitstream creation
> time using Libero.  Key MSI-related parameters include the number of
> MSIs (1/2/4/8/16/32) and the MSI address. In the device driver, extract
> this information from hw registers at init time, and use it to configure
> MSI system, including configuring MSI capability structure correctly in
> configuration space.

Minor nits for v2.

> +	/* fixup msi enable flag */

s/msi/MSI/ here and comments below to match other usage.

> +	reg = readw_relaxed(ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
> +	reg |= PCI_MSI_FLAGS_ENABLE;
> +	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
> +
> +	/* fixup msi queue flags */
> +	queue_size = reg & PCI_MSI_FLAGS_QMASK;
> +	queue_size >>= 1;
> +	reg &= ~PCI_MSI_FLAGS_QSIZE;
> +	reg |= queue_size << 4;
> +	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
> +
> +	/* fixup msi addr fields */

> +	/* allow enabling msi by disabling msi-x */

s/msi-x/MSI-X/

Re: [PATCH v1 5/9] PCI: microchip: Gather MSI information from hardware config registers

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:55:00PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> The PCIe root complex on PolarFire SoC is configured at bitstream creation
> time using Libero.  Key MSI-related parameters include the number of
> MSIs (1/2/4/8/16/32) and the MSI address. In the device driver, extract
> this information from hw registers at init time, and use it to configure

I don't know the answer here, so I am not being difficult:
Does the HSS program them in based on the XML produced alongside the
bitstream, or is that information encoded in the bitstream itself?

> MSI system, including configuring MSI capability structure correctly in
> configuration space.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 73 +++++++++++---------
>  1 file changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index ecd4d3f3e3d4..faecf419ad6f 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -20,8 +20,7 @@
>  #include "../pci.h"
>  
>  /* Number of MSI IRQs */
> -#define MC_NUM_MSI_IRQS				32
> -#define MC_NUM_MSI_IRQS_CODED			5
> +#define MC_MAX_NUM_MSI_IRQS			32
>  
>  /* PCIe Bridge Phy and Controller Phy offsets */
>  #define MC_PCIE1_BRIDGE_ADDR			0x00008000u
> @@ -31,6 +30,11 @@
>  #define MC_PCIE_CTRL_ADDR			(MC_PCIE1_CTRL_ADDR)
>  
>  /* PCIe Bridge Phy Regs */
> +#define PCIE_PCI_IRQ_DW0			0xa8
> +#define  MSIX_CAP_MASK				BIT(31)
> +#define  NUM_MSI_MSGS_MASK			GENMASK(6, 4)
> +#define  NUM_MSI_MSGS_SHIFT			4
> +
>  #define IMASK_LOCAL				0x180
>  #define  DMA_END_ENGINE_0_MASK			0x00000000u
>  #define  DMA_END_ENGINE_0_SHIFT			0
> @@ -79,7 +83,6 @@
>  #define IMASK_HOST				0x188
>  #define ISTATUS_HOST				0x18c
>  #define IMSI_ADDR				0x190
> -#define  MSI_ADDR				0x190
>  #define ISTATUS_MSI				0x194
>  
>  /* PCIe Master table init defines */
> @@ -156,8 +159,6 @@
>  
>  /* PCIe Config space MSI capability structure */
>  #define MC_MSI_CAP_CTRL_OFFSET			0xe0u
> -#define  MC_MSI_MAX_Q_AVAIL			(MC_NUM_MSI_IRQS_CODED << 1)
> -#define  MC_MSI_Q_SIZE				(MC_NUM_MSI_IRQS_CODED << 4)
>  
>  /* Events */
>  #define EVENT_PCIE_L2_EXIT			0
> @@ -257,7 +258,7 @@ struct mc_msi {
>  	struct irq_domain *dev_domain;
>  	u32 num_vectors;
>  	u64 vector_phy;
> -	DECLARE_BITMAP(used, MC_NUM_MSI_IRQS);
> +	DECLARE_BITMAP(used, MC_MAX_NUM_MSI_IRQS);
>  };
>  
>  struct mc_pcie {
> @@ -380,25 +381,29 @@ static struct {
>  
>  static char poss_clks[][5] = { "fic0", "fic1", "fic2", "fic3" };
>  
> -static void mc_pcie_enable_msi(struct mc_pcie *port, void __iomem *base)
> +static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)
>  {
>  	struct mc_msi *msi = &port->msi;
> -	u32 cap_offset = MC_MSI_CAP_CTRL_OFFSET;
> -	u16 msg_ctrl = readw_relaxed(base + cap_offset + PCI_MSI_FLAGS);
> -
> -	msg_ctrl |= PCI_MSI_FLAGS_ENABLE;
> -	msg_ctrl &= ~PCI_MSI_FLAGS_QMASK;
> -	msg_ctrl |= MC_MSI_MAX_Q_AVAIL;
> -	msg_ctrl &= ~PCI_MSI_FLAGS_QSIZE;
> -	msg_ctrl |= MC_MSI_Q_SIZE;
> -	msg_ctrl |= PCI_MSI_FLAGS_64BIT;
> -
> -	writew_relaxed(msg_ctrl, base + cap_offset + PCI_MSI_FLAGS);
> -
> +	u16 reg;
> +	u8 queue_size;
> +
> +	/* fixup msi enable flag */
> +	reg = readw_relaxed(ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
> +	reg |= PCI_MSI_FLAGS_ENABLE;
> +	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
> +
> +	/* fixup msi queue flags */
> +	queue_size = reg & PCI_MSI_FLAGS_QMASK;
> +	queue_size >>= 1;
> +	reg &= ~PCI_MSI_FLAGS_QSIZE;
> +	reg |= queue_size << 4;

Could you please explain where the >> 1 & << 4 come from? Maybe it's
sufficient to just make them defines, or the comment here could be
expanded on.

> +	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
> +
> +	/* fixup msi addr fields */
>  	writel_relaxed(lower_32_bits(msi->vector_phy),
> -		       base + cap_offset + PCI_MSI_ADDRESS_LO);
> +		       ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_ADDRESS_LO);
>  	writel_relaxed(upper_32_bits(msi->vector_phy),
> -		       base + cap_offset + PCI_MSI_ADDRESS_HI);
> +		       ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_ADDRESS_HI);
>  }
>  
>  static void mc_handle_msi(struct irq_desc *desc)
> @@ -471,10 +476,7 @@ static int mc_irq_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  {
>  	struct mc_pcie *port = domain->host_data;
>  	struct mc_msi *msi = &port->msi;
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	unsigned long bit;
> -	u32 val;
>  
>  	mutex_lock(&msi->lock);
>  	bit = find_first_zero_bit(msi->used, msi->num_vectors);
> @@ -488,11 +490,6 @@ static int mc_irq_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  	irq_domain_set_info(domain, virq, bit, &mc_msi_bottom_irq_chip,
>  			    domain->host_data, handle_edge_irq, NULL, NULL);
>  
> -	/* Enable MSI interrupts */
> -	val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> -	val |= PM_MSI_INT_MSI_MASK;
> -	writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> -
>  	mutex_unlock(&msi->lock);
>  
>  	return 0;
> @@ -1102,6 +1099,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	void __iomem *bridge_base_addr;
>  	void __iomem *ctrl_base_addr;
>  	int ret;
> +	u32 val;
>  
>  	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>  	if (!port)
> @@ -1123,11 +1121,20 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
>  
> -	port->msi.vector_phy = MSI_ADDR;
> -	port->msi.num_vectors = MC_NUM_MSI_IRQS;
> +	/* allow enabling msi by disabling msi-x */
> +	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	val &= ~MSIX_CAP_MASK;
> +	writel(val, bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +
> +	/* pick num vectors from design */
> +	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	val &= NUM_MSI_MSGS_MASK;
> +	val >>= NUM_MSI_MSGS_SHIFT;
> +
> +	port->msi.num_vectors = 1 << val;
>  
> -	/* Hardware doesn't setup MSI by default */
> -	mc_pcie_enable_msi(port, cfg->win);
> +	/* pick vector address from design */

"Design" might make sense to you or I, but I think you could expand this
comment for the lay reader. Copy-paste from the commit message maybe
hah.

Anyways, as with the rest of the series - I'm just nitpicking.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> +	port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
>  
>  	/* Configure Address Translation Table 0 for PCIe config space */
>  	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff,
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 6/9] PCI: microchip: Re-partition code between probe() and init()

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

Continuing to use pci_host_common_probe() for the PCIe root complex on
PolarFire SoC was leading to an extremely large _init() function and
some unnatural code flow. Re-partition so some tasks are done in
a _probe() routine, which calls pci_host_common_probe() and then use a
much smaller _init() function, mainly to enable interrupts after address
translation tables are set up.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 55 ++++++++++++++------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index faecf419ad6f..73856647f321 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -381,6 +381,8 @@ static struct {
 
 static char poss_clks[][5] = { "fic0", "fic1", "fic2", "fic3" };
 
+static struct mc_pcie *port;
+
 static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)
 {
 	struct mc_msi *msi = &port->msi;
@@ -1095,7 +1097,34 @@ static int mc_platform_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
 	struct platform_device *pdev = to_platform_device(dev);
-	struct mc_pcie *port;
+	void __iomem *bridge_base_addr =
+		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
+	int ret;
+
+	/* Configure address translation table 0 for PCIe config space */
+	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
+			     cfg->res.start,
+			     resource_size(&cfg->res));
+
+	/* Need some fixups in config space */
+	mc_pcie_fixup_ecam(port, cfg->win);
+
+	/* Configure non-config space outbound ranges */
+	ret = mc_pcie_setup_windows(pdev, port);
+	if (ret)
+		return ret;
+
+	/* address translation is up; safe to enable interrupts */
+	ret = mc_init_interrupts(pdev, port);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mc_host_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
 	void __iomem *bridge_base_addr;
 	void __iomem *ctrl_base_addr;
 	int ret;
@@ -1104,13 +1133,8 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
 	if (!port)
 		return -ENOMEM;
-	port->dev = dev;
 
-	ret = mc_pcie_init_clks(dev);
-	if (ret) {
-		dev_err(dev, "failed to get clock resources, error %d\n", ret);
-		return -ENODEV;
-	}
+	port->dev = dev;
 
 	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(port->axi_base_addr))
@@ -1136,16 +1160,13 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	/* pick vector address from design */
 	port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
 
-	/* 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));
-
-	ret = mc_pcie_setup_windows(pdev, port);
-	if (ret)
-		return ret;
+	ret = mc_pcie_init_clks(dev);
+	if (ret) {
+		dev_err(dev, "failed to get clock resources, error %d\n", ret);
+		return -ENODEV;
+	}
 
-	/* address translation is up; safe to enable interrupts */
-	return mc_init_interrupts(pdev, port);
+	return pci_host_common_probe(pdev);
 }
 
 static const struct pci_ecam_ops mc_ecam_ops = {
@@ -1168,7 +1189,7 @@ static const struct of_device_id mc_pcie_of_match[] = {
 MODULE_DEVICE_TABLE(of, mc_pcie_of_match);
 
 static struct platform_driver mc_pcie_driver = {
-	.probe = pci_host_common_probe,
+	.probe = mc_host_probe,
 	.driver = {
 		.name = "microchip-pcie",
 		.of_match_table = mc_pcie_of_match,
-- 
2.25.1

Re: [PATCH v1 6/9] PCI: microchip: Re-partition code between probe() and init()

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:55:01PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Continuing to use pci_host_common_probe() for the PCIe root complex on
> PolarFire SoC was leading to an extremely large _init() function and
> some unnatural code flow. Re-partition so some tasks are done in
> a _probe() routine, which calls pci_host_common_probe() and then use a
> much smaller _init() function, mainly to enable interrupts after address
> translation tables are set up.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 55 ++++++++++++++------
>  1 file changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index faecf419ad6f..73856647f321 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -381,6 +381,8 @@ static struct {
>  
>  static char poss_clks[][5] = { "fic0", "fic1", "fic2", "fic3" };
>  
> +static struct mc_pcie *port;
> +
>  static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)
>  {
>  	struct mc_msi *msi = &port->msi;
> @@ -1095,7 +1097,34 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
>  	struct platform_device *pdev = to_platform_device(dev);
> -	struct mc_pcie *port;
> +	void __iomem *bridge_base_addr =
> +		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	int ret;
> +
> +	/* Configure address translation table 0 for PCIe config space */
> +	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
> +			     cfg->res.start,
> +			     resource_size(&cfg->res));
> +
> +	/* Need some fixups in config space */
> +	mc_pcie_fixup_ecam(port, cfg->win);
> +
> +	/* Configure non-config space outbound ranges */
> +	ret = mc_pcie_setup_windows(pdev, port);
> +	if (ret)
> +		return ret;
> +
> +	/* address translation is up; safe to enable interrupts */

I think that Bjorn mentioned it elsewhere, but consistent capitalisation
would be nice. Otherwise, code movement looks good to me.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> +	ret = mc_init_interrupts(pdev, port);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mc_host_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
>  	void __iomem *bridge_base_addr;
>  	void __iomem *ctrl_base_addr;
>  	int ret;
> @@ -1104,13 +1133,8 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>  	if (!port)
>  		return -ENOMEM;
> -	port->dev = dev;
>  
> -	ret = mc_pcie_init_clks(dev);
> -	if (ret) {
> -		dev_err(dev, "failed to get clock resources, error %d\n", ret);
> -		return -ENODEV;
> -	}
> +	port->dev = dev;
>  
>  	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
>  	if (IS_ERR(port->axi_base_addr))
> @@ -1136,16 +1160,13 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	/* pick vector address from design */
>  	port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
>  
> -	/* 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));
> -
> -	ret = mc_pcie_setup_windows(pdev, port);
> -	if (ret)
> -		return ret;
> +	ret = mc_pcie_init_clks(dev);
> +	if (ret) {
> +		dev_err(dev, "failed to get clock resources, error %d\n", ret);
> +		return -ENODEV;
> +	}
>  
> -	/* address translation is up; safe to enable interrupts */
> -	return mc_init_interrupts(pdev, port);
> +	return pci_host_common_probe(pdev);
>  }
>  
>  static const struct pci_ecam_ops mc_ecam_ops = {
> @@ -1168,7 +1189,7 @@ static const struct of_device_id mc_pcie_of_match[] = {
>  MODULE_DEVICE_TABLE(of, mc_pcie_of_match);
>  
>  static struct platform_driver mc_pcie_driver = {
> -	.probe = pci_host_common_probe,
> +	.probe = mc_host_probe,
>  	.driver = {
>  		.name = "microchip-pcie",
>  		.of_match_table = mc_pcie_of_match,
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 7/9] PCI: microchip: Partition outbound address translation

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
inter connect (fic) busses that encapsulate busses like ABP/AHP and
AXI-M. Depending on which fic(s) the rootport is wired through to cpu
space, the rootport driver needs to take account of the address
translation done by a parent (e.g. fabric) node before setting up its
own outbound address translation tables to config space and attached
devices.

Parse the range properties to determine how much address translation
needs to be done in the rootport.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 109 +++++++++++++++----
 1 file changed, 86 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 73856647f321..62f8c5edfd0e 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -85,27 +85,42 @@
 #define IMSI_ADDR				0x190
 #define ISTATUS_MSI				0x194
 
+#define ATR_WINDOW_DESC_SIZE			32
+#define ATR_PCIE_ATR_SIZE			0x25
+#define ATR_SIZE_SHIFT				1
+#define ATR_IMPL_ENABLE				1
+
 /* PCIe Master table init defines */
 #define ATR0_PCIE_WIN0_SRCADDR_PARAM		0x600u
-#define  ATR0_PCIE_ATR_SIZE			0x25
-#define  ATR0_PCIE_ATR_SIZE_SHIFT		1
 #define ATR0_PCIE_WIN0_SRC_ADDR			0x604u
 #define ATR0_PCIE_WIN0_TRSL_ADDR_LSB		0x608u
 #define ATR0_PCIE_WIN0_TRSL_ADDR_UDW		0x60cu
 #define ATR0_PCIE_WIN0_TRSL_PARAM		0x610u
 
+enum {
+	TRSL_ID_PCIE_TXRX = 0,
+	TRSL_ID_PCIE_CONFIG = 1,
+	TRSL_ID_AXI4_LITE_MASTER = 2,
+	TRSL_ID_AXI4_MASTER_0 = 4,
+	TRSL_ID_AXI4_MASTER_1 = 5,
+	TRSL_ID_AXI4_MASTER_2 = 6,
+	TRSL_ID_AXI4_MASTER_3 = 7,
+	TRSL_ID_AXI4_STREAM_0 = 8,
+	TRSL_ID_AXI4_STREAM_1 = 9,
+	TRSL_ID_AXI4_STREAM_2 = 10,
+	TRSL_ID_AXI4_STREAM_3 = 11,
+	TRSL_ID_INTERNAL_BRIDGE_REGISTERS = 12,
+};
+
+#define ATR0_PCIE_WIN0_TRSL_MASK_LSB		0x618u
+#define ATR0_PCIE_WIN0_TRSL_MASK_UDW		0x61cu
+
 /* PCIe AXI slave table init defines */
 #define ATR0_AXI4_SLV0_SRCADDR_PARAM		0x800u
-#define  ATR_SIZE_SHIFT				1
-#define  ATR_IMPL_ENABLE			1
 #define ATR0_AXI4_SLV0_SRC_ADDR			0x804u
 #define ATR0_AXI4_SLV0_TRSL_ADDR_LSB		0x808u
 #define ATR0_AXI4_SLV0_TRSL_ADDR_UDW		0x80cu
 #define ATR0_AXI4_SLV0_TRSL_PARAM		0x810u
-#define  PCIE_TX_RX_INTERFACE			0x00000000u
-#define  PCIE_CONFIG_INTERFACE			0x00000001u
-
-#define ATR_ENTRY_SIZE				32
 
 /* PCIe Controller Phy Regs */
 #define SEC_ERROR_EVENT_CNT			0x20
@@ -268,6 +283,7 @@ struct mc_pcie {
 	struct irq_domain *event_domain;
 	raw_spinlock_t lock;
 	struct mc_msi msi;
+	u64 outbound_range_offset;
 };
 
 struct cause {
@@ -928,36 +944,36 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
 				 phys_addr_t axi_addr, phys_addr_t pci_addr,
 				 size_t size)
 {
-	u32 atr_sz = ilog2(size) - 1;
+	u32 atr_size = ilog2(size) - 1;
 	u32 val;
 
 	if (index == 0)
-		val = PCIE_CONFIG_INTERFACE;
+		val = TRSL_ID_PCIE_CONFIG;
 	else
-		val = PCIE_TX_RX_INTERFACE;
+		val = TRSL_ID_PCIE_TXRX;
 
-	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
+	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
 	       ATR0_AXI4_SLV0_TRSL_PARAM);
 
-	val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) |
+	val = lower_32_bits(axi_addr) | (atr_size << ATR_SIZE_SHIFT) |
 			    ATR_IMPL_ENABLE;
-	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
+	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
 	       ATR0_AXI4_SLV0_SRCADDR_PARAM);
 
 	val = upper_32_bits(axi_addr);
-	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
+	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
 	       ATR0_AXI4_SLV0_SRC_ADDR);
 
 	val = lower_32_bits(pci_addr);
-	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
+	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
 	       ATR0_AXI4_SLV0_TRSL_ADDR_LSB);
 
 	val = upper_32_bits(pci_addr);
-	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
+	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_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);
+	val |= (ATR_PCIE_ATR_SIZE << ATR_SIZE_SHIFT);
 	writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM);
 	writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
 }
@@ -970,14 +986,14 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
 	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
 	struct resource_entry *entry;
 	u64 pci_addr;
-	u32 index = 1;
+	u32 index = 1; /* window 0 used for config space */
 
 	resource_list_for_each_entry(entry, &bridge->windows) {
 		if (resource_type(entry->res) == IORESOURCE_MEM) {
 			pci_addr = entry->res->start - entry->offset;
 			mc_pcie_setup_window(bridge_base_addr, index,
-					     entry->res->start, pci_addr,
-					     resource_size(entry->res));
+					     entry->res->start - port->outbound_range_offset,
+					     pci_addr, resource_size(entry->res));
 			index++;
 		}
 	}
@@ -1093,6 +1109,44 @@ static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port
 	return 0;
 }
 
+static int mc_check_for_parent_range_handling(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;
+	u64 cpu_addr;
+
+	/* find any pcie range */
+	if (of_range_parser_init(&parser, dn)) {
+		dev_err(dev, "missing ranges property\n");
+		return -EINVAL;
+	}
+
+	for_each_of_range(&parser, &range) {
+		cpu_addr = range.cpu_addr;
+		/*
+		 * first range is enough - extend if anyone
+		 * ever needs more than one fabric interface
+		 */
+		break;
+	}
+
+	/* check for one level up; that is enough */
+	dn = of_get_parent(dn);
+	if (dn) {
+		of_range_parser_init(&parser, dn);
+		for_each_of_range(&parser, &range) {
+			/* find the parent range that contains cpu_addr */
+			if (range.cpu_addr > port->outbound_range_offset &&
+			    range.cpu_addr < cpu_addr)
+				port->outbound_range_offset = range.cpu_addr;
+		}
+	}
+
+	return 0;
+}
+
 static int mc_platform_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
@@ -1101,9 +1155,18 @@ static int mc_platform_init(struct pci_config_window *cfg)
 		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
 	int ret;
 
+	/*
+	 * need information about any parent bus that may
+	 * be performing some of the outbound address translation
+	 * to setup outbound address translation tables later
+	 */
+	ret = mc_check_for_parent_range_handling(pdev, port);
+	if (ret)
+		return ret;
+
 	/* Configure address translation table 0 for PCIe config space */
-	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
-			     cfg->res.start,
+	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start - port->outbound_range_offset,
+			     cfg->res.start - port->outbound_range_offset,
 			     resource_size(&cfg->res));
 
 	/* Need some fixups in config space */
-- 
2.25.1

Re: [PATCH v1 7/9] PCI: microchip: Partition outbound address translation

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:55:02PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
> inter connect (fic) busses that encapsulate busses like ABP/AHP and
> AXI-M. Depending on which fic(s) the rootport is wired through to cpu
> space, the rootport driver needs to take account of the address
> translation done by a parent (e.g. fabric) node before setting up its
> own outbound address translation tables to config space and attached
> devices.
> 
> Parse the range properties to determine how much address translation
> needs to be done in the rootport.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 109 +++++++++++++++----
>  1 file changed, 86 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 73856647f321..62f8c5edfd0e 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -85,27 +85,42 @@
>  #define IMSI_ADDR				0x190
>  #define ISTATUS_MSI				0x194
>  
> +#define ATR_WINDOW_DESC_SIZE			32
> +#define ATR_PCIE_ATR_SIZE			0x25
> +#define ATR_SIZE_SHIFT				1
> +#define ATR_IMPL_ENABLE				1
> +
>  /* PCIe Master table init defines */
>  #define ATR0_PCIE_WIN0_SRCADDR_PARAM		0x600u
> -#define  ATR0_PCIE_ATR_SIZE			0x25
> -#define  ATR0_PCIE_ATR_SIZE_SHIFT		1
>  #define ATR0_PCIE_WIN0_SRC_ADDR			0x604u
>  #define ATR0_PCIE_WIN0_TRSL_ADDR_LSB		0x608u
>  #define ATR0_PCIE_WIN0_TRSL_ADDR_UDW		0x60cu
>  #define ATR0_PCIE_WIN0_TRSL_PARAM		0x610u
>  
> +enum {
> +	TRSL_ID_PCIE_TXRX = 0,
> +	TRSL_ID_PCIE_CONFIG = 1,
> +	TRSL_ID_AXI4_LITE_MASTER = 2,
> +	TRSL_ID_AXI4_MASTER_0 = 4,

My enum-fu is pretty bad, but can't we just drop all the numbers here
apart from 4 & the ordering will still be correct?
Maybe that's not ideal from a readability point of view though?

Otherwise, with whatever complaints LKP had & Bjorn's/my previous nits
fixed:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> +	TRSL_ID_AXI4_MASTER_1 = 5,
> +	TRSL_ID_AXI4_MASTER_2 = 6,
> +	TRSL_ID_AXI4_MASTER_3 = 7,
> +	TRSL_ID_AXI4_STREAM_0 = 8,
> +	TRSL_ID_AXI4_STREAM_1 = 9,
> +	TRSL_ID_AXI4_STREAM_2 = 10,
> +	TRSL_ID_AXI4_STREAM_3 = 11,
> +	TRSL_ID_INTERNAL_BRIDGE_REGISTERS = 12,
> +};
> +
> +#define ATR0_PCIE_WIN0_TRSL_MASK_LSB		0x618u
> +#define ATR0_PCIE_WIN0_TRSL_MASK_UDW		0x61cu
> +
>  /* PCIe AXI slave table init defines */
>  #define ATR0_AXI4_SLV0_SRCADDR_PARAM		0x800u
> -#define  ATR_SIZE_SHIFT				1
> -#define  ATR_IMPL_ENABLE			1
>  #define ATR0_AXI4_SLV0_SRC_ADDR			0x804u
>  #define ATR0_AXI4_SLV0_TRSL_ADDR_LSB		0x808u
>  #define ATR0_AXI4_SLV0_TRSL_ADDR_UDW		0x80cu
>  #define ATR0_AXI4_SLV0_TRSL_PARAM		0x810u
> -#define  PCIE_TX_RX_INTERFACE			0x00000000u
> -#define  PCIE_CONFIG_INTERFACE			0x00000001u
> -
> -#define ATR_ENTRY_SIZE				32
>  
>  /* PCIe Controller Phy Regs */
>  #define SEC_ERROR_EVENT_CNT			0x20
> @@ -268,6 +283,7 @@ struct mc_pcie {
>  	struct irq_domain *event_domain;
>  	raw_spinlock_t lock;
>  	struct mc_msi msi;
> +	u64 outbound_range_offset;
>  };
>  
>  struct cause {
> @@ -928,36 +944,36 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
>  				 phys_addr_t axi_addr, phys_addr_t pci_addr,
>  				 size_t size)
>  {
> -	u32 atr_sz = ilog2(size) - 1;
> +	u32 atr_size = ilog2(size) - 1;
>  	u32 val;
>  
>  	if (index == 0)
> -		val = PCIE_CONFIG_INTERFACE;
> +		val = TRSL_ID_PCIE_CONFIG;
>  	else
> -		val = PCIE_TX_RX_INTERFACE;
> +		val = TRSL_ID_PCIE_TXRX;
>  
> -	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
> +	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
>  	       ATR0_AXI4_SLV0_TRSL_PARAM);
>  
> -	val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) |
> +	val = lower_32_bits(axi_addr) | (atr_size << ATR_SIZE_SHIFT) |
>  			    ATR_IMPL_ENABLE;
> -	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
> +	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
>  	       ATR0_AXI4_SLV0_SRCADDR_PARAM);
>  
>  	val = upper_32_bits(axi_addr);
> -	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
> +	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
>  	       ATR0_AXI4_SLV0_SRC_ADDR);
>  
>  	val = lower_32_bits(pci_addr);
> -	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
> +	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_SIZE) +
>  	       ATR0_AXI4_SLV0_TRSL_ADDR_LSB);
>  
>  	val = upper_32_bits(pci_addr);
> -	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
> +	writel(val, bridge_base_addr + (index * ATR_WINDOW_DESC_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);
> +	val |= (ATR_PCIE_ATR_SIZE << ATR_SIZE_SHIFT);
>  	writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM);
>  	writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
>  }
> @@ -970,14 +986,14 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
>  	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
>  	struct resource_entry *entry;
>  	u64 pci_addr;
> -	u32 index = 1;
> +	u32 index = 1; /* window 0 used for config space */
>  
>  	resource_list_for_each_entry(entry, &bridge->windows) {
>  		if (resource_type(entry->res) == IORESOURCE_MEM) {
>  			pci_addr = entry->res->start - entry->offset;
>  			mc_pcie_setup_window(bridge_base_addr, index,
> -					     entry->res->start, pci_addr,
> -					     resource_size(entry->res));
> +					     entry->res->start - port->outbound_range_offset,
> +					     pci_addr, resource_size(entry->res));
>  			index++;
>  		}
>  	}
> @@ -1093,6 +1109,44 @@ static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port
>  	return 0;
>  }
>  
> +static int mc_check_for_parent_range_handling(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;
> +	u64 cpu_addr;
> +
> +	/* find any pcie range */
> +	if (of_range_parser_init(&parser, dn)) {
> +		dev_err(dev, "missing ranges property\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_of_range(&parser, &range) {
> +		cpu_addr = range.cpu_addr;
> +		/*
> +		 * first range is enough - extend if anyone
> +		 * ever needs more than one fabric interface
> +		 */
> +		break;
> +	}
> +
> +	/* check for one level up; that is enough */
> +	dn = of_get_parent(dn);
> +	if (dn) {
> +		of_range_parser_init(&parser, dn);
> +		for_each_of_range(&parser, &range) {
> +			/* find the parent range that contains cpu_addr */
> +			if (range.cpu_addr > port->outbound_range_offset &&
> +			    range.cpu_addr < cpu_addr)
> +				port->outbound_range_offset = range.cpu_addr;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int mc_platform_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
> @@ -1101,9 +1155,18 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	int ret;
>  
> +	/*
> +	 * need information about any parent bus that may
> +	 * be performing some of the outbound address translation
> +	 * to setup outbound address translation tables later
> +	 */
> +	ret = mc_check_for_parent_range_handling(pdev, port);
> +	if (ret)
> +		return ret;
> +
>  	/* Configure address translation table 0 for PCIe config space */
> -	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
> -			     cfg->res.start,
> +	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start - port->outbound_range_offset,
> +			     cfg->res.start - port->outbound_range_offset,
>  			     resource_size(&cfg->res));
>  
>  	/* Need some fixups in config space */
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 8/9] PCI: microchip: Partition inbound address translation

[daire.mcnamara]

From: Daire McNamara <daire.mcnamara@microchip.com>

On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
inter connect (fic) busses that encapsulate busses like ABP/AHP, AXI-S
and AXI-M. Depending on which fic(s) the rootport is wired through to
cpu space, the rootport driver needs to take account of the address
translation done by a parent (e.g. fabric) node before setting up its
own inbound address translation tables from attached devices.

Parse the dma-range properties to determine how much address translation
to perform in the root port and how much is being provided by the
fabric.

Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/pcie-microchip-host.c | 184 ++++++++++++++++++-
 1 file changed, 178 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 62f8c5edfd0e..a90a0a675f14 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -22,6 +22,9 @@
 /* Number of MSI IRQs */
 #define MC_MAX_NUM_MSI_IRQS			32
 
+#define MC_MAX_NUM_INBOUND_WINDOWS		8
+#define MC_ATT_MASK				GENMASK(63, 31)
+
 /* PCIe Bridge Phy and Controller Phy offsets */
 #define MC_PCIE1_BRIDGE_ADDR			0x00008000u
 #define MC_PCIE1_CTRL_ADDR			0x0000a000u
@@ -86,10 +89,13 @@
 #define ISTATUS_MSI				0x194
 
 #define ATR_WINDOW_DESC_SIZE			32
-#define ATR_PCIE_ATR_SIZE			0x25
 #define ATR_SIZE_SHIFT				1
 #define ATR_IMPL_ENABLE				1
 
+#define ATR_PCIE_WIN0_SRCADDR			0x80000000
+#define ATR_PCIE_ATR_SIZE			(512 * 1024 * 1024ul)
+#define ATR_PCIE_NUM_WINDOWS			8
+
 /* PCIe Master table init defines */
 #define ATR0_PCIE_WIN0_SRCADDR_PARAM		0x600u
 #define ATR0_PCIE_WIN0_SRC_ADDR			0x604u
@@ -276,6 +282,12 @@ struct mc_msi {
 	DECLARE_BITMAP(used, MC_MAX_NUM_MSI_IRQS);
 };
 
+struct inbound_windows {
+	u64 axi_addr;
+	u64 pci_addr;
+	u64 size;
+};
+
 struct mc_pcie {
 	void __iomem *axi_base_addr;
 	struct device *dev;
@@ -284,6 +296,8 @@ struct mc_pcie {
 	raw_spinlock_t lock;
 	struct mc_msi msi;
 	u64 outbound_range_offset;
+	u32 num_inbound_windows;
+	struct inbound_windows inbound_windows[MC_MAX_NUM_INBOUND_WINDOWS];
 };
 
 struct cause {
@@ -940,6 +954,46 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port)
 	return mc_allocate_msi_domains(port);
 }
 
+static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port)
+{
+	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
+	phys_addr_t pcie_addr;
+	phys_addr_t axi_addr;
+	u32 atr_size;
+	u32 val;
+	int i;
+
+	for (i = 0; i < port->num_inbound_windows; i++) {
+		atr_size = ilog2(port->inbound_windows[i].size) - 1;
+		atr_size &= GENMASK(5, 0);
+
+		pcie_addr = port->inbound_windows[i].pci_addr;
+
+		val = lower_32_bits(pcie_addr) & GENMASK(31, 12);
+		val |= (atr_size << ATR_SIZE_SHIFT);
+		val |= ATR_IMPL_ENABLE;
+		writel(val, bridge_base_addr +
+		       ATR0_PCIE_WIN0_SRCADDR_PARAM + (i * ATR_WINDOW_DESC_SIZE));
+		writel(upper_32_bits(pcie_addr), bridge_base_addr +
+		       ATR0_PCIE_WIN0_SRC_ADDR + (i * ATR_WINDOW_DESC_SIZE));
+
+		axi_addr = port->inbound_windows[i].axi_addr;
+
+		writel(lower_32_bits(axi_addr), bridge_base_addr +
+		       ATR0_PCIE_WIN0_TRSL_ADDR_LSB + (i * ATR_WINDOW_DESC_SIZE));
+		writel(upper_32_bits(axi_addr), bridge_base_addr +
+		       ATR0_PCIE_WIN0_TRSL_ADDR_UDW + (i * ATR_WINDOW_DESC_SIZE));
+
+		writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr +
+		       ATR0_PCIE_WIN0_TRSL_PARAM + (i * ATR_WINDOW_DESC_SIZE));
+
+		dev_dbg(&pdev->dev, "0x%010llx..0x%010llx -> 0x%010llx\n",
+			pcie_addr, pcie_addr + port->inbound_windows[i].size - 1, axi_addr);
+	}
+
+	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,
 				 size_t size)
@@ -971,11 +1025,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_WINDOW_DESC_SIZE) +
 	       ATR0_AXI4_SLV0_TRSL_ADDR_UDW);
-
-	val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM);
-	val |= (ATR_PCIE_ATR_SIZE << 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,
@@ -1147,6 +1196,119 @@ static int mc_check_for_parent_range_handling(struct platform_device *pdev, stru
 	return 0;
 }
 
+static int mc_check_for_parent_dma_range_handling(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 num_parent_ranges = 0;
+	int num_ranges = 0;
+	struct inbound_windows ranges[MC_MAX_NUM_INBOUND_WINDOWS] = { 0 };
+	u64 start_axi = GENMASK(63, 0);
+	u64 end_axi = 0;
+	u64 start_pci = GENMASK(63, 0);
+	u64 end_pci = 0;
+	s64 size;
+	u64 window_size;
+	int i;
+
+	/* find all dma-ranges */
+	if (of_pci_dma_range_parser_init(&parser, dn)) {
+		dev_err(dev, "missing dma-ranges property\n");
+		return -EINVAL;
+	}
+
+	for_each_of_range(&parser, &range) {
+		if (num_ranges > MC_MAX_NUM_INBOUND_WINDOWS) {
+			dev_err(dev, "too many inbound ranges; %d available tables\n",
+				MC_MAX_NUM_INBOUND_WINDOWS);
+			return -EINVAL;
+		}
+		ranges[num_ranges].axi_addr = range.cpu_addr;
+		ranges[num_ranges].pci_addr = range.pci_addr;
+		ranges[num_ranges].size = range.size;
+
+		num_ranges++;
+	}
+
+	/*
+	 * check for one level up; will need to adjust
+	 * address translation tables for these
+	 */
+	dn = of_get_parent(dn);
+	if (dn) {
+		of_pci_dma_range_parser_init(&parser, dn);
+
+		for_each_of_range(&parser, &range) {
+			if (num_parent_ranges > MC_MAX_NUM_INBOUND_WINDOWS) {
+				dev_err(dev, "too many parent inbound ranges; %d available tables\n",
+					MC_MAX_NUM_INBOUND_WINDOWS);
+				return -EINVAL;
+			}
+			ranges[num_parent_ranges].axi_addr = range.pci_addr;
+			num_parent_ranges++;
+		}
+	}
+
+	if (num_parent_ranges) {
+		if (num_ranges != num_parent_ranges) {
+			dev_err(dev, "num parent inbound ranges must be 0 or match num inbound ranges\n");
+			return -EINVAL;
+		}
+	}
+
+	/* merge ranges */
+	for (i = 0; i < num_ranges; i++) {
+		struct inbound_windows *range = &ranges[i];
+
+		if (range->axi_addr < start_axi) {
+			start_axi = range->axi_addr;
+			start_pci = range->pci_addr;
+		}
+
+		if (range->axi_addr + range->size > end_axi) {
+			end_axi = range->axi_addr + range->size;
+			end_pci = range->pci_addr + range->size;
+		}
+	}
+
+	/* move starts back as far as possible */
+	start_axi &= MC_ATT_MASK;
+	start_pci &= MC_ATT_MASK;
+
+	/* adjust size to take account of that change */
+	size = end_axi - start_axi;
+
+	/* may need to adjust size up to the next largest power of 2 */
+	if (size < 1ull << ilog2(size))
+		size = 1ull << (ilog2(size) + 1);
+
+	window_size = 1ull << (ilog2(size) - 1);
+
+	/* divide merged range into windows */
+	i = 0;
+	while (size > 0 && i < MC_MAX_NUM_INBOUND_WINDOWS) {
+		port->inbound_windows[i].axi_addr = start_axi;
+		port->inbound_windows[i].pci_addr = start_pci;
+		port->inbound_windows[i].size = window_size;
+
+		size -= window_size;
+		start_axi += window_size;
+		start_pci += window_size;
+		i++;
+		port->num_inbound_windows = i;
+	}
+
+	if (size < 0) {
+		dev_err(dev, "insufficient windows to map inbound ranges\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int mc_platform_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
@@ -1164,6 +1326,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	if (ret)
 		return ret;
 
+	/* and similarly, check for inbound address translation */
+	ret = mc_check_for_parent_dma_range_handling(pdev, port);
+	if (ret)
+		return ret;
+
 	/* Configure address translation table 0 for PCIe config space */
 	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start - port->outbound_range_offset,
 			     cfg->res.start - port->outbound_range_offset,
@@ -1177,6 +1344,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)
-- 
2.25.1

Re: [PATCH v1 8/9] PCI: microchip: Partition inbound address translation

[Bjorn Helgaas]

On Wed, Nov 16, 2022 at 01:55:03PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
> inter connect (fic) busses that encapsulate busses like ABP/AHP, AXI-S
> and AXI-M. Depending on which fic(s) the rootport is wired through to
> cpu space, the rootport driver needs to take account of the address
> translation done by a parent (e.g. fabric) node before setting up its
> own inbound address translation tables from attached devices.

Hi Daire, minor nits:

s/inter connect/interconnect/
s/fic/FIC/ ?  Sounds like an initialism similar to ABP, AHP, etc?
s/busses/buses/  Both ok, but "buses" much more common in drivers/pci/
s/cpu/CPU/
s/rootport/Root Port/  I try to follow PCIe spec usage.  Below you use
"root port" (with a space).  At least add the space to make consistent
here.

Some apply to previous commit logs, too, IIRC.

> +	/*
> +	 * check for one level up; will need to adjust
> +	 * address translation tables for these

Wrap to fill 78 columns or so.  Most existing comments in the file are
also capitalized per normal English conventions.

Re: [PATCH v1 8/9] PCI: microchip: Partition inbound address translation

[Conor Dooley]

On Wed, Nov 16, 2022 at 10:49:33AM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 16, 2022 at 01:55:03PM +0000, daire.mcnamara@microchip.com wrote:
> > From: Daire McNamara <daire.mcnamara@microchip.com>
> > 
> > On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
> > inter connect (fic) busses that encapsulate busses like ABP/AHP, AXI-S
> > and AXI-M. Depending on which fic(s) the rootport is wired through to
> > cpu space, the rootport driver needs to take account of the address
> > translation done by a parent (e.g. fabric) node before setting up its
> > own inbound address translation tables from attached devices.
> 
> Hi Daire, minor nits:
> 
> s/inter connect/interconnect/
> s/fic/FIC/ ?  Sounds like an initialism similar to ABP, AHP, etc?

Daire, we've been living a lie. The TRM says "Fabric Interface
Controllers (FICs)" so I think we should switch the that wording.
Fits the acronym better too..

Re: [PATCH v1 8/9] PCI: microchip: Partition inbound address translation

[Conor Dooley]

On Wed, Nov 16, 2022 at 01:55:03PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
> inter connect (fic) busses that encapsulate busses like ABP/AHP, AXI-S
> and AXI-M. Depending on which fic(s) the rootport is wired through to
> cpu space, the rootport driver needs to take account of the address
> translation done by a parent (e.g. fabric) node before setting up its
> own inbound address translation tables from attached devices.
> 
> Parse the dma-range properties to determine how much address translation
> to perform in the root port and how much is being provided by the
> fabric.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 184 ++++++++++++++++++-
>  1 file changed, 178 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 62f8c5edfd0e..a90a0a675f14 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c

> @@ -940,6 +954,46 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port)
>  	return mc_allocate_msi_domains(port);
>  }
>  
> +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port)
> +{
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	phys_addr_t pcie_addr;
> +	phys_addr_t axi_addr;
> +	u32 atr_size;
> +	u32 val;
> +	int i;
> +
> +	for (i = 0; i < port->num_inbound_windows; i++) {
> +		atr_size = ilog2(port->inbound_windows[i].size) - 1;
> +		atr_size &= GENMASK(5, 0);
> +
> +		pcie_addr = port->inbound_windows[i].pci_addr;
> +
> +		val = lower_32_bits(pcie_addr) & GENMASK(31, 12);
> +		val |= (atr_size << ATR_SIZE_SHIFT);
> +		val |= ATR_IMPL_ENABLE;
> +		writel(val, bridge_base_addr +
> +		       ATR0_PCIE_WIN0_SRCADDR_PARAM + (i * ATR_WINDOW_DESC_SIZE));
> +		writel(upper_32_bits(pcie_addr), bridge_base_addr +
> +		       ATR0_PCIE_WIN0_SRC_ADDR + (i * ATR_WINDOW_DESC_SIZE));
> +
> +		axi_addr = port->inbound_windows[i].axi_addr;
> +
> +		writel(lower_32_bits(axi_addr), bridge_base_addr +
> +		       ATR0_PCIE_WIN0_TRSL_ADDR_LSB + (i * ATR_WINDOW_DESC_SIZE));
> +		writel(upper_32_bits(axi_addr), bridge_base_addr +
> +		       ATR0_PCIE_WIN0_TRSL_ADDR_UDW + (i * ATR_WINDOW_DESC_SIZE));
> +
> +		writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr +
> +		       ATR0_PCIE_WIN0_TRSL_PARAM + (i * ATR_WINDOW_DESC_SIZE));
> +
> +		dev_dbg(&pdev->dev, "0x%010llx..0x%010llx -> 0x%010llx\n",
> +			pcie_addr, pcie_addr + port->inbound_windows[i].size - 1, axi_addr);

If you're going to leave the dbg print in, can you make it more verbose
so that the log message stands on its own?

Other than that, I made most of my comments before submission so LGTM..
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[PATCH v1 9/9] riscv: dts: microchip: add parent ranges and dma-ranges for IKRD v2022.09

[daire.mcnamara]

From: Conor Dooley <conor.dooley@microchip.com>

we have replaced the "microchip,matro0" hack property with what was
suggested by Rob - create a parent bus and use ranges and dma-ranges in
the parent bus and pcie device to achieve the address translations we
need. Add the appropriate ranges and dma-ranges for the v2022.09 IKRD
so that it remains functional.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
---
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi | 62 +++++++++++--------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
index 1069134f2e12..51ce87e70b33 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
@@ -26,33 +26,41 @@ i2c2: i2c@40000200 {
 		status = "disabled";
 	};
 
-	pcie: pcie@3000000000 {
-		compatible = "microchip,pcie-host-1.0";
-		#address-cells = <0x3>;
-		#interrupt-cells = <0x1>;
-		#size-cells = <0x2>;
-		device_type = "pci";
-		reg = <0x30 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
-		reg-names = "cfg", "apb";
-		bus-range = <0x0 0x7f>;
-		interrupt-parent = <&plic>;
-		interrupts = <119>;
-		interrupt-map = <0 0 0 1 &pcie_intc 0>,
-				<0 0 0 2 &pcie_intc 1>,
-				<0 0 0 3 &pcie_intc 2>,
-				<0 0 0 4 &pcie_intc 3>;
-		interrupt-map-mask = <0 0 0 7>;
-		clocks = <&ccc_nw CLK_CCC_PLL0_OUT1>, <&ccc_nw CLK_CCC_PLL0_OUT3>;
-		clock-names = "fic1", "fic3";
-		ranges = <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
-		dma-ranges = <0x02000000 0x0 0x00000000 0x0 0x00000000 0x1 0x00000000>;
-		msi-parent = <&pcie>;
-		msi-controller;
-		status = "disabled";
-		pcie_intc: interrupt-controller {
-			#address-cells = <0>;
-			#interrupt-cells = <1>;
-			interrupt-controller;
+	fabric-pcie-bus {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges = <0x0 0x40000000 0x0 0x40000000 0x0 0x20000000>,
+			 <0x30 0x0 0x30 0x0 0x10 0x0>;
+		dma-ranges = <0x0 0x0 0x10 0x0 0x0 0x80000000>;
+		pcie: pcie@3000000000 {
+			compatible = "microchip,pcie-host-1.0";
+			#address-cells = <0x3>;
+			#interrupt-cells = <0x1>;
+			#size-cells = <0x2>;
+			device_type = "pci";
+			reg = <0x30 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
+			reg-names = "cfg", "apb";
+			bus-range = <0x0 0x7f>;
+			interrupt-parent = <&plic>;
+			interrupts = <119>;
+			interrupt-map = <0 0 0 1 &pcie_intc 0>,
+					<0 0 0 2 &pcie_intc 1>,
+					<0 0 0 3 &pcie_intc 2>,
+					<0 0 0 4 &pcie_intc 3>;
+			interrupt-map-mask = <0 0 0 7>;
+			clocks = <&ccc_nw CLK_CCC_PLL0_OUT1>, <&ccc_nw CLK_CCC_PLL0_OUT3>;
+			clock-names = "fic1", "fic3";
+			ranges = <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
+			dma-ranges = <0x3000000 0x10 0x0 0x0 0x0 0x0 0x80000000>;
+			msi-parent = <&pcie>;
+			msi-controller;
+			status = "disabled";
+			pcie_intc: interrupt-controller {
+				#address-cells = <0>;
+				#interrupt-cells = <1>;
+				interrupt-controller;
+			};
 		};
 	};
 
-- 
2.25.1

Re: [PATCH v1 9/9] riscv: dts: microchip: add parent ranges and dma-ranges for IKRD v2022.09

[Conor Dooley]

Hey Rob,

On Wed, Nov 16, 2022 at 01:55:04PM +0000, daire.mcnamara@microchip.com wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> we have replaced the "microchip,matro0" hack property with what was
> suggested by Rob - create a parent bus and use ranges and dma-ranges in
> the parent bus and pcie device to achieve the address translations we
> need. Add the appropriate ranges and dma-ranges for the v2022.09 IKRD
> so that it remains functional.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>

This patch was included as demonstration of what the series results in
DT wise. It's the custom address translation property that you had
NACKED in [0] but done (we think) in the way that you suggested with an
extra, middle-man bus. Could you take a look & see if it fits with what
you requested?

Thanks,
Conor.

0 - https://lore.kernel.org/linux-riscv/20220902142202.2437658-1-daire.mcnamara@microchip.com/

> ---
>  .../dts/microchip/mpfs-icicle-kit-fabric.dtsi | 62 +++++++++++--------
>  1 file changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
> index 1069134f2e12..51ce87e70b33 100644
> --- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
> +++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
> @@ -26,33 +26,41 @@ i2c2: i2c@40000200 {
>  		status = "disabled";
>  	};
>  
> -	pcie: pcie@3000000000 {
> -		compatible = "microchip,pcie-host-1.0";
> -		#address-cells = <0x3>;
> -		#interrupt-cells = <0x1>;
> -		#size-cells = <0x2>;
> -		device_type = "pci";
> -		reg = <0x30 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
> -		reg-names = "cfg", "apb";
> -		bus-range = <0x0 0x7f>;
> -		interrupt-parent = <&plic>;
> -		interrupts = <119>;
> -		interrupt-map = <0 0 0 1 &pcie_intc 0>,
> -				<0 0 0 2 &pcie_intc 1>,
> -				<0 0 0 3 &pcie_intc 2>,
> -				<0 0 0 4 &pcie_intc 3>;
> -		interrupt-map-mask = <0 0 0 7>;
> -		clocks = <&ccc_nw CLK_CCC_PLL0_OUT1>, <&ccc_nw CLK_CCC_PLL0_OUT3>;
> -		clock-names = "fic1", "fic3";
> -		ranges = <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
> -		dma-ranges = <0x02000000 0x0 0x00000000 0x0 0x00000000 0x1 0x00000000>;
> -		msi-parent = <&pcie>;
> -		msi-controller;
> -		status = "disabled";
> -		pcie_intc: interrupt-controller {
> -			#address-cells = <0>;
> -			#interrupt-cells = <1>;
> -			interrupt-controller;
> +	fabric-pcie-bus {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges = <0x0 0x40000000 0x0 0x40000000 0x0 0x20000000>,
> +			 <0x30 0x0 0x30 0x0 0x10 0x0>;
> +		dma-ranges = <0x0 0x0 0x10 0x0 0x0 0x80000000>;
> +		pcie: pcie@3000000000 {
> +			compatible = "microchip,pcie-host-1.0";
> +			#address-cells = <0x3>;
> +			#interrupt-cells = <0x1>;
> +			#size-cells = <0x2>;
> +			device_type = "pci";
> +			reg = <0x30 0x0 0x0 0x8000000>, <0x0 0x43000000 0x0 0x10000>;
> +			reg-names = "cfg", "apb";
> +			bus-range = <0x0 0x7f>;
> +			interrupt-parent = <&plic>;
> +			interrupts = <119>;
> +			interrupt-map = <0 0 0 1 &pcie_intc 0>,
> +					<0 0 0 2 &pcie_intc 1>,
> +					<0 0 0 3 &pcie_intc 2>,
> +					<0 0 0 4 &pcie_intc 3>;
> +			interrupt-map-mask = <0 0 0 7>;
> +			clocks = <&ccc_nw CLK_CCC_PLL0_OUT1>, <&ccc_nw CLK_CCC_PLL0_OUT3>;
> +			clock-names = "fic1", "fic3";
> +			ranges = <0x3000000 0x0 0x8000000 0x30 0x8000000 0x0 0x80000000>;
> +			dma-ranges = <0x3000000 0x10 0x0 0x0 0x0 0x0 0x80000000>;
> +			msi-parent = <&pcie>;
> +			msi-controller;
> +			status = "disabled";
> +			pcie_intc: interrupt-controller {
> +				#address-cells = <0>;
> +				#interrupt-cells = <1>;
> +				interrupt-controller;
> +			};
>  		};
>  	};
>  
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/9] PCI: microchip: Partition address translations

[Conor Dooley]

Hey Daire,

On Wed, Nov 16, 2022 at 01:54:55PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Microchip PolarFire SoC is a 64-bit device and has DDR starting at
> 0x80000000 and 0x1000000000. Its PCIe rootport is connected to the CPU
> Coreplex via an FPGA fabric. The AXI connections between the Coreplex and
> the fabric are 64-bit and the AXI connections between the fabric and the
> rootport are 32-bit.  For the CPU CorePlex to act as an AXI-Master to the
> PCIe devices and for the PCIe devices to act as bus masters to DDR at these
> base addresses, the fabric can be customised to add/remove offsets for bits
> 38-32 in each direction. These offsets, if present, vary with each
> customer's design.
> 
> To support this variety, the rootport driver must know how much address
> translation (both inbound and outbound) is performed by a particular
> customer design and how much address translation must be provided by the
> rootport.
> 
> This patchset contains a parent/child dma-ranges scheme suggested by Rob
> Herring. It creates an FPGA PCIe parent bus which wraps the PCIe rootport
> and implements a parsing scheme where the root port identifies what address
> translations are performed by the FPGA fabric parent bus, and what
> address translations must be done by the rootport itself.

I've tried this scheme with a bunch of different PCI configurations, and
it holds water, so I am happy with it :) Hopefully Rob is a lot happier
with this version of it too!

It's been long enough that I think you should be good to submit a
cleaned up version, provided Rob's happy on the DT side I think.

Thanks,
Conor.

> See https://lore.kernel.org/linux-pci/20220902142202.2437658-1-daire.mcnamara@microchip.com/
> for the relevant previous patch submission discussion.
> 
> It also re-partitions the probe() and init() functions as suggested by
> Bjorn Helgaas to make them more maintainable as the init() function had
> become too large.
> 
> It also contains some minor fixes and clean-ups that are pre-requisites:
> - to align register, offset, and mask names with the hardware documentation
>   and to have the register definitions appear in the same order as in the
>   hardware documentation;
> - to harvest the MSI information from the hardware configuration register
>   as these depend on the FPGA fabric design and can vary with different
>   customer designs;
> - to clean up interrupt initialisation to make it more maintainable;
> - to fix SEC and DED interrupt handling.
> 
> I expect Conor will take the dts patch via the soc tree once the PCIe parts
> of the series are accepted.
> 
> Conor Dooley (1):
>   riscv: dts: microchip: add parent ranges and dma-ranges for IKRD
>     v2022.09
> 
> Daire McNamara (8):
>   PCI: microchip: Align register, offset, and mask names with hw docs
>   PCI: microchip: Correct the DED and SEC interrupt bit offsets
>   PCI: microchip: Enable event handlers to access bridge and ctrl ptrs
>   PCI: microchip: Clean up initialisation of interrupts
>   PCI: microchip: Gather MSI information from hardware config registers
>   PCI: microchip: Re-partition code between probe() and init()
>   PCI: microchip: Partition outbound address translation
>   PCI: microchip: Partition inbound address translation
> 
>  .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  62 +-
>  drivers/pci/controller/pcie-microchip-host.c  | 676 +++++++++++++-----
>  2 files changed, 522 insertions(+), 216 deletions(-)
> 
> 
> base-commit: 3c1f24109dfc4fb1a3730ed237e50183c6bb26b3
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv