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

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

[Sebastian Reichel]

From: Peter Senna Tschudin <peter.senna@collabora.com>

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@collabora.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 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;
+		};
+	};
+
 	clocks {
 		mclk: clock@0 {
 			compatible = "fixed-clock";
-- 
2.15.1

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

[Shawn Guo]

+ Frank

On Mon, Jan 15, 2018 at 05:07:22PM +0100, Sebastian Reichel wrote:
> From: Peter Senna Tschudin <peter.senna@collabora.com>
> 
> 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@collabora.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  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?

Shawn

> +		};
> +	};
> +
>  	clocks {
>  		mclk: clock@0 {
>  			compatible = "fixed-clock";
> -- 
> 2.15.1
> 

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

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@collabora.com>
> > 
> > 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@collabora.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  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.

-- Sebastian

> 
> Shawn
> 
> > +		};
> > +	};
> > +
> >  	clocks {
> >  		mclk: clock@0 {
> >  			compatible = "fixed-clock";
> > -- 
> > 2.15.1
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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@collabora.com>
> > > 
> > > 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@collabora.com>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > ---
> > >  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

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

[Sebastian Reichel]

From: Peter Senna Tschudin <peter.senna@collabora.com>

Enable secure debug enable register access for Bx50v3 devices to enable
PMU and hardware counters for perf.

Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
Changes since PATCHv1:
 * add property by adding a label to the pmu node in imx6qdl and
   referencing it in bx50v3.dtsi.
---
 arch/arm/boot/dts/imx6q-bx50v3.dtsi | 4 ++++
 arch/arm/boot/dts/imx6qdl.dtsi      | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6q-bx50v3.dtsi b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
index 916ea94d75ca..2baffc8b9094 100644
--- a/arch/arm/boot/dts/imx6q-bx50v3.dtsi
+++ b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
@@ -388,3 +388,7 @@
 		#interrupt-cells = <1>;
 	};
 };
+
+&pmu {
+	secure-reg-access;
+};
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 59ff86695a14..2b921de44c02 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -143,7 +143,7 @@
 		};
 	};
 
-	pmu {
+	pmu: pmu {
 		compatible = "arm,cortex-a9-pmu";
 		interrupt-parent = <&gpc>;
 		interrupts = <0 94 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.15.1

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

[Fabio Estevam]

On Mon, Feb 5, 2018 at 3:08 PM, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
> From: Peter Senna Tschudin <peter.senna@collabora.com>
>
> Enable secure debug enable register access for Bx50v3 devices to enable
> PMU and hardware counters for perf.
>
> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

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

[Shawn Guo]

On Mon, Feb 05, 2018 at 06:08:40PM +0100, Sebastian Reichel wrote:
> From: Peter Senna Tschudin <peter.senna@collabora.com>
> 
> Enable secure debug enable register access for Bx50v3 devices to enable
> PMU and hardware counters for perf.
> 
> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
> Changes since PATCHv1:
>  * add property by adding a label to the pmu node in imx6qdl and
>    referencing it in bx50v3.dtsi.
> ---
>  arch/arm/boot/dts/imx6q-bx50v3.dtsi | 4 ++++
>  arch/arm/boot/dts/imx6qdl.dtsi      | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-bx50v3.dtsi b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
> index 916ea94d75ca..2baffc8b9094 100644
> --- a/arch/arm/boot/dts/imx6q-bx50v3.dtsi
> +++ b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
> @@ -388,3 +388,7 @@
>  		#interrupt-cells = <1>;
>  	};
>  };
> +
> +&pmu {
> +	secure-reg-access;
> +};

The labelled node should be sorted alphabetically.  That said, it should
be added before &usdhc2.  I fixed it up and applied the patch.

Shawn

> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index 59ff86695a14..2b921de44c02 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -143,7 +143,7 @@
>  		};
>  	};
>  
> -	pmu {
> +	pmu: pmu {
>  		compatible = "arm,cortex-a9-pmu";
>  		interrupt-parent = <&gpc>;
>  		interrupts = <0 94 IRQ_TYPE_LEVEL_HIGH>;
> -- 
> 2.15.1
> 

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

[Frank Li]

> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: Friday, February 02, 2018 1:18 AM
> To: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Sascha Hauer <kernel@pengutronix.de>; Frank Li <frank.li@nxp.com>;
> Fabio Estevam <fabio.estevam@nxp.com>; Ian Ray <ian.ray@ge.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] ARM: dts: imx6q-bx50v3: Enable secure-reg-access
> 
> + Frank
> 
> On Mon, Jan 15, 2018 at 05:07:22PM +0100, Sebastian Reichel wrote:
> > From: Peter Senna Tschudin <peter.senna@collabora.com>
> >
> > 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@collabora.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  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?

I agree. It should be in imx6qdl.dtsi

Frank Li

> 
> Shawn
> 
> > +		};
> > +	};
> > +
> >  	clocks {
> >  		mclk: clock@0 {
> >  			compatible = "fixed-clock";
> > --
> > 2.15.1
> >