Skip to contentSkip to conversation

arm64: rk3399: enable SPL with ATF support

RFC PATCH Superseded Kever Yang 4 participants 15 messages Dec 29, 2016 – Jan 28, 2017

Subsystems (6)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Kever Yang

author
#0 lore raw

cover arm64: rk3399: enable SPL with ATF support

RK3399 SPL is not enable bacause of the lack of ATF support in SPL, after port some source code from ATF, I manage to enable the support for ATF in SPL. This patch set depends on some patch for SPL support multi binary in FIT which is from Andre.

The patch of sdram controller is still not clean, because I want to get comments from upstream.I port this driver from coreboot, and I don't want to make much change on it. The rk3399 dram controller has many registers to config, I want to make them directly used by driver instead of parse and copy them one by one from dts.

Another part of comment is welcome for the implementation of spl_atf.c and the entry in spl.c

Kever Yang (3):
arm64: rk3399: add SPL support
arm64: rk3399: add ddr controller driver
spl: add support to booting with ATF

 arch/arm/Kconfig                                   |    1 +
 arch/arm/dts/rk3399.dtsi                           |   24 +
 arch/arm/include/asm/arch-rockchip/grf_rk3399.h    |  118 ++
 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h  |  188 +++
 arch/arm/mach-rockchip/Kconfig                     |    2 +
 arch/arm/mach-rockchip/Makefile                    |    1 +
 arch/arm/mach-rockchip/rk3399-board-spl.c          |  157 ++
 arch/arm/mach-rockchip/rk3399/Makefile             |    1 +
 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc | 1565 ++++++++++++++++++++
 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c       | 1121 ++++++++++++++
 common/spl/Kconfig                                 |   14 +
 common/spl/Makefile                                |    1 +
 common/spl/spl.c                                   |    4 +
 common/spl/spl_atf.c                               |   91 ++
 configs/evb-rk3399_defconfig                       |   17 +
 drivers/clk/rockchip/clk_rk3399.c                  |   42 +-
 drivers/pinctrl/rockchip/pinctrl_rk3399.c          |  106 --
 include/atf_common.h                               |  295 ++++
 include/configs/rk3399_common.h                    |   11 +
 include/dt-bindings/clock/rk3399-cru.h             |   16 +-
 include/spl.h                                      |    1 +
 21 files changed, 3663 insertions(+), 113 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h
 create mode 100644 arch/arm/mach-rockchip/rk3399-board-spl.c
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c
 create mode 100644 common/spl/spl_atf.c
 create mode 100644 include/atf_common.h
Signature · 1 line
1.9.1
author
#1 lore raw

RFC PATCH 1/3 arm64: rk3399: add SPL support

Add spl support for rk3399.

---

 arch/arm/Kconfig                                |   1 +
 arch/arm/dts/rk3399.dtsi                        |  24 ++++
 arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 118 ++++++++++++++++++
 arch/arm/mach-rockchip/Kconfig                  |   2 +
 arch/arm/mach-rockchip/Makefile                 |   1 +
 arch/arm/mach-rockchip/rk3399-board-spl.c       | 157 ++++++++++++++++++++++++
 configs/evb-rk3399_defconfig                    |  17 +++
 drivers/clk/rockchip/clk_rk3399.c               |  42 ++++++-
 drivers/pinctrl/rockchip/pinctrl_rk3399.c       | 106 ----------------
 include/configs/rk3399_common.h                 |  11 ++
 include/dt-bindings/clock/rk3399-cru.h          |  16 ++-
 11 files changed, 382 insertions(+), 113 deletions(-)
 create mode 100644 arch/arm/mach-rockchip/rk3399-board-spl.c
arch/arm/Kconfig +1 -0
index d871a45..9a0efe4 100644
@@ -882,6 +882,7 @@ config ARCH_ROCKCHIP
 	select DM
 	select SPL_DM if SPL
 	select SYS_MALLOC_F
+	select SPL_SEPARATE_BSS if SPL
 	select SPL_SYS_MALLOC_SIMPLE if SPL
 	select DM_GPIO
 	select DM_I2C
arch/arm/dts/rk3399.dtsi +24 -0
index 22277ff..c57fa5bc 100644
@@ -183,6 +183,7 @@
 	};
 
 	sdhci: sdhci at fe330000 {
+		u-boot,dm-pre-reloc;
 		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
 		reg = <0x0 0xfe330000 0x0 0x10000>;
 		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
@@ -416,6 +417,7 @@
 	};
 
 	pmugrf: syscon at ff320000 {
+		u-boot,dm-pre-reloc;
 		compatible = "rockchip,rk3399-pmugrf", "syscon", "simple-mfd";
 		reg = <0x0 0xff320000 0x0 0x1000>;
 		#address-cells = <1>;
@@ -497,7 +499,26 @@
 		status = "disabled";
 	};
 
+	dfi: dfi at ff630000 {
+		reg = <0x00 0xff630000 0x00 0x4000>;
+		compatible = "rockchip,rk3399-dfi";
+		rockchip,pmu = <&pmugrf>;
+		clocks = <&cru PCLK_DDR_MON>;
+		clock-names = "pclk_ddr_mon";
+		status = "disabled";
+	};
+
+	dmc: dmc {
+		u-boot,dm-pre-reloc;
+		compatible = "rockchip,rk3399-dmc";
+		devfreq-events = <&dfi>;
+		interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru SCLK_DDRCLK>;
+		clock-names = "dmc_clk";
+	};
+
 	pmucru: pmu-clock-controller at ff750000 {
+		u-boot,dm-pre-reloc;
 		compatible = "rockchip,rk3399-pmucru";
 		reg = <0x0 0xff750000 0x0 0x1000>;
 		#clock-cells = <1>;
@@ -507,6 +528,7 @@
 	};
 
 	cru: clock-controller at ff760000 {
+		u-boot,dm-pre-reloc;
 		compatible = "rockchip,rk3399-cru";
 		reg = <0x0 0xff760000 0x0 0x1000>;
 		#clock-cells = <1>;
@@ -530,6 +552,7 @@
 	};
 
 	grf: syscon at ff770000 {
+		u-boot,dm-pre-reloc;
 		compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd";
 		reg = <0x0 0xff770000 0x0 0x10000>;
 		#address-cells = <1>;
@@ -607,6 +630,7 @@
 	};
 
 	pinctrl: pinctrl {
+		u-boot,dm-pre-reloc;
 		compatible = "rockchip,rk3399-pinctrl";
 		rockchip,grf = <&grf>;
 		rockchip,pmu = <&pmugrf>;
arch/arm/include/asm/arch-rockchip/grf_rk3399.h +118 -0

122 lines not on this page · raw message

arch/arm/mach-rockchip/Kconfig +2 -0
index 5c4a4c2..cd8fef8 100644
@@ -26,6 +26,8 @@ config ROCKCHIP_RK3288
 config ROCKCHIP_RK3399
 	bool "Support Rockchip RK3399"
 	select ARM64
+	select SUPPORT_SPL
+	select SPL
 	help
 	  The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72
 	  and quad-core Cortex-A53.
arch/arm/mach-rockchip/Makefile +1 -0
index 6e79fed..b58c02d 100644
@@ -7,6 +7,7 @@
 ifdef CONFIG_SPL_BUILD
 obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board-spl.o
 obj-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-spl.o
+obj-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board-spl.o
 obj-$(CONFIG_ROCKCHIP_SPL_BACK_TO_BROM) += save_boot_param.o
 else
 obj-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board.o
arch/arm/mach-rockchip/rk3399-board-spl.c new file +157 -0

157 lines not on this page · raw message

configs/evb-rk3399_defconfig +17 -0
index 40a8295..78c2846 100644
@@ -3,7 +3,16 @@ CONFIG_ARCH_ROCKCHIP=y
 CONFIG_ROCKCHIP_RK3399=y
 CONFIG_DEFAULT_DEVICE_TREE="rk3399-evb"
 CONFIG_FIT=y
+CONFIG_SPL_LOAD_FIT=y
+CONFIG_SPL_OF_LIBFDT=y
+CONFIG_SPL_ATF_SUPPORT=y
+CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x200
+CONFIG_SPL_ATF_TEXT_BASE=0x00010000
 # CONFIG_DISPLAY_CPUINFO is not set
+CONFIG_SPL_STACK_R=y
+CONFIG_SPL_STACK_R_ADDR=0x80000
+CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x4000
+CONFIG_SYS_MALLOC_F_LEN=0x4000
 CONFIG_HUSH_PARSER=y
 CONFIG_CMD_BOOTZ=y
 # CONFIG_CMD_IMLS is not set
@@ -16,18 +25,26 @@ CONFIG_CMD_EXT2=y
 CONFIG_CMD_EXT4=y
 CONFIG_CMD_FAT=y
 CONFIG_CMD_FS_GENERIC=y
+CONFIG_CMD_PXE=y
+CONFIG_SPL_OF_CONTROL=y
+CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
 CONFIG_REGMAP=y
+CONFIG_SPL_REGMAP=y
 CONFIG_SYSCON=y
+CONFIG_SPL_SYSCON=y
 CONFIG_CLK=y
+CONFIG_SPL_CLK=y
 CONFIG_ROCKCHIP_GPIO=y
 CONFIG_ROCKCHIP_DWMMC=y
 CONFIG_ROCKCHIP_SDHCI=y
 CONFIG_PINCTRL=y
+CONFIG_SPL_PINCTRL=y
 CONFIG_ROCKCHIP_RK3399_PINCTRL=y
 CONFIG_PWM_ROCKCHIP=y
 CONFIG_REGULATOR_PWM=y
 CONFIG_DM_REGULATOR_FIXED=y
 CONFIG_RAM=y
+CONFIG_SPL_RAM=y
 CONFIG_DEBUG_UART=y
 CONFIG_DEBUG_UART_BASE=0xFF1A0000
 CONFIG_DEBUG_UART_CLOCK=24000000
drivers/clk/rockchip/clk_rk3399.c +41 -1

60 lines not on this page · raw message

drivers/pinctrl/rockchip/pinctrl_rk3399.c +0 -106

112 lines not on this page · raw message

include/configs/rk3399_common.h +11 -0
index aa646c6..3699a9d 100644
@@ -17,12 +17,23 @@
 #define CONFIG_SYS_MALLOC_LEN		(32 << 20)
 #define CONFIG_SYS_CBSIZE		1024
 #define CONFIG_SKIP_LOWLEVEL_INIT
+#define CONFIG_SPL_FRAMEWORK
+#define CONFIG_SPL_DRIVERS_MISC_SUPPORT
+#define CONFIG_SPL_LIBCOMMON_SUPPORT
+#define CONFIG_SPL_LIBGENERIC_SUPPORT
+#define CONFIG_SPL_SERIAL_SUPPORT
 
 #define CONFIG_SYS_NS16550_MEM32
 
 #define CONFIG_SYS_TEXT_BASE		0x00200000
 #define CONFIG_SYS_INIT_SP_ADDR		0x00300000
 #define CONFIG_SYS_LOAD_ADDR		0x00800800
+#define CONFIG_SPL_STACK		0xff8effff
+#define CONFIG_SPL_TEXT_BASE		0xff8c2008
+#define CONFIG_SPL_MAX_SIZE		0x30000
+/*  BSS setup */
+#define CONFIG_SPL_BSS_START_ADDR       0xff8e0000
+#define CONFIG_SPL_BSS_MAX_SIZE         0x10000
 
 #define CONFIG_SYS_BOOTM_LEN	(64 << 20)	/* 64M */
 
include/dt-bindings/clock/rk3399-cru.h +10 -6
index 0a86aec..d4bdcc6 100644
@@ -122,6 +122,10 @@
 #define SCLK_DPHY_RX0_CFG		165
 #define SCLK_RMII_SRC			166
 #define SCLK_PCIEPHY_REF100M		167
+#define SCLK_USBPHY0_480M_SRC		168
+#define SCLK_USBPHY1_480M_SRC		169
+#define SCLK_DDRCLK			170
+#define SCLK_TESTOUT2			171
 
 #define DCLK_VOP0			180
 #define DCLK_VOP1			181
@@ -589,13 +593,13 @@
 #define SRST_P_SPI0			214
 #define SRST_P_SPI1			215
 #define SRST_P_SPI2			216
-#define SRST_P_SPI3			217
-#define SRST_P_SPI4			218
+#define SRST_P_SPI4			217
+#define SRST_P_SPI5			218
 #define SRST_SPI0			219
 #define SRST_SPI1			220
 #define SRST_SPI2			221
-#define SRST_SPI3			222
-#define SRST_SPI4			223
+#define SRST_SPI4			222
+#define SRST_SPI5			223
 
 /* cru_softrst_con14 */
 #define SRST_I2S0_8CH			224
@@ -717,8 +721,8 @@
 #define SRST_H_CM0S_NOC			3
 #define SRST_DBG_CM0S			4
 #define SRST_PO_CM0S			5
-#define SRST_P_SPI6			6
-#define SRST_SPI6			7
+#define SRST_P_SPI3			6
+#define SRST_SPI3			7
 #define SRST_P_TIMER_0_1		8
 #define SRST_P_TIMER_0			9
 #define SRST_P_TIMER_1			10
Signature · 1 line
1.9.1
#2 lore raw

RFC PATCH 1/3 arm64: rk3399: add SPL support

Hi Kever,

Quoted 5 lines · Kever Yang

Add spl support for rk3399.

---

Quoted 13 lines
 arch/arm/Kconfig                                |   1 +
 arch/arm/dts/rk3399.dtsi                        |  24 ++++
 arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 118 ++++++++++++++++++
 arch/arm/mach-rockchip/Kconfig                  |   2 +
 arch/arm/mach-rockchip/Makefile                 |   1 +
 arch/arm/mach-rockchip/rk3399-board-spl.c       | 157 ++++++++++++++++++++++++
 configs/evb-rk3399_defconfig                    |  17 +++
 drivers/clk/rockchip/clk_rk3399.c               |  42 ++++++-
 drivers/pinctrl/rockchip/pinctrl_rk3399.c       | 106 ----------------
 include/configs/rk3399_common.h                 |  11 ++
 include/dt-bindings/clock/rk3399-cru.h          |  16 ++-
 11 files changed, 382 insertions(+), 113 deletions(-)
 create mode 100644 arch/arm/mach-rockchip/rk3399-board-spl.c

The code looks fine. But can you please split this into 3 patches?

- move grf register definitions to grf_rk3399.h
- add clock driver
- add SPL support

Also check your header #ifdef guards - they should not have SOC in them.

Regards,
Simon

author
#3 lore raw

RFC PATCH 2/3 arm64: rk3399: add ddr controller driver

In reply to Kever Yang

RK3399 support DDR3, LPDDR3, DDR4 sdram, this patch is porting from coreboot, support 4GB lpddr3 in this version.

---

 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h  |  188 +++
 arch/arm/mach-rockchip/rk3399/Makefile             |    1 +
 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc | 1565 ++++++++++++++++++++
 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c       | 1121 ++++++++++++++
 4 files changed, 2875 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +188 -0
new file mode 100644
index 0000000..fab0faf
@@ -0,0 +1,188 @@
+/*
+ * Copyright (C) 2015 Rockchip Electronics Co., Ltd
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#ifndef __SOC_ROCKCHIP_RK3399_SDRAM_H__
+#define __SOC_ROCKCHIP_RK3399_SDRAM_H__
+
+#include <stddef.h>
+
+enum {
+	DDR3 = 3,
+	LPDDR2 = 5,
+	LPDDR3 = 6,
+	LPDDR4 = 7,
+	UNUSED = 0xFF
+};
+
+struct rk3399_ddr_pctl_regs {
+	u32 denali_ctl[332];
+};
+
+struct rk3399_ddr_publ_regs {
+	u32 denali_phy[959];
+};
+
+struct rk3399_ddr_pi_regs {
+	u32 denali_pi[200];
+};
+union noc_ddrtiminga0 {
+	u32 d32;
+	struct {
+		unsigned acttoact : 6;
+		unsigned reserved0 : 2;
+		unsigned rdtomiss : 6;
+		unsigned reserved1 : 2;
+		unsigned wrtomiss : 6;
+		unsigned reserved2 : 2;
+		unsigned readlatency : 8;
+	} b;
+};
+
+union noc_ddrtimingb0 {
+	u32 d32;
+	struct {
+		unsigned rdtowr : 5;
+		unsigned reserved0 : 3;
+		unsigned wrtord : 5;
+		unsigned reserved1 : 3;
+		unsigned rrd : 4;
+		unsigned reserved2 : 4;
+		unsigned faw : 6;
+		unsigned reserved3 : 2;
+	} b;
+};
+
+union noc_ddrtimingc0 {
+	u32 d32;
+	struct {
+		unsigned burstpenalty : 4;
+		unsigned reserved0 : 4;
+		unsigned wrtomwr : 6;
+		unsigned reserved1 : 18;
+	} b;
+};
+
+union noc_devtodev0 {
+	u32 d32;
+	struct {
+		unsigned busrdtord : 3;
+		unsigned reserved0 : 1;
+		unsigned busrdtowr : 3;
+		unsigned reserved1 : 1;
+		unsigned buswrtord : 3;
+		unsigned reserved2 : 1;
+		unsigned buswrtowr : 3;
+		unsigned reserved3 : 17;
+	} b;
+};
+
+union noc_ddrmode {
+	u32 d32;
+	struct {
+		unsigned autoprecharge : 1;
+		unsigned bypassfiltering : 1;
+		unsigned fawbank : 1;
+		unsigned burstsize : 2;
+		unsigned mwrsize : 2;
+		unsigned reserved2 : 1;
+		unsigned forceorder : 8;
+		unsigned forceorderstate : 8;
+		unsigned reserved3 : 8;
+	} b;
+};
+
+struct rk3399_msch_regs {
+	u32 coreid;
+	u32 revisionid;
+	u32 ddrconf;
+	u32 ddrsize;
+	union noc_ddrtiminga0 ddrtiminga0;
+	union noc_ddrtimingb0 ddrtimingb0;
+	union noc_ddrtimingc0 ddrtimingc0;
+	union noc_devtodev0 devtodev0;
+	u32 reserved0[(0x110-0x20)/4];
+	union noc_ddrmode ddrmode;
+	u32 reserved1[(0x1000-0x114)/4];
+	u32 agingx0;
+};
+
+struct rk3399_msch_timings {
+	union noc_ddrtiminga0 ddrtiminga0;
+	union noc_ddrtimingb0 ddrtimingb0;
+	union noc_ddrtimingc0 ddrtimingc0;
+	union noc_devtodev0 devtodev0;
+	union noc_ddrmode ddrmode;
+	u32 agingx0;
+};
+
+struct rk3399_ddr_cic_regs {
+	u32 cic_ctrl0;
+	u32 cic_ctrl1;
+	u32 cic_idle_th;
+	u32 cic_cg_wait_th;
+	u32 cic_status0;
+	u32 cic_status1;
+	u32 cic_ctrl2;
+	u32 cic_ctrl3;
+	u32 cic_ctrl4;
+};
+
+/* DENALI_CTL_00 */
+#define START		(1)
+
+/* DENALI_CTL_68 */
+#define PWRUP_SREFRESH_EXIT	(1 << 16)
+
+/* DENALI_CTL_274 */
+#define MEM_RST_VALID	(1)
+
+struct rk3399_sdram_channel {
+	unsigned char rank;
+	/* col = 0, means this channel is invalid */
+	unsigned char col;
+	/* 3:8bank, 2:4bank */
+	unsigned char bk;
+	/* channel buswidth, 2:32bit, 1:16bit, 0:8bit */
+	unsigned char bw;
+	/* die buswidth, 2:32bit, 1:16bit, 0:8bit */
+	unsigned char dbw;
+	/* row_3_4 = 1: 6Gb or 12Gb die
+	 * row_3_4 = 0: normal die, power of 2
+	 */
+	unsigned char row_3_4;
+	unsigned char cs0_row;
+	unsigned char cs1_row;
+	unsigned int ddrconfig;
+	struct rk3399_msch_timings noc_timings;
+};
+
+struct rk3399_sdram_params {
+	struct rk3399_sdram_channel ch[2];
+	unsigned int ddr_freq;
+	unsigned char dramtype;
+	unsigned char num_channels;
+	unsigned char stride;
+	unsigned char odt;
+	/* align 8 byte */
+	struct rk3399_ddr_pctl_regs pctl_regs;
+	/* align 8 byte */
+	struct rk3399_ddr_pi_regs pi_regs;
+	/* align 8 byte */
+	struct rk3399_ddr_publ_regs phy_regs;
+	/* used for align 8byte for next struct */
+	unsigned int align_8;
+};
+
+#define PI_CA_TRAINING	(1 << 0)
+#define PI_WRITE_LEVELING	(1 << 1)
+#define PI_READ_GATE_TRAINING	(1 << 2)
+#define PI_READ_LEVELING	(1 << 3)
+#define PI_WDQ_LEVELING	(1 << 4)
+#define PI_FULL_TRAINING	(0xff)
+
+size_t sdram_size_mb(void);
+
+#endif
arch/arm/mach-rockchip/rk3399/Makefile +1 -0
index 98ebeac..437d851 100644
@@ -7,3 +7,4 @@
 obj-y += clk_rk3399.o
 obj-y += rk3399.o
 obj-y += syscon_rk3399.o
+obj-y += sdram_rk3399.o
arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc new file +1565 -0

1,565 lines not on this page · raw message

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +1121 -0

1,121 lines not on this page · raw message

Signature · 1 line
1.9.1
#4 lore raw

RFC PATCH 2/3 arm64: rk3399: add ddr controller driver

Hi Kever,

Quoted 21 lines · Kever Yang

RK3399 support DDR3, LPDDR3, DDR4 sdram, this patch is porting from coreboot, support 4GB lpddr3 in this version.

---

 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h  |  188 +++
 arch/arm/mach-rockchip/rk3399/Makefile             |    1 +
 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc | 1565 ++++++++++++++++++++
 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c       | 1121 ++++++++++++++
 4 files changed, 2875 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +0 -0
new file mode 100644
index 0000000..fab0faf

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 8

Quoted 8 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +8 -0
@@ -0,0 +1,188 @@
+/*
+ * Copyright (C) 2015 Rockchip Electronics Co., Ltd
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#ifndef __SOC_ROCKCHIP_RK3399_SDRAM_H__
+#define __SOC_ROCKCHIP_RK3399_SDRAM_H__

Please use the same format as other headers (i.e. no SOC).

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 10

Quoted 2 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +2 -0
+
+#include <stddef.h>

What is this for?

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 17

Quoted 7 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +7 -0
+
+enum {
+       DDR3 = 3,
+       LPDDR2 = 5,
+       LPDDR3 = 6,
+       LPDDR4 = 7,
+       UNUSED = 0xFF

Please can you use lower-case hex?

Quoted 14 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +14 -0
+};
+
+struct rk3399_ddr_pctl_regs {
+       u32 denali_ctl[332];
+};
+
+struct rk3399_ddr_publ_regs {
+       u32 denali_phy[959];
+};
+
+struct rk3399_ddr_pi_regs {
+       u32 denali_pi[200];
+};
+union noc_ddrtiminga0 {

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 41

Quoted 10 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +10 -0
+       u32 d32;
+       struct {
+               unsigned acttoact : 6;
+               unsigned reserved0 : 2;
+               unsigned rdtomiss : 6;
+               unsigned reserved1 : 2;
+               unsigned wrtomiss : 6;
+               unsigned reserved2 : 2;
+               unsigned readlatency : 8;
+       } b;

Can we use a simple value here, and drop the bitfields?

Quoted 55 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +55 -0
+};
+
+union noc_ddrtimingb0 {
+       u32 d32;
+       struct {
+               unsigned rdtowr : 5;
+               unsigned reserved0 : 3;
+               unsigned wrtord : 5;
+               unsigned reserved1 : 3;
+               unsigned rrd : 4;
+               unsigned reserved2 : 4;
+               unsigned faw : 6;
+               unsigned reserved3 : 2;
+       } b;
+};
+
+union noc_ddrtimingc0 {
+       u32 d32;
+       struct {
+               unsigned burstpenalty : 4;
+               unsigned reserved0 : 4;
+               unsigned wrtomwr : 6;
+               unsigned reserved1 : 18;
+       } b;
+};
+
+union noc_devtodev0 {
+       u32 d32;
+       struct {
+               unsigned busrdtord : 3;
+               unsigned reserved0 : 1;
+               unsigned busrdtowr : 3;
+               unsigned reserved1 : 1;
+               unsigned buswrtord : 3;
+               unsigned reserved2 : 1;
+               unsigned buswrtowr : 3;
+               unsigned reserved3 : 17;
+       } b;
+};
+
+union noc_ddrmode {
+       u32 d32;
+       struct {
+               unsigned autoprecharge : 1;
+               unsigned bypassfiltering : 1;
+               unsigned fawbank : 1;
+               unsigned burstsize : 2;
+               unsigned mwrsize : 2;
+               unsigned reserved2 : 1;
+               unsigned forceorder : 8;
+               unsigned forceorderstate : 8;
+               unsigned reserved3 : 8;
+       } b;
+};
+

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 106

Quoted 10 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +10 -0
+struct rk3399_msch_regs {
+       u32 coreid;
+       u32 revisionid;
+       u32 ddrconf;
+       u32 ddrsize;
+       union noc_ddrtiminga0 ddrtiminga0;
+       union noc_ddrtimingb0 ddrtimingb0;
+       union noc_ddrtimingc0 ddrtimingc0;
+       union noc_devtodev0 devtodev0;
+       u32 reserved0[(0x110-0x20)/4];

Please add spaces around operators:

      u32 reserved0[(0x110 - 0x20) / 4];

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 112

Quoted 6 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +6 -0
+       union noc_ddrmode ddrmode;
+       u32 reserved1[(0x1000-0x114)/4];
+       u32 agingx0;
+};
+
+struct rk3399_msch_timings {

These structures and members should have comments.

Quoted 12 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +12 -0
+       union noc_ddrtiminga0 ddrtiminga0;
+       union noc_ddrtimingb0 ddrtimingb0;
+       union noc_ddrtimingc0 ddrtimingc0;
+       union noc_devtodev0 devtodev0;
+       union noc_ddrmode ddrmode;
+       u32 agingx0;
+};
+
+struct rk3399_ddr_cic_regs {
+       u32 cic_ctrl0;
+       u32 cic_ctrl1;
+       u32 cic_idle_th;

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 134

Quoted 10 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +10 -0
+       u32 cic_cg_wait_th;
+       u32 cic_status0;
+       u32 cic_status1;
+       u32 cic_ctrl2;
+       u32 cic_ctrl3;
+       u32 cic_ctrl4;
+};
+
+/* DENALI_CTL_00 */
+#define START          (1)

Drop brackets around simple constants

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 144

Quoted 10 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +10 -0
+
+/* DENALI_CTL_68 */
+#define PWRUP_SREFRESH_EXIT    (1 << 16)
+
+/* DENALI_CTL_274 */
+#define MEM_RST_VALID  (1)
+
+struct rk3399_sdram_channel {
+       unsigned char rank;
+       /* col = 0, means this channel is invalid */

But what is col? Please can you expand these comments a bit?

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 152

Quoted 8 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +8 -0
+       unsigned char col;
+       /* 3:8bank, 2:4bank */
+       unsigned char bk;
+       /* channel buswidth, 2:32bit, 1:16bit, 0:8bit */
+       unsigned char bw;
+       /* die buswidth, 2:32bit, 1:16bit, 0:8bit */
+       unsigned char dbw;
+       /* row_3_4 = 1: 6Gb or 12Gb die

Multi-line comment style:

/*
* ...
* ...
*/

Quoted 24 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +24 -0
+        * row_3_4 = 0: normal die, power of 2
+        */
+       unsigned char row_3_4;
+       unsigned char cs0_row;
+       unsigned char cs1_row;
+       unsigned int ddrconfig;
+       struct rk3399_msch_timings noc_timings;
+};
+
+struct rk3399_sdram_params {
+       struct rk3399_sdram_channel ch[2];
+       unsigned int ddr_freq;
+       unsigned char dramtype;
+       unsigned char num_channels;
+       unsigned char stride;
+       unsigned char odt;
+       /* align 8 byte */
+       struct rk3399_ddr_pctl_regs pctl_regs;
+       /* align 8 byte */
+       struct rk3399_ddr_pi_regs pi_regs;
+       /* align 8 byte */
+       struct rk3399_ddr_publ_regs phy_regs;
+       /* used for align 8byte for next struct */
+       unsigned int align_8;

arch/arm/include/asm/arch-rockchip/sdram_rk3399.h line 186

Quoted 10 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +10 -0
+};
+
+#define PI_CA_TRAINING (1 << 0)
+#define PI_WRITE_LEVELING      (1 << 1)
+#define PI_READ_GATE_TRAINING  (1 << 2)
+#define PI_READ_LEVELING       (1 << 3)
+#define PI_WDQ_LEVELING        (1 << 4)
+#define PI_FULL_TRAINING       (0xff)
+
+size_t sdram_size_mb(void);

Function comment

arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc

Quoted 16 lines
arch/arm/include/asm/arch-rockchip/sdram_rk3399.h new file +2 -0
+
+#endif
arch/arm/mach-rockchip/rk3399/Makefile +1 -0
index 98ebeac..437d851 100644
@@ -7,3 +7,4 @@
 obj-y += clk_rk3399.o
 obj-y += rk3399.o
 obj-y += syscon_rk3399.o
+obj-y += sdram_rk3399.o
arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc new file +0 -0
new file mode 100644
index 0000000..2b29bdf

Can you put this into the device tree as is done for rk3288?

Quoted 15 lines
arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc new file +14 -0
@@ -0,0 +1,1565 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+

[...]

Quoted 7 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +1 -0
new file mode 100644
index 0000000..67a92cc
@@ -0,0 +1,1121 @@
+/*

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 11

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+ * (C) Copyright 2016 Rockchip Inc.
+ *
+ * SPDX-License-Identifier:     GPL-2.0
+ *
+ * Adapted from coreboot.
+ */
+#include <common.h>
+#include <dm.h>
+#include <asm/io.h>
+#include <clk.h>

clk.h should go after common.h

Quoted 30 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +30 -0
+#include <asm/arch/sdram_rk3399.h>
+#include <asm/arch/cru_rk3399.h>
+#include <asm/arch/grf_rk3399.h>
+#include <asm/arch/hardware.h>
+#include <linux/err.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define DDRC0_BASE_ADDR         0xffa80000
+#define SERVER_MSCH0_BASE_ADDR  0xffa84000
+#define DDRC1_BASE_ADDR         0xffa88000
+#define SERVER_MSCH1_BASE_ADDR  0xffa8c000
+
+#define DDR_PI_OFFSET                  0x800
+#define DDR_PHY_OFFSET                 0x2000
+#define DDRC0_PI_BASE_ADDR             (DDRC0_BASE_ADDR + DDR_PI_OFFSET)
+#define DDRC0_PHY_BASE_ADDR            (DDRC0_BASE_ADDR + DDR_PHY_OFFSET)
+#define DDRC1_PI_BASE_ADDR             (DDRC1_BASE_ADDR + DDR_PI_OFFSET)
+#define DDRC1_PHY_BASE_ADDR            (DDRC1_BASE_ADDR + DDR_PHY_OFFSET)
+
+#define PMUCRU_BASE             0xff750000
+#define CRU_BASE                0xff760000
+#define PMUGRF_BASE             0xff320000
+#define PMUSGRF_BASE            0xff330000
+#define CIC_BASE_ADDR          0xff620000
+
+static struct rk3399_pmucru * const pmucru_ptr = (void *)PMUCRU_BASE;
+static struct rk3399_cru * const cru_ptr = (void *)CRU_BASE;
+static struct rk3399_pmugrf_regs * const rk3399_pmugrf = (void *)PMUGRF_BASE;
+static struct rk3399_pmusgrf_regs * const rk3399_pmusgrf = (void *)PMUSGRF_BASE;

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 51

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+
+static struct rk3399_ddr_pctl_regs * const rk3399_ddr_pctl[2] = {
+       (void *)DDRC0_BASE_ADDR, (void *)DDRC1_BASE_ADDR };
+static struct rk3399_ddr_pi_regs * const rk3399_ddr_pi[2] = {
+       (void *)DDRC0_PI_BASE_ADDR, (void *)DDRC1_PI_BASE_ADDR };
+static struct rk3399_ddr_publ_regs * const rk3399_ddr_publ[2] = {
+       (void *)DDRC0_PHY_BASE_ADDR, (void *)DDRC1_PHY_BASE_ADDR };
+static struct rk3399_msch_regs * const rk3399_msch[2] = {
+       (void *)SERVER_MSCH0_BASE_ADDR, (void *)SERVER_MSCH1_BASE_ADDR };
+static struct rk3399_ddr_cic_regs *const rk3399_ddr_cic = (void *)CIC_BASE_ADDR;

These should all come from the device tree, right?

Quoted 41 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +41 -0
+
+/*
+ * sys_reg bitfield struct
+ * [31]                row_3_4_ch1
+ * [30]                row_3_4_ch0
+ * [29:28]     chinfo
+ * [27]                rank_ch1
+ * [26:25]     col_ch1
+ * [24]                bk_ch1
+ * [23:22]     cs0_row_ch1
+ * [21:20]     cs1_row_ch1
+ * [19:18]     bw_ch1
+ * [17:16]     dbw_ch1;
+ * [15:13]     ddrtype
+ * [12]                channelnum
+ * [11]                rank_ch0
+ * [10:9]      col_ch0
+ * [8]         bk_ch0
+ * [7:6]       cs0_row_ch0
+ * [5:4]       cs1_row_ch0
+ * [3:2]       bw_ch0
+ * [1:0]       dbw_ch0
+*/
+#define SYS_REG_ENC_ROW_3_4(n, ch)     ((n) << (30 + (ch)))
+#define SYS_REG_DEC_ROW_3_4(n, ch)     ((n >> (30 + ch)) & 0x1)
+#define SYS_REG_ENC_CHINFO(ch)         (1 << (28 + (ch)))
+#define SYS_REG_ENC_DDRTYPE(n)         ((n) << 13)
+#define SYS_REG_ENC_NUM_CH(n)          (((n) - 1) << 12)
+#define SYS_REG_DEC_NUM_CH(n)          (1 + ((n >> 12) & 0x1))
+#define SYS_REG_ENC_RANK(n, ch)                (((n) - 1) << (11 + ((ch) * 16)))
+#define SYS_REG_DEC_RANK(n, ch)                (1 + ((n >> (11 + 16 * ch)) & 0x1))
+#define SYS_REG_ENC_COL(n, ch)         (((n) - 9) << (9 + ((ch) * 16)))
+#define SYS_REG_DEC_COL(n, ch)         (9 + ((n >> (9 + 16 * ch)) & 0x3))
+#define SYS_REG_ENC_BK(n, ch)          (((n) == 3 ? 0 : 1) \
+                                               << (8 + ((ch) * 16)))
+#define SYS_REG_DEC_BK(n, ch)          (3 - ((n >> (8 + 16 * ch)) & 0x1))
+#define SYS_REG_ENC_CS0_ROW(n, ch)     (((n) - 13) << (6 + ((ch) * 16)))
+#define SYS_REG_DEC_CS0_ROW(n, ch)     (13 + ((n >> (6 + 16 * ch)) & 0x3))
+#define SYS_REG_ENC_CS1_ROW(n, ch)     (((n) - 13) << (4 + ((ch) * 16)))
+#define SYS_REG_DEC_CS1_ROW(n, ch)     (13 + ((n >> (4 + 16 * ch)) & 0x3))
+#define SYS_REG_ENC_BW(n, ch)          ((2 >> (n)) << (2 + ((ch) * 16)))

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 102

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+#define SYS_REG_DEC_BW(n, ch)          (2 >> ((n >> (2 + 16 * ch)) & 0x3))
+#define SYS_REG_ENC_DBW(n, ch)         ((2 >> (n)) << (0 + ((ch) * 16)))
+#define SYS_REG_DEC_DBW(n, ch)         (2 >> ((n >> (0 + 16 * ch)) & 0x3))
+
+#define DDR_STRIDE(n)          writel((0x1F << (10 + 16)) | (n << 10), \
+                                       &rk3399_pmusgrf->soc_con4)
+
+#define PRESET_SGRF_HOLD(n)    ((0x1 << (6+16)) | ((n) << 6))
+#define PRESET_GPIO0_HOLD(n)   ((0x1 << (7+16)) | ((n) << 7))
+#define PRESET_GPIO1_HOLD(n)   ((0x1 << (8+16)) | ((n) << 8))

