devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
@ 2012-04-02 16:30 Marc Zyngier
  2012-04-03  9:22 ` David Vrabel
  0 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2012-04-02 16:30 UTC (permalink / raw)
  To: linux-arm-kernel, devicetree-discuss; +Cc: rob.herring

The GICv2 can have virtualization extension support, consisting
of an additional set of registers and interrupts. Add the necessary
binding to the GIC DT documentation.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/devicetree/bindings/arm/gic.txt |   29 +++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 9b4b82a..fb1b5ab 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -57,3 +57,32 @@ Example:
 		      <0xfff10100 0x100>;
 	};
 
+
+* GIC virtualization extensions (VGIC)
+
+For ARM cores that support the virtualization extensions, an
+additional node describes the VGIC hardware block. Only exists if the
+GIC is the primary interrupt controller.
+
+Required properties:
+- compatible : should be one of:
+	"arm,cortex-a15-vgic"
+	"arm,cortex-a7-vgic"
+	"arm,vgic"
+
+- reg : Specifies base physical address and size of the VGIC
+  registers. The first region is the GIC virtual interface control
+  register base and size.  the 2nd region is the GIC virtual cpu
+  interface register base and size.
+
+- interrupts : VGIC maintainance interrupt. Must be relative to the
+  primary GIC.
+
+Example:
+
+	vgic@2c004000 {
+		compatible = "arm,cortex-a15-vgic", "arm,vgic";
+		reg = <0x2c004000 0x2000>,
+		      <0x2c006000 0x2000>;
+		interrupts = <1 9 0xf04>;
+	};
-- 
1.7.7.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
  2012-04-02 16:30 [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC) Marc Zyngier
@ 2012-04-03  9:22 ` David Vrabel
  2012-04-03  9:53   ` Marc Zyngier
  0 siblings, 1 reply; 10+ messages in thread
From: David Vrabel @ 2012-04-03  9:22 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: devicetree-discuss, rob.herring, linux-arm-kernel

On 02/04/12 17:30, Marc Zyngier wrote:
> The GICv2 can have virtualization extension support, consisting
> of an additional set of registers and interrupts. Add the necessary
> binding to the GIC DT documentation.

The Xen hypervisor's device tree support is very much incomplete so I've
not looked into this is much detail.

Would it make more sense to extend the existing gic binding with the the
additional information rather than adding a new node?

David

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   29 +++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 9b4b82a..fb1b5ab 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -57,3 +57,32 @@ Example:
>  		      <0xfff10100 0x100>;
>  	};
>  
> +
> +* GIC virtualization extensions (VGIC)
> +
> +For ARM cores that support the virtualization extensions, an
> +additional node describes the VGIC hardware block. Only exists if the
> +GIC is the primary interrupt controller.
> +
> +Required properties:
> +- compatible : should be one of:
> +	"arm,cortex-a15-vgic"
> +	"arm,cortex-a7-vgic"
> +	"arm,vgic"
> +
> +- reg : Specifies base physical address and size of the VGIC
> +  registers. The first region is the GIC virtual interface control
> +  register base and size.  the 2nd region is the GIC virtual cpu
> +  interface register base and size.
> +
> +- interrupts : VGIC maintainance interrupt. Must be relative to the
> +  primary GIC.
> +
> +Example:
> +
> +	vgic@2c004000 {
> +		compatible = "arm,cortex-a15-vgic", "arm,vgic";
> +		reg = <0x2c004000 0x2000>,
> +		      <0x2c006000 0x2000>;
> +		interrupts = <1 9 0xf04>;
> +	};

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
  2012-04-03  9:22 ` David Vrabel
@ 2012-04-03  9:53   ` Marc Zyngier
  2012-04-03 15:35     ` Grant Likely
  0 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2012-04-03  9:53 UTC (permalink / raw)
  To: David Vrabel
  Cc: devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	linux-arm-kernel@lists.infradead.org

On 03/04/12 10:22, David Vrabel wrote:

Hi David,

