Patchwork [V4] powerpc/MPIC: Add get_version API both for internal and external use

login
register
mail settings
Submitter Hongtao Jia
Date April 8, 2013, 2:01 a.m.
Message ID <1365386514-14647-1-git-send-email-hongtao.jia@freescale.com>
Download mbox | patch
Permalink /patch/234558/
State Superseded
Delegated to: Kumar Gala
Headers show

Comments

Hongtao Jia - April 8, 2013, 2:01 a.m.
MPIC version is useful information for both mpic_alloc() and mpic_init().
The patch provide an API to get MPIC version for reusing the code.
Also, some other IP block may need MPIC version for their own use.
The API for external use is also provided.

Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
Changes for V4:
* change the name of function from mpic_get_version() to
  fsl_mpic_get_version().

Changes for V3:
* change the name of function from mpic_primary_get_version() to
  fsl_mpic_primary_get_version().
* return 0 if mpic_primary is null.

Changes for V2:
* Using mpic_get_version() to implement mpic_primary_get_version()

 arch/powerpc/include/asm/mpic.h |  3 +++
 arch/powerpc/sysdev/mpic.c      | 29 ++++++++++++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)
Hongtao Jia - April 10, 2013, 2:22 a.m.
Hi Kumar and Scott,

Any more comments for this patch and MSI-X erratum patch?

Thanks.
-Hongtao.



