[PATCH 0/3] arm: mach-imx: tzasc: port lock id_swap_bypass bit

[PATCH 0/3] arm: mach-imx: tzasc: port lock id_swap_bypass bit

[Stefan Kerkmann]

This series ports the U-Boot commit 1289ff7bd7e4 ("imx8m: lock
id_swap_bypass bit in tzc380 enable") to barebox and refactors the tzasc
driver for the imx platform to use the `cpu_is_mx8xyz` macros.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
Stefan Kerkmann (3):
      arm: mach-imx: tzasc: lock id_swap_bypass bit
      arm: mach-imx: set cpu type in pbl
      arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

 arch/arm/mach-imx/Makefile |  2 +-
 arch/arm/mach-imx/atf.c    | 13 +++++++++----
 arch/arm/mach-imx/imx8m.c  |  2 +-
 arch/arm/mach-imx/tzasc.c  | 47 ++++++++++++++++++++++++++--------------------
 include/mach/imx/generic.h |  5 +++++
 include/mach/imx/tzasc.h   |  8 ++------
 6 files changed, 45 insertions(+), 32 deletions(-)
---
base-commit: ed7c14536d521793199abf0597164a46ba68e8e5
change-id: 20240226-v2024-02-0-topic-imx8m-n-p-tzac-480c113ca052

Best regards,
-- 
Stefan Kerkmann <s.kerkmann@pengutronix.de>

[PATCH 1/3] arm: mach-imx: tzasc: lock id_swap_bypass bit

[Stefan Kerkmann]

This commit ports U-Boot commit 1289ff7bd7e4 ("imx8m: lock
id_swap_bypass bit in tzc380 enable") to barebox. This is the original
commit message:

> According to TRM for i.MX8M Nano and Plus, GPR10 register contains lock
> bit for TZASC_ID_SWAP_BYPASS bit. This bit is required to be set in
> order to avoid AXI bus errors when GPU is enabled on the platform.
> TZASC_ID_SWAP_BYPASS bit is alread set for all imx8m applicable
> derivatives, but is missing a lock settings to be applied.
>
> Set the TZASC_ID_SWAP_BYPASS_LOCK bit for those derivatives which have
> it implemented.
>
> Since we're here, provide also names to bits from TRM instead of using
> BIT() macro in the code.

Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 arch/arm/mach-imx/tzasc.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-imx/tzasc.c b/arch/arm/mach-imx/tzasc.c
index 9c71108c99..1f8d7426c1 100644
--- a/arch/arm/mach-imx/tzasc.c
+++ b/arch/arm/mach-imx/tzasc.c
@@ -5,37 +5,59 @@
 #include <mach/imx/imx8m-regs.h>
 #include <io.h>
 
-#define GPR_TZASC_EN		BIT(0)
-#define GPR_TZASC_SWAP_ID	BIT(1)
-#define GPR_TZASC_EN_LOCK	BIT(16)
+#define GPR_TZASC_EN					BIT(0)
+#define GPR_TZASC_ID_SWAP_BYPASS		BIT(1)
+#define GPR_TZASC_EN_LOCK				BIT(16)
+#define GPR_TZASC_ID_SWAP_BYPASS_LOCK	BIT(17)
 
-static void enable_tzc380(bool bypass_id_swap)
+#define MX8M_TZASC_REGION_ATTRIBUTES_0		(MX8M_TZASC_BASE_ADDR + 0x108)
+#define MX8M_TZASC_REGION_ATTRIBUTES_0_SP	GENMASK(31, 28)
+
+static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
 {
 	u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
 
 	/* Enable TZASC and lock setting */
 	setbits_le32(&gpr[10], GPR_TZASC_EN);
 	setbits_le32(&gpr[10], GPR_TZASC_EN_LOCK);
+
+	/*
+	 * According to TRM, TZASC_ID_SWAP_BYPASS should be set in
+	 * order to avoid AXI Bus errors when GPU is in use
+	 */
 	if (bypass_id_swap)
-		setbits_le32(&gpr[10], BIT(1));
+		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS);
+
+	/*
+	 * imx8mn and imx8mp implements the lock bit for
+	 * TZASC_ID_SWAP_BYPASS, enable it to lock settings
+	 */
+	if (bypass_id_swap_lock)
+		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS_LOCK);
+
 	/*
 	 * set Region 0 attribute to allow secure and non-secure
 	 * read/write permission. Found some masters like usb dwc3
 	 * controllers can't work with secure memory.
 	 */
-	writel(0xf0000000, MX8M_TZASC_BASE_ADDR + 0x108);
+	writel(MX8M_TZASC_REGION_ATTRIBUTES_0_SP,
+		   MX8M_TZASC_REGION_ATTRIBUTES_0);
 }
 
 void imx8mq_tzc380_init(void)
 {
-	enable_tzc380(false);
+	enable_tzc380(false, false);
 }
 
-void imx8mn_tzc380_init(void) __alias(imx8mm_tzc380_init);
-void imx8mp_tzc380_init(void) __alias(imx8mm_tzc380_init);
 void imx8mm_tzc380_init(void)
 {
-	enable_tzc380(true);
+	enable_tzc380(true, false);
+}
+
+void imx8mn_tzc380_init(void) __alias(imx8mp_tzc380_init);
+void imx8mp_tzc380_init(void)
+{
+	enable_tzc380(true, true);
 }
 
 bool tzc380_is_enabled(void)

-- 
2.39.2

Re: [PATCH 1/3] arm: mach-imx: tzasc: lock id_swap_bypass bit

[Ahmad Fatoum]

