| Message ID | 1365386514-14647-1-git-send-email-hongtao.jia@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
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
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
> -----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.
> -----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.
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
> -----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.
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
> -----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.
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
> -----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
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.