[PATCH v6 0/7] Add support for the LAN966x PCI device using a DT overlay

[PATCH v6 0/7] Add support for the LAN966x PCI device using a DT overlay

[Herve Codina]

Hi,

This series adds support for the LAN966x chip when used as a PCI
device.

For reference, the LAN996x chip is a System-on-chip that integrates an
Ethernet switch and a number of other traditional hardware blocks such
as a GPIO controller, I2C controllers, SPI controllers, etc. The
LAN996x can be used in two different modes:

- With Linux running on its Linux built-in ARM cores.
  This mode is already supported by the upstream Linux kernel, with the
  LAN996x described as a standard ARM Device Tree in
  arch/arm/boot/dts/microchip/lan966x.dtsi. Thanks to this support,
  all hardware blocks in the LAN996x already have drivers in the
  upstream Linux kernel.

- As a PCI device, thanks to its built-in PCI endpoint controller.
  In this case, the LAN996x ARM cores are not used, but all peripherals
  of the LAN996x can be accessed by the PCI host using memory-mapped
  I/O through the PCI BARs.

This series aims at supporting this second use-case. As all peripherals
of the LAN996x already have drivers in the Linux kernel, our goal is to
reuse them as-is to support this second use-case.

Therefore, this patch series introduces a PCI driver that binds on the
LAN996x PCI VID/PID, and when probed, instantiates all devices that are
accessible through the PCI BAR. As the list and characteristics of such
devices are non-discoverable, this PCI driver loads a Device Tree
overlay that allows to teach the kernel about which devices are
available, and allows to probe the relevant drivers in kernel, re-using
all existing drivers with no change.

This patch series for now adds a Device Tree overlay that describes an
initial subset of the devices available over PCI in the LAN996x, and
follow-up patch series will add support for more once this initial
support has landed.

In order to add this PCI driver, a number of preparation changes are
needed:
 - Patches 1 and 2 (new in this version) allow to remove the syscon
   device usage from the reset driver (only the reset driver use the
   syscon device)

 - Patches 3 and 4 introduce the LAN996x PCI driver itself, together
   with its DT overlay and the related MAINTAINER entry.

 - Patches 5 to 7 allow the reset driver used for the LAN996x to be
   built as a module. Indeed, in the case where Linux runs on the ARM
   cores, it is common to have the reset driver built-in. However, when
   the LAN996x is used as a PCI device, it makes sense that all its
   drivers can be loaded as modules.

Compare to the previous iteration:
  https://lore.kernel.org/lkml/20240808154658.247873-1-herve.codina@bootlin.com/
this v6 series mainly:
  - Remove patches adding support for refcounting and device removal in
    syscon.
  - Remove the syscon usage from the reset controller (the only syscon
    user).
  - Remove a patch already applied.
  - Add a 'Acked-by'
  - Rebase on top of v6.12-rc1

Best regards,
Hervé

Changes v5 -> v6
  - Patch 1
    New patch in v6 relatead to removing syscon usage.

  - Patch 2
    New patch in v6 related to removing syscon usage.

  - Patch 3 (patch 1 in v5)
    Add 'Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>'
    Remove syscon node from the overlay.

  - Patch 4, 5, 6 and 7 (patches 2, 4, 5 and 8 in v5)
    No changes

  Patches removed in v6
    - Patch 3 in v5
      Rejected

    - Patch 6 in v5
      No more applicable

    - Patch 7 in v5
      Already applied

Changes v4 -> v5
  - Patch 1
    Add missing include files and keep pci_ids.h.
    Remove IRQ_RETVAL() usage.
    Use __free().
    Remove the pdev->irq check.
    Avoid local variables in devm_pci_dev_remove_intr_ctrl() and
    lan966x_pci_load_overlay().
    Use dev_err_probe().
    Sort header includes in alphabetical order in dtbs file.

  - Patch 3
    Fix a typo in commit log.
    Simplify modification done in device_node_get_syscon().
    Use devm_add_action_or_reset().

  - Patches 4, 5, 6 and 8
    Add 'Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>'

Changes v3 -> v4
  - Patch 1 and 2 (v3 patch 6 and 7)
    Move the driver from drivers/mfd to drivers/misc.

  - Patch 4 and 5 (v3 patch 2)
    Rework reset driver dependencies and module building support.
    Split v3 patch into two distinct patches:
      - patch 4, as suggested by Geert, add a dependency on the
        LAN966x PCI device
      - patch 5, allows to build the reset controller driver as a module

  - Other patches
    Except reordering, no changes

Changes v2 -> v3
  - Patches 1 and 5
    No changes

  - Patch 6 (v2 patch 18)
    Add a blank line in the commit log to split paragraphs
    Remove unneeded header file inclusion
    Use IRQ_RETVAL()
    Remove blank line
    Use dev_of_node()
    Use pci_{set,get}_drvdata()
    Remove unneeded pci_clear_master() call
    Move { 0, } to { }
    Remove the unneeded pci_dev member from the lan966x_pci structure
    Use PCI_VENDOR_ID_EFAR instead of the hardcoded 0x1055 PCI Vendor ID
    Add a comment related to the of_node check.

  - Patch 7 (v2 patch 19)
    No changes

  Patches removed in v3
    - Patches 6 and 7
      Extracted and sent separately
      https://lore.kernel.org/lkml/20240620120126.412323-1-herve.codina@bootlin.com/

    - Patches 9
      Already applied

    - Patches 8, 10 to 12
      Extracted, reworked and sent separately
      https://lore.kernel.org/lkml/20240614173232.1184015-1-herve.codina@bootlin.com/

    - Patches 13 to 14
      Already applied

Changes v1 -> v2
  - Patch 1
    Fix a typo in syscon.h (s/intline/inline/)

  - Patches 2..5
    No changes

  - Patch 6
    Improve the reset property description

  - Patch 7
    Fix a wrong reverse x-mass tree declaration

  - Patch 8 removed (sent alone to net)
    https://lore.kernel.org/lkml/20240513111853.58668-1-herve.codina@bootlin.com/

  - Patch 8 (v1 patch 9)
    Add 'Reviewed-by: Rob Herring (Arm) <robh@kernel.org>'

  - Patch 9 (v1 patch 10)
    Rephrase and ident parameters descriptions

  - Patch 10 (v1 patch 11)
    No changes

  - Patch 11 (v1 patch 12)
    Fix a missing ret value assignment before a goto in .probe()
    Limit lines to 80 columns
    Use indices in register offset definitions

  - Patch 13 and 14 (new patches in v2)
    Add new test cases for existing of_changeset_add_prop_*()

  - Patch 15 (v1 patch 14)
    No changes

  - Patch 16 (new patches in v2)
    Add tests for of_changeset_add_prop_bool()

  - Patch 17 (v1 patch 15)
    Update commit subject
    Rewrap a paragraph in commit log

  - Patch 18 (v1 patch 16)
    Use PCI_IRQ_INTX instead of PCI_IRQ_LEGACY

  - Patch 19 (v1 patch 17)
    No changes

Clément Léger (2):
  reset: mchp: sparx5: Allow building as a module
  reset: mchp: sparx5: set the dev member of the reset controller