On 26.02.24 15:40, Stefan Kerkmann wrote:
> This commit ports U-Boot commit 1289ff7bd7e4 ("imx8m: lock
> id_swap_bypass bit in tzc380 enable") to barebox. This is the original
> commit message:
> 
>> According to TRM for i.MX8M Nano and Plus, GPR10 register contains lock
>> bit for TZASC_ID_SWAP_BYPASS bit. This bit is required to be set in
>> order to avoid AXI bus errors when GPU is enabled on the platform.
>> TZASC_ID_SWAP_BYPASS bit is alread set for all imx8m applicable
>> derivatives, but is missing a lock settings to be applied.
>>
>> Set the TZASC_ID_SWAP_BYPASS_LOCK bit for those derivatives which have
>> it implemented.
>>
>> Since we're here, provide also names to bits from TRM instead of using
>> BIT() macro in the code.
> 
> Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  arch/arm/mach-imx/tzasc.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/tzasc.c b/arch/arm/mach-imx/tzasc.c
> index 9c71108c99..1f8d7426c1 100644
> --- a/arch/arm/mach-imx/tzasc.c
> +++ b/arch/arm/mach-imx/tzasc.c
> @@ -5,37 +5,59 @@
>  #include <mach/imx/imx8m-regs.h>
>  #include <io.h>
>  
> -#define GPR_TZASC_EN		BIT(0)
> -#define GPR_TZASC_SWAP_ID	BIT(1)
> -#define GPR_TZASC_EN_LOCK	BIT(16)
> +#define GPR_TZASC_EN					BIT(0)
> +#define GPR_TZASC_ID_SWAP_BYPASS		BIT(1)
> +#define GPR_TZASC_EN_LOCK				BIT(16)
> +#define GPR_TZASC_ID_SWAP_BYPASS_LOCK	BIT(17)
>  
> -static void enable_tzc380(bool bypass_id_swap)
> +#define MX8M_TZASC_REGION_ATTRIBUTES_0		(MX8M_TZASC_BASE_ADDR + 0x108)
> +#define MX8M_TZASC_REGION_ATTRIBUTES_0_SP	GENMASK(31, 28)
> +
> +static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
>  {
>  	u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
>  
>  	/* Enable TZASC and lock setting */
>  	setbits_le32(&gpr[10], GPR_TZASC_EN);
>  	setbits_le32(&gpr[10], GPR_TZASC_EN_LOCK);
> +
> +	/*
> +	 * According to TRM, TZASC_ID_SWAP_BYPASS should be set in
> +	 * order to avoid AXI Bus errors when GPU is in use
> +	 */
>  	if (bypass_id_swap)
> -		setbits_le32(&gpr[10], BIT(1));
> +		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS);
> +
> +	/*
> +	 * imx8mn and imx8mp implements the lock bit for
> +	 * TZASC_ID_SWAP_BYPASS, enable it to lock settings
> +	 */
> +	if (bypass_id_swap_lock)
> +		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS_LOCK);
> +
>  	/*
>  	 * set Region 0 attribute to allow secure and non-secure
>  	 * read/write permission. Found some masters like usb dwc3
>  	 * controllers can't work with secure memory.
>  	 */
> -	writel(0xf0000000, MX8M_TZASC_BASE_ADDR + 0x108);
> +	writel(MX8M_TZASC_REGION_ATTRIBUTES_0_SP,
> +		   MX8M_TZASC_REGION_ATTRIBUTES_0);
>  }
>  
>  void imx8mq_tzc380_init(void)
>  {
> -	enable_tzc380(false);
> +	enable_tzc380(false, false);
>  }
>  
> -void imx8mn_tzc380_init(void) __alias(imx8mm_tzc380_init);
> -void imx8mp_tzc380_init(void) __alias(imx8mm_tzc380_init);
>  void imx8mm_tzc380_init(void)
>  {
> -	enable_tzc380(true);
> +	enable_tzc380(true, false);
> +}
> +
> +void imx8mn_tzc380_init(void) __alias(imx8mp_tzc380_init);
> +void imx8mp_tzc380_init(void)
> +{
> +	enable_tzc380(true, true);
>  }
>  
>  bool tzc380_is_enabled(void)
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH 1/3] arm: mach-imx: tzasc: lock id_swap_bypass bit

[ZHIZHIKIN Andrey]

> -----Original Message-----
> From: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> Sent: Monday, February 26, 2024 3:40 PM
> To: Sascha Hauer <s.hauer@pengutronix.de>; BAREBOX <barebox@lists.infradead.org>
> Cc: Stefan Kerkmann <s.kerkmann@pengutronix.de>; ZHIZHIKIN Andrey
> <andrey.zhizhikin@leica-geosystems.com>
> Subject: [PATCH 1/3] arm: mach-imx: tzasc: lock id_swap_bypass bit
> 
> 
> This commit ports U-Boot commit 1289ff7bd7e4 ("imx8m: lock
> id_swap_bypass bit in tzc380 enable") to barebox. This is the original
> commit message:
> 
> > According to TRM for i.MX8M Nano and Plus, GPR10 register contains lock
> > bit for TZASC_ID_SWAP_BYPASS bit. This bit is required to be set in
> > order to avoid AXI bus errors when GPU is enabled on the platform.
> > TZASC_ID_SWAP_BYPASS bit is alread set for all imx8m applicable
> > derivatives, but is missing a lock settings to be applied.
> >
> > Set the TZASC_ID_SWAP_BYPASS_LOCK bit for those derivatives which have
> > it implemented.
> >
> > Since we're here, provide also names to bits from TRM instead of using
> > BIT() macro in the code.
> 
> Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>

> ---
>  arch/arm/mach-imx/tzasc.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/tzasc.c b/arch/arm/mach-imx/tzasc.c
> index 9c71108c99..1f8d7426c1 100644
> --- a/arch/arm/mach-imx/tzasc.c
> +++ b/arch/arm/mach-imx/tzasc.c
> @@ -5,37 +5,59 @@
>  #include <mach/imx/imx8m-regs.h>
>  #include <io.h>
> 
> -#define GPR_TZASC_EN           BIT(0)
> -#define GPR_TZASC_SWAP_ID      BIT(1)
> -#define GPR_TZASC_EN_LOCK      BIT(16)
> +#define GPR_TZASC_EN                                   BIT(0)
> +#define GPR_TZASC_ID_SWAP_BYPASS               BIT(1)
> +#define GPR_TZASC_EN_LOCK                              BIT(16)
> +#define GPR_TZASC_ID_SWAP_BYPASS_LOCK  BIT(17)
> 
> -static void enable_tzc380(bool bypass_id_swap)
> +#define MX8M_TZASC_REGION_ATTRIBUTES_0         (MX8M_TZASC_BASE_ADDR + 0x108)
> +#define MX8M_TZASC_REGION_ATTRIBUTES_0_SP      GENMASK(31, 28)
> +
> +static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
>  {
>         u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
> 
>         /* Enable TZASC and lock setting */
>         setbits_le32(&gpr[10], GPR_TZASC_EN);
>         setbits_le32(&gpr[10], GPR_TZASC_EN_LOCK);
> +
> +       /*
> +        * According to TRM, TZASC_ID_SWAP_BYPASS should be set in
> +        * order to avoid AXI Bus errors when GPU is in use
> +        */
>         if (bypass_id_swap)
> -               setbits_le32(&gpr[10], BIT(1));
> +               setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS);
> +
> +       /*
> +        * imx8mn and imx8mp implements the lock bit for
> +        * TZASC_ID_SWAP_BYPASS, enable it to lock settings
> +        */
> +       if (bypass_id_swap_lock)
> +               setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS_LOCK);
> +
>         /*
>          * set Region 0 attribute to allow secure and non-secure
>          * read/write permission. Found some masters like usb dwc3
>          * controllers can't work with secure memory.
>          */
> -       writel(0xf0000000, MX8M_TZASC_BASE_ADDR + 0x108);
> +       writel(MX8M_TZASC_REGION_ATTRIBUTES_0_SP,
> +                  MX8M_TZASC_REGION_ATTRIBUTES_0);
>  }
> 
>  void imx8mq_tzc380_init(void)
>  {
> -       enable_tzc380(false);
> +       enable_tzc380(false, false);
>  }
> 
> -void imx8mn_tzc380_init(void) __alias(imx8mm_tzc380_init);
> -void imx8mp_tzc380_init(void) __alias(imx8mm_tzc380_init);
>  void imx8mm_tzc380_init(void)
>  {
> -       enable_tzc380(true);
> +       enable_tzc380(true, false);
> +}
> +
> +void imx8mn_tzc380_init(void) __alias(imx8mp_tzc380_init);
> +void imx8mp_tzc380_init(void)
> +{
> +       enable_tzc380(true, true);
>  }
> 
>  bool tzc380_is_enabled(void)
> 
> --
> 2.39.2

