| Message ID | 1466090128-24425-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thursday, June 16, 2016 4:15:28 PM CEST Sudeep Holla wrote: > The Linux AMBA bus framework probes the peripheral IDs when adding the > AMBA devices very early on the boot. Generally they are on APB bus and > just require APB clocks to be on even when most of the core logic of the > IP is powered down. > > However on Juno, the entire debugsys domain needs to be ON to access > even the coresight components' CID/PID registers and hence broken by > design. Accessing those while debugsys power domain is off will lead to > the bridge stalling the transactions instead of returning the slave error. > > Further, the AMBA framework can't deal with !CONFIG_PM case: it ignores > the error and proceeds to access the device region. It was suggested to > always enable CONFIG_PM in order to handle this scenario. > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/arm64/Kconfig.platforms | 1 + > 1 file changed, 1 insertion(+) > > Hi ARM-SoC team, > > The discussion on this happened on linux-pm list[1]. This is need on > Juno once we introduce coresight components in the DT. With !CONFIG_PM, > the board stalls on boot and hence this patch is needed. This shouldn't > change any thing in the defconfig as couple of other platforms already > do the same. It's needed in case all other ARCH_* configs are disabled. > > Without this, we need a dirty trick in the DT[2] to handle !CONFIG_PM. > > Can you please pick this for v4.8 ? Question: if AMBA cannot deal with CONFIG_PM disabled, should the 'select' be done from CONFIG_ARM_AMBA instead? I also feel a little uncomfortable with just adding 'select PM', given that this option is normally just implied by CONFIG_SUSPEND || CONFIG_HIBERNATE_CALLBACKS, but not selected individually, so we might run into regressions when we get configurations that were invalid before. Then again ARCH_RENESAS seems to already do this. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17/06/16 12:53, Arnd Bergmann wrote: > On Thursday, June 16, 2016 4:15:28 PM CEST Sudeep Holla wrote: >> The Linux AMBA bus framework probes the peripheral IDs when adding the >> AMBA devices very early on the boot. Generally they are on APB bus and >> just require APB clocks to be on even when most of the core logic of the >> IP is powered down. >> >> However on Juno, the entire debugsys domain needs to be ON to access >> even the coresight components' CID/PID registers and hence broken by >> design. Accessing those while debugsys power domain is off will lead to >> the bridge stalling the transactions instead of returning the slave error. >> >> Further, the AMBA framework can't deal with !CONFIG_PM case: it ignores >> the error and proceeds to access the device region. It was suggested to >> always enable CONFIG_PM in order to handle this scenario. >> >> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> arch/arm64/Kconfig.platforms | 1 + >> 1 file changed, 1 insertion(+) >> >> Hi ARM-SoC team, >> >> The discussion on this happened on linux-pm list[1]. This is need on >> Juno once we introduce coresight components in the DT. With !CONFIG_PM, >> the board stalls on boot and hence this patch is needed. This shouldn't >> change any thing in the defconfig as couple of other platforms already >> do the same. It's needed in case all other ARCH_* configs are disabled. >> >> Without this, we need a dirty trick in the DT[2] to handle !CONFIG_PM. >> >> Can you please pick this for v4.8 ? > > Question: if AMBA cannot deal with CONFIG_PM disabled, should > the 'select' be done from CONFIG_ARM_AMBA instead? > But few platforms using AMBA may not want PM at all. > I also feel a little uncomfortable with just adding 'select PM', given > that this option is normally just implied by CONFIG_SUSPEND || > CONFIG_HIBERNATE_CALLBACKS, On that analogy, for the current requirement I would say PM_GENERIC_DOMAINS should select but it just depends on PM. > but not selected individually, I agree, but Ulf suggested this. > so we might run into regressionswhen we get configurations that > were invalid before. Sorry my build/config knowledge is limited, I don't understand what you mean by the above. I am asking just to improve my knowledge. > Then again ARCH_RENESAS seems to already do this. Yes, it was hard to find get !CONFIG_PM as many platforms select it directly or indirectly. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Friday, June 17, 2016 2:38:32 PM CEST Sudeep Holla wrote: > On 17/06/16 12:53, Arnd Bergmann wrote: > >> The discussion on this happened on linux-pm list[1]. This is need on > >> Juno once we introduce coresight components in the DT. With !CONFIG_PM, > >> the board stalls on boot and hence this patch is needed. This shouldn't > >> change any thing in the defconfig as couple of other platforms already > >> do the same. It's needed in case all other ARCH_* configs are disabled. > >> > >> Without this, we need a dirty trick in the DT[2] to handle !CONFIG_PM. > >> > >> Can you please pick this for v4.8 ? > > > > Question: if AMBA cannot deal with CONFIG_PM disabled, should > > the 'select' be done from CONFIG_ARM_AMBA instead? > > > > But few platforms using AMBA may not want PM at all. Ok, then I just misunderstood what you meant with "the AMBA framework can't deal with !CONFIG_PM case". > > I also feel a little uncomfortable with just adding 'select PM', given > > that this option is normally just implied by CONFIG_SUSPEND || > > CONFIG_HIBERNATE_CALLBACKS, > > On that analogy, for the current requirement I would say > PM_GENERIC_DOMAINS should select but it just depends on PM. Possibly, it's not entirely clear what the dependency is used for here, or what CONFIG_PM actually means. I'm also still unsure what aspect of CONFIG_PM you actually rely on here. Is this about the amba_pm_runtime_suspend/amba_pm_runtime_resume operations or something else? > > so we might run into regressionswhen we get configurations that > > were invalid before. > > Sorry my build/config knowledge is limited, I don't understand what you > mean by the above. I am asking just to improve my knowledge. If today, CONFIG_PM is only set when CONFIG_PM_SLEEP is set and we introduce a case where we have PM without PM_SLEEP, a lot of drivers that incorrectly make the assumption that PM_SLEEP is mean with CONFIG_PM may behave in new unexpected ways. > > Then again ARCH_RENESAS seems to already do this. > > Yes, it was hard to find get !CONFIG_PM as many platforms select it > directly or indirectly. Starting from an allmodconfig build, I just needed to disable ARCH_RENESAS, ARCH_OMAP2PLUS_TYPICAL, SUSPEND and HIBERNATE. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17/06/16 15:16, Arnd Bergmann wrote: > On Friday, June 17, 2016 2:38:32 PM CEST Sudeep Holla wrote: >> On 17/06/16 12:53, Arnd Bergmann wrote: >>>> The discussion on this happened on linux-pm list[1]. This is >>>> need on Juno once we introduce coresight components in the DT. >>>> With !CONFIG_PM, the board stalls on boot and hence this patch >>>> is needed. This shouldn't change any thing in the defconfig as >>>> couple of other platforms already do the same. It's needed in >>>> case all other ARCH_* configs are disabled. >>>> >>>> Without this, we need a dirty trick in the DT[2] to handle >>>> !CONFIG_PM. >>>> >>>> Can you please pick this for v4.8 ? >>> >>> Question: if AMBA cannot deal with CONFIG_PM disabled, should the >>> 'select' be done from CONFIG_ARM_AMBA instead? >>> >> >> But few platforms using AMBA may not want PM at all. > > Ok, then I just misunderstood what you meant with "the AMBA framework > can't deal with !CONFIG_PM case". > To be more precise genpd/runtime code return same error -ENODEV when PM_GENERIC_DOMAINS=n and if the no power domain is found with PM_GENERIC_DOMAINS=y . The AMBA bus code just checks for -EPROBE_DEFER to stop adding devices, while proceeds to access the device otherwise. However on Juno, the coresight needs to powered up before accessing it. Well all is fine if you specify power domains in DT and have PM_GENERIC_DOMAINS=y. However if PM_GENERIC_DOMAINS=n the AMBA ignores the error(-ENODEV) from runtime PM and access the device resulting in boot hang when adding amba devices. I thought we need is to have code to check for power domains in DT for the device even when PM_GENERIC_DOMAINS=n and return an error that can be used to skip registering the device and clearly identifies with other allowed errors. It may need a bit of surgery and closer look IMO. But when asked for opinion, I was suggested to do this. I admit that I didn't propose my thoughts then :) >>> I also feel a little uncomfortable with just adding 'select PM', >>> given that this option is normally just implied by CONFIG_SUSPEND >>> || CONFIG_HIBERNATE_CALLBACKS, >> >> On that analogy, for the current requirement I would say >> PM_GENERIC_DOMAINS should select but it just depends on PM. > > Possibly, it's not entirely clear what the dependency is used for > here, or what CONFIG_PM actually means. I'm also still unsure what > aspect of CONFIG_PM you actually rely on here. Is this about the > amba_pm_runtime_suspend/amba_pm_runtime_resume operations or > something else? > Yes, as amba_pm_runtime and power domains via DT as explained above. Sorry I should have explained this in the commit message. >>> so we might run into regressions when we get configurations that >>> were invalid before. >> >> Sorry my build/config knowledge is limited, I don't understand what >> you mean by the above. I am asking just to improve my knowledge. > > If today, CONFIG_PM is only set when CONFIG_PM_SLEEP is set and we > introduce a case where we have PM without PM_SLEEP, a lot of drivers > that incorrectly make the assumption that PM_SLEEP is mean with > CONFIG_PM may behave in new unexpected ways. > Understood. >>> Then again ARCH_RENESAS seems to already do this. >> >> Yes, it was hard to find get !CONFIG_PM as many platforms select >> it directly or indirectly. > > Starting from an allmodconfig build, I just needed to disable > ARCH_RENESAS, ARCH_OMAP2PLUS_TYPICAL, SUSPEND and HIBERNATE. > I assume this is in ARM32, I am looking at ARM64, but allmodconfig might help. -- Regards, Sudeep -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Friday, June 17, 2016 3:59:34 PM CEST Sudeep Holla wrote: > > On 17/06/16 15:16, Arnd Bergmann wrote: > > On Friday, June 17, 2016 2:38:32 PM CEST Sudeep Holla wrote: > >> On 17/06/16 12:53, Arnd Bergmann wrote: > >>>> The discussion on this happened on linux-pm list[1]. This is > >>>> need on Juno once we introduce coresight components in the DT. > >>>> With !CONFIG_PM, the board stalls on boot and hence this patch > >>>> is needed. This shouldn't change any thing in the defconfig as > >>>> couple of other platforms already do the same. It's needed in > >>>> case all other ARCH_* configs are disabled. > >>>> > >>>> Without this, we need a dirty trick in the DT[2] to handle > >>>> !CONFIG_PM. > >>>> > >>>> Can you please pick this for v4.8 ? > >>> > >>> Question: if AMBA cannot deal with CONFIG_PM disabled, should the > >>> 'select' be done from CONFIG_ARM_AMBA instead? > >>> > >> > >> But few platforms using AMBA may not want PM at all. > > > > Ok, then I just misunderstood what you meant with "the AMBA framework > > can't deal with !CONFIG_PM case". > > > > To be more precise genpd/runtime code return same error -ENODEV when > PM_GENERIC_DOMAINS=n and if the no power domain is found with > PM_GENERIC_DOMAINS=y . The AMBA bus code just checks for -EPROBE_DEFER > to stop adding devices, while proceeds to access the device otherwise. > > However on Juno, the coresight needs to powered up before accessing it. > Well all is fine if you specify power domains in DT and have > PM_GENERIC_DOMAINS=y. However if PM_GENERIC_DOMAINS=n the AMBA ignores > the error(-ENODEV) from runtime PM and access the device resulting in > boot hang when adding amba devices. > > I thought we need is to have code to check for power domains in DT for > the device even when PM_GENERIC_DOMAINS=n and return an error that can > be used to skip registering the device and clearly identifies with other > allowed errors. It may need a bit of surgery and closer look IMO. But > when asked for opinion, I was suggested to do this. I admit that I > didn't propose my thoughts then :) Ok, so we also need PM_GENERIC_DOMAINS, not just PM, right? It also sounds like there is no easy way to just turn on the clocks in the amba bus handler when PM_GENERIC_DOMAINS is turned off, other than reimplementing PM_GENERIC_DOMAINS? I'm just guessing here, I haven't looked into the code in enough depth. > >>> Then again ARCH_RENESAS seems to already do this. > >> > >> Yes, it was hard to find get !CONFIG_PM as many platforms select > >> it directly or indirectly. > > > > Starting from an allmodconfig build, I just needed to disable > > ARCH_RENESAS, ARCH_OMAP2PLUS_TYPICAL, SUSPEND and HIBERNATE. > > > > I assume this is in ARM32, I am looking at ARM64, but allmodconfig might > help. Yes, I was looking at ARM32. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17/06/16 16:07, Arnd Bergmann wrote: > On Friday, June 17, 2016 3:59:34 PM CEST Sudeep Holla wrote: [...] >> To be more precise genpd/runtime code return same error -ENODEV when >> PM_GENERIC_DOMAINS=n and if the no power domain is found with >> PM_GENERIC_DOMAINS=y . The AMBA bus code just checks for -EPROBE_DEFER >> to stop adding devices, while proceeds to access the device otherwise. >> >> However on Juno, the coresight needs to powered up before accessing it. >> Well all is fine if you specify power domains in DT and have >> PM_GENERIC_DOMAINS=y. However if PM_GENERIC_DOMAINS=n the AMBA ignores >> the error(-ENODEV) from runtime PM and access the device resulting in >> boot hang when adding amba devices. >> >> I thought we need is to have code to check for power domains in DT for >> the device even when PM_GENERIC_DOMAINS=n and return an error that can >> be used to skip registering the device and clearly identifies with other >> allowed errors. It may need a bit of surgery and closer look IMO. But >> when asked for opinion, I was suggested to do this. I admit that I >> didn't propose my thoughts then :) > > Ok, so we also need PM_GENERIC_DOMAINS, not just PM, right? > Yes, but latter won't select former. Generally I have seen domain controller code selecting PM_GENERIC_DOMAINS if PM=y > It also sounds like there is no easy way to just turn on the clocks > in the amba bus handler when PM_GENERIC_DOMAINS is turned off, other > than reimplementing PM_GENERIC_DOMAINS? > Turn on power domain, not just clocks(just to be precise), yes. It needs some thoughts as there are some assumptions on which existing platform work. Adding the support to handle the scenario I mentioned earlier is not trivial. Also the driver to handle the device may not be compiled in when PM_GENERIC_DOMAINS=n or when PM=n. So I was thinking of not accessing the device and hence skip adding it. > I'm just guessing here, I haven't looked into the code in enough > depth. > Ok, but that's correct. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Friday, June 17, 2016 4:49:34 PM CEST Sudeep Holla wrote: > > On 17/06/16 16:07, Arnd Bergmann wrote: > > On Friday, June 17, 2016 3:59:34 PM CEST Sudeep Holla wrote: > > [...] > > >> To be more precise genpd/runtime code return same error -ENODEV when > >> PM_GENERIC_DOMAINS=n and if the no power domain is found with > >> PM_GENERIC_DOMAINS=y . The AMBA bus code just checks for -EPROBE_DEFER > >> to stop adding devices, while proceeds to access the device otherwise. > >> > >> However on Juno, the coresight needs to powered up before accessing it. > >> Well all is fine if you specify power domains in DT and have > >> PM_GENERIC_DOMAINS=y. However if PM_GENERIC_DOMAINS=n the AMBA ignores > >> the error(-ENODEV) from runtime PM and access the device resulting in > >> boot hang when adding amba devices. > >> > >> I thought we need is to have code to check for power domains in DT for > >> the device even when PM_GENERIC_DOMAINS=n and return an error that can > >> be used to skip registering the device and clearly identifies with other > >> allowed errors. It may need a bit of surgery and closer look IMO. But > >> when asked for opinion, I was suggested to do this. I admit that I > >> didn't propose my thoughts then :) > > > > Ok, so we also need PM_GENERIC_DOMAINS, not just PM, right? > > > Yes, but latter won't select former. Generally I have seen domain > controller code selecting PM_GENERIC_DOMAINS if PM=y Then at least we need the vexpress code to select both as well, otherwise you could end up with PM_GENERIC_DOMAINS disabled in configurations that have none of the other code. > > It also sounds like there is no easy way to just turn on the clocks > > in the amba bus handler when PM_GENERIC_DOMAINS is turned off, other > > than reimplementing PM_GENERIC_DOMAINS? > > > > Turn on power domain, not just clocks(just to be precise), yes. It needs > some thoughts as there are some assumptions on which existing platform > work. Adding the support to handle the scenario I mentioned earlier is > not trivial. Also the driver to handle the device may not be compiled in > when PM_GENERIC_DOMAINS=n or when PM=n. So I was thinking of not > accessing the device and hence skip adding it. I see. Another idea I had is flawed, too: we can't easily make ARM_AMBA depend on PM_GENERIC_DOMAINS, because the main relevant use case I can see for !PM builds is a kernel that is only used in virtual machines, but we still want to use the PL01x serial port driver in that configuration. I'd say let's select both PM_GENERIC_DOMAINS and PM on VEXPRESS for now, (almost) as you have in your patch. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17/06/16 20:38, Arnd Bergmann wrote: > On Friday, June 17, 2016 4:49:34 PM CEST Sudeep Holla wrote: >> >> On 17/06/16 16:07, Arnd Bergmann wrote: >>> On Friday, June 17, 2016 3:59:34 PM CEST Sudeep Holla wrote: >> >> [...] >> >>>> To be more precise genpd/runtime code return same error -ENODEV when >>>> PM_GENERIC_DOMAINS=n and if the no power domain is found with >>>> PM_GENERIC_DOMAINS=y . The AMBA bus code just checks for -EPROBE_DEFER >>>> to stop adding devices, while proceeds to access the device otherwise. >>>> >>>> However on Juno, the coresight needs to powered up before accessing it. >>>> Well all is fine if you specify power domains in DT and have >>>> PM_GENERIC_DOMAINS=y. However if PM_GENERIC_DOMAINS=n the AMBA ignores >>>> the error(-ENODEV) from runtime PM and access the device resulting in >>>> boot hang when adding amba devices. >>>> >>>> I thought we need is to have code to check for power domains in DT for >>>> the device even when PM_GENERIC_DOMAINS=n and return an error that can >>>> be used to skip registering the device and clearly identifies with other >>>> allowed errors. It may need a bit of surgery and closer look IMO. But >>>> when asked for opinion, I was suggested to do this. I admit that I >>>> didn't propose my thoughts then :) >>> >>> Ok, so we also need PM_GENERIC_DOMAINS, not just PM, right? >>> >> Yes, but latter won't select former. Generally I have seen domain >> controller code selecting PM_GENERIC_DOMAINS if PM=y > > Then at least we need the vexpress code to select both as well, > otherwise you could end up with PM_GENERIC_DOMAINS disabled in > configurations that have none of the other code. > True, will add it. >>> It also sounds like there is no easy way to just turn on the clocks >>> in the amba bus handler when PM_GENERIC_DOMAINS is turned off, other >>> than reimplementing PM_GENERIC_DOMAINS? >>> >> >> Turn on power domain, not just clocks(just to be precise), yes. It needs >> some thoughts as there are some assumptions on which existing platform >> work. Adding the support to handle the scenario I mentioned earlier is >> not trivial. Also the driver to handle the device may not be compiled in >> when PM_GENERIC_DOMAINS=n or when PM=n. So I was thinking of not >> accessing the device and hence skip adding it. > > I see. Another idea I had is flawed, too: we can't easily make ARM_AMBA > depend on PM_GENERIC_DOMAINS, because the main relevant use case I > can see for !PM builds is a kernel that is only used in virtual machines, > but we still want to use the PL01x serial port driver in that configuration. > > I'd say let's select both PM_GENERIC_DOMAINS and PM on VEXPRESS for now, > (almost) as you have in your patch. > OK, thanks for taking time to review. I understand it's not the best solution but *just OK* for now. I will repost with suggested change. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms index 7ef1d05859ae..8983ab14c9ca 100644 --- a/arch/arm64/Kconfig.platforms +++ b/arch/arm64/Kconfig.platforms @@ -160,6 +160,7 @@ config ARCH_VEXPRESS bool "ARMv8 software model (Versatile Express)" select ARCH_REQUIRE_GPIOLIB select COMMON_CLK_VERSATILE + select PM select POWER_RESET_VEXPRESS select VEXPRESS_CONFIG help
The Linux AMBA bus framework probes the peripheral IDs when adding the AMBA devices very early on the boot. Generally they are on APB bus and just require APB clocks to be on even when most of the core logic of the IP is powered down. However on Juno, the entire debugsys domain needs to be ON to access even the coresight components' CID/PID registers and hence broken by design. Accessing those while debugsys power domain is off will lead to the bridge stalling the transactions instead of returning the slave error. Further, the AMBA framework can't deal with !CONFIG_PM case: it ignores the error and proceeds to access the device region. It was suggested to always enable CONFIG_PM in order to handle this scenario. Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- arch/arm64/Kconfig.platforms | 1 + 1 file changed, 1 insertion(+) Hi ARM-SoC team, The discussion on this happened on linux-pm list[1]. This is need on Juno once we introduce coresight components in the DT. With !CONFIG_PM, the board stalls on boot and hence this patch is needed. This shouldn't change any thing in the defconfig as couple of other platforms already do the same. It's needed in case all other ARCH_* configs are disabled. Without this, we need a dirty trick in the DT[2] to handle !CONFIG_PM. Can you please pick this for v4.8 ? Regards, Sudeep [1] http://marc.info/?l=linux-pm&m=146607608629880&w=2 [2] https://marc.info/?l=linux-arm-kernel&m=146522896503670&w=2 -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel