2021-07-26 05:24:33

by Maxim Kochetkov

[permalink] [raw]
Subject: [PATCH v3] soc: fsl: qe: convert QE interrupt controller to platform_device

Since 5.13 QE's ucc nodes can't get interrupts from devicetree:

ucc@2000 {
cell-index = <1>;
reg = <0x2000 0x200>;
interrupts = <32>;
interrupt-parent = <&qeic>;
};

Now fw_devlink expects driver to create and probe a struct device
for interrupt controller.

So lets convert this driver to simple platform_device with probe().
Also use platform_get_ and devm_ family function to get/allocate
resources and drop unused .compatible = "qeic".

[1] - https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com
Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Fixes: ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")
Signed-off-by: Maxim Kochetkov <[email protected]>
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
---
Changes in v3:
- use .compatible = "qeic" again (Li Yang <[email protected]> asks to keep it)

Changes in v2:
- use devm_ family functions to allocate mem/resources
- use platform_get_ family functions to get resources/irqs
- drop unused .compatible = "qeic"

drivers/soc/fsl/qe/qe_ic.c | 75 ++++++++++++++++++++++----------------
1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c
index 3f711c1a0996..54cabd2605dd 100644
--- a/drivers/soc/fsl/qe/qe_ic.c
+++ b/drivers/soc/fsl/qe/qe_ic.c
@@ -23,6 +23,7 @@
#include <linux/signal.h>
#include <linux/device.h>
#include <linux/spinlock.h>
+#include <linux/platform_device.h>
#include <asm/irq.h>
#include <asm/io.h>
#include <soc/fsl/qe/qe.h>
@@ -404,41 +405,40 @@ static void qe_ic_cascade_muxed_mpic(struct irq_desc *desc)
chip->irq_eoi(&desc->irq_data);
}

-static void __init qe_ic_init(struct device_node *node)
+static int qe_ic_init(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
void (*low_handler)(struct irq_desc *desc);
void (*high_handler)(struct irq_desc *desc);
struct qe_ic *qe_ic;
- struct resource res;
- u32 ret;
+ struct resource *res;
+ struct device_node *node = pdev->dev.of_node;

- ret = of_address_to_resource(node, 0, &res);
- if (ret)
- return;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(dev, "no memory resource defined\n");
+ return -ENODEV;
+ }

- qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL);
+ qe_ic = devm_kzalloc(dev, sizeof(*qe_ic), GFP_KERNEL);
if (qe_ic == NULL)
- return;
+ return -ENOMEM;

- qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
- &qe_ic_host_ops, qe_ic);
- if (qe_ic->irqhost == NULL) {
- kfree(qe_ic);
- return;
+ qe_ic->regs = devm_ioremap(dev, res->start, resource_size(res));
+ if (qe_ic->regs == NULL) {
+ dev_err(dev, "failed to ioremap() registers\n");
+ return -ENODEV;
}

- qe_ic->regs = ioremap(res.start, resource_size(&res));
-
qe_ic->hc_irq = qe_ic_irq_chip;

- qe_ic->virq_high = irq_of_parse_and_map(node, 0);
- qe_ic->virq_low = irq_of_parse_and_map(node, 1);
+ qe_ic->virq_high = platform_get_irq(pdev, 0);
+ qe_ic->virq_low = platform_get_irq(pdev, 1);

- if (!qe_ic->virq_low) {
- printk(KERN_ERR "Failed to map QE_IC low IRQ\n");
- kfree(qe_ic);
- return;
+ if (qe_ic->virq_low < 0) {
+ return -ENODEV;
}
+
if (qe_ic->virq_high != qe_ic->virq_low) {
low_handler = qe_ic_cascade_low;
high_handler = qe_ic_cascade_high;
@@ -447,6 +447,13 @@ static void __init qe_ic_init(struct device_node *node)
high_handler = NULL;
}

+ qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
+ &qe_ic_host_ops, qe_ic);
+ if (qe_ic->irqhost == NULL) {
+ dev_err(dev, "failed to add irq domain\n");
+ return -ENODEV;
+ }
+
qe_ic_write(qe_ic->regs, QEIC_CICR, 0);

irq_set_handler_data(qe_ic->virq_low, qe_ic);
@@ -456,20 +463,26 @@ static void __init qe_ic_init(struct device_node *node)
irq_set_handler_data(qe_ic->virq_high, qe_ic);
irq_set_chained_handler(qe_ic->virq_high, high_handler);
}
+ return 0;
}
+static const struct of_device_id qe_ic_ids[] = {
+ { .compatible = "fsl,qe-ic"},
+ { .compatible = "qeic"},
+ {},
+};

