RE: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip

From: Qiang Zhao
Date: Tue Jul 05 2016 - 07:00:14 EST

On 07/05/2016 11:19 AM, Jason Cooper <jason@xxxxxxxxxxxxxx> wrote:
> -----Original Message-----
> From: Jason Cooper [mailto:jason@xxxxxxxxxxxxxx]
> Sent: Tuesday, July 05, 2016 11:19 AM
> To: Qiang Zhao <qiang.zhao@xxxxxxx>
> Cc: oss@xxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; marc.zyngier@xxxxxxx; linuxppc-
> dev@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Xiaobo Xie
> <xiaobo.xie@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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