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

Qiang Zhao qiang.zhao at nxp.com
Tue Jul 5 17:27:21 AEST 2016


On 07/05/2016 11:19 AM, Jason Cooper <jason at lakedaemon.net> wrote:
> -----Original Message-----
> From: Jason Cooper [mailto:jason at lakedaemon.net]
> Sent: Tuesday, July 05, 2016 11:19 AM
> To: Qiang Zhao <qiang.zhao at nxp.com>
> Cc: oss at buserror.net; tglx at linutronix.de; marc.zyngier at arm.com; linuxppc-
> dev at lists.ozlabs.org; linux-kernel at vger.kernel.org; Xiaobo Xie
> <xiaobo.xie at 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 at 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 at 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


More information about the Linuxppc-dev mailing list