[PATCH] ARM: gic: Document Power and Clock Domain optional properties

[PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Geert Uytterhoeven]

On some SoCs, the GIC may be part of a PM Domain (hardware Power and/or
Clock Domain).  Document the related optional DT properties.

Note: As the current GIC driver doesn't support Runtime PM yet, PM
Domain constraints must be handled elsewhere in e.g. platform code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
To preserve DT stability, we would like to add these properties to the
affected shmobile dtsi files.

On Thu, Mar 26, 2015 at 11:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 25/03/15 21:19, Geert Uytterhoeven wrote:
>> I would like to add the clock and GIC dependency on the clock in the DTS now,
>> for reasons of DTS stability. But that means I need a temporary workaround
>> to avoid the clock from being disabled, until the GIC driver handles this.
>>
>> I don't expect a fix for the GIC code to just show up magically. I just wanted
>> you to be aware of the problem. GIC is not the only problematic module here,
>> there are others, cfr. the last slide of [2].
>
> As long as there is an agreement from the DT people on the presence of
> that extra property in the GIC node, I'm happy with that. I'd like it to
> be documented though.

Full thread at
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331876.html

 Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 2da059a4790cb3c6..b21113b35f085f27 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -58,6 +58,14 @@ Optional
   regions, used when the GIC doesn't have banked registers. The offset is
   cpu-offset * cpu-nr.
 
+- power-domains : A phandle and PM domain specifier as defined by bindings of
+		  the power controller specified by phandle, used when the GIC
+		  is part of a Power or Clock Domain.
+
+- clocks        : A phandle and clock specifier as defined by bindings of
+		  the clock controller specified by phandle, used when the GIC
+		  is part of a Clock Domain.
+
 Example:
 
 	intc: interrupt-controller@fff11000 {
-- 
1.9.1

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Rob Herring]

On Mon, Apr 27, 2015 at 10:00 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> On some SoCs, the GIC may be part of a PM Domain (hardware Power and/or
> Clock Domain).  Document the related optional DT properties.
>
> Note: As the current GIC driver doesn't support Runtime PM yet, PM
> Domain constraints must be handled elsewhere in e.g. platform code.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Rob Herring <robh@kernel.org>

One comment below.

> ---
> To preserve DT stability, we would like to add these properties to the
> affected shmobile dtsi files.
>
> On Thu, Mar 26, 2015 at 11:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 25/03/15 21:19, Geert Uytterhoeven wrote:
>>> I would like to add the clock and GIC dependency on the clock in the DTS now,
>>> for reasons of DTS stability. But that means I need a temporary workaround
>>> to avoid the clock from being disabled, until the GIC driver handles this.
>>>
>>> I don't expect a fix for the GIC code to just show up magically. I just wanted
>>> you to be aware of the problem. GIC is not the only problematic module here,
>>> there are others, cfr. the last slide of [2].
>>
>> As long as there is an agreement from the DT people on the presence of
>> that extra property in the GIC node, I'm happy with that. I'd like it to
>> be documented though.
>
> Full thread at
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331876.html
>
>  Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 2da059a4790cb3c6..b21113b35f085f27 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -58,6 +58,14 @@ Optional
>    regions, used when the GIC doesn't have banked registers. The offset is
>    cpu-offset * cpu-nr.
>
> +- power-domains : A phandle and PM domain specifier as defined by bindings of
> +                 the power controller specified by phandle, used when the GIC
> +                 is part of a Power or Clock Domain.

"or Clock" should be removed?

> +
> +- clocks        : A phandle and clock specifier as defined by bindings of
> +                 the clock controller specified by phandle, used when the GIC
> +                 is part of a Clock Domain.
> +
>  Example:
>
>         intc: interrupt-controller@fff11000 {
> --
> 1.9.1
>

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Mark Rutland]

On Mon, Apr 27, 2015 at 04:00:11PM +0100, Geert Uytterhoeven wrote:
> On some SoCs, the GIC may be part of a PM Domain (hardware Power and/or
> Clock Domain).  Document the related optional DT properties.
> 
> Note: As the current GIC driver doesn't support Runtime PM yet, PM
> Domain constraints must be handled elsewhere in e.g. platform code.

I'm worried that without a current user these properties aren't going to
see any testing...

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> To preserve DT stability, we would like to add these properties to the
> affected shmobile dtsi files.

... which means that they could be wrong, and will get in the way of
stability rather than aiding it.

I'm also concerned that the carving up of clock inputs, power domains,
and other physical details is implementation-specific. I imagine that
pretty much every user that will care about this is using GIC-400, so
could we make this specific to GIC-400?