> -----Original Message-----
> From: Jia Hongtao-B38951
> Sent: Monday, April 08, 2013 10:02 AM
> To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> Cc: Wood Scott-B07421; Li Yang-R58472; Jia Hongtao-B38951
> Subject: [PATCH V4] powerpc/MPIC: Add get_version API both for internal
> and external use
> 
> MPIC version is useful information for both mpic_alloc() and mpic_init().
> The patch provide an API to get MPIC version for reusing the code.
> Also, some other IP block may need MPIC version for their own use.
> The API for external use is also provided.
> 
> Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> Changes for V4:
> * change the name of function from mpic_get_version() to
>   fsl_mpic_get_version().
> 
> Changes for V3:
> * change the name of function from mpic_primary_get_version() to
>   fsl_mpic_primary_get_version().
> * return 0 if mpic_primary is null.
> 
> Changes for V2:
> * Using mpic_get_version() to implement mpic_primary_get_version()
> 
>  arch/powerpc/include/asm/mpic.h |  3 +++
>  arch/powerpc/sysdev/mpic.c      | 29 ++++++++++++++++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/mpic.h
> b/arch/powerpc/include/asm/mpic.h index c0f9ef9..ea6bf72 100644
> --- a/arch/powerpc/include/asm/mpic.h
> +++ b/arch/powerpc/include/asm/mpic.h
> @@ -393,6 +393,9 @@ struct mpic
>  #define	MPIC_REGSET_STANDARD		MPIC_REGSET(0)	/* Original
> MPIC */
>  #define	MPIC_REGSET_TSI108		MPIC_REGSET(1)	/* Tsi108/109
> PIC */
> 
> +/* Get the version of primary MPIC */
> +extern u32 fsl_mpic_primary_get_version(void);
> +
>  /* Allocate the controller structure and setup the linux irq descs
>   * for the range if interrupts passed in. No HW initialization is
>   * actually performed.
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index d30e6a6..48c8fae 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -1165,10 +1165,30 @@ static struct irq_domain_ops mpic_host_ops = {
>  	.xlate = mpic_host_xlate,
>  };
> 
> +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> +	u32 brr1;
> +
> +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> +			MPIC_FSL_BRR1);
> +
> +	return brr1 & MPIC_FSL_BRR1_VER;
> +}
> +
>  /*
>   * Exported functions
>   */
> 
> +u32 fsl_mpic_primary_get_version(void)
> +{
> +	struct mpic *mpic = mpic_primary;
> +
> +	if (mpic)
> +		return fsl_mpic_get_version(mpic);
> +
> +	return 0;
> +}
> +
>  struct mpic * __init mpic_alloc(struct device_node *node,
>  				phys_addr_t phys_addr,
>  				unsigned int flags,
> @@ -1315,7 +1335,6 @@ struct mpic * __init mpic_alloc(struct device_node
> *node,
>  	mpic_map(mpic, mpic->paddr, &mpic->tmregs, MPIC_INFO(TIMER_BASE),
> 0x1000);
> 
>  	if (mpic->flags & MPIC_FSL) {
> -		u32 brr1;
>  		int ret;
> 
>  		/*
> @@ -1326,9 +1345,7 @@ struct mpic * __init mpic_alloc(struct device_node
> *node,
>  		mpic_map(mpic, mpic->paddr, &mpic->thiscpuregs,
>  			 MPIC_CPU_THISBASE, 0x1000);
> 
> -		brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> -				MPIC_FSL_BRR1);
> -		fsl_version = brr1 & MPIC_FSL_BRR1_VER;
> +		fsl_version = fsl_mpic_get_version(mpic);
> 
>  		/* Error interrupt mask register (EIMR) is required for
>  		 * handling individual device error interrupts. EIMR @@ -
> 1518,9 +1535,7 @@ void __init mpic_init(struct mpic *mpic)
>  	mpic_cpu_write(MPIC_INFO(CPU_CURRENT_TASK_PRI), 0xf);
> 
>  	if (mpic->flags & MPIC_FSL) {
> -		u32 brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> -				      MPIC_FSL_BRR1);
> -		u32 version = brr1 & MPIC_FSL_BRR1_VER;
> +		u32 version = fsl_mpic_get_version(mpic);
> 
>  		/*
>  		 * Timer group B is present at the latest in MPIC 3.1 (e.g.
> --
> 1.8.0
Scott Wood - April 10, 2013, 2:32 a.m.
On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index d30e6a6..48c8fae 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -1165,10 +1165,30 @@ static struct irq_domain_ops mpic_host_ops = {
>  	.xlate = mpic_host_xlate,
>  };
> 
> +static u32 fsl_mpic_get_version(struct mpic *mpic)
> +{
> +	u32 brr1;
> +
> +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> +			MPIC_FSL_BRR1);
> +
> +	return brr1 & MPIC_FSL_BRR1_VER;
> +}

If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please check  
mpic->flags for MPIC_FSL.

> +
>  /*
>   * Exported functions
>   */
> 
> +u32 fsl_mpic_primary_get_version(void)
> +{
> +	struct mpic *mpic = mpic_primary;
> +
> +	if (mpic)
> +		return fsl_mpic_get_version(mpic);
> +
> +	return 0;
> +}

...especially since the external version doesn't check for it either.

Otherwise, this and the MSI-X patch look OK to me.

-Scott
Hongtao Jia - April 10, 2013, 2:35 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, April 10, 2013 10:32 AM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott-
> B07421; Li Yang-R58472; Jia Hongtao-B38951
> Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> internal and external use
> 
> On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index d30e6a6..48c8fae 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1165,10 +1165,30 @@ static struct irq_domain_ops mpic_host_ops = {
> >  	.xlate = mpic_host_xlate,
> >  };
> >
> > +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> > +	u32 brr1;
> > +
> > +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > +			MPIC_FSL_BRR1);
> > +
> > +	return brr1 & MPIC_FSL_BRR1_VER;
> > +}
> 
> If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please check
> mpic->flags for MPIC_FSL.

I will add the check soon.

> 
> > +
> >  /*
> >   * Exported functions
> >   */
> >
> > +u32 fsl_mpic_primary_get_version(void)
> > +{
> > +	struct mpic *mpic = mpic_primary;
> > +
> > +	if (mpic)
> > +		return fsl_mpic_get_version(mpic);
> > +
> > +	return 0;
> > +}
> 
> ...especially since the external version doesn't check for it either.
> 
> Otherwise, this and the MSI-X patch look OK to me.
> 
> -Scott

Thanks.
-Hongtao.
Hongtao Jia - April 10, 2013, 3:04 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, April 10, 2013 10:32 AM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott-
> B07421; Li Yang-R58472; Jia Hongtao-B38951
> Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> internal and external use
> 
> On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index d30e6a6..48c8fae 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1165,10 +1165,30 @@ static struct irq_domain_ops mpic_host_ops = {
> >  	.xlate = mpic_host_xlate,
> >  };
> >
> > +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> > +	u32 brr1;
> > +
> > +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > +			MPIC_FSL_BRR1);
> > +
> > +	return brr1 & MPIC_FSL_BRR1_VER;
> > +}
> 
> If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please check
> mpic->flags for MPIC_FSL.
> 
> > +
> >  /*
> >   * Exported functions
> >   */
> >
> > +u32 fsl_mpic_primary_get_version(void)
> > +{
> > +	struct mpic *mpic = mpic_primary;
> > +
> > +	if (mpic)
> > +		return fsl_mpic_get_version(mpic);
> > +
> > +	return 0;
> > +}
> 
> ...especially since the external version doesn't check for it either.
> 
> Otherwise, this and the MSI-X patch look OK to me.
> 
> -Scott


Since all the functions including mpic_alloc() and mpic_init() do the
check for MPIC_FSL before using fsl_mpic_get_version() I'd like to add
check just for fsl_mpic_primary_get_version().

It will be like this:
u32 fsl_mpic_primary_get_version(void)
{
        struct mpic *mpic = mpic_primary;

        if (mpic && (mpic->flags & MPIC_FSL))
                return fsl_mpic_get_version(mpic);

        return 0;
}

Could we reach an agreement here?

Thanks.
-Hongtao.
Scott Wood - April 10, 2013, 3:07 a.m.
On 04/09/2013 10:04:44 PM, Jia Hongtao-B38951 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, April 10, 2013 10:32 AM
> > To: Jia Hongtao-B38951
> > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood  
> Scott-
> > B07421; Li Yang-R58472; Jia Hongtao-B38951
> > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> > internal and external use
> >
> > On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> > > diff --git a/arch/powerpc/sysdev/mpic.c  
> b/arch/powerpc/sysdev/mpic.c
> > > index d30e6a6..48c8fae 100644
> > > --- a/arch/powerpc/sysdev/mpic.c
> > > +++ b/arch/powerpc/sysdev/mpic.c
> > > @@ -1165,10 +1165,30 @@ static struct irq_domain_ops  
> mpic_host_ops = {
> > >  	.xlate = mpic_host_xlate,
> > >  };
> > >
> > > +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> > > +	u32 brr1;
> > > +
> > > +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > > +			MPIC_FSL_BRR1);
> > > +
> > > +	return brr1 & MPIC_FSL_BRR1_VER;
> > > +}
> >
> > If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please  
> check
> > mpic->flags for MPIC_FSL.
> >
> > > +
> > >  /*
> > >   * Exported functions
> > >   */
> > >
> > > +u32 fsl_mpic_primary_get_version(void)
> > > +{
> > > +	struct mpic *mpic = mpic_primary;
> > > +
> > > +	if (mpic)
> > > +		return fsl_mpic_get_version(mpic);
> > > +
> > > +	return 0;
> > > +}
> >
> > ...especially since the external version doesn't check for it  
> either.
> >
> > Otherwise, this and the MSI-X patch look OK to me.
> >
> > -Scott
> 
> 
> Since all the functions including mpic_alloc() and mpic_init() do the
> check for MPIC_FSL before using fsl_mpic_get_version() I'd like to add
> check just for fsl_mpic_primary_get_version().
> 
> It will be like this:
> u32 fsl_mpic_primary_get_version(void)
> {
>         struct mpic *mpic = mpic_primary;
> 
>         if (mpic && (mpic->flags & MPIC_FSL))
>                 return fsl_mpic_get_version(mpic);
> 
>         return 0;
> }
> 
> Could we reach an agreement here?

Is there any particular reason?  It would be more robust and more  
consistent if the check were done in fsl_mpic_get_version().