-static int __init qe_ic_of_init(void)
+static struct platform_driver qe_ic_driver =
{
- struct device_node *np;
+ .driver = {
+ .name = "qe-ic",
+ .of_match_table = qe_ic_ids,
+ },
+ .probe = qe_ic_init,
+};

- np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
- if (!np) {
- np = of_find_node_by_type(NULL, "qeic");
- if (!np)
- return -ENODEV;
- }
- qe_ic_init(np);
- of_node_put(np);
+static int __init qe_ic_of_init(void)
+{
+ platform_driver_register(&qe_ic_driver);
return 0;
}
subsys_initcall(qe_ic_of_init);
--
2.31.1


2021-07-26 20:48:24

by Leo Li

[permalink] [raw]
Subject: RE: [PATCH v3] soc: fsl: qe: convert QE interrupt controller to platform_device



> -----Original Message-----
> From: Maxim Kochetkov <[email protected]>
> Sent: Monday, July 26, 2021 12:22 AM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Leo Li <[email protected]>; Qiang Zhao
> <[email protected]>; [email protected]; Maxim Kochetkov
> <[email protected]>; kernel test robot <[email protected]>; Dan Carpenter
> <[email protected]>
> Subject: [PATCH v3] soc: fsl: qe: convert QE interrupt controller to
> platform_device
>
> Since 5.13 QE's ucc nodes can't get interrupts from devicetree:
>
> ucc@2000 {
> cell-index = <1>;
> reg = <0x2000 0x200>;
> interrupts = <32>;
> interrupt-parent = <&qeic>;
> };
>
> Now fw_devlink expects driver to create and probe a struct device for
> interrupt controller.
>
> So lets convert this driver to simple platform_device with probe().
> Also use platform_get_ and devm_ family function to get/allocate resources
> and drop unused .compatible = "qeic".
>
> [1] -
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Flkml%2FCAGETcx9PiX%3D%3DmLxB9PO8Myyk6u2vhPVwTMsA
> 5NkD-
> ywH5xhusw%40mail.gmail.com&amp;data=04%7C01%7Cleoyang.li%40nxp.co
> m%7C6e64e4b86f2d4a89390808d94ff50bec%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C637628736153046082%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C1000&amp;sdata=G7HhGFmRLvMyNMULSddWctD3HhtVWMfZAxPXjl8
> CBTY%3D&amp;reserved=0
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Fixes: ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by
> default""")
> Signed-off-by: Maxim Kochetkov <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> ---
> Changes in v3:
> - use .compatible = "qeic" again (Li Yang <[email protected]> asks to keep
> it)
>
> Changes in v2:
> - use devm_ family functions to allocate mem/resources
> - use platform_get_ family functions to get resources/irqs
> - drop unused .compatible = "qeic"
>
> drivers/soc/fsl/qe/qe_ic.c | 75 ++++++++++++++++++++++----------------
> 1 file changed, 44 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c index
> 3f711c1a0996..54cabd2605dd 100644
> --- a/drivers/soc/fsl/qe/qe_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ic.c
> @@ -23,6 +23,7 @@
> #include <linux/signal.h>
> #include <linux/device.h>
> #include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> #include <asm/irq.h>
> #include <asm/io.h>
> #include <soc/fsl/qe/qe.h>
> @@ -404,41 +405,40 @@ static void qe_ic_cascade_muxed_mpic(struct
> irq_desc *desc)
> chip->irq_eoi(&desc->irq_data);
> }
>
> -static void __init qe_ic_init(struct device_node *node)
> +static int qe_ic_init(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> void (*low_handler)(struct irq_desc *desc);
> void (*high_handler)(struct irq_desc *desc);
> struct qe_ic *qe_ic;
> - struct resource res;
> - u32 ret;
> + struct resource *res;
> + struct device_node *node = pdev->dev.of_node;
>
> - ret = of_address_to_resource(node, 0, &res);
> - if (ret)
> - return;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(dev, "no memory resource defined\n");
> + return -ENODEV;
> + }
>
> - qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL);
> + qe_ic = devm_kzalloc(dev, sizeof(*qe_ic), GFP_KERNEL);
> if (qe_ic == NULL)
> - return;
> + return -ENOMEM;
>
> - qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
> - &qe_ic_host_ops, qe_ic);
> - if (qe_ic->irqhost == NULL) {
> - kfree(qe_ic);
> - return;
> + qe_ic->regs = devm_ioremap(dev, res->start, resource_size(res));
> + if (qe_ic->regs == NULL) {
> + dev_err(dev, "failed to ioremap() registers\n");
> + return -ENODEV;
> }
>
> - qe_ic->regs = ioremap(res.start, resource_size(&res));
> -
> qe_ic->hc_irq = qe_ic_irq_chip;
>
> - qe_ic->virq_high = irq_of_parse_and_map(node, 0);
> - qe_ic->virq_low = irq_of_parse_and_map(node, 1);
> + qe_ic->virq_high = platform_get_irq(pdev, 0);
> + qe_ic->virq_low = platform_get_irq(pdev, 1);
>
> - if (!qe_ic->virq_low) {
> - printk(KERN_ERR "Failed to map QE_IC low IRQ\n");
> - kfree(qe_ic);
> - return;
> + if (qe_ic->virq_low < 0) {
> + return -ENODEV;
> }
> +
> if (qe_ic->virq_high != qe_ic->virq_low) {
> low_handler = qe_ic_cascade_low;
> high_handler = qe_ic_cascade_high;
> @@ -447,6 +447,13 @@ static void __init qe_ic_init(struct device_node
> *node)
> high_handler = NULL;
> }
>
> + qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
> + &qe_ic_host_ops, qe_ic);
> + if (qe_ic->irqhost == NULL) {
> + dev_err(dev, "failed to add irq domain\n");
> + return -ENODEV;
> + }
> +
> qe_ic_write(qe_ic->regs, QEIC_CICR, 0);
>
> irq_set_handler_data(qe_ic->virq_low, qe_ic); @@ -456,20 +463,26
> @@ static void __init qe_ic_init(struct device_node *node)
> irq_set_handler_data(qe_ic->virq_high, qe_ic);
> irq_set_chained_handler(qe_ic->virq_high, high_handler);
> }
> + return 0;
> }
> +static const struct of_device_id qe_ic_ids[] = {
> + { .compatible = "fsl,qe-ic"},
> + { .compatible = "qeic"},

Probably you missed the point from my first response. The original code is match by device type "qeic" instead of compatible string "qeic". So we should use { .type = "qeic"} here.

> + {},
> +};
>
> -static int __init qe_ic_of_init(void)
> +static struct platform_driver qe_ic_driver =
> {
> - struct device_node *np;
> + .driver = {
> + .name = "qe-ic",
> + .of_match_table = qe_ic_ids,
> + },
> + .probe = qe_ic_init,
> +};
>
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> - if (!np) {
> - np = of_find_node_by_type(NULL, "qeic");
> - if (!np)
> - return -ENODEV;
> - }
> - qe_ic_init(np);
> - of_node_put(np);
> +static int __init qe_ic_of_init(void)
> +{
> + platform_driver_register(&qe_ic_driver);
> return 0;
> }
> subsys_initcall(qe_ic_of_init);
> --
> 2.31.1