| Message ID | 1414541562-10076-3-git-send-email-abrestic@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Andrew, On 29/10/14 00:12, Andrew Bresticker wrote: > - changed compatible string to include CPU version > +Required properties: > +- compatible : Should be "mti,<cpu>-gic". Supported variants: > + - "mti,interaptiv-gic" > +Required properties for timer sub-node: > +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants: > + - "mti,interaptiv-gic-timer" Erm, I'm a bit confused... Why do you include the core name in the compatible string? You seem to be suggesting that: 1) The GIC/timer drivers need to know what core they're running on. Is that really true? 2) It isn't possible to probe the core type. But the kernel already knows this, so what's wrong with using current_cpu_type() like everything else that needs to know? 3) Every new core should require a new compatible string to be added before the GIC will work. You don't even have a generic compatible string that DT can specify after the core specific one as a fallback. Please lets not do this unless it's actually necessary (which AFAICT it really isn't). Thanks James -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/29/2014 12:12 AM, Andrew Bresticker wrote: > +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors > + to which the GIC may route interrupts. May contain up to 6 entries, one > + for each of the CPU's hardware interrupt vectors. Valid values are 2 - 7. > + This property is ignored if the CPU is started in EIC mode. > + Wouldn't it be better to have this in the reversed sense ie: mti,nonavailable-cpu-vectors? I think the assumption that by default they're all available unless something else is connected to them which is unlikely in most cases. It can be made optional property then. I don't have a strong opinion about it though. Qais -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/29/2014 12:12 AM, Andrew Bresticker wrote: > +- reg : Base address and length of the GIC registers. > Also except for sead3, the base address should be properly reported by the hardware. The size is fixed (for a specific version of GIC at least - which is also reported by the hardware). So it would be nice to make this optional. Qais -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi James, On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <james.hogan@imgtec.com> wrote: > Hi Andrew, > > On 29/10/14 00:12, Andrew Bresticker wrote: >> - changed compatible string to include CPU version > >> +Required properties: >> +- compatible : Should be "mti,<cpu>-gic". Supported variants: >> + - "mti,interaptiv-gic" > >> +Required properties for timer sub-node: >> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants: >> + - "mti,interaptiv-gic-timer" > > Erm, I'm a bit confused... > Why do you include the core name in the compatible string? > > You seem to be suggesting that: > > 1) The GIC/timer drivers need to know what core they're running on. > > Is that really true? They don't now, but it's possible that a future CPU has a newer revision of the GIC which has some differences that need to be accounted for in the driver. > 2) It isn't possible to probe the core type. > > But the kernel already knows this, so what's wrong with using > current_cpu_type() like everything else that needs to know? > > 3) Every new core should require a new compatible string to be added > before the GIC will work. You don't even have a generic compatible > string that DT can specify after the core specific one as a fallback. Yes, adding a generic compatible string would be a good idea. > Please lets not do this unless it's actually necessary (which AFAICT it > really isn't). The point of this was to future-proof these bindings and I though that CPU type was the best way to indicate version in the compatible string. This is also how it's done for the ARM GIC and arch timers. Perhaps the best thing to do is to require both a core-specific ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and just match on the generic one for now until there's a need to use the core-specific one. Thoughts? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 4:01 AM, Qais Yousef <qais.yousef@imgtec.com> wrote: > On 10/29/2014 12:12 AM, Andrew Bresticker wrote: >> >> +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors >> + to which the GIC may route interrupts. May contain up to 6 entries, >> one >> + for each of the CPU's hardware interrupt vectors. Valid values are 2 - >> 7. >> + This property is ignored if the CPU is started in EIC mode. >> + > > > Wouldn't it be better to have this in the reversed sense ie: > mti,nonavailable-cpu-vectors? I think the assumption that by default they're > all available unless something else is connected to them which is unlikely > in most cases. It can be made optional property then. > > I don't have a strong opinion about it though. Actually, I think I like the reversed sense as well. Perhaps "mti,reserved-cpu-vectors"? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/29/2014 04:56 PM, Andrew Bresticker wrote: > On Wed, Oct 29, 2014 at 4:01 AM, Qais Yousef <qais.yousef@imgtec.com> wrote: >> On 10/29/2014 12:12 AM, Andrew Bresticker wrote: >>> +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors >>> + to which the GIC may route interrupts. May contain up to 6 entries, >>> one >>> + for each of the CPU's hardware interrupt vectors. Valid values are 2 - >>> 7. >>> + This property is ignored if the CPU is started in EIC mode. >>> + >> >> Wouldn't it be better to have this in the reversed sense ie: >> mti,nonavailable-cpu-vectors? I think the assumption that by default they're >> all available unless something else is connected to them which is unlikely >> in most cases. It can be made optional property then. >> >> I don't have a strong opinion about it though. > Actually, I think I like the reversed sense as well. Perhaps > "mti,reserved-cpu-vectors"? Yep that's a better wording for sure :) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 4:09 AM, Qais Yousef <qais.yousef@imgtec.com> wrote: > On 10/29/2014 12:12 AM, Andrew Bresticker wrote: >> >> +- reg : Base address and length of the GIC registers. >> > > Also except for sead3, the base address should be properly reported by the > hardware. The size is fixed (for a specific version of GIC at least - which > is also reported by the hardware). So it would be nice to make this > optional. Even though this is usually probable, I'd prefer to leave this as required, or at least "optional, but recommended". I don't have a very strong opinion on it though, but perhaps the device-tree folks do? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/10/14 16:55, Andrew Bresticker wrote: > Hi James, > > On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <james.hogan@imgtec.com> wrote: >> Hi Andrew, >> >> On 29/10/14 00:12, Andrew Bresticker wrote: >>> - changed compatible string to include CPU version >> >>> +Required properties: >>> +- compatible : Should be "mti,<cpu>-gic". Supported variants: >>> + - "mti,interaptiv-gic" >> >>> +Required properties for timer sub-node: >>> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants: >>> + - "mti,interaptiv-gic-timer" >> >> Erm, I'm a bit confused... >> Why do you include the core name in the compatible string? >> >> You seem to be suggesting that: >> >> 1) The GIC/timer drivers need to know what core they're running on. >> >> Is that really true? > > They don't now, but it's possible that a future CPU has a newer > revision of the GIC which has some differences that need to be > accounted for in the driver. > >> 2) It isn't possible to probe the core type. >> >> But the kernel already knows this, so what's wrong with using >> current_cpu_type() like everything else that needs to know? >> >> 3) Every new core should require a new compatible string to be added >> before the GIC will work. You don't even have a generic compatible >> string that DT can specify after the core specific one as a fallback. > > Yes, adding a generic compatible string would be a good idea. > >> Please lets not do this unless it's actually necessary (which AFAICT it >> really isn't). > > The point of this was to future-proof these bindings and I though that > CPU type was the best way to indicate version in the compatible > string. This is also how it's done for the ARM GIC and arch timers. > Perhaps the best thing to do is to require both a core-specific > ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and > just match on the generic one for now until there's a need to use the > core-specific one. Thoughts? FPGA boards like Malta are something else to consider (when it is eventually converted to DT - Paul on CC knows more than me). You might load an interAptiv, or a proAptiv, or a P5600 bitstream, and the gic setup will be pretty much the same I think, since e.g. the address depends on where it is convenient to put it in the address space of the platform. Any thoughts on the existence of current_cpu_type(), and the GIC revision register? They pretty much make encoding of core in compatible string redundant I think. Cheers James -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/29/2014 05:08 PM, Andrew Bresticker wrote: > On Wed, Oct 29, 2014 at 4:09 AM, Qais Yousef <qais.yousef@imgtec.com> wrote: >> On 10/29/2014 12:12 AM, Andrew Bresticker wrote: >>> +- reg : Base address and length of the GIC registers. >>> >> Also except for sead3, the base address should be properly reported by the >> hardware. The size is fixed (for a specific version of GIC at least - which >> is also reported by the hardware). So it would be nice to make this >> optional. > Even though this is usually probable, I'd prefer to leave this as > required, or at least "optional, but recommended". I don't have a > very strong opinion on it though, but perhaps the device-tree folks > do? The biggest advantage I can think of is that it can potentially make GIC DT definition more shareable across for instance multiple revisions of an SoC that might have the GIC at different base addresses. I won't insist too much though. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 10:13 AM, James Hogan <james.hogan@imgtec.com> wrote: > On 29/10/14 16:55, Andrew Bresticker wrote: >> Hi James, >> >> On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <james.hogan@imgtec.com> wrote: >>> Hi Andrew, >>> >>> On 29/10/14 00:12, Andrew Bresticker wrote: >>>> - changed compatible string to include CPU version >>> >>>> +Required properties: >>>> +- compatible : Should be "mti,<cpu>-gic". Supported variants: >>>> + - "mti,interaptiv-gic" >>> >>>> +Required properties for timer sub-node: >>>> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants: >>>> + - "mti,interaptiv-gic-timer" >>> >>> Erm, I'm a bit confused... >>> Why do you include the core name in the compatible string? >>> >>> You seem to be suggesting that: >>> >>> 1) The GIC/timer drivers need to know what core they're running on. >>> >>> Is that really true? >> >> They don't now, but it's possible that a future CPU has a newer >> revision of the GIC which has some differences that need to be >> accounted for in the driver. >> >>> 2) It isn't possible to probe the core type. >>> >>> But the kernel already knows this, so what's wrong with using >>> current_cpu_type() like everything else that needs to know? >>> >>> 3) Every new core should require a new compatible string to be added >>> before the GIC will work. You don't even have a generic compatible >>> string that DT can specify after the core specific one as a fallback. >> >> Yes, adding a generic compatible string would be a good idea. >> >>> Please lets not do this unless it's actually necessary (which AFAICT it >>> really isn't). >> >> The point of this was to future-proof these bindings and I though that >> CPU type was the best way to indicate version in the compatible >> string. This is also how it's done for the ARM GIC and arch timers. >> Perhaps the best thing to do is to require both a core-specific >> ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and >> just match on the generic one for now until there's a need to use the >> core-specific one. Thoughts? > > FPGA boards like Malta are something else to consider (when it is > eventually converted to DT - Paul on CC knows more than me). You might > load an interAptiv, or a proAptiv, or a P5600 bitstream, and the gic > setup will be pretty much the same I think, since e.g. the address > depends on where it is convenient to put it in the address space of the > platform. Ah, I didn't realize that the CPU bitstream could be changed independently of the GIC. In that case, the CPU revision isn't that useful. > Any thoughts on the existence of current_cpu_type(), and the GIC > revision register? They pretty much make encoding of core in compatible > string redundant I think. Ok, I suppose using the revision register is fine then. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 05:23:59PM +0000, Qais Yousef wrote: > On 10/29/2014 05:08 PM, Andrew Bresticker wrote: > > On Wed, Oct 29, 2014 at 4:09 AM, Qais Yousef <qais.yousef@imgtec.com> wrote: > >> On 10/29/2014 12:12 AM, Andrew Bresticker wrote: > >>> +- reg : Base address and length of the GIC registers. > >>> > >> Also except for sead3, the base address should be properly reported by the > >> hardware. The size is fixed (for a specific version of GIC at least - which > >> is also reported by the hardware). So it would be nice to make this > >> optional. > > Even though this is usually probable, I'd prefer to leave this as > > required, or at least "optional, but recommended". I don't have a > > very strong opinion on it though, but perhaps the device-tree folks > > do? It boils down to how reliable the values you can read out of the HW are. If it can always be probed reliably, then the property isn't strictly necessary. However, the fact that you can probe it now doesn't mean you can always probe it reliably (e.g. a future CPU's reporting mechanism might be different, or values might be plain wrong). It depends on what you expect in future in that regard. As an example, for a while on ARM we thought we could probe the number of CPUs present from the HW, but new CPUs and multi-cluster designs broke our assumptions there. Now we just rely on the /cpus node containing the appropriate cpu sub-nodes (regardless of whether this could be probed for a particular CPU/SoC). Luckily we were able to change that as we were still in the early days of DT conversion. So consider the possible ways your current probing mechanism is realistically likely to be broken. If there are clear ways of working around that, you're probably fine with an optional property. If it looks like things could change substantially, require the property for now -- you can always ignore it in future if things turn out to be reliably probeable through other means. > The biggest advantage I can think of is that it can potentially make GIC > DT definition more shareable across for instance multiple revisions of > an SoC that might have the GIC at different base addresses. For the different revisions of an SoC, I would expect that if your interrupt controller moved other elements would also? A layer or two of dtsi files can keep all the common stuff common while allowing per-revision changes. > I won't insist too much though. Similarly, I don't really have a strong opinion either way. There's no single answer on this. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 04:55:56PM +0000, Andrew Bresticker wrote: > Hi James, > > On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <james.hogan@imgtec.com> wrote: > > Hi Andrew, > > > > On 29/10/14 00:12, Andrew Bresticker wrote: > >> - changed compatible string to include CPU version > > > >> +Required properties: > >> +- compatible : Should be "mti,<cpu>-gic". Supported variants: > >> + - "mti,interaptiv-gic" > > > >> +Required properties for timer sub-node: > >> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants: > >> + - "mti,interaptiv-gic-timer" > > > > Erm, I'm a bit confused... > > Why do you include the core name in the compatible string? > > > > You seem to be suggesting that: > > > > 1) The GIC/timer drivers need to know what core they're running on. > > > > Is that really true? > > They don't now, but it's possible that a future CPU has a newer > revision of the GIC which has some differences that need to be > accounted for in the driver. At that point you can allocate a new compatible string. Until then you don't necessarily need to distinguish. Is the timer defined by the architecture, or is it specific to this CPU (and might get reused in future)? > > 2) It isn't possible to probe the core type. > > > > But the kernel already knows this, so what's wrong with using > > current_cpu_type() like everything else that needs to know? > > > > 3) Every new core should require a new compatible string to be added > > before the GIC will work. You don't even have a generic compatible > > string that DT can specify after the core specific one as a fallback. > > Yes, adding a generic compatible string would be a good idea. > > > Please lets not do this unless it's actually necessary (which AFAICT it > > really isn't). > > The point of this was to future-proof these bindings and I though that > CPU type was the best way to indicate version in the compatible > string. This is also how it's done for the ARM GIC and arch timers. > Perhaps the best thing to do is to require both a core-specific > ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and > just match on the generic one for now until there's a need to use the > core-specific one. Thoughts? If this timer is architected you can have a generic string for now, with each CPU having a more specific string just in case, e.g. compatible = "mti,interaptiv-gic-timer", "mti,gic-timer". The kernel driver can currently match just "mti,gic-timer", andd everything should be fine if it turns out nothing changes with new CPUs: compatible = "mti,newcpu-gic-timer", "mti,gic-timer"; If the new CPU's timer doesn't quite match, you add its comaptible string to the driver, and drop "mti,gic-timer" from the node's comaptible list: compatible = "mti,newcpu-gic-timer"; Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On Wed, Oct 29, 2014 at 10:25:27AM -0700, Andrew Bresticker wrote: > On Wed, Oct 29, 2014 at 10:13 AM, James Hogan <james.hogan@imgtec.com> wrote: > > On 29/10/14 16:55, Andrew Bresticker wrote: > >> On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <james.hogan@imgtec.com> wrote: > >>> Please lets not do this unless it's actually necessary (which AFAICT it > >>> really isn't). > >> > >> The point of this was to future-proof these bindings and I though that > >> CPU type was the best way to indicate version in the compatible > >> string. This is also how it's done for the ARM GIC and arch timers. > >> Perhaps the best thing to do is to require both a core-specific > >> ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and > >> just match on the generic one for now until there's a need to use the > >> core-specific one. Thoughts? > > > > FPGA boards like Malta are something else to consider (when it is > > eventually converted to DT - Paul on CC knows more than me). You might > > load an interAptiv, or a proAptiv, or a P5600 bitstream, and the gic > > setup will be pretty much the same I think, since e.g. the address > > depends on where it is convenient to put it in the address space of the > > platform. > > Ah, I didn't realize that the CPU bitstream could be changed > independently of the GIC. To clarify, the GIC is still closely bound to the CPU and contained within the FPGA bitstream. The register interface should I believe always comply with some version of the GIC architecture specification, and I don't think anybody wants per-bitstream DT files / kernels, so in practice the way the GIC is set up for Malta (how interrupt lines are connected up and where in address space GIC can go) is unlikely to become incompatible. Cheers James -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt b/Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt new file mode 100644 index 0000000..84cbbed --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt @@ -0,0 +1,55 @@ +MIPS Global Interrupt Controller (GIC) + +The MIPS GIC routes external interrupts to individual VPEs and IRQ pins. +It also supports local (per-processor) interrupts and software-generated +interrupts which can be used as IPIs. The GIC also includes a free-running +global timer, per-CPU count/compare timers, and a watchdog. + +Required properties: +- compatible : Should be "mti,<cpu>-gic". Supported variants: + - "mti,interaptiv-gic" +- reg : Base address and length of the GIC registers. +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt specifier. Should be 3. + - The first cell is the type of interrupt, local or shared. + See <include/dt-bindings/interrupt-controller/mips-gic.h>. + - The second cell is the GIC interrupt number. + - The third cell encodes the interrupt flags. + See <include/dt-bindings/interrupt-controller/irq.h> for a list of valid + flags. +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors + to which the GIC may route interrupts. May contain up to 6 entries, one + for each of the CPU's hardware interrupt vectors. Valid values are 2 - 7. + This property is ignored if the CPU is started in EIC mode. + +Required properties for timer sub-node: +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants: + - "mti,interaptiv-gic-timer" +- interrupts : Interrupt for the GIC local timer. +- clock-frequency : Clock frequency at which the GIC timers operate. + +Example: + + gic: interrupt-controller@1bdc0000 { + compatible = "mti,interaptiv-gic"; + reg = <0x1bdc0000 0x20000>; + + interrupt-controller; + #interrupt-cells = <3>; + + mti,available-cpu-vectors = <2>, <3>, <4>, <5>; + + timer { + compatible = "mti,interaptiv-gic-timer"; + interrupts = <GIC_LOCAL 1 IRQ_TYPE_NONE>; + clock-frequency = <50000000>; + }; + }; + + uart@18101400 { + ... + interrupt-parent = <&gic>; + interrupts = <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>; + ... + }; diff --git a/include/dt-bindings/interrupt-controller/mips-gic.h b/include/dt-bindings/interrupt-controller/mips-gic.h new file mode 100644 index 0000000..cf35a57 --- /dev/null +++ b/include/dt-bindings/interrupt-controller/mips-gic.h @@ -0,0 +1,9 @@ +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MIPS_GIC_H +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MIPS_GIC_H + +#include <dt-bindings/interrupt-controller/irq.h> + +#define GIC_SHARED 0 +#define GIC_LOCAL 1 + +#endif
The Global Interrupt Controller (GIC) present on certain MIPS systems can be used to route external interrupts to individual VPEs and CPU interrupt vectors. It also supports a timer and software-generated interrupts. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- Changes from v2: - added third cell to specify local vs. shared - added documentation for timer sub-node - changed compatible string to include CPU version Changes from v1: - moved from mips/ to interrupt-controller/ - removed interrupts and interrupt-parent properties - added available-cpu-vectors property - dropped third cell in interrupt specifier --- .../bindings/interrupt-controller/mips-gic.txt | 55 ++++++++++++++++++++++ .../dt-bindings/interrupt-controller/mips-gic.h | 9 ++++ 2 files changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt create mode 100644 include/dt-bindings/interrupt-controller/mips-gic.h