ARM64: juno: add NOR flash to device tree

Message ID CACRpkdbpD+Kr01hDc7QnL4x_4=A_rnAO+G0CaMce8+5ns+bx2Q@mail.gmail.com
State New
Headers show

Commit Message

Linus Walleij Oct. 27, 2015, 12:22 p.m.
On Tue, Oct 27, 2015 at 1:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 27/10/15 11:55, Linus Walleij wrote:

>> On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org>

>> wrote:

>>

>>> FYI, in the latest Juno motherboard firmware [1], I have a file called

>>> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the

>>> UEFI

>>> (and now u-boot) config area at the end of the NOR flash.

>>>

>>> I mostly put this there so that you can "touch blank.img" to erase the

>>> config, but also to show that the region is reserved to anyone who might

>>> think about putting something there.

>>

>>

>> Then it is safe to enable NOR flash and AFS in the kernel.

>

> *No*, not on Juno. If we need to enable them for other platforms, then

> we should either disable NOR flash in DT(or even remove it completely

> whichever is appropriate).

>

> Since idle is enable in defconfig and if DT has idle states, then it

> can't boot for the reason I previously mentioned.


Yeah right I remember that now. Let's say it is safe to enable
for the tamper-security reason. There may be other reasons
not to...

If this is about different idle functionalities blocking flash
access, what we should do is to in Kconfig make it impossible
to compile in both deep idle states and flash support at the
same time, as that is how the system really behaves.

I.e do you mean something like this, or am I getting it wrong?

index 21340e0be73e..07d91776bcfe 100644

Hiding hardware from the devicetree seems over the top to me,
it is better to keep the device trees describing the actual hardware
and the operating system to figure out how things need to be
set up to interoperate, at config-time or run-time.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Sudeep Holla Oct. 27, 2015, 12:33 p.m. | #1
On 27/10/15 12:22, Linus Walleij wrote:
> On Tue, Oct 27, 2015 at 1:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

>> On 27/10/15 11:55, Linus Walleij wrote:


[...]

>>> Then it is safe to enable NOR flash and AFS in the kernel.

>>

>> *No*, not on Juno. If we need to enable them for other platforms, then

>> we should either disable NOR flash in DT(or even remove it completely

>> whichever is appropriate).

>>

>> Since idle is enable in defconfig and if DT has idle states, then it

>> can't boot for the reason I previously mentioned.

>

> Yeah right I remember that now. Let's say it is safe to enable

> for the tamper-security reason. There may be other reasons

> not to...

>

> If this is about different idle functionalities blocking flash

> access, what we should do is to in Kconfig make it impossible

> to compile in both deep idle states and flash support at the

> same time, as that is how the system really behaves.

>

> I.e do you mean something like this, or am I getting it wrong?

>


But with this you will never enable idle on all VEXPRESS platforms, what
if the issue on Juno is resolved in future silicon. So I prefer DT way
if we have to enable NOR flash in defconfig.

> index 21340e0be73e..07d91776bcfe 100644

> --- a/drivers/cpuidle/Kconfig.arm

> +++ b/drivers/cpuidle/Kconfig.arm

> @@ -4,6 +4,7 @@

>   config ARM_CPUIDLE

>           bool "Generic ARM/ARM64 CPU idle Driver"

>           select DT_IDLE_STATES

> +       depends on !(ARCH_VEXPRESS && MTD)

>           help

>             Select this to enable generic cpuidle driver for ARM.

>             It provides a generic idle driver whose idle states are configured

>

> Hiding hardware from the devicetree seems over the top to me,


OK, I agree with you on not hiding. But disabling it seems OK for me as
it's a hardware errata(SROM can't be used and hence NOR is used in warm
reset path)

> it is better to keep the device trees describing the actual hardware


Agreed, but we need a way to specify that it used as alternative for
SROM to workaround the hardware issue and hence not available for Linux.
One way I believe is setting the status as disabled.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Oct. 27, 2015, 12:33 p.m. | #2
On Tue, Oct 27, 2015 at 01:22:39PM +0100, Linus Walleij wrote:
> On Tue, Oct 27, 2015 at 1:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> > On 27/10/15 11:55, Linus Walleij wrote:

> >> On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org>

