[PATCH v4 00/10] Add PCIe support for bcm2712

[PATCH v4 00/10] Add PCIe support for bcm2712

[Stanimir Varbanov]

Here is v4 of the series which aims to add support for PCIe on bcm2712 SoC
used by RPi5. Previous v3 can be found at [1].

v3 -> v4 changes include:
 - Addressed comments on the interrupt-controller driver (Thomas)
 - Moved "Reuse config structure" patch earlier in the series (Bjorn)
 - Merged "Avoid turn off of bridge reset" into "Add bcm2712 support" (Bjorn)
 - Fixed DTB warnings on broadcom/bcm2712-rpi-5-b.dtb 

For more detailed info check patches.

Comments are welcome!
~Stan

[1] https://patchwork.kernel.org/project/linux-pci/list/?series=898891

Stanimir Varbanov (10):
  dt-bindings: interrupt-controller: Add bcm2712 MSI-X DT bindings
  dt-bindings: PCI: brcmstb: Update bindings for PCIe on bcm2712
  irqchip: Add Broadcom bcm2712 MSI-X interrupt controller
  PCI: brcmstb: Reuse config structure
  PCI: brcmstb: Expand inbound window size up to 64GB
  PCI: brcmstb: Enable external MSI-X if available
  PCI: brcmstb: Add bcm2712 support
  PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk
  arm64: dts: broadcom: bcm2712: Add PCIe DT nodes
  arm64: dts: broadcom: bcm2712-rpi-5-b: Enable PCIe DT nodes

 .../brcm,bcm2712-msix.yaml                    |  60 ++++
 .../bindings/pci/brcm,stb-pcie.yaml           |  21 ++
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |   8 +
 arch/arm64/boot/dts/broadcom/bcm2712.dtsi     | 162 +++++++++
 drivers/irqchip/Kconfig                       |  16 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-bcm2712-mip.c             | 310 ++++++++++++++++++
 drivers/pci/controller/pcie-brcmstb.c         | 204 +++++++++---
 8 files changed, 735 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml
 create mode 100644 drivers/irqchip/irq-bcm2712-mip.c

-- 
2.43.0

[PATCH v4 01/10] dt-bindings: interrupt-controller: Add bcm2712 MSI-X DT bindings

[Stanimir Varbanov]

Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
v3 -> v4:
 - Added Reviewed-by.

 .../brcm,bcm2712-msix.yaml                    | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml
new file mode 100644
index 000000000000..c84614663b5d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/brcm,bcm2712-msix.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom bcm2712 MSI-X Interrupt Peripheral support
+
+maintainers:
+  - Stanimir Varbanov <svarbanov@suse.de>
+
+description:
+  This interrupt controller is used to provide interrupt vectors to the
+  generic interrupt controller (GIC) on bcm2712. It will be used as
+  external MSI-X controller for PCIe root complex.
+
+allOf:
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    const: brcm,bcm2712-mip
+
+  reg:
+    items:
+      - description: Base register address
+      - description: PCIe message address
+
+  "#msi-cells":
+    const: 0
+
+  brcm,msi-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Shift the allocated MSI's.
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - msi-controller
+  - msi-ranges
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    axi {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        msi-controller@1000130000 {
+            compatible = "brcm,bcm2712-mip";
+            reg = <0x10 0x00130000 0x00 0xc0>,
+                  <0xff 0xfffff000 0x00 0x1000>;
+            msi-controller;
+            #msi-cells = <0>;
+            msi-ranges = <&gicv2 GIC_SPI 128 IRQ_TYPE_EDGE_RISING 64>;
+        };
+    };
-- 
2.43.0

[PATCH v4 02/10] dt-bindings: PCI: brcmstb: Update bindings for PCIe on bcm2712

[Stanimir Varbanov]

Update brcmstb PCIe controller bindings with bcm2712 compatible
and add new resets.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
---
v3 -> v4:
 - Dropped Reviewed-by and Acked-by tags because I have to re-work the patch
   in order to fix newly produced  DTB warnings on the .dts files.
 - Account the number of resets for bcm2712 which are differs from bcm7712.

 .../bindings/pci/brcm,stb-pcie.yaml           | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 0925c520195a..df9eeaef93cd 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -14,6 +14,7 @@ properties:
     items:
       - enum:
           - brcm,bcm2711-pcie # The Raspberry Pi 4
+          - brcm,bcm2712-pcie # Raspberry Pi 5
           - brcm,bcm4908-pcie
           - brcm,bcm7211-pcie # Broadcom STB version of RPi4
           - brcm,bcm7216-pcie # Broadcom 7216 Arm
@@ -175,6 +176,26 @@ allOf:
         - resets
         - reset-names
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm2712-pcie
+    then:
+      properties:
+        resets:
+          minItems: 2
+          maxItems: 2
+
+        reset-names:
+          items:
+            - const: bridge
+            - const: rescal
+
+      required:
+        - resets
+        - reset-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.43.0

Re: [PATCH v4 02/10] dt-bindings: PCI: brcmstb: Update bindings for PCIe on bcm2712

[Rob Herring]

On Fri, Oct 25, 2024 at 03:45:07PM +0300, Stanimir Varbanov wrote:
> Update brcmstb PCIe controller bindings with bcm2712 compatible
> and add new resets.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> ---
> v3 -> v4:
>  - Dropped Reviewed-by and Acked-by tags because I have to re-work the patch
>    in order to fix newly produced  DTB warnings on the .dts files.
>  - Account the number of resets for bcm2712 which are differs from bcm7712.
> 
>  .../bindings/pci/brcm,stb-pcie.yaml           | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 0925c520195a..df9eeaef93cd 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -14,6 +14,7 @@ properties:
>      items:
>        - enum:
>            - brcm,bcm2711-pcie # The Raspberry Pi 4
> +          - brcm,bcm2712-pcie # Raspberry Pi 5
>            - brcm,bcm4908-pcie
>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
> @@ -175,6 +176,26 @@ allOf:
>          - resets
>          - reset-names
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm2712-pcie
> +    then:
> +      properties:
> +        resets:
> +          minItems: 2
> +          maxItems: 2
> +
> +        reset-names:
> +          items:
> +            - const: bridge
> +            - const: rescal

Sigh. Why the opposite order of the existing bindings?

I would make the top level:

minItems: 1
items:
  - enum: [perst, rescal]
  - const: bridge
  - const: swinit

Rob

[PATCH v4 03/10] irqchip: Add Broadcom bcm2712 MSI-X interrupt controller

[Stanimir Varbanov]

Add an interrupt controller driver for MSI-X Interrupt Peripheral (MIP)
hardware block found in bcm2712. The interrupt controller is used to
handle MSI-X interrupts from peripherials behind PCIe endpoints like
RP1 south bridge found in RPi5.

There are two MIPs on bcm2712, the first has 64 consecutive SPIs
assigned to 64 output vectors, and the second has 17 SPIs, but only
8 of them are consecutive starting at the 8th output vector.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
---
v3 -> v4:
 - Addressed the comments for wrongly used PCI/MSI flags (Thomas)

 drivers/irqchip/Kconfig           |  16 ++
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-bcm2712-mip.c | 310 ++++++++++++++++++++++++++++++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/irqchip/irq-bcm2712-mip.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 341cd9ca5a05..c9bd0a4f6871 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -116,6 +116,22 @@ config I8259
 	bool
 	select IRQ_DOMAIN
 
+config BCM2712_MIP
+	tristate "Broadcom BCM2712 MSI-X Interrupt Peripheral support"
+	depends on ARCH_BRCMSTB || COMPILE_TEST
+	default m if ARCH_BRCMSTB
+	depends on ARM_GIC
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN_HIERARCHY
+	select GENERIC_MSI_IRQ
+	select IRQ_MSI_LIB
+	help
+	  Enable support for the Broadcom BCM2712 MSI-X target peripheral
+	  (MIP) needed by brcmstb PCIe to handle MSI-X interrupts on
+	  Raspberry Pi 5.
+
+	  If unsure say n.
+
 config BCM6345_L1_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e3679ec2b9f7..a11307b1b610 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
 obj-$(CONFIG_XILINX_INTC)		+= irq-xilinx-intc.o
 obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
 obj-$(CONFIG_SOC_VF610)			+= irq-vf610-mscm-ir.o
+obj-$(CONFIG_BCM2712_MIP)               += irq-bcm2712-mip.o
 obj-$(CONFIG_BCM6345_L1_IRQ)		+= irq-bcm6345-l1.o
 obj-$(CONFIG_BCM7038_L1_IRQ)		+= irq-bcm7038-l1.o
 obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
diff --git a/drivers/irqchip/irq-bcm2712-mip.c b/drivers/irqchip/irq-bcm2712-mip.c
new file mode 100644
index 000000000000..fd73f2d41279
--- /dev/null
+++ b/drivers/irqchip/irq-bcm2712-mip.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Raspberry Pi Ltd., All Rights Reserved.
+ * Copyright (c) 2024 SUSE
+ */
+
+#include <linux/bitmap.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#include "irq-msi-lib.h"
+
+#define MIP_INT_RAISE		0x00
+#define MIP_INT_CLEAR		0x10
+#define MIP_INT_CFGL_HOST	0x20
+#define MIP_INT_CFGH_HOST	0x30
+#define MIP_INT_MASKL_HOST	0x40
+#define MIP_INT_MASKH_HOST	0x50
+#define MIP_INT_MASKL_VPU	0x60
+#define MIP_INT_MASKH_VPU	0x70
+#define MIP_INT_STATUSL_HOST	0x80
+#define MIP_INT_STATUSH_HOST	0x90
+#define MIP_INT_STATUSL_VPU	0xa0
+#define MIP_INT_STATUSH_VPU	0xb0
+
+/**
+ * struct mip_priv - MSI-X interrupt controller data
+ * @lock:	Used to protect bitmap alloc/free
+ * @base:	Base address of MMIO area
+ * @msg_addr:	PCIe MSI-X address
+ * @msi_base:	MSI base
+ * @num_msis:	Count of MSIs
+ * @msi_offset:	MSI offset
+ * @bitmap:	A bitmap for hwirqs
+ * @parent:	Parent domain (GIC)
+ * @dev:	A device pointer
+ */
+struct mip_priv {
+	spinlock_t		lock;
+	void __iomem		*base;
+	u64			msg_addr;
+	u32			msi_base;
+	u32			num_msis;
+	u32			msi_offset;
+	unsigned long		*bitmap;
+	struct irq_domain	*parent;
+	struct device		*dev;
+};
+
+static void mip_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct mip_priv *mip = irq_data_get_irq_chip_data(d);
+
+	msg->address_hi = upper_32_bits(mip->msg_addr);
+	msg->address_lo = lower_32_bits(mip->msg_addr);
+	msg->data = d->hwirq;
+}
+
+static struct irq_chip mip_middle_irq_chip = {
+	.name			= "MIP",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_compose_msi_msg	= mip_compose_msi_msg,
+};
+
+static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs,
+			   unsigned int *hwirq)
+{
+	int bit;
+
+	spin_lock(&mip->lock);
+	bit = bitmap_find_free_region(mip->bitmap, mip->num_msis,
+				      ilog2(nr_irqs));
+	spin_unlock(&mip->lock);
+
+	if (bit < 0)
+		return bit;
+
+	if (hwirq)
+		*hwirq = bit;
+
+	return 0;
+}
+
+static void mip_free_hwirq(struct mip_priv *mip, unsigned int hwirq,
+			   unsigned int nr_irqs)
+{
+	spin_lock(&mip->lock);
+	bitmap_release_region(mip->bitmap, hwirq, ilog2(nr_irqs));
+	spin_unlock(&mip->lock);
+}
+
+static int mip_middle_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *arg)
+{
+	struct mip_priv *mip = domain->host_data;
+	struct irq_fwspec fwspec = {0};
+	unsigned int hwirq, irq, i;
+	struct irq_data *irqd;
+	int ret;
+
+	ret = mip_alloc_hwirq(mip, nr_irqs, &irq);
+	if (ret < 0)
+		return ret;
+
+	hwirq = irq + mip->msi_offset;
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 3;
+	fwspec.param[0] = 0;
+	fwspec.param[1] = hwirq + mip->msi_base;
+	fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &fwspec);
+	if (ret) {
+		mip_free_hwirq(mip, irq, nr_irqs);
+		return ret;
+	}
+
+	for (i = 0; i < nr_irqs; i++) {
+		irqd = irq_domain_get_irq_data(domain->parent, virq + i);
+		irqd->chip->irq_set_type(irqd, IRQ_TYPE_EDGE_RISING);
+
+		ret = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+						    &mip_middle_irq_chip, mip);
+		if (ret)
+			goto err_free;
+
+		irqd = irq_get_irq_data(virq + i);
+		irqd_set_single_target(irqd);
+		irqd_set_affinity_on_activate(irqd);
+	}
+
+	return 0;
+
+err_free:
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+	mip_free_hwirq(mip, irq, nr_irqs);
+	return ret;
+}
+
+static void mip_middle_domain_free(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs)
+{
+	struct irq_data *irqd = irq_domain_get_irq_data(domain, virq);
+	struct mip_priv *mip;
+	unsigned int hwirq;
+
+	if (!irqd)
+		return;
+
+	mip = irq_data_get_irq_chip_data(irqd);
+	hwirq = irqd_to_hwirq(irqd);
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+	mip_free_hwirq(mip, hwirq - mip->msi_offset, nr_irqs);
+}
+
+static const struct irq_domain_ops mip_middle_domain_ops = {
+	.select		= msi_lib_irq_domain_select,
+	.alloc		= mip_middle_domain_alloc,
+	.free		= mip_middle_domain_free,
+};
+
+#define MIP_MSI_FLAGS_REQUIRED	(MSI_FLAG_USE_DEF_DOM_OPS |	\
+				 MSI_FLAG_USE_DEF_CHIP_OPS |	\
+				 MSI_FLAG_PCI_MSI_MASK_PARENT)
+
+#define MIP_MSI_FLAGS_SUPPORTED	(MSI_GENERIC_FLAGS_MASK |	\
+				 MSI_FLAG_MULTI_PCI_MSI |	\
+				 MSI_FLAG_PCI_MSIX)
+
+static const struct msi_parent_ops mip_msi_parent_ops = {
+	.supported_flags	= MIP_MSI_FLAGS_SUPPORTED,
+	.required_flags		= MIP_MSI_FLAGS_REQUIRED,
+	.bus_select_token       = DOMAIN_BUS_GENERIC_MSI,
+	.bus_select_mask	= MATCH_PCI_MSI,
+	.prefix			= "MIP-MSI-",
+	.init_dev_msi_info	= msi_lib_init_dev_msi_info,
+};
+
+static int mip_init_domains(struct mip_priv *mip, struct device_node *np)
+{
+	struct irq_domain *middle;
+
+	middle = irq_domain_add_hierarchy(mip->parent, 0, mip->num_msis, np,
+					  &mip_middle_domain_ops, mip);
+	if (!middle)
+		return -ENOMEM;
+
+	irq_domain_update_bus_token(middle, DOMAIN_BUS_GENERIC_MSI);
+	middle->dev = mip->dev;
+	middle->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
+	middle->msi_parent_ops = &mip_msi_parent_ops;
+
+	return 0;
+}
+
+static int mip_parse_dt(struct mip_priv *mip, struct device_node *np)
+{
+	struct of_phandle_args args;
+	u64 size;
+	int ret;
+
+	ret = of_property_read_u32(np, "brcm,msi-offset", &mip->msi_offset);
+	if (ret)
+		mip->msi_offset = 0;
+
+	ret = of_parse_phandle_with_args(np, "msi-ranges", "#interrupt-cells",
+					 0, &args);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32_index(np, "msi-ranges", args.args_count + 1,
+					 &mip->num_msis);
+	if (ret)
+		goto err_put;
+
+	ret = of_property_read_reg(np, 1, &mip->msg_addr, &size);
+	if (ret)
+		goto err_put;
+
+	mip->msi_base = args.args[1];
+
+	mip->parent = irq_find_host(args.np);
+	if (!mip->parent)
+		ret = -EINVAL;
+
+err_put:
+	of_node_put(args.np);
+	return ret;
+}
+
+static int __init mip_of_msi_init(struct device_node *node,
+				  struct device_node *parent)
+{
+	struct platform_device *pdev;
+	struct mip_priv *mip;
+	int ret;
+
+	pdev = of_find_device_by_node(node);
+	of_node_put(node);
+	if (!pdev)
+		return -EPROBE_DEFER;
+
+	mip = kzalloc(sizeof(*mip), GFP_KERNEL);
+	if (!mip)
+		return -ENOMEM;
+
+	spin_lock_init(&mip->lock);
+	mip->dev = &pdev->dev;
+
+	ret = mip_parse_dt(mip, node);
+	if (ret)
+		goto err_priv;
+
+	mip->base = of_iomap(node, 0);
+	if (!mip->base) {
+		ret = -ENXIO;
+		goto err_priv;
+	}
+
+	mip->bitmap = bitmap_zalloc(mip->num_msis, GFP_KERNEL);
+	if (!mip->bitmap) {
+		ret = -ENOMEM;
+		goto err_base;
+	}
+
+	/*
+	 * All MSI-X masked in for the host, masked out for the
+	 * VPU, and edge-triggered.
+	 */
+	writel(0, mip->base + MIP_INT_MASKL_HOST);
+	writel(0, mip->base + MIP_INT_MASKH_HOST);
+	writel(~0, mip->base + MIP_INT_MASKL_VPU);
+	writel(~0, mip->base + MIP_INT_MASKH_VPU);
+	writel(~0, mip->base + MIP_INT_CFGL_HOST);
+	writel(~0, mip->base + MIP_INT_CFGH_HOST);
+
+	ret = mip_init_domains(mip, node);
+	if (ret)
+		goto err_map;
+
+	dev_dbg(&pdev->dev,
+		"MIP: MSI-X count: %u, base: %u, offset: %u, msg_addr: %llx\n",
+		mip->num_msis, mip->msi_base, mip->msi_offset, mip->msg_addr);
+
+	return 0;
+
+err_map:
+	bitmap_free(mip->bitmap);
+err_base:
+	iounmap(mip->base);
+err_priv:
+	kfree(mip);
+	return ret;
+}
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(mip_msi)
+IRQCHIP_MATCH("brcm,bcm2712-mip", mip_of_msi_init)
+IRQCHIP_PLATFORM_DRIVER_END(mip_msi)
+MODULE_DESCRIPTION("Broadcom BCM2712 MSI interrupt controller");
+MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>");
+MODULE_AUTHOR("Stanimir Varbanov <svarbanov@suse.de>");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v4 03/10] irqchip: Add Broadcom bcm2712 MSI-X interrupt controller

[Thomas Gleixner]

On Fri, Oct 25 2024 at 15:45, Stanimir Varbanov wrote:

> Add an interrupt controller driver for MSI-X Interrupt Peripheral (MIP)
> hardware block found in bcm2712. The interrupt controller is used to
> handle MSI-X interrupts from peripherials behind PCIe endpoints like
> RP1 south bridge found in RPi5.
>
> There are two MIPs on bcm2712, the first has 64 consecutive SPIs
> assigned to 64 output vectors, and the second has 17 SPIs, but only
> 8 of them are consecutive starting at the 8th output vector.

This starts to converge nicely. Just a few remaining nitpicks.

> +static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs,
> +			   unsigned int *hwirq)
> +{
> +	int bit;
> +
> +	spin_lock(&mip->lock);
> +	bit = bitmap_find_free_region(mip->bitmap, mip->num_msis,
> +				      ilog2(nr_irqs));
> +	spin_unlock(&mip->lock);

This should be

        scoped_guard(spinlock, &mip->lock)
		bit = bitmap_find_free_region(mip->bitmap, mip->num_msis, ilog2(nr_irqs));

> +	if (bit < 0)
> +		return bit;
> +
> +	if (hwirq)
> +		*hwirq = bit;

But what's the point of this conditional? The only call site hands in a
valid pointer, no?

> +	return 0;

And therefore the whole thing can be simplified to:

static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs)
{
        guard(spinlock)(&mip_lock);
        return bitmap_find_free_region(mip->bitmap, mip->num_msis, ilog2(nr_irqs));
}

and the callsite becomes:

        irq = mip_alloc_hwirq(mip, nr_irqs);
        if (irq < 0)
        	return irq;
Hmm?

> +}
> +
> +static void mip_free_hwirq(struct mip_priv *mip, unsigned int hwirq,
> +			   unsigned int nr_irqs)
> +{
> +	spin_lock(&mip->lock);

	guard(spinlock)(&mip->lock);

> +	bitmap_release_region(mip->bitmap, hwirq, ilog2(nr_irqs));
> +	spin_unlock(&mip->lock);
> +}

> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &fwspec);
> +	if (ret) {
> +		mip_free_hwirq(mip, irq, nr_irqs);
> +		return ret;

                goto err_free_hwirq; ?

> +	}
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irqd = irq_domain_get_irq_data(domain->parent, virq + i);
> +		irqd->chip->irq_set_type(irqd, IRQ_TYPE_EDGE_RISING);
> +
> +		ret = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +						    &mip_middle_irq_chip, mip);
> +		if (ret)
> +			goto err_free;
> +
> +		irqd = irq_get_irq_data(virq + i);
> +		irqd_set_single_target(irqd);
> +		irqd_set_affinity_on_activate(irqd);
> +	}
> +
> +	return 0;
> +
> +err_free:
> +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +	mip_free_hwirq(mip, irq, nr_irqs);
> +	return ret;
> +}
> +
> +static int __init mip_of_msi_init(struct device_node *node,
> +				  struct device_node *parent)

No line break required here.

