Patchwork [15/16] ARM: fiq: save FIQ_START by passing absolute fiq number

login
register
mail settings
Submitter Shawn Guo
Date June 14, 2012, 5:59 a.m.
Message ID <1339653587-4832-16-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/164834/
State New
Headers show

Comments

Shawn Guo - June 14, 2012, 5:59 a.m.
The commit a2be01b (ARM: only include mach/irqs.h for !SPARSE_IRQ)
makes mach/irqs.h only be included for !SPARSE_IRQ build.  There are
a nubmer of platforms have FIQ_START defined in mach/irqs.h.

  arch/arm/mach-at91/include/mach/irqs.h:#define FIQ_START AT91_ID_FIQ
  arch/arm/mach-rpc/include/mach/irqs.h:#define FIQ_START         64
  arch/arm/mach-s3c24xx/include/mach/irqs.h:#define FIQ_START             IRQ_EINT0
  arch/arm/plat-mxc/include/mach/irqs.h:#define FIQ_START 0
  arch/arm/plat-omap/include/plat/irqs.h:#define FIQ_START                1024

If SPARSE_IRQ is enabled for any of these platforms, the following
compile error will be seen.

  arch/arm/kernel/fiq.c: In function ‘enable_fiq’:
  arch/arm/kernel/fiq.c:127:19: error: ‘FIQ_START’ undeclared (first use in this function)
  arch/arm/kernel/fiq.c:127:19: note: each undeclared identifier is reported only once for each function it appears in
  arch/arm/kernel/fiq.c: In function ‘disable_fiq’:
  arch/arm/kernel/fiq.c:132:20: error: ‘FIQ_START’ undeclared (first use in this function)

Though FIQ_START is defined in above 5 platforms, a grep on the whole
tree only reports the following users of enable_fiq/disable_fiq.

  arch/arm/mach-rpc/dma.c
  drivers/media/video/mx1_camera.c
  sound/soc/fsl/imx-pcm-fiq.c

That said, only rpc and imx are actually using enable_fiq/disable_fiq.

The patch changes enable_fiq/disable_fiq a little bit to have the
absolute fiq number than offset passed into by parameter "fiq".  While
fiq on imx starts from 0, only rpc needs a fix-up to adapt the change.

With this change, all those FIQ_START definitions in platform irqs.h
can be removed now, but we chose to leave the decision to platform
maintainers, it should be removed or just left there as a document
on where fiq starts on the platform.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
---
 arch/arm/kernel/fiq.c                 |    4 ++--
 arch/arm/mach-rpc/include/mach/irqs.h |   12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)