[PATCH 2/3] arm: mach-imx: set cpu type in pbl

[Stefan Kerkmann]

In order to use the `cpu_is_imxxyz` macro family in the pbl,
`__imx_cpu_type` has to be defined and initialized. As we don't have
access to the devicetree at this point, we resort to manual assignment.

Note: It is safe to build the same imx.o object file for both barebox
pbl and proper as the `imx_init` function is discarded during linking as
the whole `init_call` section is not linked into the final binary.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 arch/arm/mach-imx/Makefile | 2 +-
 arch/arm/mach-imx/atf.c    | 5 +++++
 include/mach/imx/generic.h | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index ce8af486ae..a2d9702bf4 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -28,7 +28,7 @@ obj-$(CONFIG_NAND_IMX) += nand.o
 lwl-$(CONFIG_ARCH_IMX_EXTERNAL_BOOT_NAND) += external-nand-boot.o
 obj-y += devices.o imx.o
 obj-$(CONFIG_CMD_BOOTROM) += bootrom-cmd.o
-obj-pbl-y += esdctl.o boot.o
+obj-pbl-y += esdctl.o boot.o imx.o
 obj-$(CONFIG_BAREBOX_UPDATE) += imx-bbu-internal.o
 obj-$(CONFIG_BAREBOX_UPDATE_IMX_EXTERNAL_NAND) += imx-bbu-external-nand.o
 pbl-$(CONFIG_USB_GADGET_DRIVER_ARC_PBL) += imx-udc.o
diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
index 2d8388e8e9..e8060ebd95 100644
--- a/arch/arm/mach-imx/atf.c
+++ b/arch/arm/mach-imx/atf.c
@@ -148,6 +148,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
 	size_t bl31_size;
 	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(32);
 
+	imx_set_cpu_type(IMX_CPU_IMX8MM);
 	imx8mm_init_scratch_space();
 	imx8m_save_bootrom_log();
 	imx8mm_load_bl33(bl33);
@@ -218,6 +219,7 @@ __noreturn void __imx8mp_load_and_start_image_via_tfa(void *bl33)
 	size_t bl31_size;
 	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(32);
 
+	imx_set_cpu_type(IMX_CPU_IMX8MP);
 	imx8mp_init_scratch_space();
 	imx8m_save_bootrom_log();
 	imx8mp_load_bl33(bl33);
@@ -289,6 +291,7 @@ __noreturn void __imx8mn_load_and_start_image_via_tfa(void *bl33)
 	size_t bl31_size;
 	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(16);
 
+	imx_set_cpu_type(IMX_CPU_IMX8MN);
 	imx8mn_init_scratch_space();
 	imx8m_save_bootrom_log();
 	imx8mn_load_bl33(bl33);
@@ -353,6 +356,7 @@ __noreturn void __imx8mq_load_and_start_image_via_tfa(void *bl33)
 	size_t bl31_size;
 	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(32);
 
+	imx_set_cpu_type(IMX_CPU_IMX8MQ);
 	imx8mq_init_scratch_space();
 	imx8m_save_bootrom_log();
 	imx8mq_load_bl33(bl33);
@@ -388,6 +392,7 @@ void __noreturn imx93_load_and_start_image_via_tfa(void)
 	void *bl33 = (void *)MX93_ATF_BL33_BASE_ADDR;
 	unsigned long endmem = MX9_DDR_CSD1_BASE_ADDR + imx9_ddrc_sdram_size();
 
+	imx_set_cpu_type(IMX_CPU_IMX93);
 	imx93_init_scratch_space(true);
 
 	/*
diff --git a/include/mach/imx/generic.h b/include/mach/imx/generic.h
index 1dca335fdd..2b26a1d45a 100644
--- a/include/mach/imx/generic.h
+++ b/include/mach/imx/generic.h
@@ -82,6 +82,11 @@ void imx93_cpu_lowlevel_init(void);
 
 extern unsigned int __imx_cpu_type;
 
+static __always_inline void imx_set_cpu_type(unsigned int cpu_type)
+{
+	__imx_cpu_type = cpu_type;
+}
+
 #ifdef CONFIG_ARCH_IMX1
 # ifdef imx_cpu_type
 #  undef imx_cpu_type

-- 
2.39.2

Re: [PATCH 2/3] arm: mach-imx: set cpu type in pbl

[Ahmad Fatoum]

On 26.02.24 15:40, Stefan Kerkmann wrote:
> In order to use the `cpu_is_imxxyz` macro family in the pbl,
> `__imx_cpu_type` has to be defined and initialized. As we don't have
> access to the devicetree at this point, we resort to manual assignment.
> 
> Note: It is safe to build the same imx.o object file for both barebox
> pbl and proper as the `imx_init` function is discarded during linking as
> the whole `init_call` section is not linked into the final binary.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  arch/arm/mach-imx/Makefile | 2 +-
>  arch/arm/mach-imx/atf.c    | 5 +++++
>  include/mach/imx/generic.h | 5 +++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> index ce8af486ae..a2d9702bf4 100644
> --- a/arch/arm/mach-imx/Makefile
> +++ b/arch/arm/mach-imx/Makefile
> @@ -28,7 +28,7 @@ obj-$(CONFIG_NAND_IMX) += nand.o
>  lwl-$(CONFIG_ARCH_IMX_EXTERNAL_BOOT_NAND) += external-nand-boot.o
>  obj-y += devices.o imx.o
>  obj-$(CONFIG_CMD_BOOTROM) += bootrom-cmd.o
> -obj-pbl-y += esdctl.o boot.o
> +obj-pbl-y += esdctl.o boot.o imx.o
>  obj-$(CONFIG_BAREBOX_UPDATE) += imx-bbu-internal.o
>  obj-$(CONFIG_BAREBOX_UPDATE_IMX_EXTERNAL_NAND) += imx-bbu-external-nand.o
>  pbl-$(CONFIG_USB_GADGET_DRIVER_ARC_PBL) += imx-udc.o
> diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
> index 2d8388e8e9..e8060ebd95 100644
> --- a/arch/arm/mach-imx/atf.c
> +++ b/arch/arm/mach-imx/atf.c
> @@ -148,6 +148,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
>  	size_t bl31_size;
>  	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(32);
>  
> +	imx_set_cpu_type(IMX_CPU_IMX8MM);
>  	imx8mm_init_scratch_space();
>  	imx8m_save_bootrom_log();
>  	imx8mm_load_bl33(bl33);
> @@ -218,6 +219,7 @@ __noreturn void __imx8mp_load_and_start_image_via_tfa(void *bl33)
>  	size_t bl31_size;
>  	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(32);
>  
> +	imx_set_cpu_type(IMX_CPU_IMX8MP);
>  	imx8mp_init_scratch_space();
>  	imx8m_save_bootrom_log();
>  	imx8mp_load_bl33(bl33);
> @@ -289,6 +291,7 @@ __noreturn void __imx8mn_load_and_start_image_via_tfa(void *bl33)
>  	size_t bl31_size;
>  	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(16);
>  
> +	imx_set_cpu_type(IMX_CPU_IMX8MN);
>  	imx8mn_init_scratch_space();
>  	imx8m_save_bootrom_log();
>  	imx8mn_load_bl33(bl33);
> @@ -353,6 +356,7 @@ __noreturn void __imx8mq_load_and_start_image_via_tfa(void *bl33)
>  	size_t bl31_size;
>  	unsigned long endmem = MX8M_DDR_CSD1_BASE_ADDR + imx8m_barebox_earlymem_size(32);
>  
> +	imx_set_cpu_type(IMX_CPU_IMX8MQ);
>  	imx8mq_init_scratch_space();
>  	imx8m_save_bootrom_log();
>  	imx8mq_load_bl33(bl33);
> @@ -388,6 +392,7 @@ void __noreturn imx93_load_and_start_image_via_tfa(void)
>  	void *bl33 = (void *)MX93_ATF_BL33_BASE_ADDR;
>  	unsigned long endmem = MX9_DDR_CSD1_BASE_ADDR + imx9_ddrc_sdram_size();
>  
> +	imx_set_cpu_type(IMX_CPU_IMX93);
>  	imx93_init_scratch_space(true);
>  
>  	/*
> diff --git a/include/mach/imx/generic.h b/include/mach/imx/generic.h
> index 1dca335fdd..2b26a1d45a 100644
> --- a/include/mach/imx/generic.h
> +++ b/include/mach/imx/generic.h
> @@ -82,6 +82,11 @@ void imx93_cpu_lowlevel_init(void);
>  
>  extern unsigned int __imx_cpu_type;
>  
> +static __always_inline void imx_set_cpu_type(unsigned int cpu_type)
> +{
> +	__imx_cpu_type = cpu_type;
> +}
> +
>  #ifdef CONFIG_ARCH_IMX1
>  # ifdef imx_cpu_type
>  #  undef imx_cpu_type
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Stefan Kerkmann]

Instead of passing in configuration parameters at runtime we can utilize
the `cpu_is_mx8xyz` macro family to determine which bits should be set.

As the tzasc driver is imx specific, all functions are prefixed with
`imx8m_` as well.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 arch/arm/mach-imx/atf.c   |  8 ++++----
 arch/arm/mach-imx/imx8m.c |  2 +-
 arch/arm/mach-imx/tzasc.c | 25 +++++--------------------
 include/mach/imx/tzasc.h  |  8 ++------
 4 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
index e8060ebd95..9cbc38ef11 100644
--- a/arch/arm/mach-imx/atf.c
+++ b/arch/arm/mach-imx/atf.c
@@ -158,7 +158,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
 		size_t bl32_size;
 		void *bl32_image;
 
-		imx8mm_tzc380_init();
+		imx8m_tzc380_init();
 		get_builtin_firmware_ext(imx8mm_bl32_bin,
 				bl33, &bl32_image,
 				&bl32_size);
@@ -229,7 +229,7 @@ __noreturn void __imx8mp_load_and_start_image_via_tfa(void *bl33)
 		size_t bl32_size;
 		void *bl32_image;
 
-		imx8mp_tzc380_init();
+		imx8m_tzc380_init();
 		get_builtin_firmware_ext(imx8mp_bl32_bin,
 				bl33, &bl32_image,
 				&bl32_size);
@@ -301,7 +301,7 @@ __noreturn void __imx8mn_load_and_start_image_via_tfa(void *bl33)
 		size_t bl32_size;
 		void *bl32_image;
 
-		imx8mn_tzc380_init();
+		imx8m_tzc380_init();
 		get_builtin_firmware_ext(imx8mn_bl32_bin,
 				bl33, &bl32_image,
 				&bl32_size);
@@ -366,7 +366,7 @@ __noreturn void __imx8mq_load_and_start_image_via_tfa(void *bl33)
 		size_t bl32_size;
 		void *bl32_image;
 
-		imx8mq_tzc380_init();
+		imx8m_tzc380_init();
 		get_builtin_firmware_ext(imx8mq_bl32_bin,
 				bl33, &bl32_image,
 				&bl32_size);
diff --git a/arch/arm/mach-imx/imx8m.c b/arch/arm/mach-imx/imx8m.c
index ed0fe38a3b..ab9e8edb5b 100644
--- a/arch/arm/mach-imx/imx8m.c
+++ b/arch/arm/mach-imx/imx8m.c
@@ -68,7 +68,7 @@ static int imx8m_init(const char *cputypestr)
 	imx_set_reset_reason(src + IMX7_SRC_SRSR, imx7_reset_reasons);
 	pr_info("%s unique ID: %llx\n", cputypestr, imx8m_uid());
 
-	if (IS_ENABLED(CONFIG_PBL_OPTEE) && tzc380_is_enabled()) {
+	if (IS_ENABLED(CONFIG_PBL_OPTEE) && imx8m_tzc380_is_enabled()) {
 		static struct of_optee_fixup_data optee_fixup_data = {
 			.shm_size = OPTEE_SHM_SIZE,
 			.method = "smc",
diff --git a/arch/arm/mach-imx/tzasc.c b/arch/arm/mach-imx/tzasc.c
index 1f8d7426c1..4cb4d7c5cf 100644
--- a/arch/arm/mach-imx/tzasc.c
+++ b/arch/arm/mach-imx/tzasc.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <mach/imx/generic.h>
 #include <mach/imx/tzasc.h>
 #include <linux/bitops.h>
 #include <mach/imx/imx8m-regs.h>
@@ -13,7 +14,7 @@
 #define MX8M_TZASC_REGION_ATTRIBUTES_0		(MX8M_TZASC_BASE_ADDR + 0x108)
 #define MX8M_TZASC_REGION_ATTRIBUTES_0_SP	GENMASK(31, 28)
 
-static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
+void imx8m_tzc380_init(void)
 {
 	u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
 
@@ -25,14 +26,14 @@ static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
 	 * According to TRM, TZASC_ID_SWAP_BYPASS should be set in
 	 * order to avoid AXI Bus errors when GPU is in use
 	 */