> +{
> +	struct platform_device *pdev;
> +	struct mip_priv *mip;
> +	int ret;
> +
> +	pdev = of_find_device_by_node(node);
> +	of_node_put(node);
> +	if (!pdev)
> +		return -EPROBE_DEFER;
> +
> +	mip = kzalloc(sizeof(*mip), GFP_KERNEL);
> +	if (!mip)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&mip->lock);
> +	mip->dev = &pdev->dev;
> +
> +	ret = mip_parse_dt(mip, node);
> +	if (ret)
> +		goto err_priv;
> +
> +	mip->base = of_iomap(node, 0);
> +	if (!mip->base) {
> +		ret = -ENXIO;
> +		goto err_priv;
> +	}
> +
> +	mip->bitmap = bitmap_zalloc(mip->num_msis, GFP_KERNEL);
> +	if (!mip->bitmap) {
> +		ret = -ENOMEM;
> +		goto err_base;
> +	}
> +
> +	/*
> +	 * All MSI-X masked in for the host, masked out for the
> +	 * VPU, and edge-triggered.
> +	 */
> +	writel(0, mip->base + MIP_INT_MASKL_HOST);
> +	writel(0, mip->base + MIP_INT_MASKH_HOST);
> +	writel(~0, mip->base + MIP_INT_MASKL_VPU);
> +	writel(~0, mip->base + MIP_INT_MASKH_VPU);
> +	writel(~0, mip->base + MIP_INT_CFGL_HOST);
> +	writel(~0, mip->base + MIP_INT_CFGH_HOST);

What undoes that in case mpi_init_domains() fails? Or is it harmless? I
really have no idea what masked in and masked out means here.

> +	dev_dbg(&pdev->dev,
> +		"MIP: MSI-X count: %u, base: %u, offset: %u, msg_addr: %llx\n",

Please move the string up. You have 100 characters width available.

> +		mip->num_msis, mip->msi_base, mip->msi_offset, mip->msg_addr);

Thanks,

        tglx

Re: [PATCH v4 03/10] irqchip: Add Broadcom bcm2712 MSI-X interrupt controller

[Stanimir Varbanov]

Hi Thomas,

Thank you for the review!

On 10/28/24 22:06, Thomas Gleixner wrote:
> On Fri, Oct 25 2024 at 15:45, Stanimir Varbanov wrote:
> 
>> Add an interrupt controller driver for MSI-X Interrupt Peripheral (MIP)
>> hardware block found in bcm2712. The interrupt controller is used to
>> handle MSI-X interrupts from peripherials behind PCIe endpoints like
>> RP1 south bridge found in RPi5.
>>
>> There are two MIPs on bcm2712, the first has 64 consecutive SPIs
>> assigned to 64 output vectors, and the second has 17 SPIs, but only
>> 8 of them are consecutive starting at the 8th output vector.
> 
> This starts to converge nicely. Just a few remaining nitpicks.
> 
>> +static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs,
>> +			   unsigned int *hwirq)
>> +{
>> +	int bit;
>> +
>> +	spin_lock(&mip->lock);
>> +	bit = bitmap_find_free_region(mip->bitmap, mip->num_msis,
>> +				      ilog2(nr_irqs));
>> +	spin_unlock(&mip->lock);
> 
> This should be
> 
>         scoped_guard(spinlock, &mip->lock)
> 		bit = bitmap_find_free_region(mip->bitmap, mip->num_msis, ilog2(nr_irqs));
> 
>> +	if (bit < 0)
>> +		return bit;
>> +
>> +	if (hwirq)
>> +		*hwirq = bit;
> 
> But what's the point of this conditional? The only call site hands in a
> valid pointer, no?
> 
>> +	return 0;
> 
> And therefore the whole thing can be simplified to:
> 
> static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs)
> {
>         guard(spinlock)(&mip_lock);
>         return bitmap_find_free_region(mip->bitmap, mip->num_msis, ilog2(nr_irqs));
> }
> 
> and the callsite becomes:
> 
>         irq = mip_alloc_hwirq(mip, nr_irqs);
>         if (irq < 0)
>         	return irq;
> Hmm?
> 
>> +}
>> +
>> +static void mip_free_hwirq(struct mip_priv *mip, unsigned int hwirq,
>> +			   unsigned int nr_irqs)
>> +{
>> +	spin_lock(&mip->lock);
> 
> 	guard(spinlock)(&mip->lock);

Will address the above comments in next version.

> 
>> +	bitmap_release_region(mip->bitmap, hwirq, ilog2(nr_irqs));
>> +	spin_unlock(&mip->lock);
>> +}
> 
>> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &fwspec);
>> +	if (ret) {
>> +		mip_free_hwirq(mip, irq, nr_irqs);
>> +		return ret;
> 
>                 goto err_free_hwirq; ?
> 
>> +	}
>> +
>> +	for (i = 0; i < nr_irqs; i++) {
>> +		irqd = irq_domain_get_irq_data(domain->parent, virq + i);
>> +		irqd->chip->irq_set_type(irqd, IRQ_TYPE_EDGE_RISING);
>> +
>> +		ret = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +						    &mip_middle_irq_chip, mip);
>> +		if (ret)
>> +			goto err_free;
>> +
>> +		irqd = irq_get_irq_data(virq + i);
>> +		irqd_set_single_target(irqd);
>> +		irqd_set_affinity_on_activate(irqd);
>> +	}
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
>> +	mip_free_hwirq(mip, irq, nr_irqs);
>> +	return ret;
>> +}
>> +
>> +static int __init mip_of_msi_init(struct device_node *node,
>> +				  struct device_node *parent)
> 
> No line break required here.

OK.

> 
>> +{
>> +	struct platform_device *pdev;
>> +	struct mip_priv *mip;
>> +	int ret;
>> +
>> +	pdev = of_find_device_by_node(node);
>> +	of_node_put(node);
>> +	if (!pdev)
>> +		return -EPROBE_DEFER;
>> +
>> +	mip = kzalloc(sizeof(*mip), GFP_KERNEL);
>> +	if (!mip)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&mip->lock);
>> +	mip->dev = &pdev->dev;
>> +
>> +	ret = mip_parse_dt(mip, node);
>> +	if (ret)
>> +		goto err_priv;
>> +
>> +	mip->base = of_iomap(node, 0);
>> +	if (!mip->base) {
>> +		ret = -ENXIO;
>> +		goto err_priv;
>> +	}
>> +
>> +	mip->bitmap = bitmap_zalloc(mip->num_msis, GFP_KERNEL);
>> +	if (!mip->bitmap) {
>> +		ret = -ENOMEM;
>> +		goto err_base;
>> +	}
>> +
>> +	/*
>> +	 * All MSI-X masked in for the host, masked out for the
>> +	 * VPU, and edge-triggered.
>> +	 */
>> +	writel(0, mip->base + MIP_INT_MASKL_HOST);
>> +	writel(0, mip->base + MIP_INT_MASKH_HOST);
>> +	writel(~0, mip->base + MIP_INT_MASKL_VPU);
>> +	writel(~0, mip->base + MIP_INT_MASKH_VPU);
>> +	writel(~0, mip->base + MIP_INT_CFGL_HOST);
>> +	writel(~0, mip->base + MIP_INT_CFGH_HOST);
> 
> What undoes that in case mpi_init_domains() fails? Or is it harmless? I
> really have no idea what masked in and masked out means here.

It should be harmless, but I could move registers initialization in
mip_init_domains() and fix the comments.

> 
>> +	dev_dbg(&pdev->dev,
>> +		"MIP: MSI-X count: %u, base: %u, offset: %u, msg_addr: %llx\n",
> 
> Please move the string up. You have 100 characters width available.

OK.

> 
>> +		mip->num_msis, mip->msi_base, mip->msi_offset, mip->msg_addr);
> 
> Thanks,
> 
>         tglx

regards,
~Stan

[PATCH v4 04/10] PCI: brcmstb: Reuse config structure

[Stanimir Varbanov]

Instead of copying fields from pcie_cfg_data structure to
brcm_pcie reference it directly.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelil <florian.fainelli@broadcom.com>
---
v3 -> v4:
 - no changes.

 drivers/pci/controller/pcie-brcmstb.c | 70 ++++++++++++---------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 9321280f6edb..12bcc5919924 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -191,11 +191,11 @@
 #define SSC_STATUS_PLL_LOCK_MASK	0x800
 #define PCIE_BRCM_MAX_MEMC		3
 
-#define IDX_ADDR(pcie)			((pcie)->reg_offsets[EXT_CFG_INDEX])
-#define DATA_ADDR(pcie)			((pcie)->reg_offsets[EXT_CFG_DATA])
-#define PCIE_RGR1_SW_INIT_1(pcie)	((pcie)->reg_offsets[RGR1_SW_INIT_1])
-#define HARD_DEBUG(pcie)		((pcie)->reg_offsets[PCIE_HARD_DEBUG])
-#define INTR2_CPU_BASE(pcie)		((pcie)->reg_offsets[PCIE_INTR2_CPU_BASE])
+#define IDX_ADDR(pcie)			((pcie)->cfg->offsets[EXT_CFG_INDEX])
+#define DATA_ADDR(pcie)			((pcie)->cfg->offsets[EXT_CFG_DATA])
+#define PCIE_RGR1_SW_INIT_1(pcie)	((pcie)->cfg->offsets[RGR1_SW_INIT_1])
+#define HARD_DEBUG(pcie)		((pcie)->cfg->offsets[PCIE_HARD_DEBUG])
+#define INTR2_CPU_BASE(pcie)		((pcie)->cfg->offsets[PCIE_INTR2_CPU_BASE])
 
 /* Rescal registers */
 #define PCIE_DVT_PMU_PCIE_PHY_CTRL				0xc700