> On Thu, Mar 26, 2015 at 11:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 25/03/15 21:19, Geert Uytterhoeven wrote:
> >> I would like to add the clock and GIC dependency on the clock in the DTS now,
> >> for reasons of DTS stability. But that means I need a temporary workaround
> >> to avoid the clock from being disabled, until the GIC driver handles this.
> >>
> >> I don't expect a fix for the GIC code to just show up magically. I just wanted
> >> you to be aware of the problem. GIC is not the only problematic module here,
> >> there are others, cfr. the last slide of [2].
> >
> > As long as there is an agreement from the DT people on the presence of
> > that extra property in the GIC node, I'm happy with that. I'd like it to
> > be documented though.
> 
> Full thread at
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331876.html
> 
>  Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 2da059a4790cb3c6..b21113b35f085f27 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -58,6 +58,14 @@ Optional
>    regions, used when the GIC doesn't have banked registers. The offset is
>    cpu-offset * cpu-nr.
>  
> +- power-domains : A phandle and PM domain specifier as defined by bindings of
> +		  the power controller specified by phandle, used when the GIC
> +		  is part of a Power or Clock Domain.

I imagine that it's possible for the distributor and CPU interfaces to
be in separate power domains in some implementaitons.

Which components of the GIC does the apply to? The whole thing? Just the
GICD?

> +- clocks        : A phandle and clock specifier as defined by bindings of
> +		  the clock controller specified by phandle, used when the GIC
> +		  is part of a Clock Domain.

Depending on implementation, a GIC could require multiple clocks, and
their names would be implementation-specific (that said, GIC-400 has a
single "CLK" input).

Assuming that you're using GIC-400, could we use clock-names to make
that explicit?

Thanks,
Mark.

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Geert Uytterhoeven]

On Mon, Apr 27, 2015 at 5:25 PM, Rob Herring <robherring2@gmail.com> wrote:
>> --- a/Documentation/devicetree/bindings/arm/gic.txt
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -58,6 +58,14 @@ Optional
>>    regions, used when the GIC doesn't have banked registers. The offset is
>>    cpu-offset * cpu-nr.
>>
>> +- power-domains : A phandle and PM domain specifier as defined by bindings of
>> +                 the power controller specified by phandle, used when the GIC
>> +                 is part of a Power or Clock Domain.
>
> "or Clock" should be removed?

No, both power domains and clock domains use the "power-domains"
property.

In case of clock domains, "power-domains" refers to the power controller,
which controls the clocks.
"clocks" is used for specifying the clock to control.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Geert Uytterhoeven]

Hi Mark,

On Mon, Apr 27, 2015 at 5:54 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Apr 27, 2015 at 04:00:11PM +0100, Geert Uytterhoeven wrote:
>> On some SoCs, the GIC may be part of a PM Domain (hardware Power and/or
>> Clock Domain).  Document the related optional DT properties.
>>
>> Note: As the current GIC driver doesn't support Runtime PM yet, PM
>> Domain constraints must be handled elsewhere in e.g. platform code.
>
> I'm worried that without a current user these properties aren't going to
> see any testing...

Still, DT is supposed to describe the hardware. Even if the driver doesn't
use the description.

>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> To preserve DT stability, we would like to add these properties to the
>> affected shmobile dtsi files.
>
> ... which means that they could be wrong, and will get in the way of
> stability rather than aiding it.

We do know the GIC is part of the power domain, and has a controllable
clock (on the affected SoCs).

> I'm also concerned that the carving up of clock inputs, power domains,
> and other physical details is implementation-specific. I imagine that
> pretty much every user that will care about this is using GIC-400, so
> could we make this specific to GIC-400?

I have no idea which GIC version is being used.

This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
According to the DTS, they're compatible with "arm,cortex-a15-gic" and
"arm,cortex-a7-gic", and work with that value.

>> On Thu, Mar 26, 2015 at 11:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> > On 25/03/15 21:19, Geert Uytterhoeven wrote:
>> >> I would like to add the clock and GIC dependency on the clock in the DTS now,
>> >> for reasons of DTS stability. But that means I need a temporary workaround
>> >> to avoid the clock from being disabled, until the GIC driver handles this.
>> >>
>> >> I don't expect a fix for the GIC code to just show up magically. I just wanted
>> >> you to be aware of the problem. GIC is not the only problematic module here,
>> >> there are others, cfr. the last slide of [2].
>> >
>> > As long as there is an agreement from the DT people on the presence of
>> > that extra property in the GIC node, I'm happy with that. I'd like it to
>> > be documented though.
>>
>> Full thread at
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331876.html
>>
>>  Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> index 2da059a4790cb3c6..b21113b35f085f27 100644
>> --- a/Documentation/devicetree/bindings/arm/gic.txt
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -58,6 +58,14 @@ Optional
>>    regions, used when the GIC doesn't have banked registers. The offset is
>>    cpu-offset * cpu-nr.
>>
>> +- power-domains : A phandle and PM domain specifier as defined by bindings of
>> +               the power controller specified by phandle, used when the GIC
>> +               is part of a Power or Clock Domain.
>
> I imagine that it's possible for the distributor and CPU interfaces to
> be in separate power domains in some implementaitons.

Possibly (then we're gonna need subnodes, each with their own
power-domains properties?).

> Which components of the GIC does the apply to? The whole thing? Just the
> GICD?

Unfortunately that's not documented...

>> +- clocks        : A phandle and clock specifier as defined by bindings of
>> +               the clock controller specified by phandle, used when the GIC
>> +               is part of a Clock Domain.
>
> Depending on implementation, a GIC could require multiple clocks, and
> their names would be implementation-specific (that said, GIC-400 has a
> single "CLK" input).

Documentation/devicetree/bindings/arm/gic.txt doesn't mention any clocks
for GIC-400?

> Assuming that you're using GIC-400, could we use clock-names to make
> that explicit?

You can. But you have to be careful to avoid conflicts, as the controller
for the clock domain needs to know which clock to use.

On Renesas SoCs, we always used the first clock for power control.
For the future, we plan to use "fck" if it exists.

Yes, this can be complicated, as ideally it needs synchronization between
device DT bindings and platform (clock/power domain) DT bindings.

Please also note that any device (hardware IP core) can be reused on an
SoC that supports clock and/or power domains, and suddenly starts to rely
on power-domains and clocks properties.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Mark Rutland]

On Mon, Apr 27, 2015 at 05:43:13PM +0100, Geert Uytterhoeven wrote:
> Hi Mark,
> 
> On Mon, Apr 27, 2015 at 5:54 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Apr 27, 2015 at 04:00:11PM +0100, Geert Uytterhoeven wrote:
> >> On some SoCs, the GIC may be part of a PM Domain (hardware Power and/or
> >> Clock Domain).  Document the related optional DT properties.
> >>
> >> Note: As the current GIC driver doesn't support Runtime PM yet, PM
> >> Domain constraints must be handled elsewhere in e.g. platform code.
> >
> > I'm worried that without a current user these properties aren't going to
> > see any testing...
> 
> Still, DT is supposed to describe the hardware. Even if the driver doesn't
> use the description.

I appreciate that...

> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> To preserve DT stability, we would like to add these properties to the
> >> affected shmobile dtsi files.
> >
> > ... which means that they could be wrong, and will get in the way of
> > stability rather than aiding it.
> 
> We do know the GIC is part of the power domain, and has a controllable
> clock (on the affected SoCs).

... my concern is that the data we place into the DT will be untested
given that we don't have software relying on it. If said data is not
correct, it is harmful to have, especially for such fundamental
properties.

> > I'm also concerned that the carving up of clock inputs, power domains,
> > and other physical details is implementation-specific. I imagine that
> > pretty much every user that will care about this is using GIC-400, so
> > could we make this specific to GIC-400?
> 
> I have no idea which GIC version is being used.

This is unfortunate.

> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
> According to the DTS, they're compatible with "arm,cortex-a15-gic" and
> "arm,cortex-a7-gic", and work with that value.

Who put the DT together in the first place?

If it's a multi-cluster SoC then we know that we're not using any
built-in distributor...

> >> On Thu, Mar 26, 2015 at 11:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> > On 25/03/15 21:19, Geert Uytterhoeven wrote:
> >> >> I would like to add the clock and GIC dependency on the clock in the DTS now,
> >> >> for reasons of DTS stability. But that means I need a temporary workaround
> >> >> to avoid the clock from being disabled, until the GIC driver handles this.
> >> >>
> >> >> I don't expect a fix for the GIC code to just show up magically. I just wanted
> >> >> you to be aware of the problem. GIC is not the only problematic module here,
> >> >> there are others, cfr. the last slide of [2].
> >> >
> >> > As long as there is an agreement from the DT people on the presence of
> >> > that extra property in the GIC node, I'm happy with that. I'd like it to
> >> > be documented though.
> >>
> >> Full thread at
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331876.html
> >>
> >>  Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> >> index 2da059a4790cb3c6..b21113b35f085f27 100644
> >> --- a/Documentation/devicetree/bindings/arm/gic.txt
> >> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> >> @@ -58,6 +58,14 @@ Optional
> >>    regions, used when the GIC doesn't have banked registers. The offset is
> >>    cpu-offset * cpu-nr.
> >>
> >> +- power-domains : A phandle and PM domain specifier as defined by bindings of
> >> +               the power controller specified by phandle, used when the GIC
> >> +               is part of a Power or Clock Domain.
> >
> > I imagine that it's possible for the distributor and CPU interfaces to
> > be in separate power domains in some implementaitons.
> 
> Possibly (then we're gonna need subnodes, each with their own
> power-domains properties?).

Potentially for such implementations, yes. To the best of my knowledge a
GIC-400 is a single block, which is one reason why it would be nice to
focus on that particular implementation if that's applicable.

> > Which components of the GIC does the apply to? The whole thing? Just the
> > GICD?
> 
> Unfortunately that's not documented...

That worries me somewhat, given my comments above, though I'd very much
suspect this is a GIC-400.

> >> +- clocks        : A phandle and clock specifier as defined by bindings of
> >> +               the clock controller specified by phandle, used when the GIC
> >> +               is part of a Clock Domain.
> >
> > Depending on implementation, a GIC could require multiple clocks, and
> > their names would be implementation-specific (that said, GIC-400 has a
> > single "CLK" input).
> 
> Documentation/devicetree/bindings/arm/gic.txt doesn't mention any clocks
> for GIC-400?

I'm going by the TRM. There are other items not in the bindign that are
implementation-specific.

> > Assuming that you're using GIC-400, could we use clock-names to make
> > that explicit?
> 
> You can. But you have to be careful to avoid conflicts, as the controller
> for the clock domain needs to know which clock to use.

Could you elborate on that? I don't see why a node's clock-names
property should be relevant to the clock controller.

> On Renesas SoCs, we always used the first clock for power control.
> For the future, we plan to use "fck" if it exists.

That's irrelevant from the PoV of the GIC; per the GIC architecture you
can't assume anything about the set of input clocks, and GIC-400 happens
to have a single clock input called "CLK". It's not IP specific to
Renesas.

> Yes, this can be complicated, as ideally it needs synchronization between
> device DT bindings and platform (clock/power domain) DT bindings.
> 
> Please also note that any device (hardware IP core) can be reused on an
> SoC that supports clock and/or power domains, and suddenly starts to rely
> on power-domains and clocks properties.

Sorry, I don't follow this last part.

Mark.

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Geert Uytterhoeven]

Hi Mark,

On Mon, Apr 27, 2015 at 7:15 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Apr 27, 2015 at 05:43:13PM +0100, Geert Uytterhoeven wrote:
>> On Mon, Apr 27, 2015 at 5:54 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Apr 27, 2015 at 04:00:11PM +0100, Geert Uytterhoeven wrote:
>> >> On some SoCs, the GIC may be part of a PM Domain (hardware Power and/or
>> >> Clock Domain).  Document the related optional DT properties.
>> >>
>> >> Note: As the current GIC driver doesn't support Runtime PM yet, PM
>> >> Domain constraints must be handled elsewhere in e.g. platform code.
>> >
>> > I'm worried that without a current user these properties aren't going to
>> > see any testing...
>>
>> Still, DT is supposed to describe the hardware. Even if the driver doesn't
>> use the description.
>
> I appreciate that...
>
>> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> >> ---
>> >> To preserve DT stability, we would like to add these properties to the
>> >> affected shmobile dtsi files.
>> >
>> > ... which means that they could be wrong, and will get in the way of
>> > stability rather than aiding it.
>>
>> We do know the GIC is part of the power domain, and has a controllable
>> clock (on the affected SoCs).
>
> ... my concern is that the data we place into the DT will be untested
> given that we don't have software relying on it. If said data is not
> correct, it is harmful to have, especially for such fundamental
> properties.

Your statement challenges the viability of Stable DT Requirements, as we
can thus never write a DTS until the full software implementation has been
completed ;-)

We do know switching off that clock disables the GIC, as per documentation
(and experimentation: lock up), and have used the same strategy with
other Renesas interrupt controllers (renesas,intc-irqpin and renesas,irqc).

>> > I'm also concerned that the carving up of clock inputs, power domains,
>> > and other physical details is implementation-specific. I imagine that
>> > pretty much every user that will care about this is using GIC-400, so
>> > could we make this specific to GIC-400?
>>
>> I have no idea which GIC version is being used.
>
> This is unfortunate.
>
>> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
>> According to the DTS, they're compatible with "arm,cortex-a15-gic" and
>> "arm,cortex-a7-gic", and work with that value.
>
> Who put the DT together in the first place?

Magnus (added to CC).

> If it's a multi-cluster SoC then we know that we're not using any
> built-in distributor...

R-Mobile APE6 has 2 clusters (4xCA15 and 4xCA7).
R-Car Gen2 has 1 or 2 clusters (2/4xCA15 and/or 2/4xCA7).

>> >> On Thu, Mar 26, 2015 at 11:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> >> > On 25/03/15 21:19, Geert Uytterhoeven wrote:
>> >> >> I would like to add the clock and GIC dependency on the clock in the DTS now,
>> >> >> for reasons of DTS stability. But that means I need a temporary workaround
>> >> >> to avoid the clock from being disabled, until the GIC driver handles this.
>> >> >>
>> >> >> I don't expect a fix for the GIC code to just show up magically. I just wanted
>> >> >> you to be aware of the problem. GIC is not the only problematic module here,
>> >> >> there are others, cfr. the last slide of [2].
>> >> >
>> >> > As long as there is an agreement from the DT people on the presence of
>> >> > that extra property in the GIC node, I'm happy with that. I'd like it to
>> >> > be documented though.
>> >>
>> >> Full thread at
>> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331876.html
>> >>
>> >>  Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++++
>> >>  1 file changed, 8 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> >> index 2da059a4790cb3c6..b21113b35f085f27 100644
>> >> --- a/Documentation/devicetree/bindings/arm/gic.txt
>> >> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> >> @@ -58,6 +58,14 @@ Optional
>> >>    regions, used when the GIC doesn't have banked registers. The offset is
>> >>    cpu-offset * cpu-nr.
>> >>
>> >> +- power-domains : A phandle and PM domain specifier as defined by bindings of
>> >> +               the power controller specified by phandle, used when the GIC
>> >> +               is part of a Power or Clock Domain.
>> >
>> > I imagine that it's possible for the distributor and CPU interfaces to
>> > be in separate power domains in some implementaitons.
>>
>> Possibly (then we're gonna need subnodes, each with their own
>> power-domains properties?).
>
> Potentially for such implementations, yes. To the best of my knowledge a
> GIC-400 is a single block, which is one reason why it would be nice to
> focus on that particular implementation if that's applicable.
>
>> > Which components of the GIC does the apply to? The whole thing? Just the
>> > GICD?
>>
>> Unfortunately that's not documented...
>
> That worries me somewhat, given my comments above, though I'd very much
> suspect this is a GIC-400.
>
>> >> +- clocks        : A phandle and clock specifier as defined by bindings of
>> >> +               the clock controller specified by phandle, used when the GIC
>> >> +               is part of a Clock Domain.
>> >
>> > Depending on implementation, a GIC could require multiple clocks, and
>> > their names would be implementation-specific (that said, GIC-400 has a
>> > single "CLK" input).
>>
>> Documentation/devicetree/bindings/arm/gic.txt doesn't mention any clocks
>> for GIC-400?
>
> I'm going by the TRM. There are other items not in the bindign that are
> implementation-specific.
>
>> > Assuming that you're using GIC-400, could we use clock-names to make
>> > that explicit?
>>
>> You can. But you have to be careful to avoid conflicts, as the controller
>> for the clock domain needs to know which clock to use.
>
> Could you elborate on that? I don't see why a node's clock-names
> property should be relevant to the clock controller.