Can we please have the shift/mask values defined, and use explicit shift/mask in the code, instead of these macros?

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 111

Quoted 9 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +9 -0
+
+#define PHY_DRV_ODT_Hi_Z       (0x0)
+#define PHY_DRV_ODT_240                (0x1)
+#define PHY_DRV_ODT_120                (0x8)
+#define PHY_DRV_ODT_80         (0x9)
+#define PHY_DRV_ODT_60         (0xc)
+#define PHY_DRV_ODT_48         (0xd)
+#define PHY_DRV_ODT_40         (0xe)
+#define PHY_DRV_ODT_34_3       (0xf)

Drop brackets

Quoted 5 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +5 -0
+
+#ifdef CONFIG_SPL_BUILD
+static void copy_to_reg(u32 *dest, const u32 *src, u32 n)
+{
+       int i;

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 126

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+
+       for (i = 0; i < n / sizeof(u32); i++) {
+               writel(*src, dest);
+               src++;
+               dest++;
+       }
+}
+
+static void phy_dll_bypass_set(u32 channel,
+       struct rk3399_ddr_publ_regs *ddr_publ_regs, u32 freq)

Can you check patman/checkpatch for this? The alignment seems wrong.

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 134

Quoted 8 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +8 -0
+{
+       u32 *denali_phy = ddr_publ_regs->denali_phy;
+       if (freq <= 125*MHz) {
+               /* phy_sw_master_mode_X PHY_86/214/342/470 4bits offset_8 */
+               setbits_le32(&denali_phy[86], (0x3 << 2) << 8);
+               setbits_le32(&denali_phy[214], (0x3 << 2) << 8);
+               setbits_le32(&denali_phy[342], (0x3 << 2) << 8);
+               setbits_le32(&denali_phy[470], (0x3 << 2) << 8);

These should really be defined as SHIFT and MASK values.

Quoted 21 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +21 -0
+
+               /* phy_adrctl_sw_master_mode PHY_547/675/803 4bits offset_16 */
+               setbits_le32(&denali_phy[547], (0x3 << 2) << 16);
+               setbits_le32(&denali_phy[675], (0x3 << 2) << 16);
+               setbits_le32(&denali_phy[803], (0x3 << 2) << 16);
+       } else {
+               /* phy_sw_master_mode_X PHY_86/214/342/470 4bits offset_8 */
+               clrbits_le32(&denali_phy[86], (0x3 << 2) << 8);
+               clrbits_le32(&denali_phy[214], (0x3 << 2) << 8);
+               clrbits_le32(&denali_phy[342], (0x3 << 2) << 8);
+               clrbits_le32(&denali_phy[470], (0x3 << 2) << 8);
+
+               /* phy_adrctl_sw_master_mode PHY_547/675/803 4bits offset_16 */
+               clrbits_le32(&denali_phy[547], (0x3 << 2) << 16);
+               clrbits_le32(&denali_phy[675], (0x3 << 2) << 16);
+               clrbits_le32(&denali_phy[803], (0x3 << 2) << 16);
+       }
+}
+
+static void set_memory_map(u32 channel,
+                          const struct rk3399_sdram_params *sdram_params)

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 165

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+{
+       const struct rk3399_sdram_channel *sdram_ch =
+               &sdram_params->ch[channel];
+       u32 *denali_ctl = rk3399_ddr_pctl[channel]->denali_ctl;
+       u32 *denali_pi = rk3399_ddr_pi[channel]->denali_pi;
+       u32 cs_map;
+       u32 reduc;
+       u32 row;
+
+       if ((sdram_ch->ddrconfig < 2) || (sdram_ch->ddrconfig == 4))

Can you add a comment as to what this code is doing?

Quoted 156 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +156 -0
+               row = 16;
+       else if (sdram_ch->ddrconfig == 3)
+               row = 14;
+       else
+               row = 15;
+
+       cs_map = (sdram_ch->rank > 1) ? 3 : 1;
+       reduc = (sdram_ch->bw == 2) ? 0 : 1;
+
+       clrsetbits_le32(&denali_ctl[191], 0xF, (12 - sdram_ch->col));
+       clrsetbits_le32(&denali_ctl[190], (0x3 << 16) | (0x7 << 24),
+                       ((3 - sdram_ch->bk) << 16) |
+                       ((16 - row) << 24));
+
+       clrsetbits_le32(&denali_ctl[196], 0x3 | (1 << 16),
+                       cs_map | (reduc << 16));
+
+       /* PI_199 PI_COL_DIFF:RW:0:4 */
+       clrsetbits_le32(&denali_pi[199], 0xF, (12 - sdram_ch->col));
+
+       /* PI_155 PI_ROW_DIFF:RW:24:3 PI_BANK_DIFF:RW:16:2 */
+       clrsetbits_le32(&denali_pi[155], (0x3 << 16) | (0x7 << 24),
+                       ((3 - sdram_ch->bk) << 16) |
+                       ((16 - row) << 24));
+       /* PI_41 PI_CS_MAP:RW:24:4 */
+       clrsetbits_le32(&denali_pi[41], 0xf << 24, cs_map << 24);
+       if ((sdram_ch->rank == 1) && (sdram_params->dramtype == DDR3))
+               writel(0x2EC7FFFF, &denali_pi[34]);
+}
+
+static void set_ds_odt(u32 channel,
+                      const struct rk3399_sdram_params *sdram_params)
+{
+       u32 *denali_phy = rk3399_ddr_publ[channel]->denali_phy;
+
+       u32 tsel_idle_en, tsel_wr_en, tsel_rd_en;
+       u32 tsel_idle_select_p, tsel_wr_select_p, tsel_rd_select_p;
+       u32 ca_tsel_wr_select_p, ca_tsel_wr_select_n;
+       u32 tsel_idle_select_n, tsel_wr_select_n, tsel_rd_select_n;
+       u32 reg_value;
+
+       if (sdram_params->dramtype == LPDDR4) {
+               tsel_rd_select_p = PHY_DRV_ODT_Hi_Z;
+               tsel_wr_select_p = PHY_DRV_ODT_40;
+               ca_tsel_wr_select_p = PHY_DRV_ODT_40;
+               tsel_idle_select_p = PHY_DRV_ODT_Hi_Z;
+
+               tsel_rd_select_n = PHY_DRV_ODT_240;
+               tsel_wr_select_n = PHY_DRV_ODT_40;
+               ca_tsel_wr_select_n = PHY_DRV_ODT_40;
+               tsel_idle_select_n = PHY_DRV_ODT_240;
+       } else if (sdram_params->dramtype == LPDDR3) {
+               tsel_rd_select_p = PHY_DRV_ODT_240;
+               tsel_wr_select_p = PHY_DRV_ODT_34_3;
+               ca_tsel_wr_select_p = PHY_DRV_ODT_48;
+               tsel_idle_select_p = PHY_DRV_ODT_240;
+
+               tsel_rd_select_n = PHY_DRV_ODT_Hi_Z;
+               tsel_wr_select_n = PHY_DRV_ODT_34_3;
+               ca_tsel_wr_select_n = PHY_DRV_ODT_48;
+               tsel_idle_select_n = PHY_DRV_ODT_Hi_Z;
+       } else {
+               tsel_rd_select_p = PHY_DRV_ODT_240;
+               tsel_wr_select_p = PHY_DRV_ODT_34_3;
+               ca_tsel_wr_select_p = PHY_DRV_ODT_34_3;
+               tsel_idle_select_p = PHY_DRV_ODT_240;
+
+               tsel_rd_select_n = PHY_DRV_ODT_240;
+               tsel_wr_select_n = PHY_DRV_ODT_34_3;
+               ca_tsel_wr_select_n = PHY_DRV_ODT_34_3;
+               tsel_idle_select_n = PHY_DRV_ODT_240;
+       }
+
+       if (sdram_params->odt == 1)
+               tsel_rd_en = 1;
+       else
+               tsel_rd_en = 0;
+
+       tsel_wr_en = 0;
+       tsel_idle_en = 0;
+
+       /*
+        * phy_dq_tsel_select_X 24bits DENALI_PHY_6/134/262/390 offset_0
+        * sets termination values for read/idle cycles and drive strength
+        * for write cycles for DQ/DM
+        */
+       reg_value = tsel_rd_select_n | (tsel_rd_select_p << 0x4) |
+                   (tsel_wr_select_n << 8) | (tsel_wr_select_p << 12) |
+                   (tsel_idle_select_n << 16) | (tsel_idle_select_p << 20);
+       clrsetbits_le32(&denali_phy[6], 0xffffff, reg_value);
+       clrsetbits_le32(&denali_phy[134], 0xffffff, reg_value);
+       clrsetbits_le32(&denali_phy[262], 0xffffff, reg_value);
+       clrsetbits_le32(&denali_phy[390], 0xffffff, reg_value);
+
+       /*
+        * phy_dqs_tsel_select_X 24bits DENALI_PHY_7/135/263/391 offset_0
+        * sets termination values for read/idle cycles and drive strength
+        * for write cycles for DQS
+        */
+       clrsetbits_le32(&denali_phy[7], 0xffffff, reg_value);
+       clrsetbits_le32(&denali_phy[135], 0xffffff, reg_value);
+       clrsetbits_le32(&denali_phy[263], 0xffffff, reg_value);
+       clrsetbits_le32(&denali_phy[391], 0xffffff, reg_value);
+
+       /* phy_adr_tsel_select_ 8bits DENALI_PHY_544/672/800 offset_0 */
+       reg_value = ca_tsel_wr_select_n | (ca_tsel_wr_select_p << 0x4);
+       clrsetbits_le32(&denali_phy[544], 0xff, reg_value);
+       clrsetbits_le32(&denali_phy[672], 0xff, reg_value);
+       clrsetbits_le32(&denali_phy[800], 0xff, reg_value);
+
+       /* phy_pad_addr_drive 8bits DENALI_PHY_928 offset_0 */
+       clrsetbits_le32(&denali_phy[928], 0xff, reg_value);
+
+       /* phy_pad_rst_drive 8bits DENALI_PHY_937 offset_0 */
+       clrsetbits_le32(&denali_phy[937], 0xff, reg_value);
+
+       /* phy_pad_cke_drive 8bits DENALI_PHY_935 offset_0 */
+       clrsetbits_le32(&denali_phy[935], 0xff, reg_value);
+
+       /* phy_pad_cs_drive 8bits DENALI_PHY_939 offset_0 */
+       clrsetbits_le32(&denali_phy[939], 0xff, reg_value);
+
+       /* phy_pad_clk_drive 8bits DENALI_PHY_929 offset_0 */
+       clrsetbits_le32(&denali_phy[929], 0xff, reg_value);
+
+       /* phy_pad_fdbk_drive 23bit DENALI_PHY_924/925 */
+       clrsetbits_le32(&denali_phy[924], 0xff,
+                       tsel_wr_select_n | (tsel_wr_select_p << 4));
+       clrsetbits_le32(&denali_phy[925], 0xff,
+                       tsel_rd_select_n | (tsel_rd_select_p << 4));
+
+       /* phy_dq_tsel_enable_X 3bits DENALI_PHY_5/133/261/389 offset_16 */
+       reg_value = (tsel_rd_en | (tsel_wr_en << 1) | (tsel_idle_en << 2))
+               << 16;
+       clrsetbits_le32(&denali_phy[5], 0x7 << 16, reg_value);
+       clrsetbits_le32(&denali_phy[133], 0x7 << 16, reg_value);
+       clrsetbits_le32(&denali_phy[261], 0x7 << 16, reg_value);
+       clrsetbits_le32(&denali_phy[389], 0x7 << 16, reg_value);
+
+       /* phy_dqs_tsel_enable_X 3bits DENALI_PHY_6/134/262/390 offset_24 */
+       reg_value = (tsel_rd_en | (tsel_wr_en << 1) | (tsel_idle_en << 2))
+               << 24;
+       clrsetbits_le32(&denali_phy[6], 0x7 << 24, reg_value);
+       clrsetbits_le32(&denali_phy[134], 0x7 << 24, reg_value);
+       clrsetbits_le32(&denali_phy[262], 0x7 << 24, reg_value);
+       clrsetbits_le32(&denali_phy[390], 0x7 << 24, reg_value);
+
+       /* phy_adr_tsel_enable_ 1bit DENALI_PHY_518/646/774 offset_8 */
+       reg_value = tsel_wr_en << 8;
+       clrsetbits_le32(&denali_phy[518], 0x1 << 8, reg_value);
+       clrsetbits_le32(&denali_phy[646], 0x1 << 8, reg_value);
+       clrsetbits_le32(&denali_phy[774], 0x1 << 8, reg_value);
+
+       /* phy_pad_addr_term tsel 1bit DENALI_PHY_933 offset_17 */
+       reg_value = tsel_wr_en << 17;
+       clrsetbits_le32(&denali_phy[933], 0x1 << 17, reg_value);

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 331

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+       /*
+        * pad_rst/cke/cs/clk_term tsel 1bits
+        * DENALI_PHY_938/936/940/934 offset_17
+        */
+       clrsetbits_le32(&denali_phy[938], 0x1 << 17, reg_value);
+       clrsetbits_le32(&denali_phy[936], 0x1 << 17, reg_value);
+       clrsetbits_le32(&denali_phy[940], 0x1 << 17, reg_value);
+       clrsetbits_le32(&denali_phy[934], 0x1 << 17, reg_value);
+
+       /* phy_pad_fdbk_term 1bit DENALI_PHY_930 offset_17 */

These could all use SHIFT/MASK values

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 343

Quoted 12 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +12 -0
+       clrsetbits_le32(&denali_phy[930], 0x1 << 17, reg_value);
+}
+
+static void phy_io_config(u32 channel,
+                         const struct rk3399_sdram_params *sdram_params)
+{
+       u32 *denali_phy = rk3399_ddr_publ[channel]->denali_phy;
+       u32 vref_mode_dq = 0;
+       u32 vref_value_dq = 0;
+       u32 vref_mode_ac = 0;
+       u32 vref_value_ac = 0;
+       u32 mode_sel = 0;

Don't init here unless you need to.

Quoted 19 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +19 -0
+       u32 reg_value;
+       u32 drv_value, odt_value;
+       u32 speed;
+
+       /* vref setting */
+       if (sdram_params->dramtype == LPDDR4) {
+               /* LPDDR4 */
+               vref_mode_dq = 0x6;
+               vref_value_dq = 0x1f;
+               vref_mode_ac = 0x6;
+               vref_value_ac = 0x1f;
+       } else if (sdram_params->dramtype == LPDDR3) {
+               if (sdram_params->odt == 1) {
+                       vref_mode_dq = 0x5;  /* LPDDR3 ODT */
+                       drv_value = (readl(&denali_phy[6]) >> 12) & 0xf;
+                       odt_value = (readl(&denali_phy[6]) >> 4) & 0xf;
+                       if (drv_value == PHY_DRV_ODT_48) {
+                               switch (odt_value) {
+                               case PHY_DRV_ODT_240:

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 372

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+                                       vref_value_dq = 0x16;
+                                       break;
+                               case PHY_DRV_ODT_120:
+                                       vref_value_dq = 0x26;
+                                       break;
+                               case PHY_DRV_ODT_60:
+                                       vref_value_dq = 0x36;
+                                       break;
+                               default:
+                                       error("Halting: Invalid ODT value.\n");

Can we return an error and use debug() ?

Quoted 115 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +115 -0
+                               }
+                       } else if (drv_value == PHY_DRV_ODT_40) {
+                               switch (odt_value) {
+                               case PHY_DRV_ODT_240:
+                                       vref_value_dq = 0x19;
+                                       break;
+                               case PHY_DRV_ODT_120:
+                                       vref_value_dq = 0x23;
+                                       break;
+                               case PHY_DRV_ODT_60:
+                                       vref_value_dq = 0x31;
+                                       break;
+                               default:
+                                       error("Halting: Invalid ODT value.\n");
+                               }
+                       } else if (drv_value == PHY_DRV_ODT_34_3) {
+                               switch (odt_value) {
+                               case PHY_DRV_ODT_240:
+                                       vref_value_dq = 0x17;
+                                       break;
+                               case PHY_DRV_ODT_120:
+                                       vref_value_dq = 0x20;
+                                       break;
+                               case PHY_DRV_ODT_60:
+                                       vref_value_dq = 0x2e;
+                                       break;
+                               default:
+                                       error("Halting: Invalid ODT value.\n");
+                               }
+                       } else {
+                               error("Halting: Invalid DRV value.\n");
+                       }
+               } else {
+                       vref_mode_dq = 0x2;  /* LPDDR3 */
+                       vref_value_dq = 0x1f;
+               }
+               vref_mode_ac = 0x2;
+               vref_value_ac = 0x1f;
+       } else if (sdram_params->dramtype == DDR3) {
+               /* DDR3L */
+               vref_mode_dq = 0x1;
+               vref_value_dq = 0x1f;
+               vref_mode_ac = 0x1;
+               vref_value_ac = 0x1f;
+       }
+       else
+               error("Halting: Unknown DRAM type.\n");
+
+       reg_value = (vref_mode_dq << 9) | (0x1 << 8) | vref_value_dq;
+
+       /* PHY_913 PHY_PAD_VREF_CTRL_DQ_0 12bits offset_8 */
+       clrsetbits_le32(&denali_phy[913], 0xfff << 8, reg_value << 8);
+       /* PHY_914 PHY_PAD_VREF_CTRL_DQ_1 12bits offset_0 */
+       clrsetbits_le32(&denali_phy[914], 0xfff, reg_value);
+       /* PHY_914 PHY_PAD_VREF_CTRL_DQ_2 12bits offset_16 */
+       clrsetbits_le32(&denali_phy[914], 0xfff << 16, reg_value << 16);
+       /* PHY_915 PHY_PAD_VREF_CTRL_DQ_3 12bits offset_0 */
+       clrsetbits_le32(&denali_phy[915], 0xfff, reg_value);
+
+       reg_value = (vref_mode_ac << 9) | (0x1 << 8) | vref_value_ac;
+
+       /* PHY_915 PHY_PAD_VREF_CTRL_AC 12bits offset_16 */
+       clrsetbits_le32(&denali_phy[915], 0xfff << 16, reg_value << 16);
+
+       if (sdram_params->dramtype == LPDDR4)
+               mode_sel = 0x6;
+       else if (sdram_params->dramtype == LPDDR3)
+               mode_sel = 0x0;
+       else if (sdram_params->dramtype == DDR3)
+               mode_sel = 0x1;
+
+       /* PHY_924 PHY_PAD_FDBK_DRIVE */
+       clrsetbits_le32(&denali_phy[924], 0x7 << 15, mode_sel << 15);
+       /* PHY_926 PHY_PAD_DATA_DRIVE */
+       clrsetbits_le32(&denali_phy[926], 0x7 << 6, mode_sel << 6);
+       /* PHY_927 PHY_PAD_DQS_DRIVE */
+       clrsetbits_le32(&denali_phy[927], 0x7 << 6, mode_sel << 6);
+       /* PHY_928 PHY_PAD_ADDR_DRIVE */
+       clrsetbits_le32(&denali_phy[928], 0x7 << 14, mode_sel << 14);
+       /* PHY_929 PHY_PAD_CLK_DRIVE */
+       clrsetbits_le32(&denali_phy[929], 0x7 << 14, mode_sel << 14);
+       /* PHY_935 PHY_PAD_CKE_DRIVE */
+       clrsetbits_le32(&denali_phy[935], 0x7 << 14, mode_sel << 14);
+       /* PHY_937 PHY_PAD_RST_DRIVE */
+       clrsetbits_le32(&denali_phy[937], 0x7 << 14, mode_sel << 14);
+       /* PHY_939 PHY_PAD_CS_DRIVE */
+       clrsetbits_le32(&denali_phy[939], 0x7 << 14, mode_sel << 14);
+
+
+       /* speed setting */
+       if (sdram_params->ddr_freq < 400*MHz)
+               speed = 0x0;
+       else if (sdram_params->ddr_freq < 800*MHz)
+               speed = 0x1;
+       else if (sdram_params->ddr_freq < 1200*MHz)
+               speed = 0x2;
+       else
+               speed = 0x3;
+
+       /* PHY_924 PHY_PAD_FDBK_DRIVE */
+       clrsetbits_le32(&denali_phy[924], 0x3 << 21, speed << 21);
+       /* PHY_926 PHY_PAD_DATA_DRIVE */
+       clrsetbits_le32(&denali_phy[926], 0x3 << 9, speed << 9);
+       /* PHY_927 PHY_PAD_DQS_DRIVE */
+       clrsetbits_le32(&denali_phy[927], 0x3 << 9, speed << 9);
+       /* PHY_928 PHY_PAD_ADDR_DRIVE */
+       clrsetbits_le32(&denali_phy[928], 0x3 << 17, speed << 17);
+       /* PHY_929 PHY_PAD_CLK_DRIVE */
+       clrsetbits_le32(&denali_phy[929], 0x3 << 17, speed << 17);
+       /* PHY_935 PHY_PAD_CKE_DRIVE */
+       clrsetbits_le32(&denali_phy[935], 0x3 << 17, speed << 17);
+       /* PHY_937 PHY_PAD_RST_DRIVE */
+       clrsetbits_le32(&denali_phy[937], 0x3 << 17, speed << 17);
+       /* PHY_939 PHY_PAD_CS_DRIVE */
+       clrsetbits_le32(&denali_phy[939], 0x3 << 17, speed << 17);

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 497

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+}
+
+static int pctl_cfg(u32 channel,
+                   const struct rk3399_sdram_params *sdram_params)
+{
+       u32 *denali_ctl = rk3399_ddr_pctl[channel]->denali_ctl;
+       u32 *denali_pi = rk3399_ddr_pi[channel]->denali_pi;
+       u32 *denali_phy = rk3399_ddr_publ[channel]->denali_phy;
+       const u32 *params_ctl = sdram_params->pctl_regs.denali_ctl;
+       const u32 *params_phy = sdram_params->phy_regs.denali_phy;

I think all of these should go in a struct, and you can set up the values by reading from the DT.

Quoted 28 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +28 -0
+       u32 tmp, tmp1, tmp2;
+       u32 pwrup_srefresh_exit;
+
+       /*
+        * work around controller bug:
+        * Do not program DRAM_CLASS until NO_PHY_IND_TRAIN_INT is programmed
+        */
+       copy_to_reg(&denali_ctl[1], &params_ctl[1],
+                   sizeof(struct rk3399_ddr_pctl_regs) - 4);
+       writel(params_ctl[0], &denali_ctl[0]);
+       copy_to_reg(denali_pi, &sdram_params->pi_regs.denali_pi[0],
+                   sizeof(struct rk3399_ddr_pi_regs));
+       /* rank count need to set for init */
+       set_memory_map(channel, sdram_params);
+
+       writel(sdram_params->phy_regs.denali_phy[910], &denali_phy[910]);
+       writel(sdram_params->phy_regs.denali_phy[911], &denali_phy[911]);
+       writel(sdram_params->phy_regs.denali_phy[912], &denali_phy[912]);
+
+       pwrup_srefresh_exit = readl(&denali_ctl[68]) & PWRUP_SREFRESH_EXIT;
+       clrbits_le32(&denali_ctl[68], PWRUP_SREFRESH_EXIT);
+
+       /* PHY_DLL_RST_EN */
+       clrsetbits_le32(&denali_phy[957], 0x3 << 24, 1 << 24);
+
+       setbits_le32(&denali_pi[0], START);
+       setbits_le32(&denali_ctl[0], START);
+

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 535

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+       while (1) {
+               tmp = readl(&denali_phy[920]);
+               tmp1 = readl(&denali_phy[921]);
+               tmp2 = readl(&denali_phy[922]);
+               if ((((tmp >> 16) & 0x1) == 0x1) &&
+                   (((tmp1 >> 16) & 0x1) == 0x1) &&
+                   (((tmp1 >> 0) & 0x1) == 0x1) &&
+                   (((tmp2 >> 0) & 0x1) == 0x1))
+                       break;
+       }

What is this loop doing? SHIFT/MASK defines might help.

Quoted 34 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +34 -0
+
+       copy_to_reg(&denali_phy[896], &params_phy[896], (958 - 895) * 4);
+       copy_to_reg(&denali_phy[0], &params_phy[0], (90 - 0 + 1) * 4);
+       copy_to_reg(&denali_phy[128], &params_phy[128], (218 - 128 + 1) * 4);
+       copy_to_reg(&denali_phy[256], &params_phy[256], (346 - 256 + 1) * 4);
+       copy_to_reg(&denali_phy[384], &params_phy[384], (474 - 384 + 1) * 4);
+       copy_to_reg(&denali_phy[512], &params_phy[512], (549 - 512 + 1) * 4);
+       copy_to_reg(&denali_phy[640], &params_phy[640], (677 - 640 + 1) * 4);
+       copy_to_reg(&denali_phy[768], &params_phy[768], (805 - 768 + 1) * 4);
+       set_ds_odt(channel, sdram_params);
+
+       /*
+        * phy_dqs_tsel_wr_timing_X 8bits DENALI_PHY_84/212/340/468 offset_8
+        * dqs_tsel_wr_end[7:4] add Half cycle
+        */
+       tmp = (readl(&denali_phy[84]) >> 8) & 0xff;
+       clrsetbits_le32(&denali_phy[84], 0xff << 8, (tmp + 0x10) << 8);
+       tmp = (readl(&denali_phy[212]) >> 8) & 0xff;
+       clrsetbits_le32(&denali_phy[212], 0xff << 8, (tmp + 0x10) << 8);
+       tmp = (readl(&denali_phy[340]) >> 8) & 0xff;
+       clrsetbits_le32(&denali_phy[340], 0xff << 8, (tmp + 0x10) << 8);
+       tmp = (readl(&denali_phy[468]) >> 8) & 0xff;
+       clrsetbits_le32(&denali_phy[468], 0xff << 8, (tmp + 0x10) << 8);
+
+       /*
+        * phy_dqs_tsel_wr_timing_X 8bits DENALI_PHY_83/211/339/467 offset_8
+        * dq_tsel_wr_end[7:4] add Half cycle
+        */
+       tmp = (readl(&denali_phy[83]) >> 16) & 0xff;
+       clrsetbits_le32(&denali_phy[83], 0xff << 16, (tmp + 0x10) << 16);
+       tmp = (readl(&denali_phy[211]) >> 16) & 0xff;
+       clrsetbits_le32(&denali_phy[211], 0xff << 16, (tmp + 0x10) << 16);
+       tmp = (readl(&denali_phy[339]) >> 16) & 0xff;
+       clrsetbits_le32(&denali_phy[339], 0xff << 16, (tmp + 0x10) << 16);

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 579

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+       tmp = (readl(&denali_phy[467]) >> 16) & 0xff;
+       clrsetbits_le32(&denali_phy[467], 0xff << 16, (tmp + 0x10) << 16);
+
+       phy_io_config(channel, sdram_params);
+
+       /* PHY_DLL_RST_EN */
+       clrsetbits_le32(&denali_phy[957], 0x3 << 24, 0x2 << 24);
+
+       /*
+        * FIXME:

TODO(email):

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 588

Quoted 9 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +9 -0
+        * need to care ERROR bit,
+        * if 100ms do not get right status, return err
+        */
+       tmp = 0;
+       while (!(readl(&denali_ctl[203]) & (1 << 3))) {
+               mdelay(10);
+               tmp ++;
+               if (tmp > 10)
+                       return -1;

-EIO or something else?

Quoted 18 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +18 -0
+       }
+
+       clrsetbits_le32(&denali_ctl[68], PWRUP_SREFRESH_EXIT,
+                       pwrup_srefresh_exit);
+       return 0;
+}
+
+static void select_per_cs_training_index(u32 channel, u32 rank)
+{
+       u32 *denali_phy = rk3399_ddr_publ[channel]->denali_phy;
+
+       /* PHY_84 PHY_PER_CS_TRAINING_EN_0 1bit offset_16 */
+       if ((readl(&denali_phy[84])>>16) & 1) {
+               /*
+                * PHY_8/136/264/392
+                * phy_per_cs_training_index_X 1bit offset_24
+                */
+               clrsetbits_le32(&denali_phy[8], 0x1 << 24, rank << 24);

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 616

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+               clrsetbits_le32(&denali_phy[136], 0x1 << 24, rank << 24);
+               clrsetbits_le32(&denali_phy[264], 0x1 << 24, rank << 24);
+               clrsetbits_le32(&denali_phy[392], 0x1 << 24, rank << 24);
+       }
+}
+
+static void override_write_leveling_value(u32 channel)
+{
+       u32 *denali_ctl = rk3399_ddr_pctl[channel]->denali_ctl;
+       u32 *denali_phy = rk3399_ddr_publ[channel]->denali_phy;

Again if you pass in a struct with all these values you can avoid assigning them here.

Quoted 55 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +55 -0
+       u32 byte;
+
+       /* PHY_896 PHY_FREQ_SEL_MULTICAST_EN 1bit offset_0 */
+       setbits_le32(&denali_phy[896], 1);
+
+       /*
+        * PHY_8/136/264/392
+        * phy_per_cs_training_multicast_en_X 1bit offset_16
+        */
+       clrsetbits_le32(&denali_phy[8], 0x1 << 16, 1 << 16);
+       clrsetbits_le32(&denali_phy[136], 0x1 << 16, 1 << 16);
+       clrsetbits_le32(&denali_phy[264], 0x1 << 16, 1 << 16);
+       clrsetbits_le32(&denali_phy[392], 0x1 << 16, 1 << 16);
+
+       for (byte = 0; byte < 4; byte++)
+               clrsetbits_le32(&denali_phy[63 + (128 * byte)], 0xffff << 16,
+                       0x200 << 16);
+
+       /* PHY_896 PHY_FREQ_SEL_MULTICAST_EN 1bit offset_0 */
+       clrbits_le32(&denali_phy[896], 1);
+
+       /* CTL_200 ctrlupd_req 1bit offset_8 */
+       clrsetbits_le32(&denali_ctl[200], 0x1 << 8, 0x1 << 8);
+}
+
+static int data_training(u32 channel,
+                        const struct rk3399_sdram_params *sdram_params,
+                        u32 training_flag)
+{
+       u32 *denali_pi = rk3399_ddr_pi[channel]->denali_pi;
+       u32 *denali_phy = rk3399_ddr_publ[channel]->denali_phy;
+       u32 i, tmp;
+       u32 obs_0, obs_1, obs_2, obs_3, obs_err = 0;
+       u32 rank = sdram_params->ch[channel].rank;
+
+       /* PHY_927 PHY_PAD_DQS_DRIVE  RPULL offset_22 */
+       setbits_le32(&denali_phy[927], (1 << 22));
+
+       if (training_flag == PI_FULL_TRAINING) {
+               if (sdram_params->dramtype == LPDDR4) {
+                       training_flag = PI_CA_TRAINING | PI_WRITE_LEVELING |
+                                       PI_READ_GATE_TRAINING |
+                                       PI_READ_LEVELING | PI_WDQ_LEVELING;
+               } else if (sdram_params->dramtype == LPDDR3) {
+                       training_flag = PI_CA_TRAINING | PI_WRITE_LEVELING |
+                                       PI_READ_GATE_TRAINING;
+               } else if (sdram_params->dramtype == DDR3) {
+                       training_flag = PI_WRITE_LEVELING |
+                                       PI_READ_GATE_TRAINING |
+                                       PI_READ_LEVELING;
+               }
+       }
+
+       /* ca training(LPDDR4,LPDDR3 support) */
+       if ((training_flag & PI_CA_TRAINING) == PI_CA_TRAINING) {

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 681

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+               for (i = 0; i < rank; i++) {
+                       select_per_cs_training_index(channel, i);
+                       /* PI_100 PI_CALVL_EN:RW:8:2 */
+                       clrsetbits_le32(&denali_pi[100], 0x3 << 8, 0x2 << 8);
+                       /* PI_92 PI_CALVL_REQ:WR:16:1,PI_CALVL_CS:RW:24:2 */
+                       clrsetbits_le32(&denali_pi[92],
+                                       (0x1 << 16) | (0x3 << 24),
+                                       (0x1 << 16) | (i << 24));
+
+                       while (1) {

What is this loop doing? Can you add a comment?

Quoted 66 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +66 -0
+                               /* PI_174 PI_INT_STATUS:RD:8:18 */
+                               tmp = readl(&denali_pi[174]) >> 8;
+                               /*
+                                * check status obs
+                                * PHY_532/660/789 phy_adr_calvl_obs1_:0:32
+                                */
+                               obs_0 = readl(&denali_phy[532]);
+                               obs_1 = readl(&denali_phy[660]);
+                               obs_2 = readl(&denali_phy[788]);
+                               if (((obs_0 >> 30) & 0x3) ||
+                                   ((obs_1 >> 30) & 0x3) ||
+                                   ((obs_2 >> 30) & 0x3))
+                                       obs_err = 1;
+                               if ((((tmp >> 11) & 0x1) == 0x1) &&
+                                   (((tmp >> 13) & 0x1) == 0x1) &&
+                                   (((tmp >> 5) & 0x1) == 0x0) &&
+                                   (obs_err == 0))
+                                       break;
+                               else if ((((tmp >> 5) & 0x1) == 0x1) ||
+                                        (obs_err == 1))
+                                       return -1;
+                       }
+                       /* clear interrupt,PI_175 PI_INT_ACK:WR:0:17 */
+                       writel(0x00003f7c, (&denali_pi[175]));
+               }
+               clrbits_le32(&denali_pi[100], 0x3 << 8);
+       }
+
+       /* write leveling(LPDDR4,LPDDR3,DDR3 support) */
+       if ((training_flag & PI_WRITE_LEVELING) == PI_WRITE_LEVELING) {
+               for (i = 0; i < rank; i++) {
+                       select_per_cs_training_index(channel, i);
+                       /* PI_60 PI_WRLVL_EN:RW:8:2 */
+                       clrsetbits_le32(&denali_pi[60], 0x3 << 8, 0x2 << 8);
+                       /* PI_59 PI_WRLVL_REQ:WR:8:1,PI_WRLVL_CS:RW:16:2 */
+                       clrsetbits_le32(&denali_pi[59],
+                                       (0x1 << 8) | (0x3 << 16),
+                                       (0x1 << 8) | (i << 16));
+
+                       while (1) {
+                               /* PI_174 PI_INT_STATUS:RD:8:18 */
+                               tmp = readl(&denali_pi[174]) >> 8;
+
+                               /*
+                                * check status obs, if error maybe can not
+                                * get leveling done PHY_40/168/296/424
+                                * phy_wrlvl_status_obs_X:0:13
+                                */
+                               obs_0 = readl(&denali_phy[40]);
+                               obs_1 = readl(&denali_phy[168]);
+                               obs_2 = readl(&denali_phy[296]);
+                               obs_3 = readl(&denali_phy[424]);
+                               if (((obs_0 >> 12) & 0x1) ||
+                                   ((obs_1 >> 12) & 0x1) ||
+                                   ((obs_2 >> 12) & 0x1) ||
+                                   ((obs_3 >> 12) & 0x1))
+                                       obs_err = 1;
+                               if ((((tmp >> 10) & 0x1) == 0x1) &&
+                                   (((tmp >> 13) & 0x1) == 0x1) &&
+                                   (((tmp >> 4) & 0x1) == 0x0) &&
+                                   (obs_err == 0))
+                                       break;
+                               else if ((((tmp >> 4) & 0x1) == 0x1) ||
+                                        (obs_err == 1))
+                                       return -1;
+                       }

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 757

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+                       /* clear interrupt,PI_175 PI_INT_ACK:WR:0:17 */
+                       writel(0x00003f7c, (&denali_pi[175]));
+               }
+
+               override_write_leveling_value(channel);
+               clrbits_le32(&denali_pi[60], 0x3 << 8);
+       }
+
+       /* read gate training(LPDDR4,LPDDR3,DDR3 support) */
+       if ((training_flag & PI_READ_GATE_TRAINING) == PI_READ_GATE_TRAINING) {

This is a very long function. Can you put each stage in a separate function?

if ((training_flag & PI_CA_TRAINING) == PI_CA_TRAINING)
ca_training();
if ((training_flag & PI_READ_GATE_TRAINING) == PI_READ_GATE_TRAINING)

    read_gate_training();

Or similar...

Quoted 47 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +47 -0
+               for (i = 0; i < rank; i++) {
+                       select_per_cs_training_index(channel, i);
+                       /* PI_80 PI_RDLVL_GATE_EN:RW:24:2 */
+                       clrsetbits_le32(&denali_pi[80], 0x3 << 24, 0x2 << 24);
+                       /*
+                        * PI_74 PI_RDLVL_GATE_REQ:WR:16:1
+                        * PI_RDLVL_CS:RW:24:2
+                        */
+                       clrsetbits_le32(&denali_pi[74],
+                                       (0x1 << 16) | (0x3 << 24),
+                                       (0x1 << 16) | (i << 24));
+
+                       while (1) {
+                               /* PI_174 PI_INT_STATUS:RD:8:18 */
+                               tmp = readl(&denali_pi[174]) >> 8;
+
+                               /*
+                                * check status obs
+                                * PHY_43/171/299/427
+                                *     PHY_GTLVL_STATUS_OBS_x:16:8
+                                */
+                               obs_0 = readl(&denali_phy[43]);
+                               obs_1 = readl(&denali_phy[171]);
+                               obs_2 = readl(&denali_phy[299]);
+                               obs_3 = readl(&denali_phy[427]);
+                               if (((obs_0 >> (16 + 6)) & 0x3) ||
+                                   ((obs_1 >> (16 + 6)) & 0x3) ||
+                                   ((obs_2 >> (16 + 6)) & 0x3) ||
+                                   ((obs_3 >> (16 + 6)) & 0x3))
+                                       obs_err = 1;
+                               if ((((tmp >> 9) & 0x1) == 0x1) &&
+                                   (((tmp >> 13) & 0x1) == 0x1) &&
+                                   (((tmp >> 3) & 0x1) == 0x0) &&
+                                   (obs_err == 0))
+                                       break;
+                               else if ((((tmp >> 3) & 0x1) == 0x1) ||
+                                        (obs_err == 1))
+                                       return -1;
+                       }
+                       /* clear interrupt,PI_175 PI_INT_ACK:WR:0:17 */
+                       writel(0x00003f7c, (&denali_pi[175]));
+               }
+               clrbits_le32(&denali_pi[80], 0x3 << 24);
+       }
+
+       /* read leveling(LPDDR4,LPDDR3,DDR3 support) */
+       if ((training_flag & PI_READ_LEVELING) == PI_READ_LEVELING) {

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 814

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+               for (i = 0; i < rank; i++) {
+                       select_per_cs_training_index(channel, i);
+                       /* PI_80 PI_RDLVL_EN:RW:16:2 */
+                       clrsetbits_le32(&denali_pi[80], 0x3 << 16, 0x2 << 16);
+                       /* PI_74 PI_RDLVL_REQ:WR:8:1,PI_RDLVL_CS:RW:24:2 */
+                       clrsetbits_le32(&denali_pi[74],
+                                       (0x1 << 8) | (0x3 << 24),
+                                       (0x1 << 8) | (i << 24));
+
+                       while (1) {

What is this loop doing? Is there any commonality compared to the other similar loops? Perhaps a function where you pass in the mask to check?

Quoted 79 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +79 -0
+                               /* PI_174 PI_INT_STATUS:RD:8:18 */
+                               tmp = readl(&denali_pi[174]) >> 8;
+
+                               /*
+                                * make sure status obs not report error bit
+                                * PHY_46/174/302/430
+                                *     phy_rdlvl_status_obs_X:16:8
+                                */
+                               if ((((tmp >> 8) & 0x1) == 0x1) &&
+                                   (((tmp >> 13) & 0x1) == 0x1) &&
+                                   (((tmp >> 2) & 0x1) == 0x0))
+                                       break;
+                               else if (((tmp >> 2) & 0x1) == 0x1)
+                                       return -1;
+                       }
+                       /* clear interrupt,PI_175 PI_INT_ACK:WR:0:17 */
+                       writel(0x00003f7c, (&denali_pi[175]));
+               }
+               clrbits_le32(&denali_pi[80], 0x3 << 16);
+       }
+
+       /* wdq leveling(LPDDR4 support) */
+       if ((training_flag & PI_WDQ_LEVELING) == PI_WDQ_LEVELING) {
+               for (i = 0; i < rank; i++) {
+                       select_per_cs_training_index(channel, i);
+                       /*
+                        * disable PI_WDQLVL_VREF_EN before wdq leveling?
+                        * PI_181 PI_WDQLVL_VREF_EN:RW:8:1
+                        */
+                       clrbits_le32(&denali_pi[181], 0x1 << 8);
+                       /* PI_124 PI_WDQLVL_EN:RW:16:2 */
+                       clrsetbits_le32(&denali_pi[124], 0x3 << 16, 0x2 << 16);
+                       /* PI_121 PI_WDQLVL_REQ:WR:8:1,PI_WDQLVL_CS:RW:16:2 */
+                       clrsetbits_le32(&denali_pi[121],
+                                       (0x1 << 8) | (0x3 << 16),
+                                       (0x1 << 8) | (i << 16));
+
+                       while (1) {
+                               /* PI_174 PI_INT_STATUS:RD:8:18 */
+                               tmp = readl(&denali_pi[174]) >> 8;
+                               if ((((tmp >> 12) & 0x1) == 0x1) &&
+                                   (((tmp >> 13) & 0x1) == 0x1) &&
+                                   (((tmp >> 6) & 0x1) == 0x0))
+                                       break;
+                               else if (((tmp >> 6) & 0x1) == 0x1)
+                                       return -1;
+                       }
+                       /* clear interrupt,PI_175 PI_INT_ACK:WR:0:17 */
+                       writel(0x00003f7c, (&denali_pi[175]));
+               }
+               clrbits_le32(&denali_pi[124], 0x3 << 16);
+       }
+
+       /* PHY_927 PHY_PAD_DQS_DRIVE  RPULL offset_22 */
+       clrbits_le32(&denali_phy[927], (1 << 22));
+
+       return 0;
+}
+
+static void set_ddrconfig(const struct rk3399_sdram_params *sdram_params,
+                         unsigned char channel, u32 ddrconfig)
+{
+       /* only need to set ddrconfig */
+       struct rk3399_msch_regs *ddr_msch_regs = rk3399_msch[channel];
+       unsigned int cs0_cap = 0;
+       unsigned int cs1_cap = 0;
+
+       cs0_cap = (1 << (sdram_params->ch[channel].cs0_row
+                       + sdram_params->ch[channel].col
+                       + sdram_params->ch[channel].bk
+                       + sdram_params->ch[channel].bw - 20));
+       if (sdram_params->ch[channel].rank > 1)
+               cs1_cap = cs0_cap >> (sdram_params->ch[channel].cs0_row
+                               - sdram_params->ch[channel].cs1_row);
+       if (sdram_params->ch[channel].row_3_4) {
+               cs0_cap = cs0_cap * 3 / 4;
+               cs1_cap = cs1_cap * 3 / 4;
+       }
+

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 903

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+       writel(ddrconfig | (ddrconfig << 8), &ddr_msch_regs->ddrconf);
+       writel(((cs0_cap / 32) & 0xff) | (((cs1_cap / 32) & 0xff) << 8),
+               &ddr_msch_regs->ddrsize);
+}
+
+static void dram_all_config(const struct rk3399_sdram_params *sdram_params)
+{
+       u32 sys_reg = 0;
+       unsigned int channel;
+       unsigned int use;

What is 'use' for? Is 'index' a better name?

Quoted 59 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +59 -0
+
+       sys_reg |= SYS_REG_ENC_DDRTYPE(sdram_params->dramtype);
+       sys_reg |= SYS_REG_ENC_NUM_CH(sdram_params->num_channels);
+       for (channel = 0, use = 0;
+            (use < sdram_params->num_channels) && (channel < 2); channel++) {
+               const struct rk3399_sdram_channel *info =
+                       &sdram_params->ch[channel];
+               struct rk3399_msch_regs *ddr_msch_regs;
+               const struct rk3399_msch_timings *noc_timing;
+
+               if (sdram_params->ch[channel].col == 0)
+                       continue;
+               use++;
+               sys_reg |= SYS_REG_ENC_ROW_3_4(info->row_3_4, channel);
+               sys_reg |= SYS_REG_ENC_CHINFO(channel);
+               sys_reg |= SYS_REG_ENC_RANK(info->rank, channel);
+               sys_reg |= SYS_REG_ENC_COL(info->col, channel);
+               sys_reg |= SYS_REG_ENC_BK(info->bk, channel);
+               sys_reg |= SYS_REG_ENC_CS0_ROW(info->cs0_row, channel);
+               if (sdram_params->ch[channel].rank > 1)
+                       sys_reg |= SYS_REG_ENC_CS1_ROW(info->cs1_row, channel);
+               sys_reg |= SYS_REG_ENC_BW(info->bw, channel);
+               sys_reg |= SYS_REG_ENC_DBW(info->dbw, channel);
+
+               ddr_msch_regs = rk3399_msch[channel];
+               noc_timing = &sdram_params->ch[channel].noc_timings;
+               writel(noc_timing->ddrtiminga0.d32,
+                      &ddr_msch_regs->ddrtiminga0.d32);
+               writel(noc_timing->ddrtimingb0.d32,
+                       &ddr_msch_regs->ddrtimingb0.d32);
+               writel(noc_timing->ddrtimingc0.d32,
+                       &ddr_msch_regs->ddrtimingc0.d32);
+               writel(noc_timing->devtodev0.d32,
+                       &ddr_msch_regs->devtodev0.d32);
+               writel(noc_timing->ddrmode.d32,
+                       &ddr_msch_regs->ddrmode.d32);
+
+               /* rank 1 memory clock disable (dfi_dram_clk_disable = 1) */
+               if (sdram_params->ch[channel].rank == 1)
+                       setbits_le32(&rk3399_ddr_pctl[channel]->denali_ctl[276],
+                                    1 << 17);
+       }
+
+       writel(sys_reg, &rk3399_pmugrf->os_reg2);
+       DDR_STRIDE(sdram_params->stride);
+
+       /* reboot hold register set */
+       writel(PRESET_SGRF_HOLD(0) | PRESET_GPIO0_HOLD(1) |
+               PRESET_GPIO1_HOLD(1),
+               &pmucru_ptr->pmucru_rstnhold_con[1]);
+       clrsetbits_le32(&cru_ptr->glb_rst_con, 0x3, 0x3);
+}
+
+static void switch_to_phy_index1(const struct rk3399_sdram_params *sdram_params)
+{
+       u32 channel;
+       u32 *denali_phy;
+       u32 ch_count = sdram_params->num_channels;
+       int i = 0;

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 972

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+
+       writel(RK_CLRSETBITS(0x03 << 4 | 1 << 2 | 1,
+                             1 << 4 | 1 << 2 | 1),
+                       &rk3399_ddr_cic->cic_ctrl0);
+       while (!(readl(&rk3399_ddr_cic->cic_status0) & (1 << 2))) {
+               mdelay(10);
+               i++;
+               if (i > 10) {
+                       error("index1 frequency change overtime, reset\n");
+                       hang();

Please have this return an error, so there is just one hang(), at the top-level caller. Also consider using debug() here.

Quoted 70 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +70 -0
+               }
+       }
+
+       i = 0;
+       writel(RK_CLRSETBITS(1 << 1, 1 << 1), &rk3399_ddr_cic->cic_ctrl0);
+       while (!(readl(&rk3399_ddr_cic->cic_status0) & (1 << 0))) {
+               mdelay(10);
+               if (i>10) {
+                       error("index1 frequency done overtime, reset\n");
+                       hang();
+               }
+       }
+
+       for (channel = 0; channel < ch_count; channel++) {
+               denali_phy = rk3399_ddr_publ[channel]->denali_phy;
+               clrsetbits_le32(&denali_phy[896], (0x3 << 8) | 1, 1 << 8);
+               if (data_training(channel, sdram_params, PI_FULL_TRAINING)) {
+                       error("index1 training failed, reset\n");
+                       hang();
+               }
+       }
+}
+static struct rk3399_sdram_params sdram_configs[] = {
+#include "sdram-lpddr3-4GB.inc"
+};
+
+void sdram_init(const struct rk3399_sdram_params *sdram_params)
+{
+       unsigned char dramtype = sdram_params->dramtype;
+       unsigned int ddr_freq = sdram_params->ddr_freq;
+       int channel;
+
+       debug("Starting SDRAM initialization...\n");
+
+       if ((dramtype == DDR3 && ddr_freq > 800*MHz) ||
+           (dramtype == LPDDR3 && ddr_freq > 933*MHz) ||
+           (dramtype == LPDDR4 && ddr_freq > 800*MHz))
+               error("SDRAM frequency is to high!");
+
+
+       for (channel = 0; channel < 2; channel++) {
+               phy_dll_bypass_set(channel, rk3399_ddr_publ[channel], ddr_freq);
+
+               if (channel >= sdram_params->num_channels)
+                       continue;
+
+               /*
+                * TODO: we need to find the root cause why this
+                * step may fail, before that, we just reset the
+                * system, and start again.
+                */
+               if (pctl_cfg(channel, sdram_params) != 0) {
+                       printf("pctl_cfg fail, reset\n");
+                       hang();
+               }
+
+               /* LPDDR2/LPDDR3 need to wait DAI complete, max 10us */
+               if (dramtype == LPDDR3)
+                       udelay(10);
+
+               if (data_training(channel, sdram_params, PI_FULL_TRAINING)) {
+                       printf("SDRAM initialization failed, reset\n");
+                       hang();
+               }
+
+               set_ddrconfig(sdram_params, channel,
+                             sdram_params->ch[channel].ddrconfig);
+       }
+       dram_all_config(sdram_params);
+       switch_to_phy_index1(sdram_params);

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 1052

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+
+       debug("Finish SDRAM initialization...\n");
+}
+#endif
+
+size_t sdram_size_mb(void)
+{
+       u32 rank, col, bk, cs0_row, cs1_row, bw, row_3_4;
+       size_t chipsize_mb = 0;
+       static size_t size_mb = 0;

Do we need this? Perhaps we can just calculate it each time? It will only be called once or twice I suspect.

Quoted 44 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +44 -0
+       u32 ch;
+
+       if (!size_mb) {
+               u32 sys_reg = readl(&rk3399_pmugrf->os_reg2);
+               u32 ch_num = SYS_REG_DEC_NUM_CH(sys_reg);
+
+               for (ch = 0; ch < ch_num; ch++) {
+                       rank = SYS_REG_DEC_RANK(sys_reg, ch);
+                       col = SYS_REG_DEC_COL(sys_reg, ch);
+                       bk = SYS_REG_DEC_BK(sys_reg, ch);
+                       cs0_row = SYS_REG_DEC_CS0_ROW(sys_reg, ch);
+                       cs1_row = SYS_REG_DEC_CS1_ROW(sys_reg, ch);
+                       bw = SYS_REG_DEC_BW(sys_reg, ch);
+                       row_3_4 = SYS_REG_DEC_ROW_3_4(sys_reg, ch);
+
+                       chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
+
+                       if (rank > 1)
+                               chipsize_mb += chipsize_mb >>
+                                       (cs0_row - cs1_row);
+                       if (row_3_4)
+                               chipsize_mb = chipsize_mb * 3 / 4;
+                       size_mb += chipsize_mb;
+               }
+
+               /*
+                * we use the 0x00000000~0xf7ffffff space
+                * since 0xf8000000~0xffffffff is soc register space
+                * so we reserve it
+                */
+               size_mb = min_t(size_t, size_mb, 0xf8000000/(1<<20));
+       }
+
+       return size_mb;
+}
+
+static int rk3399_dmc_probe(struct udevice *dev)
+{
+#ifdef CONFIG_SPL_BUILD
+       struct rk3399_sdram_params *plat = sdram_configs;
+       int ret;
+       struct clk ddr_clk;
+
+       ret = clk_get_by_index(dev, 0, &ddr_clk);

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 1106

Quoted 10 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +10 -0
+       if (ret){
+               debug("%s clk get failed %d\n", __func__, ret);
+               return ret;
+       }
+       ret = clk_set_rate(&ddr_clk, plat->ddr_freq);
+       if (ret < 0){
+               debug("%s clk set failed %d\n", __func__, ret);
+               return ret;
+       }
+       sdram_init(plat);

Check return value here.

Quoted 16 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +15 -1
+#endif
+       return 0;
+}
+
+static const struct udevice_id rk3399_dmc_ids[] = {
+       { .compatible = "rockchip,rk3399-dmc" },
+       { }
+};
+
+U_BOOT_DRIVER(dmc_rk3399) = {
+       .name = "rockchip_rk3399_dmc",
+       .id = UCLASS_RAM,
+       .of_match = rk3399_dmc_ids,
+       .probe = rk3399_dmc_probe,
+};
--
Quoted 2 lines