@@ -276,8 +276,6 @@ struct brcm_pcie {
 	int			gen;
 	u64			msi_target_addr;
 	struct brcm_msi		*msi;
-	const int		*reg_offsets;
-	enum pcie_soc_base	soc_base;
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge_reset;
@@ -285,17 +283,14 @@ struct brcm_pcie {
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
-	int			(*perst_set)(struct brcm_pcie *pcie, u32 val);
-	int			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
-	bool			has_phy;
-	u8			num_inbound_wins;
+	const struct pcie_cfg_data	*cfg;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
 {
-	return pcie->soc_base == BCM7435 || pcie->soc_base == BCM7425;
+	return pcie->cfg->soc_base == BCM7435 || pcie->cfg->soc_base == BCM7425;
 }
 
 /*
@@ -855,7 +850,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
 	 * security considerations, and is not implemented in our modern
 	 * SoCs.
 	 */
-	if (pcie->soc_base != BCM7712)
+	if (pcie->cfg->soc_base != BCM7712)
 		add_inbound_win(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
@@ -872,10 +867,10 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
 		 * That being said, each BARs size must still be a power of
 		 * two.
 		 */
-		if (pcie->soc_base == BCM7712)
+		if (pcie->cfg->soc_base == BCM7712)
 			add_inbound_win(b++, &n, size, cpu_start, pcie_start);
 
-		if (n > pcie->num_inbound_wins)
+		if (n > pcie->cfg->num_inbound_wins)
 			break;
 	}
 
@@ -889,7 +884,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
 	 * that enables multiple memory controllers.  As such, it can return
 	 * now w/o doing special configuration.
 	 */
-	if (pcie->soc_base == BCM7712)
+	if (pcie->cfg->soc_base == BCM7712)
 		return n;
 
 	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
@@ -1012,7 +1007,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie,
 		 * 7712:
 		 *     All of their BARs need to be set.
 		 */
-		if (pcie->soc_base == BCM7712) {
+		if (pcie->cfg->soc_base == BCM7712) {
 			/* BUS remap register settings */
 			reg_offset = brcm_ubus_reg_offset(i);
 			tmp = lower_32_bits(cpu_addr) & ~0xfff;
@@ -1036,15 +1031,15 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	int memc, ret;
 
 	/* Reset the bridge */
-	ret = pcie->bridge_sw_init_set(pcie, 1);
+	ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
 	if (ret)
 		return ret;
 
 	/* Ensure that PERST# is asserted; some bootloaders may deassert it. */
-	if (pcie->soc_base == BCM2711) {
-		ret = pcie->perst_set(pcie, 1);
+	if (pcie->cfg->soc_base == BCM2711) {
+		ret = pcie->cfg->perst_set(pcie, 1);
 		if (ret) {
-			pcie->bridge_sw_init_set(pcie, 0);
+			pcie->cfg->bridge_sw_init_set(pcie, 0);
 			return ret;
 		}
 	}
@@ -1052,7 +1047,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	usleep_range(100, 200);
 
 	/* Take the bridge out of reset */
-	ret = pcie->bridge_sw_init_set(pcie, 0);
+	ret = pcie->cfg->bridge_sw_init_set(pcie, 0);
 	if (ret)
 		return ret;
 
@@ -1072,9 +1067,9 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	 */
 	if (is_bmips(pcie))
 		burst = 0x1; /* 256 bytes */
-	else if (pcie->soc_base == BCM2711)
+	else if (pcie->cfg->soc_base == BCM2711)
 		burst = 0x0; /* 128 bytes */
-	else if (pcie->soc_base == BCM7278)
+	else if (pcie->cfg->soc_base == BCM7278)
 		burst = 0x3; /* 512 bytes */
 	else
 		burst = 0x2; /* 512 bytes */
@@ -1199,7 +1194,7 @@ static void brcm_extend_rbus_timeout(struct brcm_pcie *pcie)
 	u32 timeout_us = 4000000; /* 4 seconds, our setting for L1SS */
 
 	/* 7712 does not have this (RGR1) timer */
-	if (pcie->soc_base == BCM7712)
+	if (pcie->cfg->soc_base == BCM7712)
 		return;
 
 	/* Each unit in timeout register is 1/216,000,000 seconds */
@@ -1277,7 +1272,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 	int ret, i;
 
 	/* Unassert the fundamental reset */
-	ret = pcie->perst_set(pcie, 0);
+	ret = pcie->cfg->perst_set(pcie, 0);
 	if (ret)
 		return ret;
 
@@ -1463,12 +1458,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
 
 static inline int brcm_phy_start(struct brcm_pcie *pcie)
 {
-	return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
+	return pcie->cfg->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
 static inline int brcm_phy_stop(struct brcm_pcie *pcie)
 {
-	return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
+	return pcie->cfg->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
 }
 
 static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1479,7 +1474,7 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	if (brcm_pcie_link_up(pcie))
 		brcm_pcie_enter_l23(pcie);
 	/* Assert fundamental reset */
-	ret = pcie->perst_set(pcie, 1);
+	ret = pcie->cfg->perst_set(pcie, 1);
 	if (ret)
 		return ret;
 
@@ -1582,7 +1577,7 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 		goto err_reset;
 
 	/* Take bridge out of reset so we can access the SERDES reg */
-	pcie->bridge_sw_init_set(pcie, 0);
+	pcie->cfg->bridge_sw_init_set(pcie, 0);
 
 	/* SERDES_IDDQ = 0 */
 	tmp = readl(base + HARD_DEBUG(pcie));
@@ -1803,12 +1798,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie = pci_host_bridge_priv(bridge);
 	pcie->dev = &pdev->dev;
 	pcie->np = np;
-	pcie->reg_offsets = data->offsets;
-	pcie->soc_base = data->soc_base;
-	pcie->perst_set = data->perst_set;
-	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
-	pcie->has_phy = data->has_phy;
-	pcie->num_inbound_wins = data->num_inbound_wins;
+	pcie->cfg = data;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
@@ -1843,7 +1833,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
 
-	pcie->bridge_sw_init_set(pcie, 0);
+	pcie->cfg->bridge_sw_init_set(pcie, 0);
 
 	if (pcie->swinit_reset) {
 		ret = reset_control_assert(pcie->swinit_reset);
@@ -1882,7 +1872,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto fail;
 
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
-	if (pcie->soc_base == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
+	if (pcie->cfg->soc_base == BCM4908 &&
+	    pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
 		ret = -ENODEV;
 		goto fail;
@@ -1897,7 +1888,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	bridge->ops = pcie->soc_base == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
+	bridge->ops = pcie->cfg->soc_base == BCM7425 ?
+				&brcm7425_pcie_ops : &brcm_pcie_ops;
 	bridge->sysdata = pcie;
 
 	platform_set_drvdata(pdev, pcie);
-- 
2.43.0

[PATCH v4 05/10] PCI: brcmstb: Expand inbound window size up to 64GB

[Stanimir Varbanov]

BCM2712 memory map can support up to 64GB of system memory, thus expand
the inbound window size in calculation helper function.

The change is save for the currently supported SoCs that has smaller
inbound window sizes.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
v3 -> v4:
 - Improved the subject and description of the patch (Bjorn)

 drivers/pci/controller/pcie-brcmstb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 12bcc5919924..c01462b07eea 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -304,8 +304,8 @@ static int brcm_pcie_encode_ibar_size(u64 size)
 	if (log2_in >= 12 && log2_in <= 15)
 		/* Covers 4KB to 32KB (inclusive) */
 		return (log2_in - 12) + 0x1c;
-	else if (log2_in >= 16 && log2_in <= 35)
-		/* Covers 64KB to 32GB, (inclusive) */
+	else if (log2_in >= 16 && log2_in <= 36)
+		/* Covers 64KB to 64GB, (inclusive) */
 		return log2_in - 15;
 	/* Something is awry so disable */
 	return 0;
-- 
2.43.0

[PATCH v4 06/10] PCI: brcmstb: Enable external MSI-X if available

[Stanimir Varbanov]

On RPi5 there is an external MIP MSI-X interrupt controller
which can handle up to 64 interrupts.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
---
v3 -> v4:
 - no changes.

 drivers/pci/controller/pcie-brcmstb.c | 63 +++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c01462b07eea..af01a7915c94 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1318,6 +1318,52 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 	return 0;
 }
 
+static int brcm_pcie_enable_external_msix(struct brcm_pcie *pcie,
+					  struct device_node *msi_np)
+{
+	struct inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
+	u64 msi_pci_addr, msi_phys_addr;
+	struct resource r;
+	int mip_bar, ret;
+	u32 val, reg;
+
+	ret = of_property_read_reg(msi_np, 1, &msi_pci_addr, NULL);
+	if (ret)
+		return ret;
+
+	ret = of_address_to_resource(msi_np, 0, &r);
+	if (ret)
+		return ret;
+
+	msi_phys_addr = r.start;
+
+	/* Find free inbound window for MIP access */
+	mip_bar = brcm_pcie_get_inbound_wins(pcie, inbound_wins);
+	if (mip_bar < 0)
+		return mip_bar;
+
+	mip_bar += 1;
+	reg = brcm_bar_reg_offset(mip_bar);
+
+	val = lower_32_bits(msi_pci_addr);
+	val |= brcm_pcie_encode_ibar_size(SZ_4K);
+	writel(val, pcie->base + reg);
+
+	val = upper_32_bits(msi_pci_addr);
+	writel(val, pcie->base + reg + 4);
+
+	reg = brcm_ubus_reg_offset(mip_bar);
+
+	val = lower_32_bits(msi_phys_addr);
+	val |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
+	writel(val, pcie->base + reg);
+
+	val = upper_32_bits(msi_phys_addr);
+	writel(val, pcie->base + reg + 4);
+
+	return 0;
+}
+
 static const char * const supplies[] = {
 	"vpcie3v3",
 	"vpcie3v3aux",
@@ -1879,11 +1925,20 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
-	if (pci_msi_enabled() && msi_np == pcie->np) {
-		ret = brcm_pcie_enable_msi(pcie);
+	if (pci_msi_enabled()) {
+		msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
+		const char *str;
+
+		if (msi_np == pcie->np) {
+			str = "internal MSI";
+			ret = brcm_pcie_enable_msi(pcie);
+		} else {
+			str = "external MSI-X";
+			ret = brcm_pcie_enable_external_msix(pcie, msi_np);
+		}
+
 		if (ret) {
-			dev_err(pcie->dev, "probe of internal MSI failed");
+			dev_err(pcie->dev, "enable of %s failed\n", str);
 			goto fail;
 		}
 	}
-- 
2.43.0

Re: [PATCH v4 06/10] PCI: brcmstb: Enable external MSI-X if available

[Stanimir Varbanov]

Hi,

Jim, Florian there is no review comments on this patch. Could you please
take a look.

regards,
~Stan

On 10/25/24 15:45, Stanimir Varbanov wrote:
> On RPi5 there is an external MIP MSI-X interrupt controller
> which can handle up to 64 interrupts.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> ---
> v3 -> v4:
>  - no changes.
> 
>  drivers/pci/controller/pcie-brcmstb.c | 63 +++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 

<cut>

Re: [PATCH v4 06/10] PCI: brcmstb: Enable external MSI-X if available

[James Quinlan]

On 10/25/24 08:45, Stanimir Varbanov wrote:
> On RPi5 there is an external MIP MSI-X interrupt controller
> which can handle up to 64 interrupts.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> ---
> v3 -> v4:
>   - no changes.
>
>   drivers/pci/controller/pcie-brcmstb.c | 63 +++++++++++++++++++++++++--
>   1 file changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c01462b07eea..af01a7915c94 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1318,6 +1318,52 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
>   	return 0;
>   }
>   
> +static int brcm_pcie_enable_external_msix(struct brcm_pcie *pcie,
> +					  struct device_node *msi_np)
> +{
> +	struct inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
> +	u64 msi_pci_addr, msi_phys_addr;
> +	struct resource r;
> +	int mip_bar, ret;
> +	u32 val, reg;
> +
> +	ret = of_property_read_reg(msi_np, 1, &msi_pci_addr, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_address_to_resource(msi_np, 0, &r);
> +	if (ret)
> +		return ret;
> +
> +	msi_phys_addr = r.start;
> +
> +	/* Find free inbound window for MIP access */
> +	mip_bar = brcm_pcie_get_inbound_wins(pcie, inbound_wins);
> +	if (mip_bar < 0)
> +		return mip_bar;
> +
> +	mip_bar += 1;
> +	reg = brcm_bar_reg_offset(mip_bar);
> +
> +	val = lower_32_bits(msi_pci_addr);
> +	val |= brcm_pcie_encode_ibar_size(SZ_4K);
> +	writel(val, pcie->base + reg);
> +
> +	val = upper_32_bits(msi_pci_addr);
> +	writel(val, pcie->base + reg + 4);
> +
> +	reg = brcm_ubus_reg_offset(mip_bar);
> +
> +	val = lower_32_bits(msi_phys_addr);
> +	val |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
> +	writel(val, pcie->base + reg);
> +
> +	val = upper_32_bits(msi_phys_addr);
> +	writel(val, pcie->base + reg + 4);

Hi Stan,

It looks like all this is doing is setting up an identity-mapped inbound 
window, is that correct?  If so, couldn't you get the same effect by 
adding an identity-mapped dma-range in the PCIe DT node?

Jim Quinlan  Broadcom STB/CM

> +
> +	return 0;
> +}
> +
>   static const char * const supplies[] = {
>   	"vpcie3v3",
>   	"vpcie3v3aux",
> @@ -1879,11 +1925,20 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>   		goto fail;
>   	}
>   
> -	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
> -	if (pci_msi_enabled() && msi_np == pcie->np) {
> -		ret = brcm_pcie_enable_msi(pcie);
> +	if (pci_msi_enabled()) {
> +		msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
> +		const char *str;
> +
> +		if (msi_np == pcie->np) {
> +			str = "internal MSI";
> +			ret = brcm_pcie_enable_msi(pcie);
> +		} else {
> +			str = "external MSI-X";
> +			ret = brcm_pcie_enable_external_msix(pcie, msi_np);
> +		}
> +
>   		if (ret) {
> -			dev_err(pcie->dev, "probe of internal MSI failed");
> +			dev_err(pcie->dev, "enable of %s failed\n", str);
>   			goto fail;
>   		}
>   	}

Re: [PATCH v4 06/10] PCI: brcmstb: Enable external MSI-X if available

[Stanimir Varbanov]

Hi Jim,

Thanks for comments!

On 12/11/24 10:01 PM, James Quinlan wrote:
> On 10/25/24 08:45, Stanimir Varbanov wrote:
>> On RPi5 there is an external MIP MSI-X interrupt controller
>> which can handle up to 64 interrupts.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
>> ---
>> v3 -> v4:
>>   - no changes.
>>
>>   drivers/pci/controller/pcie-brcmstb.c | 63 +++++++++++++++++++++++++--
>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
>> controller/pcie-brcmstb.c
>> index c01462b07eea..af01a7915c94 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -1318,6 +1318,52 @@ static int brcm_pcie_start_link(struct
>> brcm_pcie *pcie)
>>       return 0;
>>   }
>>   +static int brcm_pcie_enable_external_msix(struct brcm_pcie *pcie,
>> +                      struct device_node *msi_np)
>> +{
>> +    struct inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
>> +    u64 msi_pci_addr, msi_phys_addr;
>> +    struct resource r;
>> +    int mip_bar, ret;
>> +    u32 val, reg;
>> +
>> +    ret = of_property_read_reg(msi_np, 1, &msi_pci_addr, NULL);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = of_address_to_resource(msi_np, 0, &r);
>> +    if (ret)
>> +        return ret;
>> +
>> +    msi_phys_addr = r.start;
>> +
>> +    /* Find free inbound window for MIP access */
>> +    mip_bar = brcm_pcie_get_inbound_wins(pcie, inbound_wins);
>> +    if (mip_bar < 0)
>> +        return mip_bar;
>> +
>> +    mip_bar += 1;
>> +    reg = brcm_bar_reg_offset(mip_bar);
>> +
>> +    val = lower_32_bits(msi_pci_addr);
>> +    val |= brcm_pcie_encode_ibar_size(SZ_4K);
>> +    writel(val, pcie->base + reg);
>> +
>> +    val = upper_32_bits(msi_pci_addr);
>> +    writel(val, pcie->base + reg + 4);
>> +
>> +    reg = brcm_ubus_reg_offset(mip_bar);
>> +
>> +    val = lower_32_bits(msi_phys_addr);
>> +    val |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
>> +    writel(val, pcie->base + reg);
>> +
>> +    val = upper_32_bits(msi_phys_addr);
>> +    writel(val, pcie->base + reg + 4);
> 
> Hi Stan,
> 
> It looks like all this is doing is setting up an identity-mapped inbound
> window, is that correct?  If so, couldn't you get the same effect by
> adding an identity-mapped dma-range in the PCIe DT node?

Yes, that approach could work, I verified it.

Do you want me to drop this patch from the series and make the relevant
changes in PCIe dma-ranges properties?

~Stan

Re: [PATCH v4 06/10] PCI: brcmstb: Enable external MSI-X if available

[Jim Quinlan]

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

On Wed, Dec 18, 2024 at 9:54 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> Thanks for comments!
>
> On 12/11/24 10:01 PM, James Quinlan wrote:
> > On 10/25/24 08:45, Stanimir Varbanov wrote:
> >> On RPi5 there is an external MIP MSI-X interrupt controller
> >> which can handle up to 64 interrupts.
> >>
> >> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> >> ---
> >> v3 -> v4:
> >>   - no changes.
> >>
> >>   drivers/pci/controller/pcie-brcmstb.c | 63 +++++++++++++++++++++++++--
> >>   1 file changed, 59 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
> >> controller/pcie-brcmstb.c
> >> index c01462b07eea..af01a7915c94 100644
> >> --- a/drivers/pci/controller/pcie-brcmstb.c
> >> +++ b/drivers/pci/controller/pcie-brcmstb.c
> >> @@ -1318,6 +1318,52 @@ static int brcm_pcie_start_link(struct
> >> brcm_pcie *pcie)
> >>       return 0;
> >>   }
> >>   +static int brcm_pcie_enable_external_msix(struct brcm_pcie *pcie,
> >> +                      struct device_node *msi_np)
> >> +{
> >> +    struct inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
> >> +    u64 msi_pci_addr, msi_phys_addr;
> >> +    struct resource r;
> >> +    int mip_bar, ret;
> >> +    u32 val, reg;
> >> +
> >> +    ret = of_property_read_reg(msi_np, 1, &msi_pci_addr, NULL);
> >> +    if (ret)
> >> +        return ret;
> >> +
> >> +    ret = of_address_to_resource(msi_np, 0, &r);
> >> +    if (ret)
> >> +        return ret;
> >> +
> >> +    msi_phys_addr = r.start;
> >> +
> >> +    /* Find free inbound window for MIP access */
> >> +    mip_bar = brcm_pcie_get_inbound_wins(pcie, inbound_wins);
> >> +    if (mip_bar < 0)
> >> +        return mip_bar;
> >> +
> >> +    mip_bar += 1;
> >> +    reg = brcm_bar_reg_offset(mip_bar);
> >> +
> >> +    val = lower_32_bits(msi_pci_addr);
> >> +    val |= brcm_pcie_encode_ibar_size(SZ_4K);
> >> +    writel(val, pcie->base + reg);
> >> +
> >> +    val = upper_32_bits(msi_pci_addr);
> >> +    writel(val, pcie->base + reg + 4);
> >> +
> >> +    reg = brcm_ubus_reg_offset(mip_bar);
> >> +
> >> +    val = lower_32_bits(msi_phys_addr);
> >> +    val |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
> >> +    writel(val, pcie->base + reg);
> >> +
> >> +    val = upper_32_bits(msi_phys_addr);
> >> +    writel(val, pcie->base + reg + 4);
> >
> > Hi Stan,
> >
> > It looks like all this is doing is setting up an identity-mapped inbound
> > window, is that correct?  If so, couldn't you get the same effect by
> > adding an identity-mapped dma-range in the PCIe DT node?
>
> Yes, that approach could work, I verified it.
>
> Do you want me to drop this patch from the series and make the relevant
> changes in PCIe dma-ranges properties?
>
Hi Stan,

Yes please, that was what I was hoping for -- the less code the
better, assuming everything still works :-)

Thanks & regards,
Jim Quinlan
Broadcom STB/CM

> ~Stan
>
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[PATCH v4 07/10] PCI: brcmstb: Add bcm2712 support

[Stanimir Varbanov]

Add bare minimum amount of changes in order to support PCIe RC hardware
IP found on RPi5. The PCIe controller on bcm2712 is based on bcm7712 and
as such it inherits register offsets, perst, bridge_reset ops and inbound
windows count.
Although, the implementation for bcm2712 needs a workaround related to the
control of the bridge_reset where turning off of the root port must not
shutdown the bridge_reset and this must be avoided. To implement this
workaround a quirks field is introduced in pcie_cfg_data struct.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
v3 -> v4:
 - Merged "PCI: brcmstb: Avoid turn off of bridge reset" from v3 here
   in this patch (Bjorn)

 drivers/pci/controller/pcie-brcmstb.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index af01a7915c94..d970a76aa9ef 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -234,10 +234,20 @@ struct inbound_win {
 	u64 cpu_addr;
 };
 
+/*
+ * The RESCAL block is tied to PCIe controller #1, regardless of the number of
+ * controllers, and turning off PCIe controller #1 prevents access to the RESCAL
+ * register blocks, therefore no other controller can access this register
+ * space, and depending upon the bus fabric we may get a timeout (UBUS/GISB),
+ * or a hang (AXI).
+ */
+#define CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN		BIT(0)
+
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_soc_base soc_base;
 	const bool has_phy;
+	const u32 quirks;
 	u8 num_inbound_wins;
 	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
@@ -1534,8 +1544,9 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	u32p_replace_bits(&tmp, 1, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
 	writel(tmp, base + HARD_DEBUG(pcie));
 
-	/* Shutdown PCIe bridge */
-	ret = pcie->bridge_sw_init_set(pcie, 1);
+	if (!(pcie->cfg->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN))
+		/* Shutdown PCIe bridge */
+		ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
 
 	return ret;
 }
@@ -1745,6 +1756,15 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.num_inbound_wins = 3,
 };
 
+static const struct pcie_cfg_data bcm2712_cfg = {
+	.offsets	= pcie_offsets_bcm7712,
+	.soc_base	= BCM7712,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.quirks		= CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN,
+	.num_inbound_wins = 10,
+};
+
 static const struct pcie_cfg_data bcm4908_cfg = {
 	.offsets	= pcie_offsets,
 	.soc_base	= BCM4908,
@@ -1796,6 +1816,7 @@ static const struct pcie_cfg_data bcm7712_cfg = {
 
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
+	{ .compatible = "brcm,bcm2712-pcie", .data = &bcm2712_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
 	{ .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
-- 
2.43.0

[PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[Stanimir Varbanov]

The default input reference clock for the PHY PLL is 100Mhz, except for
some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.

To implement this adjustments introduce a new .post_setup op in
pcie_cfg_data and call it at the end of brcm_pcie_setup function.

The bcm2712 .post_setup callback implements the required MDIO writes that
switch the PLL refclk and also change PHY PM clock period.

Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
the expansion connector.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
---
v3 -> v4:
 - Improved patch description (Florian)

 drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d970a76aa9ef..2571dcc14560 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -55,6 +55,10 @@
 #define PCIE_RC_DL_MDIO_WR_DATA				0x1104
 #define PCIE_RC_DL_MDIO_RD_DATA				0x1108
 
+#define PCIE_RC_PL_PHY_CTL_15				0x184c
+#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK		0x400000
+#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK	0xff
+
 #define PCIE_MISC_MISC_CTRL				0x4008
 #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK	0x80
 #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK	0x400
@@ -251,6 +255,7 @@ struct pcie_cfg_data {
 	u8 num_inbound_wins;
 	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int (*post_setup)(struct brcm_pcie *pcie);
 };
 
 struct subdev_regulators {
@@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
 	return 0;
 }
 
+static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
+{
+	const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030, 0x5030, 0x0007 };
+	const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
+	int ret, i;
+	u32 tmp;
+
+	/* Allow a 54MHz (xosc) refclk source */
+	ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, SET_ADDR_OFFSET, 0x1600);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(regs); i++) {
+		ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i], data[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	usleep_range(100, 200);
+
+	/* Fix for L1SS errata */
+	tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
+	tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
+	/* PM clock period is 18.52ns (round down) */
+	tmp |= 0x12;
+	writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);
+
+	return 0;
+}
+
 static void add_inbound_win(struct inbound_win *b, u8 *count, u64 size,
 			    u64 cpu_addr, u64 pci_offset)
 {
@@ -1189,6 +1224,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
 	writel(tmp, base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
 
+	if (pcie->cfg->post_setup) {
+		ret = pcie->cfg->post_setup(pcie);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -1761,6 +1802,7 @@ static const struct pcie_cfg_data bcm2712_cfg = {
 	.soc_base	= BCM7712,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.post_setup	= brcm_pcie_post_setup_bcm2712,
 	.quirks		= CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN,
 	.num_inbound_wins = 10,
 };
-- 
2.43.0

Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[Florian Fainelli]

On 10/25/24 05:45, Stanimir Varbanov wrote:
> The default input reference clock for the PHY PLL is 100Mhz, except for
> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
> 
> To implement this adjustments introduce a new .post_setup op in
> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
> 
> The bcm2712 .post_setup callback implements the required MDIO writes that
> switch the PLL refclk and also change PHY PM clock period.
> 
> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
> the expansion connector.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[James Quinlan]

On 10/25/24 08:45, Stanimir Varbanov wrote:
> The default input reference clock for the PHY PLL is 100Mhz, except for
> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
>
> To implement this adjustments introduce a new .post_setup op in
> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
>
> The bcm2712 .post_setup callback implements the required MDIO writes that
> switch the PLL refclk and also change PHY PM clock period.
>
> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
> the expansion connector.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> ---
> v3 -> v4:
>   - Improved patch description (Florian)
>
>   drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d970a76aa9ef..2571dcc14560 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -55,6 +55,10 @@
>   #define PCIE_RC_DL_MDIO_WR_DATA				0x1104
>   #define PCIE_RC_DL_MDIO_RD_DATA				0x1108
>   
> +#define PCIE_RC_PL_PHY_CTL_15				0x184c
> +#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK		0x400000
> +#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK	0xff
> +
>   #define PCIE_MISC_MISC_CTRL				0x4008
>   #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK	0x80
>   #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK	0x400
> @@ -251,6 +255,7 @@ struct pcie_cfg_data {
>   	u8 num_inbound_wins;
>   	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
>   	int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> +	int (*post_setup)(struct brcm_pcie *pcie);
>   };
>   
>   struct subdev_regulators {
> @@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
>   	return 0;
>   }
>   
> +static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
> +{
> +	const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030, 0x5030, 0x0007 };
> +	const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
> +	int ret, i;
> +	u32 tmp;
> +
> +	/* Allow a 54MHz (xosc) refclk source */
> +	ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, SET_ADDR_OFFSET, 0x1600);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(regs); i++) {
> +		ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i], data[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	usleep_range(100, 200);
> +
> +	/* Fix for L1SS errata */
> +	tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
> +	tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
> +	/* PM clock period is 18.52ns (round down) */
> +	tmp |= 0x12;
> +	writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);

Hi Stan,

Can you please say more about where this errata came from?  I asked the 
7712 PCIe HW folks and they said that there best guess was that it was a 
old workaround for a particular Broadcom Wifi endpoint.  Do you know its 
origin?

Thanks,

Jim Quinlan

Broadcom STB/CM

> +
> +	return 0;
> +}
> +
>   static void add_inbound_win(struct inbound_win *b, u8 *count, u64 size,
>   			    u64 cpu_addr, u64 pci_offset)
>   {
> @@ -1189,6 +1224,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>   		PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
>   	writel(tmp, base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
>   
> +	if (pcie->cfg->post_setup) {
> +		ret = pcie->cfg->post_setup(pcie);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -1761,6 +1802,7 @@ static const struct pcie_cfg_data bcm2712_cfg = {
>   	.soc_base	= BCM7712,
>   	.perst_set	= brcm_pcie_perst_set_7278,
>   	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.post_setup	= brcm_pcie_post_setup_bcm2712,
>   	.quirks		= CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN,
>   	.num_inbound_wins = 10,
>   };

Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[Stanimir Varbanov]

Hi Jim

On 12/10/24 12:52 AM, James Quinlan wrote:
> On 10/25/24 08:45, Stanimir Varbanov wrote:
>> The default input reference clock for the PHY PLL is 100Mhz, except for
>> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
>>
>> To implement this adjustments introduce a new .post_setup op in
>> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
>>
>> The bcm2712 .post_setup callback implements the required MDIO writes that
>> switch the PLL refclk and also change PHY PM clock period.
>>
>> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
>> the expansion connector.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
>> ---
>> v3 -> v4:
>>   - Improved patch description (Florian)
>>
>>   drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
>> controller/pcie-brcmstb.c
>> index d970a76aa9ef..2571dcc14560 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -55,6 +55,10 @@
>>   #define PCIE_RC_DL_MDIO_WR_DATA                0x1104
>>   #define PCIE_RC_DL_MDIO_RD_DATA                0x1108
>>   +#define PCIE_RC_PL_PHY_CTL_15                0x184c
>> +#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK        0x400000
>> +#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK    0xff
>> +
>>   #define PCIE_MISC_MISC_CTRL                0x4008
>>   #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK    0x80
>>   #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK    0x400
>> @@ -251,6 +255,7 @@ struct pcie_cfg_data {
>>       u8 num_inbound_wins;
>>       int (*perst_set)(struct brcm_pcie *pcie, u32 val);
>>       int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>> +    int (*post_setup)(struct brcm_pcie *pcie);
>>   };
>>     struct subdev_regulators {
>> @@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct
>> brcm_pcie *pcie, u32 val)
>>       return 0;
>>   }
>>   +static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
>> +{
>> +    const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030,
>> 0x5030, 0x0007 };
>> +    const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
>> +    int ret, i;
>> +    u32 tmp;
>> +
>> +    /* Allow a 54MHz (xosc) refclk source */
>> +    ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0,
>> SET_ADDR_OFFSET, 0x1600);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(regs); i++) {
>> +        ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i],
>> data[i]);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +    usleep_range(100, 200);
>> +
>> +    /* Fix for L1SS errata */
>> +    tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
>> +    tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
>> +    /* PM clock period is 18.52ns (round down) */
>> +    tmp |= 0x12;
>> +    writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);
> 
> Hi Stan,
> 
> Can you please say more about where this errata came from?  I asked the
> 7712 PCIe HW folks and they said that there best guess was that it was a
> old workaround for a particular Broadcom Wifi endpoint.  Do you know its
> origin?

Unfortunately, I don't know the details. See the comments on previous
series version [1]. My observation shows that MDIO writes are
implemented in RPi platform firmware only for pcie2 (where RP1 south
bridge is connected) but not for pcie1 expansion connector.

~Stan

[1] https://www.spinics.net/lists/linux-pci/msg160842.html

Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[James Quinlan]

On 12/10/24 08:42, Stanimir Varbanov wrote:
> Hi Jim
>
> On 12/10/24 12:52 AM, James Quinlan wrote:
>> On 10/25/24 08:45, Stanimir Varbanov wrote:
>>> The default input reference clock for the PHY PLL is 100Mhz, except for
>>> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
>>>
>>> To implement this adjustments introduce a new .post_setup op in
>>> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
>>>
>>> The bcm2712 .post_setup callback implements the required MDIO writes that
>>> switch the PLL refclk and also change PHY PM clock period.
>>>
>>> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
>>> the expansion connector.
>>>
>>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
>>> ---
>>> v3 -> v4:
>>>    - Improved patch description (Florian)
>>>
>>>    drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
>>>    1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
>>> controller/pcie-brcmstb.c
>>> index d970a76aa9ef..2571dcc14560 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>> @@ -55,6 +55,10 @@
>>>    #define PCIE_RC_DL_MDIO_WR_DATA                0x1104
>>>    #define PCIE_RC_DL_MDIO_RD_DATA                0x1108
>>>    +#define PCIE_RC_PL_PHY_CTL_15                0x184c
>>> +#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK        0x400000
>>> +#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK    0xff
>>> +
>>>    #define PCIE_MISC_MISC_CTRL                0x4008
>>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK    0x80
>>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK    0x400
>>> @@ -251,6 +255,7 @@ struct pcie_cfg_data {
>>>        u8 num_inbound_wins;
>>>        int (*perst_set)(struct brcm_pcie *pcie, u32 val);
>>>        int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>>> +    int (*post_setup)(struct brcm_pcie *pcie);
>>>    };
>>>      struct subdev_regulators {
>>> @@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct
>>> brcm_pcie *pcie, u32 val)
>>>        return 0;
>>>    }
>>>    +static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
>>> +{
>>> +    const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030,
>>> 0x5030, 0x0007 };
>>> +    const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
>>> +    int ret, i;
>>> +    u32 tmp;
>>> +
>>> +    /* Allow a 54MHz (xosc) refclk source */
>>> +    ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0,
>>> SET_ADDR_OFFSET, 0x1600);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(regs); i++) {
>>> +        ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i],
>>> data[i]);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +    }
>>> +
>>> +    usleep_range(100, 200);
>>> +
>>> +    /* Fix for L1SS errata */
>>> +    tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
>>> +    tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
>>> +    /* PM clock period is 18.52ns (round down) */
>>> +    tmp |= 0x12;
>>> +    writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);
>> Hi Stan,
>>
>> Can you please say more about where this errata came from?  I asked the
>> 7712 PCIe HW folks and they said that there best guess was that it was a
>> old workaround for a particular Broadcom Wifi endpoint.  Do you know its
>> origin?
> Unfortunately, I don't know the details. See the comments on previous
> series version [1]. My observation shows that MDIO writes are
> implemented in RPi platform firmware only for pcie2 (where RP1 south
> bridge is connected) but not for pcie1 expansion connector.

Well, I think my concern is more about the comment "Fix for L1SS errata" 
rather than the code.  If this is a bonafide errata it should have an 
identifier and some documentation somewhere. Declaring it to be an 
unknown errata provides little info.

Code-wise, you could use u32p_replace_bits(..., PM_CLK_PERIOD_MASK) to 
do the field value insertion.

All the above being said, I have no objection since this code is 
specific to the RPi platform.

Jim Quinlan  Broadcom STB/CM

>
> ~Stan
>
> [1] https://www.spinics.net/lists/linux-pci/msg160842.html
>

Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[Jonathan Bell]

On Wed, 11 Dec 2024 at 19:39, James Quinlan <james.quinlan@broadcom.com> wrote:
>
> On 12/10/24 08:42, Stanimir Varbanov wrote:
> > Hi Jim
> >
> > On 12/10/24 12:52 AM, James Quinlan wrote:
> >> On 10/25/24 08:45, Stanimir Varbanov wrote:
> >>> The default input reference clock for the PHY PLL is 100Mhz, except for
> >>> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
> >>>
> >>> To implement this adjustments introduce a new .post_setup op in
> >>> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
> >>>
> >>> The bcm2712 .post_setup callback implements the required MDIO writes that
> >>> switch the PLL refclk and also change PHY PM clock period.
> >>>
> >>> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
> >>> the expansion connector.
> >>>
> >>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> >>> ---
> >>> v3 -> v4:
> >>>    - Improved patch description (Florian)
> >>>
> >>>    drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
> >>>    1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
> >>> controller/pcie-brcmstb.c
> >>> index d970a76aa9ef..2571dcc14560 100644
> >>> --- a/drivers/pci/controller/pcie-brcmstb.c
> >>> +++ b/drivers/pci/controller/pcie-brcmstb.c
> >>> @@ -55,6 +55,10 @@
> >>>    #define PCIE_RC_DL_MDIO_WR_DATA                0x1104
> >>>    #define PCIE_RC_DL_MDIO_RD_DATA                0x1108
> >>>    +#define PCIE_RC_PL_PHY_CTL_15                0x184c
> >>> +#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK        0x400000
> >>> +#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK    0xff
> >>> +
> >>>    #define PCIE_MISC_MISC_CTRL                0x4008
> >>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK    0x80
> >>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK    0x400
> >>> @@ -251,6 +255,7 @@ struct pcie_cfg_data {
> >>>        u8 num_inbound_wins;
> >>>        int (*perst_set)(struct brcm_pcie *pcie, u32 val);
> >>>        int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> >>> +    int (*post_setup)(struct brcm_pcie *pcie);
> >>>    };
> >>>      struct subdev_regulators {
> >>> @@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct
> >>> brcm_pcie *pcie, u32 val)
> >>>        return 0;
> >>>    }
> >>>    +static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
> >>> +{
> >>> +    const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030,
> >>> 0x5030, 0x0007 };
> >>> +    const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
> >>> +    int ret, i;
> >>> +    u32 tmp;
> >>> +
> >>> +    /* Allow a 54MHz (xosc) refclk source */
> >>> +    ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0,
> >>> SET_ADDR_OFFSET, 0x1600);
> >>> +    if (ret < 0)
> >>> +        return ret;
> >>> +
> >>> +    for (i = 0; i < ARRAY_SIZE(regs); i++) {
> >>> +        ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i],
> >>> data[i]);
> >>> +        if (ret < 0)
> >>> +            return ret;
> >>> +    }
> >>> +
> >>> +    usleep_range(100, 200);
> >>> +
> >>> +    /* Fix for L1SS errata */
> >>> +    tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
> >>> +    tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
> >>> +    /* PM clock period is 18.52ns (round down) */
> >>> +    tmp |= 0x12;
> >>> +    writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);
> >> Hi Stan,
> >>
> >> Can you please say more about where this errata came from?  I asked the
> >> 7712 PCIe HW folks and they said that there best guess was that it was a
> >> old workaround for a particular Broadcom Wifi endpoint.  Do you know its
> >> origin?
> > Unfortunately, I don't know the details. See the comments on previous
> > series version [1]. My observation shows that MDIO writes are
> > implemented in RPi platform firmware only for pcie2 (where RP1 south
> > bridge is connected) but not for pcie1 expansion connector.
>
> Well, I think my concern is more about the comment "Fix for L1SS errata"
> rather than the code.  If this is a bonafide errata it should have an
> identifier and some documentation somewhere. Declaring it to be an
> unknown errata provides little info.

