[PATCH v5 0/2] PCI: microchip: support using either instance 1 or 2

[PATCH v5 0/2] PCI: microchip: support using either instance 1 or 2

[Conor Dooley]

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

The current driver and binding for PolarFire SoC's PCI controller assume
that the root port instance in use is instance 1. The second reg
property constitutes the region encompassing both "control" and "bridge"
registers for both instances. In the driver, a fixed offset is applied to
find the base addresses for instance 1's "control" and "bridge"
registers. The BeagleV Fire uses root port instance 2, so something must
be done so that software can differentiate. This series splits the
second reg property in two, with dedicated "control" and "bridge"
entries so that either instance can be used.

Cheers,
Conor.

v5:
- rebase on top of 6.11-rc1, which brought about a lot of driver change
  due to the plda common driver creation - although little actually
  changed in terms of the lines edited in this patch.

v4:
- fix a cocci warning reported off list about an inconsistent variable
  used between IS_ERR() and PTR_ERR() calls.

v3:
- rename a variable in probe s/axi/apb/

v2:
- try the new reg format before the old one to avoid warnings in the
  good case
- reword $subject for 2/2

CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
CC: Krzysztof Wilczyński <kw@linux.com>
CC: Rob Herring <robh@kernel.org>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
CC: linux-pci@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-riscv@lists.infradead.org

Conor Dooley (2):
  dt-bindings: PCI: microchip,pcie-host: fix reg properties
  PCI: microchip: rework reg region handing to support using either
    instance 1 or 2

 .../bindings/pci/microchip,pcie-host.yaml     |  11 +-
 .../pci/plda,xpressrich3-axi-common.yaml      |  14 ++-
 .../bindings/pci/starfive,jh7110-pcie.yaml    |   7 ++
 .../pci/controller/plda/pcie-microchip-host.c | 116 +++++++++---------
 4 files changed, 87 insertions(+), 61 deletions(-)

-- 
2.43.0

[PATCH v5 1/2] dt-bindings: PCI: microchip,pcie-host: fix reg properties

[Conor Dooley]

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

The PCI host controller on PolarFire SoC has multiple "instances", each
with their own bridge and ctrl address spaces. The original binding has
an "apb" register region, and it is expected to be set to the base
address of the host controllers register space. Some defines in the
Linux driver were used to compute the addresses of the bridge and ctrl
address ranges corresponding to instance1. Some customers want to use
instance2 however and that requires changing the defines in the driver,
which is clearly not a portable solution.

Remove this "apb" register region from the binding and add "bridge" &
"ctrl" regions instead, that will directly communicate the address of
these regions

Fixes: 6ee6c89aac35 ("dt-bindings: PCI: microchip: Add Microchip PolarFire host binding")
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/pci/microchip,pcie-host.yaml          | 11 +++++++++--
 .../bindings/pci/plda,xpressrich3-axi-common.yaml  | 14 ++++++++++----
 .../bindings/pci/starfive,jh7110-pcie.yaml         |  7 +++++++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
index 612633ba59e2..2e1547569702 100644
--- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
@@ -17,6 +17,12 @@ properties:
   compatible:
     const: microchip,pcie-host-1.0 # PolarFire
 
+  reg:
+    minItems: 3
+
+  reg-names:
+    minItems: 3
+
   clocks:
     description:
       Fabric Interface Controllers, FICs, are the interface between the FPGA