Herve Codina (5):
  dt-bindings: reset: microchip,rst: Allow to replace cpu-syscon by an
    additional reg item
  reset: mchp: sparx5: Use the second reg item when cpu-syscon is not
    present
  misc: Add support for LAN966x PCI device
  MAINTAINERS: Add the Microchip LAN966x PCI driver entry
  reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency

 .../bindings/reset/microchip,rst.yaml         |  35 ++-
 MAINTAINERS                                   |   6 +
 drivers/misc/Kconfig                          |  24 ++
 drivers/misc/Makefile                         |   3 +
 drivers/misc/lan966x_pci.c                    | 215 ++++++++++++++++++
 drivers/misc/lan966x_pci.dtso                 | 161 +++++++++++++
 drivers/pci/quirks.c                          |   1 +
 drivers/reset/Kconfig                         |   4 +-
 drivers/reset/reset-microchip-sparx5.c        |  19 +-
 9 files changed, 464 insertions(+), 4 deletions(-)
 create mode 100644 drivers/misc/lan966x_pci.c
 create mode 100644 drivers/misc/lan966x_pci.dtso

-- 
2.46.1

[PATCH v6 1/7] dt-bindings: reset: microchip,rst: Allow to replace cpu-syscon by an additional reg item

[Herve Codina]

In the LAN966x PCI device use case, syscon cannot be used as syscon
devices do not support removal [1]. A syscon device is a core "system"
device and not a device available in some addon boards and so, it is not
supposed to be removed.

In order to remove the syscon device usage, allow the reset controller
to have a direct access to the address range it needs to use.

Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../bindings/reset/microchip,rst.yaml         | 35 ++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/reset/microchip,rst.yaml b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
index f2da0693b05a..5164239a372c 100644
--- a/Documentation/devicetree/bindings/reset/microchip,rst.yaml
+++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
@@ -25,12 +25,16 @@ properties:
       - microchip,lan966x-switch-reset
 
   reg:
+    minItems: 1
     items:
       - description: global control block registers
+      - description: cpu system block registers
 
   reg-names:
+    minItems: 1
     items:
       - const: gcb
+      - const: cpu
 
   "#reset-cells":
     const: 1
@@ -39,12 +43,29 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: syscon used to access CPU reset
 
+allOf:
+  # Allow to use the second reg item instead of cpu-syscon
+  - if:
+      required:
+        - cpu-syscon
+    then:
+      properties:
+        reg:
+          maxItems: 1
+        reg-names:
+          maxItems: 1
+    else:
+      properties:
+        reg:
+          minItems: 2
+        reg-names:
+          minItems: 2
+
 required:
   - compatible
   - reg
   - reg-names
   - "#reset-cells"
-  - cpu-syscon
 
 additionalProperties: false
 
@@ -57,3 +78,15 @@ examples:
         #reset-cells = <1>;
         cpu-syscon = <&cpu_ctrl>;
     };
+
+    /*
+     * The following construction can be used if the cpu-syscon device is not
+     * present. This is the case when the LAN966x is used as a PCI device.
+     */
+    reset-controller@22010008 {
+        compatible = "microchip,lan966x-switch-reset";
+        reg = <0xe200400c 0x4>,
+              <0xe00c0000 0xa8>;
+        reg-names = "gcb", "cpu";
+        #reset-cells = <1>;
+    };
-- 
2.46.1

[PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Herve Codina]

In the LAN966x PCI device use case, syscon cannot be used as syscon
devices do not support removal [1]. A syscon device is a core "system"
device and not a device available in some addon boards and so, it is not
supposed to be removed.

In order to remove the syscon usage, use a local mapping of a reg
address range when cpu-syscon is not present.

Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/reset/reset-microchip-sparx5.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 636e85c388b0..1c095fa41d69 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -114,8 +114,22 @@ static int mchp_sparx5_reset_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	err = mchp_sparx5_map_syscon(pdev, "cpu-syscon", &ctx->cpu_ctrl);
-	if (err)
+	switch (err) {
+	case 0:
+		break;
+	case -ENODEV:
+		/*
+		 * The cpu-syscon device is not available.
+		 * Fall back with IO mapping (i.e. mapping from reg property).
+		 */
+		err = mchp_sparx5_map_io(pdev, 1, &ctx->cpu_ctrl);
+		if (err)
+			return err;
+		break;
+	default:
 		return err;
+	}
+
 	err = mchp_sparx5_map_io(pdev, 0, &ctx->gcb_ctrl);
 	if (err)
 		return err;
-- 
2.46.1

[PATCH v6 3/7] misc: Add support for LAN966x PCI device

[Herve Codina]

Add a PCI driver that handles the LAN966x PCI device using a device-tree
overlay. This overlay is applied to the PCI device DT node and allows to
describe components that are present in the device.

The memory from the device-tree is remapped to the BAR memory thanks to
"ranges" properties computed at runtime by the PCI core during the PCI
enumeration.

The PCI device itself acts as an interrupt controller and is used as the
parent of the internal LAN966x interrupt controller to route the
interrupts to the assigned PCI INTx interrupt.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/misc/Kconfig          |  24 ++++
 drivers/misc/Makefile         |   3 +
 drivers/misc/lan966x_pci.c    | 215 ++++++++++++++++++++++++++++++++++
 drivers/misc/lan966x_pci.dtso | 161 +++++++++++++++++++++++++
 drivers/pci/quirks.c          |   1 +
 5 files changed, 404 insertions(+)
 create mode 100644 drivers/misc/lan966x_pci.c
 create mode 100644 drivers/misc/lan966x_pci.dtso

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3fe7e2a9bd29..8e5b06ac9b6f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -610,6 +610,30 @@ config MARVELL_CN10K_DPI
 	  To compile this driver as a module, choose M here: the module
 	  will be called mrvl_cn10k_dpi.
 
+config MCHP_LAN966X_PCI
+	tristate "Microchip LAN966x PCIe Support"
+	depends on PCI
+	select OF
+	select OF_OVERLAY
+	select IRQ_DOMAIN
+	help
+	  This enables the support for the LAN966x PCIe device.
+	  This is used to drive the LAN966x PCIe device from the host system
+	  to which it is connected.
+
+	  This driver uses an overlay to load other drivers to support for
+	  LAN966x internal components.
+	  Even if this driver does not depend on these other drivers, in order
+	  to have a fully functional board, the following drivers are needed:
+	    - fixed-clock (COMMON_CLK)
+	    - lan966x-oic (LAN966X_OIC)
+	    - lan966x-cpu-syscon (MFD_SYSCON)
+	    - lan966x-switch-reset (RESET_MCHP_SPARX5)
+	    - lan966x-pinctrl (PINCTRL_OCELOT)
+	    - lan966x-serdes (PHY_LAN966X_SERDES)
+	    - lan966x-miim (MDIO_MSCC_MIIM)
+	    - lan966x-switch (LAN966X_SWITCH)
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a9f94525e181..885b22989580 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -71,4 +71,7 @@ obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
 obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
 obj-$(CONFIG_NSM)		+= nsm.o
 obj-$(CONFIG_MARVELL_CN10K_DPI)	+= mrvl_cn10k_dpi.o
