* Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID
2013-02-19 8:55 [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID Kuninori Morimoto
@ 2013-02-19 10:15 ` Magnus Damm
2013-02-20 0:14 ` Kuninori Morimoto
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Magnus Damm @ 2013-02-19 10:15 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID
2013-02-19 8:55 [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID Kuninori Morimoto
2013-02-19 10:15 ` Magnus Damm
@ 2013-02-20 0:14 ` Kuninori Morimoto
2013-02-20 11:41 ` Paul Mundt
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Kuninori Morimoto @ 2013-02-20 0:14 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID
2013-02-19 8:55 [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID Kuninori Morimoto
2013-02-19 10:15 ` Magnus Damm
2013-02-20 0:14 ` Kuninori Morimoto
@ 2013-02-20 11:41 ` Paul Mundt
2013-02-22 20:40 ` Simon Horman
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Paul Mundt @ 2013-02-20 11:41 UTC (permalink / raw)
To: linux-sh
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID
2013-02-19 8:55 [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID Kuninori Morimoto
` (2 preceding siblings ...)
2013-02-20 11:41 ` Paul Mundt
@ 2013-02-22 20:40 ` Simon Horman
2013-02-25 0:05 ` Kuninori Morimoto
2013-02-25 4:41 ` Simon Horman
5 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2013-02-22 20:40 UTC (permalink / raw)
To: linux-sh
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?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID
2013-02-19 8:55 [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID Kuninori Morimoto
` (3 preceding siblings ...)
2013-02-22 20:40 ` Simon Horman
@ 2013-02-25 0:05 ` Kuninori Morimoto
2013-02-25 4:41 ` Simon Horman
5 siblings, 0 replies; 7+ messages in thread
From: Kuninori Morimoto @ 2013-02-25 0:05 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID
2013-02-19 8:55 [RFC][PATCH v2] ARM: shmobile: R-Car: add gic_iid macro for ICCIAR / interrupt ID Kuninori Morimoto
` (4 preceding siblings ...)
2013-02-25 0:05 ` Kuninori Morimoto
@ 2013-02-25 4:41 ` Simon Horman
5 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2013-02-25 4:41 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread