| Message ID | 1467683219-29326-1-git-send-email-qiang.zhao@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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.
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. > > > - 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? 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); } > > > - 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? In kernel, there are no boards containing type "qeic" in dtb. -Zhao Qiang BR
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.
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 > > 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 = 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. > Ok, I will add this check in next version. Thanks -Zhao Qiang
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);
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(-)