+lan966x-pci-objs		:= lan966x_pci.o
+lan966x-pci-objs		+= lan966x_pci.dtbo.o
+obj-$(CONFIG_MCHP_LAN966X_PCI)	+= lan966x-pci.o
 obj-y				+= keba/
diff --git a/drivers/misc/lan966x_pci.c b/drivers/misc/lan966x_pci.c
new file mode 100644
index 000000000000..9c79b58137e5
--- /dev/null
+++ b/drivers/misc/lan966x_pci.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip LAN966x PCI driver
+ *
+ * Copyright (c) 2024 Microchip Technology Inc. and its subsidiaries.
+ *
+ * Authors:
+ *	Clément Léger <clement.leger@bootlin.com>
+ *	Hervé Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/device.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+#include <linux/slab.h>
+
+/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */
+extern char __dtbo_lan966x_pci_begin[];
+extern char __dtbo_lan966x_pci_end[];
+
+struct pci_dev_intr_ctrl {
+	struct pci_dev *pci_dev;
+	struct irq_domain *irq_domain;
+	int irq;
+};
+
+static int pci_dev_irq_domain_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t hw)
+{
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+	return 0;
+}
+
+static const struct irq_domain_ops pci_dev_irq_domain_ops = {
+	.map = pci_dev_irq_domain_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static irqreturn_t pci_dev_irq_handler(int irq, void *data)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl = data;
+	int ret;
+
+	ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
+	return ret ? IRQ_NONE : IRQ_HANDLED;
+}
+
+static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl __free(kfree) = NULL;
+	struct fwnode_handle *fwnode;
+	int ret;
+
+	fwnode = dev_fwnode(&pdev->dev);
+	if (!fwnode)
+		return ERR_PTR(-ENODEV);
+
+	intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);
+	if (!intr_ctrl)
+		return ERR_PTR(-ENOMEM);
+
+	intr_ctrl->pci_dev = pdev;
+
+	intr_ctrl->irq_domain = irq_domain_create_linear(fwnode, 1, &pci_dev_irq_domain_ops,
+							 intr_ctrl);
+	if (!intr_ctrl->irq_domain) {
+		pci_err(pdev, "Failed to create irqdomain\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
+	if (ret < 0) {
+		pci_err(pdev, "Unable alloc irq vector (%d)\n", ret);
+		goto err_remove_domain;
+	}
+	intr_ctrl->irq = pci_irq_vector(pdev, 0);
+	ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
+			  pci_name(pdev), intr_ctrl);
+	if (ret) {
+		pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
+		goto err_free_irq_vector;
+	}
+
+	return_ptr(intr_ctrl);
+
+err_free_irq_vector:
+	pci_free_irq_vectors(pdev);
+err_remove_domain:
+	irq_domain_remove(intr_ctrl->irq_domain);
+	return ERR_PTR(ret);
+}
+
+static void pci_dev_remove_intr_ctrl(struct pci_dev_intr_ctrl *intr_ctrl)
+{
+	free_irq(intr_ctrl->irq, intr_ctrl);
+	pci_free_irq_vectors(intr_ctrl->pci_dev);
+	irq_dispose_mapping(irq_find_mapping(intr_ctrl->irq_domain, 0));
+	irq_domain_remove(intr_ctrl->irq_domain);
+	kfree(intr_ctrl);
+}
+
+static void devm_pci_dev_remove_intr_ctrl(void *intr_ctrl)
+{
+	pci_dev_remove_intr_ctrl(intr_ctrl);
+}
+
+static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl;
+
+	intr_ctrl = pci_dev_create_intr_ctrl(pdev);
+	if (IS_ERR(intr_ctrl))
+		return PTR_ERR(intr_ctrl);
+
+	return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl);
+}
+
+struct lan966x_pci {
+	struct device *dev;
+	int ovcs_id;
+};
+
+static int lan966x_pci_load_overlay(struct lan966x_pci *data)
+{
+	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
+	void *dtbo_start = __dtbo_lan966x_pci_begin;
+
+	return of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, dev_of_node(data->dev));
+}
+
+static void lan966x_pci_unload_overlay(struct lan966x_pci *data)
+{
+	of_overlay_remove(&data->ovcs_id);
+}
+
+static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data;
+	int ret;
+
+	/*
+	 * On ACPI system, fwnode can point to the ACPI node.
+	 * This driver needs an of_node to be used as the device-tree overlay
+	 * target. This of_node should be set by the PCI core if it succeeds in
+	 * creating it (CONFIG_PCI_DYNAMIC_OF_NODES feature).
+	 * Check here for the validity of this of_node.
+	 */
+	if (!dev_of_node(dev))
+		return dev_err_probe(dev, -EINVAL, "Missing of_node for device\n");
+
+	/* Need to be done before devm_pci_dev_create_intr_ctrl.
+	 * It allocates an IRQ and so pdev->irq is updated.
+	 */
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	ret = devm_pci_dev_create_intr_ctrl(pdev);
+	if (ret)
+		return ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	pci_set_drvdata(pdev, data);
+	data->dev = dev;
+
+	ret = lan966x_pci_load_overlay(data);
+	if (ret)
+		return ret;
+
+	pci_set_master(pdev);
+
+	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
+	if (ret)
+		goto err_unload_overlay;
+
+	return 0;
+
+err_unload_overlay:
+	lan966x_pci_unload_overlay(data);
+	return ret;
+}
+
+static void lan966x_pci_remove(struct pci_dev *pdev)
+{
+	struct lan966x_pci *data = pci_get_drvdata(pdev);
+
+	of_platform_depopulate(data->dev);
+
+	lan966x_pci_unload_overlay(data);
+}
+
+static struct pci_device_id lan966x_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_EFAR, 0x9660) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, lan966x_pci_ids);
+
+static struct pci_driver lan966x_pci_driver = {
+	.name = "mchp_lan966x_pci",
+	.id_table = lan966x_pci_ids,
+	.probe = lan966x_pci_probe,
+	.remove = lan966x_pci_remove,
+};
+module_pci_driver(lan966x_pci_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("Microchip LAN966x PCI driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
new file mode 100644
index 000000000000..db57b3a43fe9
--- /dev/null
+++ b/drivers/misc/lan966x_pci.dtso
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Microchip UNG
+ */
+
+#include <dt-bindings/clock/microchip,lan966x.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/mfd/atmel-flexcom.h>
+#include <dt-bindings/phy/phy-lan966x-serdes.h>
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	fragment@0 {
+		target-path="";
+		__overlay__ {
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			pci-ep-bus@0 {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				/*
+				 * map @0xe2000000 (32MB) to BAR0 (CPU)
+				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
+				 */
+				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
+				          0xe0000000 0x01 0x00 0x00 0x1000000>;
+
+				oic: oic@e00c0120 {
+					compatible = "microchip,lan966x-oic";
+					#interrupt-cells = <2>;
+					interrupt-controller;
+					interrupts = <0>; /* PCI INTx assigned interrupt */
+					reg = <0xe00c0120 0x190>;
+				};
+
+				cpu_clk: cpu_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <600000000>;  // CPU clock = 600MHz
+				};
+
+				ddr_clk: ddr_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <30000000>;  // Fabric clock = 30MHz
+				};
+
+				sys_clk: sys_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <15625000>;  // System clock = 15.625MHz
+				};
+
+				reset: reset@e200400c {
+					compatible = "microchip,lan966x-switch-reset";
+					reg = <0xe200400c 0x4>, <0xe00c0000 0xa8>;
+					reg-names = "gcb","cpu";
+					#reset-cells = <1>;
+				};
+
+				gpio: pinctrl@e2004064 {
+					compatible = "microchip,lan966x-pinctrl";
+					reg = <0xe2004064 0xb4>,
+					      <0xe2010024 0x138>;
+					resets = <&reset 0>;
+					reset-names = "switch";
+					gpio-controller;
+					#gpio-cells = <2>;
+					gpio-ranges = <&gpio 0 0 78>;
+					interrupt-parent = <&oic>;
+					interrupt-controller;
+					interrupts = <17 IRQ_TYPE_LEVEL_HIGH>;
+					#interrupt-cells = <2>;
+
+					tod_pins: tod_pins {
+						pins = "GPIO_36";
+						function = "ptpsync_1";
+					};
+
+					fc0_a_pins: fcb4-i2c-pins {
+						/* RXD, TXD */
+						pins = "GPIO_9", "GPIO_10";
+						function = "fc0_a";
+					};
+
+				};
+
+				serdes: serdes@e202c000 {
+					compatible = "microchip,lan966x-serdes";
+					reg = <0xe202c000 0x9c>,
+					      <0xe2004010 0x4>;
+					#phy-cells = <2>;
+				};
+
+				mdio1: mdio@e200413c {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					compatible = "microchip,lan966x-miim";
+					reg = <0xe200413c 0x24>,
+					      <0xe2010020 0x4>;
+
+					resets = <&reset 0>;
+					reset-names = "switch";
+
+					lan966x_phy0: ethernet-lan966x_phy@1 {
+						reg = <1>;
+					};
+
+					lan966x_phy1: ethernet-lan966x_phy@2 {
+						reg = <2>;
+					};
+				};
+
+				switch: switch@e0000000 {
+					compatible = "microchip,lan966x-switch";
+					reg = <0xe0000000 0x0100000>,
+					      <0xe2000000 0x0800000>;
+					reg-names = "cpu", "gcb";
+
+					interrupt-parent = <&oic>;
+					interrupts = <12 IRQ_TYPE_LEVEL_HIGH>,
+						     <9 IRQ_TYPE_LEVEL_HIGH>;
+					interrupt-names = "xtr", "ana";
+
+					resets = <&reset 0>;
+					reset-names = "switch";
+
+					pinctrl-names = "default";
+					pinctrl-0 = <&tod_pins>;
+
+					ethernet-ports {
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						port0: port@0 {
+							phy-handle = <&lan966x_phy0>;
+
+							reg = <0>;
+							phy-mode = "gmii";
+							phys = <&serdes 0 CU(0)>;
+						};
+
+						port1: port@1 {
+							phy-handle = <&lan966x_phy1>;
+
+							reg = <1>;
+							phy-mode = "gmii";
+							phys = <&serdes 1 CU(1)>;
+						};
+					};
+				};
+			};
+		};
+	};
+};
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index dccb60c1d9cc..41dec625ed7b 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6266,6 +6266,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa76e, dpc_log_size);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_EFAR, 0x9660, of_pci_make_dev_node);
 
 /*
  * Devices known to require a longer delay before first config space access
-- 
2.46.1

[PATCH v6 4/7] MAINTAINERS: Add the Microchip LAN966x PCI driver entry

[Herve Codina]

After contributing the driver, add myself as the maintainer for the
Microchip LAN966x PCI driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c27f3190737f..79125b6f7814 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15184,6 +15184,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/interrupt-controller/microchip,lan966x-oic.yaml
 F:	drivers/irqchip/irq-lan966x-oic.c
 
+MICROCHIP LAN966X PCI DRIVER
+M:	Herve Codina <herve.codina@bootlin.com>
+S:	Maintained
+F:	drivers/misc/lan966x_pci.c
+F:	drivers/misc/lan966x_pci.dtso
+
 MICROCHIP LCDFB DRIVER
 M:	Nicolas Ferre <nicolas.ferre@microchip.com>
 L:	linux-fbdev@vger.kernel.org
-- 
2.46.1

[PATCH v6 5/7] reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency

[Herve Codina]

The sparx5 reset controller depends on the SPARX5 architecture or the
LAN966x SoC.

This reset controller can be used by the LAN966x PCI device and so it
needs to be available when the LAN966x PCI device is enabled.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
---
 drivers/reset/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 5484a65f66b9..86a5504950cb 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -147,7 +147,7 @@ config RESET_LPC18XX
 
 config RESET_MCHP_SPARX5
 	bool "Microchip Sparx5 reset driver"
-	depends on ARCH_SPARX5 || SOC_LAN966 || COMPILE_TEST
+	depends on ARCH_SPARX5 || SOC_LAN966 || MCHP_LAN966X_PCI || COMPILE_TEST
 	default y if SPARX5_SWITCH
 	select MFD_SYSCON
 	help
-- 
2.46.1

[PATCH v6 6/7] reset: mchp: sparx5: Allow building as a module

[Herve Codina]

From: Clément Léger <clement.leger@bootlin.com>

This reset controller can be used by the LAN966x PCI device.

The LAN966x PCI device driver can be built as a module and this reset
controller driver has no reason to be a builtin driver in that case.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
---
 drivers/reset/Kconfig                  | 2 +-
 drivers/reset/reset-microchip-sparx5.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 86a5504950cb..93cddbe8609b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -146,7 +146,7 @@ config RESET_LPC18XX
 	  This enables the reset controller driver for NXP LPC18xx/43xx SoCs.
 
 config RESET_MCHP_SPARX5
-	bool "Microchip Sparx5 reset driver"
+	tristate "Microchip Sparx5 reset driver"
 	depends on ARCH_SPARX5 || SOC_LAN966 || MCHP_LAN966X_PCI || COMPILE_TEST
 	default y if SPARX5_SWITCH
 	select MFD_SYSCON
diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 1c095fa41d69..8b931af67383 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -172,6 +172,7 @@ static const struct of_device_id mchp_sparx5_reset_of_match[] = {
 	},
 	{ }
 };
+MODULE_DEVICE_TABLE(of, mchp_sparx5_reset_of_match);
 
 static struct platform_driver mchp_sparx5_reset_driver = {
 	.probe = mchp_sparx5_reset_probe,
@@ -194,3 +195,4 @@ postcore_initcall(mchp_sparx5_reset_init);
 
 MODULE_DESCRIPTION("Microchip Sparx5 switch reset driver");
 MODULE_AUTHOR("Steen Hegelund <steen.hegelund@microchip.com>");
+MODULE_LICENSE("GPL");
-- 
2.46.1

[PATCH v6 7/7] reset: mchp: sparx5: set the dev member of the reset controller

[Herve Codina]

From: Clément Léger <clement.leger@bootlin.com>

In order to guarantee the device will not be deleted by the reset
controller consumer, set the dev member of the reset controller.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
---
 drivers/reset/reset-microchip-sparx5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 8b931af67383..6bcc3669b71a 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -135,6 +135,7 @@ static int mchp_sparx5_reset_probe(struct platform_device *pdev)
 		return err;
 
 	ctx->rcdev.owner = THIS_MODULE;
+	ctx->rcdev.dev = &pdev->dev;
 	ctx->rcdev.nr_resets = 1;
 	ctx->rcdev.ops = &sparx5_reset_ops;
 	ctx->rcdev.of_node = dn;
-- 
2.46.1

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Steen Hegelund]

Hi Herve,

On Mon, 2024-09-30 at 14:15 +0200, Herve Codina wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> In the LAN966x PCI device use case, syscon cannot be used as syscon
> devices do not support removal [1]. A syscon device is a core
> "system"
> device and not a device available in some addon boards and so, it is
> not
> supposed to be removed.
> 
> In order to remove the syscon usage, use a local mapping of a reg
> address range when cpu-syscon is not present.
> 
> Link:
> https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/reset/reset-microchip-sparx5.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/reset-microchip-sparx5.c
> b/drivers/reset/reset-microchip-sparx5.c
> index 636e85c388b0..1c095fa41d69 100644
> --- a/drivers/reset/reset-microchip-sparx5.c
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -114,8 +114,22 @@ static int mchp_sparx5_reset_probe(struct
> platform_device *pdev)
>                 return -ENOMEM;
> 
>         err = mchp_sparx5_map_syscon(pdev, "cpu-syscon", &ctx-
> >cpu_ctrl);
> -       if (err)
> +       switch (err) {
> +       case 0:
> +               break;
> +       case -ENODEV:
> +               /*
> +                * The cpu-syscon device is not available.
> +                * Fall back with IO mapping (i.e. mapping from reg
> property).
> +                */
> +               err = mchp_sparx5_map_io(pdev, 1, &ctx->cpu_ctrl);
> +               if (err)
> +                       return err;
> +               break;
> +       default:
>                 return err;
> +       }
> +
>         err = mchp_sparx5_map_io(pdev, 0, &ctx->gcb_ctrl);
>         if (err)
>                 return err;
> --
> 2.46.1
> 