> On 02/04/12 17:30, Marc Zyngier wrote:
>> The GICv2 can have virtualization extension support, consisting
>> of an additional set of registers and interrupts. Add the necessary
>> binding to the GIC DT documentation.
> 
> The Xen hypervisor's device tree support is very much incomplete so I've
> not looked into this is much detail.
> 
> Would it make more sense to extend the existing gic binding with the the
> additional information rather than adding a new node?

I'm actually torn between the two approaches. On one side, the VGIC is
part of the GIC spec, hence should be part of the GIC node. On the other
hand, it is logically handled by a different piece of software (the
hypervisor), and would normally be probed separately. Having a separate
node makes the probing more sensible.

It also makes it easier to dynamically filter out the VGIC node if you
want to derive the host DT for the guest (assuming you're emulating the
same platform).

But again, I have no strong feeling about it.

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
  2012-04-03  9:53   ` Marc Zyngier
@ 2012-04-03 15:35     ` Grant Likely
  2012-04-05 12:59       ` Marc Zyngier
  0 siblings, 1 reply; 10+ messages in thread
From: Grant Likely @ 2012-04-03 15:35 UTC (permalink / raw)
  To: Marc Zyngier, David Vrabel
  Cc: devicetree-discuss@lists.ozlabs.org,
	linux-arm-kernel@lists.infradead.org, rob.herring@calxeda.com

On Tue, 03 Apr 2012 10:53:44 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 03/04/12 10:22, David Vrabel wrote:
> 
> Hi David,
> 
> > On 02/04/12 17:30, Marc Zyngier wrote:
> >> The GICv2 can have virtualization extension support, consisting
> >> of an additional set of registers and interrupts. Add the necessary
> >> binding to the GIC DT documentation.
> > 
> > The Xen hypervisor's device tree support is very much incomplete so I've
> > not looked into this is much detail.
> > 
> > Would it make more sense to extend the existing gic binding with the the
> > additional information rather than adding a new node?
> 
> I'm actually torn between the two approaches. On one side, the VGIC is
> part of the GIC spec, hence should be part of the GIC node. On the other
> hand, it is logically handled by a different piece of software (the
> hypervisor), and would normally be probed separately. Having a separate
> node makes the probing more sensible.

Don't get too hung up on the software side of things.  Describe it in
a way that makes sense for the hardware.  There is lots of precidence
for two hunks of software initializating from the same node; either by
probe kicking off two init hooks, or by early init code going looking
for the node manually.

g.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
  2012-04-03 15:35     ` Grant Likely
@ 2012-04-05 12:59       ` Marc Zyngier
       [not found]         ` <4F7D974C.9050506-5wv7dgnIgG8@public.gmane.org>
  2012-04-07  1:35         ` Grant Likely
  0 siblings, 2 replies; 10+ messages in thread
From: Marc Zyngier @ 2012-04-05 12:59 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-arm-kernel@lists.infradead.org,
	devicetree-discuss@lists.ozlabs.org, David Vrabel,
	rob.herring@calxeda.com

On 03/04/12 16:35, Grant Likely wrote:

Hi Grant,

> On Tue, 03 Apr 2012 10:53:44 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 03/04/12 10:22, David Vrabel wrote:
>>
>> Hi David,
>>
>>> On 02/04/12 17:30, Marc Zyngier wrote:
>>>> The GICv2 can have virtualization extension support, consisting
>>>> of an additional set of registers and interrupts. Add the necessary
>>>> binding to the GIC DT documentation.
>>>
>>> The Xen hypervisor's device tree support is very much incomplete so I've
>>> not looked into this is much detail.
>>>
>>> Would it make more sense to extend the existing gic binding with the the
>>> additional information rather than adding a new node?
>>
>> I'm actually torn between the two approaches. On one side, the VGIC is
>> part of the GIC spec, hence should be part of the GIC node. On the other
>> hand, it is logically handled by a different piece of software (the
>> hypervisor), and would normally be probed separately. Having a separate
>> node makes the probing more sensible.
> 
> Don't get too hung up on the software side of things.  Describe it in
> a way that makes sense for the hardware.  There is lots of precidence
> for two hunks of software initializating from the same node; either by
> probe kicking off two init hooks, or by early init code going looking
> for the node manually.

What I'm trying to avoid is a royal mess in the future if we get some
other extension to the GIC.

Let's say we implement the following:

	gic: interrupt-controller@2c001000 {
		compatible = "arm,cortex-a15-gic";
		#interrupt-cells = <3>;
		#address-cells = <1>;
		interrupt-controller;
		reg = <0x2c001000 0x1000>,
		      <0x2c002000 0x100>,
		      <0x2c004000 0x2000>,
		      <0x2c006000 0x2000>;
		interrupts = <1 9 0xf04>;
	};

It's all fine (the two last regions and the interrupt are for VGIC),
until someone comes up with extension FOO which requires two new regions
and am interrupt. It is then impossible to distinguish between the two,
short of adding more attributes.

How about this?

	gic: interrupt-controller@2c001000 {
		compatible = "arm,cortex-a15-gic";
		#interrupt-cells = <3>;
		#address-cells = <1>;
		#size-cells = <1>;
		interrupt-controller;
		reg = <0x2c001000 0x1000>,
		      <0x2c002000 0x100>;

		vgic@2c004000 {
			compatible = "arm,cortex-a15-vgic", "arm,vgic";
			reg = <0x2c004000 0x2000>,
			      <0x2c006000 0x2000>;
			interrupts = <1 9 0xf04>;
		};
	};

It cleanly separate the extension from the core GIC, and still make it
part of the GIC node.

What do you think?

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
       [not found]         ` <4F7D974C.9050506-5wv7dgnIgG8@public.gmane.org>
@ 2012-04-05 13:15           ` David Vrabel
  2012-04-05 13:34           ` Rob Herring
  1 sibling, 0 replies; 10+ messages in thread
From: David Vrabel @ 2012-04-05 13:15 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org

On 05/04/12 13:59, Marc Zyngier wrote:
> 
> How about this?
> 
> 	gic: interrupt-controller@2c001000 {
> 		compatible = "arm,cortex-a15-gic";
> 		#interrupt-cells = <3>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		interrupt-controller;
> 		reg = <0x2c001000 0x1000>,
> 		      <0x2c002000 0x100>;
> 
> 		vgic@2c004000 {
> 			compatible = "arm,cortex-a15-vgic", "arm,vgic";
> 			reg = <0x2c004000 0x2000>,
> 			      <0x2c006000 0x2000>;
> 			interrupts = <1 9 0xf04>;
> 		};
> 	};
> 
> It cleanly separate the extension from the core GIC, and still make it
> part of the GIC node.

This looks sensible to me.

David

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
       [not found]         ` <4F7D974C.9050506-5wv7dgnIgG8@public.gmane.org>
  2012-04-05 13:15           ` David Vrabel
@ 2012-04-05 13:34           ` Rob Herring
  2012-04-05 14:07             ` Marc Zyngier
  1 sibling, 1 reply; 10+ messages in thread
From: Rob Herring @ 2012-04-05 13:34 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org, David Vrabel,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

On 04/05/2012 07:59 AM, Marc Zyngier wrote:
> On 03/04/12 16:35, Grant Likely wrote:
> 
> Hi Grant,
> 
>> On Tue, 03 Apr 2012 10:53:44 +0100, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>>> On 03/04/12 10:22, David Vrabel wrote:
>>>
>>> Hi David,
>>>
>>>> On 02/04/12 17:30, Marc Zyngier wrote:
>>>>> The GICv2 can have virtualization extension support, consisting
>>>>> of an additional set of registers and interrupts. Add the necessary
>>>>> binding to the GIC DT documentation.
>>>>
>>>> The Xen hypervisor's device tree support is very much incomplete so I've
>>>> not looked into this is much detail.
>>>>
>>>> Would it make more sense to extend the existing gic binding with the the
>>>> additional information rather than adding a new node?
>>>
>>> I'm actually torn between the two approaches. On one side, the VGIC is
>>> part of the GIC spec, hence should be part of the GIC node. On the other
>>> hand, it is logically handled by a different piece of software (the
>>> hypervisor), and would normally be probed separately. Having a separate
>>> node makes the probing more sensible.
>>
>> Don't get too hung up on the software side of things.  Describe it in
>> a way that makes sense for the hardware.  There is lots of precidence
>> for two hunks of software initializating from the same node; either by
>> probe kicking off two init hooks, or by early init code going looking
>> for the node manually.
> 
> What I'm trying to avoid is a royal mess in the future if we get some
> other extension to the GIC.
> 