-Scott
Hongtao Jia - April 10, 2013, 3:10 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, April 10, 2013 11:08 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org; Li Yang-R58472
> Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> internal and external use
> 
> On 04/09/2013 10:04:44 PM, Jia Hongtao-B38951 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, April 10, 2013 10:32 AM
> > > To: Jia Hongtao-B38951
> > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood
> > Scott-
> > > B07421; Li Yang-R58472; Jia Hongtao-B38951
> > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> > > internal and external use
> > >
> > > On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> > > > diff --git a/arch/powerpc/sysdev/mpic.c
> > b/arch/powerpc/sysdev/mpic.c
> > > > index d30e6a6..48c8fae 100644
> > > > --- a/arch/powerpc/sysdev/mpic.c
> > > > +++ b/arch/powerpc/sysdev/mpic.c
> > > > @@ -1165,10 +1165,30 @@ static struct irq_domain_ops
> > mpic_host_ops = {
> > > >  	.xlate = mpic_host_xlate,
> > > >  };
> > > >
> > > > +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> > > > +	u32 brr1;
> > > > +
> > > > +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > > > +			MPIC_FSL_BRR1);
> > > > +
> > > > +	return brr1 & MPIC_FSL_BRR1_VER;
> > > > +}
> > >
> > > If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please
> > check
> > > mpic->flags for MPIC_FSL.
> > >
> > > > +
> > > >  /*
> > > >   * Exported functions
> > > >   */
> > > >
> > > > +u32 fsl_mpic_primary_get_version(void)
> > > > +{
> > > > +	struct mpic *mpic = mpic_primary;
> > > > +
> > > > +	if (mpic)
> > > > +		return fsl_mpic_get_version(mpic);
> > > > +
> > > > +	return 0;
> > > > +}
> > >
> > > ...especially since the external version doesn't check for it
> > either.
> > >
> > > Otherwise, this and the MSI-X patch look OK to me.
> > >
> > > -Scott
> >
> >
> > Since all the functions including mpic_alloc() and mpic_init() do the
> > check for MPIC_FSL before using fsl_mpic_get_version() I'd like to add
> > check just for fsl_mpic_primary_get_version().
> >
> > It will be like this:
> > u32 fsl_mpic_primary_get_version(void)
> > {
> >         struct mpic *mpic = mpic_primary;
> >
> >         if (mpic && (mpic->flags & MPIC_FSL))
> >                 return fsl_mpic_get_version(mpic);
> >
> >         return 0;
> > }
> >
> > Could we reach an agreement here?
> 
> Is there any particular reason?  It would be more robust and more
> consistent if the check were done in fsl_mpic_get_version().
> 
> -Scott

I found out that all the functions using fsl_mpic_get_version() have
already done the check. Adding the check in fsl_mpic_get_version() will
cause duplicate check there. This is my consideration.

