| Message ID | CACRpkdbpD+Kr01hDc7QnL4x_4=A_rnAO+G0CaMce8+5ns+bx2Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
--- 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