LGTM

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>

BR
Steen

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Geert Uytterhoeven]

Hi Hervé,

On Mon, Sep 30, 2024 at 2:16 PM Herve Codina <herve.codina@bootlin.com> wrote:
> In the LAN966x PCI device use case, syscon cannot be used as syscon
> devices do not support removal [1]. A syscon device is a core "system"
> device and not a device available in some addon boards and so, it is not
> supposed to be removed.
>
> In order to remove the syscon usage, use a local mapping of a reg
> address range when cpu-syscon is not present.
>
> Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Thanks for your patch!

> --- a/drivers/reset/reset-microchip-sparx5.c
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -114,8 +114,22 @@ static int mchp_sparx5_reset_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         err = mchp_sparx5_map_syscon(pdev, "cpu-syscon", &ctx->cpu_ctrl);
> -       if (err)
> +       switch (err) {
> +       case 0:
> +               break;
> +       case -ENODEV:
> +               /*
> +                * The cpu-syscon device is not available.
> +                * Fall back with IO mapping (i.e. mapping from reg property).
> +                */
> +               err = mchp_sparx5_map_io(pdev, 1, &ctx->cpu_ctrl);
> +               if (err)
> +                       return err;
> +               break;
> +       default:
>                 return err;
> +       }
> +

This can be shortened to:

    if (err == -ENODEV) {
            /*
             * The cpu-syscon device is not available.
             * Fall back with IO mapping (i.e. mapping from reg property).
             */
            err = mchp_sparx5_map_io(pdev, 1, &ctx->cpu_ctrl);
    }
    if (err)
            return err;