-Hongtao.
Scott Wood - April 10, 2013, 3:12 a.m.
On 04/09/2013 10:10:37 PM, Jia Hongtao-B38951 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, April 10, 2013 11:08 AM
> > To: Jia Hongtao-B38951
> > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > galak@kernel.crashing.org; Li Yang-R58472
> > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> > internal and external use
> >
> > On 04/09/2013 10:04:44 PM, Jia Hongtao-B38951 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, April 10, 2013 10:32 AM
> > > > To: Jia Hongtao-B38951
> > > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org;  
> Wood
> > > Scott-
> > > > B07421; Li Yang-R58472; Jia Hongtao-B38951
> > > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both  
> for
> > > > internal and external use
> > > >
> > > > On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> > > > > diff --git a/arch/powerpc/sysdev/mpic.c
> > > b/arch/powerpc/sysdev/mpic.c
> > > > > index d30e6a6..48c8fae 100644
> > > > > --- a/arch/powerpc/sysdev/mpic.c
> > > > > +++ b/arch/powerpc/sysdev/mpic.c
> > > > > @@ -1165,10 +1165,30 @@ static struct irq_domain_ops
> > > mpic_host_ops = {
> > > > >  	.xlate = mpic_host_xlate,
> > > > >  };
> > > > >
> > > > > +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> > > > > +	u32 brr1;
> > > > > +
> > > > > +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > > > > +			MPIC_FSL_BRR1);
> > > > > +
> > > > > +	return brr1 & MPIC_FSL_BRR1_VER;
> > > > > +}
> > > >
> > > > If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please
> > > check
> > > > mpic->flags for MPIC_FSL.
> > > >
> > > > > +
> > > > >  /*
> > > > >   * Exported functions
> > > > >   */
> > > > >
> > > > > +u32 fsl_mpic_primary_get_version(void)
> > > > > +{
> > > > > +	struct mpic *mpic = mpic_primary;
> > > > > +
> > > > > +	if (mpic)
> > > > > +		return fsl_mpic_get_version(mpic);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > >
> > > > ...especially since the external version doesn't check for it
> > > either.
> > > >
> > > > Otherwise, this and the MSI-X patch look OK to me.
> > > >
> > > > -Scott
> > >
> > >
> > > Since all the functions including mpic_alloc() and mpic_init() do  
> the
> > > check for MPIC_FSL before using fsl_mpic_get_version() I'd like  
> to add
> > > check just for fsl_mpic_primary_get_version().
> > >
> > > It will be like this:
> > > u32 fsl_mpic_primary_get_version(void)
> > > {
> > >         struct mpic *mpic = mpic_primary;
> > >
> > >         if (mpic && (mpic->flags & MPIC_FSL))
> > >                 return fsl_mpic_get_version(mpic);
> > >
> > >         return 0;
> > > }
> > >
> > > Could we reach an agreement here?
> >
> > Is there any particular reason?  It would be more robust and more
> > consistent if the check were done in fsl_mpic_get_version().
> >
> > -Scott
> 
> I found out that all the functions using fsl_mpic_get_version() have
> already done the check. Adding the check in fsl_mpic_get_version()  
> will
> cause duplicate check there. This is my consideration.

Does that duplicate check cause any harm?

-Scott
Hongtao Jia - April 10, 2013, 3:14 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, April 10, 2013 11:12 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org; Li Yang-R58472
> Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> internal and external use
> 
> On 04/09/2013 10:10:37 PM, Jia Hongtao-B38951 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, April 10, 2013 11:08 AM
> > > To: Jia Hongtao-B38951
> > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > > galak@kernel.crashing.org; Li Yang-R58472
> > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> > > internal and external use
> > >
> > > On 04/09/2013 10:04:44 PM, Jia Hongtao-B38951 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Wednesday, April 10, 2013 10:32 AM
> > > > > To: Jia Hongtao-B38951
> > > > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org;
> > Wood
> > > > Scott-
> > > > > B07421; Li Yang-R58472; Jia Hongtao-B38951
> > > > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both
> > for
> > > > > internal and external use
> > > > >
> > > > > On 04/07/2013 09:01:54 PM, Jia Hongtao wrote:
> > > > > > diff --git a/arch/powerpc/sysdev/mpic.c
> > > > b/arch/powerpc/sysdev/mpic.c
> > > > > > index d30e6a6..48c8fae 100644
> > > > > > --- a/arch/powerpc/sysdev/mpic.c
> > > > > > +++ b/arch/powerpc/sysdev/mpic.c
> > > > > > @@ -1165,10 +1165,30 @@ static struct irq_domain_ops
> > > > mpic_host_ops = {
> > > > > >  	.xlate = mpic_host_xlate,
> > > > > >  };
> > > > > >
> > > > > > +static u32 fsl_mpic_get_version(struct mpic *mpic) {
> > > > > > +	u32 brr1;
> > > > > > +
> > > > > > +	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > > > > > +			MPIC_FSL_BRR1);
> > > > > > +
> > > > > > +	return brr1 & MPIC_FSL_BRR1_VER; }
> > > > >
> > > > > If it's not an FSL mpic, thiscpuregs->base will be NULL.  Please
> > > > check
> > > > > mpic->flags for MPIC_FSL.
> > > > >
> > > > > > +
> > > > > >  /*
> > > > > >   * Exported functions
> > > > > >   */
> > > > > >
> > > > > > +u32 fsl_mpic_primary_get_version(void)
> > > > > > +{
> > > > > > +	struct mpic *mpic = mpic_primary;
> > > > > > +
> > > > > > +	if (mpic)
> > > > > > +		return fsl_mpic_get_version(mpic);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > >
> > > > > ...especially since the external version doesn't check for it
> > > > either.
> > > > >
> > > > > Otherwise, this and the MSI-X patch look OK to me.
> > > > >
> > > > > -Scott
> > > >
> > > >
> > > > Since all the functions including mpic_alloc() and mpic_init() do
> > the
> > > > check for MPIC_FSL before using fsl_mpic_get_version() I'd like
> > to add
> > > > check just for fsl_mpic_primary_get_version().
> > > >
> > > > It will be like this:
> > > > u32 fsl_mpic_primary_get_version(void)
> > > > {
> > > >         struct mpic *mpic = mpic_primary;
> > > >
> > > >         if (mpic && (mpic->flags & MPIC_FSL))
> > > >                 return fsl_mpic_get_version(mpic);
> > > >
> > > >         return 0;
> > > > }
> > > >
> > > > Could we reach an agreement here?
> > >
> > > Is there any particular reason?  It would be more robust and more
> > > consistent if the check were done in fsl_mpic_get_version().
> > >
> > > -Scott
> >
> > I found out that all the functions using fsl_mpic_get_version() have
> > already done the check. Adding the check in fsl_mpic_get_version()
> > will cause duplicate check there. This is my consideration.
> 
> Does that duplicate check cause any harm?
> 
> -Scott

No harm at all just not necessary.
I wonder if I could add check in fsl_mpic_get_version() and remove all the
check from functions in which using fsl_mpic_get_version()?

-Hongtao.
Scott Wood - April 10, 2013, 3:20 a.m.
On 04/09/2013 10:14:06 PM, Jia Hongtao-B38951 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, April 10, 2013 11:12 AM
> > To: Jia Hongtao-B38951
> > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > galak@kernel.crashing.org; Li Yang-R58472
> > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> > internal and external use
> >
> > On 04/09/2013 10:10:37 PM, Jia Hongtao-B38951 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, April 10, 2013 11:08 AM
> > > > To: Jia Hongtao-B38951
> > > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > > > galak@kernel.crashing.org; Li Yang-R58472
> > > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both  
> for
> > > > internal and external use
> > > >
> > > > On 04/09/2013 10:04:44 PM, Jia Hongtao-B38951 wrote:
> > > > > Since all the functions including mpic_alloc() and  
> mpic_init() do
> > > the
> > > > > check for MPIC_FSL before using fsl_mpic_get_version() I'd  
> like
> > > to add
> > > > > check just for fsl_mpic_primary_get_version().
> > > > >
> > > > > It will be like this:
> > > > > u32 fsl_mpic_primary_get_version(void)
> > > > > {
> > > > >         struct mpic *mpic = mpic_primary;
> > > > >
> > > > >         if (mpic && (mpic->flags & MPIC_FSL))
> > > > >                 return fsl_mpic_get_version(mpic);
> > > > >
> > > > >         return 0;
> > > > > }
> > > > >
> > > > > Could we reach an agreement here?
> > > >
> > > > Is there any particular reason?  It would be more robust and  
> more
> > > > consistent if the check were done in fsl_mpic_get_version().
> > > >
> > > > -Scott
> > >
> > > I found out that all the functions using fsl_mpic_get_version()  
> have
> > > already done the check. Adding the check in fsl_mpic_get_version()
> > > will cause duplicate check there. This is my consideration.
> >
> > Does that duplicate check cause any harm?
> >
> > -Scott
> 
> No harm at all just not necessary.

Not *necessary*, but makes it more robust and more consistent.

> I wonder if I could add check in fsl_mpic_get_version() and remove  
> all the
> check from functions in which using fsl_mpic_get_version()?

One of the two places that calls it is the place that maps thiscpuregs  
in the first place, so no. :-)