Dong Aisheng - June 18, 2012, 8:39 a.m.
On Thu, Jun 14, 2012 at 01:59:46PM +0800, Shawn Guo wrote:
> The commit a2be01b (ARM: only include mach/irqs.h for !SPARSE_IRQ)
> makes mach/irqs.h only be included for !SPARSE_IRQ build.  There are
> a nubmer of platforms have FIQ_START defined in mach/irqs.h.
> 
>   arch/arm/mach-at91/include/mach/irqs.h:#define FIQ_START AT91_ID_FIQ
>   arch/arm/mach-rpc/include/mach/irqs.h:#define FIQ_START         64
>   arch/arm/mach-s3c24xx/include/mach/irqs.h:#define FIQ_START             IRQ_EINT0
>   arch/arm/plat-mxc/include/mach/irqs.h:#define FIQ_START 0
>   arch/arm/plat-omap/include/plat/irqs.h:#define FIQ_START                1024
> 
> If SPARSE_IRQ is enabled for any of these platforms, the following
> compile error will be seen.
> 
>   arch/arm/kernel/fiq.c: In function ‘enable_fiq’:
>   arch/arm/kernel/fiq.c:127:19: error: ‘FIQ_START’ undeclared (first use in this function)
>   arch/arm/kernel/fiq.c:127:19: note: each undeclared identifier is reported only once for each function it appears in
>   arch/arm/kernel/fiq.c: In function ‘disable_fiq’:
>   arch/arm/kernel/fiq.c:132:20: error: ‘FIQ_START’ undeclared (first use in this function)
> 
> Though FIQ_START is defined in above 5 platforms, a grep on the whole
> tree only reports the following users of enable_fiq/disable_fiq.
> 
>   arch/arm/mach-rpc/dma.c
>   drivers/media/video/mx1_camera.c
>   sound/soc/fsl/imx-pcm-fiq.c
> 
> That said, only rpc and imx are actually using enable_fiq/disable_fiq.
> 
> The patch changes enable_fiq/disable_fiq a little bit to have the
> absolute fiq number than offset passed into by parameter "fiq".  While
> fiq on imx starts from 0, only rpc needs a fix-up to adapt the change.
> 
> With this change, all those FIQ_START definitions in platform irqs.h
> can be removed now, but we chose to leave the decision to platform
> maintainers, it should be removed or just left there as a document
> on where fiq starts on the platform.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> ---
>  arch/arm/kernel/fiq.c                 |    4 ++--
>  arch/arm/mach-rpc/include/mach/irqs.h |   12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
> index c32f845..5953bea 100644
> --- a/arch/arm/kernel/fiq.c
> +++ b/arch/arm/kernel/fiq.c
> @@ -124,12 +124,12 @@ void release_fiq(struct fiq_handler *f)
>  
>  void enable_fiq(int fiq)
>  {
> -	enable_irq(fiq + FIQ_START);
> +	enable_irq(fiq);
>  }
>  
>  void disable_fiq(int fiq)
>  {
> -	disable_irq(fiq + FIQ_START);
> +	disable_irq(fiq);
>  }
>  

I don't know why we put FIQ_START in fiq.c, but this looks like a reasonable
solution, so:
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Also waiting for other people's comments.

Regards
Dong Aisheng
Shawn Guo - June 18, 2012, 2:31 p.m.
Hi Russell,

Do you have any comment, or may I have your ack on this patch?

Regards,
Shawn