But that would be a new compatible string as is this case.

> Let's say we implement the following:
> 
> 	gic: interrupt-controller@2c001000 {
> 		compatible = "arm,cortex-a15-gic";
> 		#interrupt-cells = <3>;
> 		#address-cells = <1>;
> 		interrupt-controller;
> 		reg = <0x2c001000 0x1000>,
> 		      <0x2c002000 0x100>,
> 		      <0x2c004000 0x2000>,
> 		      <0x2c006000 0x2000>;
> 		interrupts = <1 9 0xf04>;

Does this work having an interrupt within the parent itself? Normally
this would be the connection to the next level up.

> 	};
> 
> It's all fine (the two last regions and the interrupt are for VGIC),
> until someone comes up with extension FOO which requires two new regions
> and am interrupt. It is then impossible to distinguish between the two,
> short of adding more attributes.
> 
> How about this?
> 
> 	gic: interrupt-controller@2c001000 {
> 		compatible = "arm,cortex-a15-gic";
> 		#interrupt-cells = <3>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		interrupt-controller;
> 		reg = <0x2c001000 0x1000>,
> 		      <0x2c002000 0x100>;
> 
> 		vgic@2c004000 {
> 			compatible = "arm,cortex-a15-vgic", "arm,vgic";
> 			reg = <0x2c004000 0x2000>,
> 			      <0x2c006000 0x2000>;
> 			interrupts = <1 9 0xf04>;
> 		};
> 	};
> 
> It cleanly separate the extension from the core GIC, and still make it
> part of the GIC node.
> 
> What do you think?
> 

I prefer the first option.

Rob

> 	M.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
  2012-04-05 13:34           ` Rob Herring
@ 2012-04-05 14:07             ` Marc Zyngier
       [not found]               ` <4F7DA71A.4020809-5wv7dgnIgG8@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2012-04-05 14:07 UTC (permalink / raw)
  To: Rob Herring
  Cc: Grant Likely, devicetree-discuss@lists.ozlabs.org,
	rob.herring@calxeda.com, David Vrabel,
	linux-arm-kernel@lists.infradead.org

On 05/04/12 14:34, Rob Herring wrote:
> On 04/05/2012 07:59 AM, Marc Zyngier wrote:
>> On 03/04/12 16:35, Grant Likely wrote:
>>
>> Hi Grant,
>>
>>> On Tue, 03 Apr 2012 10:53:44 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 03/04/12 10:22, David Vrabel wrote:
>>>>
>>>> Hi David,
>>>>
>>>>> On 02/04/12 17:30, Marc Zyngier wrote:
>>>>>> The GICv2 can have virtualization extension support, consisting
>>>>>> of an additional set of registers and interrupts. Add the necessary
>>>>>> binding to the GIC DT documentation.
>>>>>
>>>>> The Xen hypervisor's device tree support is very much incomplete so I've
>>>>> not looked into this is much detail.
>>>>>
>>>>> Would it make more sense to extend the existing gic binding with the the
>>>>> additional information rather than adding a new node?
>>>>
>>>> I'm actually torn between the two approaches. On one side, the VGIC is
>>>> part of the GIC spec, hence should be part of the GIC node. On the other
>>>> hand, it is logically handled by a different piece of software (the
>>>> hypervisor), and would normally be probed separately. Having a separate
>>>> node makes the probing more sensible.
>>>
>>> Don't get too hung up on the software side of things.  Describe it in
>>> a way that makes sense for the hardware.  There is lots of precidence
>>> for two hunks of software initializating from the same node; either by
>>> probe kicking off two init hooks, or by early init code going looking
>>> for the node manually.
>>
>> What I'm trying to avoid is a royal mess in the future if we get some
>> other extension to the GIC.
>>
> 
> But that would be a new compatible string as is this case.

Yes, probably.

>> Let's say we implement the following:
>>
>> 	gic: interrupt-controller@2c001000 {
>> 		compatible = "arm,cortex-a15-gic";
>> 		#interrupt-cells = <3>;
>> 		#address-cells = <1>;
>> 		interrupt-controller;
>> 		reg = <0x2c001000 0x1000>,
>> 		      <0x2c002000 0x100>,
>> 		      <0x2c004000 0x2000>,
>> 		      <0x2c006000 0x2000>;
>> 		interrupts = <1 9 0xf04>;
> 
> Does this work having an interrupt within the parent itself? Normally
> this would be the connection to the next level up.

In this case we don't have an interrupt parent property, so the GIC is
properly identified as the top-level interrupt controller. This is
admittedly a bit fragile, and contradicts the current wording of the
binding.

>> 	};
>>
>> It's all fine (the two last regions and the interrupt are for VGIC),
>> until someone comes up with extension FOO which requires two new regions
>> and am interrupt. It is then impossible to distinguish between the two,
>> short of adding more attributes.
>>
>> How about this?
>>
>> 	gic: interrupt-controller@2c001000 {
>> 		compatible = "arm,cortex-a15-gic";
>> 		#interrupt-cells = <3>;
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		interrupt-controller;
>> 		reg = <0x2c001000 0x1000>,
>> 		      <0x2c002000 0x100>;
>>
>> 		vgic@2c004000 {
>> 			compatible = "arm,cortex-a15-vgic", "arm,vgic";
>> 			reg = <0x2c004000 0x2000>,
>> 			      <0x2c006000 0x2000>;
>> 			interrupts = <1 9 0xf04>;
>> 		};
>> 	};
>>
>> It cleanly separate the extension from the core GIC, and still make it
>> part of the GIC node.
>>
>> What do you think?
>>
> 
> I prefer the first option.

The first I posted (vgic node outside of the gic), or the one with
everything in the same node?

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
       [not found]               ` <4F7DA71A.4020809-5wv7dgnIgG8@public.gmane.org>
@ 2012-04-05 14:25                 ` Rob Herring
  0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2012-04-05 14:25 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org, David Vrabel,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

On 04/05/2012 09:07 AM, Marc Zyngier wrote:
> On 05/04/12 14:34, Rob Herring wrote:
>> On 04/05/2012 07:59 AM, Marc Zyngier wrote:
>>> On 03/04/12 16:35, Grant Likely wrote:
>>>
>>> Hi Grant,
>>>
>>>> On Tue, 03 Apr 2012 10:53:44 +0100, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>>>>> On 03/04/12 10:22, David Vrabel wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>>> On 02/04/12 17:30, Marc Zyngier wrote:
>>>>>>> The GICv2 can have virtualization extension support, consisting
>>>>>>> of an additional set of registers and interrupts. Add the necessary
>>>>>>> binding to the GIC DT documentation.
>>>>>>
>>>>>> The Xen hypervisor's device tree support is very much incomplete so I've
>>>>>> not looked into this is much detail.
>>>>>>
>>>>>> Would it make more sense to extend the existing gic binding with the the
>>>>>> additional information rather than adding a new node?
>>>>>
>>>>> I'm actually torn between the two approaches. On one side, the VGIC is
>>>>> part of the GIC spec, hence should be part of the GIC node. On the other
>>>>> hand, it is logically handled by a different piece of software (the
>>>>> hypervisor), and would normally be probed separately. Having a separate
>>>>> node makes the probing more sensible.
>>>>
>>>> Don't get too hung up on the software side of things.  Describe it in
>>>> a way that makes sense for the hardware.  There is lots of precidence
>>>> for two hunks of software initializating from the same node; either by
>>>> probe kicking off two init hooks, or by early init code going looking
>>>> for the node manually.
>>>
>>> What I'm trying to avoid is a royal mess in the future if we get some
>>> other extension to the GIC.
>>>
>>
>> But that would be a new compatible string as is this case.
> 
> Yes, probably.
> 
>>> Let's say we implement the following:
>>>
>>> 	gic: interrupt-controller@2c001000 {
>>> 		compatible = "arm,cortex-a15-gic";
>>> 		#interrupt-cells = <3>;
>>> 		#address-cells = <1>;
>>> 		interrupt-controller;
>>> 		reg = <0x2c001000 0x1000>,
>>> 		      <0x2c002000 0x100>,
>>> 		      <0x2c004000 0x2000>,
>>> 		      <0x2c006000 0x2000>;
>>> 		interrupts = <1 9 0xf04>;
>>
>> Does this work having an interrupt within the parent itself? Normally
>> this would be the connection to the next level up.
> 
> In this case we don't have an interrupt parent property, so the GIC is
> properly identified as the top-level interrupt controller. This is
> admittedly a bit fragile, and contradicts the current wording of the
> binding.
> 

Actually, you typically do have interrupt parent set in the next level
up if the gic is one the same bus or you globally set the parent. If the
parent is itself, the code treats this as the root controller.

>>> 	};
>>>
>>> It's all fine (the two last regions and the interrupt are for VGIC),
>>> until someone comes up with extension FOO which requires two new regions
>>> and am interrupt. It is then impossible to distinguish between the two,
>>> short of adding more attributes.
>>>
>>> How about this?
>>>
>>> 	gic: interrupt-controller@2c001000 {
>>> 		compatible = "arm,cortex-a15-gic";
>>> 		#interrupt-cells = <3>;
>>> 		#address-cells = <1>;
>>> 		#size-cells = <1>;
>>> 		interrupt-controller;
>>> 		reg = <0x2c001000 0x1000>,
>>> 		      <0x2c002000 0x100>;
>>>
>>> 		vgic@2c004000 {
>>> 			compatible = "arm,cortex-a15-vgic", "arm,vgic";
>>> 			reg = <0x2c004000 0x2000>,
>>> 			      <0x2c006000 0x2000>;
>>> 			interrupts = <1 9 0xf04>;
>>> 		};
>>> 	};
>>>
>>> It cleanly separate the extension from the core GIC, and still make it
>>> part of the GIC node.
>>>
>>> What do you think?
>>>
>>
>> I prefer the first option.
> 
> The first I posted (vgic node outside of the gic), or the one with
> everything in the same node?
> 

The first one here. I don't see any advantage of a separate node.

Rob

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC)
  2012-04-05 12:59       ` Marc Zyngier
       [not found]         ` <4F7D974C.9050506-5wv7dgnIgG8@public.gmane.org>
@ 2012-04-07  1:35         ` Grant Likely
  1 sibling, 0 replies; 10+ messages in thread
From: Grant Likely @ 2012-04-07  1:35 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-arm-kernel@lists.infradead.org,
	devicetree-discuss@lists.ozlabs.org, David Vrabel,
	rob.herring@calxeda.com

On Thu, 05 Apr 2012 13:59:56 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 03/04/12 16:35, Grant Likely wrote:
> 
> Hi Grant,
> 
> > On Tue, 03 Apr 2012 10:53:44 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> On 03/04/12 10:22, David Vrabel wrote:
> >>
> >> Hi David,
> >>
> >>> On 02/04/12 17:30, Marc Zyngier wrote:
> >>>> The GICv2 can have virtualization extension support, consisting
> >>>> of an additional set of registers and interrupts. Add the necessary
> >>>> binding to the GIC DT documentation.
> >>>
> >>> The Xen hypervisor's device tree support is very much incomplete so I've
> >>> not looked into this is much detail.
> >>>
> >>> Would it make more sense to extend the existing gic binding with the the
> >>> additional information rather than adding a new node?
> >>
> >> I'm actually torn between the two approaches. On one side, the VGIC is
> >> part of the GIC spec, hence should be part of the GIC node. On the other
> >> hand, it is logically handled by a different piece of software (the
> >> hypervisor), and would normally be probed separately. Having a separate
> >> node makes the probing more sensible.
> > 
> > Don't get too hung up on the software side of things.  Describe it in
> > a way that makes sense for the hardware.  There is lots of precidence
> > for two hunks of software initializating from the same node; either by
> > probe kicking off two init hooks, or by early init code going looking
> > for the node manually.
> 
> What I'm trying to avoid is a royal mess in the future if we get some
> other extension to the GIC.
> 
> Let's say we implement the following:
> 
> 	gic: interrupt-controller@2c001000 {
> 		compatible = "arm,cortex-a15-gic";
> 		#interrupt-cells = <3>;
> 		#address-cells = <1>;
> 		interrupt-controller;
> 		reg = <0x2c001000 0x1000>,
> 		      <0x2c002000 0x100>,
> 		      <0x2c004000 0x2000>,
> 		      <0x2c006000 0x2000>;
> 		interrupts = <1 9 0xf04>;
> 	};
> 
> It's all fine (the two last regions and the interrupt are for VGIC),
> until someone comes up with extension FOO which requires two new regions
> and am interrupt. It is then impossible to distinguish between the two,
> short of adding more attributes.
> 
> How about this?
> 
> 	gic: interrupt-controller@2c001000 {
> 		compatible = "arm,cortex-a15-gic";
> 		#interrupt-cells = <3>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		interrupt-controller;
> 		reg = <0x2c001000 0x1000>,
> 		      <0x2c002000 0x100>;
> 
> 		vgic@2c004000 {
> 			compatible = "arm,cortex-a15-vgic", "arm,vgic";
> 			reg = <0x2c004000 0x2000>,
> 			      <0x2c006000 0x2000>;
> 			interrupts = <1 9 0xf04>;
> 		};
> 	};
> 
> It cleanly separate the extension from the core GIC, and still make it
> part of the GIC node.

I think that the compatible property already supports what you need to
do.  If an extension is added to the core binding, then suppliment it
with a new compatible value:

	gic: interrupt-controller@2c001000 {
		compatible = "arm,cortex-a15-vgic", "arm,cortex-a15-gic";
		#interrupt-cells = <3>;
		#address-cells = <1>;
		#size-cells = <1>;
		interrupt-controller;
		reg = <0x2c001000 0x1000>,
		      <0x2c002000 0x100>,
		      <0x2c004000 0x2000>,
		      <0x2c006000 0x2000>;
		interrupts = <1 9 0xf04>;
	};

Or, if it can be done in a backwards compatible way, then just extend
the existing binding.  In you're example above, the 'vgic' extension
only gets enabled if the 3rd and 4th 'reg' tuples are added.

g.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-04-07  1:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-02 16:30 [PATCH] ARM: DT: Add binding for GIC virtualization extentions (VGIC) Marc Zyngier
2012-04-03  9:22 ` David Vrabel
2012-04-03  9:53   ` Marc Zyngier
2012-04-03 15:35     ` Grant Likely
2012-04-05 12:59       ` Marc Zyngier
     [not found]         ` <4F7D974C.9050506-5wv7dgnIgG8@public.gmane.org>
2012-04-05 13:15           ` David Vrabel
2012-04-05 13:34           ` Rob Herring
2012-04-05 14:07             ` Marc Zyngier
     [not found]               ` <4F7DA71A.4020809-5wv7dgnIgG8@public.gmane.org>
2012-04-05 14:25                 ` Rob Herring
2012-04-07  1:35         ` Grant Likely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).