[RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Kuninori Morimoto]

R-Car H1 datasheet GIC number is indicating
GIC ICCIAR / interrupt ID number, not SPI number,
but current marzen board code is using gic_spi() with
un-understandable calculation.

This patch adds new gic_iid() macro which means
ICCIAR / interrupt ID, and used the number
currently written on datasheet.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - gic_iic() -> gic_iid()
 - update git log comments

 arch/arm/mach-shmobile/board-marzen.c      |   12 ++++++------
 arch/arm/mach-shmobile/include/mach/irqs.h |    1 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
index cdcb799..2338d4b 100644
--- a/arch/arm/mach-shmobile/board-marzen.c
+++ b/arch/arm/mach-shmobile/board-marzen.c
@@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
 		.flags		= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start		= gic_spi(28), /* IRQ 1 */
+		.start		= gic_iid(0x3c), /* IRQ 1 */
 		.flags		= IORESOURCE_IRQ,
 	},
 };
@@ -96,7 +96,7 @@ static struct resource sdhi0_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start	= gic_spi(104),
+		.start	= gic_iid(0x88),
 		.flags	= IORESOURCE_IRQ,
 	},
 };
@@ -214,7 +214,7 @@ static struct resource ehci0_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start	= gic_spi(44),
+		.start	= gic_iid(0x4c),
 		.flags	= IORESOURCE_IRQ,
 	},
 };
@@ -238,7 +238,7 @@ static struct resource ehci1_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start	= gic_spi(45),
+		.start	= gic_iid(0x4d),
 		.flags	= IORESOURCE_IRQ,
 	},
 };
@@ -268,7 +268,7 @@ static struct resource ohci0_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start	= gic_spi(44),
+		.start	= gic_iid(0x4c),
 		.flags	= IORESOURCE_IRQ,
 	},
 };
@@ -292,7 +292,7 @@ static struct resource ohci1_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
-		.start	= gic_spi(45),
+		.start	= gic_iid(0x4d),
 		.flags	= IORESOURCE_IRQ,
 	},
 };
diff --git a/arch/arm/mach-shmobile/include/mach/irqs.h b/arch/arm/mach-shmobile/include/mach/irqs.h
index 06a5da3..83f1c84 100644
--- a/arch/arm/mach-shmobile/include/mach/irqs.h
+++ b/arch/arm/mach-shmobile/include/mach/irqs.h
@@ -5,6 +5,7 @@
 
 /* GIC */
 #define gic_spi(nr)		((nr) + 32)
+#define gic_iid(nr)		(nr)
 
 /* INTCS */
 #define INTCS_VECT_BASE		0x3400
-- 
1.7.9.5

Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Magnus Damm]

Hi Morimoto-san,

On Tue, Feb 19, 2013 at 5:55 PM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> R-Car H1 datasheet GIC number is indicating
> GIC ICCIAR / interrupt ID number, not SPI number,
> but current marzen board code is using gic_spi() with
> un-understandable calculation.
>
> This patch adds new gic_iid() macro which means
> ICCIAR / interrupt ID, and used the number
> currently written on datasheet.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
>
>  - gic_iic() -> gic_iid()
>  - update git log comments
>
>  arch/arm/mach-shmobile/board-marzen.c      |   12 ++++++------
>  arch/arm/mach-shmobile/include/mach/irqs.h |    1 +
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
> index cdcb799..2338d4b 100644
> --- a/arch/arm/mach-shmobile/board-marzen.c
> +++ b/arch/arm/mach-shmobile/board-marzen.c
> @@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
>                 .flags          = IORESOURCE_MEM,
>         },
>         [1] = {
> -               .start          = gic_spi(28), /* IRQ 1 */
> +               .start          = gic_iid(0x3c), /* IRQ 1 */
>                 .flags          = IORESOURCE_IRQ,
>         },
>  };

In general we're moving over to DT so for this case I can't say that I
care very much. However, if you want to change things around why don't
you just do the following?

> +               .start          = 0x3c, /* IRQ 1 */

Thanks,

/ magnus

Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Kuninori Morimoto]

Hi Magnus

Thank you for your reply

> > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
> > index cdcb799..2338d4b 100644
> > --- a/arch/arm/mach-shmobile/board-marzen.c
> > +++ b/arch/arm/mach-shmobile/board-marzen.c
> > @@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
> >                 .flags          = IORESOURCE_MEM,
> >         },
> >         [1] = {
> > -               .start          = gic_spi(28), /* IRQ 1 */
> > +               .start          = gic_iid(0x3c), /* IRQ 1 */
> >                 .flags          = IORESOURCE_IRQ,
> >         },
> >  };
> 
> In general we're moving over to DT so for this case I can't say that I
> care very much. However, if you want to change things around why don't
> you just do the following?
> 
> > +               .start          = 0x3c, /* IRQ 1 */

Actually, I considered this "direct value" style,
but I thought that it seems  un-understandable.
This gic_iid(xxx) shows some kind of "GIC" value.

Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Paul Mundt]

On Tue, Feb 19, 2013 at 04:14:31PM -0800, Kuninori Morimoto wrote:
> 
> Hi Magnus
> 
> Thank you for your reply
> 
> > > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
> > > index cdcb799..2338d4b 100644
> > > --- a/arch/arm/mach-shmobile/board-marzen.c
> > > +++ b/arch/arm/mach-shmobile/board-marzen.c
> > > @@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
> > >                 .flags          = IORESOURCE_MEM,
> > >         },
> > >         [1] = {
> > > -               .start          = gic_spi(28), /* IRQ 1 */
> > > +               .start          = gic_iid(0x3c), /* IRQ 1 */
> > >                 .flags          = IORESOURCE_IRQ,
> > >         },
> > >  };
> > 
> > In general we're moving over to DT so for this case I can't say that I
> > care very much. However, if you want to change things around why don't
> > you just do the following?
> > 
> > > +               .start          = 0x3c, /* IRQ 1 */
> 
> Actually, I considered this "direct value" style,
> but I thought that it seems  un-understandable.
> This gic_iid(xxx) shows some kind of "GIC" value.
> 
Agreed. The direct mapping works fine if its understood that the value
being assigned is a direct IRQ number. In the case that it's some other
value that's being adapted, it should go through a canonicalization
wrapper to make the use obvious. Whether there is a 1:1 correlation or
not doesn't particularly matter, as it's still adopting a non-IRQ
resource as an IRQ value.

Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Simon Horman]

On Wed, Feb 20, 2013 at 08:41:47PM +0900, Paul Mundt wrote:
> On Tue, Feb 19, 2013 at 04:14:31PM -0800, Kuninori Morimoto wrote:
> > 
> > Hi Magnus
> > 
> > Thank you for your reply
> > 
> > > > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
> > > > index cdcb799..2338d4b 100644
> > > > --- a/arch/arm/mach-shmobile/board-marzen.c
> > > > +++ b/arch/arm/mach-shmobile/board-marzen.c
> > > > @@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
> > > >                 .flags          = IORESOURCE_MEM,
> > > >         },
> > > >         [1] = {
> > > > -               .start          = gic_spi(28), /* IRQ 1 */
> > > > +               .start          = gic_iid(0x3c), /* IRQ 1 */
> > > >                 .flags          = IORESOURCE_IRQ,
> > > >         },
> > > >  };
> > > 
> > > In general we're moving over to DT so for this case I can't say that I
> > > care very much. However, if you want to change things around why don't
> > > you just do the following?
> > > 
> > > > +               .start          = 0x3c, /* IRQ 1 */
> > 
> > Actually, I considered this "direct value" style,
> > but I thought that it seems  un-understandable.
> > This gic_iid(xxx) shows some kind of "GIC" value.
> > 
> Agreed. The direct mapping works fine if its understood that the value
> being assigned is a direct IRQ number. In the case that it's some other
> value that's being adapted, it should go through a canonicalization
> wrapper to make the use obvious. Whether there is a 1:1 correlation or
> not doesn't particularly matter, as it's still adopting a non-IRQ
> resource as an IRQ value.

Thanks Paul.

Does anyone have any outstanding objections to v2 of this patch?

Morimoto-san, is this just an RFC or would you like it applied?

Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Kuninori Morimoto]

Hi Simon

> On Wed, Feb 20, 2013 at 08:41:47PM +0900, Paul Mundt wrote:
> > On Tue, Feb 19, 2013 at 04:14:31PM -0800, Kuninori Morimoto wrote:
> > > 
> > > Hi Magnus
> > > 
> > > Thank you for your reply
> > > 
> > > > > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
> > > > > index cdcb799..2338d4b 100644
> > > > > --- a/arch/arm/mach-shmobile/board-marzen.c
> > > > > +++ b/arch/arm/mach-shmobile/board-marzen.c
> > > > > @@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
> > > > >                 .flags          = IORESOURCE_MEM,
> > > > >         },
> > > > >         [1] = {
> > > > > -               .start          = gic_spi(28), /* IRQ 1 */
> > > > > +               .start          = gic_iid(0x3c), /* IRQ 1 */
> > > > >                 .flags          = IORESOURCE_IRQ,
> > > > >         },
> > > > >  };
> > > > 
> > > > In general we're moving over to DT so for this case I can't say that I
> > > > care very much. However, if you want to change things around why don't
> > > > you just do the following?
> > > > 
> > > > > +               .start          = 0x3c, /* IRQ 1 */
> > > 
> > > Actually, I considered this "direct value" style,
> > > but I thought that it seems  un-understandable.
> > > This gic_iid(xxx) shows some kind of "GIC" value.
> > > 
> > Agreed. The direct mapping works fine if its understood that the value
> > being assigned is a direct IRQ number. In the case that it's some other
> > value that's being adapted, it should go through a canonicalization
> > wrapper to make the use obvious. Whether there is a 1:1 correlation or
> > not doesn't particularly matter, as it's still adopting a non-IRQ
> > resource as an IRQ value.
> 
> Thanks Paul.
> 
> Does anyone have any outstanding objections to v2 of this patch?
> 
> Morimoto-san, is this just an RFC or would you like it applied?

Thank you.

I noticed one thing.
Do you think is it better if gic_iid() macro has explain comment ?
I can send v3 patch

#define gic_iid(nr)		(nr) /* ICCIAR / interrupt ID */


Best regards
---
Kuninori Morimoto

Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID

[Simon Horman]

On Sun, Feb 24, 2013 at 04:05:43PM -0800, Kuninori Morimoto wrote:
> 
> Hi Simon
> 
> > On Wed, Feb 20, 2013 at 08:41:47PM +0900, Paul Mundt wrote:
> > > On Tue, Feb 19, 2013 at 04:14:31PM -0800, Kuninori Morimoto wrote:
> > > > 
> > > > Hi Magnus
> > > > 
> > > > Thank you for your reply
> > > > 
> > > > > > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c
> > > > > > index cdcb799..2338d4b 100644
> > > > > > --- a/arch/arm/mach-shmobile/board-marzen.c
> > > > > > +++ b/arch/arm/mach-shmobile/board-marzen.c
> > > > > > @@ -66,7 +66,7 @@ static struct resource smsc911x_resources[] = {
> > > > > >                 .flags          = IORESOURCE_MEM,
> > > > > >         },
> > > > > >         [1] = {
> > > > > > -               .start          = gic_spi(28), /* IRQ 1 */
> > > > > > +               .start          = gic_iid(0x3c), /* IRQ 1 */
> > > > > >                 .flags          = IORESOURCE_IRQ,
> > > > > >         },
> > > > > >  };
> > > > > 
> > > > > In general we're moving over to DT so for this case I can't say that I
> > > > > care very much. However, if you want to change things around why don't
> > > > > you just do the following?
> > > > > 
> > > > > > +               .start          = 0x3c, /* IRQ 1 */
> > > > 
> > > > Actually, I considered this "direct value" style,
> > > > but I thought that it seems  un-understandable.
> > > > This gic_iid(xxx) shows some kind of "GIC" value.
> > > > 
> > > Agreed. The direct mapping works fine if its understood that the value
> > > being assigned is a direct IRQ number. In the case that it's some other
> > > value that's being adapted, it should go through a canonicalization
> > > wrapper to make the use obvious. Whether there is a 1:1 correlation or
> > > not doesn't particularly matter, as it's still adopting a non-IRQ
> > > resource as an IRQ value.
> > 
> > Thanks Paul.
> > 
> > Does anyone have any outstanding objections to v2 of this patch?
> > 
> > Morimoto-san, is this just an RFC or would you like it applied?
> 
> Thank you.
> 
> I noticed one thing.
> Do you think is it better if gic_iid() macro has explain comment ?

Yes, I think it would be better. Perhaps a brief explanation that it
is an identity mapping.

> I can send v3 patch
> 
> #define gic_iid(nr)		(nr) /* ICCIAR / interrupt ID */
> 
> 
> Best regards
> ---
> Kuninori Morimoto
>