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

[PATCH v9 0/6] 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 changes are needed:
 - Patches 1 and 2 introduce the LAN996x PCI driver itself, together
   with its DT overlay and the related MAINTAINER entry.

 - Patch 3 removes the syscon API usage from the reset driver used for
   the LAN966x.

 - Patches 4 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.

The v8 series sent didn't contains the expected modification and have to
be simply ignore.
This v9 series fixes that and contains modification expected in v8

Compare to the previous iteration:
  https://lore.kernel.org/lkml/20241003081647.642468-1-herve.codina@bootlin.com/
this v9 series mainly:
  - Add missing spaces
  - Add "Reviewed-by" tags

Best regards,
Hervé

Changes v9 -> v8
  v8 was incorrect (modifications not present).
  v9 fixed that and contains modification that should be present in v8

Changes v7 -> v8
  - Patch 1
    Add missing spaces around '=' in dtso

  - Patches 2, 4, 5 and 6
    Add 'Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>'

  - Patch 3
    Add 'Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>'
    Add	'Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>'

Changes v6 -> v7
  - Patch 1 (patch 3 in v6)
    Re-introduce the syscon node in the overlay

  - Patch 2 (patch 4 in v6)
    No changes

  - Patch 3 (patch 2 in v6)
    Rework code to map the syscon device locally without using the
    syscon API in the LAN966x case.
    Update commit log

  - Patches 4, 5 and 6 (patches 5, 6, and 7 in v6)
    No changes

  Patch removed in v7
    - Patch 1 in v6 (reset controller DT binding modification)
      Rejected

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 (4):
  misc: Add support for LAN966x PCI device
  MAINTAINERS: Add the Microchip LAN966x PCI driver entry
  reset: mchp: sparx5: Map cpu-syscon locally in case of LAN966x
  reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency

 MAINTAINERS                            |   6 +
 drivers/misc/Kconfig                   |  24 +++
 drivers/misc/Makefile                  |   3 +
 drivers/misc/lan966x_pci.c             | 215 +++++++++++++++++++++++++
 drivers/misc/lan966x_pci.dtso          | 167 +++++++++++++++++++
 drivers/pci/quirks.c                   |   1 +
 drivers/reset/Kconfig                  |   4 +-
 drivers/reset/reset-microchip-sparx5.c |  38 ++++-
 8 files changed, 455 insertions(+), 3 deletions(-)
 create mode 100644 drivers/misc/lan966x_pci.c
 create mode 100644 drivers/misc/lan966x_pci.dtso

-- 
2.46.2

[PATCH v9 1/6] 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 | 167 ++++++++++++++++++++++++++
 drivers/pci/quirks.c          |   1 +
 5 files changed, 410 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..7282687df25f
--- /dev/null
+++ b/drivers/misc/lan966x_pci.dtso
@@ -0,0 +1,167 @@
+// 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
+				};
+
+				cpu_ctrl: syscon@e00c0000 {
+					compatible = "microchip,lan966x-cpu-syscon", "syscon";
+					reg = <0xe00c0000 0xa8>;
+				};
+
+				reset: reset@e200400c {
+					compatible = "microchip,lan966x-switch-reset";
+					reg = <0xe200400c 0x4>, <0xe00c0000 0xa8>;
+					reg-names = "gcb","cpu";
+					#reset-cells = <1>;
+					cpu-syscon = <&cpu_ctrl>;
+				};
+
+				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.2

[PATCH v9 2/6] 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.2

[PATCH v9 3/6] reset: mchp: sparx5: Map cpu-syscon locally in case of LAN966x

[Herve Codina]

In the LAN966x PCI device use case, the syscon API cannot be used as
it does not support device 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. The syscon API follows this assumption
but this assumption is no longer valid in the LAN966x use case.

In order to avoid the use of the syscon API and so, support for removal,
use a local mapping of the syscon device.

Link: https://lore.kernel.org/all/20240923100741.11277439@bootlin.com/ [1]
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/reset-microchip-sparx5.c | 35 +++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 636e85c388b0..48a62d5da78d 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -62,6 +62,28 @@ static const struct reset_control_ops sparx5_reset_ops = {
 	.reset = sparx5_reset_noop,
 };
 
