Re: [PATCH] ARM: dts: imx6q-bx50v3: Enable secure-reg-access

[Shawn Guo]

On Fri, Feb 02, 2018 at 12:10:04PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Feb 02, 2018 at 03:18:20PM +0800, Shawn Guo wrote:
> > + Frank
> > 
> > On Mon, Jan 15, 2018 at 05:07:22PM +0100, Sebastian Reichel wrote:
> > > From: Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>
> > > 
> > > Add secure-reg-access on device tree include file for Bx50 devices
> > > to enable PMU and hardware counters for perf.
> > > 
> > > Signed-off-by: Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/imx6q-bx50v3.dtsi | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/imx6q-bx50v3.dtsi b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
> > > index 86cfd4481e72..ccaaee83e2fa 100644
> > > --- a/arch/arm/boot/dts/imx6q-bx50v3.dtsi
> > > +++ b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
> > > @@ -43,6 +43,13 @@
> > >  #include "imx6q-ba16.dtsi"
> > >  
> > >  / {
> > > +	soc {
> > > +		pmu {
> > > +			compatible = "arm,cortex-a9-pmu";
> > > +			secure-reg-access;
> > 
> > I'm not sure this could be a board level configuration.  Shouldn't this
> > property just be added into pmu node in imx6qdl.dtsi?
> 
> This cannot become part of imx6qdl.dtsi for two reasons:
> 
> 1. It's not safe to be enabled on imx6qdl in secure state, see
>    excerpt from DT binding documentation below
> 2. We cannot enable it by default, since DTS format does not provide
>    a method to unset it in board files using a imx6qdl in secure
>    mode.
> 
> Here is an extract from the DT documentation for the PMU node:
> 
> - secure-reg-access : Indicates that the ARMv7 Secure Debug Enable Register
> 		      (SDER) is accessible. This will cause the driver to do
> 		      any setup required that is only possible in ARMv7 secure
> 		      state. If not present the ARMv7 SDER will not be touched,
> 		      which means the PMU may fail to operate unless external
> 		      code (bootloader or security monitor) has performed the
> 		      appropriate initialisation. Note that this property is
> 		      not valid for non-ARMv7 CPUs or ARMv7 CPUs booting Linux
> 		      in Non-secure state.
> 
> For B450v3, B650v3 and B850v3 it's safe to enable the bit and it
> improves debugging capabilities a lot.

Okay.  Then please label the pmu node in imx6qdl.dtsi and referencing
the node in board dts without duplicating soc node and compatible.

Shawn