>         err = mchp_sparx5_map_io(pdev, 0, &ctx->gcb_ctrl);
>         if (err)
>                 return err;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Arnd Bergmann]

On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:
> In the LAN966x PCI device use case, syscon cannot be used as syscon
> devices do not support removal [1]. A syscon device is a core "system"
> device and not a device available in some addon boards and so, it is not
> supposed to be removed.
>
> In order to remove the syscon usage, use a local mapping of a reg
> address range when cpu-syscon is not present.
>
> Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

>>  	err = mchp_sparx5_map_syscon(pdev, "cpu-syscon", &ctx->cpu_ctrl);
> -	if (err)
> +	switch (err) {
> +	case 0:
> +		break;
> +	case -ENODEV:

I was expecting a patch that would read the phandle and map the
syscon node to keep the behavior unchanged, but I guess this one
works as well.

The downside of your approach is that it requires an different
DT binding, which only works as long as there are no other
users of the syscon registers.

     Arnd

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Herve Codina]

On Mon, 30 Sep 2024 13:57:01 +0000
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:
> > In the LAN966x PCI device use case, syscon cannot be used as syscon
> > devices do not support removal [1]. A syscon device is a core "system"
> > device and not a device available in some addon boards and so, it is not
> > supposed to be removed.
> >
> > In order to remove the syscon usage, use a local mapping of a reg
> > address range when cpu-syscon is not present.
> >
> > Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---  
> 
> >>  	err = mchp_sparx5_map_syscon(pdev, "cpu-syscon", &ctx->cpu_ctrl);  
> > -	if (err)
> > +	switch (err) {
> > +	case 0:
> > +		break;
> > +	case -ENODEV:  
> 
> I was expecting a patch that would read the phandle and map the
> syscon node to keep the behavior unchanged, but I guess this one
> works as well.
> 
> The downside of your approach is that it requires an different
> DT binding, which only works as long as there are no other
> users of the syscon registers.

Yes, I knwow but keeping the binding with the syscon device (i.e. compatible
= "...", "syscon";) leads to confusion.
Indeed, the syscon API cannot be used because using this API leads issues
when the syscon device is removed.
That means the you have a "syscon" node (compatible = "syscon") but we cannot
use the syscon API (include/linux/mfd/syscon.h) with this node.

Also, in order to share resources between several consumers of the "syscon"
registers, we need exactly what is done in syscon. I mean we need to map
resources only once, provide this resource throught a regmap an share this
regmap between the consumers. Indeed a lock needs to be shared in order to
protect against registers RMW accesses done by several consumers.
In other word, we need to copy/paste syscon code with support for removal
implemented (feature needed in the LAN966x PCI device use case).

So, I found really simpler and less confusing to fully discard the syscon node
and handle registers directly in the only one consumer.

With all of these, do you thing my approach can be acceptable ?

Best regards,
Hervé

Re: [PATCH v6 1/7] dt-bindings: reset: microchip,rst: Allow to replace cpu-syscon by an additional reg item

[Krzysztof Kozlowski]

On Mon, Sep 30, 2024 at 02:15:41PM +0200, Herve Codina wrote:
> In the LAN966x PCI device use case, syscon cannot be used as syscon
> devices do not support removal [1]. A syscon device is a core "system"
> device and not a device available in some addon boards and so, it is not
> supposed to be removed.

That's not accurate. syscon is our own, Linux term which means also
anything exposing set of registers.

If you need to unload syscons, implement it. syscon is the same resource
as all others so should be handled same way.

> 
> In order to remove the syscon device usage, allow the reset controller
> to have a direct access to the address range it needs to use.

So you map same address twice? That's not good, because you have no
locking over concurrent register accesses.

> 
> Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  .../bindings/reset/microchip,rst.yaml         | 35 ++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/reset/microchip,rst.yaml b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> index f2da0693b05a..5164239a372c 100644
> --- a/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> +++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> @@ -25,12 +25,16 @@ properties:
>        - microchip,lan966x-switch-reset
>  
>    reg:
> +    minItems: 1
>      items:
>        - description: global control block registers
> +      - description: cpu system block registers
>  
>    reg-names:
> +    minItems: 1
>      items:
>        - const: gcb
> +      - const: cpu
>  
>    "#reset-cells":
>      const: 1
> @@ -39,12 +43,29 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description: syscon used to access CPU reset
>  
> +allOf:
> +  # Allow to use the second reg item instead of cpu-syscon
> +  - if:
> +      required:
> +        - cpu-syscon
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +        reg-names:
> +          maxItems: 1
> +    else:
> +      properties:
> +        reg:
> +          minItems: 2
> +        reg-names:
> +          minItems: 2
> +
>  required:
>    - compatible
>    - reg
>    - reg-names
>    - "#reset-cells"
> -  - cpu-syscon
>  
>  additionalProperties: false
>  
> @@ -57,3 +78,15 @@ examples:
>          #reset-cells = <1>;
>          cpu-syscon = <&cpu_ctrl>;
>      };
> +
> +    /*
> +     * The following construction can be used if the cpu-syscon device is not
> +     * present. This is the case when the LAN966x is used as a PCI device.
> +     */
> +    reset-controller@22010008 {
> +        compatible = "microchip,lan966x-switch-reset";
> +        reg = <0xe200400c 0x4>,
> +              <0xe00c0000 0xa8>;

If you have here CPU address, then syscon device is present...

Best regards,
Krzysztof

Re: [PATCH v6 1/7] dt-bindings: reset: microchip,rst: Allow to replace cpu-syscon by an additional reg item

[Herve Codina]

Hi Krystoff,

On Tue, 1 Oct 2024 08:43:23 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On Mon, Sep 30, 2024 at 02:15:41PM +0200, Herve Codina wrote:
> > In the LAN966x PCI device use case, syscon cannot be used as syscon
> > devices do not support removal [1]. A syscon device is a core "system"
> > device and not a device available in some addon boards and so, it is not
> > supposed to be removed.  
> 
> That's not accurate. syscon is our own, Linux term which means also
> anything exposing set of registers.
> 
> If you need to unload syscons, implement it. syscon is the same resource
> as all others so should be handled same way.
> 
> > 
> > In order to remove the syscon device usage, allow the reset controller
> > to have a direct access to the address range it needs to use.  
> 
> So you map same address twice? That's not good, because you have no
> locking over concurrent register accesses.
> 

I will remove this patch and keep using the syscon node in the next
iteration.

Best regards,
Hervé

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Herve Codina]