1.9.1

Regards,
Simon

author
#5 lore raw

RFC PATCH 2/3 arm64: rk3399: add ddr controller driver

Hi Simon,

Thanks for your review, I would like to take all the comments but
one below,
because I think the change only get a lot of work to do but have no any help on understand the source code, because each of shift/mask operation has comment
for it.

I have spend a lot of time on make different controller base
addresses from dts
and gather them into one structure, which we think it does not make any helpful on
source code itself but only met some bug on of-platdata for address decode.

There are hundreds of ctrl/pi/phy register in this dram controller,
I don't think it's a
good idea to add MASK/SHIFT MACRO definition for all of them since comments are already there.

I will send my patch V2 without mask/shift change, but updates by
your other comments.

Thanks,
- Kever

Quoted 10 lines · Simon Glass
Quoted 8 lines

arch/arm/mach-rockchip/rk3399/sdram_rk3399.c line 134

Quoted 8 lines
arch/arm/mach-rockchip/rk3399/sdram_rk3399.c new file +8 -0
+{
+       u32 *denali_phy = ddr_publ_regs->denali_phy;
+       if (freq <= 125*MHz) {
+               /* phy_sw_master_mode_X PHY_86/214/342/470 4bits offset_8 */
+               setbits_le32(&denali_phy[86], (0x3 << 2) << 8);
+               setbits_le32(&denali_phy[214], (0x3 << 2) << 8);
+               setbits_le32(&denali_phy[342], (0x3 << 2) << 8);
+               setbits_le32(&denali_phy[470], (0x3 << 2) << 8);

These should really be defined as SHIFT and MASK values.

author
#6 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

In reply to Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

---

 common/spl/Kconfig   |  14 +++
 common/spl/Makefile  |   1 +
 common/spl/spl.c     |   4 +
 common/spl/spl_atf.c |  91 ++++++++++++++++
 include/atf_common.h | 295 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/spl.h        |   1 +
 6 files changed, 406 insertions(+)
 create mode 100644 common/spl/spl_atf.c
 create mode 100644 include/atf_common.h
common/spl/Kconfig +14 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
 	  means of transmitting U-Boot over a serial line for using in SPL,
 	  with a checksum to ensure correctness.
 
+config SPL_ATF_SUPPORT
+	bool "Support ARM trust firmware"
+	depends on SPL
+	help
+	  ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+	  load by SPL(consider as BL2 in ATF).
+	  More detail at: https://github.com/ARM-software/arm-trusted-firmware
+
+config SPL_ATF_TEXT_BASE
+	depends on SPL_ATF_SUPPORT
+	hex "ATF TEXT BASE addr"
+	help
+	  This is the base address in memory for ATF text and entry point.
+
 config TPL_ENV_SUPPORT
 	bool "Support an environment"
 	depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
 obj-$(CONFIG_SPL_UBI) += spl_ubi.o
 obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
 obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
 obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
 obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
 obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +4 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	      gd->malloc_ptr / 1024);
 #endif
 
+#ifdef CONFIG_SPL_ATF_SUPPORT
+	bl31_entry();
+#endif
+
 	debug("loaded - jumping to U-Boot...");
 	spl_board_prepare_for_boot();
 	jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +91 -0

91 lines not on this page · raw message

include/atf_common.h new file +295 -0

295 lines not on this page · raw message

include/spl.h +1 -0
index 918c464..ab5eaca 100644
@@ -251,4 +251,5 @@ int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr);
 int spl_mmc_load_image(struct spl_image_info *spl_image,
 		       struct spl_boot_device *bootdev);
 
