imx: mx7: snvs: Add an SNVS init routine

Working with HAB on the i.MX7 we've encountered a case where a board that
successfully authenticates u-boot when booting Linux via OPTEE subsequently
fails to properly bring up the RTC.

The RTC registers live in the low-power block of the Secure Non-Volatile
Storage (SNVS) block.

The root cause of the error has been traced to the HAB handing off the
SNVS-RTC in a state where HPCOMR::NPSWA_EN = 0 in other words where the
Non-Privileged Software Access Enable bit is zero. In ordinary
circumstances this is OK since we typically do not run in TZ mode, however
when we boot via HAB and enablng TrustZone, it is required to set
HPCOMR::NPSWA_EN = 1 in order for the upstream Linux driver to have
sufficient permissions to manipulate the SNVS-LP block.

On our reference board it is the difference between Linux doing this:

root@imx7s-warp-mbl:~# dmesg | grep rtc
snvs_rtc_enable read 0x00000000 from SNVS_LPLR @ 0x00000034
snvs_rtc_enable read 0x00000021 from SNVS_LPCR @ 0x00000038
snvs_rtc_enable read 0x00000000 from SNVS_HPLR @ 0x00000000
snvs_rtc_enable read 0x80002100 from SNVS_HPCOMR @ 0x00000004
snvs_rtc 30370000.snvs:snvs-rtc-lp: rtc core: registered
         30370000.snvs:snvs-rtc-lp as rtc0
snvs_rtc 30370000.snvs:snvs-rtc-lp: setting system clock to2018-04-01 00:51:04 UTC (1522543864)

and doing this:

root@imx7s-warp-mbl:~# dmesg | grep rtc
snvs_rtc_enable read 0x00000000 from SNVS_LPLR @ 0x00000034
snvs_rtc_enable read 0x00000020 from SNVS_LPCR @ 0x00000038
snvs_rtc_enable read 0x00000001 from SNVS_HPLR @ 0x00000000
snvs_rtc_enable read 0x00002020 from SNVS_HPCOMR @ 0x00000004
snvs_rtc 30370000.snvs:snvs-rtc-lp: failed to enable rtc -110
snvs_rtc: probe of 30370000.snvs:snvs-rtc-lp failed with error -110
hctosys: unable to open rtc device (rtc0)

Note bit 1 of LPCR is not set in the second case and is set in the first
case and that bit 31 of HPCOMR is set in the second case but not in the
first.

Setting NPSWA_EN in HPCOMR allows us to boot through enabling TrustZone
and continue onto the kernel. The kernel then has the necessary permissions
to set LPCR::SRTC_ENV (RTC enable in the LP command register) whereas in
contrast - in the failing case the non-privileged kernel cannot do so.

This patch adds a simple init_snvs() call which sets the permission-bit
called from soc.c for the i.MX7. It may be possible, safe and desirable to
perform this on other i.MX processors but for now this is only tested on
i.MX7 as working.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm/include/asm/mach-imx/sys_proto.h |  1 +
 arch/arm/mach-imx/mx7/Makefile            |  2 +-
 arch/arm/mach-imx/mx7/snvs.c              | 22 ++++++++++++++++++++++
 arch/arm/mach-imx/mx7/soc.c               |  2 ++
 4 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-imx/mx7/snvs.c

+ Stefano