> >> wrote:

> >>

> >>> FYI, in the latest Juno motherboard firmware [1], I have a file called

> >>> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the

> >>> UEFI

> >>> (and now u-boot) config area at the end of the NOR flash.

> >>>

> >>> I mostly put this there so that you can "touch blank.img" to erase the

> >>> config, but also to show that the region is reserved to anyone who might

> >>> think about putting something there.

> >>

> >>

> >> Then it is safe to enable NOR flash and AFS in the kernel.

> >

> > *No*, not on Juno. If we need to enable them for other platforms, then

> > we should either disable NOR flash in DT(or even remove it completely

> > whichever is appropriate).

> >

> > Since idle is enable in defconfig and if DT has idle states, then it

> > can't boot for the reason I previously mentioned.

> 

> Yeah right I remember that now. Let's say it is safe to enable

> for the tamper-security reason. There may be other reasons

> not to...

> 

> If this is about different idle functionalities blocking flash

> access, what we should do is to in Kconfig make it impossible

> to compile in both deep idle states and flash support at the

> same time, as that is how the system really behaves.

> 

> I.e do you mean something like this, or am I getting it wrong?

> 

> index 21340e0be73e..07d91776bcfe 100644

> --- a/drivers/cpuidle/Kconfig.arm

> +++ b/drivers/cpuidle/Kconfig.arm

> @@ -4,6 +4,7 @@

>  config ARM_CPUIDLE

>          bool "Generic ARM/ARM64 CPU idle Driver"

>          select DT_IDLE_STATES

> +       depends on !(ARCH_VEXPRESS && MTD)

>          help

>            Select this to enable generic cpuidle driver for ARM.

>            It provides a generic idle driver whose idle states are configured

> 

> Hiding hardware from the devicetree seems over the top to me,

> it is better to keep the device trees describing the actual hardware

> and the operating system to figure out how things need to be

> set up to interoperate, at config-time or run-time.


... except that the current bindings do not imply any of said
negotiation, and imply that the hardware is available for use. We'd need
new bindings to describe that.

For situations like these we typically have the node, but ensure that it
has status = "disabled". That way the DT is safe by default, and FW can
grant the OS the ability to use the hardware by replacing/removing the
status property.

It sounds like even for U-Boot, PSCI* and Non-secure OS access to the NOR
are mutually exclusive. Given the inability to describe that constraint,
the only agent that can possibly filter the DT appropriately is the FW.

* Sudeep, I assume the constraints on CPU_SUSPEND would also apply to
  CPU_ON, is that correct?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sudeep Holla Oct. 27, 2015, 12:41 p.m. | #3
On 27/10/15 12:33, Mark Rutland wrote:

[...]

>

> * Sudeep, I assume the constraints on CPU_SUSPEND would also apply to

>    CPU_ON, is that correct?

>


Yes, if you meant that this is an issue for even CPU hotplug. We need to
disable CPU_HOTPLUG in that case which is horrible.

So this kind of conditional compile is simply not a solution to
support/work around an hardware issue on one platform.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Linus Walleij Oct. 27, 2015, 9:41 p.m. | #4
On Tue, Oct 27, 2015 at 1:33 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> OK, I agree with you on not hiding. But disabling it seems OK for me as

> it's a hardware errata(SROM can't be used and hence NOR is used in warm

> reset path)


OK then, what about:

status = "disabled";

With a comment explaining why?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Linus Walleij Oct. 27, 2015, 9:43 p.m. | #5
On Tue, Oct 27, 2015 at 1:33 PM, Mark Rutland <mark.rutland@arm.com> wrote:

> For situations like these we typically have the node, but ensure that it

> has status = "disabled". That way the DT is safe by default, and FW can

> grant the OS the ability to use the hardware by replacing/removing the

> status property.


Fair enough, I suggested the same as it happens. Will repost
with this changed and a fat comment describing why.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch

--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -4,6 +4,7 @@ 
 config ARM_CPUIDLE
         bool "Generic ARM/ARM64 CPU idle Driver"
         select DT_IDLE_STATES
+       depends on !(ARCH_VEXPRESS && MTD)
         help
           Select this to enable generic cpuidle driver for ARM.
           It provides a generic idle driver whose idle states are configured