[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Sudeep Holla]

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

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
--
2.7.4

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Arnd Bergmann]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Sudeep Holla]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Arnd Bergmann]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Sudeep Holla]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Arnd Bergmann]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Sudeep Holla]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Arnd Bergmann]

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

[PATCH] arm64: Kconfig: select PM for ARCH_VEXPRESS

[Sudeep Holla]

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