The check in mpic_init() for the number of timers could perhaps have  
the check removed if we're comfortable equating a version of zero with  
a non-FSL MPIC.  This really isn't something that's worth worrying  
about, though.

-Scott
Hongtao Jia - April 10, 2013, 3:26 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, April 10, 2013 11:20 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org; Li Yang-R58472
> Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> internal and external use
> 
> On 04/09/2013 10:14:06 PM, Jia Hongtao-B38951 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, April 10, 2013 11:12 AM
> > > To: Jia Hongtao-B38951
> > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > > galak@kernel.crashing.org; Li Yang-R58472
> > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both for
> > > internal and external use
> > >
> > > On 04/09/2013 10:10:37 PM, Jia Hongtao-B38951 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Wednesday, April 10, 2013 11:08 AM
> > > > > To: Jia Hongtao-B38951
> > > > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > > > > galak@kernel.crashing.org; Li Yang-R58472
> > > > > Subject: Re: [PATCH V4] powerpc/MPIC: Add get_version API both
> > for
> > > > > internal and external use
> > > > >
> > > > > On 04/09/2013 10:04:44 PM, Jia Hongtao-B38951 wrote:
> > > > > > Since all the functions including mpic_alloc() and
> > mpic_init() do
> > > > the
> > > > > > check for MPIC_FSL before using fsl_mpic_get_version() I'd
> > like
> > > > to add
> > > > > > check just for fsl_mpic_primary_get_version().
> > > > > >
> > > > > > It will be like this:
> > > > > > u32 fsl_mpic_primary_get_version(void)
> > > > > > {
> > > > > >         struct mpic *mpic = mpic_primary;
> > > > > >
> > > > > >         if (mpic && (mpic->flags & MPIC_FSL))
> > > > > >                 return fsl_mpic_get_version(mpic);
> > > > > >
> > > > > >         return 0;
> > > > > > }
> > > > > >
> > > > > > Could we reach an agreement here?
> > > > >
> > > > > Is there any particular reason?  It would be more robust and
> > more
> > > > > consistent if the check were done in fsl_mpic_get_version().
> > > > >
> > > > > -Scott
> > > >
> > > > I found out that all the functions using fsl_mpic_get_version()
> > have
> > > > already done the check. Adding the check in fsl_mpic_get_version()
> > > > will cause duplicate check there. This is my consideration.
> > >
> > > Does that duplicate check cause any harm?
> > >
> > > -Scott
> >
> > No harm at all just not necessary.
> 
> Not *necessary*, but makes it more robust and more consistent.
> 
> > I wonder if I could add check in fsl_mpic_get_version() and remove
> > all the
> > check from functions in which using fsl_mpic_get_version()?
> 
> One of the two places that calls it is the place that maps thiscpuregs
> in the first place, so no. :-)

