Get rid of FIQ_START/enable/disable_fiq() + some FIQ cleanups

Hello Russell,

During KDB FIQ patches review you mentioned that I should not introduce another FIQ_START. It seems that in v3.6-rc the FIQ_START issue was somewhat band-aided, i.e. machines don't necessary need to define this stuff any longer, but I also read the background of the issue, and you once said that you don't want the FIQ subsystem to mess with genirq.

It is surely makes sense as FIQs are arch-specific, plus FIQs are special in that way that platform makers are free to connects device directly to the FIQ line, avoiding IC routing, and then enable_fiq(<IRQ>) would look awkward, at the best.

So, given that, it seems that the only entity that knows for sure how the particular FIQ is routed is whoever claims the FIQ and fills-in the fiq_handler struct. It might make sense to introduce disable/enable callbacks in the fiq_handler struct, and then prototypes for disable/ enable FIQ calls would look like this:

	enable_fiq(struct fiq_handler *fh);
	disable_fiq(struct fiq_handler *fh);

I.e. completely abstracted from the genirq.

Although, to not duplicate IC code, I think it's pretty legitimate to call genirq functions iff the caller knows for sure that FIQ comes from IRQ line and it is is routed through the well-known platform IC.

In that case (which is a majority), the typical user would do this:

	static int irq_line;

	void foo_enable_fiq(struct fiq_handler *fh)
	{
		enable_irq(irq_line);
	}

	static struct fiq_handler foo_fiq = {
		.name = "foo",
		.enable_fiq = foo_enable_fiq,
	};

	claim_fiq(&foo_fiq);
	enable_fiq(&foo_fiq);

Obviously, it's needless indirection, so I don't do this. If we ever pass 'foo_fiq' to some device, which does not know all the routing details, then it would make sense to introduce it, but not now.

So, the patch set is pretty straightforward:

- Get rid of FIQ_START. Nobody but platform-specific code (and drivers) should know the details about which interrupt can be routed to FIQ and which cannot;

- Remove disable/enable_fiq() calls from the FIQ subsys (the calls can be reintroduced w/ new prototypes when/if needed).

Does the approach make sense?

Thanks!

--

 arch/arm/include/asm/fiq.h                |    2 --
 arch/arm/include/asm/mach/irq.h           |    9 +++++++--
 arch/arm/kernel/fiq.c                     |   22 +++-------------------
 arch/arm/kernel/irq.c                     |    2 --
 arch/arm/mach-rpc/dma.c                   |    4 ++--
 arch/arm/mach-rpc/include/mach/irqs.h     |    8 ++++----
 arch/arm/mach-rpc/irq.c                   |   21 +++++----------------
 arch/arm/mach-s3c24xx/include/mach/irqs.h |    3 ---
 arch/arm/plat-mxc/avic.c                  |    4 +---
 arch/arm/plat-mxc/include/mach/irqs.h     |    2 --
 arch/arm/plat-mxc/tzic.c                  |    4 +---
 arch/arm/plat-omap/include/plat/irqs.h    |    4 ----
 arch/arm/plat-s3c24xx/irq.c               |    6 ++----
 drivers/media/video/mx1_camera.c          |    6 +++---
 sound/soc/fsl/imx-pcm-fiq.c               |    4 ++--
 15 files changed, 30 insertions(+), 71 deletions(-)

Anton Vorontsov
Email: cbouatmailru at gmail.com

mach-rps registers FIQ controller with genirq, which makes no sense:
these FIQs cannot be routed to IRQs, so there is no need to register it with genirq.

This effectively makes FIQ_START irrelevant.

---
 arch/arm/mach-rpc/dma.c               |    4 ++--
 arch/arm/mach-rpc/include/mach/irqs.h |    5 +++++
 arch/arm/mach-rpc/irq.c               |   19 ++++---------------
 3 files changed, 11 insertions(+), 17 deletions(-)

index 85883b2..4a525be 100644
@@ -289,13 +289,13 @@ static void floppy_enable_dma(unsigned int chan, dma_t *dma)
 
 	set_fiq_handler(fiqhandler_start, fiqhandler_length);
 	set_fiq_regs(&regs);
-	enable_fiq(fdma->fiq);
+	iomd_unmask_fiq(fdma->fiq);
 }
 
 static void floppy_disable_dma(unsigned int chan, dma_t *dma)
 {
 	struct floppy_dma *fdma = container_of(dma, struct floppy_dma, dma);
-	disable_fiq(fdma->fiq);
+	iomd_mask_fiq(fdma->fiq);
 	release_fiq(&fh);
 }
 