-	if (bypass_id_swap)
+	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_mx8mp())
 		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS);
 
 	/*
 	 * imx8mn and imx8mp implements the lock bit for
 	 * TZASC_ID_SWAP_BYPASS, enable it to lock settings
 	 */
-	if (bypass_id_swap_lock)
+	if (cpu_is_mx8mn() || cpu_is_mx8mp())
 		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS_LOCK);
 
 	/*
@@ -44,23 +45,7 @@ static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
 		   MX8M_TZASC_REGION_ATTRIBUTES_0);
 }
 
-void imx8mq_tzc380_init(void)
-{
-	enable_tzc380(false, false);
-}
-
-void imx8mm_tzc380_init(void)
-{
-	enable_tzc380(true, false);
-}
-
-void imx8mn_tzc380_init(void) __alias(imx8mp_tzc380_init);
-void imx8mp_tzc380_init(void)
-{
-	enable_tzc380(true, true);
-}
-
-bool tzc380_is_enabled(void)
+bool imx8m_tzc380_is_enabled(void)
 {
 	u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
 
diff --git a/include/mach/imx/tzasc.h b/include/mach/imx/tzasc.h
index 724ba50ead..51c86f168e 100644
--- a/include/mach/imx/tzasc.h
+++ b/include/mach/imx/tzasc.h
@@ -6,11 +6,7 @@
 #include <linux/types.h>
 #include <asm/system.h>
 
-void imx8mq_tzc380_init(void);
-void imx8mm_tzc380_init(void);
-void imx8mn_tzc380_init(void);
-void imx8mp_tzc380_init(void);
-
-bool tzc380_is_enabled(void);
+void imx8m_tzc380_init(void);
+bool imx8m_tzc380_is_enabled(void);
 
 #endif

-- 
2.39.2

Re: [PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Ahmad Fatoum]

On 26.02.24 15:40, Stefan Kerkmann wrote:
> Instead of passing in configuration parameters at runtime we can utilize
> the `cpu_is_mx8xyz` macro family to determine which bits should be set.
> 
> As the tzasc driver is imx specific, all functions are prefixed with
> `imx8m_` as well.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  arch/arm/mach-imx/atf.c   |  8 ++++----
>  arch/arm/mach-imx/imx8m.c |  2 +-
>  arch/arm/mach-imx/tzasc.c | 25 +++++--------------------
>  include/mach/imx/tzasc.h  |  8 ++------
>  4 files changed, 12 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
> index e8060ebd95..9cbc38ef11 100644
> --- a/arch/arm/mach-imx/atf.c
> +++ b/arch/arm/mach-imx/atf.c
> @@ -158,7 +158,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
>  		size_t bl32_size;
>  		void *bl32_image;
>  
> -		imx8mm_tzc380_init();
> +		imx8m_tzc380_init();
>  		get_builtin_firmware_ext(imx8mm_bl32_bin,
>  				bl33, &bl32_image,
>  				&bl32_size);
> @@ -229,7 +229,7 @@ __noreturn void __imx8mp_load_and_start_image_via_tfa(void *bl33)
>  		size_t bl32_size;
>  		void *bl32_image;
>  
> -		imx8mp_tzc380_init();
> +		imx8m_tzc380_init();
>  		get_builtin_firmware_ext(imx8mp_bl32_bin,
>  				bl33, &bl32_image,
>  				&bl32_size);
> @@ -301,7 +301,7 @@ __noreturn void __imx8mn_load_and_start_image_via_tfa(void *bl33)
>  		size_t bl32_size;
>  		void *bl32_image;
>  
> -		imx8mn_tzc380_init();
> +		imx8m_tzc380_init();
>  		get_builtin_firmware_ext(imx8mn_bl32_bin,
>  				bl33, &bl32_image,
>  				&bl32_size);
> @@ -366,7 +366,7 @@ __noreturn void __imx8mq_load_and_start_image_via_tfa(void *bl33)
>  		size_t bl32_size;
>  		void *bl32_image;
>  
> -		imx8mq_tzc380_init();
> +		imx8m_tzc380_init();
>  		get_builtin_firmware_ext(imx8mq_bl32_bin,
>  				bl33, &bl32_image,
>  				&bl32_size);
> diff --git a/arch/arm/mach-imx/imx8m.c b/arch/arm/mach-imx/imx8m.c
> index ed0fe38a3b..ab9e8edb5b 100644
> --- a/arch/arm/mach-imx/imx8m.c
> +++ b/arch/arm/mach-imx/imx8m.c
> @@ -68,7 +68,7 @@ static int imx8m_init(const char *cputypestr)
>  	imx_set_reset_reason(src + IMX7_SRC_SRSR, imx7_reset_reasons);
>  	pr_info("%s unique ID: %llx\n", cputypestr, imx8m_uid());
>  
> -	if (IS_ENABLED(CONFIG_PBL_OPTEE) && tzc380_is_enabled()) {
> +	if (IS_ENABLED(CONFIG_PBL_OPTEE) && imx8m_tzc380_is_enabled()) {
>  		static struct of_optee_fixup_data optee_fixup_data = {
>  			.shm_size = OPTEE_SHM_SIZE,
>  			.method = "smc",
> diff --git a/arch/arm/mach-imx/tzasc.c b/arch/arm/mach-imx/tzasc.c
> index 1f8d7426c1..4cb4d7c5cf 100644
> --- a/arch/arm/mach-imx/tzasc.c
> +++ b/arch/arm/mach-imx/tzasc.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
> +#include <mach/imx/generic.h>
>  #include <mach/imx/tzasc.h>
>  #include <linux/bitops.h>
>  #include <mach/imx/imx8m-regs.h>
> @@ -13,7 +14,7 @@
>  #define MX8M_TZASC_REGION_ATTRIBUTES_0		(MX8M_TZASC_BASE_ADDR + 0x108)
>  #define MX8M_TZASC_REGION_ATTRIBUTES_0_SP	GENMASK(31, 28)
>  
> -static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
> +void imx8m_tzc380_init(void)
>  {
>  	u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
>  
> @@ -25,14 +26,14 @@ static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
>  	 * According to TRM, TZASC_ID_SWAP_BYPASS should be set in
>  	 * order to avoid AXI Bus errors when GPU is in use
>  	 */
> -	if (bypass_id_swap)
> +	if (cpu_is_mx8mm() || cpu_is_mx8mn() || cpu_is_mx8mp())
>  		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS);
>  
>  	/*
>  	 * imx8mn and imx8mp implements the lock bit for
>  	 * TZASC_ID_SWAP_BYPASS, enable it to lock settings
>  	 */
> -	if (bypass_id_swap_lock)
> +	if (cpu_is_mx8mn() || cpu_is_mx8mp())
>  		setbits_le32(&gpr[10], GPR_TZASC_ID_SWAP_BYPASS_LOCK);
>  
>  	/*
> @@ -44,23 +45,7 @@ static void enable_tzc380(bool bypass_id_swap, bool bypass_id_swap_lock)
>  		   MX8M_TZASC_REGION_ATTRIBUTES_0);
>  }
>  
> -void imx8mq_tzc380_init(void)
> -{
> -	enable_tzc380(false, false);
> -}
> -
> -void imx8mm_tzc380_init(void)
> -{
> -	enable_tzc380(true, false);
> -}
> -
> -void imx8mn_tzc380_init(void) __alias(imx8mp_tzc380_init);
> -void imx8mp_tzc380_init(void)
> -{
> -	enable_tzc380(true, true);
> -}
> -
> -bool tzc380_is_enabled(void)
> +bool imx8m_tzc380_is_enabled(void)
>  {
>  	u32 __iomem *gpr = IOMEM(MX8M_IOMUXC_GPR_BASE_ADDR);
>  
> diff --git a/include/mach/imx/tzasc.h b/include/mach/imx/tzasc.h
> index 724ba50ead..51c86f168e 100644
> --- a/include/mach/imx/tzasc.h
> +++ b/include/mach/imx/tzasc.h
> @@ -6,11 +6,7 @@
>  #include <linux/types.h>
>  #include <asm/system.h>
>  
> -void imx8mq_tzc380_init(void);
> -void imx8mm_tzc380_init(void);
> -void imx8mn_tzc380_init(void);
> -void imx8mp_tzc380_init(void);
> -
> -bool tzc380_is_enabled(void);
> +void imx8m_tzc380_init(void);
> +bool imx8m_tzc380_is_enabled(void);
>  
>  #endif
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Sascha Hauer]

On Mon, Feb 26, 2024 at 03:40:23PM +0100, Stefan Kerkmann wrote:
> Instead of passing in configuration parameters at runtime we can utilize
> the `cpu_is_mx8xyz` macro family to determine which bits should be set.
> 
> As the tzasc driver is imx specific, all functions are prefixed with
> `imx8m_` as well.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> ---
>  arch/arm/mach-imx/atf.c   |  8 ++++----
>  arch/arm/mach-imx/imx8m.c |  2 +-
>  arch/arm/mach-imx/tzasc.c | 25 +++++--------------------
>  include/mach/imx/tzasc.h  |  8 ++------
>  4 files changed, 12 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
> index e8060ebd95..9cbc38ef11 100644
> --- a/arch/arm/mach-imx/atf.c
> +++ b/arch/arm/mach-imx/atf.c
> @@ -158,7 +158,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
>  		size_t bl32_size;
>  		void *bl32_image;
>  
> -		imx8mm_tzc380_init();
> +		imx8m_tzc380_init();

I am not so sure about this patch. So far the whole PBL is coded in the
way that we inherently know the SoC type from the code path chosen.

This patch changes this. It doesn't really matter for this patch, but it
sends a sign how we want to solve this in future.

One implication of this patch is that cpu_is_mx() is a runtime decision,
so code paths behind an unused cpu_is_mx() can't be discarded anymore.

Another thing is that the usage of cpu_is() has the tendency to lead to
cascades of if (cpu_is_x() || cpu_is_y() || cpu_is_z()) which is not
paticularly nice to read.

Both are not really strong points, but on the other hand there's not
much improvement in this patch, so I tend to not take it.

> -bool tzc380_is_enabled(void)
> +bool imx8m_tzc380_is_enabled(void)

This change is good though as the function is clearly i.MX8 specific.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Stefan Kerkmann]

Hi Sascha,

On 27.02.24 09:44, Sascha Hauer wrote:
> On Mon, Feb 26, 2024 at 03:40:23PM +0100, Stefan Kerkmann wrote:
>> Instead of passing in configuration parameters at runtime we can utilize
>> the `cpu_is_mx8xyz` macro family to determine which bits should be set.
>>
>> As the tzasc driver is imx specific, all functions are prefixed with
>> `imx8m_` as well.
>>
>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>> ---
>>   arch/arm/mach-imx/atf.c   |  8 ++++----
>>   arch/arm/mach-imx/imx8m.c |  2 +-
>>   arch/arm/mach-imx/tzasc.c | 25 +++++--------------------
>>   include/mach/imx/tzasc.h  |  8 ++------
>>   4 files changed, 12 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
>> index e8060ebd95..9cbc38ef11 100644
>> --- a/arch/arm/mach-imx/atf.c
>> +++ b/arch/arm/mach-imx/atf.c
>> @@ -158,7 +158,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
>>   		size_t bl32_size;
>>   		void *bl32_image;
>>   
>> -		imx8mm_tzc380_init();
>> +		imx8m_tzc380_init();
> 
> I am not so sure about this patch. So far the whole PBL is coded in the
> way that we inherently know the SoC type from the code path chosen.
> 
> This patch changes this. It doesn't really matter for this patch, but it
> sends a sign how we want to solve this in future.

Let's see if I can persuade you that this is a good thing :-).

> One implication of this patch is that cpu_is_mx() is a runtime decision,
> so code paths behind an unused cpu_is_mx() can't be discarded anymore.

My argument here is that the overhead in code size is probably neglect 
able in most cases, as most of the code paths are still discarded:

1. If there is only one ARCH selected e.g., `CONFIG_ARCH_IMX8MM` the 
`cpu_is_mx8mm()` macro is still evaluated at compile time. As the 
`__imx_cpu_type` variable is only assigned and never read it can be 
stripped away by the compiler/linker and become a nop.

2. Runtime evaluation is only selected if a second arch is enabled for 
the build. But even then the runtime decision is only compiled in for 
the two selected arches, as all other `cpu_is_xyz` macros still evaluate 
at compile time to false. So code paths that don't touch the selected 
arches will still be stripped.

> Another thing is that the usage of cpu_is() has the tendency to lead to
> cascades of if (cpu_is_x() || cpu_is_y() || cpu_is_z()) which is not
> paticularly nice to read.
> 

That is arguably subjective :-).

For me as a developer that is new to barebox, it was confusing to find 
two different styles of arch dependent code. I prefer the `cpu_is_xyz` 
style approach which is used in barebox proper much more.

In the case of the TZC380 driver the pseudo (as they are probably 
optimized away) runtime arguments to the init functions seem 
unnecessarily complicated, as does the approach to define aliases to the 
same function for all arches. The if style is clearer in intend as it 
keeps the distinction between the arches local to the parts that are 
actually different. Which is straight forward to read IMHO.

> Both are not really strong points, but on the other hand there's not
> much improvement in this patch, so I tend to not take it.
> 
>> -bool tzc380_is_enabled(void)
>> +bool imx8m_tzc380_is_enabled(void)
> 
> This change is good though as the function is clearly i.MX8 specific.
> 
> Sascha
> 

Cheers,
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |

Re: [PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Ahmad Fatoum]

Hello Stefan,
Hello Sascha,

On 28.02.24 09:46, Stefan Kerkmann wrote:
>> Another thing is that the usage of cpu_is() has the tendency to lead to
>> cascades of if (cpu_is_x() || cpu_is_y() || cpu_is_z()) which is not
>> paticularly nice to read.
>>
> 
> That is arguably subjective :-).
> 
> For me as a developer that is new to barebox, it was confusing to find two different styles of arch dependent code. I prefer the `cpu_is_xyz` style approach which is used in barebox proper much more.
> 
> In the case of the TZC380 driver the pseudo (as they are probably optimized away) runtime arguments to the init functions seem unnecessarily complicated,

I would suspect they aren't even optimized away, because it's only known
at link-time that the function is only called once, so a late inlining
would require LTO. This means your approach may have a lower overhead
than what we are doing right now... :-)

> as does the approach to define aliases to the same function for all arches. The if style is clearer in intend as it keeps the distinction between the arches local to the parts that are actually different. Which is straight forward to read IMHO.

I also think it's a good idea that in future we should extend to more places
in the i.MX8M init code. The code duplication in ./arch/arm/mach-imx/atf.c
is not a pretty sight.

Cheers,
Ahmad

> 
>> Both are not really strong points, but on the other hand there's not
>> much improvement in this patch, so I tend to not take it.
>>
>>> -bool tzc380_is_enabled(void)
>>> +bool imx8m_tzc380_is_enabled(void)
>>
>> This change is good though as the function is clearly i.MX8 specific.
>>
>> Sascha
>>
> 
> Cheers,
> Stefan
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Sascha Hauer]

On Wed, Feb 28, 2024 at 09:46:51AM +0100, Stefan Kerkmann wrote:
> Hi Sascha,
> 
> On 27.02.24 09:44, Sascha Hauer wrote:
> > On Mon, Feb 26, 2024 at 03:40:23PM +0100, Stefan Kerkmann wrote:
> > > Instead of passing in configuration parameters at runtime we can utilize
> > > the `cpu_is_mx8xyz` macro family to determine which bits should be set.
> > > 
> > > As the tzasc driver is imx specific, all functions are prefixed with
> > > `imx8m_` as well.
> > > 
> > > Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
> > > ---
> > >   arch/arm/mach-imx/atf.c   |  8 ++++----
> > >   arch/arm/mach-imx/imx8m.c |  2 +-
> > >   arch/arm/mach-imx/tzasc.c | 25 +++++--------------------
> > >   include/mach/imx/tzasc.h  |  8 ++------
> > >   4 files changed, 12 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
> > > index e8060ebd95..9cbc38ef11 100644
> > > --- a/arch/arm/mach-imx/atf.c
> > > +++ b/arch/arm/mach-imx/atf.c
> > > @@ -158,7 +158,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
> > >   		size_t bl32_size;
> > >   		void *bl32_image;
> > > -		imx8mm_tzc380_init();
> > > +		imx8m_tzc380_init();
> > 
> > I am not so sure about this patch. So far the whole PBL is coded in the
> > way that we inherently know the SoC type from the code path chosen.
> > 
> > This patch changes this. It doesn't really matter for this patch, but it
> > sends a sign how we want to solve this in future.
> 
> Let's see if I can persuade you that this is a good thing :-).
> 
> > One implication of this patch is that cpu_is_mx() is a runtime decision,
> > so code paths behind an unused cpu_is_mx() can't be discarded anymore.
> 
> My argument here is that the overhead in code size is probably neglect able
> in most cases, as most of the code paths are still discarded:
> 
> 1. If there is only one ARCH selected e.g., `CONFIG_ARCH_IMX8MM` the
> `cpu_is_mx8mm()` macro is still evaluated at compile time. As the
> `__imx_cpu_type` variable is only assigned and never read it can be stripped
> away by the compiler/linker and become a nop.
> 
> 2. Runtime evaluation is only selected if a second arch is enabled for the
> build. But even then the runtime decision is only compiled in for the two
> selected arches, as all other `cpu_is_xyz` macros still evaluate at compile
> time to false. So code paths that don't touch the selected arches will still
> be stripped.
> 
> > Another thing is that the usage of cpu_is() has the tendency to lead to
> > cascades of if (cpu_is_x() || cpu_is_y() || cpu_is_z()) which is not
> > paticularly nice to read.
> > 
> 
> That is arguably subjective :-).
> 
> For me as a developer that is new to barebox, it was confusing to find two
> different styles of arch dependent code. I prefer the `cpu_is_xyz` style
> approach which is used in barebox proper much more.
> 
> In the case of the TZC380 driver the pseudo (as they are probably optimized
> away) runtime arguments to the init functions seem unnecessarily
> complicated, as does the approach to define aliases to the same function for
> all arches. The if style is clearer in intend as it keeps the distinction
> between the arches local to the parts that are actually different. Which is
> straight forward to read IMHO.

Ok, let's see where this brings us. Can you rebase on current next?
Some of the code you are modifying went to drivers/soc/imx/soc-imx8m.c
recently.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] arm: mach-imx: tzasc: convert to cpu_is_mx8xyz macros

[Stefan Kerkmann]

Hi Sascha,

On 28.02.24 12:05, Sascha Hauer wrote:
> On Wed, Feb 28, 2024 at 09:46:51AM +0100, Stefan Kerkmann wrote:
>> Hi Sascha,
>>
>> On 27.02.24 09:44, Sascha Hauer wrote:
>>> On Mon, Feb 26, 2024 at 03:40:23PM +0100, Stefan Kerkmann wrote:
>>>> Instead of passing in configuration parameters at runtime we can utilize
>>>> the `cpu_is_mx8xyz` macro family to determine which bits should be set.
>>>>
>>>> As the tzasc driver is imx specific, all functions are prefixed with
>>>> `imx8m_` as well.
>>>>
>>>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>>>> ---
>>>>   arch/arm/mach-imx/atf.c   |  8 ++++----
>>>>   arch/arm/mach-imx/imx8m.c |  2 +-
>>>>   arch/arm/mach-imx/tzasc.c | 25 +++++--------------------
>>>>   include/mach/imx/tzasc.h  |  8 ++------
>>>>   4 files changed, 12 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-imx/atf.c b/arch/arm/mach-imx/atf.c
>>>> index e8060ebd95..9cbc38ef11 100644
>>>> --- a/arch/arm/mach-imx/atf.c
>>>> +++ b/arch/arm/mach-imx/atf.c
>>>> @@ -158,7 +158,7 @@ __noreturn void __imx8mm_load_and_start_image_via_tfa(void *bl33)
>>>>   		size_t bl32_size;
>>>>   		void *bl32_image;
>>>> -		imx8mm_tzc380_init();
>>>> +		imx8m_tzc380_init();
>>>
>>> I am not so sure about this patch. So far the whole PBL is coded in the
>>> way that we inherently know the SoC type from the code path chosen.
>>>
>>> This patch changes this. It doesn't really matter for this patch, but it
>>> sends a sign how we want to solve this in future.
>>
>> Let's see if I can persuade you that this is a good thing :-).
>>
>>> One implication of this patch is that cpu_is_mx() is a runtime decision,
>>> so code paths behind an unused cpu_is_mx() can't be discarded anymore.
>>
>> My argument here is that the overhead in code size is probably neglect able
>> in most cases, as most of the code paths are still discarded:
>>
>> 1. If there is only one ARCH selected e.g., `CONFIG_ARCH_IMX8MM` the
>> `cpu_is_mx8mm()` macro is still evaluated at compile time. As the
>> `__imx_cpu_type` variable is only assigned and never read it can be stripped
>> away by the compiler/linker and become a nop.
>>
>> 2. Runtime evaluation is only selected if a second arch is enabled for the
>> build. But even then the runtime decision is only compiled in for the two
>> selected arches, as all other `cpu_is_xyz` macros still evaluate at compile
>> time to false. So code paths that don't touch the selected arches will still
>> be stripped.
>>
>>> Another thing is that the usage of cpu_is() has the tendency to lead to
>>> cascades of if (cpu_is_x() || cpu_is_y() || cpu_is_z()) which is not
>>> paticularly nice to read.
>>>
>>
>> That is arguably subjective :-).
>>
>> For me as a developer that is new to barebox, it was confusing to find two
>> different styles of arch dependent code. I prefer the `cpu_is_xyz` style
>> approach which is used in barebox proper much more.
>>
>> In the case of the TZC380 driver the pseudo (as they are probably optimized
>> away) runtime arguments to the init functions seem unnecessarily
>> complicated, as does the approach to define aliases to the same function for
>> all arches. The if style is clearer in intend as it keeps the distinction
>> between the arches local to the parts that are actually different. Which is
>> straight forward to read IMHO.
> 
> Ok, let's see where this brings us. Can you rebase on current next?
> Some of the code you are modifying went to drivers/soc/imx/soc-imx8m.c
> recently.
> 

Great :-)! I've rebased and sent a v2 of this patch set.

> Sascha
> 

Cheers,
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |