| Message ID | 572C56A6.7020408@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jon, On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >> 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. > > On further testing, by bailing out in the presence of clocks and/or > power-domains, the problem I now see is that although the primary gic-400 > has been registered, we still try to probe it again later as it matches > the platform driver. One way to avoid this would be ... > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c > index e7bfc175b8e1..631da7ad0dbf 100644 > --- a/drivers/of/irq.c > +++ b/drivers/of/irq.c > @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) > * its children can get processed in a subsequent pass. > */ > list_add_tail(&desc->list, &intc_parent_list); > + > + of_node_set_flag(desc->dev, OF_POPULATED); > } That sounds like the right thing to do to me... > If this is not appropriate then I guess I will just need to use > "tegra210-agic" for the compatibility flag. As I want this for plain gic-400, I'd be unhappy ;-) 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
Hi Geert, On 07/05/16 15:10, Geert Uytterhoeven wrote: > Hi Jon, > > On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>> 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. >> >> On further testing, by bailing out in the presence of clocks and/or >> power-domains, the problem I now see is that although the primary gic-400 >> has been registered, we still try to probe it again later as it matches >> the platform driver. One way to avoid this would be ... >> >> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >> index e7bfc175b8e1..631da7ad0dbf 100644 >> --- a/drivers/of/irq.c >> +++ b/drivers/of/irq.c >> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >> * its children can get processed in a subsequent pass. >> */ >> list_add_tail(&desc->list, &intc_parent_list); >> + >> + of_node_set_flag(desc->dev, OF_POPULATED); >> } > > That sounds like the right thing to do to me... OK. The more I think about this, it does seem silly to create a device and pdata for a device that has already been instantiated. >> If this is not appropriate then I guess I will just need to use >> "tegra210-agic" for the compatibility flag. > > As I want this for plain gic-400, I'd be unhappy ;-) No problem. However, there is more work that would be needed to get this to work for root controllers which I think that you want. 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 08/05/16 13:25, Jon Hunter wrote: > Hi Geert, > > On 07/05/16 15:10, Geert Uytterhoeven wrote: >> Hi Jon, >> >> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>>> 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. >>> >>> On further testing, by bailing out in the presence of clocks and/or >>> power-domains, the problem I now see is that although the primary gic-400 >>> has been registered, we still try to probe it again later as it matches >>> the platform driver. One way to avoid this would be ... >>> >>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >>> index e7bfc175b8e1..631da7ad0dbf 100644 >>> --- a/drivers/of/irq.c >>> +++ b/drivers/of/irq.c >>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >>> * its children can get processed in a subsequent pass. >>> */ >>> list_add_tail(&desc->list, &intc_parent_list); >>> + >>> + of_node_set_flag(desc->dev, OF_POPULATED); >>> } >> >> That sounds like the right thing to do to me... > > OK. The more I think about this, it does seem silly to create a device > and pdata for a device that has already been instantiated. > >>> If this is not appropriate then I guess I will just need to use >>> "tegra210-agic" for the compatibility flag. >> >> As I want this for plain gic-400, I'd be unhappy ;-) > > No problem. However, there is more work that would be needed to get this > to work for root controllers which I think that you want. All this brings the discussion back to the root of the problem: irqchips (and timers) are not first class devices, because we need them too early for that. I'd really like to solve this, but the kernel init is incredibly complicated, and the subsystem dependencies completely undocumented. It looks like we need the timer early because the we fork a thread for PID-1, and the scheduler is going to need some form of tick. So ideally, we'd be able to move the irq/timer stuff *after* the device framework (which itself requires devtmpfs to be up and running, hence dragging the whole VM and VFS), but before the scheduler is initialized. I'm sure there is plenty of other dependencies I haven't worked out yet. If anyone has some spare time and willing to help, please speak now! ;-) Thanks, M.
On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Jon, > > On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>> 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. >> >> On further testing, by bailing out in the presence of clocks and/or >> power-domains, the problem I now see is that although the primary gic-400 >> has been registered, we still try to probe it again later as it matches >> the platform driver. One way to avoid this would be ... >> >> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >> index e7bfc175b8e1..631da7ad0dbf 100644 >> --- a/drivers/of/irq.c >> +++ b/drivers/of/irq.c >> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >> * its children can get processed in a subsequent pass. >> */ >> list_add_tail(&desc->list, &intc_parent_list); >> + >> + of_node_set_flag(desc->dev, OF_POPULATED); >> } > > That sounds like the right thing to do to me... Seems fine to me, but it would be a problem since this is a global decision if you wanted to have some hand-off from an "early driver" to a platform driver. I guess setting the flag could move to drivers that need it although I don't think drivers should be touching the flags. >> If this is not appropriate then I guess I will just need to use >> "tegra210-agic" for the compatibility flag. > > As I want this for plain gic-400, I'd be unhappy ;-) IMO, the plain gic-400 should not have these dependencies and you should use SoC specific compatible strings should you need to deal with this problem. Rob -- 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 Rob, On 11/05/16 16:51, Rob Herring wrote: > On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> Hi Jon, >> >> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>>> 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. >>> >>> On further testing, by bailing out in the presence of clocks and/or >>> power-domains, the problem I now see is that although the primary gic-400 >>> has been registered, we still try to probe it again later as it matches >>> the platform driver. One way to avoid this would be ... >>> >>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >>> index e7bfc175b8e1..631da7ad0dbf 100644 >>> --- a/drivers/of/irq.c >>> +++ b/drivers/of/irq.c >>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >>> * its children can get processed in a subsequent pass. >>> */ >>> list_add_tail(&desc->list, &intc_parent_list); >>> + >>> + of_node_set_flag(desc->dev, OF_POPULATED); >>> } >> >> That sounds like the right thing to do to me... > > Seems fine to me, but it would be a problem since this is a global > decision if you wanted to have some hand-off from an "early driver" to > a platform driver. I guess setting the flag could move to drivers that > need it although I don't think drivers should be touching the flags. Isn't this the other way around? Setting this flag means that I have been populated and so don't bother creating a platform device for this device as it isn't needed. A by-product if this, is that if we did happen to have a platform driver for the irqchip that also has an early driver, then the hand-off would never happen if the early init was successful. The driver would still have to decide whether to hand-off and to do that it would need to return an error from the early driver [0]. >>> If this is not appropriate then I guess I will just need to use >>> "tegra210-agic" for the compatibility flag. >> >> As I want this for plain gic-400, I'd be unhappy ;-) > > IMO, the plain gic-400 should not have these dependencies and you > should use SoC specific compatible strings should you need to deal > with this problem. It is fine for my case, but it does mean I cannot say ... compatible = "tegra210-agic", "gic-400"; ... because this will always match the early driver (unless we do something like I have suggested above). So I would have ... compatible = "tegra210-agic"; Cheers Jon [0] http://marc.info/?l=devicetree&m=146237938725709&w=2 -- 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 11/05/16 17:08, Jon Hunter wrote: > Hi Rob, > > On 11/05/16 16:51, Rob Herring wrote: >> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> Hi Jon, >>> >>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>>>> 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. >>>> >>>> On further testing, by bailing out in the presence of clocks and/or >>>> power-domains, the problem I now see is that although the primary gic-400 >>>> has been registered, we still try to probe it again later as it matches >>>> the platform driver. One way to avoid this would be ... >>>> >>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >>>> index e7bfc175b8e1..631da7ad0dbf 100644 >>>> --- a/drivers/of/irq.c >>>> +++ b/drivers/of/irq.c >>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >>>> * its children can get processed in a subsequent pass. >>>> */ >>>> list_add_tail(&desc->list, &intc_parent_list); >>>> + >>>> + of_node_set_flag(desc->dev, OF_POPULATED); >>>> } >>> >>> That sounds like the right thing to do to me... >> >> Seems fine to me, but it would be a problem since this is a global >> decision if you wanted to have some hand-off from an "early driver" to >> a platform driver. I guess setting the flag could move to drivers that >> need it although I don't think drivers should be touching the flags. > > Isn't this the other way around? Setting this flag means that I have > been populated and so don't bother creating a platform device for this > device as it isn't needed. A by-product if this, is that if we did > happen to have a platform driver for the irqchip that also has an early > driver, then the hand-off would never happen if the early init was > successful. > > The driver would still have to decide whether to hand-off and to do that > it would need to return an error from the early driver [0]. I mean, the "early driver would still have to decide whether to hand-off ..." 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 11/05/16 17:08, Jon Hunter wrote: > Hi Rob, > > On 11/05/16 16:51, Rob Herring wrote: >> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> Hi Jon, >>> >>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>>>> 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. >>>> >>>> On further testing, by bailing out in the presence of clocks and/or >>>> power-domains, the problem I now see is that although the primary gic-400 >>>> has been registered, we still try to probe it again later as it matches >>>> the platform driver. One way to avoid this would be ... >>>> >>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >>>> index e7bfc175b8e1..631da7ad0dbf 100644 >>>> --- a/drivers/of/irq.c >>>> +++ b/drivers/of/irq.c >>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >>>> * its children can get processed in a subsequent pass. >>>> */ >>>> list_add_tail(&desc->list, &intc_parent_list); >>>> + >>>> + of_node_set_flag(desc->dev, OF_POPULATED); >>>> } >>> >>> That sounds like the right thing to do to me... >> >> Seems fine to me, but it would be a problem since this is a global >> decision if you wanted to have some hand-off from an "early driver" to >> a platform driver. I guess setting the flag could move to drivers that >> need it although I don't think drivers should be touching the flags. > > Isn't this the other way around? Setting this flag means that I have > been populated and so don't bother creating a platform device for this > device as it isn't needed. A by-product if this, is that if we did > happen to have a platform driver for the irqchip that also has an early > driver, then the hand-off would never happen if the early init was > successful. > > The driver would still have to decide whether to hand-off and to do that > it would need to return an error from the early driver [0]. > >>>> If this is not appropriate then I guess I will just need to use >>>> "tegra210-agic" for the compatibility flag. >>> >>> As I want this for plain gic-400, I'd be unhappy ;-) >> >> IMO, the plain gic-400 should not have these dependencies and you >> should use SoC specific compatible strings should you need to deal >> with this problem. > > It is fine for my case, but it does mean I cannot say ... > > compatible = "tegra210-agic", "gic-400"; > > ... because this will always match the early driver (unless we do > something like I have suggested above). So I would have ... Sorry this is wrong. The above will always match the early driver. The problem with the above compatibility string is that, if the platform driver matches "gic-400" then it will try to probe all gic-400s even if they have been initialised early and this is definitely not what we want. This could be solved by setting the OF_POPULATED flag. 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, May 11, 2016 at 11:16 AM, Jon Hunter <jonathanh@nvidia.com> wrote: > > On 11/05/16 17:08, Jon Hunter wrote: >> Hi Rob, >> >> On 11/05/16 16:51, Rob Herring wrote: >>> On Sat, May 7, 2016 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>> Hi Jon, >>>> >>>> On Fri, May 6, 2016 at 10:32 AM, Jon Hunter <jonathanh@nvidia.com> wrote: >>>>>> 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. >>>>> >>>>> On further testing, by bailing out in the presence of clocks and/or >>>>> power-domains, the problem I now see is that although the primary gic-400 >>>>> has been registered, we still try to probe it again later as it matches >>>>> the platform driver. One way to avoid this would be ... >>>>> >>>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c >>>>> index e7bfc175b8e1..631da7ad0dbf 100644 >>>>> --- a/drivers/of/irq.c >>>>> +++ b/drivers/of/irq.c >>>>> @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) >>>>> * its children can get processed in a subsequent pass. >>>>> */ >>>>> list_add_tail(&desc->list, &intc_parent_list); >>>>> + >>>>> + of_node_set_flag(desc->dev, OF_POPULATED); >>>>> } >>>> >>>> That sounds like the right thing to do to me... >>> >>> Seems fine to me, but it would be a problem since this is a global >>> decision if you wanted to have some hand-off from an "early driver" to >>> a platform driver. I guess setting the flag could move to drivers that >>> need it although I don't think drivers should be touching the flags. >> >> Isn't this the other way around? Setting this flag means that I have >> been populated and so don't bother creating a platform device for this >> device as it isn't needed. A by-product if this, is that if we did >> happen to have a platform driver for the irqchip that also has an early >> driver, then the hand-off would never happen if the early init was >> successful. >> >> The driver would still have to decide whether to hand-off and to do that >> it would need to return an error from the early driver [0]. >> >>>>> If this is not appropriate then I guess I will just need to use >>>>> "tegra210-agic" for the compatibility flag. >>>> >>>> As I want this for plain gic-400, I'd be unhappy ;-) >>> >>> IMO, the plain gic-400 should not have these dependencies and you >>> should use SoC specific compatible strings should you need to deal >>> with this problem. >> >> It is fine for my case, but it does mean I cannot say ... >> >> compatible = "tegra210-agic", "gic-400"; >> >> ... because this will always match the early driver (unless we do >> something like I have suggested above). So I would have ... > > Sorry this is wrong. The above will always match the early driver. > > The problem with the above compatibility string is that, if the platform > driver matches "gic-400" then it will try to probe all gic-400s even if > they have been initialised early and this is definitely not what we > want. This could be solved by setting the OF_POPULATED flag. A platform driver for just gic-400 is wrong IMO until we have platform drivers for all interrupt controllers. Another reason to set OF_POPULATED flag is we are needlessly creating platform devices for irq controllers that will never have platform drivers. So I'd go with that approach. Rob -- 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 Rob, Mark, On 11/05/16 17:30, Rob Herring wrote: > A platform driver for just gic-400 is wrong IMO until we have platform > drivers for all interrupt controllers. Yes, that is fine with me, but can we decide on whether the platform driver should match "tegra210-agic" or the early driver should bail-out if clocks/power-domains are present? I am fine with either, but I think that Rob prefers the tegra210-agic compat string and Mark prefers to bail-out of the early driver if clocks/power-domains are present. > Another reason to set OF_POPULATED flag is we are needlessly creating > platform devices for irq controllers that will never have platform > drivers. So I'd go with that approach. Yes exactly, that was the point I was trying to make ;-) 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, May 11, 2016 at 05:53:43PM +0100, Jon Hunter wrote: > Hi Rob, Mark, > > On 11/05/16 17:30, Rob Herring wrote: > > A platform driver for just gic-400 is wrong IMO until we have platform > > drivers for all interrupt controllers. > > Yes, that is fine with me, but can we decide on whether the platform > driver should match "tegra210-agic" or the early driver should bail-out > if clocks/power-domains are present? > > I am fine with either, but I think that Rob prefers the tegra210-agic > compat string and Mark prefers to bail-out of the early driver if > clocks/power-domains are present. If anything, I wasn't too keen on bailing out becuase of those properties, as I mentioned for the case of a root interrupt controller. I am happy to match the "tegra210-agic" string specifically. 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 11/05/16 18:28, Mark Rutland wrote: > On Wed, May 11, 2016 at 05:53:43PM +0100, Jon Hunter wrote: >> Hi Rob, Mark, >> >> On 11/05/16 17:30, Rob Herring wrote: >>> A platform driver for just gic-400 is wrong IMO until we have platform >>> drivers for all interrupt controllers. >> >> Yes, that is fine with me, but can we decide on whether the platform >> driver should match "tegra210-agic" or the early driver should bail-out >> if clocks/power-domains are present? >> >> I am fine with either, but I think that Rob prefers the tegra210-agic >> compat string and Mark prefers to bail-out of the early driver if >> clocks/power-domains are present. > > If anything, I wasn't too keen on bailing out becuase of those > properties, as I mentioned for the case of a root interrupt controller. > > I am happy to match the "tegra210-agic" string specifically. Thanks guys. I will stick with tegra210-agic for now. 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
diff --git a/drivers/of/irq.c b/drivers/of/irq.c index e7bfc175b8e1..631da7ad0dbf 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -556,6 +556,8 @@ void __init of_irq_init(const struct of_device_id *matches) * its children can get processed in a subsequent pass. */ list_add_tail(&desc->list, &intc_parent_list); + + of_node_set_flag(desc->dev, OF_POPULATED); } If this is not appropriate then I guess I will just need to use