index 6868e17..f27ead1 100644
@@ -37,6 +37,11 @@
 #define FIQ_EXPANSIONCARD	6
 #define FIQ_FORCE		7
 
+#ifndef __ASSEMBLY__
+extern void iomd_mask_fiq(int fiq);
+extern void iomd_unmask_fiq(int fiq);
+#endif
+
 /*
  * This is the offset of the FIQ "IRQ" numbers
  */

index 3e4fa84..a4221b3 100644
@@ -89,30 +89,24 @@ static struct irq_chip iomd_dma_chip = {
 	.irq_unmask	= iomd_unmask_irq_dma,
 };
 
-static void iomd_mask_irq_fiq(struct irq_data *d)
+void iomd_mask_fiq(int fiq)
 {
 	unsigned int val, mask;
 
-	mask = 1 << (d->irq & 7);
+	mask = 1 << (fiq & 7);
 	val = iomd_readb(IOMD_FIQMASK);
 	iomd_writeb(val & ~mask, IOMD_FIQMASK);
 }
 
-static void iomd_unmask_irq_fiq(struct irq_data *d)
+void iomd_unmask_fiq(int fiq)
 {
 	unsigned int val, mask;
 
-	mask = 1 << (d->irq & 7);
+	mask = 1 << (fiq & 7);
 	val = iomd_readb(IOMD_FIQMASK);
 	iomd_writeb(val | mask, IOMD_FIQMASK);
 }
 
-static struct irq_chip iomd_fiq_chip = {
-	.irq_ack	= iomd_mask_irq_fiq,
-	.irq_mask	= iomd_mask_irq_fiq,
-	.irq_unmask	= iomd_unmask_irq_fiq,
-};
-
 extern unsigned char rpc_default_fiq_start, rpc_default_fiq_end;
 
 void __init rpc_init_irq(void)

@@ -155,11 +149,6 @@ void __init rpc_init_irq(void)
 						 handle_level_irq);
 			set_irq_flags(irq, flags);
 			break;
-
-		case 64 ... 71:
-			irq_set_chip(irq, &iomd_fiq_chip);
-			set_irq_flags(irq, IRQF_VALID);
-			break;
 		}
 	}
 

1.7.10.4

We're about to remove FIQ_START mess, so move the platform-specific detail inside platform-specific s3c24xx_set_fiq().

---
 arch/arm/plat-s3c24xx/irq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fe57bbb..e4e9567 100644