+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);
+}
+
 static int mchp_sparx5_map_syscon(struct platform_device *pdev, char *name,
 				  struct regmap **target)
 {
@@ -72,7 +94,18 @@ static int mchp_sparx5_map_syscon(struct platform_device *pdev, char *name,
 	syscon_np = of_parse_phandle(pdev->dev.of_node, name, 0);
 	if (!syscon_np)
 		return -ENODEV;
-	regmap = syscon_node_to_regmap(syscon_np);
+
+	/*
+	 * The syscon API doesn't support syscon device removal.
+	 * When used in LAN966x PCI device, the cpu-syscon device needs to be
+	 * removed when the PCI device is removed.
+	 * In case of LAN966x, map the syscon device locally to support the
+	 * device removal.
+	 */
+	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);
 	of_node_put(syscon_np);
 	if (IS_ERR(regmap)) {
 		err = PTR_ERR(regmap);
-- 
2.46.2

[PATCH v9 4/6] 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>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 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.2

[PATCH v9 5/6] 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>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 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 48a62d5da78d..c4cc0edbb250 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -191,6 +191,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,
@@ -213,3 +214,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.2

[PATCH v9 6/6] 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>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 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 c4cc0edbb250..aa5464be7053 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -154,6 +154,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.2

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Bjorn Helgaas]

On Thu, Oct 10, 2024 at 08:36:01AM +0200, Herve Codina wrote:
> 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>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# quirks.c

> ---
>  drivers/misc/Kconfig          |  24 ++++
>  drivers/misc/Makefile         |   3 +
>  drivers/misc/lan966x_pci.c    | 215 ++++++++++++++++++++++++++++++++++
>  drivers/misc/lan966x_pci.dtso | 167 ++++++++++++++++++++++++++
>  drivers/pci/quirks.c          |   1 +
>  5 files changed, 410 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:

I don't think "overlay" by itself has enough context to be useful as
help text.  Maybe "device tree" or similar hint?

Add blank lines between paragraphs or reflow into a single paragraph.

> +	    - 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/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.2
> 

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Michal Kubecek]

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

On Thu, Oct 10, 2024 at 08:36:01AM +0200, Herve Codina wrote:
> 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>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# quirks.c
> ---
>  drivers/misc/Kconfig          |  24 ++++
>  drivers/misc/Makefile         |   3 +
>  drivers/misc/lan966x_pci.c    | 215 ++++++++++++++++++++++++++++++++++
>  drivers/misc/lan966x_pci.dtso | 167 ++++++++++++++++++++++++++
>  drivers/pci/quirks.c          |   1 +
>  5 files changed, 410 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

Are these "select" statements what we want? When configuring current
mainline snapshot, I accidentally enabled this driver and ended up
flooded with an enormous amount of new config options, most of which
didn't make much sense on x86_64. It took quite long to investigate why.

Couldn't we rather use

	depends on PCI && OF && OF_OVERLAY

like other drivers?

Michal

> +	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"
[...]

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

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Herve Codina]

Hi Michal,

On Thu, 28 Nov 2024 20:42:53 +0100
Michal Kubecek <mkubecek@suse.cz> wrote:

...
> > --- 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  
> 
> Are these "select" statements what we want? When configuring current
> mainline snapshot, I accidentally enabled this driver and ended up
> flooded with an enormous amount of new config options, most of which
> didn't make much sense on x86_64. It took quite long to investigate why.
> 
> Couldn't we rather use
> 
> 	depends on PCI && OF && OF_OVERLAY
> 
> like other drivers?
> 

I don't have a strong opinion on this 'select' vs 'depends on' for those
symbols.

I used select because the dependency is not obvious for a user that just
want the driver for the LAN966x PCI device.

Best regards,
Hervé

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Krzysztof Kozlowski]

On 29/11/2024 09:10, Herve Codina wrote:
>>> +config MCHP_LAN966X_PCI
>>> +	tristate "Microchip LAN966x PCIe Support"
>>> +	depends on PCI
>>> +	select OF
>>> +	select OF_OVERLAY  
>>
>> Are these "select" statements what we want? When configuring current
>> mainline snapshot, I accidentally enabled this driver and ended up
>> flooded with an enormous amount of new config options, most of which
>> didn't make much sense on x86_64. It took quite long to investigate why.
>>
>> Couldn't we rather use
>>
>> 	depends on PCI && OF && OF_OVERLAY
>>
>> like other drivers?
>>
> 
> I don't have a strong opinion on this 'select' vs 'depends on' for those
> symbols.


You should not select user-visible symbols, with exception of arch
stuff. This is not arch, so you should use dependencies.


Best regards,
Krzysztof

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Arnd Bergmann]

On Fri, Nov 29, 2024, at 09:10, Herve Codina wrote:
> Hi Michal,
> On Thu, 28 Nov 2024 20:42:53 +0100
> Michal Kubecek <mkubecek@suse.cz> wrote:
>> > --- 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  
>> 
>> Are these "select" statements what we want? When configuring current
>> mainline snapshot, I accidentally enabled this driver and ended up
>> flooded with an enormous amount of new config options, most of which
>> didn't make much sense on x86_64. It took quite long to investigate why.
>> 
>> Couldn't we rather use
>> 
>> 	depends on PCI && OF && OF_OVERLAY
>> 
>> like other drivers?

Agreed.

I would write in two lines as

        depends on PCI
        depends on OF_OVERLAY

since OF_OVERLAY already depends on OF, that can be left out.
The effect is the same as your variant though.
 
>
> I don't have a strong opinion on this 'select' vs 'depends on' for those
> symbols.
>
> I used select because the dependency is not obvious for a user that just
> want the driver for the LAN966x PCI device.

The general rules for picking one over the other are:

- use 'select' for symbols that can not be enabled manually
- prefer 'depends on' for user-visible options
- do whatever other driver do for the same symbol, to avoid
  dependency loops.

     Arnd

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Geert Uytterhoeven]

On Fri, Nov 29, 2024 at 9:25 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Nov 29, 2024, at 09:10, Herve Codina wrote:
> > On Thu, 28 Nov 2024 20:42:53 +0100
> > Michal Kubecek <mkubecek@suse.cz> wrote:
> >> > --- 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
> >>
> >> Are these "select" statements what we want? When configuring current
> >> mainline snapshot, I accidentally enabled this driver and ended up
> >> flooded with an enormous amount of new config options, most of which
> >> didn't make much sense on x86_64. It took quite long to investigate why.
> >>
> >> Couldn't we rather use
> >>
> >>      depends on PCI && OF && OF_OVERLAY
> >>
> >> like other drivers?
>
> Agreed.
>
> I would write in two lines as
>
>         depends on PCI
>         depends on OF_OVERLAY
>
> since OF_OVERLAY already depends on OF, that can be left out.
> The effect is the same as your variant though.

What about

    depends on OF
    select OF_OVERLAY

as "OF" is a clear bus dependency, due to the driver providing an OF
child bus (cfr. I2C or SPI bus controller drivers depending on I2C or
SPI), and OF_OVERLAY is an optional software mechanism?

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 v9 1/6] misc: Add support for LAN966x PCI device

[Arnd Bergmann]

On Fri, Nov 29, 2024, at 09:44, Geert Uytterhoeven wrote:
> On Fri, Nov 29, 2024 at 9:25 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Nov 29, 2024, at 09:10, Herve Codina wrote:
>> I would write in two lines as
>>
>>         depends on PCI
>>         depends on OF_OVERLAY
>>
>> since OF_OVERLAY already depends on OF, that can be left out.
>> The effect is the same as your variant though.
>
> What about
>
>     depends on OF
>     select OF_OVERLAY
>
> as "OF" is a clear bus dependency, due to the driver providing an OF
> child bus (cfr. I2C or SPI bus controller drivers depending on I2C or
> SPI), and OF_OVERLAY is an optional software mechanism?

OF_OVERLAY is currently a user visible option, so I think it's
intended to be used with 'depends on'. The only other callers
of this interface are the kunit test modules that just leave
out the overlay code if that is disabled.

If we decide to treat OF_OVERLAY as a library instead, it should
probably become a silent Kconfig option that gets selected by
all its users including the unit tests, and then we can remove
the #ifdef checks there.

Since OF_OVERLAY pulls in OF_DYNAMIC, I would still prefer that
to be a user choice. Silently enabling OF_OVERLAY definitely has
a risk of introducing regressions since it changes some of the
interesting code paths in the core, in particular it enables
reference counting in of_node_get(), which many drivers get wrong.

      Arnd

Re: [PATCH v9 1/6] misc: Add support for LAN966x PCI device

[Geert Uytterhoeven]

Hi Arnd,