@@ -62,8 +68,9 @@ examples:
             pcie0: pcie@2030000000 {
                     compatible = "microchip,pcie-host-1.0";
                     reg = <0x0 0x70000000 0x0 0x08000000>,
-                          <0x0 0x43000000 0x0 0x00010000>;
-                    reg-names = "cfg", "apb";
+                          <0x0 0x43008000 0x0 0x00002000>,
+                          <0x0 0x4300a000 0x0 0x00002000>;
+                    reg-names = "cfg", "bridge", "ctrl";
                     device_type = "pci";
                     #address-cells = <3>;
                     #size-cells = <2>;
diff --git a/Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml b/Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml
index 7a57a80052a0..039eecdbd6aa 100644
--- a/Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml
+++ b/Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml
@@ -18,12 +18,18 @@ allOf:
 
 properties:
   reg:
-    maxItems: 2
+    maxItems: 3
+    minItems: 2
 
   reg-names:
-    items:
-      - const: cfg
-      - const: apb
+    oneOf:
+      - items:
+          - const: cfg
+          - const: apb
+      - items:
+          - const: cfg
+          - const: bridge
+          - const: ctrl
 
   interrupts:
     minItems: 1
diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
index 67151aaa3948..5f432452c815 100644
--- a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
@@ -16,6 +16,13 @@ properties:
   compatible:
     const: starfive,jh7110-pcie
 
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    maxItems: 2
+
   clocks:
     items:
       - description: NOC bus clock
-- 
2.43.0

[PATCH v5 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Conor Dooley]

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

The PCI host controller on PolarFire SoC has multiple "instances", each
with their own bridge and ctrl address spaces. The original binding has
an "apb" register region, and it is expected to be set to the base
address of the host controllers register space. Defines in the driver
were used to compute the addresses of the bridge and ctrl address ranges
corresponding to instance1. Some customers want to use instance0 however
and that requires changing the defines in the driver, which is clearly
not a portable solution.

The binding has been changed from a single register region to a pair,
corresponding to the bridge and ctrl regions respectively, so modify the
driver to read these regions directly from the devicetree rather than
compute them from the base address of the abp region.

To maintain backwards compatibility with the existing binding, the
driver retains code to handle the "abp" reg and computes the base
address of the bridge and ctrl regions using the defines if it is
present. reg-names has always been a required property, so this is
safe to do.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Dropped Daire's Ack due to substantial rebasing.
---
 .../pci/controller/plda/pcie-microchip-host.c | 116 +++++++++---------
 1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
index 48f60a04b740..57f35290c83b 100644
--- a/drivers/pci/controller/plda/pcie-microchip-host.c
+++ b/drivers/pci/controller/plda/pcie-microchip-host.c
@@ -25,9 +25,6 @@
 #define MC_PCIE1_BRIDGE_ADDR			0x00008000u
 #define MC_PCIE1_CTRL_ADDR			0x0000a000u
 
-#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_EVENT_CNT			0x20
 #define DED_ERROR_EVENT_CNT			0x24
@@ -128,7 +125,6 @@
 	[EVENT_LOCAL_ ## x] = { __stringify(x), s }
 
 #define PCIE_EVENT(x) \
-	.base = MC_PCIE_CTRL_ADDR, \
 	.offset = PCIE_EVENT_INT, \
 	.mask_offset = PCIE_EVENT_INT, \
 	.mask_high = 1, \
@@ -136,7 +132,6 @@
 	.enb_mask = PCIE_EVENT_INT_ENB_MASK
 
 #define SEC_EVENT(x) \
-	.base = MC_PCIE_CTRL_ADDR, \
 	.offset = SEC_ERROR_INT, \
 	.mask_offset = SEC_ERROR_INT_MASK, \
 	.mask = SEC_ERROR_INT_ ## x ## _INT, \
@@ -144,7 +139,6 @@
 	.enb_mask = 0
 
 #define DED_EVENT(x) \
-	.base = MC_PCIE_CTRL_ADDR, \
 	.offset = DED_ERROR_INT, \
 	.mask_offset = DED_ERROR_INT_MASK, \
 	.mask_high = 1, \
@@ -152,7 +146,6 @@
 	.enb_mask = 0
 
 #define LOCAL_EVENT(x) \
-	.base = MC_PCIE_BRIDGE_ADDR, \
 	.offset = ISTATUS_LOCAL, \
 	.mask_offset = IMASK_LOCAL, \
 	.mask_high = 0, \
@@ -179,7 +172,8 @@ struct event_map {
 
 struct mc_pcie {
 	struct plda_pcie_rp plda;
-	void __iomem *axi_base_addr;
+	void __iomem *bridge_base_addr;
+	void __iomem *ctrl_base_addr;
 };
 
 struct cause {
@@ -253,7 +247,6 @@ static struct event_map local_status_to_event[] = {
 };
 
 static struct {
-	u32 base;
 	u32 offset;
 	u32 mask;
 	u32 shift;
@@ -325,8 +318,7 @@ static inline u32 reg_to_event(u32 reg, struct event_map field)
 
 static u32 pcie_events(struct mc_pcie *port)
 {
-	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
-	u32 reg = readl_relaxed(ctrl_base_addr + PCIE_EVENT_INT);
+	u32 reg = readl_relaxed(port->ctrl_base_addr + PCIE_EVENT_INT);
 	u32 val = 0;
 	int i;
 
@@ -338,8 +330,7 @@ static u32 pcie_events(struct mc_pcie *port)
 
 static u32 sec_errors(struct mc_pcie *port)
 {
-	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
-	u32 reg = readl_relaxed(ctrl_base_addr + SEC_ERROR_INT);
+	u32 reg = readl_relaxed(port->ctrl_base_addr + SEC_ERROR_INT);
 	u32 val = 0;
 	int i;
 
@@ -351,8 +342,7 @@ static u32 sec_errors(struct mc_pcie *port)
 
 static u32 ded_errors(struct mc_pcie *port)
 {
-	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
-	u32 reg = readl_relaxed(ctrl_base_addr + DED_ERROR_INT);
+	u32 reg = readl_relaxed(port->ctrl_base_addr + DED_ERROR_INT);
 	u32 val = 0;
 	int i;
 
@@ -364,8 +354,7 @@ static u32 ded_errors(struct mc_pcie *port)
 
 static u32 local_events(struct mc_pcie *port)
 {
-	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
-	u32 reg = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
+	u32 reg = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
 	u32 val = 0;
 	int i;
 
@@ -412,8 +401,12 @@ static void mc_ack_event_irq(struct irq_data *data)
 	void __iomem *addr;
 	u32 mask;
 
-	addr = mc_port->axi_base_addr + event_descs[event].base +
-		event_descs[event].offset;
+	if (event_descs[event].offset == ISTATUS_LOCAL)
+		addr = mc_port->bridge_base_addr;
+	else
+		addr = mc_port->ctrl_base_addr;
+
+	addr += event_descs[event].offset;
 	mask = event_descs[event].mask;
 	mask |= event_descs[event].enb_mask;
 
@@ -429,8 +422,12 @@ static void mc_mask_event_irq(struct irq_data *data)
 	u32 mask;
 	u32 val;
 
-	addr = mc_port->axi_base_addr + event_descs[event].base +
-		event_descs[event].mask_offset;
+	if (event_descs[event].offset == ISTATUS_LOCAL)
+		addr = mc_port->bridge_base_addr;
+	else
+		addr = mc_port->ctrl_base_addr;
+
+	addr += event_descs[event].mask_offset;
 	mask = event_descs[event].mask;
 	if (event_descs[event].enb_mask) {
 		mask <<= PCIE_EVENT_INT_ENB_SHIFT;
@@ -460,8 +457,12 @@ static void mc_unmask_event_irq(struct irq_data *data)
 	u32 mask;
 	u32 val;
 
-	addr = mc_port->axi_base_addr + event_descs[event].base +
-		event_descs[event].mask_offset;
+	if (event_descs[event].offset == ISTATUS_LOCAL)
+		addr = mc_port->bridge_base_addr;
+	else
+		addr = mc_port->ctrl_base_addr;
+
+	addr += event_descs[event].mask_offset;
 	mask = event_descs[event].mask;
 
 	if (event_descs[event].enb_mask)
@@ -554,26 +555,20 @@ static const struct plda_event mc_event = {
 
 static inline void mc_clear_secs(struct mc_pcie *port)
 {
-	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
-
-	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
+	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
 		       SEC_ERROR_INT);
-	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
+	writel_relaxed(0, port->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(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
+	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
 		       DED_ERROR_INT);
-	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
+	writel_relaxed(0, port->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;
 
 	/* Ensure ECC bypass is enabled */
@@ -581,22 +576,22 @@ static void mc_disable_interrupts(struct mc_pcie *port)
 	      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);
+	writel_relaxed(val, port->ctrl_base_addr + ECC_CONTROL);
 
 	/* Disable SEC errors and clear any outstanding */
-	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
+	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
 		       SEC_ERROR_INT_MASK);
 	mc_clear_secs(port);
 
 	/* Disable DED errors and clear any outstanding */
-	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
+	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
 		       DED_ERROR_INT_MASK);
 	mc_clear_deds(port);
 
 	/* 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);
+	writel_relaxed(0, port->bridge_base_addr + IMASK_LOCAL);
+	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_LOCAL);
+	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_MSI);
 
 	/* Disable PCIe events and clear any outstanding */
 	val = PCIE_EVENT_INT_L2_EXIT_INT |
@@ -605,11 +600,11 @@ static void mc_disable_interrupts(struct mc_pcie *port)
 	      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);
+	writel_relaxed(val, port->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);
+	writel_relaxed(0, port->bridge_base_addr + IMASK_HOST);
+	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_HOST);
 }
 
 static int mc_platform_init(struct pci_config_window *cfg)
@@ -617,12 +612,10 @@ static int mc_platform_init(struct pci_config_window *cfg)
 	struct device *dev = cfg->parent;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
-	void __iomem *bridge_base_addr =
-		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
 	int ret;
 
 	/* Configure address translation table 0 for PCIe config space */
-	plda_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
+	plda_pcie_setup_window(port->bridge_base_addr, 0, cfg->res.start,
 			       cfg->res.start,
 			       resource_size(&cfg->res));
 
@@ -649,7 +642,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
 static int mc_host_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	void __iomem *bridge_base_addr;
+	void __iomem *apb_base_addr;
 	struct plda_pcie_rp *plda;
 	int ret;
 	u32 val;
@@ -661,30 +654,43 @@ static int mc_host_probe(struct platform_device *pdev)
 	plda = &port->plda;
 	plda->dev = dev;
 
-	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(port->axi_base_addr))
-		return PTR_ERR(port->axi_base_addr);
+	port->bridge_base_addr = devm_platform_ioremap_resource_byname(pdev, "bridge");
+	port->ctrl_base_addr = devm_platform_ioremap_resource_byname(pdev, "ctrl");
+	if (!IS_ERR(port->bridge_base_addr) && !IS_ERR(port->ctrl_base_addr))
+		goto addrs_set;
 
+	/*
+	 * The original, incorrect, binding that lumped the control and
+	 * bridge addresses together still needs to be handled by the driver.
+	 */
+	apb_base_addr = devm_platform_ioremap_resource_byname(pdev, "apb");
+	if (IS_ERR(apb_base_addr))
+		return dev_err_probe(dev, PTR_ERR(apb_base_addr),
+				     "both legacy apb register and ctrl/bridge regions missing");
+
+	port->bridge_base_addr = apb_base_addr + MC_PCIE1_BRIDGE_ADDR;
+	port->ctrl_base_addr = apb_base_addr + MC_PCIE1_CTRL_ADDR;
+
+addrs_set:
 	mc_disable_interrupts(port);
 
-	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
-	plda->bridge_addr = bridge_base_addr;
+	plda->bridge_addr = port->bridge_base_addr;
 	plda->num_events = NUM_EVENTS;
 
 	/* Allow enabling MSI by disabling MSI-X */
-	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
+	val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
 	val &= ~MSIX_CAP_MASK;
-	writel(val, bridge_base_addr + PCIE_PCI_IRQ_DW0);
+	writel(val, port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
 
 	/* Pick num vectors from bitfile programmed onto FPGA fabric */
-	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
+	val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
 	val &= NUM_MSI_MSGS_MASK;
 	val >>= NUM_MSI_MSGS_SHIFT;
 
 	plda->msi.num_vectors = 1 << val;
 
 	/* Pick vector address from design */
-	plda->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
+	plda->msi.vector_phy = readl_relaxed(port->bridge_base_addr + IMSI_ADDR);
 
 	ret = mc_pcie_init_clks(dev);
 	if (ret) {
-- 
2.43.0

Re: [PATCH v5 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Bjorn Helgaas]

On Wed, Aug 14, 2024 at 09:08:42AM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The PCI host controller on PolarFire SoC has multiple "instances", each
> with their own bridge and ctrl address spaces. The original binding has
> an "apb" register region, and it is expected to be set to the base
> address of the host controllers register space. Defines in the driver
> were used to compute the addresses of the bridge and ctrl address ranges
> corresponding to instance1. Some customers want to use instance0 however
> and that requires changing the defines in the driver, which is clearly
> not a portable solution.

The subject mentions "instance 1 or 2".

This paragraph implies adding support for "instance0" ("customers want
to use instance0").

The DT patch suggests that we're adding support for "instance2"
("customers want to use instance2").

Both patches suggest that the existing support is for "instance 1".

Maybe what's being added is "instance 2", and this commit log should
s/instance0/instance 2/ ?  And probably s/instance1/instance 1/ so the
style is consistent?

Is this a "pick one or the other but not both" situation, or does this
device support two independent PCIe controllers?

I first thought this driver supported a single PCIe controller, and
you were adding support for a second independent controller.

But the fact that you say "the [singular] host controller on
PolarFire", and you're not changing mc_host_probe() to call
pci_host_common_probe() more than once makes me think there is only a
single PCIe controller, and for some reason you can choose to operate
it using either register set 1 or register set 2.

> The binding has been changed from a single register region to a pair,
> corresponding to the bridge and ctrl regions respectively, so modify the
> driver to read these regions directly from the devicetree rather than
> compute them from the base address of the abp region.
> 
> To maintain backwards compatibility with the existing binding, the
> driver retains code to handle the "abp" reg and computes the base
> address of the bridge and ctrl regions using the defines if it is
> present. reg-names has always been a required property, so this is
> safe to do.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Dropped Daire's Ack due to substantial rebasing.
> ---
>  .../pci/controller/plda/pcie-microchip-host.c | 116 +++++++++---------
>  1 file changed, 61 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
> index 48f60a04b740..57f35290c83b 100644
> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
> @@ -25,9 +25,6 @@
>  #define MC_PCIE1_BRIDGE_ADDR			0x00008000u
>  #define MC_PCIE1_CTRL_ADDR			0x0000a000u
>  
> -#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_EVENT_CNT			0x20
>  #define DED_ERROR_EVENT_CNT			0x24
> @@ -128,7 +125,6 @@
>  	[EVENT_LOCAL_ ## x] = { __stringify(x), s }
>  
>  #define PCIE_EVENT(x) \
> -	.base = MC_PCIE_CTRL_ADDR, \
>  	.offset = PCIE_EVENT_INT, \
>  	.mask_offset = PCIE_EVENT_INT, \
>  	.mask_high = 1, \
> @@ -136,7 +132,6 @@
>  	.enb_mask = PCIE_EVENT_INT_ENB_MASK
>  
>  #define SEC_EVENT(x) \
> -	.base = MC_PCIE_CTRL_ADDR, \
>  	.offset = SEC_ERROR_INT, \
>  	.mask_offset = SEC_ERROR_INT_MASK, \
>  	.mask = SEC_ERROR_INT_ ## x ## _INT, \
> @@ -144,7 +139,6 @@
>  	.enb_mask = 0
>  
>  #define DED_EVENT(x) \
> -	.base = MC_PCIE_CTRL_ADDR, \
>  	.offset = DED_ERROR_INT, \
>  	.mask_offset = DED_ERROR_INT_MASK, \
>  	.mask_high = 1, \
> @@ -152,7 +146,6 @@
>  	.enb_mask = 0
>  
>  #define LOCAL_EVENT(x) \
> -	.base = MC_PCIE_BRIDGE_ADDR, \
>  	.offset = ISTATUS_LOCAL, \
>  	.mask_offset = IMASK_LOCAL, \
>  	.mask_high = 0, \
> @@ -179,7 +172,8 @@ struct event_map {
>  
>  struct mc_pcie {
>  	struct plda_pcie_rp plda;
> -	void __iomem *axi_base_addr;
> +	void __iomem *bridge_base_addr;
> +	void __iomem *ctrl_base_addr;
>  };
>  
>  struct cause {
> @@ -253,7 +247,6 @@ static struct event_map local_status_to_event[] = {
>  };
>  
>  static struct {
> -	u32 base;
>  	u32 offset;
>  	u32 mask;
>  	u32 shift;
> @@ -325,8 +318,7 @@ static inline u32 reg_to_event(u32 reg, struct event_map field)
>  
>  static u32 pcie_events(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -	u32 reg = readl_relaxed(ctrl_base_addr + PCIE_EVENT_INT);
> +	u32 reg = readl_relaxed(port->ctrl_base_addr + PCIE_EVENT_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -338,8 +330,7 @@ static u32 pcie_events(struct mc_pcie *port)
>  
>  static u32 sec_errors(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -	u32 reg = readl_relaxed(ctrl_base_addr + SEC_ERROR_INT);
> +	u32 reg = readl_relaxed(port->ctrl_base_addr + SEC_ERROR_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -351,8 +342,7 @@ static u32 sec_errors(struct mc_pcie *port)
>  
>  static u32 ded_errors(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -	u32 reg = readl_relaxed(ctrl_base_addr + DED_ERROR_INT);
> +	u32 reg = readl_relaxed(port->ctrl_base_addr + DED_ERROR_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -364,8 +354,7 @@ static u32 ded_errors(struct mc_pcie *port)
>  
>  static u32 local_events(struct mc_pcie *port)
>  {
> -	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -	u32 reg = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> +	u32 reg = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
>  	u32 val = 0;
>  	int i;
>  
> @@ -412,8 +401,12 @@ static void mc_ack_event_irq(struct irq_data *data)
>  	void __iomem *addr;
>  	u32 mask;
>  
> -	addr = mc_port->axi_base_addr + event_descs[event].base +
> -		event_descs[event].offset;
> +	if (event_descs[event].offset == ISTATUS_LOCAL)
> +		addr = mc_port->bridge_base_addr;
> +	else
> +		addr = mc_port->ctrl_base_addr;
> +
> +	addr += event_descs[event].offset;
>  	mask = event_descs[event].mask;
>  	mask |= event_descs[event].enb_mask;
>  
> @@ -429,8 +422,12 @@ static void mc_mask_event_irq(struct irq_data *data)
>  	u32 mask;
>  	u32 val;
>  
> -	addr = mc_port->axi_base_addr + event_descs[event].base +
> -		event_descs[event].mask_offset;
> +	if (event_descs[event].offset == ISTATUS_LOCAL)
> +		addr = mc_port->bridge_base_addr;
> +	else
> +		addr = mc_port->ctrl_base_addr;
> +
> +	addr += event_descs[event].mask_offset;
>  	mask = event_descs[event].mask;
>  	if (event_descs[event].enb_mask) {
>  		mask <<= PCIE_EVENT_INT_ENB_SHIFT;
> @@ -460,8 +457,12 @@ static void mc_unmask_event_irq(struct irq_data *data)
>  	u32 mask;
>  	u32 val;
>  
> -	addr = mc_port->axi_base_addr + event_descs[event].base +
> -		event_descs[event].mask_offset;
> +	if (event_descs[event].offset == ISTATUS_LOCAL)
> +		addr = mc_port->bridge_base_addr;
> +	else
> +		addr = mc_port->ctrl_base_addr;
> +
> +	addr += event_descs[event].mask_offset;
>  	mask = event_descs[event].mask;
>  
>  	if (event_descs[event].enb_mask)
> @@ -554,26 +555,20 @@ static const struct plda_event mc_event = {
>  
>  static inline void mc_clear_secs(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -
> -	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
>  		       SEC_ERROR_INT);
> -	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> +	writel_relaxed(0, port->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(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
>  		       DED_ERROR_INT);
> -	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> +	writel_relaxed(0, port->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;
>  
>  	/* Ensure ECC bypass is enabled */
> @@ -581,22 +576,22 @@ static void mc_disable_interrupts(struct mc_pcie *port)
>  	      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);
> +	writel_relaxed(val, port->ctrl_base_addr + ECC_CONTROL);
>  
>  	/* Disable SEC errors and clear any outstanding */
> -	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
>  		       SEC_ERROR_INT_MASK);
>  	mc_clear_secs(port);
>  
>  	/* Disable DED errors and clear any outstanding */
> -	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
>  		       DED_ERROR_INT_MASK);
>  	mc_clear_deds(port);
>  
>  	/* 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);
> +	writel_relaxed(0, port->bridge_base_addr + IMASK_LOCAL);
> +	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_LOCAL);
> +	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_MSI);
>  
>  	/* Disable PCIe events and clear any outstanding */
>  	val = PCIE_EVENT_INT_L2_EXIT_INT |
> @@ -605,11 +600,11 @@ static void mc_disable_interrupts(struct mc_pcie *port)
>  	      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);
> +	writel_relaxed(val, port->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);
> +	writel_relaxed(0, port->bridge_base_addr + IMASK_HOST);
> +	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_HOST);
>  }
>  
>  static int mc_platform_init(struct pci_config_window *cfg)
> @@ -617,12 +612,10 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	struct device *dev = cfg->parent;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	int ret;
>  
>  	/* Configure address translation table 0 for PCIe config space */
> -	plda_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
> +	plda_pcie_setup_window(port->bridge_base_addr, 0, cfg->res.start,
>  			       cfg->res.start,
>  			       resource_size(&cfg->res));
>  
> @@ -649,7 +642,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  static int mc_host_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	void __iomem *bridge_base_addr;
> +	void __iomem *apb_base_addr;
>  	struct plda_pcie_rp *plda;
>  	int ret;
>  	u32 val;
> @@ -661,30 +654,43 @@ static int mc_host_probe(struct platform_device *pdev)
>  	plda = &port->plda;
>  	plda->dev = dev;
>  
> -	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> -	if (IS_ERR(port->axi_base_addr))
> -		return PTR_ERR(port->axi_base_addr);
> +	port->bridge_base_addr = devm_platform_ioremap_resource_byname(pdev, "bridge");
> +	port->ctrl_base_addr = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> +	if (!IS_ERR(port->bridge_base_addr) && !IS_ERR(port->ctrl_base_addr))
> +		goto addrs_set;
>  
> +	/*
> +	 * The original, incorrect, binding that lumped the control and
> +	 * bridge addresses together still needs to be handled by the driver.
> +	 */
> +	apb_base_addr = devm_platform_ioremap_resource_byname(pdev, "apb");
> +	if (IS_ERR(apb_base_addr))
> +		return dev_err_probe(dev, PTR_ERR(apb_base_addr),
> +				     "both legacy apb register and ctrl/bridge regions missing");
> +
> +	port->bridge_base_addr = apb_base_addr + MC_PCIE1_BRIDGE_ADDR;
> +	port->ctrl_base_addr = apb_base_addr + MC_PCIE1_CTRL_ADDR;
> +
> +addrs_set:
>  	mc_disable_interrupts(port);
>  
> -	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -	plda->bridge_addr = bridge_base_addr;
> +	plda->bridge_addr = port->bridge_base_addr;
>  	plda->num_events = NUM_EVENTS;
>  
>  	/* Allow enabling MSI by disabling MSI-X */
> -	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>  	val &= ~MSIX_CAP_MASK;
> -	writel(val, bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	writel(val, port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>  
>  	/* Pick num vectors from bitfile programmed onto FPGA fabric */
> -	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>  	val &= NUM_MSI_MSGS_MASK;
>  	val >>= NUM_MSI_MSGS_SHIFT;
>  
>  	plda->msi.num_vectors = 1 << val;
>  
>  	/* Pick vector address from design */
> -	plda->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
> +	plda->msi.vector_phy = readl_relaxed(port->bridge_base_addr + IMSI_ADDR);
>  
>  	ret = mc_pcie_init_clks(dev);
>  	if (ret) {
> -- 
> 2.43.0
> 

Re: [PATCH v5 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Krzysztof Wilczyński]

Hello,

> > The PCI host controller on PolarFire SoC has multiple "instances", each
> > with their own bridge and ctrl address spaces. The original binding has
> > an "apb" register region, and it is expected to be set to the base
> > address of the host controllers register space. Defines in the driver
> > were used to compute the addresses of the bridge and ctrl address ranges
> > corresponding to instance1. Some customers want to use instance0 however
> > and that requires changing the defines in the driver, which is clearly
> > not a portable solution.
> 
> The subject mentions "instance 1 or 2".
> 
> This paragraph implies adding support for "instance0" ("customers want
> to use instance0").
> 
> The DT patch suggests that we're adding support for "instance2"
> ("customers want to use instance2").
> 
> Both patches suggest that the existing support is for "instance 1".
> 
> Maybe what's being added is "instance 2", and this commit log should
> s/instance0/instance 2/ ?  And probably s/instance1/instance 1/ so the
> style is consistent?
> 
> Is this a "pick one or the other but not both" situation, or does this
> device support two independent PCIe controllers?
> 
> I first thought this driver supported a single PCIe controller, and
> you were adding support for a second independent controller.
> 
> But the fact that you say "the [singular] host controller on
> PolarFire", and you're not changing mc_host_probe() to call
> pci_host_common_probe() more than once makes me think there is only a
> single PCIe controller, and for some reason you can choose to operate
> it using either register set 1 or register set 2.

Conor, let me know if we need to clarify the commit log per Bjorn's
questions.  If so, then I will do it directly on the branch.

	Krzysztof

Re: [PATCH v5 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]

On Fri, Nov 01, 2024 at 02:51:29PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 14, 2024 at 09:08:42AM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > The PCI host controller on PolarFire SoC has multiple "instances", each
> > with their own bridge and ctrl address spaces. The original binding has
> > an "apb" register region, and it is expected to be set to the base
> > address of the host controllers register space. Defines in the driver
> > were used to compute the addresses of the bridge and ctrl address ranges
> > corresponding to instance1. Some customers want to use instance0 however
> > and that requires changing the defines in the driver, which is clearly
> > not a portable solution.
> 
> The subject mentions "instance 1 or 2".
> 
> This paragraph implies adding support for "instance0" ("customers want
> to use instance0").
> 
> The DT patch suggests that we're adding support for "instance2"
> ("customers want to use instance2").
> 
> Both patches suggest that the existing support is for "instance 1".
> 
> Maybe what's being added is "instance 2", and this commit log should
> s/instance0/instance 2/ ?  And probably s/instance1/instance 1/ so the
> style is consistent?

Hmm no, it would be s/instance1/instance 2/ & s/instance0/instance 1/.
The indices are 1-based, not 0-based.

> Is this a "pick one or the other but not both" situation, or does this
> device support two independent PCIe controllers?
> 
> I first thought this driver supported a single PCIe controller, and
> you were adding support for a second independent controller.
> 

I don't know if they are fully independent (Daire would have to confirm)
but as far as the driver in linux is concerned they are. As far as I
know, you could operate both instances at the same time, but I've not
heard of any customer that is actually doing that nor tested it myself.
Operating both instances would require another node in the devicetree,
which should work fine given the private data structs are allocated at
runtime. I think the config space is shared.

> But the fact that you say "the [singular] host controller on
> PolarFire", and you're not changing mc_host_probe() to call
> pci_host_common_probe() more than once makes me think there is only a
> single PCIe controller, and for some reason you can choose to operate
> it using either register set 1 or register set 2.

The wording I've used mostly stems from conversations with Daire. We've
kinda been saying that there's a single controller with two root port
instances. Each root port instance is connected to different IOs,
they're more than just different registers for accessing the same thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Bjorn Helgaas]

On Mon, Nov 04, 2024 at 11:18:43AM +0000, Conor Dooley wrote:
> On Fri, Nov 01, 2024 at 02:51:29PM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 14, 2024 at 09:08:42AM +0100, Conor Dooley wrote:
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > > 
> > > The PCI host controller on PolarFire SoC has multiple "instances", each
> > > with their own bridge and ctrl address spaces. The original binding has
> > > an "apb" register region, and it is expected to be set to the base
> > > address of the host controllers register space. Defines in the driver
> > > were used to compute the addresses of the bridge and ctrl address ranges
> > > corresponding to instance1. Some customers want to use instance0 however
> > > and that requires changing the defines in the driver, which is clearly
> > > not a portable solution.
> > 
> > The subject mentions "instance 1 or 2".
> > 
> > This paragraph implies adding support for "instance0" ("customers want
> > to use instance0").
> > 
> > The DT patch suggests that we're adding support for "instance2"
> > ("customers want to use instance2").
> > 
> > Both patches suggest that the existing support is for "instance 1".
> > 
> > Maybe what's being added is "instance 2", and this commit log should
> > s/instance0/instance 2/ ?  And probably s/instance1/instance 1/ so the
> > style is consistent?
> 
> Hmm no, it would be s/instance1/instance 2/ & s/instance0/instance 1/.
> The indices are 1-based, not 0-based.
> 
> > Is this a "pick one or the other but not both" situation, or does this
> > device support two independent PCIe controllers?
> > 
> > I first thought this driver supported a single PCIe controller, and
> > you were adding support for a second independent controller.
> 
> I don't know if they are fully independent (Daire would have to confirm)
> but as far as the driver in linux is concerned they are. As far as I
> know, you could operate both instances at the same time, but I've not
> heard of any customer that is actually doing that nor tested it myself.
> Operating both instances would require another node in the devicetree,
> which should work fine given the private data structs are allocated at
> runtime. I think the config space is shared.
> 
> > But the fact that you say "the [singular] host controller on
> > PolarFire", and you're not changing mc_host_probe() to call
> > pci_host_common_probe() more than once makes me think there is only a
> > single PCIe controller, and for some reason you can choose to operate
> > it using either register set 1 or register set 2.
> 
> The wording I've used mostly stems from conversations with Daire. We've
> kinda been saying that there's a single controller with two root port
> instances. 

If these are two separate Root Ports, can we call them "Root Ports"
instead of "instances"?  Common terminology makes for common
understanding.

> Each root port instance is connected to different IOs,
> they're more than just different registers for accessing the same thing.

Sounds like some customers use Root Port 1 and others use Root Port 2,
maybe based on things like which pins are more convenient to route.

I would very much like to reword these commit logs using as much
standard PCIe terminology as possible.  Most of these native PCIe
controller drivers have Root Complex and Root Port concepts all mixed
together, and anything we can do to standardize them will be a
benefit.

Bjorn

Re: [PATCH v5 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]

On Tue, Nov 05, 2024 at 11:18:28AM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 04, 2024 at 11:18:43AM +0000, Conor Dooley wrote:
> > On Fri, Nov 01, 2024 at 02:51:29PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 14, 2024 at 09:08:42AM +0100, Conor Dooley wrote:
> > > > From: Conor Dooley <conor.dooley@microchip.com>
> > > > 
> > > > The PCI host controller on PolarFire SoC has multiple "instances", each
> > > > with their own bridge and ctrl address spaces. The original binding has
> > > > an "apb" register region, and it is expected to be set to the base
> > > > address of the host controllers register space. Defines in the driver
> > > > were used to compute the addresses of the bridge and ctrl address ranges
> > > > corresponding to instance1. Some customers want to use instance0 however
> > > > and that requires changing the defines in the driver, which is clearly
> > > > not a portable solution.
> > > 
> > > The subject mentions "instance 1 or 2".
> > > 
> > > This paragraph implies adding support for "instance0" ("customers want
> > > to use instance0").
> > > 
> > > The DT patch suggests that we're adding support for "instance2"
> > > ("customers want to use instance2").
> > > 
> > > Both patches suggest that the existing support is for "instance 1".
> > > 
> > > Maybe what's being added is "instance 2", and this commit log should
> > > s/instance0/instance 2/ ?  And probably s/instance1/instance 1/ so the
> > > style is consistent?
> > 
> > Hmm no, it would be s/instance1/instance 2/ & s/instance0/instance 1/.
> > The indices are 1-based, not 0-based.
> > 
> > > Is this a "pick one or the other but not both" situation, or does this
> > > device support two independent PCIe controllers?
> > > 
> > > I first thought this driver supported a single PCIe controller, and
> > > you were adding support for a second independent controller.
> > 
> > I don't know if they are fully independent (Daire would have to confirm)
> > but as far as the driver in linux is concerned they are. As far as I
> > know, you could operate both instances at the same time, but I've not
> > heard of any customer that is actually doing that nor tested it myself.
> > Operating both instances would require another node in the devicetree,
> > which should work fine given the private data structs are allocated at
> > runtime. I think the config space is shared.
> > 
> > > But the fact that you say "the [singular] host controller on
> > > PolarFire", and you're not changing mc_host_probe() to call
> > > pci_host_common_probe() more than once makes me think there is only a
> > > single PCIe controller, and for some reason you can choose to operate
> > > it using either register set 1 or register set 2.
> > 
> > The wording I've used mostly stems from conversations with Daire. We've
> > kinda been saying that there's a single controller with two root port
> > instances. 
> 
> If these are two separate Root Ports, can we call them "Root Ports"
> instead of "instances"?  Common terminology makes for common
> understanding.

Sure.

> > Each root port instance is connected to different IOs,
> > they're more than just different registers for accessing the same thing.
> 
> Sounds like some customers use Root Port 1 and others use Root Port 2,
> maybe based on things like which pins are more convenient to route.

Aye, the user that motivated the patchset uses a very small package and
was not able to use root port 1 for that reason.

> I would very much like to reword these commit logs using as much
> standard PCIe terminology as possible.  Most of these native PCIe
> controller drivers have Root Complex and Root Port concepts all mixed
> together, and anything we can do to standardize them will be a
> benefit.

I can do that tomorrow.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 0/2] PCI: microchip: support using either instance 1 or 2

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On Wed, Aug 14, 2024 at 09:08:40AM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The current driver and binding for PolarFire SoC's PCI controller assume
> that the root port instance in use is instance 1. The second reg
> property constitutes the region encompassing both "control" and "bridge"
> registers for both instances. In the driver, a fixed offset is applied to
> find the base addresses for instance 1's "control" and "bridge"
> registers. The BeagleV Fire uses root port instance 2, so something must
> be done so that software can differentiate. This series splits the
> second reg property in two, with dedicated "control" and "bridge"
> entries so that either instance can be used.

Just attempting to bump this patchset. It has gone over 2 months without
response, and I am afraid it has completely fallen between the cracks.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 0/2] PCI: microchip: support using either instance 1 or 2

[Bjorn Helgaas]

On Thu, Oct 24, 2024 at 10:38:11AM +0100, Conor Dooley wrote:
> On Wed, Aug 14, 2024 at 09:08:40AM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > The current driver and binding for PolarFire SoC's PCI controller assume
> > that the root port instance in use is instance 1. The second reg
> > property constitutes the region encompassing both "control" and "bridge"
> > registers for both instances. In the driver, a fixed offset is applied to
> > find the base addresses for instance 1's "control" and "bridge"
> > registers. The BeagleV Fire uses root port instance 2, so something must
> > be done so that software can differentiate. This series splits the
> > second reg property in two, with dedicated "control" and "bridge"
> > entries so that either instance can be used.
> 
> Just attempting to bump this patchset. It has gone over 2 months without
> response, and I am afraid it has completely fallen between the cracks.

Thanks for bumping this.  It looks pretty straightforward to me, so if
nobody acts on it soon, I'll pick it up.

Bjorn

Re: [PATCH v5 0/2] PCI: microchip: support using either instance 1 or 2

[Krzysztof Wilczyński]

Hello,

> The current driver and binding for PolarFire SoC's PCI controller assume
> that the root port instance in use is instance 1. The second reg
> property constitutes the region encompassing both "control" and "bridge"
> registers for both instances. In the driver, a fixed offset is applied to
> find the base addresses for instance 1's "control" and "bridge"
> registers. The BeagleV Fire uses root port instance 2, so something must
> be done so that software can differentiate. This series splits the
> second reg property in two, with dedicated "control" and "bridge"
> entries so that either instance can be used.

Applied to controller/microchip, thank you!

[01/02] PCI: dt-bindings: PCI: microchip,pcie-host: Fix reg properties
        https://git.kernel.org/pci/pci/c/ae5ebdb405dd
[02/02] PCI: microchip: Rework reg region handing to support using either instance 1 or 2
        https://git.kernel.org/pci/pci/c/e071e6bd9940

	Krzysztof