* [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip @ 2016-07-05 1:46 Zhao Qiang 2016-07-05 1:46 ` [PATCH 2/2] qe/ic: refactor qe_ic to simplify Zhao Qiang 2016-07-05 3:18 ` [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Jason Cooper 0 siblings, 2 replies; 9+ messages in thread From: Zhao Qiang @ 2016-07-05 1:46 UTC (permalink / raw) To: oss, tglx, jason, marc.zyngier Cc: linuxppc-dev, linux-kernel, xiaobo.xie, Zhao Qiang The codes of qe_ic_init in platforms are redundant, move them to qe_ic under irqchip Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> --- arch/powerpc/platforms/83xx/misc.c | 15 --------------- arch/powerpc/platforms/85xx/corenet_generic.c | 9 --------- arch/powerpc/platforms/85xx/mpc85xx_mds.c | 14 -------------- arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 16 ---------------- arch/powerpc/platforms/85xx/twr_p102x.c | 14 -------------- drivers/irqchip/qe_ic.c | 14 ++++++++++++++ 6 files changed, 14 insertions(+), 68 deletions(-) diff --git a/arch/powerpc/platforms/83xx/misc.c b/arch/powerpc/platforms/83xx/misc.c index 7e923ca..9431fc7 100644 --- a/arch/powerpc/platforms/83xx/misc.c +++ b/arch/powerpc/platforms/83xx/misc.c @@ -93,24 +93,9 @@ void __init mpc83xx_ipic_init_IRQ(void) } #ifdef CONFIG_QUICC_ENGINE -void __init mpc83xx_qe_init_IRQ(void) -{ - struct device_node *np; - - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (!np) { - np = of_find_node_by_type(NULL, "qeic"); - if (!np) - return; - } - qe_ic_init(np, 0, qe_ic_cascade_low_ipic, qe_ic_cascade_high_ipic); - of_node_put(np); -} - void __init mpc83xx_ipic_and_qe_init_IRQ(void) { mpc83xx_ipic_init_IRQ(); - mpc83xx_qe_init_IRQ(); } #endif /* CONFIG_QUICC_ENGINE */ diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c index a2b0bc8..526fc2b 100644 --- a/arch/powerpc/platforms/85xx/corenet_generic.c +++ b/arch/powerpc/platforms/85xx/corenet_generic.c @@ -41,8 +41,6 @@ void __init corenet_gen_pic_init(void) unsigned int flags = MPIC_BIG_ENDIAN | MPIC_SINGLE_DEST_CPU | MPIC_NO_RESET; - struct device_node *np; - if (ppc_md.get_irq == mpic_get_coreint_irq) flags |= MPIC_ENABLE_COREINT; @@ -50,13 +48,6 @@ void __init corenet_gen_pic_init(void) BUG_ON(mpic == NULL); mpic_init(mpic); - - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (np) { - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, - qe_ic_cascade_high_mpic); - of_node_put(np); - } } /* diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c index f61cbe2..7ae4901 100644 --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c @@ -279,20 +279,6 @@ static void __init mpc85xx_mds_qeic_init(void) of_node_put(np); return; } - - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (!np) { - np = of_find_node_by_type(NULL, "qeic"); - if (!np) - return; - } - - if (machine_is(p1021_mds)) - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, - qe_ic_cascade_high_mpic); - else - qe_ic_init(np, 0, qe_ic_cascade_muxed_mpic, NULL); - of_node_put(np); } #else static void __init mpc85xx_mds_qe_init(void) { } diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c index 3f4dad1..779f54f 100644 --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c @@ -49,10 +49,6 @@ void __init mpc85xx_rdb_pic_init(void) struct mpic *mpic; unsigned long root = of_get_flat_dt_root(); -#ifdef CONFIG_QUICC_ENGINE - struct device_node *np; -#endif - if (of_flat_dt_is_compatible(root, "fsl,MPC85XXRDB-CAMP")) { mpic = mpic_alloc(NULL, 0, MPIC_NO_RESET | MPIC_BIG_ENDIAN | @@ -67,18 +63,6 @@ void __init mpc85xx_rdb_pic_init(void) BUG_ON(mpic == NULL); mpic_init(mpic); - -#ifdef CONFIG_QUICC_ENGINE - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (np) { - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, - qe_ic_cascade_high_mpic); - of_node_put(np); - - } else - pr_err("%s: Could not find qe-ic node\n", __func__); -#endif - } /* diff --git a/arch/powerpc/platforms/85xx/twr_p102x.c b/arch/powerpc/platforms/85xx/twr_p102x.c index 71bc255..603e244 100644 --- a/arch/powerpc/platforms/85xx/twr_p102x.c +++ b/arch/powerpc/platforms/85xx/twr_p102x.c @@ -35,26 +35,12 @@ static void __init twr_p1025_pic_init(void) { struct mpic *mpic; -#ifdef CONFIG_QUICC_ENGINE - struct device_node *np; -#endif - mpic = mpic_alloc(NULL, 0, MPIC_BIG_ENDIAN | MPIC_SINGLE_DEST_CPU, 0, 256, " OpenPIC "); BUG_ON(mpic == NULL); mpic_init(mpic); - -#ifdef CONFIG_QUICC_ENGINE - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (np) { - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, - qe_ic_cascade_high_mpic); - of_node_put(np); - } else - pr_err("Could not find qe-ic node\n"); -#endif } /* ************************************************************************ diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index ec2ca86..f7f9a81 100644 --- a/drivers/irqchip/qe_ic.c +++ b/drivers/irqchip/qe_ic.c @@ -509,4 +509,18 @@ static int __init init_qe_ic_sysfs(void) return 0; } +static int __init qeic_of_init(void) +{ + struct device_node *np; + + np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); + if (np) { + qe_ic_init(np, 0, qe_ic_cascade_low_mpic, + qe_ic_cascade_high_mpic); + of_node_put(np); + } + return 0; +} + +subsys_initcall(qeic_of_init); subsys_initcall(init_qe_ic_sysfs); -- 2.1.0.27.g96db324 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] qe/ic: refactor qe_ic to simplify 2016-07-05 1:46 [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Zhao Qiang @ 2016-07-05 1:46 ` Zhao Qiang 2016-07-05 3:51 ` Jason Cooper 2016-07-05 3:18 ` [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Jason Cooper 1 sibling, 1 reply; 9+ messages in thread From: Zhao Qiang @ 2016-07-05 1:46 UTC (permalink / raw) To: oss, tglx, jason, marc.zyngier Cc: linuxppc-dev, linux-kernel, xiaobo.xie, Zhao Qiang there are init_qe_ic_sysfs and qeic_of_init, refactor them. Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> --- drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++--------------------- include/soc/fsl/qe/qe_ic.h | 7 ---- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index f7f9a81..46652c0 100644 --- a/drivers/irqchip/qe_ic.c +++ b/drivers/irqchip/qe_ic.c @@ -317,27 +317,35 @@ unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic) return irq_linear_revmap(qe_ic->irqhost, irq); } -void __init qe_ic_init(struct device_node *node, unsigned int flags, - void (*low_handler)(struct irq_desc *desc), - void (*high_handler)(struct irq_desc *desc)) +static int __init qe_ic_init(unsigned int flags) { + struct device_node *node; struct qe_ic *qe_ic; struct resource res; - u32 temp = 0, ret, high_active = 0; + u32 temp = 0, high_active = 0; + int ret = 0; + + node = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); + if (!node) + return -ENODEV; ret = of_address_to_resource(node, 0, &res); - if (ret) - return; + if (ret) { + ret = -ENODEV; + goto err_put_node; + } qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL); - if (qe_ic == NULL) - return; + if (qe_ic == NULL) { + ret = -ENOMEM; + goto err_put_node; + } 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; + ret = -ENOMEM; + goto err_free_qe_ic; } qe_ic->regs = ioremap(res.start, resource_size(&res)); @@ -348,9 +356,9 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, qe_ic->virq_low = irq_of_parse_and_map(node, 1); if (qe_ic->virq_low == NO_IRQ) { - printk(KERN_ERR "Failed to map QE_IC low IRQ\n"); - kfree(qe_ic); - return; + pr_err("Failed to map QE_IC low IRQ\n"); + ret = -ENOMEM; + goto err_domain_remove; } /* default priority scheme is grouped. If spread mode is */ @@ -377,13 +385,23 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, qe_ic_write(qe_ic->regs, QEIC_CICR, temp); irq_set_handler_data(qe_ic->virq_low, qe_ic); - irq_set_chained_handler(qe_ic->virq_low, low_handler); + irq_set_chained_handler(qe_ic->virq_low, qe_ic_cascade_low_mpic); if (qe_ic->virq_high != NO_IRQ && qe_ic->virq_high != qe_ic->virq_low) { irq_set_handler_data(qe_ic->virq_high, qe_ic); - irq_set_chained_handler(qe_ic->virq_high, high_handler); + irq_set_chained_handler(qe_ic->virq_high, + qe_ic_cascade_high_mpic); } + return ret; + +err_domain_remove: + irq_domain_remove(qe_ic->irqhost); +err_free_qe_ic: + kfree(qe_ic); +err_put_node: + of_node_put(node); + return ret; } void qe_ic_set_highest_priority(unsigned int virq, int high) @@ -490,37 +508,26 @@ static struct device device_qe_ic = { .bus = &qe_ic_subsys, }; -static int __init init_qe_ic_sysfs(void) +static int __init init_qe_ic(void) { - int rc; + int ret; - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); + ret = qe_ic_init(0); + if (ret) + return ret; - rc = subsys_system_register(&qe_ic_subsys, NULL); - if (rc) { - printk(KERN_ERR "Failed registering qe_ic sys class\n"); + ret = subsys_system_register(&qe_ic_subsys, NULL); + if (ret) { + pr_err("Failed registering qe_ic sys class\n"); return -ENODEV; } - rc = device_register(&device_qe_ic); - if (rc) { - printk(KERN_ERR "Failed registering qe_ic sys device\n"); + ret = device_register(&device_qe_ic); + if (ret) { + pr_err("Failed registering qe_ic sys device\n"); return -ENODEV; } - return 0; -} -static int __init qeic_of_init(void) -{ - struct device_node *np; - - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); - if (np) { - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, - qe_ic_cascade_high_mpic); - of_node_put(np); - } return 0; } -subsys_initcall(qeic_of_init); -subsys_initcall(init_qe_ic_sysfs); +subsys_initcall(init_qe_ic); diff --git a/include/soc/fsl/qe/qe_ic.h b/include/soc/fsl/qe/qe_ic.h index 1e155ca..6113699 100644 --- a/include/soc/fsl/qe/qe_ic.h +++ b/include/soc/fsl/qe/qe_ic.h @@ -58,16 +58,9 @@ enum qe_ic_grp_id { }; #ifdef CONFIG_QUICC_ENGINE -void qe_ic_init(struct device_node *node, unsigned int flags, - void (*low_handler)(struct irq_desc *desc), - void (*high_handler)(struct irq_desc *desc)); unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic); unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic); #else -static inline void qe_ic_init(struct device_node *node, unsigned int flags, - void (*low_handler)(struct irq_desc *desc), - void (*high_handler)(struct irq_desc *desc)) -{} static inline unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic) { return 0; } static inline unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic) -- 2.1.0.27.g96db324 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] qe/ic: refactor qe_ic to simplify 2016-07-05 1:46 ` [PATCH 2/2] qe/ic: refactor qe_ic to simplify Zhao Qiang @ 2016-07-05 3:51 ` Jason Cooper 2016-07-05 7:38 ` Qiang Zhao 0 siblings, 1 reply; 9+ messages in thread From: Jason Cooper @ 2016-07-05 3:51 UTC (permalink / raw) To: Zhao Qiang Cc: oss, tglx, marc.zyngier, linuxppc-dev, linux-kernel, xiaobo.xie Hi Zhao Qiang, Same comment as previous patch regarding the subject line. On Tue, Jul 05, 2016 at 09:46:59AM +0800, Zhao Qiang wrote: > there are init_qe_ic_sysfs and qeic_of_init, refactor > them. Same comment from previous patch about commit log. > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > --- > drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++--------------------- > include/soc/fsl/qe/qe_ic.h | 7 ---- > 2 files changed, 45 insertions(+), 45 deletions(-) > > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c > index f7f9a81..46652c0 100644 > --- a/drivers/irqchip/qe_ic.c > +++ b/drivers/irqchip/qe_ic.c > @@ -317,27 +317,35 @@ unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic) > return irq_linear_revmap(qe_ic->irqhost, irq); > } > > -void __init qe_ic_init(struct device_node *node, unsigned int flags, > - void (*low_handler)(struct irq_desc *desc), > - void (*high_handler)(struct irq_desc *desc)) > +static int __init qe_ic_init(unsigned int flags) > { > + struct device_node *node; > struct qe_ic *qe_ic; > struct resource res; > - u32 temp = 0, ret, high_active = 0; > + u32 temp = 0, high_active = 0; > + int ret = 0; > + > + node = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > + if (!node) > + return -ENODEV; > > ret = of_address_to_resource(node, 0, &res); > - if (ret) > - return; > + if (ret) { > + ret = -ENODEV; > + goto err_put_node; > + } > > qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL); > - if (qe_ic == NULL) > - return; > + if (qe_ic == NULL) { > + ret = -ENOMEM; > + goto err_put_node; > + } > > 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; > + ret = -ENOMEM; > + goto err_free_qe_ic; > } > > qe_ic->regs = ioremap(res.start, resource_size(&res)); > @@ -348,9 +356,9 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, > qe_ic->virq_low = irq_of_parse_and_map(node, 1); > > if (qe_ic->virq_low == NO_IRQ) { > - printk(KERN_ERR "Failed to map QE_IC low IRQ\n"); > - kfree(qe_ic); > - return; > + pr_err("Failed to map QE_IC low IRQ\n"); > + ret = -ENOMEM; > + goto err_domain_remove; > } > > /* default priority scheme is grouped. If spread mode is */ > @@ -377,13 +385,23 @@ void __init qe_ic_init(struct device_node *node, unsigned int flags, > qe_ic_write(qe_ic->regs, QEIC_CICR, temp); > > irq_set_handler_data(qe_ic->virq_low, qe_ic); > - irq_set_chained_handler(qe_ic->virq_low, low_handler); > + irq_set_chained_handler(qe_ic->virq_low, qe_ic_cascade_low_mpic); > > if (qe_ic->virq_high != NO_IRQ && > qe_ic->virq_high != qe_ic->virq_low) { > irq_set_handler_data(qe_ic->virq_high, qe_ic); > - irq_set_chained_handler(qe_ic->virq_high, high_handler); > + irq_set_chained_handler(qe_ic->virq_high, > + qe_ic_cascade_high_mpic); > } > + return ret; of_node_put(node)? Explicitly return success? > + > +err_domain_remove: > + irq_domain_remove(qe_ic->irqhost); > +err_free_qe_ic: > + kfree(qe_ic); > +err_put_node: > + of_node_put(node); > + return ret; > } > > void qe_ic_set_highest_priority(unsigned int virq, int high) > @@ -490,37 +508,26 @@ static struct device device_qe_ic = { > .bus = &qe_ic_subsys, > }; > > -static int __init init_qe_ic_sysfs(void) > +static int __init init_qe_ic(void) > { > - int rc; > + int ret; > > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > + ret = qe_ic_init(0); Sorry, build machine is down atm. How was qe_ic_init() called previously? Is that removed? > + if (ret) > + return ret; > > - rc = subsys_system_register(&qe_ic_subsys, NULL); > - if (rc) { > - printk(KERN_ERR "Failed registering qe_ic sys class\n"); > + ret = subsys_system_register(&qe_ic_subsys, NULL); > + if (ret) { > + pr_err("Failed registering qe_ic sys class\n"); > return -ENODEV; > } > - rc = device_register(&device_qe_ic); > - if (rc) { > - printk(KERN_ERR "Failed registering qe_ic sys device\n"); > + ret = device_register(&device_qe_ic); > + if (ret) { > + pr_err("Failed registering qe_ic sys device\n"); > return -ENODEV; > } > - return 0; > -} > > -static int __init qeic_of_init(void) > -{ > - struct device_node *np; > - > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (np) { > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - of_node_put(np); > - } > return 0; > } > > -subsys_initcall(qeic_of_init); > -subsys_initcall(init_qe_ic_sysfs); > +subsys_initcall(init_qe_ic); > diff --git a/include/soc/fsl/qe/qe_ic.h b/include/soc/fsl/qe/qe_ic.h > index 1e155ca..6113699 100644 > --- a/include/soc/fsl/qe/qe_ic.h > +++ b/include/soc/fsl/qe/qe_ic.h > @@ -58,16 +58,9 @@ enum qe_ic_grp_id { > }; > > #ifdef CONFIG_QUICC_ENGINE > -void qe_ic_init(struct device_node *node, unsigned int flags, > - void (*low_handler)(struct irq_desc *desc), > - void (*high_handler)(struct irq_desc *desc)); > unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic); > unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic); > #else > -static inline void qe_ic_init(struct device_node *node, unsigned int flags, > - void (*low_handler)(struct irq_desc *desc), > - void (*high_handler)(struct irq_desc *desc)) > -{} > static inline unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic) > { return 0; } > static inline unsigned int qe_ic_get_high_irq(struct qe_ic *qe_ic) thx, Jason. > -- > 2.1.0.27.g96db324 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 2/2] qe/ic: refactor qe_ic to simplify 2016-07-05 3:51 ` Jason Cooper @ 2016-07-05 7:38 ` Qiang Zhao 2016-07-05 13:45 ` Jason Cooper 0 siblings, 1 reply; 9+ messages in thread From: Qiang Zhao @ 2016-07-05 7:38 UTC (permalink / raw) To: Jason Cooper Cc: oss@buserror.net, tglx@linutronix.de, marc.zyngier@arm.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Xiaobo Xie On 07/05/2016 11:51 AM, Jason Cooper <jason@lakedaemon.net> wrote: > -----Original Message----- > From: Jason Cooper [mailto:jason@lakedaemon.net] > Sent: Tuesday, July 05, 2016 11:51 AM > To: Qiang Zhao <qiang.zhao@nxp.com> > Cc: oss@buserror.net; tglx@linutronix.de; marc.zyngier@arm.com; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie > <xiaobo.xie@nxp.com> > Subject: Re: [PATCH 2/2] qe/ic: refactor qe_ic to simplify >=20 > Hi Zhao Qiang, >=20 > Same comment as previous patch regarding the subject line. >=20 > On Tue, Jul 05, 2016 at 09:46:59AM +0800, Zhao Qiang wrote: > > there are init_qe_ic_sysfs and qeic_of_init, refactor them. >=20 > Same comment from previous patch about commit log. >=20 > > > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > > --- > > drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++-------------= ----- > --- > > include/soc/fsl/qe/qe_ic.h | 7 ---- > > 2 files changed, 45 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index > > f7f9a81..46652c0 100644 > > --- a/drivers/irqchip/qe_ic.c > > +++ b/drivers/irqchip/qe_ic.c > > @@ -317,27 +317,35 @@ unsigned int qe_ic_get_high_irq(struct qe_ic *qe_= ic) > > return irq_linear_revmap(qe_ic->irqhost, irq); } > > > > -void __init qe_ic_init(struct device_node *node, unsigned int flags, > > - void (*low_handler)(struct irq_desc *desc), > > - void (*high_handler)(struct irq_desc *desc)) > > +static int __init qe_ic_init(unsigned int flags) > > { > > + struct device_node *node; > > struct qe_ic *qe_ic; > > struct resource res; > > - u32 temp =3D 0, ret, high_active =3D 0; > > + u32 temp =3D 0, high_active =3D 0; > > + int ret =3D 0; > > + > > + node =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > + if (!node) > > + return -ENODEV; > > > > ret =3D of_address_to_resource(node, 0, &res); > > - if (ret) > > - return; > > + if (ret) { > > + ret =3D -ENODEV; > > + goto err_put_node; > > + } > > > > qe_ic =3D kzalloc(sizeof(*qe_ic), GFP_KERNEL); > > - if (qe_ic =3D=3D NULL) > > - return; > > + if (qe_ic =3D=3D NULL) { > > + ret =3D -ENOMEM; > > + goto err_put_node; > > + } > > > > qe_ic->irqhost =3D irq_domain_add_linear(node, NR_QE_IC_INTS, > > &qe_ic_host_ops, qe_ic); > > if (qe_ic->irqhost =3D=3D NULL) { > > - kfree(qe_ic); > > - return; > > + ret =3D -ENOMEM; > > + goto err_free_qe_ic; > > } > > > > qe_ic->regs =3D ioremap(res.start, resource_size(&res)); @@ -348,9 > > +356,9 @@ void __init qe_ic_init(struct device_node *node, unsigned int > flags, > > qe_ic->virq_low =3D irq_of_parse_and_map(node, 1); > > > > if (qe_ic->virq_low =3D=3D NO_IRQ) { > > - printk(KERN_ERR "Failed to map QE_IC low IRQ\n"); > > - kfree(qe_ic); > > - return; > > + pr_err("Failed to map QE_IC low IRQ\n"); > > + ret =3D -ENOMEM; > > + goto err_domain_remove; > > } > > > > /* default priority scheme is grouped. If spread mode is */ > > @@ -377,13 +385,23 @@ void __init qe_ic_init(struct device_node *node, > unsigned int flags, > > qe_ic_write(qe_ic->regs, QEIC_CICR, temp); > > > > irq_set_handler_data(qe_ic->virq_low, qe_ic); > > - irq_set_chained_handler(qe_ic->virq_low, low_handler); > > + irq_set_chained_handler(qe_ic->virq_low, qe_ic_cascade_low_mpic); > > > > if (qe_ic->virq_high !=3D NO_IRQ && > > qe_ic->virq_high !=3D qe_ic->virq_low) { > > irq_set_handler_data(qe_ic->virq_high, qe_ic); > > - irq_set_chained_handler(qe_ic->virq_high, high_handler); > > + irq_set_chained_handler(qe_ic->virq_high, > > + qe_ic_cascade_high_mpic); > > } > > + return ret; >=20 > of_node_put(node)? Explicitly return success? Yes, thank you very much! >=20 > > + > > +err_domain_remove: > > + irq_domain_remove(qe_ic->irqhost); > > +err_free_qe_ic: > > + kfree(qe_ic); > > +err_put_node: > > + of_node_put(node); > > + return ret; > > } > > > > void qe_ic_set_highest_priority(unsigned int virq, int high) @@ > > -490,37 +508,26 @@ static struct device device_qe_ic =3D { > > .bus =3D &qe_ic_subsys, > > }; > > > > -static int __init init_qe_ic_sysfs(void) > > +static int __init init_qe_ic(void) > > { > > - int rc; > > + int ret; > > > > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > > + ret =3D qe_ic_init(0); >=20 > Sorry, build machine is down atm. How was qe_ic_init() called previously= ? Is > that removed? Sorry, I don't understand, could you please explain? >=20 > > + if (ret) > > + return ret; > > > > - rc =3D subsys_system_register(&qe_ic_subsys, NULL); > > - if (rc) { > > - printk(KERN_ERR "Failed registering qe_ic sys class\n"); > > + ret =3D subsys_system_register(&qe_ic_subsys, NULL); > > + if (ret) { > > + pr_err("Failed registering qe_ic sys class\n"); > > return -ENODEV; > > } > > - rc =3D device_register(&device_qe_ic); > > - if (rc) { > > - printk(KERN_ERR "Failed registering qe_ic sys device\n"); > > + ret =3D device_register(&device_qe_ic); > > + if (ret) { > > + pr_err("Failed registering qe_ic sys device\n"); > > return -ENODEV; > > } > > - return 0; > > -} > > > > -static int __init qeic_of_init(void) > > -{ > > - struct device_node *np; > > - > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (np) { > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > - qe_ic_cascade_high_mpic); > > - of_node_put(np); > > - } > > return 0; > > } > > > > -subsys_initcall(qeic_of_init); > > -subsys_initcall(init_qe_ic_sysfs); > > +subsys_initcall(init_qe_ic); > > diff --git a/include/soc/fsl/qe/qe_ic.h b/include/soc/fsl/qe/qe_ic.h > > index 1e155ca..6113699 100644 > > --- a/include/soc/fsl/qe/qe_ic.h > > +++ b/include/soc/fsl/qe/qe_ic.h > > @@ -58,16 +58,9 @@ enum qe_ic_grp_id { }; > > > > #ifdef CONFIG_QUICC_ENGINE > > -void qe_ic_init(struct device_node *node, unsigned int flags, > > - void (*low_handler)(struct irq_desc *desc), > > - void (*high_handler)(struct irq_desc *desc)); > > unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic); unsigned int > > qe_ic_get_high_irq(struct qe_ic *qe_ic); #else -static inline void > > qe_ic_init(struct device_node *node, unsigned int flags, > > - void (*low_handler)(struct irq_desc *desc), > > - void (*high_handler)(struct irq_desc *desc)) > > -{} > > static inline unsigned int qe_ic_get_low_irq(struct qe_ic *qe_ic) { > > return 0; } static inline unsigned int qe_ic_get_high_irq(struct > > qe_ic *qe_ic) -Zhao Qiang BR ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] qe/ic: refactor qe_ic to simplify 2016-07-05 7:38 ` Qiang Zhao @ 2016-07-05 13:45 ` Jason Cooper 0 siblings, 0 replies; 9+ messages in thread From: Jason Cooper @ 2016-07-05 13:45 UTC (permalink / raw) To: Qiang Zhao Cc: oss@buserror.net, tglx@linutronix.de, marc.zyngier@arm.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Xiaobo Xie Morning! On Tue, Jul 05, 2016 at 07:38:49AM +0000, Qiang Zhao wrote: > On 07/05/2016 11:51 AM, Jason Cooper <jason@lakedaemon.net> wrote: > > On Tue, Jul 05, 2016 at 09:46:59AM +0800, Zhao Qiang wrote: ... > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > > > --- > > > drivers/irqchip/qe_ic.c | 83 +++++++++++++++++++++++++------------------ > > --- > > > include/soc/fsl/qe/qe_ic.h | 7 ---- > > > 2 files changed, 45 insertions(+), 45 deletions(-) > > > > > > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c index > > > f7f9a81..46652c0 100644 > > > --- a/drivers/irqchip/qe_ic.c > > > +++ b/drivers/irqchip/qe_ic.c ... > > > - printk(KERN_DEBUG "Registering qe_ic with sysfs...\n"); > > > + ret = qe_ic_init(0); > > > > Sorry, build machine is down atm. How was qe_ic_init() called previously? Is > > that removed? > > Sorry, I don't understand, could you please explain? -ENOSLEEP when I wrote that. :) *My* build machine, with my copies of the kernel tree was down, so I had no easy way to dig into the source. And .... > > > -static int __init qeic_of_init(void) > > > -{ > > > - struct device_node *np; > > > - > > > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > > - if (np) { > > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > > - qe_ic_cascade_high_mpic); > > > - of_node_put(np); > > > - } > > > return 0; > > > } > > > > > > -subsys_initcall(qeic_of_init); this block is what I missed last night. :-/ Sorry for the noise. thx, Jason. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip 2016-07-05 1:46 [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Zhao Qiang 2016-07-05 1:46 ` [PATCH 2/2] qe/ic: refactor qe_ic to simplify Zhao Qiang @ 2016-07-05 3:18 ` Jason Cooper 2016-07-05 7:27 ` Qiang Zhao 1 sibling, 1 reply; 9+ messages in thread From: Jason Cooper @ 2016-07-05 3:18 UTC (permalink / raw) To: Zhao Qiang Cc: oss, tglx, marc.zyngier, linuxppc-dev, linux-kernel, xiaobo.xie Hi Zhao Qiang, Please reword your subject line to conform to the standard in irqchip (since that's where the code is added). Also, please adjust the subject line to express *why* the change is being made. On Tue, Jul 05, 2016 at 09:46:58AM +0800, Zhao Qiang wrote: > The codes of qe_ic_init in platforms are redundant, > move them to qe_ic under irqchip This needs to be a lot more clear. How is backwards compatibility preserved? Why is lookup by type "qeic" dropped? In short, please explain everything that looks funny so we don't have to guess. > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > --- > arch/powerpc/platforms/83xx/misc.c | 15 --------------- > arch/powerpc/platforms/85xx/corenet_generic.c | 9 --------- > arch/powerpc/platforms/85xx/mpc85xx_mds.c | 14 -------------- > arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 16 ---------------- > arch/powerpc/platforms/85xx/twr_p102x.c | 14 -------------- > drivers/irqchip/qe_ic.c | 14 ++++++++++++++ > 6 files changed, 14 insertions(+), 68 deletions(-) > > diff --git a/arch/powerpc/platforms/83xx/misc.c b/arch/powerpc/platforms/83xx/misc.c > index 7e923ca..9431fc7 100644 > --- a/arch/powerpc/platforms/83xx/misc.c > +++ b/arch/powerpc/platforms/83xx/misc.c > @@ -93,24 +93,9 @@ void __init mpc83xx_ipic_init_IRQ(void) > } > > #ifdef CONFIG_QUICC_ENGINE > -void __init mpc83xx_qe_init_IRQ(void) > -{ > - struct device_node *np; > - > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (!np) { > - np = of_find_node_by_type(NULL, "qeic"); > - if (!np) > - return; > - } This block isn't preserved in the irqchip driver. Why not? > - qe_ic_init(np, 0, qe_ic_cascade_low_ipic, qe_ic_cascade_high_ipic); > - of_node_put(np); > -} > - > void __init mpc83xx_ipic_and_qe_init_IRQ(void) > { > mpc83xx_ipic_init_IRQ(); > - mpc83xx_qe_init_IRQ(); > } > #endif /* CONFIG_QUICC_ENGINE */ > > diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c > index a2b0bc8..526fc2b 100644 > --- a/arch/powerpc/platforms/85xx/corenet_generic.c > +++ b/arch/powerpc/platforms/85xx/corenet_generic.c > @@ -41,8 +41,6 @@ void __init corenet_gen_pic_init(void) > unsigned int flags = MPIC_BIG_ENDIAN | MPIC_SINGLE_DEST_CPU | > MPIC_NO_RESET; > > - struct device_node *np; > - > if (ppc_md.get_irq == mpic_get_coreint_irq) > flags |= MPIC_ENABLE_COREINT; > > @@ -50,13 +48,6 @@ void __init corenet_gen_pic_init(void) > BUG_ON(mpic == NULL); > > mpic_init(mpic); > - > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (np) { > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - of_node_put(np); > - } > } > > /* > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > index f61cbe2..7ae4901 100644 > --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c > +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > @@ -279,20 +279,6 @@ static void __init mpc85xx_mds_qeic_init(void) > of_node_put(np); > return; > } > - > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (!np) { > - np = of_find_node_by_type(NULL, "qeic"); > - if (!np) > - return; > - } > - > - if (machine_is(p1021_mds)) > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - else > - qe_ic_init(np, 0, qe_ic_cascade_muxed_mpic, NULL); This block is also not preserved. Nor explained in the commit log. Is it really ok to drop it? If so, why? > - of_node_put(np); > } > #else > static void __init mpc85xx_mds_qe_init(void) { } > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > index 3f4dad1..779f54f 100644 > --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > @@ -49,10 +49,6 @@ void __init mpc85xx_rdb_pic_init(void) > struct mpic *mpic; > unsigned long root = of_get_flat_dt_root(); > > -#ifdef CONFIG_QUICC_ENGINE > - struct device_node *np; > -#endif > - > if (of_flat_dt_is_compatible(root, "fsl,MPC85XXRDB-CAMP")) { > mpic = mpic_alloc(NULL, 0, MPIC_NO_RESET | > MPIC_BIG_ENDIAN | > @@ -67,18 +63,6 @@ void __init mpc85xx_rdb_pic_init(void) > > BUG_ON(mpic == NULL); > mpic_init(mpic); > - > -#ifdef CONFIG_QUICC_ENGINE > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (np) { > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - of_node_put(np); > - > - } else > - pr_err("%s: Could not find qe-ic node\n", __func__); > -#endif > - > } > > /* > diff --git a/arch/powerpc/platforms/85xx/twr_p102x.c b/arch/powerpc/platforms/85xx/twr_p102x.c > index 71bc255..603e244 100644 > --- a/arch/powerpc/platforms/85xx/twr_p102x.c > +++ b/arch/powerpc/platforms/85xx/twr_p102x.c > @@ -35,26 +35,12 @@ static void __init twr_p1025_pic_init(void) > { > struct mpic *mpic; > > -#ifdef CONFIG_QUICC_ENGINE > - struct device_node *np; > -#endif > - > mpic = mpic_alloc(NULL, 0, MPIC_BIG_ENDIAN | > MPIC_SINGLE_DEST_CPU, > 0, 256, " OpenPIC "); > > BUG_ON(mpic == NULL); > mpic_init(mpic); > - > -#ifdef CONFIG_QUICC_ENGINE > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > - if (np) { > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > - qe_ic_cascade_high_mpic); > - of_node_put(np); > - } else > - pr_err("Could not find qe-ic node\n"); > -#endif > } > > /* ************************************************************************ > diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c > index ec2ca86..f7f9a81 100644 > --- a/drivers/irqchip/qe_ic.c > +++ b/drivers/irqchip/qe_ic.c > @@ -509,4 +509,18 @@ static int __init init_qe_ic_sysfs(void) > return 0; > } > > +static int __init qeic_of_init(void) > +{ > + struct device_node *np; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > + if (np) { > + qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > + qe_ic_cascade_high_mpic); > + of_node_put(np); > + } > + return 0; > +} > + > +subsys_initcall(qeic_of_init); > subsys_initcall(init_qe_ic_sysfs); Was this tested on systems with dtbs containing type "qeic" but missing "fsl,qe-ic"? What about non-p1021_mds mpc85xx_mds boards? thx, Jason. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip 2016-07-05 3:18 ` [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Jason Cooper @ 2016-07-05 7:27 ` Qiang Zhao 2016-07-05 14:21 ` Jason Cooper 0 siblings, 1 reply; 9+ messages in thread From: Qiang Zhao @ 2016-07-05 7:27 UTC (permalink / raw) To: Jason Cooper Cc: oss@buserror.net, tglx@linutronix.de, marc.zyngier@arm.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Xiaobo Xie On 07/05/2016 11:19 AM, Jason Cooper <jason@lakedaemon.net> wrote: > -----Original Message----- > From: Jason Cooper [mailto:jason@lakedaemon.net] > Sent: Tuesday, July 05, 2016 11:19 AM > To: Qiang Zhao <qiang.zhao@nxp.com> > Cc: oss@buserror.net; tglx@linutronix.de; marc.zyngier@arm.com; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie > <xiaobo.xie@nxp.com> > Subject: Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip >=20 > Hi Zhao Qiang, >=20 > Please reword your subject line to conform to the standard in irqchip (si= nce > that's where the code is added). Also, please adjust the subject line to= express > *why* the change is being made. >=20 > On Tue, Jul 05, 2016 at 09:46:58AM +0800, Zhao Qiang wrote: > > The codes of qe_ic_init in platforms are redundant, move them to qe_ic > > under irqchip >=20 > This needs to be a lot more clear. How is backwards compatibility preser= ved? > Why is lookup by type "qeic" dropped? In short, please explain everythin= g that > looks funny so we don't have to guess. Thank you for your review and feedback. >=20 > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > > --- > > arch/powerpc/platforms/83xx/misc.c | 15 --------------- > > arch/powerpc/platforms/85xx/corenet_generic.c | 9 --------- > > arch/powerpc/platforms/85xx/mpc85xx_mds.c | 14 -------------- > > arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 16 ---------------- > > arch/powerpc/platforms/85xx/twr_p102x.c | 14 -------------- > > drivers/irqchip/qe_ic.c | 14 ++++++++++++++ > > 6 files changed, 14 insertions(+), 68 deletions(-) > > > > diff --git a/arch/powerpc/platforms/83xx/misc.c > > b/arch/powerpc/platforms/83xx/misc.c > > index 7e923ca..9431fc7 100644 > > --- a/arch/powerpc/platforms/83xx/misc.c > > +++ b/arch/powerpc/platforms/83xx/misc.c > > @@ -93,24 +93,9 @@ void __init mpc83xx_ipic_init_IRQ(void) } > > > > #ifdef CONFIG_QUICC_ENGINE > > -void __init mpc83xx_qe_init_IRQ(void) -{ > > - struct device_node *np; > > - > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (!np) { > > - np =3D of_find_node_by_type(NULL, "qeic"); > > - if (!np) > > - return; > > - } >=20 > This block isn't preserved in the irqchip driver. Why not? I grep qeic in arch/powerpc/boot/dts/*, doesn't find which board use qeic a= s type. >=20 > > - qe_ic_init(np, 0, qe_ic_cascade_low_ipic, qe_ic_cascade_high_ipic); > > - of_node_put(np); > > -} > > - > > void __init mpc83xx_ipic_and_qe_init_IRQ(void) > > { > > mpc83xx_ipic_init_IRQ(); > > - mpc83xx_qe_init_IRQ(); > > } > > #endif /* CONFIG_QUICC_ENGINE */ > > > > diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c > > b/arch/powerpc/platforms/85xx/corenet_generic.c > > index a2b0bc8..526fc2b 100644 > > --- a/arch/powerpc/platforms/85xx/corenet_generic.c > > +++ b/arch/powerpc/platforms/85xx/corenet_generic.c > > @@ -41,8 +41,6 @@ void __init corenet_gen_pic_init(void) > > unsigned int flags =3D MPIC_BIG_ENDIAN | MPIC_SINGLE_DEST_CPU | > > MPIC_NO_RESET; > > > > - struct device_node *np; > > - > > if (ppc_md.get_irq =3D=3D mpic_get_coreint_irq) > > flags |=3D MPIC_ENABLE_COREINT; > > > > @@ -50,13 +48,6 @@ void __init corenet_gen_pic_init(void) > > BUG_ON(mpic =3D=3D NULL); > > > > mpic_init(mpic); > > - > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (np) { > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > - qe_ic_cascade_high_mpic); > > - of_node_put(np); > > - } > > } > > > > /* > > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > index f61cbe2..7ae4901 100644 > > --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > @@ -279,20 +279,6 @@ static void __init mpc85xx_mds_qeic_init(void) > > of_node_put(np); > > return; > > } > > - > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (!np) { > > - np =3D of_find_node_by_type(NULL, "qeic"); > > - if (!np) > > - return; > > - } > > - > > - if (machine_is(p1021_mds)) > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > - qe_ic_cascade_high_mpic); > > - else > > - qe_ic_init(np, 0, qe_ic_cascade_muxed_mpic, NULL); >=20 > This block is also not preserved. Nor explained in the commit log. Is i= t really ok > to drop it? If so, why? on non-p1021_mds mpc85xx_mds boards(mpc8568 and mpc8569). The qeic has the same interrupt number for low and high. So use qe_ic_casca= de_muxed_mpic for this situation. qeic: interrupt-controller@80 { interrupt-controller; compatible =3D "fsl,qe-ic"; #address-cells =3D <0>; #interrupt-cells =3D <1>; reg =3D <0x80 0x80>; interrupts =3D <46 2 0 0 46 2 0 0>; //high:30 low:30 interrupt-parent =3D <&mpic>; }; In qe_ic_init, the code(as below) can handle this situation. If " qe_ic->virq_high !=3D qe_ic->virq_low ", then set chaned handler for h= igh handler. if (qe_ic->virq_high !=3D NO_IRQ && qe_ic->virq_high !=3D qe_ic->virq_low) { irq_set_handler_data(qe_ic->virq_high, qe_ic); irq_set_chained_handler(qe_ic->virq_high, high_handler); }=20 >=20 > > - of_node_put(np); > > } > > #else > > static void __init mpc85xx_mds_qe_init(void) { } diff --git > > a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > > b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > > index 3f4dad1..779f54f 100644 > > --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > > +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c > > @@ -49,10 +49,6 @@ void __init mpc85xx_rdb_pic_init(void) > > struct mpic *mpic; > > unsigned long root =3D of_get_flat_dt_root(); > > > > -#ifdef CONFIG_QUICC_ENGINE > > - struct device_node *np; > > -#endif > > - > > if (of_flat_dt_is_compatible(root, "fsl,MPC85XXRDB-CAMP")) { > > mpic =3D mpic_alloc(NULL, 0, MPIC_NO_RESET | > > MPIC_BIG_ENDIAN | > > @@ -67,18 +63,6 @@ void __init mpc85xx_rdb_pic_init(void) > > > > BUG_ON(mpic =3D=3D NULL); > > mpic_init(mpic); > > - > > -#ifdef CONFIG_QUICC_ENGINE > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (np) { > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > - qe_ic_cascade_high_mpic); > > - of_node_put(np); > > - > > - } else > > - pr_err("%s: Could not find qe-ic node\n", __func__); > > -#endif > > - > > } > > > > /* > > diff --git a/arch/powerpc/platforms/85xx/twr_p102x.c > > b/arch/powerpc/platforms/85xx/twr_p102x.c > > index 71bc255..603e244 100644 > > --- a/arch/powerpc/platforms/85xx/twr_p102x.c > > +++ b/arch/powerpc/platforms/85xx/twr_p102x.c > > @@ -35,26 +35,12 @@ static void __init twr_p1025_pic_init(void) { > > struct mpic *mpic; > > > > -#ifdef CONFIG_QUICC_ENGINE > > - struct device_node *np; > > -#endif > > - > > mpic =3D mpic_alloc(NULL, 0, MPIC_BIG_ENDIAN | > > MPIC_SINGLE_DEST_CPU, > > 0, 256, " OpenPIC "); > > > > BUG_ON(mpic =3D=3D NULL); > > mpic_init(mpic); > > - > > -#ifdef CONFIG_QUICC_ENGINE > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > - if (np) { > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > - qe_ic_cascade_high_mpic); > > - of_node_put(np); > > - } else > > - pr_err("Could not find qe-ic node\n"); > > -#endif > > } > > > > /* > > > ************************************************************ > ********** > > ** diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c > > index ec2ca86..f7f9a81 100644 > > --- a/drivers/irqchip/qe_ic.c > > +++ b/drivers/irqchip/qe_ic.c > > @@ -509,4 +509,18 @@ static int __init init_qe_ic_sysfs(void) > > return 0; > > } > > > > +static int __init qeic_of_init(void) > > +{ > > + struct device_node *np; > > + > > + np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > + if (np) { > > + qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > + qe_ic_cascade_high_mpic); > > + of_node_put(np); > > + } > > + return 0; > > +} > > + > > +subsys_initcall(qeic_of_init); > > subsys_initcall(init_qe_ic_sysfs); >=20 > Was this tested on systems with dtbs containing type "qeic" but missing "= fsl,qe- > ic"? What about non-p1021_mds mpc85xx_mds boards? In kernel, there are no boards containing type "qeic" in dtb. -Zhao Qiang BR ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip 2016-07-05 7:27 ` Qiang Zhao @ 2016-07-05 14:21 ` Jason Cooper 2016-07-06 1:18 ` Qiang Zhao 0 siblings, 1 reply; 9+ messages in thread From: Jason Cooper @ 2016-07-05 14:21 UTC (permalink / raw) To: Qiang Zhao Cc: oss@buserror.net, tglx@linutronix.de, marc.zyngier@arm.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Xiaobo Xie On Tue, Jul 05, 2016 at 07:27:21AM +0000, Qiang Zhao wrote: > On 07/05/2016 11:19 AM, Jason Cooper <jason@lakedaemon.net> wrote: > > -----Original Message----- > > From: Jason Cooper [mailto:jason@lakedaemon.net] > > Sent: Tuesday, July 05, 2016 11:19 AM > > To: Qiang Zhao <qiang.zhao@nxp.com> > > Cc: oss@buserror.net; tglx@linutronix.de; marc.zyngier@arm.com; linuxppc- > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie > > <xiaobo.xie@nxp.com> > > Subject: Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip > > > > Hi Zhao Qiang, > > > > Please reword your subject line to conform to the standard in irqchip (since > > that's where the code is added). Also, please adjust the subject line to express > > *why* the change is being made. > > > > On Tue, Jul 05, 2016 at 09:46:58AM +0800, Zhao Qiang wrote: > > > The codes of qe_ic_init in platforms are redundant, move them to qe_ic > > > under irqchip > > > > This needs to be a lot more clear. How is backwards compatibility preserved? > > Why is lookup by type "qeic" dropped? In short, please explain everything that > > looks funny so we don't have to guess. > > Thank you for your review and feedback. > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@nxp.com> > > > --- > > > arch/powerpc/platforms/83xx/misc.c | 15 --------------- > > > arch/powerpc/platforms/85xx/corenet_generic.c | 9 --------- > > > arch/powerpc/platforms/85xx/mpc85xx_mds.c | 14 -------------- > > > arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 16 ---------------- > > > arch/powerpc/platforms/85xx/twr_p102x.c | 14 -------------- > > > drivers/irqchip/qe_ic.c | 14 ++++++++++++++ > > > 6 files changed, 14 insertions(+), 68 deletions(-) > > > > > > diff --git a/arch/powerpc/platforms/83xx/misc.c > > > b/arch/powerpc/platforms/83xx/misc.c > > > index 7e923ca..9431fc7 100644 > > > --- a/arch/powerpc/platforms/83xx/misc.c > > > +++ b/arch/powerpc/platforms/83xx/misc.c > > > @@ -93,24 +93,9 @@ void __init mpc83xx_ipic_init_IRQ(void) } > > > > > > #ifdef CONFIG_QUICC_ENGINE > > > -void __init mpc83xx_qe_init_IRQ(void) -{ > > > - struct device_node *np; > > > - > > > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > > - if (!np) { > > > - np = of_find_node_by_type(NULL, "qeic"); > > > - if (!np) > > > - return; > > > - } > > > > This block isn't preserved in the irqchip driver. Why not? > > I grep qeic in arch/powerpc/boot/dts/*, doesn't find which board use qeic as type. Unfortunately, checking powerpc/boot/dts/* isn't sufficient for confirming we aren't going to break backwards compatibility with boards *in the field*. Please take a look at: d4fb5ebd83c70 powerpc/83xx: consolidate init_IRQ functions 8159df72d43e2 83xx: add support for the kmeter1 board. Perhaps one or two of the authors is still around and can say why that check is there and if it's ok to remove it. Or, we could just play it safe and keep the check. ... > > > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > > b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > > index f61cbe2..7ae4901 100644 > > > --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > > +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c > > > @@ -279,20 +279,6 @@ static void __init mpc85xx_mds_qeic_init(void) > > > of_node_put(np); > > > return; > > > } > > > - > > > - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > > - if (!np) { > > > - np = of_find_node_by_type(NULL, "qeic"); > > > - if (!np) > > > - return; > > > - } > > > - > > > - if (machine_is(p1021_mds)) > > > - qe_ic_init(np, 0, qe_ic_cascade_low_mpic, > > > - qe_ic_cascade_high_mpic); > > > - else > > > - qe_ic_init(np, 0, qe_ic_cascade_muxed_mpic, NULL); > > > > This block is also not preserved. Nor explained in the commit log. Is it really ok > > to drop it? If so, why? > > on non-p1021_mds mpc85xx_mds boards(mpc8568 and mpc8569). > The qeic has the same interrupt number for low and high. So use qe_ic_cascade_muxed_mpic for this situation. > > qeic: interrupt-controller@80 { > interrupt-controller; > compatible = "fsl,qe-ic"; > #address-cells = <0>; > #interrupt-cells = <1>; > reg = <0x80 0x80>; > interrupts = <46 2 0 0 46 2 0 0>; //high:30 low:30 > interrupt-parent = <&mpic>; > }; > > In qe_ic_init, the code(as below) can handle this situation. > If " qe_ic->virq_high != qe_ic->virq_low ", then set chaned handler for high handler. > > if (qe_ic->virq_high != NO_IRQ && > qe_ic->virq_high != qe_ic->virq_low) { > irq_set_handler_data(qe_ic->virq_high, qe_ic); > irq_set_chained_handler(qe_ic->virq_high, high_handler); > } > Ok, thanks. Please clarify that in the commit log on your next version. thx, Jason. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip 2016-07-05 14:21 ` Jason Cooper @ 2016-07-06 1:18 ` Qiang Zhao 0 siblings, 0 replies; 9+ messages in thread From: Qiang Zhao @ 2016-07-06 1:18 UTC (permalink / raw) To: Jason Cooper Cc: oss@buserror.net, tglx@linutronix.de, marc.zyngier@arm.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Xiaobo Xie On 07/05/2016 11:19 AM, Jason Cooper <jason@lakedaemon.net> wrote: > -----Original Message----- > From: Jason Cooper [mailto:jason@lakedaemon.net] > Sent: Tuesday, July 05, 2016 10:22 PM > To: Qiang Zhao <qiang.zhao@nxp.com> > Cc: oss@buserror.net; tglx@linutronix.de; marc.zyngier@arm.com; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie > <xiaobo.xie@nxp.com> > Subject: Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip >=20 > On Tue, Jul 05, 2016 at 07:27:21AM +0000, Qiang Zhao wrote: > > On 07/05/2016 11:19 AM, Jason Cooper <jason@lakedaemon.net> wrote: > > > -----Original Message----- > > > From: Jason Cooper [mailto:jason@lakedaemon.net] > > > Sent: Tuesday, July 05, 2016 11:19 AM > > > To: Qiang Zhao <qiang.zhao@nxp.com> > > > Cc: oss@buserror.net; tglx@linutronix.de; marc.zyngier@arm.com; > > > linuxppc- dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo > > > Xie <xiaobo.xie@nxp.com> > > > Subject: Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to > > > > diff --git a/arch/powerpc/platforms/83xx/misc.c > > > > b/arch/powerpc/platforms/83xx/misc.c > > > > index 7e923ca..9431fc7 100644 > > > > --- a/arch/powerpc/platforms/83xx/misc.c > > > > +++ b/arch/powerpc/platforms/83xx/misc.c > > > > @@ -93,24 +93,9 @@ void __init mpc83xx_ipic_init_IRQ(void) } > > > > > > > > #ifdef CONFIG_QUICC_ENGINE > > > > -void __init mpc83xx_qe_init_IRQ(void) -{ > > > > - struct device_node *np; > > > > - > > > > - np =3D of_find_compatible_node(NULL, NULL, "fsl,qe-ic"); > > > > - if (!np) { > > > > - np =3D of_find_node_by_type(NULL, "qeic"); > > > > - if (!np) > > > > - return; > > > > - } > > > > > > This block isn't preserved in the irqchip driver. Why not? > > > > I grep qeic in arch/powerpc/boot/dts/*, doesn't find which board use qe= ic as > type. >=20 > Unfortunately, checking powerpc/boot/dts/* isn't sufficient for confirmin= g we > aren't going to break backwards compatibility with boards *in the field*. >=20 > Please take a look at: >=20 > d4fb5ebd83c70 powerpc/83xx: consolidate init_IRQ functions > 8159df72d43e2 83xx: add support for the kmeter1 board. >=20 > Perhaps one or two of the authors is still around and can say why that ch= eck is > there and if it's ok to remove it. >=20 > Or, we could just play it safe and keep the check. >=20 Ok, I will add this check in next version. Thanks -Zhao Qiang ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-06 1:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-05 1:46 [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Zhao Qiang 2016-07-05 1:46 ` [PATCH 2/2] qe/ic: refactor qe_ic to simplify Zhao Qiang 2016-07-05 3:51 ` Jason Cooper 2016-07-05 7:38 ` Qiang Zhao 2016-07-05 13:45 ` Jason Cooper 2016-07-05 3:18 ` [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip Jason Cooper 2016-07-05 7:27 ` Qiang Zhao 2016-07-05 14:21 ` Jason Cooper 2016-07-06 1:18 ` Qiang Zhao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).