@@ -503,7 +503,7 @@ int s3c24xx_set_fiq(unsigned int irq, bool on)
 	unsigned offs;
 
 	if (on) {
-		offs = irq - FIQ_START;
+		offs = irq - IRQ_EINT0;
 		if (offs > 31)
 			return -EINVAL;
 

1.7.10.4

BTW, how was going on the 'change FIQ_START to a variable' patch from Shawn Guo?
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/106486.html

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Thanks!

It's in Linus' tree already. My patch set is just a next step in the same direction.

Anton Vorontsov
Email: cbouatmailru at gmail.com

The driver uses platform-specific mxc_set_irq_fiq() with the VIRQ cookie passed to it, so it's pretty clear that the driver is absolutely sure that the FIQ is routed via platform-specific IC, and that the cookie can be used to mask/unmask FIQs. So, let's switch to the genirq routines, since we're about to remove FIQ-specific variants.

---
 drivers/media/video/mx1_camera.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index d2e6f82..1536d09 100644
@@ -801,7 +801,7 @@ static int __init mx1_camera_probe(struct platform_device *pdev)
 	set_fiq_regs(&regs);
 
 	mxc_set_irq_fiq(irq, 1);
-	enable_fiq(irq);
+	enable_irq(irq);
 
 	pcdev->soc_host.drv_name	= DRIVER_NAME;
 	pcdev->soc_host.ops		= &mx1_soc_camera_host_ops;

@@ -817,7 +817,7 @@ static int __init mx1_camera_probe(struct platform_device *pdev)
 	return 0;
 
 exit_free_irq:
-	disable_fiq(irq);
+	disable_irq(irq);
 	mxc_set_irq_fiq(irq, 0);
 	release_fiq(&fh);
 exit_free_dma:

@@ -842,7 +842,7 @@ static int __exit mx1_camera_remove(struct platform_device *pdev)
 	struct resource *res;
 
 	imx_dma_free(pcdev->dma_chan);
-	disable_fiq(pcdev->irq);
+	disable_irq(pcdev->irq);
 	mxc_set_irq_fiq(pcdev->irq, 0);
 	release_fiq(&fh);
 

1.7.10.4

The driver uses platform-specific mxc_set_irq_fiq() with the VIRQ cookie passed to it, so it's pretty clear that the driver is absolutely sure that the FIQ is routed via platform-specific IC, and that the cookie can be used to mask/unmask FIQs. So, let's switch to the genirq routines, since we're about to remove FIQ-specific variants.

---
 sound/soc/fsl/imx-pcm-fiq.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ee27ba3..993e37d 100644
@@ -139,7 +139,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		hrtimer_start(&iprtd->hrt, ns_to_ktime(iprtd->poll_time_ns),
 		      HRTIMER_MODE_REL);
 		if (++fiq_enable == 1)
-			enable_fiq(imx_pcm_fiq);
+			enable_irq(imx_pcm_fiq);
 
 		break;
 

@@ -149,7 +149,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		atomic_set(&iprtd->running, 0);
 
 		if (--fiq_enable == 0)
-			disable_fiq(imx_pcm_fiq);
+			disable_irq(imx_pcm_fiq);
 
 		break;
 	default:

1.7.10.4

I have a semi-related question about this;

Firstly, I was planning on (re-)submitting a patch for the arch/arm/plat-mxc/ssi-fiq.S code which made it build in ARM mode (since the code isn't Thumb compatible for various reasons) as it was a blocker for a Thumb2-compiled kernel. Since the code was only needed on ARM-capable processors it wouldn't cause a problem. Sascha signed off on this a long while back and I've been testing it on all my internal kernel versions, and I don't see any ill effects (that said I don't have an i.MX28 or so to really verify it, I can't see why it would not work). I realise this is redundant right now, anyway, since it's only really enabled on imx_v4_v5 configs and they don't support Thumb2 kernels anyway. What might be worth submitting is a switch to add the ".arm" directive anyway simply for correctness since it could never be compiled for Thumb anyway. We all know what gnu as is like.

Looking at it again on the back of these patches, I noticed the ssi-fiq.S code is compiled in when SND_IMX_SOC is enabled - of course, it's only needed in the kernel if SND_SOC_IMX_PCM_FIQ is enabled (the code that uses the FIQ stuff is only compiled then) but here on the Efika MX builds it's being built, and I noticed it when it broke my build because of the above. I'm therefore going to submit a patch which changes the ifdef SND_SOC_IMX to ifdef SND_SOC_IMX_PCM_FIQ so it's not compiled in unless absolutely necessary. However, there was a rumble that this code may disappear or be reworked in the future making this also quite redundant. Since it's not in the imx_v6_v7_defconfig anyway, I'm sure this only didn't get noticed because nobody's building Thumb2 kernels and nobody is trying configs with audio enabled anyway..

This begs the question, could there be something somewhere hidden deep in the kernel that is enabled by default or in some config somewhere that requires "select FIQ" in it's Kconfig entry, but isn't being enabled? On i.MX the only thing turning it on is that code, but other ARM arch enable it by default. Since things have been moved to more generic routines I can't in my mind guarantee that such a patch would be well tested here since I would be relying on symbols missing or defines not there anymore.. I have no real way to ensure that it would work on boards I don't have.

So, is the first patch (ssi-fiq.S .arm) worth it? I think the SND_SOC_IMX_PCM_FIQ patch is worth it for v6_v7 systems anyway, but maybe this should have been caught sooner, so should I update the defconfig to enable some kind of audio bus support (Babbage has it in the DT so is a needful thing for testing, I figure..)? And does anyone forsee any problems with that option changing and the only "user" of "FIQ" in the Kconfigs going away now all the FIQ-specific symbols went away outside of the generic irq subsystem?

I will probably throw out all three today anyway, but I thought it would make a good discussion anyway.

Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.

There's no point in the FIQ driver if the DMA drivers are supported.

http://patchwork.ozlabs.org/patch/128853/

Russell's sage advise; "It's worth pointing out that people end up using FIQs for certain things
because the hardware requires you to do it. So if a platform is using them, they're probably not
doing it out of choice, but are doing it because it's a baseline requirement to get something working."

I don't recall Sascha's response to this, I thought it was on the ML but it may have been in private,
but something is broken on the MX27/28 SSI FIFO which means FIQ is the only way to get reliable
audio since the DMA unit cannot get accurate alarms (this seems a common bug in Freescale
processors :) - I'll let him elaborate since I've never even seen an MX28 let alone booted one, and
our MX27 board never got tested without the FIQ code if I recall correctly.

So that needs to stay, the issue here is why did nobody catch ssi-fiq.S breaking in testing MX51
Babbage and building a Thumb2 kernel, for example? Why did nobody notice it was building when
configuring for MX3/5/6 boards (which do actually have working SSI and DMA, as you assume, and
do not need this code, nor build the imx-pcm-dma-fiq part of the driver anyway)? I'm willing to fix all
of the above, but if there's an obvious deficiency in testing at some point we need to fix that too..

Of course if Sascha says the fiq dma hack can disappear forever that's absolutely fine, I'm also
willing to be the one to delete it... :)

Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.

As far as I can tell nobody's really running much up to date mainline on older i.MX processors, all the work is going on the new stuff and most of the board are on either vendor BSPs or older kernels.

That's not true; we still run MX25, MX27, MX35, MX28 on mainline in active projects.

rsc

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

I think Shawn Guo (FSL/Linaro) would also disagree, since he's just posted a large amount of MXS patches to fix up the board for device trees, and Arnd is pulling them.

Work is ongoing, it would be awful to delete something people really relied on or were in progress fixing (ahem). If they get up to audio in the near future the audio FIQ stuff would just end up being resubmitted almost verbatim... that seems like unnecessary churn.

As far as I know, the FIQ usage is quite valid for the processor it needs to run on (MX21/27/28, right?) in the modes it runs in (AC97 on these processors, and maybe MX35 too), and I'm just trying to figure out what the steps are for

* making sure it doesn't get built for architectures/variants it's certainly NOT required on (imx_v6_v7_defconfig, if it actually enabled audio, that is - in fact, audio should be enabled as more one of the boards supported defines it in the device tree) which would amount to two seperate patches, one for the defconfig, one for the CONFIG item.

I did note that SND_SOC_EUKREA_TLV320 enables FIQ but it also depends on MX51 which makes me think this need to be split, too, so that MX51 boards don't have it but MX2/MX3 do.

* if it is built then it's built as ARM code (redundant, as previously stated, but would have stopped me from swearing at my build box when I hit the issue yet again) which could be a patch or could be ignored. I'd rather discuss this here than clutter the ML with several respins of a patch, let's get an opinion first - to .arm or no to .arm?

* make sure there's no weird FIQ stuff floating around that has so far relied on SND_SOC_IMX_PCM_FIQ doing select FIQ before I make it not compile in for other boards (since otherwise no i.MX processor selects FIQ if they're not using that driver, it could be simply coincidence nobody noticed). I don't want to be the one submitting a patch I can't test which mysteriously breaks every MX28 on the planet (since Rob, Shawn, Sascha might be the ones swearing at me instead)