I'm the originator of this thunk - erratum is perhaps the wrong description.
If the reference clock provided to the RC is 54MHz and not 100MHz, as
is the case on BCM2712, then many of the L1 sub-state timers run
slower which means state transitions are unnecessarily lengthened.

This change, and the MDIO manipulation above, should be applied
regardless of the RC instance and/or connected EP.

> Code-wise, you could use u32p_replace_bits(..., PM_CLK_PERIOD_MASK) to
> do the field value insertion.
>
> All the above being said, I have no objection since this code is
> specific to the RPi platform.
>
> Jim Quinlan  Broadcom STB/CM
>
> >
> > ~Stan
> >
> > [1] https://www.spinics.net/lists/linux-pci/msg160842.html
> >
>

Regards
Jonathan

Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[Stanimir Varbanov]

Hi

On 12/11/24 10:54 PM, Jonathan Bell wrote:
> On Wed, 11 Dec 2024 at 19:39, James Quinlan <james.quinlan@broadcom.com> wrote:
>>
>> On 12/10/24 08:42, Stanimir Varbanov wrote:
>>> Hi Jim
>>>
>>> On 12/10/24 12:52 AM, James Quinlan wrote:
>>>> On 10/25/24 08:45, Stanimir Varbanov wrote:
>>>>> The default input reference clock for the PHY PLL is 100Mhz, except for
>>>>> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
>>>>>
>>>>> To implement this adjustments introduce a new .post_setup op in
>>>>> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
>>>>>
>>>>> The bcm2712 .post_setup callback implements the required MDIO writes that
>>>>> switch the PLL refclk and also change PHY PM clock period.
>>>>>
>>>>> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
>>>>> the expansion connector.
>>>>>
>>>>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
>>>>> ---
>>>>> v3 -> v4:
>>>>>    - Improved patch description (Florian)
>>>>>
>>>>>    drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
>>>>>    1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
>>>>> controller/pcie-brcmstb.c
>>>>> index d970a76aa9ef..2571dcc14560 100644
>>>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>>>> @@ -55,6 +55,10 @@
>>>>>    #define PCIE_RC_DL_MDIO_WR_DATA                0x1104
>>>>>    #define PCIE_RC_DL_MDIO_RD_DATA                0x1108
>>>>>    +#define PCIE_RC_PL_PHY_CTL_15                0x184c
>>>>> +#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK        0x400000
>>>>> +#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK    0xff
>>>>> +
>>>>>    #define PCIE_MISC_MISC_CTRL                0x4008
>>>>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK    0x80
>>>>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK    0x400
>>>>> @@ -251,6 +255,7 @@ struct pcie_cfg_data {
>>>>>        u8 num_inbound_wins;
>>>>>        int (*perst_set)(struct brcm_pcie *pcie, u32 val);
>>>>>        int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>>>>> +    int (*post_setup)(struct brcm_pcie *pcie);
>>>>>    };
>>>>>      struct subdev_regulators {
>>>>> @@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct
>>>>> brcm_pcie *pcie, u32 val)
>>>>>        return 0;
>>>>>    }
>>>>>    +static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
>>>>> +{
>>>>> +    const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030,
>>>>> 0x5030, 0x0007 };
>>>>> +    const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
>>>>> +    int ret, i;
>>>>> +    u32 tmp;
>>>>> +
>>>>> +    /* Allow a 54MHz (xosc) refclk source */
>>>>> +    ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0,
>>>>> SET_ADDR_OFFSET, 0x1600);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    for (i = 0; i < ARRAY_SIZE(regs); i++) {
>>>>> +        ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i],
>>>>> data[i]);
>>>>> +        if (ret < 0)
>>>>> +            return ret;
>>>>> +    }
>>>>> +
>>>>> +    usleep_range(100, 200);
>>>>> +
>>>>> +    /* Fix for L1SS errata */
>>>>> +    tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
>>>>> +    tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
>>>>> +    /* PM clock period is 18.52ns (round down) */
>>>>> +    tmp |= 0x12;
>>>>> +    writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);
>>>> Hi Stan,
>>>>
>>>> Can you please say more about where this errata came from?  I asked the
>>>> 7712 PCIe HW folks and they said that there best guess was that it was a
>>>> old workaround for a particular Broadcom Wifi endpoint.  Do you know its
>>>> origin?
>>> Unfortunately, I don't know the details. See the comments on previous
>>> series version [1]. My observation shows that MDIO writes are
>>> implemented in RPi platform firmware only for pcie2 (where RP1 south
>>> bridge is connected) but not for pcie1 expansion connector.
>>
>> Well, I think my concern is more about the comment "Fix for L1SS errata"
>> rather than the code.  If this is a bonafide errata it should have an
>> identifier and some documentation somewhere. Declaring it to be an
>> unknown errata provides little info.
> 
> I'm the originator of this thunk - erratum is perhaps the wrong description.
> If the reference clock provided to the RC is 54MHz and not 100MHz, as
> is the case on BCM2712, then many of the L1 sub-state timers run
> slower which means state transitions are unnecessarily lengthened.
> 
> This change, and the MDIO manipulation above, should be applied
> regardless of the RC instance and/or connected EP.
> 