Reasonable enough.
I will just add check in fsl_mpic_get_version().

Thanks.
-Hongtao.

> 
> The check in mpic_init() for the number of timers could perhaps have
> the check removed if we're comfortable equating a version of zero with
> a non-FSL MPIC.  This really isn't something that's worth worrying
> about, though.
> 
> -Scott

Patch

diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
index c0f9ef9..ea6bf72 100644
--- a/arch/powerpc/include/asm/mpic.h
+++ b/arch/powerpc/include/asm/mpic.h
@@ -393,6 +393,9 @@  struct mpic
 #define	MPIC_REGSET_STANDARD		MPIC_REGSET(0)	/* Original MPIC */
 #define	MPIC_REGSET_TSI108		MPIC_REGSET(1)	/* Tsi108/109 PIC */
 
+/* Get the version of primary MPIC */
+extern u32 fsl_mpic_primary_get_version(void);
+
 /* Allocate the controller structure and setup the linux irq descs
  * for the range if interrupts passed in. No HW initialization is
  * actually performed.
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index d30e6a6..48c8fae 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -1165,10 +1165,30 @@  static struct irq_domain_ops mpic_host_ops = {
 	.xlate = mpic_host_xlate,
 };
 
+static u32 fsl_mpic_get_version(struct mpic *mpic)
+{
+	u32 brr1;
+
+	brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
+			MPIC_FSL_BRR1);
+
+	return brr1 & MPIC_FSL_BRR1_VER;
+}
+
 /*
  * Exported functions
  */
 
+u32 fsl_mpic_primary_get_version(void)
+{
+	struct mpic *mpic = mpic_primary;
+
+	if (mpic)
+		return fsl_mpic_get_version(mpic);
+
+	return 0;
+}
+
 struct mpic * __init mpic_alloc(struct device_node *node,
 				phys_addr_t phys_addr,
 				unsigned int flags,
@@ -1315,7 +1335,6 @@  struct mpic * __init mpic_alloc(struct device_node *node,
 	mpic_map(mpic, mpic->paddr, &mpic->tmregs, MPIC_INFO(TIMER_BASE), 0x1000);
 
 	if (mpic->flags & MPIC_FSL) {
-		u32 brr1;
 		int ret;
 
 		/*
@@ -1326,9 +1345,7 @@  struct mpic * __init mpic_alloc(struct device_node *node,
 		mpic_map(mpic, mpic->paddr, &mpic->thiscpuregs,
 			 MPIC_CPU_THISBASE, 0x1000);
 
-		brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
-				MPIC_FSL_BRR1);
-		fsl_version = brr1 & MPIC_FSL_BRR1_VER;
+		fsl_version = fsl_mpic_get_version(mpic);
 
 		/* Error interrupt mask register (EIMR) is required for
 		 * handling individual device error interrupts. EIMR
@@ -1518,9 +1535,7 @@  void __init mpic_init(struct mpic *mpic)
 	mpic_cpu_write(MPIC_INFO(CPU_CURRENT_TASK_PRI), 0xf);
 
 	if (mpic->flags & MPIC_FSL) {
-		u32 brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
-				      MPIC_FSL_BRR1);
-		u32 version = brr1 & MPIC_FSL_BRR1_VER;
+		u32 version = fsl_mpic_get_version(mpic);
 
 		/*
 		 * Timer group B is present at the latest in MPIC 3.1 (e.g.