* making sure someone's actually testing audio as above... and where/if/when the MX28 audio stuff is going in the future und so weiter..

I guess I need Sascha to pipe up and tell me what the code is needed for exactly again.. and someone to test the result of the changes..

Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.

MXS != i.MX.

Oh, ick. What is the problem that means people want to use the FIQ on these processors? There's been i.MX2x audio DMA since forever, it had DMA in mainline before any of the later i.MXs due to them having SDMA. The original mainline i.MX audio support was done on i.MX27 and didn't have any issue here, this is the fist I've heard of a problem.

Is that not bitrot due to it being there before SDMA support was?

For SSI in I2S mode, I think it works great, AC97 not so much. It's the AC97 mode
the FIQ stuff is there to fix. It seems a bunch of boards use AC97 codecs (for no
good reason)

Possibly. I'm going to wait for Sascha to explain it again.. if AC97 was the predicate
for FIQ being required, it doesn't make any sense for a codec that says in it's description that it's I2S. It may not be bitrot but pure, unadulterated zeal on the part
of whoever wrote that Kconfig snippet - enable everything, in case it fails (actually
that kind of thing is why I fear for nuking the building of this driver since if FIQ stuff
is silently required by something else and all that kept it in there was some badly
thought out or bitrotted Kconfig entries... I don't want to cause a world of hurt).

Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.

Originally the FIQ support was only used because there hasn't been SDMA support available at that time.
Nowadays the FIQ support is necessary only for AC97. The AC97 support in the SSI unit is buggy: It does not allow you to select the slots you want to receive. At least the wm9712 codec always sends (apart from the stereo data) data in slot (I think it is) 12. You find this data mixed in your audio stream. The FIQ driver skips this data to get a valid audio stream.

One other way to solve this would be to use dma here and to filter out the data afterwards.

Sascha

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Right, any device with GPIO support will do this - it's how GPIO works in AC'97.

Yup. That's probably more sane but also more work to implement.

Acked on changing SND_IMX_SOC to SND_SOC_IMX_PCM_FIQ in arch/arm/plat-mxc/Makefile.

Though i.MX23 and i.MX28 are named in i.MX family, they are actually a different architecture from IMX/MXC. It's MXS, nothing to do with SSI. Folder sound/soc/mxs/ is the one for i.MX28.

Regards,
Shawn

I hit this issue some time ago when I was trying to do a Thumb2 build of this kernel.

I don't remember having to fix the generic FIQ code just for this, but I had a patch somewhere to swap r13 with another register in ssi-fiq.S. I think that use of r13 in ways not permitted for Thumb was the only problem I found. I can try to dig it out if you want.

In any case, it didn't seem worth pushing at the time, because it seemed that the SSI FIQ code was not relevant to any v7 or later platform -- so building that code for Thumb presumably doesn't make sense.

Cheers
---Dave

There are no users left, so these can be removed.

---
 arch/arm/include/asm/fiq.h |    2 --
 arch/arm/kernel/fiq.c      |   15 ---------------
 2 files changed, 17 deletions(-)

index d493d0b..a293be4 100644
@@ -36,8 +36,6 @@ struct fiq_handler {
 extern int claim_fiq(struct fiq_handler *f);
 extern void release_fiq(struct fiq_handler *f);
 extern void set_fiq_handler(void *start, unsigned int length);
-extern void enable_fiq(int fiq);
-extern void disable_fiq(int fiq);
 
 /* helpers defined in fiqasm.S: */
 extern void __set_fiq_regs(unsigned long const *regs);

index 2adda11..29b93b8 100644
@@ -122,28 +122,13 @@ void release_fiq(struct fiq_handler *f)
 	while (current_fiq->fiq_op(current_fiq->dev_id, 0));
 }
 
-static int fiq_start;
-
-void enable_fiq(int fiq)
-{
-	enable_irq(fiq + fiq_start);
-}
-
-void disable_fiq(int fiq)
-{
-	disable_irq(fiq + fiq_start);
-}
-
 EXPORT_SYMBOL(set_fiq_handler);
 EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(__get_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(claim_fiq);
 EXPORT_SYMBOL(release_fiq);
-EXPORT_SYMBOL(enable_fiq);
-EXPORT_SYMBOL(disable_fiq);
 
 void __init init_FIQ(int start)
 {
 	no_fiq_insn = *(unsigned long *)0xffff001c;
-	fiq_start = start;
 }

1.7.10.4

RPC:

FIQ_START is irrelevant nowadays, the arch uses platform-specific iomd_{,un}mask_fiq() calls.

OMAP1:

The only user of FIQs is MACH_AMS_DELTA, and in particular its drivers/input/serio/ams_delta_serio.c driver. The driver does not rely on the FIQ interrupts directly, instead it uses a "deffered fiq" interrupt (raised after a buffer filled by the FIQ routine).

The FIQ handling routines are not using disable_fiq() or enable_fiq() stuff, so it currently does not use FIQ_START at all -- the asm code uses OMAP_IH1_BASE and twiddles the bits itself.

S3C:

The only user of FIQs on s3c24xx is spi-s3c24xx driver. The driver works with interrupt controller directly (via S3C24XX_VA_IRQ base address), and does not use IRQ subsystem to mask/unmask IRQs.

MXC:

Users now use enable/disable_irq() routines. The drivers rely on a correctly passed VIRQ cookie anyway, so FIQ_START becomes irrelevant.

---
 arch/arm/include/asm/mach/irq.h           |    2 +-
 arch/arm/kernel/fiq.c                     |    2 +-
 arch/arm/mach-rpc/include/mach/irqs.h     |    5 -----
 arch/arm/mach-rpc/irq.c                   |    2 +-
 arch/arm/mach-s3c24xx/include/mach/irqs.h |    3 ---
 arch/arm/plat-mxc/avic.c                  |    2 +-
 arch/arm/plat-mxc/include/mach/irqs.h     |    2 --
 arch/arm/plat-mxc/tzic.c                  |    2 +-
 arch/arm/plat-omap/include/plat/irqs.h    |    4 ----
 arch/arm/plat-s3c24xx/irq.c               |    2 +-
 10 files changed, 6 insertions(+), 20 deletions(-)

index 15cb035..febe495 100644
@@ -17,7 +17,7 @@ struct seq_file;
 /*
  * This is internal.  Do not use it.
  */
-extern void init_FIQ(int);
+extern void init_FIQ(void);
 extern int show_fiq_list(struct seq_file *, int);
 
 #ifdef CONFIG_MULTI_IRQ_HANDLER

index 29b93b8..bd369c5 100644
@@ -128,7 +128,7 @@ EXPORT_SYMBOL(__get_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(claim_fiq);
 EXPORT_SYMBOL(release_fiq);
 
-void __init init_FIQ(int start)
+void __init init_FIQ(void)
 {
 	no_fiq_insn = *(unsigned long *)0xffff001c;
 }

index f27ead1..2536543 100644
@@ -42,9 +42,4 @@ extern void iomd_mask_fiq(int fiq);
 extern void iomd_unmask_fiq(int fiq);
 #endif
 
-/*
- * This is the offset of the FIQ "IRQ" numbers
- */
-#define FIQ_START		64
-
 #define NR_IRQS			128

index a4221b3..07770c8 100644
@@ -152,6 +152,6 @@ void __init rpc_init_irq(void)
 		}
 	}
 
-	init_FIQ(FIQ_START);
+	init_FIQ();
 }
 