If there are multiple clocks listed, the clock controller needs to know which
clocks is to be used for power management.

If we standardize on a name for the clock to be used for power management
(a platform property), that could conflict with the naming in the binding
for the device.
Some device bindings don't mandate clock-names as only a single clock
is used.
Other device bindings do mandate clock-names. You mentioned "CLK" for
GIC-400, others use e.g. "fck", or "ick".

>> On Renesas SoCs, we always used the first clock for power control.
>> For the future, we plan to use "fck" if it exists.
>
> That's irrelevant from the PoV of the GIC; per the GIC architecture you
> can't assume anything about the set of input clocks, and GIC-400 happens
> to have a single clock input called "CLK". It's not IP specific to
> Renesas.
>
>> Yes, this can be complicated, as ideally it needs synchronization between
>> device DT bindings and platform (clock/power domain) DT bindings.
>>
>> Please also note that any device (hardware IP core) can be reused on an
>> SoC that supports clock and/or power domains, and suddenly starts to rely
>> on power-domains and clocks properties.
>
> Sorry, I don't follow this last part.

Devices may describe zero, one, or more clocks in the DT bindings. Usually
these clocks are functional, and they are described because the driver needs
to control them, and/or needs knowledge about them.

As all (current) logic is synchronous, there's always a clock that drives the
logic of the device. Gating this clock can be used for power management.
This clock may or may not be the one described in the bindings (obviously
it isn't if the binding doesn't describe any clock, but there can be other
cases).

Any IP core for a device may be reused. On some SoCs, the logic may be
driven by a fixed, non-gateable clock. On others the logic may be driven by
a gateable clock to save power. Whether or not there is a gateable clock
is a property of the platform (SoC), not of the IP core.

Please see slides 37-40 of my presentation "Last one out, turn off the lights"
at ELC2015:
http://events.linuxfoundation.org/sites/events/files/slides/elc2015_uytterhoeven.pdf
They show examples of devices with zero, one or more described clocks.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Mark Rutland]

Hi Geert,

> >> >> To preserve DT stability, we would like to add these properties to the
> >> >> affected shmobile dtsi files.
> >> >
> >> > ... which means that they could be wrong, and will get in the way of
> >> > stability rather than aiding it.
> >>
> >> We do know the GIC is part of the power domain, and has a controllable
> >> clock (on the affected SoCs).
> >
> > ... my concern is that the data we place into the DT will be untested
> > given that we don't have software relying on it. If said data is not
> > correct, it is harmful to have, especially for such fundamental
> > properties.
> 
> Your statement challenges the viability of Stable DT Requirements, as we
> can thus never write a DTS until the full software implementation has been
> completed ;-)

I appreciate this is difficult, but I disagree that it's impossible ;)

If you don't want to do clock management currently, don't describe the
clock controller, have some FW/loader pre-program the clocks, and list
fixed-clocks in the DTB. This DTB should continue to work, but a new
kernel alone won't give you fancy clock management. This is what we
expect in terms of stable DTBs.

When you add clock controller support, you need a different DT to
describe the clock controller anyway. You can have it nuke the clock
configuration at boot time as a test that everything you need is
described.

> >> > I'm also concerned that the carving up of clock inputs, power domains,
> >> > and other physical details is implementation-specific. I imagine that
> >> > pretty much every user that will care about this is using GIC-400, so
> >> > could we make this specific to GIC-400?
> >>
> >> I have no idea which GIC version is being used.
> >
> > This is unfortunate.
> >
> >> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
> >> According to the DTS, they're compatible with "arm,cortex-a15-gic" and
> >> "arm,cortex-a7-gic", and work with that value.
> >
> > Who put the DT together in the first place?
> 
> Magnus (added to CC).
> 
> > If it's a multi-cluster SoC then we know that we're not using any
> > built-in distributor...
> 
> R-Mobile APE6 has 2 clusters (4xCA15 and 4xCA7).
> R-Car Gen2 has 1 or 2 clusters (2/4xCA15 and/or 2/4xCA7).