Thank you Jonathan.

I guess you are fine to take this description in the next version of the
series?

~Stan

[PATCH v4 09/10] arm64: dts: broadcom: bcm2712: Add PCIe DT nodes

[Stanimir Varbanov]

Add PCIe devicetree nodes, plus needed reset and mip MSI-X
controllers.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
---
v3 -> v4:
 - Added msi-controller property required by brcm,stb-pcie.yaml schema

 arch/arm64/boot/dts/broadcom/bcm2712.dtsi | 162 ++++++++++++++++++++++
 1 file changed, 162 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
index 6e5a984c1d4e..8fcdf27c1221 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcm2712.dtsi
@@ -192,6 +192,12 @@ soc: soc@107c000000 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		pcie_rescal: reset-controller@119500 {
+			compatible = "brcm,bcm7216-pcie-sata-rescal";
+			reg = <0x00119500 0x10>;
+			#reset-cells = <0>;
+		};
+
 		sdio1: mmc@fff000 {
 			compatible = "brcm,bcm2712-sdhci",
 				     "brcm,sdhci-brcmstb";
@@ -204,6 +210,12 @@ sdio1: mmc@fff000 {
 			mmc-ddr-3_3v;
 		};
 
+		bcm_reset: reset-controller@1504318 {
+			compatible = "brcm,brcmstb-reset";
+			reg = <0x01504318 0x30>;
+			#reset-cells = <1>;
+		};
+
 		system_timer: timer@7c003000 {
 			compatible = "brcm,bcm2835-system-timer";
 			reg = <0x7c003000 0x1000>;
@@ -267,6 +279,156 @@ gicv2: interrupt-controller@7fff9000 {
 		};
 	};
 
+	axi@1000000000 {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+
+		ranges = <0x00 0x00000000 0x10 0x00000000 0x01 0x00000000>,
+			 <0x14 0x00000000 0x14 0x00000000 0x04 0x00000000>,
+			 <0x18 0x00000000 0x18 0x00000000 0x04 0x00000000>,
+			 <0x1c 0x00000000 0x1c 0x00000000 0x04 0x00000000>;
+
+		dma-ranges = <0x00 0x00000000 0x00 0x00000000 0x10 0x00000000>,
+			     <0x14 0x00000000 0x14 0x00000000 0x04 0x00000000>,
+			     <0x18 0x00000000 0x18 0x00000000 0x04 0x00000000>,
+			     <0x1c 0x00000000 0x1c 0x00000000 0x04 0x00000000>;
+
+		pcie0: pcie@100000 {
+			compatible = "brcm,bcm2712-pcie";
+			reg = <0x00 0x00100000 0x00 0x9310>;
+			device_type = "pci";
+			linux,pci-domain = <0>;
+			max-link-speed = <2>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <1>;
+			#address-cells = <3>;
+			#interrupt-cells = <1>;
+			#size-cells = <2>;
+			interrupt-parent = <&gicv2>;
+			interrupts = <GIC_SPI 213 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 214 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "pcie", "msi";
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &gicv2 GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &gicv2 GIC_SPI 211 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &gicv2 GIC_SPI 212 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&bcm_reset 42>, <&pcie_rescal>;
+			reset-names = "bridge", "rescal";
+			msi-controller;
+			msi-parent = <&pcie0>;
+
+			ranges =
+				/* ~4GB, 32-bit, non-prefetchable at PCIe 00_0000_0000 */
+				<0x02000000 0x00 0x00000000 0x17 0x00000000 0x00 0xfffffffc>,
+				/* 12GB, 64-bit, prefetchable at PCIe 04_0000_0000 */
+				<0x43000000 0x04 0x00000000 0x14 0x00000000 0x03 0x00000000>;
+
+			dma-ranges =
+				/* 64GB, 64-bit, prefetchable at PCIe 10_0000_0000 */
+				<0x43000000 0x10 0x00000000 0x00 0x00000000 0x10 0x00000000>;
+
+			status = "disabled";
+		};
+
+		pcie1: pcie@110000 {
+			compatible = "brcm,bcm2712-pcie";
+			reg = <0x00 0x00110000 0x00 0x9310>;
+			device_type = "pci";
+			linux,pci-domain = <1>;
+			max-link-speed = <2>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <1>;
+			#address-cells = <3>;
+			#interrupt-cells = <1>;
+			#size-cells = <2>;
+			interrupt-parent = <&gicv2>;
+			interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "pcie", "msi";
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &gicv2 GIC_SPI 220 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &gicv2 GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &gicv2 GIC_SPI 222 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&bcm_reset 43>, <&pcie_rescal>;
+			reset-names = "bridge", "rescal";
+			msi-controller;
+			msi-parent = <&mip1>;
+
+			ranges =
+				/* ~4GB, 32-bit, non-prefetchable at PCIe 00_0000_0000 */
+				<0x02000000 0x00 0x00000000 0x1b 0x00000000 0x00 0xfffffffc>,
+				/* 12GB, 64-bit, prefetchable at PCIe 04_0000_0000 */
+				<0x43000000 0x04 0x00000000 0x18 0x00000000 0x03 0x00000000>;
+
+			dma-ranges =
+				/* 64GB, 64-bit, non-prefetchable at PCIe 10_0000_0000 */
+				<0x03000000 0x10 0x00000000 0x00 0x00000000 0x10 0x00000000>;
+
+			status = "disabled";
+		};
+
+		pcie2: pcie@120000 {
+			compatible = "brcm,bcm2712-pcie";
+			reg = <0x00 0x00120000 0x00 0x9310>;
+			device_type = "pci";
+			linux,pci-domain = <2>;
+			max-link-speed = <2>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <4>;
+			#address-cells = <3>;
+			#interrupt-cells = <1>;
+			#size-cells = <2>;
+			interrupt-parent = <&gicv2>;
+			interrupts = <GIC_SPI 233 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 234 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "pcie", "msi";
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &gicv2 GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &gicv2 GIC_SPI 231 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &gicv2 GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
+			resets = <&bcm_reset 44>, <&pcie_rescal>;
+			reset-names = "bridge", "rescal";
+			msi-controller;
+			msi-parent = <&mip0>;
+
+			ranges =
+				/* ~4GB, 32-bit, non-prefetchable at PCIe 00_0000_0000 */
+				<0x02000000 0x00 0x00000000 0x1f 0x00000000 0x00 0xfffffffc>,
+				/* 12GB, 64-bit, prefetchable at PCIe 04_0000_0000 */
+				<0x43000000 0x04 0x00000000 0x1c 0x00000000 0x03 0x00000000>;
+
+			dma-ranges =
+				/* 4MB, 32-bit, non-prefetchable at PCIe 00_0000_0000 */
+				<0x02000000 0x00 0x00000000 0x1f 0x00000000 0x00 0x00400000>,
+				/* 64GB, 64-bit, prefetchable at PCIe 10_0000_0000 */
+				<0x43000000 0x10 0x00000000 0x00 0x00000000 0x10 0x00000000>;
+
+			status = "disabled";
+		};
+
+		mip0: msi-controller@130000 {
+			compatible = "brcm,bcm2712-mip";
+			reg = <0x00 0x00130000 0x00 0xc0>,
+			      <0xff 0xfffff000 0x00 0x1000>;
+			msi-controller;
+			msi-ranges = <&gicv2 GIC_SPI 128 IRQ_TYPE_EDGE_RISING 64>;
+			brcm,msi-offset = <0>;
+		};
+
+		mip1: msi-controller@131000 {
+			compatible = "brcm,bcm2712-mip";
+			reg = <0x00 0x00131000 0x00 0xc0>,
+			      <0xff 0xfffff000 0x00 0x1000>;
+			msi-controller;
+			msi-ranges = <&gicv2 GIC_SPI 247 IRQ_TYPE_EDGE_RISING 8>;
+			brcm,msi-offset = <8>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
-- 
2.43.0

[PATCH v4 10/10] arm64: dts: broadcom: bcm2712-rpi-5-b: Enable PCIe DT nodes

[Stanimir Varbanov]

Enable pcie1 and pcie2 DT nodes. Pcie1 is used for the extension
connector and pcie2 is used for RP1 south-bridge.

Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
---
v3 -> v4:
 - no changes.

 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
index 2bdbb6780242..e970a6013c6f 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
@@ -62,3 +62,11 @@ &sdio1 {
 	sd-uhs-ddr50;
 	sd-uhs-sdr104;
 };
+
+&pcie1 {
+	status = "okay";
+};
+
+&pcie2 {
+	status = "okay";
+};
-- 
2.43.0

Re: [PATCH v4 00/10] Add PCIe support for bcm2712

[Olivier Benjamin]

On 10/25/24 14:45, Stanimir Varbanov wrote:
> Here is v4 of the series which aims to add support for PCIe on bcm2712 SoC
> used by RPi5. Previous v3 can be found at [1].
> 
Hello Stanimir,

Thank you for you work on this!

> v3 -> v4 changes include:
>   - Addressed comments on the interrupt-controller driver (Thomas)
>   - Moved "Reuse config structure" patch earlier in the series (Bjorn)
>   - Merged "Avoid turn off of bridge reset" into "Add bcm2712 support" (Bjorn)
>   - Fixed DTB warnings on broadcom/bcm2712-rpi-5-b.dtb
> 
> For more detailed info check patches.
> 
I would simply like to report that I have (rather succintly) tested this 
series.
I have built a 6.13.0-rc4-v8 vanilla kernel and deployed it to a 
Raspberry Pi 5 equipped with a "Raspberry Pi SSD" NVMe Drive on an M.2 
Hat+ connected to the main board using PCIe.
This of course did not work, and I could not see my drive.
I then applied this series on top, rebuilt and deployed the kernel, and 
I could see the /dev/nvme0 device and mount the ext4 fs on the 1st 
partition.

> Comments are welcome!
> ~Stan
	
If you find it helpful, feel free to collect my Tested-By tag.

I'll be happy to do the same for future versions of the series and can 
do some additional testing if you want an extra pair of eyes.

Olivier
> 
> [1] https://patchwork.kernel.org/project/linux-pci/list/?series=898891
> 
> Stanimir Varbanov (10):
>    dt-bindings: interrupt-controller: Add bcm2712 MSI-X DT bindings
>    dt-bindings: PCI: brcmstb: Update bindings for PCIe on bcm2712
>    irqchip: Add Broadcom bcm2712 MSI-X interrupt controller
>    PCI: brcmstb: Reuse config structure
>    PCI: brcmstb: Expand inbound window size up to 64GB
>    PCI: brcmstb: Enable external MSI-X if available
>    PCI: brcmstb: Add bcm2712 support
>    PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk
>    arm64: dts: broadcom: bcm2712: Add PCIe DT nodes
>    arm64: dts: broadcom: bcm2712-rpi-5-b: Enable PCIe DT nodes
> 
>   .../brcm,bcm2712-msix.yaml                    |  60 ++++
>   .../bindings/pci/brcm,stb-pcie.yaml           |  21 ++
>   .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |   8 +
>   arch/arm64/boot/dts/broadcom/bcm2712.dtsi     | 162 +++++++++
>   drivers/irqchip/Kconfig                       |  16 +
>   drivers/irqchip/Makefile                      |   1 +
>   drivers/irqchip/irq-bcm2712-mip.c             | 310 ++++++++++++++++++
>   drivers/pci/controller/pcie-brcmstb.c         | 204 +++++++++---
>   8 files changed, 735 insertions(+), 47 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml
>   create mode 100644 drivers/irqchip/irq-bcm2712-mip.c
> 

-- 
Olivier Benjamin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 00/10] Add PCIe support for bcm2712

[Stanimir Varbanov]

Hi Olivier,

On 12/23/24 6:35 PM, Olivier Benjamin wrote:
> On 10/25/24 14:45, Stanimir Varbanov wrote:
>> Here is v4 of the series which aims to add support for PCIe on bcm2712
>> SoC
>> used by RPi5. Previous v3 can be found at [1].
>>
> Hello Stanimir,
> 
> Thank you for you work on this!
> 
>> v3 -> v4 changes include:
>>   - Addressed comments on the interrupt-controller driver (Thomas)
>>   - Moved "Reuse config structure" patch earlier in the series (Bjorn)
>>   - Merged "Avoid turn off of bridge reset" into "Add bcm2712
>> support" (Bjorn)
>>   - Fixed DTB warnings on broadcom/bcm2712-rpi-5-b.dtb
>>
>> For more detailed info check patches.
>>
> I would simply like to report that I have (rather succintly) tested this
> series.
> I have built a 6.13.0-rc4-v8 vanilla kernel and deployed it to a
> Raspberry Pi 5 equipped with a "Raspberry Pi SSD" NVMe Drive on an M.2
> Hat+ connected to the main board using PCIe.
> This of course did not work, and I could not see my drive.
> I then applied this series on top, rebuilt and deployed the kernel, and
> I could see the /dev/nvme0 device and mount the ext4 fs on the 1st
> partition.
> 
>> Comments are welcome!
>> ~Stan
>     
> If you find it helpful, feel free to collect my Tested-By tag.
> 
> I'll be happy to do the same for future versions of the series and can
> do some additional testing if you want an extra pair of eyes.
> 

Thanks for the help! I definitely will need Tested-by for the recently
posted v5 :)

~Stan