Date: Sun, 14 Jan 2018 10:48:46 +0000
Message-ID: <86efmsg1gx.wl-marc.zyngier@arm.com>
From: Marc Zyngier <marc.zyngier@arm.com>
To: Derek Basehore <dbasehore@chromium.org>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        rafael.j.wysocki@intel.com, tglx@linutronix.de,
        briannorris@chromium.org
Subject: Re: [PATCH 5/8] DT/arm,gic-v3: add save-suspend-state property
In-Reply-To: <20180112212422.148625-6-dbasehore@chromium.org>
References: <20180112212422.148625-1-dbasehore@chromium.org>
        <20180112212422.148625-6-dbasehore@chromium.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Organization: ARM Ltd
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org

On Fri, 12 Jan 2018 21:24:19 +0000,
Derek Basehore wrote:
> 
> This adds documentation for the new save-suspend-state property. This
> property enables saving and restoring the GIC for when it loses state
> in system suspend.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>

Please CC the DT maintainers for all DT binding changes.

> ---
>  .../bindings/interrupt-controller/arm,gic-v3.txt   | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> index 0a57f2f4167d..820556a8ffd2 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> @@ -57,6 +57,10 @@ Optional
>    occupied by the redistributors. Required if more than one such
>    region is present.
>  
> +- save-suspend-state : Bool property. Setting this has the kernel save
> +  and restore the GIC state for suspend and resume respectively. For
> +  when the GIC loses power during suspend.

The name is pretty backward. You're supposed to describe a property of
the system, not instruct what SW should do.

What about something like "powered-off-on-suspend"?

Also, does this apply to the GIC? The ITS? The whole thing? Does it
make sense to treat them independently? (hint: probably).

> +
>  Sub-nodes:
>  
>  PPI affinity can be expressed as a single "ppi-partitions" node,
> @@ -107,6 +111,38 @@ Examples:
>  		};
>  	};
>  
> +	gic: interrupt-controller@fee00000 {
> +		compatible = "arm,gic-v3";
> +		#interrupt-cells = <4>;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		interrupt-controller;
> +		save-suspend-state;
> +
> +		reg = <0x0 0xfee00000 0 0x10000>, /* GICD */
> +		      <0x0 0xfef00000 0 0xc0000>, /* GICR */
> +		      <0x0 0xfff00000 0 0x10000>, /* GICC */
> +		      <0x0 0xfff10000 0 0x10000>, /* GICH */
> +		      <0x0 0xfff20000 0 0x10000>; /* GICV */
> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH 0>;
> +		its: interrupt-controller@fee20000 {
> +			compatible = "arm,gic-v3-its";
> +			msi-controller;
> +			reg = <0x0 0xfee20000 0x0 0x20000>;
> +		};
> +
> +		ppi-partitions {
> +			ppi_cluster0: interrupt-partition-0 {
> +				affinity = <&cpu_l0 &cpu_l1 &cpu_l2 &cpu_l3>;
> +			};
> +
> +			ppi_cluster1: interrupt-partition-1 {
> +				affinity = <&cpu_b0 &cpu_b1>;
> +			};
> +		};
> +	};
> +

I don't think we need another full example of the binding.

>  	gic: interrupt-controller@2c010000 {
>  		compatible = "arm,gic-v3";
>  		#interrupt-cells = <4>;
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 

Thanks,

	M.