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

2016-07-05 Thread Qiang Zhao 
On 07/05/2016 11:19 AM, Jason Cooper <ja...@lakedaemon.net> wrote:

> -Original Message-
> From: Jason Cooper [mailto:ja...@lakedaemon.net]
> Sent: Tuesday, July 05, 2016 10:22 PM
> To: Qiang Zhao <qiang.z...@nxp.com>
> Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com; linuxppc-
> d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> <xiaobo....@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 +, Qiang Zhao wrote:
> > On 07/05/2016 11:19 AM, Jason Cooper <ja...@lakedaemon.net> wrote:
> > > -Original Message-
> > > From: Jason Cooper [mailto:ja...@lakedaemon.net]
> > > Sent: Tuesday, July 05, 2016 11:19 AM
> > > To: Qiang Zhao <qiang.z...@nxp.com>
> > > Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com;
> > > linuxppc- d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo
> > > Xie <xiaobo@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

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

2016-07-05 Thread Qiang Zhao 
On 07/05/2016 11:19 AM, Jason Cooper  wrote:

> -Original Message-
> From: Jason Cooper [mailto:ja...@lakedaemon.net]
> Sent: Tuesday, July 05, 2016 10:22 PM
> To: Qiang Zhao 
> Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com; linuxppc-
> d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> 
> Subject: Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip
> 
> On Tue, Jul 05, 2016 at 07:27:21AM +, Qiang Zhao wrote:
> > On 07/05/2016 11:19 AM, Jason Cooper  wrote:
> > > -Original Message-
> > > From: Jason Cooper [mailto:ja...@lakedaemon.net]
> > > Sent: Tuesday, July 05, 2016 11:19 AM
> > > To: Qiang Zhao 
> > > Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com;
> > > linuxppc- d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo
> > > Xie 
> > > 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

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

2016-07-05 Thread Jason Cooper 
On Tue, Jul 05, 2016 at 07:27:21AM +, Qiang Zhao wrote:
> On 07/05/2016 11:19 AM, Jason Cooper <ja...@lakedaemon.net> wrote:
> > -Original Message-
> > From: Jason Cooper [mailto:ja...@lakedaemon.net]
> > Sent: Tuesday, July 05, 2016 11:19 AM
> > To: Qiang Zhao <qiang.z...@nxp.com>
> > Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com; linuxppc-
> > d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> > <xiaobo@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.z...@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: interr

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

2016-07-05 Thread Jason Cooper 
On Tue, Jul 05, 2016 at 07:27:21AM +, Qiang Zhao wrote:
> On 07/05/2016 11:19 AM, Jason Cooper  wrote:
> > -Original Message-
> > From: Jason Cooper [mailto:ja...@lakedaemon.net]
> > Sent: Tuesday, July 05, 2016 11:19 AM
> > To: Qiang Zhao 
> > Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com; linuxppc-
> > d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> > 
> > 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 
> > > ---
> > >  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";
&

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

2016-07-05 Thread Qiang Zhao 
On 07/05/2016 11:19 AM, Jason Cooper <ja...@lakedaemon.net> wrote:
> -Original Message-
> From: Jason Cooper [mailto:ja...@lakedaemon.net]
> Sent: Tuesday, July 05, 2016 11:19 AM
> To: Qiang Zhao <qiang.z...@nxp.com>
> Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com; linuxppc-
> d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> <xiaobo....@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.z...@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(

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

2016-07-05 Thread Qiang Zhao 
On 07/05/2016 11:19 AM, Jason Cooper  wrote:
> -Original Message-
> From: Jason Cooper [mailto:ja...@lakedaemon.net]
> Sent: Tuesday, July 05, 2016 11:19 AM
> To: Qiang Zhao 
> Cc: o...@buserror.net; t...@linutronix.de; marc.zyng...@arm.com; linuxppc-
> d...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> 
> 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 
> > ---
> >  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);
>

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

2016-07-04 Thread Jason Cooper 
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 
> ---
>  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
> -

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

2016-07-04 Thread Jason Cooper 
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 
> ---
>  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
> -