| Message ID | 1461150237-15580-14-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/04/16 12:03, Jon Hunter wrote: > The Tegra AGIC interrupt controller is compatible with the ARM GIC-400 > interrupt controller. The Tegra AGIC requires two clocks, namely the > "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add > the compatible string and clock information for the AGIC to the GIC > device-tree binding documentation. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > > I am not sure if it will be popular to add Tegra specific clock names > to the GIC DT docs. However, in that case, then possibly the only > alternative is to move the Tegra AGIC driver into its own file and > expose the GIC APIs for it to use. Then we could add our own DT doc > for the Tegra AGIC as well (based upon the ARM GIC). Mark, Rob: any input on this? That would work for me, but I'd like an ack from one of you. Thanks, M. > > Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > index 793c20ff8fcc..6f34267f1438 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > @@ -21,6 +21,7 @@ Main node required properties: > "arm,pl390" > "arm,tc11mp-gic" > "brcm,brahma-b15-gic" > + "nvidia,tegra210-agic" > "qcom,msm-8660-qgic" > "qcom,msm-qgic2" > - interrupt-controller : Identifies the node as an interrupt controller > @@ -70,6 +71,7 @@ Optional > "PERIPHCLK", "PERIPHCLKEN" (for "arm,cortex-a9-gic") > "clk" (for "arm,gic-400") > "gclk" (for "arm,pl390") > + "ape" and "apb2ape" (for "nvidia,tegra,agic") > > - power-domains : A phandle and PM domain specifier as defined by bindings of > the power controller specified by phandle, used when the GIC >
On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote: > The Tegra AGIC interrupt controller is compatible with the ARM GIC-400 > interrupt controller. The cover letter says it _is_ a GIC-400, just used in a slightly unusual manner (i.e. not directly connected to CPUs). > The Tegra AGIC requires two clocks, namely the > "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add > the compatible string and clock information for the AGIC to the GIC > device-tree binding documentation. The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is. There isn't an APB clock described, and the manual seems to show GIC-400 directly connected to AXI rather than APB, so that doesn't seem to even be the usual "apb_pclk". Is there some wrapper logic around a GIC-400 to giove it an APB interface? Or am I misudnerstanding the spec? > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > > I am not sure if it will be popular to add Tegra specific clock names > to the GIC DT docs. However, in that case, then possibly the only > alternative is to move the Tegra AGIC driver into its own file and > expose the GIC APIs for it to use. Then we could add our own DT doc > for the Tegra AGIC as well (based upon the ARM GIC). The clock-names don't seem right to me, as they sound like provide names or global clock line names rather than consumer-side names ("clk" and "apb_pclk"). I'm also not certain about the compatible string; if this really is a GIC-400 then I would at least expect "arm,gic-400" as a fallback. Thanks, Mark. > Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > index 793c20ff8fcc..6f34267f1438 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > @@ -21,6 +21,7 @@ Main node required properties: > "arm,pl390" > "arm,tc11mp-gic" > "brcm,brahma-b15-gic" > + "nvidia,tegra210-agic" > "qcom,msm-8660-qgic" > "qcom,msm-qgic2" > - interrupt-controller : Identifies the node as an interrupt controller > @@ -70,6 +71,7 @@ Optional > "PERIPHCLK", "PERIPHCLKEN" (for "arm,cortex-a9-gic") > "clk" (for "arm,gic-400") > "gclk" (for "arm,pl390") > + "ape" and "apb2ape" (for "nvidia,tegra,agic") > > - power-domains : A phandle and PM domain specifier as defined by bindings of > the power controller specified by phandle, used when the GIC > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/04/16 11:00, Mark Rutland wrote: > On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote: >> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400 >> interrupt controller. > > The cover letter says it _is_ a GIC-400, just used in a slightly unusual > manner (i.e. not directly connected to CPUs). Correct. >> The Tegra AGIC requires two clocks, namely the >> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add >> the compatible string and clock information for the AGIC to the GIC >> device-tree binding documentation. > > The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is. > There isn't an APB clock described, and the manual seems to show GIC-400 > directly connected to AXI rather than APB, so that doesn't seem to even > be the usual "apb_pclk". > > Is there some wrapper logic around a GIC-400 to giove it an APB > interface? Or am I misudnerstanding the spec? Looking at the Tegra documentation what we have is ... APB --> AXI switch --> AGIC (GIC400) I am not sure how such a switch would typically be modeled in DT but we need the apb clock to interface to the GIC registers. I am not sure if something like simple-pm-bus is appropriate here. >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> >> I am not sure if it will be popular to add Tegra specific clock names >> to the GIC DT docs. However, in that case, then possibly the only >> alternative is to move the Tegra AGIC driver into its own file and >> expose the GIC APIs for it to use. Then we could add our own DT doc >> for the Tegra AGIC as well (based upon the ARM GIC). > > The clock-names don't seem right to me, as they sound like provide names > or global clock line names rather than consumer-side names ("clk" and > "apb_pclk"). Yes that would be fine with me. > I'm also not certain about the compatible string; if this really is a > GIC-400 then I would at least expect "arm,gic-400" as a fallback. OK. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 22, 2016 at 12:12:57PM +0100, Jon Hunter wrote: > > On 22/04/16 11:00, Mark Rutland wrote: > > On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote: > >> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400 > >> interrupt controller. > > > > The cover letter says it _is_ a GIC-400, just used in a slightly unusual > > manner (i.e. not directly connected to CPUs). > > Correct. > > >> The Tegra AGIC requires two clocks, namely the > >> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add > >> the compatible string and clock information for the AGIC to the GIC > >> device-tree binding documentation. > > > > The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is. > > There isn't an APB clock described, and the manual seems to show GIC-400 > > directly connected to AXI rather than APB, so that doesn't seem to even > > be the usual "apb_pclk". > > > > Is there some wrapper logic around a GIC-400 to giove it an APB > > interface? Or am I misudnerstanding the spec? > > Looking at the Tegra documentation what we have is ... > > APB --> AXI switch --> AGIC (GIC400) > > I am not sure how such a switch would typically be modeled in DT but we > need the apb clock to interface to the GIC registers. I am not sure if > something like simple-pm-bus is appropriate here. I think we need some representation of that AXI switch in the DT; whether simple-pm-bus is appropriate is another question. We probably need a specific compatible string / binding regardless. Thanks, Mark. > > >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > >> --- > >> > >> I am not sure if it will be popular to add Tegra specific clock names > >> to the GIC DT docs. However, in that case, then possibly the only > >> alternative is to move the Tegra AGIC driver into its own file and > >> expose the GIC APIs for it to use. Then we could add our own DT doc > >> for the Tegra AGIC as well (based upon the ARM GIC). > > > > The clock-names don't seem right to me, as they sound like provide names > > or global clock line names rather than consumer-side names ("clk" and > > "apb_pclk"). > > Yes that would be fine with me. Ok; if we model the apb_pclk as owned by the AXI switch (which it is), then there's no change for the GIC binding, short of the additional compatible string as an extension of "arm,gic-400", as we already model that clock in the GIC-400 binding. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/04/16 12:22, Mark Rutland wrote: > On Fri, Apr 22, 2016 at 12:12:57PM +0100, Jon Hunter wrote: >> >> On 22/04/16 11:00, Mark Rutland wrote: >>> On Wed, Apr 20, 2016 at 12:03:56PM +0100, Jon Hunter wrote: >>>> The Tegra AGIC interrupt controller is compatible with the ARM GIC-400 >>>> interrupt controller. >>> >>> The cover letter says it _is_ a GIC-400, just used in a slightly unusual >>> manner (i.e. not directly connected to CPUs). >> >> Correct. >> >>>> The Tegra AGIC requires two clocks, namely the >>>> "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add >>>> the compatible string and clock information for the AGIC to the GIC >>>> device-tree binding documentation. >>> >>> The GIC-400 spec only describes "CLK" (which is what I imagine "ape" is. >>> There isn't an APB clock described, and the manual seems to show GIC-400 >>> directly connected to AXI rather than APB, so that doesn't seem to even >>> be the usual "apb_pclk". >>> >>> Is there some wrapper logic around a GIC-400 to giove it an APB >>> interface? Or am I misudnerstanding the spec? >> >> Looking at the Tegra documentation what we have is ... >> >> APB --> AXI switch --> AGIC (GIC400) >> >> I am not sure how such a switch would typically be modeled in DT but we >> need the apb clock to interface to the GIC registers. I am not sure if >> something like simple-pm-bus is appropriate here. > > I think we need some representation of that AXI switch in the DT; > whether simple-pm-bus is appropriate is another question. We probably > need a specific compatible string / binding regardless. OK, I will have a look at that. >>> The clock-names don't seem right to me, as they sound like provide names >>> or global clock line names rather than consumer-side names ("clk" and >>> "apb_pclk"). >> >> Yes that would be fine with me. > > Ok; if we model the apb_pclk as owned by the AXI switch (which it is), > then there's no change for the GIC binding, short of the additional > compatible string as an extension of "arm,gic-400", as we already model > that clock in the GIC-400 binding. Yes that makes sense. Thanks Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/04/16 12:22, Mark Rutland wrote: [snip] >>>> I am not sure if it will be popular to add Tegra specific clock names >>>> to the GIC DT docs. However, in that case, then possibly the only >>>> alternative is to move the Tegra AGIC driver into its own file and >>>> expose the GIC APIs for it to use. Then we could add our own DT doc >>>> for the Tegra AGIC as well (based upon the ARM GIC). >>> >>> The clock-names don't seem right to me, as they sound like provide names >>> or global clock line names rather than consumer-side names ("clk" and >>> "apb_pclk"). >> >> Yes that would be fine with me. > > Ok; if we model the apb_pclk as owned by the AXI switch (which it is), > then there's no change for the GIC binding, short of the additional > compatible string as an extension of "arm,gic-400", as we already model > that clock in the GIC-400 binding. I have been re-working this based upon the feedback received. In the GIC driver we have the following definitions ... IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init); If I have something like the following in my dts ... agic: interrupt-controller@702f9000 { compatible = "nvidia,tegra210-agic", "arm,gic-400"; ... }; The problem with this is that it tries to register the interrupt controller early during of_irq_init() before the platform driver has chance to initialise it. To avoid this I got rid of the "nvidia,tegra210-agic" string and added the following for the platform driver ... static const struct of_device_id gic_match[] = { { .compatible = "arm,arm11mp-gic-pm", .data = &arm11mp_gic_data }, { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data }, { .compatible = "arm,cortex-a9-gic-pm", .data = &cortexa9_gic_data }, { .compatible = "arm,gic400-pm", .data = &gic400_data }, { .compatible = "arm,pl390-pm", .data = &pl390_data }, {}, }; It is not ideal as now we have a *-pm variant of each compatible string :-( Another option would be to add some code in gic_of_init() to check for the presence of a "clocks" node in the DT binding and bail out of the early initialisation if found but may be that is a bit of a hack. Mark, what are your thoughts on this? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote: > > On 22/04/16 12:22, Mark Rutland wrote: > > [snip] > > >>>> I am not sure if it will be popular to add Tegra specific clock names > >>>> to the GIC DT docs. However, in that case, then possibly the only > >>>> alternative is to move the Tegra AGIC driver into its own file and > >>>> expose the GIC APIs for it to use. Then we could add our own DT doc > >>>> for the Tegra AGIC as well (based upon the ARM GIC). > >>> > >>> The clock-names don't seem right to me, as they sound like provide names > >>> or global clock line names rather than consumer-side names ("clk" and > >>> "apb_pclk"). > >> > >> Yes that would be fine with me. > > > > Ok; if we model the apb_pclk as owned by the AXI switch (which it is), > > then there's no change for the GIC binding, short of the additional > > compatible string as an extension of "arm,gic-400", as we already model > > that clock in the GIC-400 binding. > > I have been re-working this based upon the feedback received. In the GIC > driver we have the following definitions ... > > IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); > IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); > IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); > IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); > IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); > IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); > IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); > IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); > IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init); > > > If I have something like the following in my dts ... > > agic: interrupt-controller@702f9000 { > compatible = "nvidia,tegra210-agic", "arm,gic-400"; > ... > }; > > The problem with this is that it tries to register the interrupt controller > early during of_irq_init() before the platform driver has chance to > initialise it. Probe order strikes again... > To avoid this I got rid of the "nvidia,tegra210-agic" string and added > the following for the platform driver ... > > static const struct of_device_id gic_match[] = { > { .compatible = "arm,arm11mp-gic-pm", .data = &arm11mp_gic_data }, > { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data }, > { .compatible = "arm,cortex-a9-gic-pm", .data = &cortexa9_gic_data }, > { .compatible = "arm,gic400-pm", .data = &gic400_data }, > { .compatible = "arm,pl390-pm", .data = &pl390_data }, > {}, > }; > > It is not ideal as now we have a *-pm variant of each compatible string :-( Yeah, that's a non-starter. :( > Another option would be to add some code in gic_of_init() to check for the > presence of a "clocks" node in the DT binding and bail out of the early > initialisation if found but may be that is a bit of a hack. I fear that someone may validly have a clocks property in their root GIC node, at which point things would fall apart. I was under the impression this was the case for some Renesas boards (though I didn't find an example in tree). So I suspect that using the clocks property in that way isn't going to work out well. > Mark, what are your thoughts on this? Collectively: "aargh", "oh no". We could instead explicitly match "nvidia,tegra210-agic", bailing out if we see that. Otherwise, if we can't handle it like a GIC-400, then we can just drop the GIC-400 compatible string from the fallback list. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 27, 2016 at 7:38 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote: >> On 22/04/16 12:22, Mark Rutland wrote: >> [snip] >> >> >>>> I am not sure if it will be popular to add Tegra specific clock names >> >>>> to the GIC DT docs. However, in that case, then possibly the only >> >>>> alternative is to move the Tegra AGIC driver into its own file and >> >>>> expose the GIC APIs for it to use. Then we could add our own DT doc >> >>>> for the Tegra AGIC as well (based upon the ARM GIC). >> >>> >> >>> The clock-names don't seem right to me, as they sound like provide names >> >>> or global clock line names rather than consumer-side names ("clk" and >> >>> "apb_pclk"). >> >> >> >> Yes that would be fine with me. >> > >> > Ok; if we model the apb_pclk as owned by the AXI switch (which it is), >> > then there's no change for the GIC binding, short of the additional >> > compatible string as an extension of "arm,gic-400", as we already model >> > that clock in the GIC-400 binding. >> >> I have been re-working this based upon the feedback received. In the GIC >> driver we have the following definitions ... >> >> IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); >> IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); >> IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); >> IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); >> IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); >> IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); >> IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >> IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init); >> >> >> If I have something like the following in my dts ... >> >> agic: interrupt-controller@702f9000 { >> compatible = "nvidia,tegra210-agic", "arm,gic-400"; >> ... >> }; >> >> The problem with this is that it tries to register the interrupt controller >> early during of_irq_init() before the platform driver has chance to >> initialise it. > > Probe order strikes again... > >> To avoid this I got rid of the "nvidia,tegra210-agic" string and added >> the following for the platform driver ... >> >> static const struct of_device_id gic_match[] = { >> { .compatible = "arm,arm11mp-gic-pm", .data = &arm11mp_gic_data }, >> { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data }, >> { .compatible = "arm,cortex-a9-gic-pm", .data = &cortexa9_gic_data }, >> { .compatible = "arm,gic400-pm", .data = &gic400_data }, >> { .compatible = "arm,pl390-pm", .data = &pl390_data }, >> {}, >> }; >> >> It is not ideal as now we have a *-pm variant of each compatible string :-( > > Yeah, that's a non-starter. :( > >> Another option would be to add some code in gic_of_init() to check for the >> presence of a "clocks" node in the DT binding and bail out of the early >> initialisation if found but may be that is a bit of a hack. Or the presence of a power-domains property... > I fear that someone may validly have a clocks property in their root GIC > node, at which point things would fall apart. I was under the impression > this was the case for some Renesas boards (though I didn't find an > example in tree). We don't have the GIC clocks in the GIC nodes yet, as there's no suitable mechanism (e.g. CLK_ENABLE_HAND_OFF) in upstream yet to prevent them from being disabled ("unused" clocks are disabled). 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/04/16 18:38, Mark Rutland wrote: > On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote: >> >> On 22/04/16 12:22, Mark Rutland wrote: >> >> [snip] >> >>>>>> I am not sure if it will be popular to add Tegra specific clock names >>>>>> to the GIC DT docs. However, in that case, then possibly the only >>>>>> alternative is to move the Tegra AGIC driver into its own file and >>>>>> expose the GIC APIs for it to use. Then we could add our own DT doc >>>>>> for the Tegra AGIC as well (based upon the ARM GIC). >>>>> >>>>> The clock-names don't seem right to me, as they sound like provide names >>>>> or global clock line names rather than consumer-side names ("clk" and >>>>> "apb_pclk"). >>>> >>>> Yes that would be fine with me. >>> >>> Ok; if we model the apb_pclk as owned by the AXI switch (which it is), >>> then there's no change for the GIC binding, short of the additional >>> compatible string as an extension of "arm,gic-400", as we already model >>> that clock in the GIC-400 binding. >> >> I have been re-working this based upon the feedback received. In the GIC >> driver we have the following definitions ... >> >> IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); >> IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); >> IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); >> IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); >> IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); >> IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); >> IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >> IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init); >> >> >> If I have something like the following in my dts ... >> >> agic: interrupt-controller@702f9000 { >> compatible = "nvidia,tegra210-agic", "arm,gic-400"; >> ... >> }; >> >> The problem with this is that it tries to register the interrupt controller >> early during of_irq_init() before the platform driver has chance to >> initialise it. > > Probe order strikes again... > >> To avoid this I got rid of the "nvidia,tegra210-agic" string and added >> the following for the platform driver ... >> >> static const struct of_device_id gic_match[] = { >> { .compatible = "arm,arm11mp-gic-pm", .data = &arm11mp_gic_data }, >> { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data }, >> { .compatible = "arm,cortex-a9-gic-pm", .data = &cortexa9_gic_data }, >> { .compatible = "arm,gic400-pm", .data = &gic400_data }, >> { .compatible = "arm,pl390-pm", .data = &pl390_data }, >> {}, >> }; >> >> It is not ideal as now we have a *-pm variant of each compatible string :-( > > Yeah, that's a non-starter. :( That is what I feared. Understood. >> Another option would be to add some code in gic_of_init() to check for the >> presence of a "clocks" node in the DT binding and bail out of the early >> initialisation if found but may be that is a bit of a hack. > > I fear that someone may validly have a clocks property in their root GIC > node, at which point things would fall apart. I was under the impression > this was the case for some Renesas boards (though I didn't find an > example in tree). > > So I suspect that using the clocks property in that way isn't going to > work out well. > >> Mark, what are your thoughts on this? > > Collectively: "aargh", "oh no". Yes, exactly :-( > We could instead explicitly match "nvidia,tegra210-agic", bailing out if > we see that. Otherwise, if we can't handle it like a GIC-400, then we > can just drop the GIC-400 compatible string from the fallback list. Would it also be a none-starter to have "arm,gic-pm" instead of "nvidia,tegra210-agic"? At this point it is not really specific to tegra any more and so I was hoping to get rid of that. For example, ... compatible = "arm,gic-pm", "arm,gic-400"; Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jon, On Thu, Apr 28, 2016 at 10:11 AM, Jon Hunter <jonathanh@nvidia.com> wrote: > On 27/04/16 18:38, Mark Rutland wrote: >> On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote: >>> On 22/04/16 12:22, Mark Rutland wrote: >>> [snip] >>>>>>> I am not sure if it will be popular to add Tegra specific clock names >>>>>>> to the GIC DT docs. However, in that case, then possibly the only >>>>>>> alternative is to move the Tegra AGIC driver into its own file and >>>>>>> expose the GIC APIs for it to use. Then we could add our own DT doc >>>>>>> for the Tegra AGIC as well (based upon the ARM GIC). >>>>>> >>>>>> The clock-names don't seem right to me, as they sound like provide names >>>>>> or global clock line names rather than consumer-side names ("clk" and >>>>>> "apb_pclk"). >>>>> >>>>> Yes that would be fine with me. >>>> >>>> Ok; if we model the apb_pclk as owned by the AXI switch (which it is), >>>> then there's no change for the GIC binding, short of the additional >>>> compatible string as an extension of "arm,gic-400", as we already model >>>> that clock in the GIC-400 binding. >>> >>> I have been re-working this based upon the feedback received. In the GIC >>> driver we have the following definitions ... >>> >>> IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); >>> IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); >>> IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); >>> IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); >>> IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); >>> IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); >>> IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>> IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init); >>> >>> >>> If I have something like the following in my dts ... >>> >>> agic: interrupt-controller@702f9000 { >>> compatible = "nvidia,tegra210-agic", "arm,gic-400"; >>> ... >>> }; >>> >>> The problem with this is that it tries to register the interrupt controller >>> early during of_irq_init() before the platform driver has chance to >>> initialise it. >> >> Probe order strikes again... >> >>> To avoid this I got rid of the "nvidia,tegra210-agic" string and added >>> the following for the platform driver ... >>> >>> static const struct of_device_id gic_match[] = { >>> { .compatible = "arm,arm11mp-gic-pm", .data = &arm11mp_gic_data }, >>> { .compatible = "arm,cortex-a15-gic-pm", .data = &cortexa15_gic_data }, >>> { .compatible = "arm,cortex-a9-gic-pm", .data = &cortexa9_gic_data }, >>> { .compatible = "arm,gic400-pm", .data = &gic400_data }, >>> { .compatible = "arm,pl390-pm", .data = &pl390_data }, >>> {}, >>> }; >>> >>> It is not ideal as now we have a *-pm variant of each compatible string :-( >> >> Yeah, that's a non-starter. :( > > That is what I feared. Understood. > >>> Another option would be to add some code in gic_of_init() to check for the >>> presence of a "clocks" node in the DT binding and bail out of the early >>> initialisation if found but may be that is a bit of a hack. >> >> I fear that someone may validly have a clocks property in their root GIC >> node, at which point things would fall apart. I was under the impression >> this was the case for some Renesas boards (though I didn't find an >> example in tree). >> >> So I suspect that using the clocks property in that way isn't going to >> work out well. >> >>> Mark, what are your thoughts on this? >> >> Collectively: "aargh", "oh no". > > Yes, exactly :-( > >> We could instead explicitly match "nvidia,tegra210-agic", bailing out if >> we see that. Otherwise, if we can't handle it like a GIC-400, then we >> can just drop the GIC-400 compatible string from the fallback list. > > Would it also be a none-starter to have "arm,gic-pm" instead of > "nvidia,tegra210-agic"? At this point it is not really specific to tegra > any more and so I was hoping to get rid of that. For example, ... > > compatible = "arm,gic-pm", "arm,gic-400"; The "-pm" is not a property of the GIC, but of the SoC. So IMHO the compatible value should be plain "arm,gic-400". If a device node has "clocks", "interrupts", "power-domains"[*], ... properties, and the corresponding providers are not yet available, a driver typically returns -EPROBE_DEFER, and will be retried later by the driver core. [*] For "power-domains" this is handled by the device core. I.e. .probe() won't even be called before the dependency has been fulfilled. With IRQCHIP_DECLARE(), you don't have the retrying, and probe order (w.r.t. to other subsystems) is fixed. But as you said, gic_of_init() could just bail out if it "clocks" and/or "power-domains" properties are present, but their providers aren't. Later, the remaining GICs can be initialized from the platform driver. You just have to make sure no GIC is initialized twice (I believe that's what was plaguing me last time I tried your series). That's probably the closest you can get to normal platform_driver behavior, without converting the whole GIC driver to a normal platform_driver, which may cause problems on platforms that are currently working fine. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 09:11:03AM +0100, Jon Hunter wrote: > > On 27/04/16 18:38, Mark Rutland wrote: > > On Wed, Apr 27, 2016 at 04:34:53PM +0100, Jon Hunter wrote: > >> > >> On 22/04/16 12:22, Mark Rutland wrote: > >> > >> [snip] > >> > >>>>>> I am not sure if it will be popular to add Tegra specific clock names > >>>>>> to the GIC DT docs. However, in that case, then possibly the only > >>>>>> alternative is to move the Tegra AGIC driver into its own file and > >>>>>> expose the GIC APIs for it to use. Then we could add our own DT doc > >>>>>> for the Tegra AGIC as well (based upon the ARM GIC). > >>>>> > >>>>> The clock-names don't seem right to me, as they sound like provide names > >>>>> or global clock line names rather than consumer-side names ("clk" and > >>>>> "apb_pclk"). > >>>> > >>>> Yes that would be fine with me. > >>> > >>> Ok; if we model the apb_pclk as owned by the AXI switch (which it is), > >>> then there's no change for the GIC binding, short of the additional > >>> compatible string as an extension of "arm,gic-400", as we already model > >>> that clock in the GIC-400 binding. > >> > >> I have been re-working this based upon the feedback received. In the GIC > >> driver we have the following definitions ... > >> > >> IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); > >> IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); > >> IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); > >> IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); > >> IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); > >> IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); > >> IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); > >> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); > >> IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init); > >> > >> > >> If I have something like the following in my dts ... > >> > >> agic: interrupt-controller@702f9000 { > >> compatible = "nvidia,tegra210-agic", "arm,gic-400"; > >> ... > >> }; > >> > >> The problem with this is that it tries to register the interrupt controller > >> early during of_irq_init() before the platform driver has chance to > >> initialise it. [...] > > We could instead explicitly match "nvidia,tegra210-agic", bailing out if > > we see that. Otherwise, if we can't handle it like a GIC-400, then we > > can just drop the GIC-400 compatible string from the fallback list. > > Would it also be a none-starter to have "arm,gic-pm" instead of > "nvidia,tegra210-agic"? At this point it is not really specific to tegra > any more and so I was hoping to get rid of that. For example, ... > > compatible = "arm,gic-pm", "arm,gic-400"; I'm not keen on the "*-pm" approach, as such compatible strings aren't reall describing HW, but rather the SW policy to apply, and really would only be there to bodge around a structural issue we have in Linux today w.r.t. the device model split and probe ordering. The "nvidia,tegra210-agic" string can be taken as describing any Tegra-210 specific integration quirks, though I agree that's also not fantastic for extending PM support beyond Tegra 210 and variants thereof. So maybe the best approach is bailing out in the presence of clocks and/or power domains after all, on the assumption that nothing today has those properties, though I fear we may have problems with that later down the line if/when people describe those for the root GIC to describe those must be hogged, even if not explicitly managed. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt index 793c20ff8fcc..6f34267f1438 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt @@ -21,6 +21,7 @@ Main node required properties: "arm,pl390" "arm,tc11mp-gic" "brcm,brahma-b15-gic" + "nvidia,tegra210-agic" "qcom,msm-8660-qgic" "qcom,msm-qgic2" - interrupt-controller : Identifies the node as an interrupt controller @@ -70,6 +71,7 @@ Optional "PERIPHCLK", "PERIPHCLKEN" (for "arm,cortex-a9-gic") "clk" (for "arm,gic-400") "gclk" (for "arm,pl390") + "ape" and "apb2ape" (for "nvidia,tegra,agic") - power-domains : A phandle and PM domain specifier as defined by bindings of the power controller specified by phandle, used when the GIC
The Tegra AGIC interrupt controller is compatible with the ARM GIC-400 interrupt controller. The Tegra AGIC requires two clocks, namely the "ape" (functional) and "apb2ape" (interface) clocks, to operate. Add the compatible string and clock information for the AGIC to the GIC device-tree binding documentation. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- I am not sure if it will be popular to add Tegra specific clock names to the GIC DT docs. However, in that case, then possibly the only alternative is to move the Tegra AGIC driver into its own file and expose the GIC APIs for it to use. Then we could add our own DT doc for the Tegra AGIC as well (based upon the ARM GIC). Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt | 2 ++ 1 file changed, 2 insertions(+)