+void bl31_entry(void);
 #endif
Signature · 1 line
1.9.1
#7 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

Quoted 4 lines · Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary

SPL replaces BL2 in ATF

Quoted 4 lines

like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

But the main question for this is how do load that images and in which format. It means I would think that you will introduce fit format which contain BL33(U-Boot), BL32(secure os) and BL31(ATF) and SPL will be able to load all of them.

If you look at zynqmp I did a small trick where I consider case that with ATF it is OS boot where kernel is ATF and dtb is full u-boot to get it boot.

If you adopt fit format then I expect SPL will be able to remember which part is where and based on that fill structure for ATF.
Then SPL_ATF_TEXT_BASE address is not needed because it will be read from fit format.

Quoted 72 lines

---

 common/spl/Kconfig   |  14 +++
 common/spl/Makefile  |   1 +
 common/spl/spl.c     |   4 +
 common/spl/spl_atf.c |  91 ++++++++++++++++
 include/atf_common.h | 295 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/spl.h        |   1 +
 6 files changed, 406 insertions(+)
 create mode 100644 common/spl/spl_atf.c
 create mode 100644 include/atf_common.h
common/spl/Kconfig +14 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
 	  means of transmitting U-Boot over a serial line for using in SPL,
 	  with a checksum to ensure correctness.
 
