Add NXP's i.MX8QX and i.MX8QM SoC support #1410

Anson-Huang · 2018-06-11T07:13:19Z

This pull request adds NXP's i.MX8QX and i.MX8QM SoC support, both of them are ARMv8 SoC, i.MX8QX is with 4 Cortex-A35 and 1 system controller (Cortext-M4), and i.MX8QM is with 2 Cortex-A72, 4 Cortex-A53 and 1 system controller (Cortext-M4).
This pull request adds basic SMP Linux kernel boot up support and debug lpuart support for them.

ssg-bot · 2018-06-11T07:14:02Z

Can one of the admins verify this patch?

ghost · 2018-06-11T08:13:10Z

IP Review +1

afaerber · 2018-06-12T11:05:25Z

Please add documentation for how to build and deploy to docs/plat/.

afaerber · 2018-06-12T11:11:34Z

@antonio-nino-diaz-arm This pull is against master branch, shouldn't it go into integration branch?

paulmenzel · 2018-06-12T15:11:19Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+
+#if DEBUG_CONSOLE_A35
+static void lpuart32_serial_setbrg(unsigned int base, int baudrate)
+{


Add a check that, baudrate is not zero?

Thanks, will add check.

ghost · 2018-06-12T15:25:11Z

Update the maintainers.rst file as well.

ghost

You need to add documentation and update the readme.rst and maintainers.rst files.

paulmenzel

Thank you for pushing this upstream.

In the commit messages it’d be great if you added a space before the opening brackets.
Is there any public documentation available? If yes, please mention that somewhere in the commit messages.
Could the assembler stuff be written in C?
Please add to the commit message, what Linux kernel version you used for testing.
How can this be built and tested?

paulmenzel · 2018-06-12T15:11:49Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+		if (tmp_sbr == 0)
+			tmp_sbr = 1;
+
+		/* calculate difference in actual buad w/ current values */


s/buad/baud/

paulmenzel · 2018-06-12T15:24:52Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+		tmp_diff = tmp_diff - baudrate;
+
+		/* select best values between sbr and sbr+1 */
+		if (tmp_diff > (baudrate - (rate / (tmp_osr * (tmp_sbr + 1))))) {


Use a separate variable for the calculation, and use it in the body too?

paulmenzel · 2018-06-12T15:25:20Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+{
+	unsigned int tmp;
+
+	/*disable TX & RX before enabling clocks */


Please add a space after the /*.

paulmenzel · 2018-06-12T15:26:29Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+		ERROR("sc_rm_move_all: %u\n", err);
+
+	/* iterate through peripherals to give NS OS part access */
+	for (i = 0; i < (sizeof(ns_access_allowed) / sizeof(sc_rsrc_t)); i++) {


Is there an ARRAY_SIZE macro?

paulmenzel · 2018-06-12T15:27:31Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+				}
+			}
+		}
+	}


Can you refactor the body into a separate function, so that the indentation level is decreased.

I remove the else statement to reduce the indentation level, since we do NOT expect any error here, if there is any error, the debug print message will show and we need to fix.

paulmenzel · 2018-06-12T15:28:03Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+				}
+			}
+		}
+	}


Same as upper, you can help review new pull request, thanks.

paulmenzel · 2018-06-12T15:29:00Z

plat/imx/imx8qx/imx8qx_bl31_setup.c

+	imx8_partition_resources();
+
+	/*
+	 * tell BL3-1 where the non-secure software image is located


Below bl33_image_… functions are used. Does that match the BL3-1.

paulmenzel · 2018-06-12T15:29:54Z

plat/imx/common/mxcuart_console.S

+#include <asm_macros.S>
+
+/* UART Control Register Bit Fields.*/
+#define  URXD_CHARRDY    (1<<15)


In other files spaces are put around the operator.

paulmenzel · 2018-06-12T15:30:45Z

plat/imx/common/plat_imx8_gic.c