Hi Arnd,

On Mon, 30 Sep 2024 16:26:16 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> On Mon, 30 Sep 2024 13:57:01 +0000
> "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> > On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:  
> > > In the LAN966x PCI device use case, syscon cannot be used as syscon
> > > devices do not support removal [1]. A syscon device is a core "system"
> > > device and not a device available in some addon boards and so, it is not
> > > supposed to be removed.
> > >
> > > In order to remove the syscon usage, use a local mapping of a reg
> > > address range when cpu-syscon is not present.
> > >
> > > Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---    
> >   
> > >>  	err = mchp_sparx5_map_syscon(pdev, "cpu-syscon", &ctx->cpu_ctrl);    
> > > -	if (err)
> > > +	switch (err) {
> > > +	case 0:
> > > +		break;
> > > +	case -ENODEV:    
> > 
> > I was expecting a patch that would read the phandle and map the
> > syscon node to keep the behavior unchanged, but I guess this one
> > works as well.
> > 
> > The downside of your approach is that it requires an different
> > DT binding, which only works as long as there are no other
> > users of the syscon registers.  
> 
> Yes, I knwow but keeping the binding with the syscon device (i.e. compatible
> = "...", "syscon";) leads to confusion.
> Indeed, the syscon API cannot be used because using this API leads issues
> when the syscon device is removed.
> That means the you have a "syscon" node (compatible = "syscon") but we cannot
> use the syscon API (include/linux/mfd/syscon.h) with this node.
> 
> Also, in order to share resources between several consumers of the "syscon"
> registers, we need exactly what is done in syscon. I mean we need to map
> resources only once, provide this resource throught a regmap an share this
> regmap between the consumers. Indeed a lock needs to be shared in order to
> protect against registers RMW accesses done by several consumers.
> In other word, we need to copy/paste syscon code with support for removal
> implemented (feature needed in the LAN966x PCI device use case).
> 
> So, I found really simpler and less confusing to fully discard the syscon node
> and handle registers directly in the only one consumer.
> 
> With all of these, do you thing my approach can be acceptable ?
> 

Well, the related binding has been rejected.

In the next iteration, I will keep the syscon node and implement what you
suggested (i.e. read the phandle and map the syscon node).

This will look like this:
--- 8< ---
static const struct regmap_config mchp_lan966x_syscon_regmap_config = {
       .reg_bits = 32,
       .val_bits = 32,
       .reg_stride = 4,
};

static struct regmap *mchp_lan966x_syscon_to_regmap(struct device *dev,
       	                                           struct device_node *syscon_np)
{
       struct regmap_config regmap_config = mchp_lan966x_syscon_regmap_config;
       resource_size_t size;
       void __iomem *base;

       base = devm_of_iomap(dev, syscon_np, 0, &size);
       if (IS_ERR(base))
               return ERR_CAST(base);

       regmap_config.max_register = size - 4;

       return devm_regmap_init_mmio(dev, base, &regmap_config);
}
--- 8< ---

In mchp_sparx5_map_syscon(), I will call the syscon API or the local
function based on the device compatible string:
	--- 8< ---
	if (of_device_is_compatible(pdev->dev.of_node, "microchip,lan966x-switch-reset"))
		regmap = mchp_lan966x_syscon_to_regmap(&pdev->dev, syscon_np);
	else
		regmap = syscon_node_to_regmap(syscon_np);
	--- 8< ---

Is this kind of solution you were expecting?
If you have thought about something different, can you give me some pointers?

Best regards,
Hervé

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Arnd Bergmann]

On Tue, Oct 1, 2024, at 16:30, Herve Codina wrote:
> On Mon, 30 Sep 2024 16:26:16 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> --- 8< ---
>
> In mchp_sparx5_map_syscon(), I will call the syscon API or the local
> function based on the device compatible string:
> 	--- 8< ---
> 	if (of_device_is_compatible(pdev->dev.of_node, 
> "microchip,lan966x-switch-reset"))
> 		regmap = mchp_lan966x_syscon_to_regmap(&pdev->dev, syscon_np);
> 	else
> 		regmap = syscon_node_to_regmap(syscon_np);
> 	--- 8< ---
>
> Is this kind of solution you were expecting?
> If you have thought about something different, can you give me some pointers?

Hi Hervé,

The way I had imagined this was to not need an if() check
at all but unconditionally map the syscon registers in the
reset driver.

The most important part here is to have sensible bindings
that don't need to describe the difference between PCI
and SoC mode. This seems fine for the lan966x case, but
I'm not sure why you need to handle sparx5 differently here.
Do you expect the syscon to be shared with other drivers
on sparx5 but not lan966x?

I don't thinkt this bit matters too much and what you suggest
works fine, I just want to be sure I understand what you are
doing.

      Arnd

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Herve Codina]

Hi Arnd,

On Wed, 02 Oct 2024 09:29:35 +0000
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Tue, Oct 1, 2024, at 16:30, Herve Codina wrote:
> > On Mon, 30 Sep 2024 16:26:16 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> > --- 8< ---
> >
> > In mchp_sparx5_map_syscon(), I will call the syscon API or the local
> > function based on the device compatible string:
> > 	--- 8< ---
> > 	if (of_device_is_compatible(pdev->dev.of_node, 
> > "microchip,lan966x-switch-reset"))
> > 		regmap = mchp_lan966x_syscon_to_regmap(&pdev->dev, syscon_np);
> > 	else
> > 		regmap = syscon_node_to_regmap(syscon_np);
> > 	--- 8< ---
> >
> > Is this kind of solution you were expecting?
> > If you have thought about something different, can you give me some pointers?  
> 
> Hi Hervé,
> 
> The way I had imagined this was to not need an if() check
> at all but unconditionally map the syscon registers in the
> reset driver.
> 
> The most important part here is to have sensible bindings
> that don't need to describe the difference between PCI
> and SoC mode. This seems fine for the lan966x case, but
> I'm not sure why you need to handle sparx5 differently here.
> Do you expect the syscon to be shared with other drivers
> on sparx5 but not lan966x?

Thanks for this reply.

Exactly, on sparx5 syscon is shared...
$ git grep 'microchip,sparx5-cpu-syscon'
...
arch/arm64/boot/dts/microchip/sparx5.dtsi:                      compatible = "microchip,sparx5-cpu-syscon", "syscon",
drivers/mmc/host/sdhci-of-sparx5.c:     const char *syscon = "microchip,sparx5-cpu-syscon";
drivers/power/reset/ocelot-reset.c:     .syscon          = "microchip,sparx5-cpu-syscon",
drivers/spi/spi-dw-mmio.c:      const char *syscon_name = "microchip,sparx5-cpu-syscon";
$

> 
> I don't thinkt this bit matters too much and what you suggest
> works fine, I just want to be sure I understand what you are
> doing.
> 
>       Arnd

Best regards,
Hervé

Re: [PATCH v6 2/7] reset: mchp: sparx5: Use the second reg item when cpu-syscon is not present

[Arnd Bergmann]

On Wed, Oct 2, 2024, at 10:19, Herve Codina wrote:
> On Wed, 02 Oct 2024 09:29:35 +0000

> Thanks for this reply.
>
> Exactly, on sparx5 syscon is shared...
> $ git grep 'microchip,sparx5-cpu-syscon'
> ...
> arch/arm64/boot/dts/microchip/sparx5.dtsi:                      
> compatible = "microchip,sparx5-cpu-syscon", "syscon",
> drivers/mmc/host/sdhci-of-sparx5.c:     const char *syscon = 
> "microchip,sparx5-cpu-syscon";
> drivers/power/reset/ocelot-reset.c:     .syscon          = 
> "microchip,sparx5-cpu-syscon",
> drivers/spi/spi-dw-mmio.c:      const char *syscon_name = 
> "microchip,sparx5-cpu-syscon";
> $

Ok, got it. In that case, your suggestion looks fine.

       Arnd

Re: [PATCH v6 3/7] misc: Add support for LAN966x PCI device

[Arnd Bergmann]

On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:

> +			pci-ep-bus@0 {
> +				compatible = "simple-bus";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +
> +				/*
> +				 * map @0xe2000000 (32MB) to BAR0 (CPU)
> +				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
> +				 */
> +				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
> +				          0xe0000000 0x01 0x00 0x00 0x1000000>;

I was wondering about how this fits into the PCI DT
binding, is this a child of the PCI device, or does the
"pci-ep-bus" refer to the PCI device itself?

Where do the "0x01 0x00 0x00" and "0x00 0x00 0x00" addresses
come from? Shouldn't those be "02000010 0x00 0x00" and
"02000014 0x00 0x00" to refer to the first and second
relocatable 32-bit memory BAR?

     Arnd

Re: [PATCH v6 3/7] misc: Add support for LAN966x PCI device

[Herve Codina]

Hi Arnd,

On Wed, 02 Oct 2024 11:08:15 +0000
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:
> 
> > +			pci-ep-bus@0 {
> > +				compatible = "simple-bus";
> > +				#address-cells = <1>;
> > +				#size-cells = <1>;
> > +
> > +				/*
> > +				 * map @0xe2000000 (32MB) to BAR0 (CPU)
> > +				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
> > +				 */
> > +				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
> > +				          0xe0000000 0x01 0x00 0x00 0x1000000>;  
> 
> I was wondering about how this fits into the PCI DT
> binding, is this a child of the PCI device, or does the
> "pci-ep-bus" refer to the PCI device itself?

This is a child of the PCI device.
The overlay is applied at the PCI device node and so, the pci-ep-bus is
a child of the PCI device node.

> 
> Where do the "0x01 0x00 0x00" and "0x00 0x00 0x00" addresses
> come from? Shouldn't those be "02000010 0x00 0x00" and
> "02000014 0x00 0x00" to refer to the first and second
> relocatable 32-bit memory BAR?

These addresses are built dynamically by the PCI core during the PCI scan.
  https://elixir.bootlin.com/linux/v6.11/source/drivers/pci/of_property.c#L101
They are use to reference the BARs.
0x00 for BAR0, 0x01 for BAR1, ...

The full DT, once PCI device are present, scanned and the overlay applied,
looks like the following:
--- 8< ---
	pcie@d0070000 {
		/* Node present on the base device tree */
		compatible = "marvell,armada-3700-pcie";
		#address-cells = <0x03>;
		#size-cells = <0x02>;
		ranges = <0x82000000 0x00 0xe8000000 0x00 0xe8000000 0x00 0x7f00000
			  0x81000000 0x00 0x00 0x00 0xefff0000 0x00 0x10000>;
		device_type = "pci";
		...

		pci@0,0 {
			/*
			 * Node created at runtime during the PCI scan
			 * This node is PCI bridge (class 604)
			 */
			#address-cells = <0x03>;
			#size-cells = <0x02>;
			device_type = "pci";
			compatible = "pci11ab,100\0pciclass,060400\0pciclass,0604";
			ranges = <0x82000000 0x00 0xe8000000 0x82000000 0x00 0xe8000000 0x00 0x4400000>;
			...

			dev@0,0 {
				/*
				 * Node created at runtime during the
				 * PCI scan. This is my LAN966x PCI device.
				 */
				#address-cells = <0x03>;
				interrupts = <0x01>;
				#size-cells = <0x02>;
				compatible = "pci1055,9660\0pciclass,020000\0pciclass,0200";

				/*
				 * Ranges items allow to reference BAR0,
				 * BAR1, ... from children nodes.
				 * The property is created by the PCI core
				 * during the PCI bus scan.
				 */
				ranges = <0x00 0x00 0x00 0x82010000 0x00 0xe8000000 0x00 0x2000000
					  0x01 0x00 0x00 0x82010000 0x00 0xea000000 0x00 0x1000000
					  0x02 0x00 0x00 0x82010000 0x00 0xeb000000 0x00 0x800000
					  0x03 0x00 0x00 0x82010000 0x00 0xeb800000 0x00 0x800000
					  0x04 0x00 0x00 0x82010000 0x00 0xec000000 0x00 0x20000
					  0x05 0x00 0x00 0x82010000 0x00 0xec020000 0x00 0x2000>;
				...

				pci-ep-bus@0 {
					/* Node added by the overlay */
					#address-cells = <0x01>;
					#size-cells = <0x01>;
					compatible = "simple-bus";

					/*
					 * Remap 0xe2000000 to BAR0 and
					 * 0xe0000000 to BAR1
					 */
					ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
						  0xe0000000 0x01 0x00 0x00 0x1000000>;
					...

					mdio@e200413c {
						#address-cells = <0x01>;
						resets = <0x25 0x00>;
						#size-cells = <0x00>;
						compatible = "microchip,lan966x-miim";
						reg = <0xe200413c 0x24 0xe2010020 0x04>;
						...
--- 8< ---

Hope this full picture helped to understand the address translations
involved.

Best regards,
Hervé

Re: [PATCH v6 3/7] misc: Add support for LAN966x PCI device

[Arnd Bergmann]

On Wed, Oct 2, 2024, at 12:41, Herve Codina wrote:
> On Wed, 02 Oct 2024 11:08:15 +0000
> "Arnd Bergmann" <arnd@arndb.de> wrote:
>> On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:
>> 
>> > +			pci-ep-bus@0 {
>> > +				compatible = "simple-bus";
>> > +				#address-cells = <1>;
>> > +				#size-cells = <1>;
>> > +
>> > +				/*
>> > +				 * map @0xe2000000 (32MB) to BAR0 (CPU)
>> > +				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
>> > +				 */
>> > +				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
>> > +				          0xe0000000 0x01 0x00 0x00 0x1000000>;  
>> 
>> I was wondering about how this fits into the PCI DT
>> binding, is this a child of the PCI device, or does the
>> "pci-ep-bus" refer to the PCI device itself?
>
> This is a child of the PCI device.
> The overlay is applied at the PCI device node and so, the pci-ep-bus is
> a child of the PCI device node.

Ok

> 				/*
> 				 * Ranges items allow to reference BAR0,
> 				 * BAR1, ... from children nodes.
> 				 * The property is created by the PCI core
> 				 * during the PCI bus scan.
> 				 */
> 				ranges = <0x00 0x00 0x00 0x82010000 0x00 0xe8000000 0x00 0x2000000
> 					  0x01 0x00 0x00 0x82010000 0x00 0xea000000 0x00 0x1000000
> 					  0x02 0x00 0x00 0x82010000 0x00 0xeb000000 0x00 0x800000

>
> Hope this full picture helped to understand the address translations
> involved.

Right, that makes a lot of sense now, I wasn't aware of those
range properties getting set. Now I have a new question though:

Is this designed to work both on hosts using devicetree and on
those not using it? If this is used on devicetree on a board
that has a hardwired lan966x, we may want to include the
overlay contents in the board dts file itself in order to
describe any possible connections between the lan966x chip
and other onboard components such as additional GPIOs or
ethernet PHY chips, right?

      Arnd

Re: [PATCH v6 3/7] misc: Add support for LAN966x PCI device

[Herve Codina]

On Wed, 02 Oct 2024 14:31:13 +0000
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Wed, Oct 2, 2024, at 12:41, Herve Codina wrote:
> > On Wed, 02 Oct 2024 11:08:15 +0000
> > "Arnd Bergmann" <arnd@arndb.de> wrote:  
> >> On Mon, Sep 30, 2024, at 12:15, Herve Codina wrote:
> >>   
> >> > +			pci-ep-bus@0 {
> >> > +				compatible = "simple-bus";
> >> > +				#address-cells = <1>;
> >> > +				#size-cells = <1>;
> >> > +
> >> > +				/*
> >> > +				 * map @0xe2000000 (32MB) to BAR0 (CPU)
> >> > +				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
> >> > +				 */
> >> > +				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
> >> > +				          0xe0000000 0x01 0x00 0x00 0x1000000>;    
> >> 
> >> I was wondering about how this fits into the PCI DT
> >> binding, is this a child of the PCI device, or does the
> >> "pci-ep-bus" refer to the PCI device itself?  
> >
> > This is a child of the PCI device.
> > The overlay is applied at the PCI device node and so, the pci-ep-bus is
> > a child of the PCI device node.  
> 
> Ok
> 
> > 				/*
> > 				 * Ranges items allow to reference BAR0,
> > 				 * BAR1, ... from children nodes.
> > 				 * The property is created by the PCI core
> > 				 * during the PCI bus scan.
> > 				 */
> > 				ranges = <0x00 0x00 0x00 0x82010000 0x00 0xe8000000 0x00 0x2000000
> > 					  0x01 0x00 0x00 0x82010000 0x00 0xea000000 0x00 0x1000000
> > 					  0x02 0x00 0x00 0x82010000 0x00 0xeb000000 0x00 0x800000  
> 
> >
> > Hope this full picture helped to understand the address translations
> > involved.  
> 
> Right, that makes a lot of sense now, I wasn't aware of those
> range properties getting set. Now I have a new question though:
> 
> Is this designed to work both on hosts using devicetree and on
> those not using it? If this is used on devicetree on a board
> that has a hardwired lan966x, we may want to include the
> overlay contents in the board dts file itself in order to
> describe any possible connections between the lan966x chip
> and other onboard components such as additional GPIOs or
> ethernet PHY chips, right?
> 
>       Arnd

On host with the base hardware described without device-tree (ACPI on
x86 for instance), I have a couple of patches not yet sent upstream.
With those patches, I have a the LAN966x PCI board working on x86.
I plan to send them as soon as this series is applied.

Rob said that before looking at ACPI, we need to have a working system
on DT based systems.
  https://lore.kernel.org/all/CAL_JsqKNC1Qv+fucobnzoXmxUYNockWR=BbGhds2tNAYZWqgOA@mail.gmail.com/

If hardwired on a board, the same LAN966x PCI driver could be used.
A possible improvement of the driver could be to request the overlay
from the user-space using request_firmware().
With that, the overlay can be extended with specific onboard parts the
LAN966x PCI device is connected to.

This improvement can be done later when the use case appears.

Best regards,
Hervé