On Fri, Nov 29, 2024 at 10:23 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Nov 29, 2024, at 09:44, Geert Uytterhoeven wrote:
> > On Fri, Nov 29, 2024 at 9:25 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Fri, Nov 29, 2024, at 09:10, Herve Codina wrote:
> >> I would write in two lines as
> >>
> >>         depends on PCI
> >>         depends on OF_OVERLAY
> >>
> >> since OF_OVERLAY already depends on OF, that can be left out.
> >> The effect is the same as your variant though.
> >
> > What about
> >
> >     depends on OF
> >     select OF_OVERLAY
> >
> > as "OF" is a clear bus dependency, due to the driver providing an OF
> > child bus (cfr. I2C or SPI bus controller drivers depending on I2C or
> > SPI), and OF_OVERLAY is an optional software mechanism?
>
> OF_OVERLAY is currently a user visible option, so I think it's
> intended to be used with 'depends on'. The only other callers
> of this interface are the kunit test modules that just leave
> out the overlay code if that is disabled.

Indeed, there are no real upstream users of OF_OVERLAY left.
Until commit 1760eb547276299a ("drm: rcar-du: Drop leftovers
dependencies from Kconfig"), the rcar-lvds driver selected OF_OVERLAY
to be able to fix up old DTBs.

> If we decide to treat OF_OVERLAY as a library instead, it should
> probably become a silent Kconfig option that gets selected by
> all its users including the unit tests, and then we can remove
> the #ifdef checks there.

Yep.

> Since OF_OVERLAY pulls in OF_DYNAMIC, I would still prefer that
> to be a user choice. Silently enabling OF_OVERLAY definitely has
> a risk of introducing regressions since it changes some of the
> interesting code paths in the core, in particular it enables
> reference counting in of_node_get(), which many drivers get wrong.

Distro kernels will have to enable this anyway, if they want to
support LAN966x...

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 v9 1/6] misc: Add support for LAN966x PCI device

[Herve Codina]

Hi,

+Cc Ricardo

On Fri, 29 Nov 2024 11:29:44 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Arnd,
> 
> On Fri, Nov 29, 2024 at 10:23 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Fri, Nov 29, 2024, at 09:44, Geert Uytterhoeven wrote:  
> > > On Fri, Nov 29, 2024 at 9:25 AM Arnd Bergmann <arnd@arndb.de> wrote:  
> > >> On Fri, Nov 29, 2024, at 09:10, Herve Codina wrote:
> > >> I would write in two lines as
> > >>
> > >>         depends on PCI
> > >>         depends on OF_OVERLAY
> > >>
> > >> since OF_OVERLAY already depends on OF, that can be left out.
> > >> The effect is the same as your variant though.  
> > >
> > > What about
> > >
> > >     depends on OF
> > >     select OF_OVERLAY
> > >
> > > as "OF" is a clear bus dependency, due to the driver providing an OF
> > > child bus (cfr. I2C or SPI bus controller drivers depending on I2C or
> > > SPI), and OF_OVERLAY is an optional software mechanism?  
> >

A patch has be done in that way by Ricardo Ribalda
  https://lore.kernel.org/all/20241129-lan966x-depend-v1-1-603fc4996c4f@chromium.org/

> > OF_OVERLAY is currently a user visible option, so I think it's
> > intended to be used with 'depends on'. The only other callers
> > of this interface are the kunit test modules that just leave
> > out the overlay code if that is disabled.  
> 
> Indeed, there are no real upstream users of OF_OVERLAY left.
> Until commit 1760eb547276299a ("drm: rcar-du: Drop leftovers
> dependencies from Kconfig"), the rcar-lvds driver selected OF_OVERLAY
> to be able to fix up old DTBs.
> 
> > If we decide to treat OF_OVERLAY as a library instead, it should
> > probably become a silent Kconfig option that gets selected by
> > all its users including the unit tests, and then we can remove
> > the #ifdef checks there.  
> 
> Yep.
> 
> > Since OF_OVERLAY pulls in OF_DYNAMIC, I would still prefer that
> > to be a user choice. Silently enabling OF_OVERLAY definitely has
> > a risk of introducing regressions since it changes some of the
> > interesting code paths in the core, in particular it enables
> > reference counting in of_node_get(), which many drivers get wrong.  
> 
> Distro kernels will have to enable this anyway, if they want to
> support LAN966x...
> 

Best regards,
Hervé