On Thu, Jun 14, 2012 at 01:59:46PM +0800, Shawn Guo wrote:
> The commit a2be01b (ARM: only include mach/irqs.h for !SPARSE_IRQ)
> makes mach/irqs.h only be included for !SPARSE_IRQ build.  There are
> a nubmer of platforms have FIQ_START defined in mach/irqs.h.
> 
>   arch/arm/mach-at91/include/mach/irqs.h:#define FIQ_START AT91_ID_FIQ
>   arch/arm/mach-rpc/include/mach/irqs.h:#define FIQ_START         64
>   arch/arm/mach-s3c24xx/include/mach/irqs.h:#define FIQ_START             IRQ_EINT0
>   arch/arm/plat-mxc/include/mach/irqs.h:#define FIQ_START 0
>   arch/arm/plat-omap/include/plat/irqs.h:#define FIQ_START                1024
> 
> If SPARSE_IRQ is enabled for any of these platforms, the following
> compile error will be seen.
> 
>   arch/arm/kernel/fiq.c: In function ‘enable_fiq’:
>   arch/arm/kernel/fiq.c:127:19: error: ‘FIQ_START’ undeclared (first use in this function)
>   arch/arm/kernel/fiq.c:127:19: note: each undeclared identifier is reported only once for each function it appears in
>   arch/arm/kernel/fiq.c: In function ‘disable_fiq’:
>   arch/arm/kernel/fiq.c:132:20: error: ‘FIQ_START’ undeclared (first use in this function)
> 
> Though FIQ_START is defined in above 5 platforms, a grep on the whole
> tree only reports the following users of enable_fiq/disable_fiq.
> 
>   arch/arm/mach-rpc/dma.c
>   drivers/media/video/mx1_camera.c
>   sound/soc/fsl/imx-pcm-fiq.c
> 
> That said, only rpc and imx are actually using enable_fiq/disable_fiq.
> 
> The patch changes enable_fiq/disable_fiq a little bit to have the
> absolute fiq number than offset passed into by parameter "fiq".  While
> fiq on imx starts from 0, only rpc needs a fix-up to adapt the change.
> 
> With this change, all those FIQ_START definitions in platform irqs.h
> can be removed now, but we chose to leave the decision to platform
> maintainers, it should be removed or just left there as a document
> on where fiq starts on the platform.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> ---
>  arch/arm/kernel/fiq.c                 |    4 ++--
>  arch/arm/mach-rpc/include/mach/irqs.h |   12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
> index c32f845..5953bea 100644
> --- a/arch/arm/kernel/fiq.c
> +++ b/arch/arm/kernel/fiq.c
> @@ -124,12 +124,12 @@ void release_fiq(struct fiq_handler *f)
>  
>  void enable_fiq(int fiq)
>  {
> -	enable_irq(fiq + FIQ_START);
> +	enable_irq(fiq);
>  }
>  
>  void disable_fiq(int fiq)
>  {
> -	disable_irq(fiq + FIQ_START);
> +	disable_irq(fiq);
>  }
>  
>  EXPORT_SYMBOL(set_fiq_handler);
> diff --git a/arch/arm/mach-rpc/include/mach/irqs.h b/arch/arm/mach-rpc/include/mach/irqs.h
> index 6868e17..4962bdd 100644
> --- a/arch/arm/mach-rpc/include/mach/irqs.h
> +++ b/arch/arm/mach-rpc/include/mach/irqs.h
> @@ -31,15 +31,15 @@
>  #define IRQ_DMAS0		20
>  #define IRQ_DMAS1		21
>  
> -#define FIQ_FLOPPYDATA		0
> -#define FIQ_ECONET		2
> -#define FIQ_SERIALPORT		4
> -#define FIQ_EXPANSIONCARD	6
> -#define FIQ_FORCE		7
> -
>  /*
>   * This is the offset of the FIQ "IRQ" numbers
>   */
>  #define FIQ_START		64
>  
> +#define FIQ_FLOPPYDATA		(FIQ_START + 0)
> +#define FIQ_ECONET		(FIQ_START + 2)
> +#define FIQ_SERIALPORT		(FIQ_START + 4)
> +#define FIQ_EXPANSIONCARD	(FIQ_START + 6)
> +#define FIQ_FORCE		(FIQ_START + 7)
> +
>  #define NR_IRQS			128
> -- 
> 1.7.5.4
> 
>
Russell King - ARM Linux - June 18, 2012, 4:44 p.m.
On Mon, Jun 18, 2012 at 10:31:30PM +0800, Shawn Guo wrote:
> Hi Russell,
> 
> Do you have any comment, or may I have your ack on this patch?

Yes, I haven't seen it, and now that I have I don't like it.

FIQs should be an entirely separate number space from IRQs, as we
may want to totally decouple them from the IRQ stuff (we probably
should have already done this when genirq came along.)

About the only stuff FIQs use is the enable/disable_irq as a short
cut to dealing with the mask registers.
Russell King - ARM Linux - June 20, 2012, 10:55 p.m.
On Tue, Jun 19, 2012 at 01:26:56PM +0800, Shawn Guo wrote:
> On Mon, Jun 18, 2012 at 05:44:02PM +0100, Russell King - ARM Linux wrote:
> > FIQs should be an entirely separate number space from IRQs, as we
> > may want to totally decouple them from the IRQ stuff (we probably
> > should have already done this when genirq came along.)
> > 
> > About the only stuff FIQs use is the enable/disable_irq as a short
> > cut to dealing with the mask registers.
> 
> I do not quite understand what you are asking for, but I'm guessing it
> with the patch below.  Please elaborate it a little bit more if that's
> not what you are asking for.

I was thinking about moving entirely away from any bits of genirq for
this.  We shouldn't really be mixing these two things together by
nabbing some of the IRQ numberspace for this.

Looking at this code, I'm thinking about taking this further, and
santising the whole thing... though that's not going to be a quick
change.
Shawn Guo - June 20, 2012, 11:40 p.m.
On Wed, Jun 20, 2012 at 11:55:20PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 19, 2012 at 01:26:56PM +0800, Shawn Guo wrote:
> > On Mon, Jun 18, 2012 at 05:44:02PM +0100, Russell King - ARM Linux wrote:
> > > FIQs should be an entirely separate number space from IRQs, as we
> > > may want to totally decouple them from the IRQ stuff (we probably
> > > should have already done this when genirq came along.)
> > > 
> > > About the only stuff FIQs use is the enable/disable_irq as a short
> > > cut to dealing with the mask registers.
> > 
> > I do not quite understand what you are asking for, but I'm guessing it
> > with the patch below.  Please elaborate it a little bit more if that's
> > not what you are asking for.
> 
> I was thinking about moving entirely away from any bits of genirq for
> this.  We shouldn't really be mixing these two things together by
> nabbing some of the IRQ numberspace for this.
> 
> Looking at this code, I'm thinking about taking this further, and
> santising the whole thing... though that's not going to be a quick
> change.

What I'm looking for is a quick and reasonable fixing for a compile
error when SPARSE_IRQ is enabled for imx.  Can we take the original
patch as it is and defer the whole cleanup to another series?
Shawn Guo - June 20, 2012, 11:53 p.m.
On Wed, Jun 20, 2012 at 11:55:20PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 19, 2012 at 01:26:56PM +0800, Shawn Guo wrote:
> > On Mon, Jun 18, 2012 at 05:44:02PM +0100, Russell King - ARM Linux wrote:
> > > FIQs should be an entirely separate number space from IRQs, as we
> > > may want to totally decouple them from the IRQ stuff (we probably
> > > should have already done this when genirq came along.)
> > > 
> > > About the only stuff FIQs use is the enable/disable_irq as a short
> > > cut to dealing with the mask registers.
> > 
> > I do not quite understand what you are asking for, but I'm guessing it
> > with the patch below.  Please elaborate it a little bit more if that's
> > not what you are asking for.
> 
> I was thinking about moving entirely away from any bits of genirq for
> this.  We shouldn't really be mixing these two things together by
> nabbing some of the IRQ numberspace for this.
> 
> Looking at this code, I'm thinking about taking this further, and
> santising the whole thing... though that's not going to be a quick
> change.

What I'm looking for is a quick and safe fixing for a compile error
when SPARSE_IRQ enabled for imx.  Can we take the original patch as
it is and defer the whole cleanup to another series?
Russell King - ARM Linux - June 21, 2012, 7:37 a.m.
On Thu, Jun 21, 2012 at 07:53:20AM +0800, Shawn Guo wrote:
> On Wed, Jun 20, 2012 at 11:55:20PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jun 19, 2012 at 01:26:56PM +0800, Shawn Guo wrote:
> > > On Mon, Jun 18, 2012 at 05:44:02PM +0100, Russell King - ARM Linux wrote:
> > > > FIQs should be an entirely separate number space from IRQs, as we
> > > > may want to totally decouple them from the IRQ stuff (we probably
> > > > should have already done this when genirq came along.)
> > > > 
> > > > About the only stuff FIQs use is the enable/disable_irq as a short
> > > > cut to dealing with the mask registers.
> > > 
> > > I do not quite understand what you are asking for, but I'm guessing it
> > > with the patch below.  Please elaborate it a little bit more if that's
> > > not what you are asking for.
> > 
> > I was thinking about moving entirely away from any bits of genirq for
> > this.  We shouldn't really be mixing these two things together by
> > nabbing some of the IRQ numberspace for this.
> > 
> > Looking at this code, I'm thinking about taking this further, and
> > santising the whole thing... though that's not going to be a quick
> > change.
> 
> What I'm looking for is a quick and safe fixing for a compile error
> when SPARSE_IRQ enabled for imx.  Can we take the original patch as
> it is and defer the whole cleanup to another series?

No, I don't want to make FIQ numbers somehow the same as IRQ numbers,
not even "temporarily".
Shawn Guo - June 21, 2012, 8:50 a.m.
On Wed, Jun 20, 2012 at 11:55:20PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 19, 2012 at 01:26:56PM +0800, Shawn Guo wrote:
> > On Mon, Jun 18, 2012 at 05:44:02PM +0100, Russell King - ARM Linux wrote:
> > > FIQs should be an entirely separate number space from IRQs, as we
> > > may want to totally decouple them from the IRQ stuff (we probably
> > > should have already done this when genirq came along.)
> > > 
> > > About the only stuff FIQs use is the enable/disable_irq as a short
> > > cut to dealing with the mask registers.
> > 
> > I do not quite understand what you are asking for, but I'm guessing it
> > with the patch below.  Please elaborate it a little bit more if that's
> > not what you are asking for.
> 
> I was thinking about moving entirely away from any bits of genirq for
> this.  We shouldn't really be mixing these two things together by
> nabbing some of the IRQ numberspace for this.
> 
> Looking at this code, I'm thinking about taking this further, and
> santising the whole thing... though that's not going to be a quick
> change.

Reading the comment, I still do not have any picture about how to
decouple the FIQ from IRQ numberspace.  Could you take a few minutes
to educate specifically, so that I do not have to guess what's the
best way to do that?
Shawn Guo - June 25, 2012, 4:10 p.m.
On Wed, Jun 20, 2012 at 11:55:20PM +0100, Russell King - ARM Linux wrote:
> I was thinking about moving entirely away from any bits of genirq for
> this.  We shouldn't really be mixing these two things together by
> nabbing some of the IRQ numberspace for this.
> 
> Looking at this code, I'm thinking about taking this further, and
> santising the whole thing... though that's not going to be a quick
> change.

Would you mind giving a little more hints on how this should be done
properly?  Sorry for my dumb head.

Patch

diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c
index c32f845..5953bea 100644
--- a/arch/arm/kernel/fiq.c
+++ b/arch/arm/kernel/fiq.c
@@ -124,12 +124,12 @@  void release_fiq(struct fiq_handler *f)
 
 void enable_fiq(int fiq)
 {
-	enable_irq(fiq + FIQ_START);
+	enable_irq(fiq);
 }
 
 void disable_fiq(int fiq)
 {
-	disable_irq(fiq + FIQ_START);
+	disable_irq(fiq);
 }
 
 EXPORT_SYMBOL(set_fiq_handler);
diff --git a/arch/arm/mach-rpc/include/mach/irqs.h b/arch/arm/mach-rpc/include/mach/irqs.h
index 6868e17..4962bdd 100644
--- a/arch/arm/mach-rpc/include/mach/irqs.h
+++ b/arch/arm/mach-rpc/include/mach/irqs.h
@@ -31,15 +31,15 @@ 
 #define IRQ_DMAS0		20
 #define IRQ_DMAS1		21
 
-#define FIQ_FLOPPYDATA		0
-#define FIQ_ECONET		2
-#define FIQ_SERIALPORT		4
-#define FIQ_EXPANSIONCARD	6
-#define FIQ_FORCE		7
-
 /*
  * This is the offset of the FIQ "IRQ" numbers
  */
 #define FIQ_START		64
 
+#define FIQ_FLOPPYDATA		(FIQ_START + 0)
+#define FIQ_ECONET		(FIQ_START + 2)
+#define FIQ_SERIALPORT		(FIQ_START + 4)
+#define FIQ_EXPANSIONCARD	(FIQ_START + 6)
+#define FIQ_FORCE		(FIQ_START + 7)
+
 #define NR_IRQS			128