It looks like we should be able to read the GICD_IIDR to figure out what
imlpementation is used. Could you see what GICD_IIDR reports on those
platforms? There's a patch at the end of the email to do so.

[...]

> >> >> +- clocks        : A phandle and clock specifier as defined by bindings of
> >> >> +               the clock controller specified by phandle, used when the GIC
> >> >> +               is part of a Clock Domain.
> >> >
> >> > Depending on implementation, a GIC could require multiple clocks, and
> >> > their names would be implementation-specific (that said, GIC-400 has a
> >> > single "CLK" input).
> >>
> >> Documentation/devicetree/bindings/arm/gic.txt doesn't mention any clocks
> >> for GIC-400?
> >
> > I'm going by the TRM. There are other items not in the bindign that are
> > implementation-specific.
> >
> >> > Assuming that you're using GIC-400, could we use clock-names to make
> >> > that explicit?
> >>
> >> You can. But you have to be careful to avoid conflicts, as the controller
> >> for the clock domain needs to know which clock to use.
> >
> > Could you elborate on that? I don't see why a node's clock-names
> > property should be relevant to the clock controller.
> 
> If there are multiple clocks listed, the clock controller needs to know which
> clocks is to be used for power management.
> 
> If we standardize on a name for the clock to be used for power management
> (a platform property), that could conflict with the naming in the binding
> for the device.
> Some device bindings don't mandate clock-names as only a single clock
> is used.
> Other device bindings do mandate clock-names. You mentioned "CLK" for
> GIC-400, others use e.g. "fck", or "ick".

The clock-names should be device-binding specific. Nothing else should
be there. If you want to do fancy maangement of device clocks, the
device drivers should register with the appropriate frameworks to do so,
and nothing else needs to inspect the clock-names for the device itself.

> >> On Renesas SoCs, we always used the first clock for power control.
> >> For the future, we plan to use "fck" if it exists.
> >
> > That's irrelevant from the PoV of the GIC; per the GIC architecture you
> > can't assume anything about the set of input clocks, and GIC-400 happens
> > to have a single clock input called "CLK". It's not IP specific to
> > Renesas.
> >
> >> Yes, this can be complicated, as ideally it needs synchronization between
> >> device DT bindings and platform (clock/power domain) DT bindings.
> >>
> >> Please also note that any device (hardware IP core) can be reused on an
> >> SoC that supports clock and/or power domains, and suddenly starts to rely
> >> on power-domains and clocks properties.
> >
> > Sorry, I don't follow this last part.
> 
> Devices may describe zero, one, or more clocks in the DT bindings. Usually
> these clocks are functional, and they are described because the driver needs
> to control them, and/or needs knowledge about them.
> 
> As all (current) logic is synchronous, there's always a clock that drives the
> logic of the device. Gating this clock can be used for power management.
> This clock may or may not be the one described in the bindings (obviously
> it isn't if the binding doesn't describe any clock, but there can be other
> cases).
> 
> Any IP core for a device may be reused. On some SoCs, the logic may be
> driven by a fixed, non-gateable clock. On others the logic may be driven by
> a gateable clock to save power. Whether or not there is a gateable clock
> is a property of the platform (SoC), not of the IP core.

I agree that the provider of any clock (and it's feature such as
gateability) is not device specific. 

What I am saying is that the set of input lines on an IP block (to which
clocks may be connected) are a property of that IP block. As mentioned
above, GIC-400 always has a clock input line called "CLK", though this
could be wired to arbitrary clock providers, which might call their
output line arbitrary names (e.g. "GIC_CLK").

If bindings are missing functional clocks, those can be added to the
bindings, and we can warn when they are not present. These clocks are
all specific to the particular IP blocks however, so applying an
SoC-specific naming policy to these does not make sense.

Thanks,
Mark.

---->8----
>From 8bd8a5fe42d25b45515ef3152ff921d2180a8120 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 29 Apr 2015 13:54:03 +0100
Subject: [PATCH] irqchip: gic: report GICD_IIDR

The GICD_IIDR register provides information about the implementer and
revision of the GIC distributor. In some cases this information is
useful for debugging, so log this when probing the GIC.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/irqchip/irq-gic.c       | 4 ++++
 include/linux/irqchip/arm-gic.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7b315e3..02c8bb4 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
 	int gic_irqs, irq_base, i;
+	unsigned long iidr;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
@@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_set_base_accessor(gic, gic_get_common_base);
 	}
 
