* [PATCH v4 1/8] misc: Add support for LAN966x PCI device
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-05 20:13 ` Andy Shevchenko
2024-08-05 10:17 ` [PATCH v4 2/8] MAINTAINERS: Add the Microchip LAN966x PCI driver entry Herve Codina
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni
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>
---
drivers/misc/Kconfig | 24 ++++
drivers/misc/Makefile | 3 +
drivers/misc/lan966x_pci.c | 229 ++++++++++++++++++++++++++++++++++
drivers/misc/lan966x_pci.dtso | 167 +++++++++++++++++++++++++
drivers/pci/quirks.c | 1 +
5 files changed, 424 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 41c3d2821a78..6cd105a44abb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -600,6 +600,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 c2f990862d2b..a1bdbf404f13 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -70,4 +70,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..c69350449b15
--- /dev/null
+++ b/drivers/misc/lan966x_pci.c
@@ -0,0 +1,229 @@
+// 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/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 IRQ_RETVAL(!ret);
+}
+
+static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+ struct pci_dev_intr_ctrl *intr_ctrl;
+ struct fwnode_handle *fwnode;
+ int ret;
+
+ if (!pdev->irq)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ 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");
+ ret = -ENOMEM;
+ goto err_free_intr_ctrl;
+ }
+
+ 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,
+ dev_name(&pdev->dev), intr_ctrl);
+ if (ret) {
+ pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
+ goto err_free_irq_vector;
+ }
+
+ return intr_ctrl;
+
+err_free_irq_vector:
+ pci_free_irq_vectors(pdev);
+err_remove_domain:
+ irq_domain_remove(intr_ctrl->irq_domain);
+err_free_intr_ctrl:
+ kfree(intr_ctrl);
+ 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 *data)
+{
+ struct pci_dev_intr_ctrl *intr_ctrl = data;
+
+ 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;
+ int ret;
+
+ ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, dev_of_node(data->dev));
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+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)) {
+ dev_err(dev, "Missing of_node for device\n");
+ return -EINVAL;
+ }
+
+ /* 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..041f4319e4cd
--- /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/interrupt-controller/irq.h>
+#include <dt-bindings/mfd/atmel-flexcom.h>
+#include <dt-bindings/phy/phy-lan966x-serdes.h>
+#include <dt-bindings/gpio/gpio.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>;
+ reg-names = "gcb";
+ #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 a2ce4e08edf5..bae2dd99017c 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6245,6 +6245,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.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device
2024-08-05 10:17 ` [PATCH v4 1/8] misc: Add support for LAN966x PCI device Herve Codina
@ 2024-08-05 20:13 ` Andy Shevchenko
2024-08-07 10:09 ` Herve Codina
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2024-08-05 20:13 UTC (permalink / raw)
To: Herve Codina
Cc: Geert Uytterhoeven, Simon Horman, Lee Jones, Arnd Bergmann,
Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman, Bjorn Helgaas,
Philipp Zabel, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, Rob Herring, Saravana Kannan, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Horatiu Vultur,
Andrew Lunn, linux-kernel, netdev, linux-pci, linux-arm-kernel,
devicetree, Allan Nielsen, Luca Ceresoli, Thomas Petazzoni
On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> 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.
...
+ 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>
AFAIU pci_ids..h is guaranteed to be included by pci.h, but having it
here explicitly doesn't make it worse, so up to you.
> +#include <linux/slab.h>
...
> +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 IRQ_RETVAL(!ret);
Hmm... I dunno if it was me who suggested IRQ_RETVAL() here, but it
usually makes sense for the cases where ret is not inverted.
Perhaps
if (ret)
return NONE;
return HANDLED;
is slightly better in this case?
> +}
...
> +static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
> +{
> + struct pci_dev_intr_ctrl *intr_ctrl;
> + struct fwnode_handle *fwnode;
> + int ret;
> + if (!pdev->irq)
> + return ERR_PTR(-EOPNOTSUPP);
Before even trying to get it via APIs? (see below as well)
Also, when is it possible to have 0 here?
> + fwnode = dev_fwnode(&pdev->dev);
> + if (!fwnode)
> + return ERR_PTR(-ENODEV);
> +
> + intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);
Hmm... Why not use __free()?
> + 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");
> + ret = -ENOMEM;
> + goto err_free_intr_ctrl;
> + }
> + 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;
> + }
I am wondering if you even need this in case you want solely INTx.
> + intr_ctrl->irq = pci_irq_vector(pdev, 0);
Don't remember documentation by heart for this, but the implementation
suggests that it can be called without the above for retrieving INTx.
> + ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
> + dev_name(&pdev->dev), intr_ctrl);
pci_name() ? (IIRC the macro name)
> + if (ret) {
> + pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
> + goto err_free_irq_vector;
> + }
> +
> + return intr_ctrl;
> +
> +err_free_irq_vector:
> + pci_free_irq_vectors(pdev);
> +err_remove_domain:
> + irq_domain_remove(intr_ctrl->irq_domain);
> +err_free_intr_ctrl:
> + kfree(intr_ctrl);
> + return ERR_PTR(ret);
> +}
...
> +static void devm_pci_dev_remove_intr_ctrl(void *data)
> +{
> + struct pci_dev_intr_ctrl *intr_ctrl = data;
It can be eliminated
static void devm_pci_...(void *intr_ctrl)
> + pci_dev_remove_intr_ctrl(intr_ctrl);
> +}
...
> +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;
> + int ret;
> +
> + ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, dev_of_node(data->dev));
> + if (ret)
> + return ret;
> +
> + return 0;
return of_overlay_fdt_apply() ?
> +}
...
> +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)) {
> + dev_err(dev, "Missing of_node for device\n");
> + return -EINVAL;
return dev_err_probe() ?
> + }
> +
> + /* 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;
> +}
...
> +#include <dt-bindings/clock/microchip,lan966x.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <dt-bindings/phy/phy-lan966x-serdes.h>
> +#include <dt-bindings/gpio/gpio.h>
Alphabetical order?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device
2024-08-05 20:13 ` Andy Shevchenko
@ 2024-08-07 10:09 ` Herve Codina
2024-08-08 12:32 ` Andy Shevchenko
0 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-07 10:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Geert Uytterhoeven, Simon Horman, Lee Jones, Arnd Bergmann,
Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman, Bjorn Helgaas,
Philipp Zabel, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, Rob Herring, Saravana Kannan, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Horatiu Vultur,
Andrew Lunn, linux-kernel, netdev, linux-pci, linux-arm-kernel,
devicetree, Allan Nielsen, Luca Ceresoli, Thomas Petazzoni
Hi Andy,
On Mon, 5 Aug 2024 22:13:38 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> 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.
>
> ...
>
> + device.h
Will be added.
>
> > +#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>
>
> AFAIU pci_ids..h is guaranteed to be included by pci.h, but having it
> here explicitly doesn't make it worse, so up to you.
I will keep pci_ids.h
>
> > +#include <linux/slab.h>
>
> ...
>
> > +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 IRQ_RETVAL(!ret);
>
> Hmm... I dunno if it was me who suggested IRQ_RETVAL() here, but it
> usually makes sense for the cases where ret is not inverted.
>
> Perhaps
>
> if (ret)
> return NONE;
> return HANDLED;
>
> is slightly better in this case?
Right. I will use a more compact version:
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;
> > + struct fwnode_handle *fwnode;
> > + int ret;
>
> > + if (!pdev->irq)
> > + return ERR_PTR(-EOPNOTSUPP);
>
> Before even trying to get it via APIs? (see below as well)
> Also, when is it possible to have 0 here?
pdev->irq can be 0 if the PCI device did not request any IRQ
(i.e. PCI_INTERRUPT_PIN in PCI config header is 0).
I use that to check whether or not INTx is supported.
Even if this code is present in the LAN966x PCI driver, it can be use as a
starting point for other drivers and may be moved to a common part in the
future.
Do you think I should remove it ?
If keeping it is fine, I will add a comment.
>
> > + fwnode = dev_fwnode(&pdev->dev);
> > + if (!fwnode)
> > + return ERR_PTR(-ENODEV);
> > +
> > + intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);
>
> Hmm... Why not use __free()?
Well, just because I am not used to using __free() and I didn't think
about it.
I will use it in the next operation.
>
> > + 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");
> > + ret = -ENOMEM;
> > + goto err_free_intr_ctrl;
> > + }
>
> > + 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;
> > + }
>
> I am wondering if you even need this in case you want solely INTx.
I have the feeling that it is needed.
pci_alloc_irq_vectors() will call pci_intx() which in turn enables INT
clearing PCI_COMMAND_INTX_DISABLE flag in the PCI_COMMAND config word.
>
> > + intr_ctrl->irq = pci_irq_vector(pdev, 0);
>
> Don't remember documentation by heart for this, but the implementation
> suggests that it can be called without the above for retrieving INTx.
So, with the above said, I will keep both pci_alloc_irq_vectors() and
pci_irq_vector() calls.
>
> > + ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
> > + dev_name(&pdev->dev), intr_ctrl);
>
> pci_name() ? (IIRC the macro name)
Indeed, will be changed.
>
> > + if (ret) {
> > + pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
> > + goto err_free_irq_vector;
> > + }
> > +
> > + return intr_ctrl;
> > +
> > +err_free_irq_vector:
> > + pci_free_irq_vectors(pdev);
> > +err_remove_domain:
> > + irq_domain_remove(intr_ctrl->irq_domain);
> > +err_free_intr_ctrl:
> > + kfree(intr_ctrl);
> > + return ERR_PTR(ret);
> > +}
>
> ...
>
> > +static void devm_pci_dev_remove_intr_ctrl(void *data)
> > +{
>
> > + struct pci_dev_intr_ctrl *intr_ctrl = data;
>
> It can be eliminated
>
> static void devm_pci_...(void *intr_ctrl)
I will update.
>
> > + pci_dev_remove_intr_ctrl(intr_ctrl);
> > +}
>
> ...
>
> > +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;
> > + int ret;
> > +
> > + ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, dev_of_node(data->dev));
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> return of_overlay_fdt_apply() ?
Yes indeed.
>
> > +}
>
> ...
>
> > +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)) {
>
> > + dev_err(dev, "Missing of_node for device\n");
> > + return -EINVAL;
>
> return dev_err_probe() ?
Yes, I will update.
>
> > + }
> > +
> > + /* 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;
> > +}
>
> ...
>
> > +#include <dt-bindings/clock/microchip,lan966x.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/mfd/atmel-flexcom.h>
> > +#include <dt-bindings/phy/phy-lan966x-serdes.h>
>
> > +#include <dt-bindings/gpio/gpio.h>
>
> Alphabetical order?
Yes indeed.
Thanks for the review.
Best regards,
Hervé
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device
2024-08-07 10:09 ` Herve Codina
@ 2024-08-08 12:32 ` Andy Shevchenko
2024-08-08 14:07 ` Herve Codina
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2024-08-08 12:32 UTC (permalink / raw)
To: Herve Codina
Cc: Geert Uytterhoeven, Simon Horman, Lee Jones, Arnd Bergmann,
Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman, Bjorn Helgaas,
Philipp Zabel, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, Rob Herring, Saravana Kannan, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Horatiu Vultur,
Andrew Lunn, linux-kernel, netdev, linux-pci, linux-arm-kernel,
devicetree, Allan Nielsen, Luca Ceresoli, Thomas Petazzoni
On Wed, Aug 7, 2024 at 1:10 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Mon, 5 Aug 2024 22:13:38 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
...
> > > + if (!pdev->irq)
> > > + return ERR_PTR(-EOPNOTSUPP);
> >
> > Before even trying to get it via APIs? (see below as well)
> > Also, when is it possible to have 0 here?
>
> pdev->irq can be 0 if the PCI device did not request any IRQ
> (i.e. PCI_INTERRUPT_PIN in PCI config header is 0).
> I use that to check whether or not INTx is supported.
But why do you need that? What happens if you get a new device that
supports let's say MSI?
> Even if this code is present in the LAN966x PCI driver, it can be use as a
> starting point for other drivers and may be moved to a common part in the
> future.
>
> Do you think I should remove it ?
I think pci_alloc_vectors() should be enough. Make it to be the first
call, if you think it's better.
> If keeping it is fine, I will add a comment.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device
2024-08-08 12:32 ` Andy Shevchenko
@ 2024-08-08 14:07 ` Herve Codina
0 siblings, 0 replies; 19+ messages in thread
From: Herve Codina @ 2024-08-08 14:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Geert Uytterhoeven, Simon Horman, Lee Jones, Arnd Bergmann,
Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman, Bjorn Helgaas,
Philipp Zabel, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, Rob Herring, Saravana Kannan, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Horatiu Vultur,
Andrew Lunn, linux-kernel, netdev, linux-pci, linux-arm-kernel,
devicetree, Allan Nielsen, Luca Ceresoli, Thomas Petazzoni
Hi Andy,
On Thu, 8 Aug 2024 15:32:07 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Aug 7, 2024 at 1:10 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Mon, 5 Aug 2024 22:13:38 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
>
> ...
>
> > > > + if (!pdev->irq)
> > > > + return ERR_PTR(-EOPNOTSUPP);
> > >
> > > Before even trying to get it via APIs? (see below as well)
> > > Also, when is it possible to have 0 here?
> >
> > pdev->irq can be 0 if the PCI device did not request any IRQ
> > (i.e. PCI_INTERRUPT_PIN in PCI config header is 0).
>
> > I use that to check whether or not INTx is supported.
>
> But why do you need that? What happens if you get a new device that
> supports let's say MSI?
>
> > Even if this code is present in the LAN966x PCI driver, it can be use as a
> > starting point for other drivers and may be moved to a common part in the
> > future.
> >
> > Do you think I should remove it ?
>
> I think pci_alloc_vectors() should be enough. Make it to be the first
> call, if you think it's better.
>
Thanks for your answer.
I will remove the pdev->irq check an rely pci_alloc_vectors().
Not sure that I will move the call to the first call.
Best regards.
Hervé
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 2/8] MAINTAINERS: Add the Microchip LAN966x PCI driver entry
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
2024-08-05 10:17 ` [PATCH v4 1/8] misc: Add support for LAN966x PCI device Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-05 10:17 ` [PATCH v4 3/8] mfd: syscon: Add reference counting and device managed support Herve Codina
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni
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 42decde38320..bec67cbb9224 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14971,6 +14971,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.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4 3/8] mfd: syscon: Add reference counting and device managed support
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
2024-08-05 10:17 ` [PATCH v4 1/8] misc: Add support for LAN966x PCI device Herve Codina
2024-08-05 10:17 ` [PATCH v4 2/8] MAINTAINERS: Add the Microchip LAN966x PCI driver entry Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-05 20:20 ` Andy Shevchenko
2024-08-05 10:17 ` [PATCH v4 4/8] reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency Herve Codina
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni, Clément Léger
From: Clément Léger <clement.leger@bootlin.com>
Syscon releasing is not supported.
Without release function, unbinding a driver that uses syscon whether
explicitly or due to a module removal left the used syscon in a in-use
state.
For instance a syscon_node_to_regmap() call from a consumer retrieve a
syscon regmap instance. Internally, syscon_node_to_regmap() can create
syscon instance and add it to the existing syscon list. No API is
available to release this syscon instance, remove it from the list and
free it when it is not used anymore.
Introduce reference counting in syscon in order to keep track of syscon
usage using syscon_{get,put}() and add a device managed version of
syscon_regmap_lookup_by_phandle(), to automatically release the syscon
instance on the consumer removal.
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
drivers/mfd/syscon.c | 145 ++++++++++++++++++++++++++++++++++---
include/linux/mfd/syscon.h | 16 ++++
2 files changed, 152 insertions(+), 9 deletions(-)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 33f1e07ab24d..b602fa7c5c63 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -34,6 +34,7 @@ struct syscon {
struct regmap *regmap;
struct reset_control *reset;
struct list_head list;
+ struct kref refcount;
};
static const struct regmap_config syscon_regmap_config = {
@@ -147,6 +148,8 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
syscon->regmap = regmap;
syscon->np = np;
+ of_node_get(syscon->np);
+ kref_init(&syscon->refcount);
spin_lock(&syscon_list_slock);
list_add_tail(&syscon->list, &syscon_list);
@@ -168,7 +171,30 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
return ERR_PTR(ret);
}
-static struct regmap *device_node_get_regmap(struct device_node *np,
+static void syscon_free(struct kref *kref)
+{
+ struct syscon *syscon = container_of(kref, struct syscon, refcount);
+
+ spin_lock(&syscon_list_slock);
+ list_del(&syscon->list);
+ spin_unlock(&syscon_list_slock);
+
+ regmap_exit(syscon->regmap);
+ of_node_put(syscon->np);
+ kfree(syscon);
+}
+
+static void syscon_get(struct syscon *syscon)
+{
+ kref_get(&syscon->refcount);
+}
+
+static void syscon_put(struct syscon *syscon)
+{
+ kref_put(&syscon->refcount, syscon_free);
+}
+
+static struct syscon *device_node_get_syscon(struct device_node *np,
bool check_res)
{
struct syscon *entry, *syscon = NULL;
@@ -183,9 +209,23 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
spin_unlock(&syscon_list_slock);
- if (!syscon)
+ if (!syscon) {
syscon = of_syscon_register(np, check_res);
+ if (IS_ERR(syscon))
+ return ERR_CAST(syscon);
+ } else {
+ syscon_get(syscon);
+ }
+
+ return syscon;
+}
+static struct regmap *device_node_get_regmap(struct device_node *np,
+ bool check_res)
+{
+ struct syscon *syscon;
+
+ syscon = device_node_get_syscon(np, check_res);
if (IS_ERR(syscon))
return ERR_CAST(syscon);
@@ -246,12 +286,23 @@ struct regmap *device_node_to_regmap(struct device_node *np)
}
EXPORT_SYMBOL_GPL(device_node_to_regmap);
-struct regmap *syscon_node_to_regmap(struct device_node *np)
+static struct syscon *syscon_node_to_syscon(struct device_node *np)
{
if (!of_device_is_compatible(np, "syscon"))
return ERR_PTR(-EINVAL);
- return device_node_get_regmap(np, true);
+ return device_node_get_syscon(np, true);
+}
+
+struct regmap *syscon_node_to_regmap(struct device_node *np)
+{
+ struct syscon *syscon;
+
+ syscon = syscon_node_to_syscon(np);
+ if (IS_ERR(syscon))
+ return ERR_CAST(syscon);
+
+ return syscon->regmap;
}
EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
@@ -271,11 +322,11 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
-struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
- const char *property)
+static struct syscon *syscon_lookup_by_phandle(struct device_node *np,
+ const char *property)
{
struct device_node *syscon_np;
- struct regmap *regmap;
+ struct syscon *syscon;
if (property)
syscon_np = of_parse_phandle(np, property, 0);
@@ -285,12 +336,24 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
if (!syscon_np)
return ERR_PTR(-ENODEV);
- regmap = syscon_node_to_regmap(syscon_np);
+ syscon = syscon_node_to_syscon(syscon_np);
if (property)
of_node_put(syscon_np);
- return regmap;
+ return syscon;
+}
+
+struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
+ const char *property)
+{
+ struct syscon *syscon;
+
+ syscon = syscon_lookup_by_phandle(np, property);
+ if (IS_ERR(syscon))
+ return ERR_CAST(syscon);
+
+ return syscon->regmap;
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
@@ -341,6 +404,70 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
+static struct syscon *syscon_from_regmap(struct regmap *regmap)
+{
+ struct syscon *entry, *syscon = NULL;
+
+ spin_lock(&syscon_list_slock);
+
+ list_for_each_entry(entry, &syscon_list, list)
+ if (entry->regmap == regmap) {
+ syscon = entry;
+ break;
+ }
+
+ spin_unlock(&syscon_list_slock);
+
+ return syscon;
+}
+
+void syscon_put_regmap(struct regmap *regmap)
+{
+ struct syscon *syscon;
+
+ syscon = syscon_from_regmap(regmap);
+ if (!syscon)
+ return;
+
+ syscon_put(syscon);
+}
+EXPORT_SYMBOL_GPL(syscon_put_regmap);
+
+static void devm_syscon_release(struct device *dev, void *res)
+{
+ syscon_put(*(struct syscon **)res);
+}
+
+static struct regmap *__devm_syscon_get(struct device *dev,
+ struct syscon *syscon)
+{
+ struct syscon **ptr;
+
+ if (IS_ERR(syscon))
+ return ERR_CAST(syscon);
+
+ ptr = devres_alloc(devm_syscon_release, sizeof(struct syscon *), GFP_KERNEL);
+ if (!ptr) {
+ syscon_put(syscon);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ *ptr = syscon;
+ devres_add(dev, ptr);
+
+ return syscon->regmap;
+}
+
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+ struct device_node *np,
+ const char *property)
+{
+ struct syscon *syscon = syscon_lookup_by_phandle(np, property);
+
+ return __devm_syscon_get(dev, syscon);
+}
+EXPORT_SYMBOL_GPL(devm_syscon_regmap_lookup_by_phandle);
+
static int syscon_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index aad9c6b50463..a3025b4efb4a 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -15,6 +15,7 @@
#include <linux/errno.h>
struct device_node;
+struct device;
#ifdef CONFIG_MFD_SYSCON
struct regmap *device_node_to_regmap(struct device_node *np);
@@ -30,6 +31,10 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
const char *property);
int of_syscon_register_regmap(struct device_node *np,
struct regmap *regmap);
+void syscon_put_regmap(struct regmap *regmap);
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+ struct device_node *np,
+ const char *property);
#else
static inline struct regmap *device_node_to_regmap(struct device_node *np)
{
@@ -73,6 +78,17 @@ static inline int of_syscon_register_regmap(struct device_node *np,
struct regmap *regmap)
{
return -EOPNOTSUPP;
+
+static inline void syscon_put_regmap(struct regmap *regmap)
+{
+}
+
+static inline
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+ struct device_node *np,
+ const char *property)
+{
+ return NULL;
}
#endif
--
2.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/8] mfd: syscon: Add reference counting and device managed support
2024-08-05 10:17 ` [PATCH v4 3/8] mfd: syscon: Add reference counting and device managed support Herve Codina
@ 2024-08-05 20:20 ` Andy Shevchenko
2024-08-07 13:29 ` Herve Codina
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2024-08-05 20:20 UTC (permalink / raw)
To: Herve Codina
Cc: Geert Uytterhoeven, Simon Horman, Lee Jones, Arnd Bergmann,
Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman, Bjorn Helgaas,
Philipp Zabel, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, Rob Herring, Saravana Kannan, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Horatiu Vultur,
Andrew Lunn, linux-kernel, netdev, linux-pci, linux-arm-kernel,
devicetree, Allan Nielsen, Luca Ceresoli, Thomas Petazzoni,
Clément Léger
On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
>
> From: Clément Léger <clement.leger@bootlin.com>
>
> Syscon releasing is not supported.
> Without release function, unbinding a driver that uses syscon whether
> explicitly or due to a module removal left the used syscon in a in-use
> state.
>
> For instance a syscon_node_to_regmap() call from a consumer retrieve a
retrieves?
> syscon regmap instance. Internally, syscon_node_to_regmap() can create
> syscon instance and add it to the existing syscon list. No API is
> available to release this syscon instance, remove it from the list and
> free it when it is not used anymore.
>
> Introduce reference counting in syscon in order to keep track of syscon
> usage using syscon_{get,put}() and add a device managed version of
> syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> instance on the consumer removal.
...
> - if (!syscon)
> + if (!syscon) {
> syscon = of_syscon_register(np, check_res);
> + if (IS_ERR(syscon))
> + return ERR_CAST(syscon);
> + } else {
> + syscon_get(syscon);
> + }
if (syscon)
return syscon_get();
?
> + return syscon;
...
> +static struct regmap *__devm_syscon_get(struct device *dev,
> + struct syscon *syscon)
> +{
> + struct syscon **ptr;
> +
> + if (IS_ERR(syscon))
> + return ERR_CAST(syscon);
> +
> + ptr = devres_alloc(devm_syscon_release, sizeof(struct syscon *), GFP_KERNEL);
> + if (!ptr) {
> + syscon_put(syscon);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + *ptr = syscon;
> + devres_add(dev, ptr);
> +
> + return syscon->regmap;
Can't the devm_add_action_or_reset() be used in this case? If so,
perhaps a comment to explain why?
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/8] mfd: syscon: Add reference counting and device managed support
2024-08-05 20:20 ` Andy Shevchenko
@ 2024-08-07 13:29 ` Herve Codina
0 siblings, 0 replies; 19+ messages in thread
From: Herve Codina @ 2024-08-07 13:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Geert Uytterhoeven, Simon Horman, Lee Jones, Arnd Bergmann,
Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman, Bjorn Helgaas,
Philipp Zabel, Lars Povlsen, Steen Hegelund, Daniel Machon,
UNGLinuxDriver, Rob Herring, Saravana Kannan, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Horatiu Vultur,
Andrew Lunn, linux-kernel, netdev, linux-pci, linux-arm-kernel,
devicetree, Allan Nielsen, Luca Ceresoli, Thomas Petazzoni,
Clément Léger
Hi Andy,
On Mon, 5 Aug 2024 22:20:56 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > From: Clément Léger <clement.leger@bootlin.com>
> >
> > Syscon releasing is not supported.
> > Without release function, unbinding a driver that uses syscon whether
> > explicitly or due to a module removal left the used syscon in a in-use
> > state.
> >
> > For instance a syscon_node_to_regmap() call from a consumer retrieve a
>
> retrieves?
Indeed, will be fixed.
>
> > syscon regmap instance. Internally, syscon_node_to_regmap() can create
> > syscon instance and add it to the existing syscon list. No API is
> > available to release this syscon instance, remove it from the list and
> > free it when it is not used anymore.
> >
> > Introduce reference counting in syscon in order to keep track of syscon
> > usage using syscon_{get,put}() and add a device managed version of
> > syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> > instance on the consumer removal.
>
> ...
>
> > - if (!syscon)
> > + if (!syscon) {
> > syscon = of_syscon_register(np, check_res);
> > + if (IS_ERR(syscon))
> > + return ERR_CAST(syscon);
> > + } else {
> > + syscon_get(syscon);
> > + }
>
> if (syscon)
> return syscon_get();
>
> ?
>
> > + return syscon;
Yes and further more, I will remove also the unneeded IS_ERR() and ERR_CAST().
This will lead to just:
if (syscon)
return syscon_get(syscon);
return of_syscon_register(np, check_res);
>
> ...
>
> > +static struct regmap *__devm_syscon_get(struct device *dev,
> > + struct syscon *syscon)
> > +{
> > + struct syscon **ptr;
> > +
> > + if (IS_ERR(syscon))
> > + return ERR_CAST(syscon);
> > +
> > + ptr = devres_alloc(devm_syscon_release, sizeof(struct syscon *), GFP_KERNEL);
> > + if (!ptr) {
> > + syscon_put(syscon);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + *ptr = syscon;
> > + devres_add(dev, ptr);
> > +
> > + return syscon->regmap;
>
> Can't the devm_add_action_or_reset() be used in this case? If so,
> perhaps a comment to explain why?
There is no reason to avoid the use of devm_add_action_or_reset() here.
So, I will use it in the next iteration.
Thanks for your review.
Best regards,
Hervé
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 4/8] reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
` (2 preceding siblings ...)
2024-08-05 10:17 ` [PATCH v4 3/8] mfd: syscon: Add reference counting and device managed support Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-07 7:57 ` Steen Hegelund
2024-08-05 10:17 ` [PATCH v4 5/8] reset: mchp: sparx5: Allow building as a module Herve Codina
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni
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>
---
drivers/reset/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 67bce340a87e..5b5a4d99616e 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -134,7 +134,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.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 4/8] reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency
2024-08-05 10:17 ` [PATCH v4 4/8] reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency Herve Codina
@ 2024-08-07 7:57 ` Steen Hegelund
0 siblings, 0 replies; 19+ messages in thread
From: Steen Hegelund @ 2024-08-07 7:57 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven, Andy Shevchenko, Simon Horman
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Luca Ceresoli,
Thomas Petazzoni
Hi Herve,
On Mon, 2024-08-05 at 12:17 +0200, Herve Codina wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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>
> ---
> drivers/reset/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 67bce340a87e..5b5a4d99616e 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -134,7 +134,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.45.0
>
Looks good to me.
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
BR
Steen
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 5/8] reset: mchp: sparx5: Allow building as a module
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
` (3 preceding siblings ...)
2024-08-05 10:17 ` [PATCH v4 4/8] reset: mchp: sparx5: Add MCHP_LAN966X_PCI dependency Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-07 9:14 ` Steen Hegelund
2024-08-05 10:17 ` [PATCH v4 6/8] reset: mchp: sparx5: Release syscon when not use anymore Herve Codina
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni, Clément Léger
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>
---
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 5b5a4d99616e..88350aa8a51c 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -133,7 +133,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 636e85c388b0..69915c7b4941 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -158,6 +158,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,
@@ -180,3 +181,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.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 5/8] reset: mchp: sparx5: Allow building as a module
2024-08-05 10:17 ` [PATCH v4 5/8] reset: mchp: sparx5: Allow building as a module Herve Codina
@ 2024-08-07 9:14 ` Steen Hegelund
0 siblings, 0 replies; 19+ messages in thread
From: Steen Hegelund @ 2024-08-07 9:14 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven, Andy Shevchenko, Simon Horman,
Lee Jones, Arnd Bergmann, Derek Kiernan, Dragan Cvetic,
Greg Kroah-Hartman, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Daniel Machon, UNGLinuxDriver, Rob Herring, Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Luca Ceresoli,
Thomas Petazzoni, Clément Léger
Hi Herve,
On Mon, 2024-08-05 at 12:17 +0200, Herve Codina wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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>
> ---
> 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 5b5a4d99616e..88350aa8a51c 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -133,7 +133,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 636e85c388b0..69915c7b4941 100644
> --- a/drivers/reset/reset-microchip-sparx5.c
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -158,6 +158,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,
> @@ -180,3 +181,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.45.0
>
Looks good to me.
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
BR
Steen
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 6/8] reset: mchp: sparx5: Release syscon when not use anymore
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
` (4 preceding siblings ...)
2024-08-05 10:17 ` [PATCH v4 5/8] reset: mchp: sparx5: Allow building as a module Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-07 9:48 ` Steen Hegelund
2024-08-05 10:17 ` [PATCH v4 7/8] reset: core: add get_device()/put_device on rcdev Herve Codina
2024-08-05 10:17 ` [PATCH v4 8/8] reset: mchp: sparx5: set the dev member of the reset controller Herve Codina
7 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni, Clément Léger
From: Clément Léger <clement.leger@bootlin.com>
The sparx5 reset controller does not release syscon when it is not used
anymore.
This reset controller is used by the LAN966x PCI device driver.
It can be removed from the system at runtime and needs to release its
consumed syscon on removal.
Use the newly introduced devm_syscon_regmap_lookup_by_phandle() in order
to get the syscon and automatically release it on removal.
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
drivers/reset/reset-microchip-sparx5.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 69915c7b4941..c4fe65291a43 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -65,15 +65,11 @@ static const struct reset_control_ops sparx5_reset_ops = {
static int mchp_sparx5_map_syscon(struct platform_device *pdev, char *name,
struct regmap **target)
{
- struct device_node *syscon_np;
+ struct device *dev = &pdev->dev;
struct regmap *regmap;
int err;
- syscon_np = of_parse_phandle(pdev->dev.of_node, name, 0);
- if (!syscon_np)
- return -ENODEV;
- regmap = syscon_node_to_regmap(syscon_np);
- of_node_put(syscon_np);
+ regmap = devm_syscon_regmap_lookup_by_phandle(dev, dev->of_node, name);
if (IS_ERR(regmap)) {
err = PTR_ERR(regmap);
dev_err(&pdev->dev, "No '%s' map: %d\n", name, err);
--
2.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 6/8] reset: mchp: sparx5: Release syscon when not use anymore
2024-08-05 10:17 ` [PATCH v4 6/8] reset: mchp: sparx5: Release syscon when not use anymore Herve Codina
@ 2024-08-07 9:48 ` Steen Hegelund
0 siblings, 0 replies; 19+ messages in thread
From: Steen Hegelund @ 2024-08-07 9:48 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven, Andy Shevchenko, Simon Horman,
Lee Jones, Arnd Bergmann, Derek Kiernan, Dragan Cvetic,
Greg Kroah-Hartman, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Daniel Machon, UNGLinuxDriver, Rob Herring, Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Luca Ceresoli,
Thomas Petazzoni, Clément Léger
Hi Herve,
On Mon, 2024-08-05 at 12:17 +0200, Herve Codina wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> From: Clément Léger <clement.leger@bootlin.com>
>
> The sparx5 reset controller does not release syscon when it is not
> used
> anymore.
>
> This reset controller is used by the LAN966x PCI device driver.
> It can be removed from the system at runtime and needs to release its
> consumed syscon on removal.
>
> Use the newly introduced devm_syscon_regmap_lookup_by_phandle() in
> order
> to get the syscon and automatically release it on removal.
>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> drivers/reset/reset-microchip-sparx5.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/reset/reset-microchip-sparx5.c
> b/drivers/reset/reset-microchip-sparx5.c
> index 69915c7b4941..c4fe65291a43 100644
> --- a/drivers/reset/reset-microchip-sparx5.c
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -65,15 +65,11 @@ static const struct reset_control_ops
> sparx5_reset_ops = {
> static int mchp_sparx5_map_syscon(struct platform_device *pdev, char
> *name,
> struct regmap **target)
> {
> - struct device_node *syscon_np;
> + struct device *dev = &pdev->dev;
> struct regmap *regmap;
> int err;
>
> - syscon_np = of_parse_phandle(pdev->dev.of_node, name, 0);
> - if (!syscon_np)
> - return -ENODEV;
> - regmap = syscon_node_to_regmap(syscon_np);
> - of_node_put(syscon_np);
> + regmap = devm_syscon_regmap_lookup_by_phandle(dev, dev-
> >of_node, name);
> if (IS_ERR(regmap)) {
> err = PTR_ERR(regmap);
> dev_err(&pdev->dev, "No '%s' map: %d\n", name, err);
> --
> 2.45.0
>
Looks good to me.
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
BR
Steen
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 7/8] reset: core: add get_device()/put_device on rcdev
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
` (5 preceding siblings ...)
2024-08-05 10:17 ` [PATCH v4 6/8] reset: mchp: sparx5: Release syscon when not use anymore Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-05 10:17 ` [PATCH v4 8/8] reset: mchp: sparx5: set the dev member of the reset controller Herve Codina
7 siblings, 0 replies; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni, Clément Léger
From: Clément Léger <clement.leger@bootlin.com>
Since the rcdev structure is allocated by the reset controller drivers
themselves, they need to exists as long as there is a consumer. A call to
module_get() is already existing but that does not work when using
device-tree overlays. In order to guarantee that the underlying reset
controller device does not vanish while using it, add a get_device() call
when retrieving a reset control from a reset controller device and a
put_device() when releasing that control.
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
drivers/reset/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index dba74e857be6..999c3c41cf21 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -812,6 +812,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
kref_init(&rstc->refcnt);
rstc->acquired = acquired;
rstc->shared = shared;
+ get_device(rcdev->dev);
return rstc;
}
@@ -826,6 +827,7 @@ static void __reset_control_release(struct kref *kref)
module_put(rstc->rcdev->owner);
list_del(&rstc->list);
+ put_device(rstc->rcdev->dev);
kfree(rstc);
}
--
2.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4 8/8] reset: mchp: sparx5: set the dev member of the reset controller
2024-08-05 10:17 [PATCH v4 0/8] Add support for the LAN966x PCI device using a DT overlay Herve Codina
` (6 preceding siblings ...)
2024-08-05 10:17 ` [PATCH v4 7/8] reset: core: add get_device()/put_device on rcdev Herve Codina
@ 2024-08-05 10:17 ` Herve Codina
2024-08-08 8:22 ` Steen Hegelund
7 siblings, 1 reply; 19+ messages in thread
From: Herve Codina @ 2024-08-05 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Andy Shevchenko, Simon Horman, Lee Jones,
Arnd Bergmann, Derek Kiernan, Dragan Cvetic, Greg Kroah-Hartman,
Herve Codina, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Steen Hegelund, Daniel Machon, UNGLinuxDriver, Rob Herring,
Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Steen Hegelund,
Luca Ceresoli, Thomas Petazzoni, Clément Léger
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>
---
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 c4fe65291a43..1ef2aa1602e3 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -117,6 +117,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.45.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 8/8] reset: mchp: sparx5: set the dev member of the reset controller
2024-08-05 10:17 ` [PATCH v4 8/8] reset: mchp: sparx5: set the dev member of the reset controller Herve Codina
@ 2024-08-08 8:22 ` Steen Hegelund
0 siblings, 0 replies; 19+ messages in thread
From: Steen Hegelund @ 2024-08-08 8:22 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven, Andy Shevchenko, Simon Horman,
Lee Jones, Arnd Bergmann, Derek Kiernan, Dragan Cvetic,
Greg Kroah-Hartman, Bjorn Helgaas, Philipp Zabel, Lars Povlsen,
Daniel Machon, UNGLinuxDriver, Rob Herring, Saravana Kannan
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Horatiu Vultur, Andrew Lunn, linux-kernel, netdev, linux-pci,
linux-arm-kernel, devicetree, Allan Nielsen, Luca Ceresoli,
Thomas Petazzoni, Clément Léger
Hi Herve,
On Mon, 2024-08-05 at 12:17 +0200, Herve Codina wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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>
> ---
> 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 c4fe65291a43..1ef2aa1602e3 100644
> --- a/drivers/reset/reset-microchip-sparx5.c
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -117,6 +117,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.45.0
>
Looks good to me.
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
BR
Steen
^ permalink raw reply [flat|nested] 19+ messages in thread