index b7a9f4d..7d66d41 100644
@@ -209,7 +209,4 @@
 #define IRQ_S3C244X_AC97 IRQ_S3C2443_AC97
 #endif
 
-/* Our FIQs are routable from IRQ_EINT0 to IRQ_ADCPARENT */
-#define FIQ_START		IRQ_EINT0
-
 #endif /* __ASM_ARCH_IRQ_H */

index cbd55c3..19701ec 100644
@@ -218,7 +218,7 @@ void __init mxc_init_irq(void __iomem *irqbase)
 
 #ifdef CONFIG_FIQ
 	/* Initialize FIQ */
-	init_FIQ(FIQ_START);
+	init_FIQ();
 #endif
 
 	printk(KERN_INFO "MXC IRQ initialized\n");

index d73f5e8..2ec942f 100644
@@ -13,8 +13,6 @@
 
 extern int imx_irq_set_priority(unsigned char irq, unsigned char prio);
 
-/* all normal IRQs can be FIQs */
-#define FIQ_START	0
 /* switch between IRQ and FIQ */
 extern int mxc_set_irq_fiq(unsigned int irq, unsigned int type);
 

index 3ed1adb..d09b573 100644
@@ -193,7 +193,7 @@ void __init tzic_init_irq(void __iomem *irqbase)
 
 #ifdef CONFIG_FIQ
 	/* Initialize FIQ */
-	init_FIQ(FIQ_START);
+	init_FIQ();
 #endif
 
 	pr_info("TrustZone Interrupt Controller (TZIC) initialized\n");

index 37bbbbb..d2b91e3c 100644
@@ -446,8 +446,4 @@
 
 #include <mach/hardware.h>
 
-#ifdef CONFIG_FIQ
-#define FIQ_START		1024
-#endif
-
 #endif

index e4e9567..e0de92a 100644
@@ -533,7 +533,7 @@ void __init s3c24xx_init_irq(void)
 	int i;
 
 #ifdef CONFIG_FIQ
-	init_FIQ(FIQ_START);
+	init_FIQ();
 #endif
 
 	irqdbf("s3c2410_init_irq: clearing interrupt status flags\n");

1.7.10.4

The patch fixes the following sparse warnings:

CHECK arch/arm/kernel/fiq.c
arch/arm/kernel/fiq.c:71:6: warning: symbol 'show_fiq_list' was not declared. Should it be static? arch/arm/kernel/fiq.c:129:13: warning: symbol 'init_FIQ' was not declared. Should it be static?

---
 arch/arm/kernel/fiq.c |    1 +
 1 file changed, 1 insertion(+)

index bd369c5..7be2e74 100644
@@ -46,6 +46,7 @@
 #include <asm/fiq.h>
 #include <asm/irq.h>
 #include <asm/traps.h>
+#include <asm/mach/irq.h>
 
 static unsigned long no_fiq_insn;
 

1.7.10.4

Simply removes ugly #ifdefs from C code.

---
 arch/arm/include/asm/mach/irq.h |    5 +++++
 arch/arm/kernel/irq.c           |    2 --
 arch/arm/plat-mxc/avic.c        |    2 --
 arch/arm/plat-mxc/tzic.c        |    2 --
 arch/arm/plat-s3c24xx/irq.c     |    2 --
 5 files changed, 5 insertions(+), 8 deletions(-)

index febe495..420d211 100644
@@ -17,8 +17,13 @@ struct seq_file;
 /*
  * This is internal.  Do not use it.
  */
+#ifdef CONFIG_FIQ
 extern void init_FIQ(void);
 extern int show_fiq_list(struct seq_file *, int);
+#else
+static inline void init_FIQ(void) {}
+static inline int show_fiq_list(struct seq_file *p, int prec) { return 0; }
+#endif
 
 #ifdef CONFIG_MULTI_IRQ_HANDLER
 extern void (*handle_arch_irq)(struct pt_regs *);

index 16cedb4..8013ad9 100644
@@ -44,9 +44,7 @@ unsigned long irq_err_count;
 
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
-#ifdef CONFIG_FIQ
 	show_fiq_list(p, prec);
-#endif
 #ifdef CONFIG_SMP
 	show_ipi_list(p, prec);
 #endif

index 19701ec..426980c 100644
@@ -216,10 +216,8 @@ void __init mxc_init_irq(void __iomem *irqbase)
 	for (i = 0; i < 8; i++)
 		__raw_writel(0, avic_base + AVIC_NIPRIORITY(i));
 
-#ifdef CONFIG_FIQ
 	/* Initialize FIQ */
 	init_FIQ();
-#endif
 
 	printk(KERN_INFO "MXC IRQ initialized\n");
 }

index d09b573..8a5a633 100644
@@ -191,10 +191,8 @@ void __init tzic_init_irq(void __iomem *irqbase)
 	for (i = 0; i < 4; i++, irq_base += 32)
 		tzic_init_gc(i, irq_base);
 
-#ifdef CONFIG_FIQ
 	/* Initialize FIQ */
 	init_FIQ();
-#endif
 
 	pr_info("TrustZone Interrupt Controller (TZIC) initialized\n");
 }

index e0de92a..531d6a4 100644
@@ -532,9 +532,7 @@ void __init s3c24xx_init_irq(void)
 	int irqno;
 	int i;
 
-#ifdef CONFIG_FIQ
 	init_FIQ();
-#endif
 
 	irqdbf("s3c2410_init_irq: clearing interrupt status flags\n");
 

1.7.10.4

The return value is never checked, so we can turn show_fiq_list() into returning void.

---
 arch/arm/include/asm/mach/irq.h |    4 ++--
 arch/arm/kernel/fiq.c           |    4 +---
 2 files changed, 3 insertions(+), 5 deletions(-)

index 420d211..8be5ba9 100644
@@ -19,10 +19,10 @@ struct seq_file;
  */
 #ifdef CONFIG_FIQ
 extern void init_FIQ(void);
-extern int show_fiq_list(struct seq_file *, int);
+extern void show_fiq_list(struct seq_file *, int);
 #else
 static inline void init_FIQ(void) {}
-static inline int show_fiq_list(struct seq_file *p, int prec) { return 0; }
+static inline void show_fiq_list(struct seq_file *p, int prec) {}
 #endif
 
 #ifdef CONFIG_MULTI_IRQ_HANDLER

index 7be2e74..9bf3a60 100644
@@ -69,13 +69,11 @@ static struct fiq_handler default_owner = {
 
 static struct fiq_handler *current_fiq = &default_owner;
 
-int show_fiq_list(struct seq_file *p, int prec)
+void show_fiq_list(struct seq_file *p, int prec)
 {
 	if (current_fiq != &default_owner)
 		seq_printf(p, "%*s:              %s\n", prec, "FIQ",
 			current_fiq->name);
-
-	return 0;
 }
 
 void set_fiq_handler(void *start, unsigned int length)

1.7.10.4

Hello Russell,

Just wonder if you had a chance to look into this patch set...

Plus, I also realized that maybe it's a good time to get rid of init_FIQ() altogether? Maybe there are some not so obvious reasons for its existence, but if not, does the following patch on top of this series makes sense?

(Also, I noticed that we save no_fiq_insn w/o bothering about !CONFIG_CPU_USE_DOMAINS case, but set_fiq_handler() has a special handling for this. I wonder, was that on purpose, e.g. it does not matter for init_FIQ(), or it was just overlooked? In latter case, the patch fixes the issue.)

- - - -
[PATCH] ARM: FIQ: Get rid of init_FIQ()

The function only saves initial arch-specific "no FIQ" instruction, by placing the code into set_fiq_handler() we can:

- Have less code and logic in the platform-specific files;
- Have the code that manages FIQ vector overwriting in one place, i.e. not spread the logic around (both code placement wise and execution time wise).

---
 arch/arm/include/asm/mach/irq.h |  2 --
 arch/arm/kernel/fiq.c           | 17 ++++++++---------
 arch/arm/mach-rpc/irq.c         |  2 --
 arch/arm/plat-mxc/avic.c        |  3 ---
 arch/arm/plat-mxc/tzic.c        |  3 ---
 arch/arm/plat-s3c24xx/irq.c     |  2 --
 6 files changed, 8 insertions(+), 21 deletions(-)

index 8be5ba9..6e70ae3 100644
@@ -18,10 +18,8 @@ struct seq_file;
  * This is internal.  Do not use it.
  */
 #ifdef CONFIG_FIQ
-extern void init_FIQ(void);
 extern void show_fiq_list(struct seq_file *, int);
 #else
-static inline void init_FIQ(void) {}
 static inline void show_fiq_list(struct seq_file *p, int prec) {}
 #endif
 

index 9bf3a60..3602df6 100644
@@ -49,6 +49,7 @@
 #include <asm/mach/irq.h>
 
 static unsigned long no_fiq_insn;
+static int got_no_fiq_insn;
 
 /* Default reacquire function
  * - we always relinquish FIQ control

@@ -78,11 +79,14 @@ void show_fiq_list(struct seq_file *p, int prec)
 
 void set_fiq_handler(void *start, unsigned int length)
 {
-#if defined(CONFIG_CPU_USE_DOMAINS)
-	memcpy((void *)0xffff001c, start, length);
-#else
-	memcpy(vectors_page + 0x1c, start, length);
+	unsigned long *addr = (void *)0xffff001c;
+
+#ifndef CONFIG_CPU_USE_DOMAINS
+	addr = vectors_page + 0x1c;
 #endif
+	if (!cmpxchg(&got_no_fiq_insn, 0, 1))
+		no_fiq_insn = *addr;
+	memcpy(addr, start, length);
 	flush_icache_range(0xffff001c, 0xffff001c + length);
 	if (!vectors_high())
 		flush_icache_range(0x1c, 0x1c + length);

@@ -126,8 +130,3 @@ EXPORT_SYMBOL(__set_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(__get_fiq_regs);	/* defined in fiqasm.S */
 EXPORT_SYMBOL(claim_fiq);
 EXPORT_SYMBOL(release_fiq);
-
-void __init init_FIQ(void)
-{
-	no_fiq_insn = *(unsigned long *)0xffff001c;
-}

index 07770c8..2d915ba 100644
@@ -151,7 +151,5 @@ void __init rpc_init_irq(void)
 			break;
 		}
 	}
-
-	init_FIQ();
 }
 

index 426980c..b173dc6 100644
@@ -216,8 +216,5 @@ void __init mxc_init_irq(void __iomem *irqbase)
 	for (i = 0; i < 8; i++)
 		__raw_writel(0, avic_base + AVIC_NIPRIORITY(i));
 
-	/* Initialize FIQ */
-	init_FIQ();
-
 	printk(KERN_INFO "MXC IRQ initialized\n");
 }

index 8a5a633..889267c 100644
@@ -191,9 +191,6 @@ void __init tzic_init_irq(void __iomem *irqbase)
 	for (i = 0; i < 4; i++, irq_base += 32)
 		tzic_init_gc(i, irq_base);
 
-	/* Initialize FIQ */
-	init_FIQ();
-
 	pr_info("TrustZone Interrupt Controller (TZIC) initialized\n");
 }
 

index 531d6a4..66b8856 100644
@@ -532,8 +532,6 @@ void __init s3c24xx_init_irq(void)
 	int irqno;
 	int i;
 
-	init_FIQ();
-
 	irqdbf("s3c2410_init_irq: clearing interrupt status flags\n");
 
 	/* first, clear all interrupts pending... */

1.7.11.5