+config SPL_ATF_SUPPORT
+	bool "Support ARM trust firmware"
+	depends on SPL
+	help
+	  ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+	  load by SPL(consider as BL2 in ATF).
+	  More detail at: https://github.com/ARM-software/arm-trusted-firmware
+
+config SPL_ATF_TEXT_BASE
+	depends on SPL_ATF_SUPPORT
+	hex "ATF TEXT BASE addr"
+	help
+	  This is the base address in memory for ATF text and entry point.
+
 config TPL_ENV_SUPPORT
 	bool "Support an environment"
 	depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
 obj-$(CONFIG_SPL_UBI) += spl_ubi.o
 obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
 obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
 obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
 obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
 obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +4 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	      gd->malloc_ptr / 1024);
 #endif
 
+#ifdef CONFIG_SPL_ATF_SUPPORT
+	bl31_entry();
+#endif
+
 	debug("loaded - jumping to U-Boot...");
 	spl_board_prepare_for_boot();
 	jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +0 -0
new file mode 100644
index 0000000..cf23b7a

common/spl/spl_atf.c line 7

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2016 Rockchip Electronic Co.,Ltd
+ * Written by Kever Yang <[email protected]>
+ *
+ * origin from arm-trust-firmware
+ * plat/arm/common/arm_bl2_setup.c
+ * SPDX-License-Identifier:     GPL-2.0+

this is not based on gpl file that's why license should be different.

Quoted 36 lines
common/spl/spl_atf.c new file +36 -0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <spl.h>
+#include <atf_common.h>
+
+static struct bl2_to_bl31_params_mem_t bl31_params_mem;
+static struct bl31_params_t *bl2_to_bl31_params;
+
+/*******************************************************************************
+ * This function assigns a pointer to the memory that the platform has kept
+ * aside to pass platform specific and trusted firmware related information
+ * to BL31. This memory is allocated by allocating memory to
+ * bl2_to_bl31_params_mem_t structure which is a superset of all the
+ * structure whose information is passed to BL31
+ * NOTE: This function should be called only once and should be done
+ * before generating params to BL31
+ ******************************************************************************/
+struct bl31_params_t *bl2_plat_get_bl31_params(void)
+{
+	struct entry_point_info_t *bl33_ep_info;
+
+	/*
+	 * Initialise the memory for all the arguments that needs to
+	 * be passed to BL31
+	 */
+	memset(&bl31_params_mem, 0, sizeof(struct bl2_to_bl31_params_mem_t));
+
+	/* Assign memory for TF related information */
+	bl2_to_bl31_params = &bl31_params_mem.bl31_params;
+	SET_PARAM_HEAD(bl2_to_bl31_params, PARAM_BL31, VERSION_1, 0);
+
+	/* Fill BL31 related information */
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl31_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);

common/spl/spl_atf.c line 53

Quoted 10 lines
common/spl/spl_atf.c new file +10 -0
+
+	/* Fill BL32 related information if it exists */
+#ifdef BL32_BASE
+	bl2_to_bl31_params->bl32_ep_info = &bl31_params_mem.bl32_ep_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl32_ep_info, PARAM_EP,
+		       VERSION_1, 0);
+	bl2_to_bl31_params->bl32_image_info = &bl31_params_mem.bl32_image_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl32_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+#endif /* BL32_BASE */

Is this used?

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
+
+	/* Fill BL33 related information */
+	bl2_to_bl31_params->bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+	bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+	SET_PARAM_HEAD(bl33_ep_info, PARAM_EP, VERSION_1, EP_NON_SECURE);
+
+	/* BL33 expects to receive the primary CPU MPID (through x0) */

common/spl/spl_atf.c line 70

Quoted 10 lines
common/spl/spl_atf.c new file +10 -0
+	bl33_ep_info->args.arg0 = 0xffff & read_mpidr();
+	bl33_ep_info->pc = CONFIG_SYS_TEXT_BASE;
+	bl33_ep_info->spsr = SPSR_64(MODE_EL2, MODE_SP_ELX,
+				     DISABLE_ALL_EXECPTIONS);
+
+	bl2_to_bl31_params->bl33_image_info = &bl31_params_mem.bl33_image_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl33_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+
+

double lines.

Quoted 45 lines
common/spl/spl_atf.c new file +21 -0
+	return bl2_to_bl31_params;
+}
+
+void raw_write_daif(uint32_t daif)
+{
+	__asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory");
+}
+
+void bl31_entry(void)
+{
+	struct bl31_params_t *bl31_params;
+	void (*entry)(struct bl31_params_t *params, void *plat_params) = NULL;
+
+	bl31_params = bl2_plat_get_bl31_params();
+	entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
+
+	raw_write_daif(SPSR_EXCEPTION_MASK);
+	dcache_disable();
+
+	entry(bl31_params, NULL);
+}
include/atf_common.h new file +18 -0
new file mode 100644
index 0000000..8351302
@@ -0,0 +1,295 @@
+/*
+ * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * Neither the name of ARM nor the names of its contributors may be used
+ * to endorse or promote products derived from this software without specific
+ * prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"

include/atf_common.h line 28

Quoted 10 lines
include/atf_common.h new file +10 -0
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.

This should be probably in SPDX format.

Quoted 273 lines
include/atf_common.h new file +267 -0
+ */
+
+#ifndef __BL_COMMON_H__
+#define __BL_COMMON_H__
+
+#define SECURE		0x0
+#define NON_SECURE	0x1
+#define sec_state_is_valid(s) (((s) == SECURE) || ((s) == NON_SECURE))
+
+#define UP	1
+#define DOWN	0
+
+/*******************************************************************************
+ * Constants to identify the location of a memory region in a given memory
+ * layout.
+******************************************************************************/
+#define TOP	0x1
+#define BOTTOM	!TOP
+
+/*******************************************************************************
+ * Constants that allow assembler code to access members of and the
+ * 'entry_point_info' structure at their correct offsets.
+ ******************************************************************************/
+#define ENTRY_POINT_INFO_PC_OFFSET	0x08
+#define ENTRY_POINT_INFO_ARGS_OFFSET	0x18
+
+/* The following are used to set/get image attributes. */
+#define PARAM_EP_SECURITY_MASK		(0x1)
+
+#define GET_SECURITY_STATE(x) (x & PARAM_EP_SECURITY_MASK)
+#define SET_SECURITY_STATE(x, security) \
+			((x) = ((x) & ~PARAM_EP_SECURITY_MASK) | (security))
+
+
+/*
+ * The following are used for image state attributes.
+ * Image can only be in one of the following state.
+ */
+#define IMAGE_STATE_RESET			0
+#define IMAGE_STATE_COPIED			1
+#define IMAGE_STATE_COPYING			2
+#define IMAGE_STATE_AUTHENTICATED		3
+#define IMAGE_STATE_EXECUTED			4
+#define IMAGE_STATE_INTERRUPTED			5
+
+#define EP_SECURE	0x0
+#define EP_NON_SECURE	0x1
+
+#define EP_EE_MASK	0x2
+#define EP_EE_LITTLE	0x0
+#define EP_EE_BIG	0x2
+#define EP_GET_EE(x) (x & EP_EE_MASK)
+#define EP_SET_EE(x, ee) ((x) = ((x) & ~EP_EE_MASK) | (ee))
+
+#define EP_ST_MASK	0x4
+#define EP_ST_DISABLE	0x0
+#define EP_ST_ENABLE	0x4
+#define EP_GET_ST(x) (x & EP_ST_MASK)
+#define EP_SET_ST(x, ee) ((x) = ((x) & ~EP_ST_MASK) | (ee))
+
+#define EP_EXE_MASK	0x8
+#define NON_EXECUTABLE	0x0
+#define EXECUTABLE	0x8
+#define EP_GET_EXE(x) (x & EP_EXE_MASK)
+#define EP_SET_EXE(x, ee) ((x) = ((x) & ~EP_EXE_MASK) | (ee))
+
+#define PARAM_EP		0x01
+#define PARAM_IMAGE_BINARY	0x02
+#define PARAM_BL31		0x03
+
+#define VERSION_1	0x01
+
+#define INVALID_IMAGE_ID		(0xFFFFFFFF)
+
+#define SET_PARAM_HEAD(_p, _type, _ver, _attr) do { \
+	(_p)->h.type = (uint8_t)(_type); \
+	(_p)->h.version = (uint8_t)(_ver); \
+	(_p)->h.size = (uint16_t)sizeof(*_p); \
+	(_p)->h.attr = (uint32_t)(_attr) ; \
+	} while (0)
+
+/* Following is used for populating structure members statically. */
+#define SET_STATIC_PARAM_HEAD(_p, _type, _ver, _p_type, _attr)	\
+	._p.h.type = (uint8_t)(_type), \
+	._p.h.version = (uint8_t)(_ver), \
+	._p.h.size = (uint16_t)sizeof(_p_type), \
+	._p.h.attr = (uint32_t)(_attr)
+
+#define MODE_RW_SHIFT	0x4
+#define MODE_RW_MASK	0x1
+#define MODE_RW_64	0x0
+#define MODE_RW_32	0x1
+
+#define MODE_EL_SHIFT	0x2
+#define MODE_EL_MASK	0x3
+#define MODE_EL3	0x3
+#define MODE_EL2	0x2
+#define MODE_EL1	0x1
+#define MODE_EL0	0x0
+
+#define MODE_SP_SHIFT	0x0
+#define MODE_SP_MASK	0x1
+#define MODE_SP_EL0	0x0
+#define MODE_SP_ELX	0x1
+
+#define SPSR_DAIF_SHIFT	6
+#define SPSR_DAIF_MASK	0x0f
+
+#define DAIF_FIQ_BIT (1<<0)
+#define DAIF_IRQ_BIT (1<<0)
+#define DAIF_ABT_BIT (1<<0)
+#define DAIF_DBG_BIT (1<<0)
+#define DISABLE_ALL_EXECPTIONS	\
+	(DAIF_FIQ_BIT | DAIF_IRQ_BIT | DAIF_ABT_BIT | DAIF_DBG_BIT)
+
+#define SPSR_64(el, sp, daif)		\
+	(MODE_RW_64 << MODE_RW_SHIFT |	\
+	 ((el) & MODE_EL_MASK) << MODE_EL_SHIFT |	\
+	 ((sp) & MODE_SP_MASK) << MODE_SP_SHIFT |	\
+	 ((daif) & SPSR_DAIF_MASK) << SPSR_DAIF_SHIFT)
+
+/*******************************************************************************
+ * Constants to indicate type of exception to the common exception handler.
+ ******************************************************************************/
+#define SYNC_EXCEPTION_SP_EL0		0x0
+#define IRQ_SP_EL0			0x1
+#define FIQ_SP_EL0			0x2
+#define SERROR_SP_EL0			0x3
+#define SYNC_EXCEPTION_SP_ELX		0x4
+#define IRQ_SP_ELX			0x5
+#define FIQ_SP_ELX			0x6
+#define SERROR_SP_ELX			0x7
+#define SYNC_EXCEPTION_AARCH64		0x8
+#define IRQ_AARCH64			0x9
+#define FIQ_AARCH64			0xa
+#define SERROR_AARCH64			0xb
+#define SYNC_EXCEPTION_AARCH32		0xc
+#define IRQ_AARCH32			0xd
+#define FIQ_AARCH32			0xe
+#define SERROR_AARCH32			0xf
+
+#define SPSR_USE_L           0
+#define SPSR_USE_H           1
+#define SPSR_L_H_MASK        1
+#define SPSR_M_SHIFT         4
+#define SPSR_ERET_32         (1 << SPSR_M_SHIFT)
+#define SPSR_ERET_64         (0 << SPSR_M_SHIFT)
+#define SPSR_FIQ             (1 << 6)
+#define SPSR_IRQ             (1 << 7)
+#define SPSR_SERROR          (1 << 8)
+#define SPSR_DEBUG           (1 << 9)
+#define SPSR_EXCEPTION_MASK  (SPSR_FIQ | SPSR_IRQ | SPSR_SERROR | SPSR_DEBUG)
+
+#ifndef __ASSEMBLY__
+
+/*******************************************************************************
+ * Structure used for telling the next BL how much of a particular type of
+ * memory is available for its use and how much is already used.
+ ******************************************************************************/
+struct aapcs64_params_t {
+	unsigned long arg0;
+	unsigned long arg1;
+	unsigned long arg2;
+	unsigned long arg3;
+	unsigned long arg4;
+	unsigned long arg5;
+	unsigned long arg6;
+	unsigned long arg7;
+};
+
+/***************************************************************************
+ * This structure provides version information and the size of the
+ * structure, attributes for the structure it represents
+ ***************************************************************************/
+struct param_header_t {
+	uint8_t type;		/* type of the structure */
+	uint8_t version;    /* version of this structure */
+	uint16_t size;      /* size of this structure in bytes */
+	uint32_t attr;      /* attributes: unused bits SBZ */
+};
+
+/*****************************************************************************
+ * This structure represents the superset of information needed while
+ * switching exception levels. The only two mechanisms to do so are
+ * ERET & SMC. Security state is indicated using bit zero of header
+ * attribute
+ * NOTE: BL1 expects entrypoint followed by spsr at an offset from the start
+ * of this structure defined by the macro `ENTRY_POINT_INFO_PC_OFFSET` while
+ * processing SMC to jump to BL31.
+ *****************************************************************************/
+struct entry_point_info_t {
+	struct param_header_t h;
+	uintptr_t pc;
+	uint32_t spsr;
+	struct aapcs64_params_t args;
+};
+
+/*****************************************************************************
+ * Image info binary provides information from the image loader that
+ * can be used by the firmware to manage available trusted RAM.
+ * More advanced firmware image formats can provide additional
+ * information that enables optimization or greater flexibility in the
+ * common firmware code
+ *****************************************************************************/
+struct atf_image_info_t {
+	struct param_header_t h;
+	uintptr_t image_base;   /* physical address of base of image */
+	uint32_t image_size;    /* bytes read from image file */
+};
+
+/*****************************************************************************
+ * The image descriptor struct definition.
+ *****************************************************************************/
+struct image_desc_t {
+	/* Contains unique image id for the image. */
+	unsigned int image_id;
+	/*
+	 * This member contains Image state information.
+	 * Refer IMAGE_STATE_XXX defined above.
+	 */
+	unsigned int state;
+	uint32_t copied_size;	/* image size copied in blocks */
+	struct atf_image_info_t atf_image_info;
+	struct entry_point_info_t ep_info;
+};
+
+/*******************************************************************************
+ * This structure represents the superset of information that can be passed to
+ * BL31 e.g. while passing control to it from BL2. The BL32 parameters will be
+ * populated only if BL2 detects its presence. A pointer to a structure of this
+ * type should be passed in X0 to BL31's cold boot entrypoint.
+ *
+ * Use of this structure and the X0 parameter is not mandatory: the BL31
+ * platform code can use other mechanisms to provide the necessary information
+ * about BL32 and BL33 to the common and SPD code.
+ *
+ * BL31 image information is mandatory if this structure is used. If either of
+ * the optional BL32 and BL33 image information is not provided, this is
+ * indicated by the respective image_info pointers being zero.
+ ******************************************************************************/
+struct bl31_params_t {
+	struct param_header_t h;
+	struct atf_image_info_t *bl31_image_info;
+	struct entry_point_info_t *bl32_ep_info;
+	struct atf_image_info_t *bl32_image_info;
+	struct entry_point_info_t *bl33_ep_info;
+	struct atf_image_info_t *bl33_image_info;
+};
+
+/*******************************************************************************
+ * This structure represents the superset of information that is passed to
+ * BL31, e.g. while passing control to it from BL2, bl31_params
+ * and other platform specific params
+ ******************************************************************************/
+struct bl2_to_bl31_params_mem_t {
+	struct bl31_params_t bl31_params;
+	struct atf_image_info_t bl31_image_info;
+	struct atf_image_info_t bl32_image_info;
+	struct atf_image_info_t bl33_image_info;
+	struct entry_point_info_t bl33_ep_info;
+	struct entry_point_info_t bl32_ep_info;
+	struct entry_point_info_t bl31_ep_info;
+};
+
+#endif /*__ASSEMBLY__*/
+
+#endif /* __BL_COMMON_H__ */
include/spl.h +0 -0
index 918c464..ab5eaca 100644

include/spl.h int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr); · line 255

Quoted 5 lines
include/spl.h +1 -0
@@ -251,4 +251,5 @@ int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr);
 int spl_mmc_load_image(struct spl_image_info *spl_image,
 		       struct spl_boot_device *bootdev);
 
+void bl31_entry(void);
 #endif

Thanks,
Michal

author
#8 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

Hi Michal,

Thanks for your comments.

Quoted 6 lines · Michal Simek
Quoted 4 lines · Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary

SPL replaces BL2 in ATF

OK, will follow your comment in next patch.

Quoted 9 lines
Quoted 5 lines

like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

But the main question for this is how do load that images and in which format. It means I would think that you will introduce fit format which contain BL33(U-Boot), BL32(secure os) and BL31(ATF) and SPL will be able to load all of them.

Yes, I use FIT format to contain BL33 and BL32 and SPL load all of them.

Quoted 4 lines

If you look at zynqmp I did a small trick where I consider case that with ATF it is OS boot where kernel is ATF and dtb is full u-boot to get it boot.

This is a good idea, and it look fine for support ATF in SPL in local source code,
but it will be better if we have an official support for ATF, right?

Quoted 5 lines

If you adopt fit format then I expect SPL will be able to remember which part is where and based on that fill structure for ATF.
Then SPL_ATF_TEXT_BASE address is not needed because it will be read from fit format.

Yes, you are right, SPL_ATF_TEXT_BASE is not a must, we gen get it from fit.

Quoted 74 lines
Quoted 74 lines

---

  common/spl/Kconfig   |  14 +++
  common/spl/Makefile  |   1 +
  common/spl/spl.c     |   4 +
  common/spl/spl_atf.c |  91 ++++++++++++++++
  include/atf_common.h | 295 +++++++++++++++++++++++++++++++++++++++++++++++++++
  include/spl.h        |   1 +
  6 files changed, 406 insertions(+)

create mode 100644 common/spl/spl_atf.c
create mode 100644 include/atf_common.h

common/spl/Kconfig +14 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
  	  means of transmitting U-Boot over a serial line for using in SPL,
  	  with a checksum to ensure correctness.
 
+config SPL_ATF_SUPPORT
+	bool "Support ARM trust firmware"
+	depends on SPL
+	help
+	  ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+	  load by SPL(consider as BL2 in ATF).
+	  More detail at: https://github.com/ARM-software/arm-trusted-firmware
+
+config SPL_ATF_TEXT_BASE
+	depends on SPL_ATF_SUPPORT
+	hex "ATF TEXT BASE addr"
+	help
+	  This is the base address in memory for ATF text and entry point.
+
  config TPL_ENV_SUPPORT
  	bool "Support an environment"
  	depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
  obj-$(CONFIG_SPL_UBI) += spl_ubi.o
  obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
  obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
  obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +4 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
  	      gd->malloc_ptr / 1024);
  #endif
 
+#ifdef CONFIG_SPL_ATF_SUPPORT
+	bl31_entry();
+#endif
+
  	debug("loaded - jumping to U-Boot...");
  	spl_board_prepare_for_boot();
  	jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +0 -0
new file mode 100644
index 0000000..cf23b7a
@@ -0,0 +1,91 @@
Quoted 8 lines

common/spl/spl_atf.c line 7

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
+/*
+ * Copyright (C) 2016 Rockchip Electronic Co.,Ltd
+ * Written by Kever Yang <[email protected]>
+ *
+ * origin from arm-trust-firmware
+ * plat/arm/common/arm_bl2_setup.c
+ * SPDX-License-Identifier:     GPL-2.0+

this is not based on gpl file that's why license should be different.

Sorry, I do not get what your mean, I'm not good at license policy, ARM ATF use its own license, what should I do for license when I use code from ATF?

Quoted 41 lines
Quoted 41 lines
common/spl/spl_atf.c new file +39 -0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <spl.h>
+#include <atf_common.h>
+
+static struct bl2_to_bl31_params_mem_t bl31_params_mem;
+static struct bl31_params_t *bl2_to_bl31_params;
+
+/*******************************************************************************
+ * This function assigns a pointer to the memory that the platform has kept
+ * aside to pass platform specific and trusted firmware related information
+ * to BL31. This memory is allocated by allocating memory to
+ * bl2_to_bl31_params_mem_t structure which is a superset of all the
+ * structure whose information is passed to BL31
+ * NOTE: This function should be called only once and should be done
+ * before generating params to BL31
+ ******************************************************************************/
+struct bl31_params_t *bl2_plat_get_bl31_params(void)
+{
+	struct entry_point_info_t *bl33_ep_info;
+
+	/*
+	 * Initialise the memory for all the arguments that needs to
+	 * be passed to BL31
+	 */
+	memset(&bl31_params_mem, 0, sizeof(struct bl2_to_bl31_params_mem_t));
+
+	/* Assign memory for TF related information */
+	bl2_to_bl31_params = &bl31_params_mem.bl31_params;
+	SET_PARAM_HEAD(bl2_to_bl31_params, PARAM_BL31, VERSION_1, 0);
+
+	/* Fill BL31 related information */
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl31_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+
+	/* Fill BL32 related information if it exists */
+#ifdef BL32_BASE
Quoted 8 lines

common/spl/spl_atf.c line 53

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
+	bl2_to_bl31_params->bl32_ep_info = &bl31_params_mem.bl32_ep_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl32_ep_info, PARAM_EP,
+		       VERSION_1, 0);
+	bl2_to_bl31_params->bl32_image_info = &bl31_params_mem.bl32_image_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl32_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+#endif /* BL32_BASE */

Is this used?

Not use for me now, but it may useful later, because we need to fill info about bl32 if there is.

Quoted 11 lines
Quoted 11 lines
common/spl/spl_atf.c new file +10 -0
+
+	/* Fill BL33 related information */
+	bl2_to_bl31_params->bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+	bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+	SET_PARAM_HEAD(bl33_ep_info, PARAM_EP, VERSION_1, EP_NON_SECURE);
+
+	/* BL33 expects to receive the primary CPU MPID (through x0) */
+	bl33_ep_info->args.arg0 = 0xffff & read_mpidr();
+	bl33_ep_info->pc = CONFIG_SYS_TEXT_BASE;
+	bl33_ep_info->spsr = SPSR_64(MODE_EL2, MODE_SP_ELX,
Quoted 8 lines

common/spl/spl_atf.c line 70

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
+				     DISABLE_ALL_EXECPTIONS);
+
+	bl2_to_bl31_params->bl33_image_info = &bl31_params_mem.bl33_image_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl33_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+
+

double lines.

Will fix in next patch, confuse why checkpatch did not find this.

Quoted 49 lines
Quoted 49 lines
common/spl/spl_atf.c new file +21 -0
+	return bl2_to_bl31_params;
+}
+
+void raw_write_daif(uint32_t daif)
+{
+	__asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory");
+}
+
+void bl31_entry(void)
+{
+	struct bl31_params_t *bl31_params;
+	void (*entry)(struct bl31_params_t *params, void *plat_params) = NULL;
+
+	bl31_params = bl2_plat_get_bl31_params();
+	entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
+
+	raw_write_daif(SPSR_EXCEPTION_MASK);
+	dcache_disable();
+
+	entry(bl31_params, NULL);
+}
include/atf_common.h new file +21 -0
new file mode 100644
index 0000000..8351302
@@ -0,0 +1,295 @@
+/*
+ * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * Neither the name of ARM nor the names of its contributors may be used
+ * to endorse or promote products derived from this software without specific
+ * prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
Quoted 8 lines

include/atf_common.h line 28

Quoted 7 lines
include/atf_common.h new file +7 -0
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.

This should be probably in SPDX format.

Like previous reply, I don't know how to deal with this license, do you mean I can use
SPDX license without any information about the origin License?

Thanks,
- Kever

Quoted 286 lines
Quoted 275 lines
include/atf_common.h new file +267 -0
+ */
+
+#ifndef __BL_COMMON_H__
+#define __BL_COMMON_H__
+
+#define SECURE		0x0
+#define NON_SECURE	0x1
+#define sec_state_is_valid(s) (((s) == SECURE) || ((s) == NON_SECURE))
+
+#define UP	1
+#define DOWN	0
+
+/*******************************************************************************
+ * Constants to identify the location of a memory region in a given memory
+ * layout.
+******************************************************************************/
+#define TOP	0x1
+#define BOTTOM	!TOP
+
+/*******************************************************************************
+ * Constants that allow assembler code to access members of and the
+ * 'entry_point_info' structure at their correct offsets.
+ ******************************************************************************/
+#define ENTRY_POINT_INFO_PC_OFFSET	0x08
+#define ENTRY_POINT_INFO_ARGS_OFFSET	0x18
+
+/* The following are used to set/get image attributes. */
+#define PARAM_EP_SECURITY_MASK		(0x1)
+
+#define GET_SECURITY_STATE(x) (x & PARAM_EP_SECURITY_MASK)
+#define SET_SECURITY_STATE(x, security) \
+			((x) = ((x) & ~PARAM_EP_SECURITY_MASK) | (security))
+
+
+/*
+ * The following are used for image state attributes.
+ * Image can only be in one of the following state.
+ */
+#define IMAGE_STATE_RESET			0
+#define IMAGE_STATE_COPIED			1
+#define IMAGE_STATE_COPYING			2
+#define IMAGE_STATE_AUTHENTICATED		3
+#define IMAGE_STATE_EXECUTED			4
+#define IMAGE_STATE_INTERRUPTED			5
+
+#define EP_SECURE	0x0
+#define EP_NON_SECURE	0x1
+
+#define EP_EE_MASK	0x2
+#define EP_EE_LITTLE	0x0
+#define EP_EE_BIG	0x2
+#define EP_GET_EE(x) (x & EP_EE_MASK)
+#define EP_SET_EE(x, ee) ((x) = ((x) & ~EP_EE_MASK) | (ee))
+
+#define EP_ST_MASK	0x4
+#define EP_ST_DISABLE	0x0
+#define EP_ST_ENABLE	0x4
+#define EP_GET_ST(x) (x & EP_ST_MASK)
+#define EP_SET_ST(x, ee) ((x) = ((x) & ~EP_ST_MASK) | (ee))
+
+#define EP_EXE_MASK	0x8
+#define NON_EXECUTABLE	0x0
+#define EXECUTABLE	0x8
+#define EP_GET_EXE(x) (x & EP_EXE_MASK)
+#define EP_SET_EXE(x, ee) ((x) = ((x) & ~EP_EXE_MASK) | (ee))
+
+#define PARAM_EP		0x01
+#define PARAM_IMAGE_BINARY	0x02
+#define PARAM_BL31		0x03
+
+#define VERSION_1	0x01
+
+#define INVALID_IMAGE_ID		(0xFFFFFFFF)
+
+#define SET_PARAM_HEAD(_p, _type, _ver, _attr) do { \
+	(_p)->h.type = (uint8_t)(_type); \
+	(_p)->h.version = (uint8_t)(_ver); \
+	(_p)->h.size = (uint16_t)sizeof(*_p); \
+	(_p)->h.attr = (uint32_t)(_attr) ; \
+	} while (0)
+
+/* Following is used for populating structure members statically. */
+#define SET_STATIC_PARAM_HEAD(_p, _type, _ver, _p_type, _attr)	\
+	._p.h.type = (uint8_t)(_type), \
+	._p.h.version = (uint8_t)(_ver), \
+	._p.h.size = (uint16_t)sizeof(_p_type), \
+	._p.h.attr = (uint32_t)(_attr)
+
+#define MODE_RW_SHIFT	0x4
+#define MODE_RW_MASK	0x1
+#define MODE_RW_64	0x0
+#define MODE_RW_32	0x1
+
+#define MODE_EL_SHIFT	0x2
+#define MODE_EL_MASK	0x3
+#define MODE_EL3	0x3
+#define MODE_EL2	0x2
+#define MODE_EL1	0x1
+#define MODE_EL0	0x0
+
+#define MODE_SP_SHIFT	0x0
+#define MODE_SP_MASK	0x1
+#define MODE_SP_EL0	0x0
+#define MODE_SP_ELX	0x1
+
+#define SPSR_DAIF_SHIFT	6
+#define SPSR_DAIF_MASK	0x0f
+
+#define DAIF_FIQ_BIT (1<<0)
+#define DAIF_IRQ_BIT (1<<0)
+#define DAIF_ABT_BIT (1<<0)
+#define DAIF_DBG_BIT (1<<0)
+#define DISABLE_ALL_EXECPTIONS	\
+	(DAIF_FIQ_BIT | DAIF_IRQ_BIT | DAIF_ABT_BIT | DAIF_DBG_BIT)
+
+#define SPSR_64(el, sp, daif)		\
+	(MODE_RW_64 << MODE_RW_SHIFT |	\
+	 ((el) & MODE_EL_MASK) << MODE_EL_SHIFT |	\
+	 ((sp) & MODE_SP_MASK) << MODE_SP_SHIFT |	\
+	 ((daif) & SPSR_DAIF_MASK) << SPSR_DAIF_SHIFT)
+
+/*******************************************************************************
+ * Constants to indicate type of exception to the common exception handler.
+ ******************************************************************************/
+#define SYNC_EXCEPTION_SP_EL0		0x0
+#define IRQ_SP_EL0			0x1
+#define FIQ_SP_EL0			0x2
+#define SERROR_SP_EL0			0x3
+#define SYNC_EXCEPTION_SP_ELX		0x4
+#define IRQ_SP_ELX			0x5
+#define FIQ_SP_ELX			0x6
+#define SERROR_SP_ELX			0x7
+#define SYNC_EXCEPTION_AARCH64		0x8
+#define IRQ_AARCH64			0x9
+#define FIQ_AARCH64			0xa
+#define SERROR_AARCH64			0xb
+#define SYNC_EXCEPTION_AARCH32		0xc
+#define IRQ_AARCH32			0xd
+#define FIQ_AARCH32			0xe
+#define SERROR_AARCH32			0xf
+
+#define SPSR_USE_L           0
+#define SPSR_USE_H           1
+#define SPSR_L_H_MASK        1
+#define SPSR_M_SHIFT         4
+#define SPSR_ERET_32         (1 << SPSR_M_SHIFT)
+#define SPSR_ERET_64         (0 << SPSR_M_SHIFT)
+#define SPSR_FIQ             (1 << 6)
+#define SPSR_IRQ             (1 << 7)
+#define SPSR_SERROR          (1 << 8)
+#define SPSR_DEBUG           (1 << 9)
+#define SPSR_EXCEPTION_MASK  (SPSR_FIQ | SPSR_IRQ | SPSR_SERROR | SPSR_DEBUG)
+
+#ifndef __ASSEMBLY__
+
+/*******************************************************************************
+ * Structure used for telling the next BL how much of a particular type of
+ * memory is available for its use and how much is already used.
+ ******************************************************************************/
+struct aapcs64_params_t {
+	unsigned long arg0;
+	unsigned long arg1;
+	unsigned long arg2;
+	unsigned long arg3;
+	unsigned long arg4;
+	unsigned long arg5;
+	unsigned long arg6;
+	unsigned long arg7;
+};
+
+/***************************************************************************
+ * This structure provides version information and the size of the
+ * structure, attributes for the structure it represents
+ ***************************************************************************/
+struct param_header_t {
+	uint8_t type;		/* type of the structure */
+	uint8_t version;    /* version of this structure */
+	uint16_t size;      /* size of this structure in bytes */
+	uint32_t attr;      /* attributes: unused bits SBZ */
+};
+
+/*****************************************************************************
+ * This structure represents the superset of information needed while
+ * switching exception levels. The only two mechanisms to do so are
+ * ERET & SMC. Security state is indicated using bit zero of header
+ * attribute
+ * NOTE: BL1 expects entrypoint followed by spsr at an offset from the start
+ * of this structure defined by the macro `ENTRY_POINT_INFO_PC_OFFSET` while
+ * processing SMC to jump to BL31.
+ *****************************************************************************/
+struct entry_point_info_t {
+	struct param_header_t h;
+	uintptr_t pc;
+	uint32_t spsr;
+	struct aapcs64_params_t args;
+};
+
+/*****************************************************************************
+ * Image info binary provides information from the image loader that
+ * can be used by the firmware to manage available trusted RAM.
+ * More advanced firmware image formats can provide additional
+ * information that enables optimization or greater flexibility in the
+ * common firmware code
+ *****************************************************************************/
+struct atf_image_info_t {
+	struct param_header_t h;
+	uintptr_t image_base;   /* physical address of base of image */
+	uint32_t image_size;    /* bytes read from image file */
+};
+
+/*****************************************************************************
+ * The image descriptor struct definition.
+ *****************************************************************************/
+struct image_desc_t {
+	/* Contains unique image id for the image. */
+	unsigned int image_id;
+	/*
+	 * This member contains Image state information.
+	 * Refer IMAGE_STATE_XXX defined above.
+	 */
+	unsigned int state;
+	uint32_t copied_size;	/* image size copied in blocks */
+	struct atf_image_info_t atf_image_info;
+	struct entry_point_info_t ep_info;
+};
+
+/*******************************************************************************
+ * This structure represents the superset of information that can be passed to
+ * BL31 e.g. while passing control to it from BL2. The BL32 parameters will be
+ * populated only if BL2 detects its presence. A pointer to a structure of this
+ * type should be passed in X0 to BL31's cold boot entrypoint.
+ *
+ * Use of this structure and the X0 parameter is not mandatory: the BL31
+ * platform code can use other mechanisms to provide the necessary information
+ * about BL32 and BL33 to the common and SPD code.
+ *
+ * BL31 image information is mandatory if this structure is used. If either of
+ * the optional BL32 and BL33 image information is not provided, this is
+ * indicated by the respective image_info pointers being zero.
+ ******************************************************************************/
+struct bl31_params_t {
+	struct param_header_t h;
+	struct atf_image_info_t *bl31_image_info;
+	struct entry_point_info_t *bl32_ep_info;
+	struct atf_image_info_t *bl32_image_info;
+	struct entry_point_info_t *bl33_ep_info;
+	struct atf_image_info_t *bl33_image_info;
+};
+
+/*******************************************************************************
+ * This structure represents the superset of information that is passed to
+ * BL31, e.g. while passing control to it from BL2, bl31_params
+ * and other platform specific params
+ ******************************************************************************/
+struct bl2_to_bl31_params_mem_t {
+	struct bl31_params_t bl31_params;
+	struct atf_image_info_t bl31_image_info;
+	struct atf_image_info_t bl32_image_info;
+	struct atf_image_info_t bl33_image_info;
+	struct entry_point_info_t bl33_ep_info;
+	struct entry_point_info_t bl32_ep_info;
+	struct entry_point_info_t bl31_ep_info;
+};
+
+#endif /*__ASSEMBLY__*/
+
+#endif /* __BL_COMMON_H__ */
include/spl.h +0 -0
index 918c464..ab5eaca 100644

include/spl.h int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr); · line 255

Quoted 5 lines
include/spl.h +1 -0
@@ -251,4 +251,5 @@ int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr);
  int spl_mmc_load_image(struct spl_image_info *spl_image,
  		       struct spl_boot_device *bootdev);
 
+void bl31_entry(void);
  #endif

Thanks,
Michal

#9 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

Hi,

Quoted 18 lines · Kever Yang

Hi Michal,

Thanks for your comments.

Quoted 6 lines · Michal Simek
Quoted 4 lines · Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary

SPL replaces BL2 in ATF

OK, will follow your comment in next patch.

Quoted 5 lines
Quoted 5 lines

like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

Quoted 6 lines
Quoted 4 lines

But the main question for this is how do load that images and in which format. It means I would think that you will introduce fit format which contain BL33(U-Boot), BL32(secure os) and BL31(ATF) and SPL will be able to load all of them.

Yes, I use FIT format to contain BL33 and BL32 and SPL load all of them.

Do you have some logs? I didn't check the latest code but IIRC it was possible to handle one image and dt not several images which has to be supported. There is also loadables section in fit which can help with this.

Quoted 8 lines
Quoted 4 lines

If you look at zynqmp I did a small trick where I consider case that with ATF it is OS boot where kernel is ATF and dtb is full u-boot to get it boot.

This is a good idea, and it look fine for support ATF in SPL in local source code,
but it will be better if we have an official support for ATF, right?

Definitely having support just for ATF is much better solution than what I use in ZynqMP.

Quoted 9 lines
Quoted 6 lines

If you adopt fit format then I expect SPL will be able to remember which part is where and based on that fill structure for ATF.
Then SPL_ATF_TEXT_BASE address is not needed because it will be read from fit format.

Yes, you are right, SPL_ATF_TEXT_BASE is not a must, we gen get it from fit.

ok.

Quoted 85 lines
Quoted 76 lines
Quoted 76 lines

---

  common/spl/Kconfig   |  14 +++
  common/spl/Makefile  |   1 +
  common/spl/spl.c     |   4 +
  common/spl/spl_atf.c |  91 ++++++++++++++++
  include/atf_common.h | 295
+++++++++++++++++++++++++++++++++++++++++++++++++++
  include/spl.h        |   1 +
  6 files changed, 406 insertions(+)

create mode 100644 common/spl/spl_atf.c
create mode 100644 include/atf_common.h

common/spl/Kconfig +6 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
        means of transmitting U-Boot over a serial line for using in
 SPL,
        with a checksum to ensure correctness.
  +config SPL_ATF_SUPPORT
+    bool "Support ARM trust firmware"
+    depends on SPL
+    help
+      ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+      load by SPL(consider as BL2 in ATF).
+      More detail at:

https://github.com/ARM-software/arm-trusted-firmware

common/spl/Kconfig +7 -0
+
+config SPL_ATF_TEXT_BASE
+    depends on SPL_ATF_SUPPORT
+    hex "ATF TEXT BASE addr"
+    help
+      This is the base address in memory for ATF text and entry point.
+
  config TPL_ENV_SUPPORT
      bool "Support an environment"
      depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
  obj-$(CONFIG_SPL_UBI) += spl_ubi.o
  obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
  obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
  obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +3 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
            gd->malloc_ptr / 1024);
  #endif
  +#ifdef CONFIG_SPL_ATF_SUPPORT
+    bl31_entry();
+#endif
+
      debug("loaded - jumping to U-Boot...");
      spl_board_prepare_for_boot();
      jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +0 -0
new file mode 100644
index 0000000..cf23b7a
@@ -0,0 +1,91 @@
Quoted 8 lines

common/spl/spl_atf.c line 7

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
+/*
+ * Copyright (C) 2016 Rockchip Electronic Co.,Ltd
+ * Written by Kever Yang <[email protected]>
+ *
+ * origin from arm-trust-firmware
+ * plat/arm/common/arm_bl2_setup.c
+ * SPDX-License-Identifier:     GPL-2.0+

this is not based on gpl file that's why license should be different.

Quoted 3 lines

Sorry, I do not get what your mean, I'm not good at license policy, ARM ATF use its own license, what should I do for license when I use code from ATF?

I am not that guy too. But if you took parts of code which is not GPL you can't label it as a GPL.

Quoted 60 lines
Quoted 51 lines
Quoted 51 lines
common/spl/spl_atf.c new file +12 -0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <spl.h>
+#include <atf_common.h>
+
+static struct bl2_to_bl31_params_mem_t bl31_params_mem;
+static struct bl31_params_t *bl2_to_bl31_params;
+
+/*******************************************************************************
 
+ * This function assigns a pointer to the memory that the platform

has kept
+ * aside to pass platform specific and trusted firmware related information
+ * to BL31. This memory is allocated by allocating memory to
+ * bl2_to_bl31_params_mem_t structure which is a superset of all the
+ * structure whose information is passed to BL31
+ * NOTE: This function should be called only once and should be done
+ * before generating params to BL31
+ ******************************************************************************/

common/spl/spl_atf.c new file +9 -0
+struct bl31_params_t *bl2_plat_get_bl31_params(void)
+{
+    struct entry_point_info_t *bl33_ep_info;
+
+    /*
+     * Initialise the memory for all the arguments that needs to
+     * be passed to BL31
+     */
+    memset(&bl31_params_mem, 0, sizeof(struct

bl2_to_bl31_params_mem_t));

common/spl/spl_atf.c new file +7 -0
+
+    /* Assign memory for TF related information */
+    bl2_to_bl31_params = &bl31_params_mem.bl31_params;
+    SET_PARAM_HEAD(bl2_to_bl31_params, PARAM_BL31, VERSION_1, 0);
+
+    /* Fill BL31 related information */
+    SET_PARAM_HEAD(bl2_to_bl31_params->bl31_image_info,

PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +6 -0
+               VERSION_1, 0);
+
+    /* Fill BL32 related information if it exists */
+#ifdef BL32_BASE
+    bl2_to_bl31_params->bl32_ep_info = &bl31_params_mem.bl32_ep_info;
+    SET_PARAM_HEAD(bl2_to_bl31_params->bl32_ep_info, PARAM_EP,
Quoted 8 lines

common/spl/spl_atf.c

Quoted 7 lines
common/spl/spl_atf.c new file +2 -0
+               VERSION_1, 0);
+    bl2_to_bl31_params->bl32_image_info =

&bl31_params_mem.bl32_image_info;
+ SET_PARAM_HEAD(bl2_to_bl31_params->bl32_image_info,
PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +2 -0
+               VERSION_1, 0);
+#endif /* BL32_BASE */

Is this used?

Quoted 2 lines

Not use for me now, but it may useful later, because we need to fill info about bl32 if there is.

I think that make sense to look at fit format and try to integrate all these stuff together.

Quoted 97 lines
Quoted 13 lines
Quoted 13 lines
common/spl/spl_atf.c new file +12 -0
+
+    /* Fill BL33 related information */
+    bl2_to_bl31_params->bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+    bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+    SET_PARAM_HEAD(bl33_ep_info, PARAM_EP, VERSION_1, EP_NON_SECURE);
+
+    /* BL33 expects to receive the primary CPU MPID (through x0) */
+    bl33_ep_info->args.arg0 = 0xffff & read_mpidr();
+    bl33_ep_info->pc = CONFIG_SYS_TEXT_BASE;
+    bl33_ep_info->spsr = SPSR_64(MODE_EL2, MODE_SP_ELX,
+                     DISABLE_ALL_EXECPTIONS);
+
Quoted 8 lines

common/spl/spl_atf.c line 54

Quoted 7 lines

+ bl2_to_bl31_params->bl33_image_info =
&bl31_params_mem.bl33_image_info;
+ SET_PARAM_HEAD(bl2_to_bl31_params->bl33_image_info,
PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +3 -0
+               VERSION_1, 0);
+
+

double lines.

Will fix in next patch, confuse why checkpatch did not find this.

Quoted 65 lines
Quoted 65 lines
common/spl/spl_atf.c new file +12 -0
+    return bl2_to_bl31_params;
+}
+
+void raw_write_daif(uint32_t daif)
+{
+    __asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory");
+}
+
+void bl31_entry(void)
+{
+    struct bl31_params_t *bl31_params;
+    void (*entry)(struct bl31_params_t *params, void *plat_params) =

NULL;

common/spl/spl_atf.c new file +9 -0
+
+    bl31_params = bl2_plat_get_bl31_params();
+    entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
+
+    raw_write_daif(SPSR_EXCEPTION_MASK);
+    dcache_disable();
+
+    entry(bl31_params, NULL);
+}
include/atf_common.h new file +2 -0
new file mode 100644
index 0000000..8351302
@@ -0,0 +1,295 @@
+/*
+ * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights

reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * Neither the name of ARM nor the names of its contributors may be used
+ * to endorse or promote products derived from this software without specific
+ * prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS

Quoted 8 lines
Quoted 7 lines

+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.

This should be probably in SPDX format.

Quoted 3 lines

Like previous reply, I don't know how to deal with this license, do you mean I can use
SPDX license without any information about the origin License?

Tom, Simon: Please correct me if I am wrong.
There are 2 things here.

0. Identify license and especially this part. The rest looks like BSD license

* Neither the name of ARM nor the names of its contributors may be used
* to endorse or promote products derived from this software without specific
* prior written permission.

1. If this ARM license is compatible with u-boot Licensing model
2. We have Licenses folder where License can go but not sure if only SPDX licenses can got there and if ARM published this license there to have official SPDX tag. Please look at Licenses/README

Thanks,
Michal

#10 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

+Tom for license comment

Hi,

Quoted 141 lines · Michal Simek

Hi,

Quoted 18 lines · Kever Yang

Hi Michal,

Thanks for your comments.

Quoted 6 lines · Michal Simek
Quoted 4 lines · Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary

SPL replaces BL2 in ATF

OK, will follow your comment in next patch.

Quoted 5 lines
Quoted 5 lines

like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

Quoted 6 lines
Quoted 4 lines

But the main question for this is how do load that images and in which format. It means I would think that you will introduce fit format which contain BL33(U-Boot), BL32(secure os) and BL31(ATF) and SPL will be able to load all of them.

Yes, I use FIT format to contain BL33 and BL32 and SPL load all of them.

Do you have some logs? I didn't check the latest code but IIRC it was possible to handle one image and dt not several images which has to be supported. There is also loadables section in fit which can help with this.

Quoted 8 lines
Quoted 4 lines

If you look at zynqmp I did a small trick where I consider case that with ATF it is OS boot where kernel is ATF and dtb is full u-boot to get it boot.

This is a good idea, and it look fine for support ATF in SPL in local source code,
but it will be better if we have an official support for ATF, right?

Definitely having support just for ATF is much better solution than what I use in ZynqMP.

Quoted 9 lines
Quoted 6 lines

If you adopt fit format then I expect SPL will be able to remember which part is where and based on that fill structure for ATF.
Then SPL_ATF_TEXT_BASE address is not needed because it will be read from fit format.

Yes, you are right, SPL_ATF_TEXT_BASE is not a must, we gen get it from fit.

ok.

Quoted 85 lines
Quoted 84 lines
Quoted 76 lines

---

  common/spl/Kconfig   |  14 +++
  common/spl/Makefile  |   1 +
  common/spl/spl.c     |   4 +
  common/spl/spl_atf.c |  91 ++++++++++++++++
  include/atf_common.h | 295
+++++++++++++++++++++++++++++++++++++++++++++++++++
  include/spl.h        |   1 +
  6 files changed, 406 insertions(+)

create mode 100644 common/spl/spl_atf.c
create mode 100644 include/atf_common.h

common/spl/Kconfig +6 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
        means of transmitting U-Boot over a serial line for using in
 SPL,
        with a checksum to ensure correctness.
  +config SPL_ATF_SUPPORT
+    bool "Support ARM trust firmware"
+    depends on SPL
+    help
+      ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+      load by SPL(consider as BL2 in ATF).
+      More detail at:

https://github.com/ARM-software/arm-trusted-firmware

common/spl/Kconfig +7 -0
+
+config SPL_ATF_TEXT_BASE
+    depends on SPL_ATF_SUPPORT
+    hex "ATF TEXT BASE addr"
+    help
+      This is the base address in memory for ATF text and entry point.
+
  config TPL_ENV_SUPPORT
      bool "Support an environment"
      depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
  obj-$(CONFIG_SPL_UBI) += spl_ubi.o
  obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
  obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
  obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +3 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
            gd->malloc_ptr / 1024);
  #endif
  +#ifdef CONFIG_SPL_ATF_SUPPORT
+    bl31_entry();
+#endif
+
      debug("loaded - jumping to U-Boot...");
      spl_board_prepare_for_boot();
      jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +0 -0
new file mode 100644
index 0000000..cf23b7a

common/spl/spl_atf.c line 7

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2016 Rockchip Electronic Co.,Ltd
+ * Written by Kever Yang <[email protected]>
+ *
+ * origin from arm-trust-firmware
+ * plat/arm/common/arm_bl2_setup.c
+ * SPDX-License-Identifier:     GPL-2.0+

this is not based on gpl file that's why license should be different.

Quoted 6 lines
Quoted 3 lines

Sorry, I do not get what your mean, I'm not good at license policy, ARM ATF use its own license, what should I do for license when I use code from ATF?

I am not that guy too. But if you took parts of code which is not GPL you can't label it as a GPL.

That's right. Perhaps rewrite and rename bl2_plat_get_bl31_params()? Then you can say that it is inspired by that file, and perhaps avoid the license problem.

The macros are horrible anyway.

Quoted 176 lines
Quoted 62 lines
Quoted 53 lines
Quoted 53 lines
common/spl/spl_atf.c new file +12 -0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <spl.h>
+#include <atf_common.h>
+
+static struct bl2_to_bl31_params_mem_t bl31_params_mem;
+static struct bl31_params_t *bl2_to_bl31_params;
+
+/*******************************************************************************
 
+ * This function assigns a pointer to the memory that the platform

has kept
+ * aside to pass platform specific and trusted firmware related information
+ * to BL31. This memory is allocated by allocating memory to
+ * bl2_to_bl31_params_mem_t structure which is a superset of all the
+ * structure whose information is passed to BL31
+ * NOTE: This function should be called only once and should be done
+ * before generating params to BL31
+ ******************************************************************************/

common/spl/spl_atf.c new file +9 -0
+struct bl31_params_t *bl2_plat_get_bl31_params(void)
+{
+    struct entry_point_info_t *bl33_ep_info;
+
+    /*
+     * Initialise the memory for all the arguments that needs to
+     * be passed to BL31
+     */
+    memset(&bl31_params_mem, 0, sizeof(struct

bl2_to_bl31_params_mem_t));

common/spl/spl_atf.c new file +7 -0
+
+    /* Assign memory for TF related information */
+    bl2_to_bl31_params = &bl31_params_mem.bl31_params;
+    SET_PARAM_HEAD(bl2_to_bl31_params, PARAM_BL31, VERSION_1, 0);
+
+    /* Fill BL31 related information */
+    SET_PARAM_HEAD(bl2_to_bl31_params->bl31_image_info,

PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +6 -0
+               VERSION_1, 0);
+
+    /* Fill BL32 related information if it exists */
+#ifdef BL32_BASE
+    bl2_to_bl31_params->bl32_ep_info = &bl31_params_mem.bl32_ep_info;
+    SET_PARAM_HEAD(bl2_to_bl31_params->bl32_ep_info, PARAM_EP,
Quoted 8 lines

common/spl/spl_atf.c

Quoted 7 lines
common/spl/spl_atf.c new file +2 -0
+               VERSION_1, 0);
+    bl2_to_bl31_params->bl32_image_info =

&bl31_params_mem.bl32_image_info;
+ SET_PARAM_HEAD(bl2_to_bl31_params->bl32_image_info,
PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +2 -0
+               VERSION_1, 0);
+#endif /* BL32_BASE */

Is this used?

Quoted 2 lines

Not use for me now, but it may useful later, because we need to fill info about bl32 if there is.

I think that make sense to look at fit format and try to integrate all these stuff together.

Quoted 97 lines
Quoted 13 lines
Quoted 13 lines
common/spl/spl_atf.c new file +12 -0
+
+    /* Fill BL33 related information */
+    bl2_to_bl31_params->bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+    bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+    SET_PARAM_HEAD(bl33_ep_info, PARAM_EP, VERSION_1, EP_NON_SECURE);
+
+    /* BL33 expects to receive the primary CPU MPID (through x0) */
+    bl33_ep_info->args.arg0 = 0xffff & read_mpidr();
+    bl33_ep_info->pc = CONFIG_SYS_TEXT_BASE;
+    bl33_ep_info->spsr = SPSR_64(MODE_EL2, MODE_SP_ELX,
+                     DISABLE_ALL_EXECPTIONS);
+
Quoted 8 lines

common/spl/spl_atf.c line 54

Quoted 7 lines

+ bl2_to_bl31_params->bl33_image_info =
&bl31_params_mem.bl33_image_info;
+ SET_PARAM_HEAD(bl2_to_bl31_params->bl33_image_info,
PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +3 -0
+               VERSION_1, 0);
+
+

double lines.

Will fix in next patch, confuse why checkpatch did not find this.

Quoted 65 lines
Quoted 65 lines
common/spl/spl_atf.c new file +12 -0
+    return bl2_to_bl31_params;
+}
+
+void raw_write_daif(uint32_t daif)
+{
+    __asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory");
+}
+
+void bl31_entry(void)
+{
+    struct bl31_params_t *bl31_params;
+    void (*entry)(struct bl31_params_t *params, void *plat_params) =

NULL;

common/spl/spl_atf.c new file +9 -0
+
+    bl31_params = bl2_plat_get_bl31_params();
+    entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
+
+    raw_write_daif(SPSR_EXCEPTION_MASK);
+    dcache_disable();
+
+    entry(bl31_params, NULL);
+}
include/atf_common.h new file +2 -0
new file mode 100644
index 0000000..8351302
@@ -0,0 +1,295 @@
+/*
+ * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights

reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * Neither the name of ARM nor the names of its contributors may be used
+ * to endorse or promote products derived from this software without specific
+ * prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS

Quoted 8 lines
Quoted 7 lines

+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.

This should be probably in SPDX format.

Quoted 3 lines

Like previous reply, I don't know how to deal with this license, do you mean I can use
SPDX license without any information about the origin License?

Tom, Simon: Please correct me if I am wrong.
There are 2 things here.

0. Identify license and especially this part. The rest looks like BSD license

Quoted 8 lines

* Neither the name of ARM nor the names of its contributors may be used
* to endorse or promote products derived from this software without specific
* prior written permission.

1. If this ARM license is compatible with u-boot Licensing model
2. We have Licenses folder where License can go but not sure if only SPDX licenses can got there and if ARM published this license there to have official SPDX tag. Please look at Licenses/README

Perhaps we need to get this licence registered with SPDX?

https://spdx.org/spdx-license-list/request-new-license

But it would be better to get that piece removed somehow. It makes a non-standard license. Failing that, add a new licence:

1. Add to Licenses directory with a suitable identifier (see for example commit 0f4d2f8e)
2. Submit a new license registration request

Regards,
Simon

#11 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

Quoted 346 lines · Simon Glass

+Tom for license comment

Hi,

Quoted 141 lines · Michal Simek

Hi,

Quoted 18 lines · Kever Yang

Hi Michal,

Thanks for your comments.

Quoted 6 lines · Michal Simek
Quoted 4 lines · Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary

SPL replaces BL2 in ATF

OK, will follow your comment in next patch.

Quoted 5 lines
Quoted 5 lines

like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

Quoted 6 lines
Quoted 4 lines

But the main question for this is how do load that images and in which format. It means I would think that you will introduce fit format which contain BL33(U-Boot), BL32(secure os) and BL31(ATF) and SPL will be able to load all of them.

Yes, I use FIT format to contain BL33 and BL32 and SPL load all of them.

Do you have some logs? I didn't check the latest code but IIRC it was possible to handle one image and dt not several images which has to be supported. There is also loadables section in fit which can help with this.

Quoted 8 lines
Quoted 4 lines

If you look at zynqmp I did a small trick where I consider case that with ATF it is OS boot where kernel is ATF and dtb is full u-boot to get it boot.

This is a good idea, and it look fine for support ATF in SPL in local source code,
but it will be better if we have an official support for ATF, right?

Definitely having support just for ATF is much better solution than what I use in ZynqMP.

Quoted 9 lines
Quoted 6 lines

If you adopt fit format then I expect SPL will be able to remember which part is where and based on that fill structure for ATF.
Then SPL_ATF_TEXT_BASE address is not needed because it will be read from fit format.

Yes, you are right, SPL_ATF_TEXT_BASE is not a must, we gen get it from fit.

ok.

Quoted 85 lines
Quoted 84 lines
Quoted 76 lines

---

  common/spl/Kconfig   |  14 +++
  common/spl/Makefile  |   1 +
  common/spl/spl.c     |   4 +
  common/spl/spl_atf.c |  91 ++++++++++++++++
  include/atf_common.h | 295
+++++++++++++++++++++++++++++++++++++++++++++++++++
  include/spl.h        |   1 +
  6 files changed, 406 insertions(+)

create mode 100644 common/spl/spl_atf.c
create mode 100644 include/atf_common.h

common/spl/Kconfig +6 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
        means of transmitting U-Boot over a serial line for using in
 SPL,
        with a checksum to ensure correctness.
  +config SPL_ATF_SUPPORT
+    bool "Support ARM trust firmware"
+    depends on SPL
+    help
+      ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+      load by SPL(consider as BL2 in ATF).
+      More detail at:

https://github.com/ARM-software/arm-trusted-firmware

common/spl/Kconfig +7 -0
+
+config SPL_ATF_TEXT_BASE
+    depends on SPL_ATF_SUPPORT
+    hex "ATF TEXT BASE addr"
+    help
+      This is the base address in memory for ATF text and entry point.
+
  config TPL_ENV_SUPPORT
      bool "Support an environment"
      depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
  obj-$(CONFIG_SPL_UBI) += spl_ubi.o
  obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
  obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
  obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +3 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
            gd->malloc_ptr / 1024);
  #endif
  +#ifdef CONFIG_SPL_ATF_SUPPORT
+    bl31_entry();
+#endif
+
      debug("loaded - jumping to U-Boot...");
      spl_board_prepare_for_boot();
      jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +0 -0
new file mode 100644
index 0000000..cf23b7a

common/spl/spl_atf.c line 7

Quoted 7 lines
common/spl/spl_atf.c new file +7 -0
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2016 Rockchip Electronic Co.,Ltd
+ * Written by Kever Yang <[email protected]>
+ *
+ * origin from arm-trust-firmware
+ * plat/arm/common/arm_bl2_setup.c
+ * SPDX-License-Identifier:     GPL-2.0+

this is not based on gpl file that's why license should be different.

Quoted 6 lines
Quoted 3 lines

Sorry, I do not get what your mean, I'm not good at license policy, ARM ATF use its own license, what should I do for license when I use code from ATF?

I am not that guy too. But if you took parts of code which is not GPL you can't label it as a GPL.

That's right. Perhaps rewrite and rename bl2_plat_get_bl31_params()? Then you can say that it is inspired by that file, and perhaps avoid the license problem.

The macros are horrible anyway.

Quoted 176 lines
Quoted 62 lines
Quoted 53 lines
Quoted 53 lines
common/spl/spl_atf.c new file +12 -0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <spl.h>
+#include <atf_common.h>
+
+static struct bl2_to_bl31_params_mem_t bl31_params_mem;
+static struct bl31_params_t *bl2_to_bl31_params;
+
+/*******************************************************************************
 
+ * This function assigns a pointer to the memory that the platform

has kept
+ * aside to pass platform specific and trusted firmware related information
+ * to BL31. This memory is allocated by allocating memory to
+ * bl2_to_bl31_params_mem_t structure which is a superset of all the
+ * structure whose information is passed to BL31
+ * NOTE: This function should be called only once and should be done
+ * before generating params to BL31
+ ******************************************************************************/

common/spl/spl_atf.c new file +9 -0
+struct bl31_params_t *bl2_plat_get_bl31_params(void)
+{
+    struct entry_point_info_t *bl33_ep_info;
+
+    /*
+     * Initialise the memory for all the arguments that needs to
+     * be passed to BL31
+     */
+    memset(&bl31_params_mem, 0, sizeof(struct

bl2_to_bl31_params_mem_t));

common/spl/spl_atf.c new file +7 -0
+
+    /* Assign memory for TF related information */
+    bl2_to_bl31_params = &bl31_params_mem.bl31_params;
+    SET_PARAM_HEAD(bl2_to_bl31_params, PARAM_BL31, VERSION_1, 0);
+
+    /* Fill BL31 related information */
+    SET_PARAM_HEAD(bl2_to_bl31_params->bl31_image_info,

PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +6 -0
+               VERSION_1, 0);
+
+    /* Fill BL32 related information if it exists */
+#ifdef BL32_BASE
+    bl2_to_bl31_params->bl32_ep_info = &bl31_params_mem.bl32_ep_info;
+    SET_PARAM_HEAD(bl2_to_bl31_params->bl32_ep_info, PARAM_EP,
Quoted 8 lines

common/spl/spl_atf.c

Quoted 7 lines
common/spl/spl_atf.c new file +2 -0
+               VERSION_1, 0);
+    bl2_to_bl31_params->bl32_image_info =

&bl31_params_mem.bl32_image_info;
+ SET_PARAM_HEAD(bl2_to_bl31_params->bl32_image_info,
PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +2 -0
+               VERSION_1, 0);
+#endif /* BL32_BASE */

Is this used?

Quoted 2 lines

Not use for me now, but it may useful later, because we need to fill info about bl32 if there is.

I think that make sense to look at fit format and try to integrate all these stuff together.

Quoted 97 lines
Quoted 13 lines
Quoted 13 lines
common/spl/spl_atf.c new file +12 -0
+
+    /* Fill BL33 related information */
+    bl2_to_bl31_params->bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+    bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+    SET_PARAM_HEAD(bl33_ep_info, PARAM_EP, VERSION_1, EP_NON_SECURE);
+
+    /* BL33 expects to receive the primary CPU MPID (through x0) */
+    bl33_ep_info->args.arg0 = 0xffff & read_mpidr();
+    bl33_ep_info->pc = CONFIG_SYS_TEXT_BASE;
+    bl33_ep_info->spsr = SPSR_64(MODE_EL2, MODE_SP_ELX,
+                     DISABLE_ALL_EXECPTIONS);
+
Quoted 8 lines

common/spl/spl_atf.c line 54

Quoted 7 lines

+ bl2_to_bl31_params->bl33_image_info =
&bl31_params_mem.bl33_image_info;
+ SET_PARAM_HEAD(bl2_to_bl31_params->bl33_image_info,
PARAM_IMAGE_BINARY,

common/spl/spl_atf.c new file +3 -0
+               VERSION_1, 0);
+
+

double lines.

Will fix in next patch, confuse why checkpatch did not find this.

Quoted 65 lines
Quoted 65 lines
common/spl/spl_atf.c new file +12 -0
+    return bl2_to_bl31_params;
+}
+
+void raw_write_daif(uint32_t daif)
+{
+    __asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory");
+}
+
+void bl31_entry(void)
+{
+    struct bl31_params_t *bl31_params;
+    void (*entry)(struct bl31_params_t *params, void *plat_params) =

NULL;

common/spl/spl_atf.c new file +9 -0
+
+    bl31_params = bl2_plat_get_bl31_params();
+    entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
+
+    raw_write_daif(SPSR_EXCEPTION_MASK);
+    dcache_disable();
+
+    entry(bl31_params, NULL);
+}
include/atf_common.h new file +2 -0
new file mode 100644
index 0000000..8351302
@@ -0,0 +1,295 @@
+/*
+ * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights

reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * Neither the name of ARM nor the names of its contributors may be used
+ * to endorse or promote products derived from this software without specific
+ * prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS

Quoted 8 lines
Quoted 7 lines

+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.

This should be probably in SPDX format.

Quoted 3 lines

Like previous reply, I don't know how to deal with this license, do you mean I can use
SPDX license without any information about the origin License?

Tom, Simon: Please correct me if I am wrong.
There are 2 things here.

0. Identify license and especially this part. The rest looks like BSD license

Quoted 8 lines

* Neither the name of ARM nor the names of its contributors may be used
* to endorse or promote products derived from this software without specific
* prior written permission.

1. If this ARM license is compatible with u-boot Licensing model
2. We have Licenses folder where License can go but not sure if only SPDX licenses can got there and if ARM published this license there to have official SPDX tag. Please look at Licenses/README

Perhaps we need to get this licence registered with SPDX?

Quoted 8 lines

https://spdx.org/spdx-license-list/request-new-license

But it would be better to get that piece removed somehow. It makes a non-standard license. Failing that, add a new licence:

1. Add to Licenses directory with a suitable identifier (see for example commit 0f4d2f8e)
2. Submit a new license registration request

For both license questions, we must be correct. The copy of ATF that I just looked at has a 3 clause BSD license, with the 3rd clause worded perhaps oddly, but, that looks right for 3-clause BSD. This is compatible with U-Boot. If on the other hand ATF has some data structures/etc that non-ATF code really needs to know and it is not BSD-3-Clause, we should perhaps try and contact someone with the authority to re-license it, using include/android_image.h as an example of where U-Boot hosts, officially, this kind of information in a more permissible license. Thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170128/b4b9927a/attachment.sig>
author
#12 lore raw

RFC PATCH 3/3 spl: add support to booting with ATF

In reply to Kever Yang

Hi

Who is suppose to maintain source in "common/spl"?
I hope to get more comments before my first version patch without 'RFC' and with suggestion from Michal.

Thanks,
- Kever

Quoted 482 lines · Kever Yang

ATF(ARM Trust Firmware) is used by ARM arch64 SoCs, find more infomation about ATF at:

SPL is consider as BL2 in ATF, it needs to load other part of ATF binary like BL31, BL32, SCP-BL30, and BL33(U-Boot). And needs to prepare the parameter for BL31 which including entry and image information for all other images. Then the SPL handle PC to BL31 with the parameter, the BL31 will do the rest of work and at last get into U-Boot(BL33).

---

  common/spl/Kconfig   |  14 +++
  common/spl/Makefile  |   1 +
  common/spl/spl.c     |   4 +
  common/spl/spl_atf.c |  91 ++++++++++++++++
  include/atf_common.h | 295 +++++++++++++++++++++++++++++++++++++++++++++++++++
  include/spl.h        |   1 +
  6 files changed, 406 insertions(+)

create mode 100644 common/spl/spl_atf.c
create mode 100644 include/atf_common.h

common/spl/Kconfig +14 -0
index cba51f5..1bb4360 100644
@@ -577,6 +577,20 @@ config SPL_YMODEM_SUPPORT
  	  means of transmitting U-Boot over a serial line for using in SPL,
  	  with a checksum to ensure correctness.
 
+config SPL_ATF_SUPPORT
+	bool "Support ARM trust firmware"
+	depends on SPL
+	help
+	  ATF(ARM Trust Firmware) is component for ARM arch64 which need to
+	  load by SPL(consider as BL2 in ATF).
+	  More detail at: https://github.com/ARM-software/arm-trusted-firmware
+
+config SPL_ATF_TEXT_BASE
+	depends on SPL_ATF_SUPPORT
+	hex "ATF TEXT BASE addr"
+	help
+	  This is the base address in memory for ATF text and entry point.
+
  config TPL_ENV_SUPPORT
  	bool "Support an environment"
  	depends on TPL
common/spl/Makefile +1 -0
index ed02635..620ae90 100644
@@ -20,6 +20,7 @@ endif
  obj-$(CONFIG_SPL_UBI) += spl_ubi.o
  obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
  obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
+obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
  obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
common/spl/spl.c +4 -0
index 1729034..7daf7bd 100644
@@ -390,6 +390,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
  	      gd->malloc_ptr / 1024);
  #endif
 
+#ifdef CONFIG_SPL_ATF_SUPPORT
+	bl31_entry();
+#endif
+
  	debug("loaded - jumping to U-Boot...");
  	spl_board_prepare_for_boot();
  	jump_to_image_no_args(&spl_image);
common/spl/spl_atf.c new file +91 -0
new file mode 100644
index 0000000..cf23b7a
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2016 Rockchip Electronic Co.,Ltd
+ * Written by Kever Yang <[email protected]>
+ *
+ * origin from arm-trust-firmware
+ * plat/arm/common/arm_bl2_setup.c
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <spl.h>
+#include <atf_common.h>
+
+static struct bl2_to_bl31_params_mem_t bl31_params_mem;
+static struct bl31_params_t *bl2_to_bl31_params;
+
+/*******************************************************************************
+ * This function assigns a pointer to the memory that the platform has kept
+ * aside to pass platform specific and trusted firmware related information
+ * to BL31. This memory is allocated by allocating memory to
+ * bl2_to_bl31_params_mem_t structure which is a superset of all the
+ * structure whose information is passed to BL31
+ * NOTE: This function should be called only once and should be done
+ * before generating params to BL31
+ ******************************************************************************/
+struct bl31_params_t *bl2_plat_get_bl31_params(void)
+{
+	struct entry_point_info_t *bl33_ep_info;
+
+	/*
+	 * Initialise the memory for all the arguments that needs to
+	 * be passed to BL31
+	 */
+	memset(&bl31_params_mem, 0, sizeof(struct bl2_to_bl31_params_mem_t));
+
+	/* Assign memory for TF related information */
+	bl2_to_bl31_params = &bl31_params_mem.bl31_params;
+	SET_PARAM_HEAD(bl2_to_bl31_params, PARAM_BL31, VERSION_1, 0);
+
+	/* Fill BL31 related information */
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl31_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+
+	/* Fill BL32 related information if it exists */
+#ifdef BL32_BASE
+	bl2_to_bl31_params->bl32_ep_info = &bl31_params_mem.bl32_ep_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl32_ep_info, PARAM_EP,
+		       VERSION_1, 0);
+	bl2_to_bl31_params->bl32_image_info = &bl31_params_mem.bl32_image_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl32_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+#endif /* BL32_BASE */
+
+	/* Fill BL33 related information */
+	bl2_to_bl31_params->bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+	bl33_ep_info = &bl31_params_mem.bl33_ep_info;
+	SET_PARAM_HEAD(bl33_ep_info, PARAM_EP, VERSION_1, EP_NON_SECURE);
+
+	/* BL33 expects to receive the primary CPU MPID (through x0) */
+	bl33_ep_info->args.arg0 = 0xffff & read_mpidr();
+	bl33_ep_info->pc = CONFIG_SYS_TEXT_BASE;
+	bl33_ep_info->spsr = SPSR_64(MODE_EL2, MODE_SP_ELX,
+				     DISABLE_ALL_EXECPTIONS);
+
+	bl2_to_bl31_params->bl33_image_info = &bl31_params_mem.bl33_image_info;
+	SET_PARAM_HEAD(bl2_to_bl31_params->bl33_image_info, PARAM_IMAGE_BINARY,
+		       VERSION_1, 0);
+
+
+	return bl2_to_bl31_params;
+}
+
+void raw_write_daif(uint32_t daif)
+{
+	__asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory");
+}
+
+void bl31_entry(void)
+{
+	struct bl31_params_t *bl31_params;
+	void (*entry)(struct bl31_params_t *params, void *plat_params) = NULL;
+
+	bl31_params = bl2_plat_get_bl31_params();
+	entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
+
+	raw_write_daif(SPSR_EXCEPTION_MASK);
+	dcache_disable();
+
+	entry(bl31_params, NULL);
+}
include/atf_common.h new file +295 -0
new file mode 100644
index 0000000..8351302
@@ -0,0 +1,295 @@
+/*
+ * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ * Neither the name of ARM nor the names of its contributors may be used
+ * to endorse or promote products derived from this software without specific
+ * prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef __BL_COMMON_H__
+#define __BL_COMMON_H__
+
+#define SECURE		0x0
+#define NON_SECURE	0x1
+#define sec_state_is_valid(s) (((s) == SECURE) || ((s) == NON_SECURE))
+
+#define UP	1
+#define DOWN	0
+
+/*******************************************************************************
+ * Constants to identify the location of a memory region in a given memory
+ * layout.
+******************************************************************************/
+#define TOP	0x1
+#define BOTTOM	!TOP
+
+/*******************************************************************************
+ * Constants that allow assembler code to access members of and the
+ * 'entry_point_info' structure at their correct offsets.
+ ******************************************************************************/
+#define ENTRY_POINT_INFO_PC_OFFSET	0x08
+#define ENTRY_POINT_INFO_ARGS_OFFSET	0x18
+
+/* The following are used to set/get image attributes. */
+#define PARAM_EP_SECURITY_MASK		(0x1)
+
+#define GET_SECURITY_STATE(x) (x & PARAM_EP_SECURITY_MASK)
+#define SET_SECURITY_STATE(x, security) \
+			((x) = ((x) & ~PARAM_EP_SECURITY_MASK) | (security))
+
+
+/*
+ * The following are used for image state attributes.
+ * Image can only be in one of the following state.
+ */
+#define IMAGE_STATE_RESET			0
+#define IMAGE_STATE_COPIED			1
+#define IMAGE_STATE_COPYING			2
+#define IMAGE_STATE_AUTHENTICATED		3
+#define IMAGE_STATE_EXECUTED			4
+#define IMAGE_STATE_INTERRUPTED			5
+
+#define EP_SECURE	0x0
+#define EP_NON_SECURE	0x1
+
+#define EP_EE_MASK	0x2
+#define EP_EE_LITTLE	0x0
+#define EP_EE_BIG	0x2
+#define EP_GET_EE(x) (x & EP_EE_MASK)
+#define EP_SET_EE(x, ee) ((x) = ((x) & ~EP_EE_MASK) | (ee))
+
+#define EP_ST_MASK	0x4
+#define EP_ST_DISABLE	0x0
+#define EP_ST_ENABLE	0x4
+#define EP_GET_ST(x) (x & EP_ST_MASK)
+#define EP_SET_ST(x, ee) ((x) = ((x) & ~EP_ST_MASK) | (ee))
+
+#define EP_EXE_MASK	0x8
+#define NON_EXECUTABLE	0x0
+#define EXECUTABLE	0x8
+#define EP_GET_EXE(x) (x & EP_EXE_MASK)
+#define EP_SET_EXE(x, ee) ((x) = ((x) & ~EP_EXE_MASK) | (ee))
+
+#define PARAM_EP		0x01
+#define PARAM_IMAGE_BINARY	0x02
+#define PARAM_BL31		0x03
+
+#define VERSION_1	0x01
+
+#define INVALID_IMAGE_ID		(0xFFFFFFFF)
+
+#define SET_PARAM_HEAD(_p, _type, _ver, _attr) do { \
+	(_p)->h.type = (uint8_t)(_type); \
+	(_p)->h.version = (uint8_t)(_ver); \
+	(_p)->h.size = (uint16_t)sizeof(*_p); \
+	(_p)->h.attr = (uint32_t)(_attr) ; \
+	} while (0)
+
+/* Following is used for populating structure members statically. */
+#define SET_STATIC_PARAM_HEAD(_p, _type, _ver, _p_type, _attr)	\
+	._p.h.type = (uint8_t)(_type), \
+	._p.h.version = (uint8_t)(_ver), \
+	._p.h.size = (uint16_t)sizeof(_p_type), \
+	._p.h.attr = (uint32_t)(_attr)
+
+#define MODE_RW_SHIFT	0x4
+#define MODE_RW_MASK	0x1
+#define MODE_RW_64	0x0
+#define MODE_RW_32	0x1
+
+#define MODE_EL_SHIFT	0x2
+#define MODE_EL_MASK	0x3
+#define MODE_EL3	0x3
+#define MODE_EL2	0x2
+#define MODE_EL1	0x1
+#define MODE_EL0	0x0
+
+#define MODE_SP_SHIFT	0x0
+#define MODE_SP_MASK	0x1
+#define MODE_SP_EL0	0x0
+#define MODE_SP_ELX	0x1
+
+#define SPSR_DAIF_SHIFT	6
+#define SPSR_DAIF_MASK	0x0f
+
+#define DAIF_FIQ_BIT (1<<0)
+#define DAIF_IRQ_BIT (1<<0)
+#define DAIF_ABT_BIT (1<<0)
+#define DAIF_DBG_BIT (1<<0)
+#define DISABLE_ALL_EXECPTIONS	\
+	(DAIF_FIQ_BIT | DAIF_IRQ_BIT | DAIF_ABT_BIT | DAIF_DBG_BIT)
+
+#define SPSR_64(el, sp, daif)		\
+	(MODE_RW_64 << MODE_RW_SHIFT |	\
+	 ((el) & MODE_EL_MASK) << MODE_EL_SHIFT |	\
+	 ((sp) & MODE_SP_MASK) << MODE_SP_SHIFT |	\
+	 ((daif) & SPSR_DAIF_MASK) << SPSR_DAIF_SHIFT)
+
+/*******************************************************************************
+ * Constants to indicate type of exception to the common exception handler.
+ ******************************************************************************/
+#define SYNC_EXCEPTION_SP_EL0		0x0
+#define IRQ_SP_EL0			0x1
+#define FIQ_SP_EL0			0x2
+#define SERROR_SP_EL0			0x3
+#define SYNC_EXCEPTION_SP_ELX		0x4
+#define IRQ_SP_ELX			0x5
+#define FIQ_SP_ELX			0x6
+#define SERROR_SP_ELX			0x7
+#define SYNC_EXCEPTION_AARCH64		0x8
+#define IRQ_AARCH64			0x9
+#define FIQ_AARCH64			0xa
+#define SERROR_AARCH64			0xb
+#define SYNC_EXCEPTION_AARCH32		0xc
+#define IRQ_AARCH32			0xd
+#define FIQ_AARCH32			0xe
+#define SERROR_AARCH32			0xf
+
+#define SPSR_USE_L           0
+#define SPSR_USE_H           1
+#define SPSR_L_H_MASK        1
+#define SPSR_M_SHIFT         4
+#define SPSR_ERET_32         (1 << SPSR_M_SHIFT)
+#define SPSR_ERET_64         (0 << SPSR_M_SHIFT)
+#define SPSR_FIQ             (1 << 6)
+#define SPSR_IRQ             (1 << 7)
+#define SPSR_SERROR          (1 << 8)
+#define SPSR_DEBUG           (1 << 9)
+#define SPSR_EXCEPTION_MASK  (SPSR_FIQ | SPSR_IRQ | SPSR_SERROR | SPSR_DEBUG)
+
+#ifndef __ASSEMBLY__
+
+/*******************************************************************************
+ * Structure used for telling the next BL how much of a particular type of
+ * memory is available for its use and how much is already used.
+ ******************************************************************************/
+struct aapcs64_params_t {
+	unsigned long arg0;
+	unsigned long arg1;
+	unsigned long arg2;
+	unsigned long arg3;
+	unsigned long arg4;
+	unsigned long arg5;
+	unsigned long arg6;
+	unsigned long arg7;
+};
+
+/***************************************************************************
+ * This structure provides version information and the size of the
+ * structure, attributes for the structure it represents
+ ***************************************************************************/
+struct param_header_t {
+	uint8_t type;		/* type of the structure */
+	uint8_t version;    /* version of this structure */
+	uint16_t size;      /* size of this structure in bytes */
+	uint32_t attr;      /* attributes: unused bits SBZ */
+};
+
+/*****************************************************************************
+ * This structure represents the superset of information needed while
+ * switching exception levels. The only two mechanisms to do so are
+ * ERET & SMC. Security state is indicated using bit zero of header
+ * attribute
+ * NOTE: BL1 expects entrypoint followed by spsr at an offset from the start
+ * of this structure defined by the macro `ENTRY_POINT_INFO_PC_OFFSET` while
+ * processing SMC to jump to BL31.
+ *****************************************************************************/
+struct entry_point_info_t {
+	struct param_header_t h;
+	uintptr_t pc;
+	uint32_t spsr;
+	struct aapcs64_params_t args;
+};
+
+/*****************************************************************************
+ * Image info binary provides information from the image loader that
+ * can be used by the firmware to manage available trusted RAM.
+ * More advanced firmware image formats can provide additional
+ * information that enables optimization or greater flexibility in the
+ * common firmware code
+ *****************************************************************************/
+struct atf_image_info_t {
+	struct param_header_t h;
+	uintptr_t image_base;   /* physical address of base of image */
+	uint32_t image_size;    /* bytes read from image file */
+};
+
+/*****************************************************************************
+ * The image descriptor struct definition.
+ *****************************************************************************/
+struct image_desc_t {
+	/* Contains unique image id for the image. */
+	unsigned int image_id;
+	/*
+	 * This member contains Image state information.
+	 * Refer IMAGE_STATE_XXX defined above.
+	 */
+	unsigned int state;
+	uint32_t copied_size;	/* image size copied in blocks */
+	struct atf_image_info_t atf_image_info;
+	struct entry_point_info_t ep_info;
+};
+
+/*******************************************************************************
+ * This structure represents the superset of information that can be passed to
+ * BL31 e.g. while passing control to it from BL2. The BL32 parameters will be
+ * populated only if BL2 detects its presence. A pointer to a structure of this
+ * type should be passed in X0 to BL31's cold boot entrypoint.
+ *
+ * Use of this structure and the X0 parameter is not mandatory: the BL31
+ * platform code can use other mechanisms to provide the necessary information
+ * about BL32 and BL33 to the common and SPD code.
+ *
+ * BL31 image information is mandatory if this structure is used. If either of
+ * the optional BL32 and BL33 image information is not provided, this is
+ * indicated by the respective image_info pointers being zero.
+ ******************************************************************************/
+struct bl31_params_t {
+	struct param_header_t h;
+	struct atf_image_info_t *bl31_image_info;
+	struct entry_point_info_t *bl32_ep_info;
+	struct atf_image_info_t *bl32_image_info;
+	struct entry_point_info_t *bl33_ep_info;
+	struct atf_image_info_t *bl33_image_info;
+};
+
+/*******************************************************************************
+ * This structure represents the superset of information that is passed to
+ * BL31, e.g. while passing control to it from BL2, bl31_params
+ * and other platform specific params
+ ******************************************************************************/
+struct bl2_to_bl31_params_mem_t {
+	struct bl31_params_t bl31_params;
+	struct atf_image_info_t bl31_image_info;
+	struct atf_image_info_t bl32_image_info;
+	struct atf_image_info_t bl33_image_info;
+	struct entry_point_info_t bl33_ep_info;
+	struct entry_point_info_t bl32_ep_info;
+	struct entry_point_info_t bl31_ep_info;
+};
+
+#endif /*__ASSEMBLY__*/
+
+#endif /* __BL_COMMON_H__ */
include/spl.h +1 -0
index 918c464..ab5eaca 100644
@@ -251,4 +251,5 @@ int spl_dfu_cmd(int usbctrl, char *dfu_alt_info, char *interface, char *devstr);
  int spl_mmc_load_image(struct spl_image_info *spl_image,
  		       struct spl_boot_device *bootdev);
 
+void bl31_entry(void);
  #endif
#13 lore raw

cover arm64: rk3399: enable SPL with ATF support

In reply to Kever Yang

Hi,

separation of rk3399 SPL from ATF SPL would be worth.

Thanks,
Michal

Quoted 51 lines · Kever Yang

RK3399 SPL is not enable bacause of the lack of ATF support in SPL, after port some source code from ATF, I manage to enable the support for ATF in SPL. This patch set depends on some patch for SPL support multi binary in FIT which is from Andre.

The patch of sdram controller is still not clean, because I want to get comments from upstream.I port this driver from coreboot, and I don't want to make much change on it. The rk3399 dram controller has many registers to config, I want to make them directly used by driver instead of parse and copy them one by one from dts.

Another part of comment is welcome for the implementation of spl_atf.c and the entry in spl.c

Kever Yang (3):
arm64: rk3399: add SPL support
arm64: rk3399: add ddr controller driver
spl: add support to booting with ATF

 arch/arm/Kconfig                                   |    1 +
 arch/arm/dts/rk3399.dtsi                           |   24 +
 arch/arm/include/asm/arch-rockchip/grf_rk3399.h    |  118 ++
 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h  |  188 +++
 arch/arm/mach-rockchip/Kconfig                     |    2 +
 arch/arm/mach-rockchip/Makefile                    |    1 +
 arch/arm/mach-rockchip/rk3399-board-spl.c          |  157 ++
 arch/arm/mach-rockchip/rk3399/Makefile             |    1 +
 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc | 1565 ++++++++++++++++++++
 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c       | 1121 ++++++++++++++
 common/spl/Kconfig                                 |   14 +
 common/spl/Makefile                                |    1 +
 common/spl/spl.c                                   |    4 +
 common/spl/spl_atf.c                               |   91 ++
 configs/evb-rk3399_defconfig                       |   17 +
 drivers/clk/rockchip/clk_rk3399.c                  |   42 +-
 drivers/pinctrl/rockchip/pinctrl_rk3399.c          |  106 --
 include/atf_common.h                               |  295 ++++
 include/configs/rk3399_common.h                    |   11 +
 include/dt-bindings/clock/rk3399-cru.h             |   16 +-
 include/spl.h                                      |    1 +
 21 files changed, 3663 insertions(+), 113 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h
 create mode 100644 arch/arm/mach-rockchip/rk3399-board-spl.c
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc
 create mode 100644 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c
 create mode 100644 common/spl/spl_atf.c
 create mode 100644 include/atf_common.h
author
#14 lore raw

cover arm64: rk3399: enable SPL with ATF support

Hi Michal,

Quoted 3 lines · Michal Simek

Hi,

separation of rk3399 SPL from ATF SPL would be worth.

Thanks for your comment, this may help my rk3399 SPL patches get accept faster.
I will do it after people get back from holiday and send me other comments.

Thanks,
- Kever

Quoted 58 lines

Thanks,
Michal

Quoted 50 lines · Kever Yang

RK3399 SPL is not enable bacause of the lack of ATF support in SPL, after port some source code from ATF, I manage to enable the support for ATF in SPL. This patch set depends on some patch for SPL support multi binary in FIT which is from Andre.

The patch of sdram controller is still not clean, because I want to get comments from upstream.I port this driver from coreboot, and I don't want to make much change on it. The rk3399 dram controller has many registers to config, I want to make them directly used by driver instead of parse and copy them one by one from dts.

Another part of comment is welcome for the implementation of spl_atf.c and the entry in spl.c

Kever Yang (3):
arm64: rk3399: add SPL support
arm64: rk3399: add ddr controller driver
spl: add support to booting with ATF

  arch/arm/Kconfig                                   |    1 +
  arch/arm/dts/rk3399.dtsi                           |   24 +
  arch/arm/include/asm/arch-rockchip/grf_rk3399.h    |  118 ++
  arch/arm/include/asm/arch-rockchip/sdram_rk3399.h  |  188 +++
  arch/arm/mach-rockchip/Kconfig                     |    2 +
  arch/arm/mach-rockchip/Makefile                    |    1 +
  arch/arm/mach-rockchip/rk3399-board-spl.c          |  157 ++
  arch/arm/mach-rockchip/rk3399/Makefile             |    1 +
  arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc | 1565 ++++++++++++++++++++
  arch/arm/mach-rockchip/rk3399/sdram_rk3399.c       | 1121 ++++++++++++++
  common/spl/Kconfig                                 |   14 +
  common/spl/Makefile                                |    1 +
  common/spl/spl.c                                   |    4 +
  common/spl/spl_atf.c                               |   91 ++
  configs/evb-rk3399_defconfig                       |   17 +
  drivers/clk/rockchip/clk_rk3399.c                  |   42 +-
  drivers/pinctrl/rockchip/pinctrl_rk3399.c          |  106 --
  include/atf_common.h                               |  295 ++++
  include/configs/rk3399_common.h                    |   11 +
  include/dt-bindings/clock/rk3399-cru.h             |   16 +-
  include/spl.h                                      |    1 +
  21 files changed, 3663 insertions(+), 113 deletions(-)

create mode 100644 arch/arm/include/asm/arch-rockchip/sdram_rk3399.h create mode 100644 arch/arm/mach-rockchip/rk3399-board-spl.c create mode 100644 arch/arm/mach-rockchip/rk3399/sdram-lpddr3-4GB.inc create mode 100644 arch/arm/mach-rockchip/rk3399/sdram_rk3399.c create mode 100644 common/spl/spl_atf.c
create mode 100644 include/atf_common.h