+{
+	/*
+	 * the GICv3 driver is initialized in EL3 and does not need
+	 * to be initialized again in SEL1. This is because the S-EL1


Inconsistent spelling of SEL1/S-EL1.

paulmenzel · 2018-06-12T15:31:25Z

plat/imx/common/include/plat_macros.S

+ */
+
+/*
+ * the below macro print out relevant GIC


Either macros print or macro prints. (Also below.)

I would like to keep assembler stuff for now, since it is same as our internal code, if we have any optimization later, I will update in upstream as well. Otherwise
I addressed all the comments, please help review new pull request, thanks.

paulmenzel · 2018-06-14T14:43:37Z

@Anson-Huang, thank you for the update. The changes look good. To make it perfect I’d improve the commit messages too, but it’s not my call.

Thank you for writing the documentation.

soby-mathew · 2018-06-15T14:11:13Z

plat/imx/imx8qx/platform.mk

+PLAT_INCLUDES		:=	-Iplat/imx/imx8qx/include		\
+				-Iplat/imx/common/include		\
+
+PLAT_GIC_SOURCES	:=	drivers/arm/gic/v3/gicv3_helpers.c	\


Suggest to prefix internal variables with IMX_ as PLAT_ is usually reserved exporting values from platform for generic makefile use.

Thanks, I replace the PLAT_GIC_SOURCES with IMX_GIC_SOURCES.

soby-mathew · 2018-06-15T14:15:13Z

plat/imx/common/mxcuart_console.S

+#include <asm_macros.S>
+
+/* UART Control Register Bit Fields.*/
+#define  URXD_CHARRDY    (1 << 15)


Would prefer if the defines here can move to a header file?

I removed the mxcuart support, the mxcuart is another UART IP rather than LPUART, i.MX8QX/QM are using LPUART ONLY, mxcuart will be added when other i.MX8 platforms added which use it.

soby-mathew · 2018-06-15T14:15:42Z

plat/imx/common/lpuart_console.S

+#include <assert_macros.S>
+#include "lpuart.h"
+
+#define UARTBAUD	0x10


Can these defines here move to lpuart.h ?

Done, thanks.

soby-mathew · 2018-06-15T14:16:28Z

plat/imx/common/include/iomux.h

@@ -0,0 +1,25 @@
+/*


suggest file name as imx8_io_mux.h

Similar comments for all the headers in this folder because this is being included in the PLAT_INCLUDES path

soby-mathew · 2018-06-15T15:22:36Z

plat/imx/common/include/sci/types.h

@@ -0,0 +1,849 @@
+/*


prefer the file name to be sci_types.h

soby-mathew · 2018-06-15T15:23:20Z

plat/imx/common/sci/svc/pad/rpc.h

@@ -0,0 +1,66 @@
+/*


prefer sci_pad_rpc.h as file name

soby-mathew · 2018-06-15T15:24:16Z

plat/imx/common/sci/svc/pm/rpc.h

@@ -0,0 +1,71 @@
+/*


Similar comment for filename

soby-mathew · 2018-06-15T15:25:49Z

plat/imx/common/sci/svc/rm/rpc.h

@@ -0,0 +1,81 @@
+/*


suggest sc_rm_rpc.h as header name.

Done, I improve all these rph.c to sci_xx_rpc.h

NXP's i.MX8 SoCs have system controller (M4 core) which takes control of clock management, power management, partition management, PAD management etc., other clusters like Cortex-A35 can send out command via MU (Message Unit) to system controller for clock/power management etc.. This patch adds basic IPC(inter-processor communication) support. Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Add NXP's imx SoC debug uart driver. Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

This patch adds support for NXP's imx SoC common function support like topology, gic implementation. Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

NXP's i.MX8QX is an ARMv8 SoC with 4 Cortex-A35 cores and system controller (Cortex-M4) inside, documentation can be found in below link: https://www.nxp.com/products/processors-and-microcontrollers/ applications-processors/i.mx-applications-processors/i.mx-8-processors:IMX8-SERIES This patch adds support for booting up SMP linux kernel (v4.9). Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

NXP's i.MX8QM is an ARMv8 SoC with 2 clusters, 2 Cortex-A72 cores in one cluster and 4 Cortex-A53 in the other cluster, and also has system controller (Cortex-M4) inside, documentation can be found in below link: https://www.nxp.com/products/processors-and-microcontrollers/ applications-processors/i.mx-applications-processors/i.mx-8-processors:IMX8-SERIES This patch adds support for booting up SMP linux kernel (v4.9). Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

soby-mathew

Thanks for the rework. Looks good to me.

ghost approved these changes Jun 11, 2018

View reviewed changes

danh-arm changed the base branch from master to integration June 12, 2018 11:46

ghost mentioned this pull request Jun 12, 2018

TI K3 platform support #1403

Merged

paulmenzel reviewed Jun 12, 2018

View reviewed changes

ghost suggested changes Jun 12, 2018

View reviewed changes

paulmenzel reviewed Jun 12, 2018

View reviewed changes

Anson-Huang force-pushed the master branch from c3faa65 to c80d0de Compare June 13, 2018 03:28

ghost approved these changes Jun 14, 2018

View reviewed changes

soby-mathew requested changes Jun 15, 2018

View reviewed changes

Anson-Huang added 5 commits June 19, 2018 10:24

Support for NXP's imx SoC debug uart

27b9d5e

Add NXP's imx SoC debug uart driver. Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Support for NXP's imx SoC common function

bd08def

This patch adds support for NXP's imx SoC common function support like topology, gic implementation. Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Anson-Huang force-pushed the master branch from c80d0de to baa7650 Compare June 19, 2018 02:31

soby-mathew approved these changes Jun 19, 2018

View reviewed changes

ghost merged commit d135ad7 into ARM-software:integration Jun 19, 2018

This pull request was closed.

Add NXP's i.MX8QX and i.MX8QM SoC support #1410

Add NXP's i.MX8QX and i.MX8QM SoC support #1410

Conversation

Anson-Huang commented Jun 11, 2018

ssg-bot commented Jun 11, 2018

ghost commented Jun 11, 2018

afaerber commented Jun 12, 2018

afaerber commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 12, 2018

ghost left a comment

Choose a reason for hiding this comment

paulmenzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmenzel commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soby-mathew left a comment

Choose a reason for hiding this comment