+	iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR);
+	pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr);
+
 	/*
 	 * Initialize the CPU interface map to all CPUs.
 	 * It will be refined as each CPU probes its ID.
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 36ec4ae..c29df66 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -29,6 +29,7 @@
 
 #define GIC_DIST_CTRL			0x000
 #define GIC_DIST_CTR			0x004
+#define GIC_DIST_IIDR			0x008
 #define GIC_DIST_IGROUP			0x080
 #define GIC_DIST_ENABLE_SET		0x100
 #define GIC_DIST_ENABLE_CLEAR		0x180
-- 
1.9.1

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Geert Uytterhoeven]

Hi Mark,

On Wed, Apr 29, 2015 at 2:57 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> >> To preserve DT stability, we would like to add these properties to the
>> >> >> affected shmobile dtsi files.
>> >> >
>> >> > ... which means that they could be wrong, and will get in the way of
>> >> > stability rather than aiding it.
>> >>
>> >> We do know the GIC is part of the power domain, and has a controllable
>> >> clock (on the affected SoCs).
>> >
>> > ... my concern is that the data we place into the DT will be untested
>> > given that we don't have software relying on it. If said data is not
>> > correct, it is harmful to have, especially for such fundamental
>> > properties.
>>
>> Your statement challenges the viability of Stable DT Requirements, as we
>> can thus never write a DTS until the full software implementation has been
>> completed ;-)
>
> I appreciate this is difficult, but I disagree that it's impossible ;)
>
> If you don't want to do clock management currently, don't describe the
> clock controller, have some FW/loader pre-program the clocks, and list
> fixed-clocks in the DTB. This DTB should continue to work, but a new
> kernel alone won't give you fancy clock management. This is what we
> expect in terms of stable DTBs.
>
> When you add clock controller support, you need a different DT to
> describe the clock controller anyway. You can have it nuke the clock

Currently we have the clock controller in the DTS. It only lacks a few clocks
(like INTC-SYS -> GIC).

> configuration at boot time as a test that everything you need is
> described.

Don't worry, I have early boot code that disables all non-critical clocks.
This already caught many bugs (missing clock handling). It also caused a
bug, as I missed an interrupt storm due to a disabled clock ;-)

>> >> > I'm also concerned that the carving up of clock inputs, power domains,
>> >> > and other physical details is implementation-specific. I imagine that
>> >> > pretty much every user that will care about this is using GIC-400, so
>> >> > could we make this specific to GIC-400?
>> >>
>> >> I have no idea which GIC version is being used.
>> >
>> > This is unfortunate.
>> >
>> >> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
>> >> According to the DTS, they're compatible with "arm,cortex-a15-gic" and
>> >> "arm,cortex-a7-gic", and work with that value.
>> >
>> > Who put the DT together in the first place?
>>
>> Magnus (added to CC).
>>
>> > If it's a multi-cluster SoC then we know that we're not using any
>> > built-in distributor...
>>
>> R-Mobile APE6 has 2 clusters (4xCA15 and 4xCA7).
>> R-Car Gen2 has 1 or 2 clusters (2/4xCA15 and/or 2/4xCA7).
>
> It looks like we should be able to read the GICD_IIDR to figure out what
> imlpementation is used. Could you see what GICD_IIDR reports on those
> platforms? There's a patch at the end of the email to do so.

Thanks!

  - R-Mobile APE6 (r8a73a4):        GICD_IIDR reports 0x0200043b
  - R-Car Gen M2-W (r8a7791):       GICD_IIDR reports 0x0200043b

        ProductID = 0x02 (GIC-400)
        Variant = 0x0
        Revision = 0x0
        Implementer = 0x43b (ARM)

So both are GIC-400 (in the mean time I found a reference to GIC-400 in
the APE6 docs).

I also ran it on a few other SoCs:

  - SH-Mobile AG5 (sh73a0):         GICD_IIDR reports 0x0102043b

        Implementation version = 0x01 (Cortex-A9 MPCore)
        Revision number = 0x020
        Implementer = 0x43b (ARM)

    Which GIC is that?

  - R-Mobile A1 (r8a7740):          GICD_IIDR reports 0x0000043b

        ProductID = 0x00 (GIC-500 or Cortex-A15 MPCore or PL390)
        Variant = 0x0
        Revision = 0x0
        Implementer = 0x43b (ARM)

    Seems like 0x0000043b can mean multiple things. Docs say PL390 r0p0.

> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 7b315e3..02c8bb4 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>         irq_hw_number_t hwirq_base;
>         struct gic_chip_data *gic;
>         int gic_irqs, irq_base, i;
> +       unsigned long iidr;
>
>         BUG_ON(gic_nr >= MAX_GIC_NR);
>
> @@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>                 gic_set_base_accessor(gic, gic_get_common_base);
>         }
>
> +       iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR);
> +       pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr);

No need for the cast.

Perhaps just

        pr_debug("GICD_IIDR reports 0x%08lx\n",
                 readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR));

?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Mark Rutland]

Hi Geert,

> >> >> > I'm also concerned that the carving up of clock inputs, power domains,
> >> >> > and other physical details is implementation-specific. I imagine that
> >> >> > pretty much every user that will care about this is using GIC-400, so
> >> >> > could we make this specific to GIC-400?
> >> >>
> >> >> I have no idea which GIC version is being used.
> >> >
> >> > This is unfortunate.
> >> >
> >> >> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
> >> >> According to the DTS, they're compatible with "arm,cortex-a15-gic" and
> >> >> "arm,cortex-a7-gic", and work with that value.
> >> >
> >> > Who put the DT together in the first place?
> >>
> >> Magnus (added to CC).
> >>
> >> > If it's a multi-cluster SoC then we know that we're not using any
> >> > built-in distributor...
> >>
> >> R-Mobile APE6 has 2 clusters (4xCA15 and 4xCA7).
> >> R-Car Gen2 has 1 or 2 clusters (2/4xCA15 and/or 2/4xCA7).
> >
> > It looks like we should be able to read the GICD_IIDR to figure out what
> > imlpementation is used. Could you see what GICD_IIDR reports on those
> > platforms? There's a patch at the end of the email to do so.
> 
> Thanks!
> 
>   - R-Mobile APE6 (r8a73a4):        GICD_IIDR reports 0x0200043b
>   - R-Car Gen M2-W (r8a7791):       GICD_IIDR reports 0x0200043b
> 
>         ProductID = 0x02 (GIC-400)
>         Variant = 0x0
>         Revision = 0x0
>         Implementer = 0x43b (ARM)
> 
> So both are GIC-400 (in the mean time I found a reference to GIC-400 in
> the APE6 docs).

Ok. We already support the "arm,gic-400" compatible string, so these
could be moved over, and given a CLK clock input.

> I also ran it on a few other SoCs:
> 
>   - SH-Mobile AG5 (sh73a0):         GICD_IIDR reports 0x0102043b
> 
>         Implementation version = 0x01 (Cortex-A9 MPCore)
>         Revision number = 0x020
>         Implementer = 0x43b (ARM)
> 
>     Which GIC is that?

That seems to be a GIC internal to the A9 MPCore, so "arm,cortex-a9-gic"
is fine for this.

It seems to take separate PERIPHCLK and PERIPHCLKEN clock inputs, though
it's not clear to be from the TRM whether PERIPHCLKEN is generated
internally from PERIPHCLK, or if it needs to be generated by an external
clock controller.

>   - R-Mobile A1 (r8a7740):          GICD_IIDR reports 0x0000043b
> 
>         ProductID = 0x00 (GIC-500 or Cortex-A15 MPCore or PL390)
>         Variant = 0x0
>         Revision = 0x0
>         Implementer = 0x43b (ARM)
> 
>     Seems like 0x0000043b can mean multiple things. Docs say PL390 r0p0.

We might want to add an "arm,pl390" compatible string for this. The
PL390 seems to take a single "gclk" clock input.

> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 7b315e3..02c8bb4 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >         irq_hw_number_t hwirq_base;
> >         struct gic_chip_data *gic;
> >         int gic_irqs, irq_base, i;
> > +       unsigned long iidr;
> >
> >         BUG_ON(gic_nr >= MAX_GIC_NR);
> >
> > @@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >                 gic_set_base_accessor(gic, gic_get_common_base);
> >         }
> >
> > +       iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR);
> > +       pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr);
> 
> No need for the cast.

For arm64 there is ;)

> Perhaps just
> 
>         pr_debug("GICD_IIDR reports 0x%08lx\n",
>                  readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR));

I could fold the read in, but with the cast it was either overly long or
weirdly formatted.

Thanks,
Mark.

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Geert Uytterhoeven]

On Wed, Apr 29, 2015 at 5:15 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> > index 7b315e3..02c8bb4 100644
>> > --- a/drivers/irqchip/irq-gic.c
>> > +++ b/drivers/irqchip/irq-gic.c
>> > @@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>> >         irq_hw_number_t hwirq_base;
>> >         struct gic_chip_data *gic;
>> >         int gic_irqs, irq_base, i;
>> > +       unsigned long iidr;
>> >
>> >         BUG_ON(gic_nr >= MAX_GIC_NR);
>> >
>> > @@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>> >                 gic_set_base_accessor(gic, gic_get_common_base);
>> >         }
>> >
>> > +       iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR);
>> > +       pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr);
>>
>> No need for the cast.
>
> For arm64 there is ;)

Why? iidr is already unsigned long.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Mark Rutland]

On Wed, Apr 29, 2015 at 07:12:32PM +0100, Geert Uytterhoeven wrote:
> On Wed, Apr 29, 2015 at 5:15 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> >> > index 7b315e3..02c8bb4 100644
> >> > --- a/drivers/irqchip/irq-gic.c
> >> > +++ b/drivers/irqchip/irq-gic.c
> >> > @@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >> >         irq_hw_number_t hwirq_base;
> >> >         struct gic_chip_data *gic;
> >> >         int gic_irqs, irq_base, i;
> >> > +       unsigned long iidr;
> >> >
> >> >         BUG_ON(gic_nr >= MAX_GIC_NR);
> >> >
> >> > @@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >> >                 gic_set_base_accessor(gic, gic_get_common_base);
> >> >         }
> >> >
> >> > +       iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR);
> >> > +       pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr);
> >>
> >> No need for the cast.
> >
> > For arm64 there is ;)
> 
> Why? iidr is already unsigned long.

Ah. indeed it is! For some reason I thought I'd left it as a u32.

Sorry about that; I'll drop the cast.

Thanks,
Mark.