| Message ID | 1339653587-4832-16-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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.
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.
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?
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?
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".
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?
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.
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
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(-)