On 05/04/18 19:46, Bryan O'Donoghue wrote:
> Working with HAB on the i.MX7 we've encountered a case where a board that
> successfully authenticates u-boot when booting Linux via OPTEE subsequently
> fails to properly bring up the RTC.
> 
> The RTC registers live in the low-power block of the Secure Non-Volatile
> Storage (SNVS) block.
> 
> The root cause of the error has been traced to the HAB handing off the
> SNVS-RTC in a state where HPCOMR::NPSWA_EN = 0 in other words where the
> Non-Privileged Software Access Enable bit is zero. In ordinary
> circumstances this is OK since we typically do not run in TZ mode, however
> when we boot via HAB and enablng TrustZone, it is required to set
> HPCOMR::NPSWA_EN = 1 in order for the upstream Linux driver to have
> sufficient permissions to manipulate the SNVS-LP block.
> 
> On our reference board it is the difference between Linux doing this:
> 
> root@imx7s-warp-mbl:~# dmesg | grep rtc
> snvs_rtc_enable read 0x00000000 from SNVS_LPLR @ 0x00000034
> snvs_rtc_enable read 0x00000021 from SNVS_LPCR @ 0x00000038
> snvs_rtc_enable read 0x00000000 from SNVS_HPLR @ 0x00000000
> snvs_rtc_enable read 0x80002100 from SNVS_HPCOMR @ 0x00000004
> snvs_rtc 30370000.snvs:snvs-rtc-lp: rtc core: registered
>           30370000.snvs:snvs-rtc-lp as rtc0
> snvs_rtc 30370000.snvs:snvs-rtc-lp: setting system clock to2018-04-01 00:51:04 UTC (1522543864)
> 
> and doing this:
> 
> root@imx7s-warp-mbl:~# dmesg | grep rtc
> snvs_rtc_enable read 0x00000000 from SNVS_LPLR @ 0x00000034
> snvs_rtc_enable read 0x00000020 from SNVS_LPCR @ 0x00000038
> snvs_rtc_enable read 0x00000001 from SNVS_HPLR @ 0x00000000
> snvs_rtc_enable read 0x00002020 from SNVS_HPCOMR @ 0x00000004
> snvs_rtc 30370000.snvs:snvs-rtc-lp: failed to enable rtc -110
> snvs_rtc: probe of 30370000.snvs:snvs-rtc-lp failed with error -110
> hctosys: unable to open rtc device (rtc0)
> 
> Note bit 1 of LPCR is not set in the second case and is set in the first
> case and that bit 31 of HPCOMR is set in the second case but not in the
> first.
> 
> Setting NPSWA_EN in HPCOMR allows us to boot through enabling TrustZone
> and continue onto the kernel. The kernel then has the necessary permissions
> to set LPCR::SRTC_ENV (RTC enable in the LP command register) whereas in
> contrast - in the failing case the non-privileged kernel cannot do so.
> 
> This patch adds a simple init_snvs() call which sets the permission-bit
> called from soc.c for the i.MX7. It may be possible, safe and desirable to
> perform this on other i.MX processors but for now this is only tested on
> i.MX7 as working.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   arch/arm/include/asm/mach-imx/sys_proto.h |  1 +
>   arch/arm/mach-imx/mx7/Makefile            |  2 +-
>   arch/arm/mach-imx/mx7/snvs.c              | 22 ++++++++++++++++++++++
>   arch/arm/mach-imx/mx7/soc.c               |  2 ++
>   4 files changed, 26 insertions(+), 1 deletion(-)
>   create mode 100644 arch/arm/mach-imx/mx7/snvs.c
> 
> diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h
> index 96795e1..aa51c0d 100644
> --- a/arch/arm/include/asm/mach-imx/sys_proto.h
> +++ b/arch/arm/include/asm/mach-imx/sys_proto.h
> @@ -107,6 +107,7 @@ void set_chipselect_size(int const);
>   
>   void init_aips(void);
>   void init_src(void);
> +void init_snvs(void);
>   void imx_wdog_disable_powerdown(void);
>   
>   int board_mmc_get_env_dev(int devno);
> diff --git a/arch/arm/mach-imx/mx7/Makefile b/arch/arm/mach-imx/mx7/Makefile
> index ce289c1..e6bef6a 100644
> --- a/arch/arm/mach-imx/mx7/Makefile
> +++ b/arch/arm/mach-imx/mx7/Makefile
> @@ -5,7 +5,7 @@
>   #
>   #
>   
> -obj-y	:= soc.o clock.o clock_slice.o ddr.o
> +obj-y	:= soc.o clock.o clock_slice.o ddr.o snvs.o
>   
>   ifdef CONFIG_ARMV7_PSCI
>   obj-y  += psci-mx7.o psci.o
> diff --git a/arch/arm/mach-imx/mx7/snvs.c b/arch/arm/mach-imx/mx7/snvs.c
> new file mode 100644
> index 0000000..7e649b8
> --- /dev/null
> +++ b/arch/arm/mach-imx/mx7/snvs.c
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright 2018 Linaro
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <asm/io.h>
> +#include <asm/arch/imx-regs.h>
> +#include <linux/bitops.h>
> +
> +#define SNVS_HPCOMR		0x04
> +#define SNVS_HPCOMR_NPSWA_EN	BIT(31)
> +
> +void init_snvs(void)
> +{
> +	u32 val;
> +
> +	/* Ensure SNVS HPCOMR sets NPSWA_EN to allow unpriv access to SNVS LP */
> +	val = readl(SNVS_BASE_ADDR + SNVS_HPCOMR);
> +	val |= SNVS_HPCOMR_NPSWA_EN;
> +	writel(val, SNVS_BASE_ADDR + SNVS_HPCOMR);
> +}
> diff --git a/arch/arm/mach-imx/mx7/soc.c b/arch/arm/mach-imx/mx7/soc.c
> index fb92a26..3ceeeff 100644
> --- a/arch/arm/mach-imx/mx7/soc.c
> +++ b/arch/arm/mach-imx/mx7/soc.c
> @@ -180,6 +180,8 @@ int arch_cpu_init(void)
>   	isolate_resource();
>   #endif
>   
> +	init_snvs();
> +
>   	return 0;
>   }
>   
>

Hi Bryan,

On Thu, Apr 5, 2018 at 3:46 PM, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:

> --- a/arch/arm/mach-imx/mx7/soc.c
> +++ b/arch/arm/mach-imx/mx7/soc.c
> @@ -180,6 +180,8 @@ int arch_cpu_init(void)
>         isolate_resource();
>  #endif
>
> +       init_snvs();

Shouldn't this be called only if CONFIG_SECURE_BOOT is selected?

On 05/04/18 20:29, Fabio Estevam wrote:
> Hi Bryan,
> 
> On Thu, Apr 5, 2018 at 3:46 PM, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
> 
>> --- a/arch/arm/mach-imx/mx7/soc.c
>> +++ b/arch/arm/mach-imx/mx7/soc.c
>> @@ -180,6 +180,8 @@ int arch_cpu_init(void)
>>          isolate_resource();
>>   #endif
>>
>> +       init_snvs();
> 
> Shouldn't this be called only if CONFIG_SECURE_BOOT is selected?
> 

No, I don't think so.

HAB can hand off to u-boot with the bit unset and you don't have to have 
CONFIG_SECURE_BOOT set in u-boot to install or chain-load a TEE.

There's no technical requirement that I know of that would make this a 
secure-boot only dependency

On Fri, Apr 6, 2018 at 6:41 AM, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:

> No, I don't think so.
>
> HAB can hand off to u-boot with the bit unset and you don't have to have
> CONFIG_SECURE_BOOT set in u-boot to install or chain-load a TEE.
>
> There's no technical requirement that I know of that would make this a
> secure-boot only dependency

Ok, thanks for the clarification.

On 05/04/2018 20:46, Bryan O'Donoghue wrote:
> Working with HAB on the i.MX7 we've encountered a case where a board that
> successfully authenticates u-boot when booting Linux via OPTEE subsequently
> fails to properly bring up the RTC.
> 
> The RTC registers live in the low-power block of the Secure Non-Volatile
> Storage (SNVS) block.
> 
> The root cause of the error has been traced to the HAB handing off the
> SNVS-RTC in a state where HPCOMR::NPSWA_EN = 0 in other words where the
> Non-Privileged Software Access Enable bit is zero. In ordinary
> circumstances this is OK since we typically do not run in TZ mode, however
> when we boot via HAB and enablng TrustZone, it is required to set
> HPCOMR::NPSWA_EN = 1 in order for the upstream Linux driver to have
> sufficient permissions to manipulate the SNVS-LP block.
> 
> On our reference board it is the difference between Linux doing this:
> 
> root@imx7s-warp-mbl:~# dmesg | grep rtc
> snvs_rtc_enable read 0x00000000 from SNVS_LPLR @ 0x00000034
> snvs_rtc_enable read 0x00000021 from SNVS_LPCR @ 0x00000038
> snvs_rtc_enable read 0x00000000 from SNVS_HPLR @ 0x00000000
> snvs_rtc_enable read 0x80002100 from SNVS_HPCOMR @ 0x00000004
> snvs_rtc 30370000.snvs:snvs-rtc-lp: rtc core: registered
>          30370000.snvs:snvs-rtc-lp as rtc0
> snvs_rtc 30370000.snvs:snvs-rtc-lp: setting system clock to2018-04-01 00:51:04 UTC (1522543864)
> 
> and doing this:
> 
> root@imx7s-warp-mbl:~# dmesg | grep rtc
> snvs_rtc_enable read 0x00000000 from SNVS_LPLR @ 0x00000034
> snvs_rtc_enable read 0x00000020 from SNVS_LPCR @ 0x00000038
> snvs_rtc_enable read 0x00000001 from SNVS_HPLR @ 0x00000000
> snvs_rtc_enable read 0x00002020 from SNVS_HPCOMR @ 0x00000004
> snvs_rtc 30370000.snvs:snvs-rtc-lp: failed to enable rtc -110
> snvs_rtc: probe of 30370000.snvs:snvs-rtc-lp failed with error -110
> hctosys: unable to open rtc device (rtc0)
> 
> Note bit 1 of LPCR is not set in the second case and is set in the first
> case and that bit 31 of HPCOMR is set in the second case but not in the
> first.
> 
> Setting NPSWA_EN in HPCOMR allows us to boot through enabling TrustZone
> and continue onto the kernel. The kernel then has the necessary permissions
> to set LPCR::SRTC_ENV (RTC enable in the LP command register) whereas in
> contrast - in the failing case the non-privileged kernel cannot do so.
> 
> This patch adds a simple init_snvs() call which sets the permission-bit
> called from soc.c for the i.MX7. It may be possible, safe and desirable to
> perform this on other i.MX processors but for now this is only tested on
> i.MX7 as working.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  arch/arm/include/asm/mach-imx/sys_proto.h |  1 +
>  arch/arm/mach-imx/mx7/Makefile            |  2 +-
>  arch/arm/mach-imx/mx7/snvs.c              | 22 ++++++++++++++++++++++
>  arch/arm/mach-imx/mx7/soc.c               |  2 ++
>  4 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-imx/mx7/snvs.c
> 
> diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h
> index 96795e1..aa51c0d 100644
> --- a/arch/arm/include/asm/mach-imx/sys_proto.h
> +++ b/arch/arm/include/asm/mach-imx/sys_proto.h
> @@ -107,6 +107,7 @@ void set_chipselect_size(int const);
>  
>  void init_aips(void);
>  void init_src(void);
> +void init_snvs(void);
>  void imx_wdog_disable_powerdown(void);
>  
>  int board_mmc_get_env_dev(int devno);
> diff --git a/arch/arm/mach-imx/mx7/Makefile b/arch/arm/mach-imx/mx7/Makefile
> index ce289c1..e6bef6a 100644
> --- a/arch/arm/mach-imx/mx7/Makefile
> +++ b/arch/arm/mach-imx/mx7/Makefile
> @@ -5,7 +5,7 @@
>  #
>  #
>  
> -obj-y	:= soc.o clock.o clock_slice.o ddr.o
> +obj-y	:= soc.o clock.o clock_slice.o ddr.o snvs.o
>  
>  ifdef CONFIG_ARMV7_PSCI
>  obj-y  += psci-mx7.o psci.o
> diff --git a/arch/arm/mach-imx/mx7/snvs.c b/arch/arm/mach-imx/mx7/snvs.c
> new file mode 100644
> index 0000000..7e649b8
> --- /dev/null
> +++ b/arch/arm/mach-imx/mx7/snvs.c
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright 2018 Linaro
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <asm/io.h>
> +#include <asm/arch/imx-regs.h>
> +#include <linux/bitops.h>
> +
> +#define SNVS_HPCOMR		0x04
> +#define SNVS_HPCOMR_NPSWA_EN	BIT(31)
> +
> +void init_snvs(void)
> +{
> +	u32 val;
> +
> +	/* Ensure SNVS HPCOMR sets NPSWA_EN to allow unpriv access to SNVS LP */
> +	val = readl(SNVS_BASE_ADDR + SNVS_HPCOMR);
> +	val |= SNVS_HPCOMR_NPSWA_EN;
> +	writel(val, SNVS_BASE_ADDR + SNVS_HPCOMR);
> +}
> diff --git a/arch/arm/mach-imx/mx7/soc.c b/arch/arm/mach-imx/mx7/soc.c
> index fb92a26..3ceeeff 100644
> --- a/arch/arm/mach-imx/mx7/soc.c
> +++ b/arch/arm/mach-imx/mx7/soc.c
> @@ -180,6 +180,8 @@ int arch_cpu_init(void)
>  	isolate_resource();
>  #endif
>  
> +	init_snvs();
> +
>  	return 0;
>  }
>  
> 


Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic