Rk322x #1666

TonyXie06 · 2017-07-07T06:31:27Z

add plat-rockchip support into optee_os, the SoC is RK322X.

jenswi-linaro · 2017-07-07T07:16:22Z

core/arch/arm/plat-rockchip/cru.h

+#define CRU_SNDRST_VAL			0xeca8
+#define PLLS_SLOW_MODE			0x11030000
+
+#define CORE_SOFT_RESET(core)		(0x100010 << (core))


Please use SHIFT_U32() (from <util.h.>, like SHIFT_U32(0x100010, (core)) instead
Same for the other defines below.

jenswi-linaro · 2017-07-07T07:19:20Z

core/arch/arm/plat-rockchip/plat_init.S

+	read_actlr  r0
+	orr     r0, r0, #ACTLR_SMP
+	write_actlr r0
+


bx lr missing?

jenswi-linaro · 2017-07-07T07:20:48Z

core/arch/arm/plat-rockchip/platform.c

+
+#define SGRF_SOC_CON(n)		((n) * 4)
+#define DDR_SGRF_DDR_CON(n)	((n) * 4)
+#define DDR_RGN0_NS		((1 << 30) | (0 << 14))


Please use the BIT32() macro from <util.h> instead, like (BIT32(30) | BIT32(14))

Hi, there is (0 << 14), not (1 << 14).
I would like to set bit14 as 0. For obvious, so I use (0 << 14). Should I use "#define DDR_RGN0_NS BIT(30)" ?

jenswi-linaro · 2017-07-07T07:21:48Z

core/arch/arm/plat-rockchip/platform_config.h

+/* Make stacks aligned to data cache line length */
+#define STACK_ALIGNMENT		64
+
+#ifdef ARM64


Since this is a ARMv7 platform, ARM64 will never be defined so this isn't needed.

jenswi-linaro · 2017-07-07T07:22:55Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+#include <tee/entry_std.h>
+#include <tee/entry_fast.h>
+
+#define CORE_WFE_MSK(core)	(0x02 << (core))


Use SHIFT_U32() instead

jenswi-linaro · 2017-07-07T07:23:58Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+}
+
+int psci_cpu_on(uint32_t core_idx, uint32_t entry,
+		uint32_t context_id __attribute__((unused)))


Please use the __unused define from <compiler.h>

jenswi-linaro · 2017-07-07T07:25:55Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+	write32(LOCK_TAG, isram_base + LOCK_ADDR_OFFSET);
+	dsb();
+
+	__asm("sev");


Please define a sev() function in <arm32.h> and use that instead.

jenswi-linaro · 2017-07-07T07:28:36Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+
+	/* in order to sync with secondary up cpu */
+	cache_op_inner(DCACHE_CLEAN_INV, NULL, 0);
+	dsb();


The function doing the clean invalidate ends with:

dsb isb

just before returning so these two should not be needed.

jenswi-linaro · 2017-07-07T07:29:13Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+	isb();
+
+	while (1)
+		__asm("wfi");


Please define a wfi() function in <arm32.h> and use that instead.

jenswi-linaro · 2017-07-07T07:35:34Z

core/drivers/serial8250_uart.c

@@ -86,7 +86,7 @@ static int serial8250_uart_getchar(struct serial_chip *chip)
 		/* Transmit FIFO is empty, waiting again */
 		;
 	}
-	return read8(base + UART_RHR);
+	return read32(base + UART_RHR);


I wonder if we should mask the result with 0xff just to make sure that this function doesn't return unexpected bits.

jenswi-linaro · 2017-07-07T10:43:18Z

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
but I'd like feedback from some of the maintainers of platforms that use the serial8250_uart driver.
@sorenb-xlnx @glneo Ping?

jforissier · 2017-07-07T11:35:37Z

but I'd like feedback from some of the maintainers of platforms that use the serial8250_uart driver.

Agreed, ideally I'd like some Tested-by's on the driver patch before merging it.

jforissier · 2017-07-07T12:02:49Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+	vaddr_t va_base = (vaddr_t)phys_to_virt_io(GRF_BASE);
+
+	wfei_mask = CORE_WFE_I_MSK(core);
+	while (!(read32(va_base + GRF_CPU_STATUS1) & wfei_mask) && loop < 500) {


What happens if loop reaches 500 and the condition is still not met? Should you be returning an error for the caller to deal with the situation? Or assert()?

I discussed with my team mate, we thought it is better to print error message and return an error code, then give up cpu up this time.

jforissier · 2017-07-07T12:08:50Z

core/arch/arm/plat-rockchip/delay.h

+#ifndef DELAY_H
+#define DELAY_H
+
+#include <arm32.h>


Replace with #include <arm.h> and move this file to core/arch/arm/include/kernel/. Other platforms will likely need this sooner or later.

Sorry for my clerical error："while (read_cntfrq() - start <= target)" , it should be read_cntpct(). I will fix this.

jforissier · 2017-07-07T12:13:02Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+	}
+}
+
+static int core_held_in_reset(uint32_t core)


s/int/bool/

pardon? I can't get what you mean.

you mean this function should return a bool type? like this: static bool core_held_in_reset(uint32_t core) ?

sorenb-xlnx · 2017-07-07T14:07:26Z

@jenswi-linaro: Zynqmp uses a Cadence UART (drivers/cdns_uart.c). I don't think we have any dependency on the 8250.

jforissier · 2017-07-07T14:35:52Z

We need T-b for platforms rpi3, mtk-mt8173 and ti-<something>, if I'm not mistaken.

igoropaniuk · 2017-07-07T14:52:39Z

@jforissier right,

$ grep -r "#include <drivers/serial8250_uart.h>" .
./documentation/porting_guidelines.md:#include <drivers/serial8250_uart.h>
./core/arch/arm/plat-mediatek/main.c:#include <drivers/serial8250_uart.h>
./core/arch/arm/plat-rpi3/main.c:#include <drivers/serial8250_uart.h>
./core/arch/arm/plat-ti/main.c:#include <drivers/serial8250_uart.h>
./core/drivers/serial8250_uart.c:#include <drivers/serial8250_uart.h>

I can test on RPi3 and TI-AM57xx today

igoropaniuk · 2017-07-07T15:52:50Z

@jforissier @jenswi-linaro
Successfully receiving SW output on RPi3, no issues found (checked on e847d4f)

Tested-by: Igor Opaniuk <igor.opaniuk@linaro.org> (serial8250_uart, RPi3)

jforissier

Just 2 minor comments, in any case:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2017-07-10T07:10:44Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+
+	val = read32(va_base + CRU_SOFTRST_CON(0));
+
+	return !!(val & CORE_HELD_IN_RESET(core));


!! is not needed since you're returning bool now

jforissier · 2017-07-10T07:11:58Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+		loop++;
+	}
+
+	return !!(read32(va_base + GRF_CPU_STATUS1) & wfei_mask);


Here also !! is useless

jenswi-linaro · 2017-07-10T07:42:36Z

@sorenb-xlnx thanks, my mistake.

igoropaniuk · 2017-07-10T08:36:43Z

The same for TI-AM57xx (checked only SW output over UART):

Tested-by: Igor Opaniuk <igor.opaniuk@linaro.org> (serial8250_uart, TI-AM57xx)

TonyXie06 · 2017-07-13T01:08:12Z

Hi, Jens, do you approve above update ?
If nothing complains, I'd like to add another 2 patches to support system suspend which is relay on above commits. Should I append 2 patches after above commits in current pull request ?

jenswi-linaro · 2017-07-13T06:36:51Z

core/arch/arm/include/kernel/delay.h

+
+#include <arm.h>
+
+static inline void udelay(uint32_t us)


This function looks a bit large to be an inline function.

jenswi-linaro · 2017-07-13T06:41:01Z

core/arch/arm/include/kernel/delay.h

+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef DELAY_H


Use __KERNEL_DELAY_H instead

jenswi-linaro · 2017-07-13T06:43:07Z

core/arch/arm/include/kernel/delay.h

+	uint64_t start, target;
+
+	start = read_cntpct();
+	target = read_cntfrq() / 1000000 * us;


Can this overflow?

Agreed, I think use 1000000ULL can fix this.

jenswi-linaro · 2017-07-13T06:46:59Z

core/arch/arm/plat-rockchip/psci_rk322x.c

+	if (!core_held_in_reset(core_idx)) {
+		wfei = wait_core_wfe_i(core_idx);
+		if (!wfei) {
+			EMSG("Can't wait cpu%d wfei before softrst", core_idx);


core_idx is an uint32_t so replace %d with %" PRIu32 "
Please fix the EMSG() below also.

jenswi-linaro · 2017-07-13T06:53:27Z

I think it's better to only add fixes to this pull request. Once we've merged this create a new pull request with the new stuff.

jenswi-linaro · 2017-07-14T05:46:52Z

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

Please fixup all the commits. You can apply my tag to all the commits, but I think we should give @jforissier another chance to approve it.

TonyXie06 · 2017-07-14T09:58:50Z

Hi, jens, do you mean I should fixup 3th~11th commits into 1st, 2nd commits, only 2 commit left finally and use git push -f ?（Sorry that I am not fimiliar with pull request work flow）

jenswi-linaro · 2017-07-14T10:23:41Z

Yes, I guess it will be around 3 commits in the end.
The 8250 commit
The delay commit
and the plat-rockchip commit

TonyXie06 · 2017-07-17T01:28:45Z

Hey, Jens, this weekend, I found MrVan's patches for plat-imx PSCI was merged a few days ago, I read it and there are something good for me to follow, so I want to have a update before fixup my commits. I will do a update later.

TonyXie06 · 2017-07-20T07:12:30Z

Hi, jens, some opinions ? Should I fixup commits ?

jenswi-linaro · 2017-07-20T07:32:39Z

Please fixup and I'll review it again.

jforissier

Looks good to me. For all 3 commits:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@TonyXie06 you can also apply @igoropaniuk's Tested-by's to the UART commit. Could someone test on mtk-mt8173?

jforissier · 2017-07-24T08:19:38Z

core/arch/arm/plat-rockchip/common.h

+#define PLAT_ROCKCHIP_COMMON_H
+
+/* For SMP cpu bootup, they are common for rockchip platforms */
+#define LOCK_TAG		0xDEADBEAF


Is it really 0xDEADBEAF and not 0xDEADBEEF?

Yes, really.

TonyXie06 · 2017-07-25T03:12:09Z

Jforissier, thank you for your approve and remind, I will apply tags after Jens review and approve.

jenswi-linaro · 2017-07-25T05:10:34Z

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

Due to hardware design, some platforms can't access the peripheral IO registers once a byte(8-bit) but once a word(32-bit). Obviously, using 32-bit accesses to the registers is more flexible for other plaforms to use serial8250 uart. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Tested-by: Igor Opaniuk <igor.opaniuk@linaro.org> (serial8250_uart, TI-AM57xx)

Using ARM Generic Timer to support time delay, make sure CNTFRQ register has been initialized before use. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

Initial version support for rockchip SoCs.(RK322X and next SoCs). This patch adds to support the RK322X. It is one of the Rockchip family SoCs, which is a 4*A7 multi-cores ARM SoCs. plat-rockchip support features: 1.Support SMP cpu boot up and power down; 2.Support system reset; 3.Support GIC driver initialization. make PLATFORM=rockchip-rk322x Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

TonyXie06 · 2017-07-25T09:38:25Z

tags applied.
I learned and will be efficient next time.

jforissier · 2017-07-25T12:51:35Z

@TonyXie06 thanks for your contribution!

One thing I notice only now: would you mind updating README.md, .travis.yml, and (optionally) MAINTAINERS.md? See for instance how it's done in https://github.com/OP-TEE/optee_os/pull/1714/files.

…nux/kernel/git/mmind/linux-rockchip into next/dt Pull "Rockchip dts32 changes for 4.14" from Heiko Stübner: Removal of the deprectated num-slots property from all Rockchip dw-mmc nodes. The rv1108 gains support for sd-cards on the evaluation board and the general nodes get a bit of cosmetic. On rk3288 the evb gains support saradc and the adc-key connected to it while some more boards also get their mali gpu enabled (fennec, evb, tinker). The biggest set of changes can be found on the rk3228/rk3229 combo this time. It gets core support for efuse, sdmmc, sdio, io-domans and spdif as well as a separate rk3229.dtsi that will keep the slight differences between the two brothers rk3228/rk3229. The evaluation board also gets some attention and abled nodes (regulators, io-domains, emmc, tsadc keys) But I think the most interesting change is the cpu enable-method for it. Instead of using the older in-kernel method, we're now also moving to handling this in firmware via the psci interface on 32bit Rockchip socs. In a recently merged pull request [0] support for the rk3228/rk3229 was added to OP-TEE including the psci support and it seems supporting other 32bit Rockchip socs that way is also planned for the future. [0] OP-TEE/optee_os#1666 * tag 'v4.14-rockchip-dts32-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip: (23 commits) ARM: dts: rockchip: fix property-ordering in rv1108 mmc nodes ARM: dts: rockchip: enable sdmmc for rv1108 evb ARM: dts: rockchip: add efuse device node for rk3228 ARM: dts: rockchip: add gpio power-key for rk3229-evb ARM: dts: rockchip: enable tsadc for rk3229-evb ARM: dts: rockchip: enable eMMC for rk3229-evb ARM: dts: rockchip: enable io-domain for rk3229-evb ARM: dts: rockchip: add cpu-supply property for cpu node of rk3229-evb ARM: dts: rockchip: add regulator nodes for rk3229-evb ARM: dts: rockchip: add sdmmc and sdio nodes for rk3228 SoC ARM: dts: rockchip: fix compatible string for eMMC node of rk3228 SoC ARM: dts: rockchip: add cpu enable method for rk3228 SoC ARM: dts: rockchip: remove num-slots from all platforms ARM: dts: rockchip: Add io-domain node for rk3228 ARM: dts: rockchip: add basic dtsi file for RK3229 SoC ARM: dts: rockchip: enable adc key for rk3288-evb ARM: dts: rockchip: enable saradc for rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-fennec ARM: dts: rockchip: enable ARM Mali GPU on rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-tinker ...

…nux/kernel/git/mmind/linux-rockchip into next/dt Pull "Rockchip dts32 changes for 4.14" from Heiko Stübner: Removal of the deprectated num-slots property from all Rockchip dw-mmc nodes. The rv1108 gains support for sd-cards on the evaluation board and the general nodes get a bit of cosmetic. On rk3288 the evb gains support saradc and the adc-key connected to it while some more boards also get their mali gpu enabled (fennec, evb, tinker). The biggest set of changes can be found on the rk3228/rk3229 combo this time. It gets core support for efuse, sdmmc, sdio, io-domans and spdif as well as a separate rk3229.dtsi that will keep the slight differences between the two brothers rk3228/rk3229. The evaluation board also gets some attention and abled nodes (regulators, io-domains, emmc, tsadc keys) But I think the most interesting change is the cpu enable-method for it. Instead of using the older in-kernel method, we're now also moving to handling this in firmware via the psci interface on 32bit Rockchip socs. In a recently merged pull request [0] support for the rk3228/rk3229 was added to OP-TEE including the psci support and it seems supporting other 32bit Rockchip socs that way is also planned for the future. [0] OP-TEE/optee_os#1666 * tag 'v4.14-rockchip-dts32-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip: (23 commits) ARM: dts: rockchip: fix property-ordering in rv1108 mmc nodes ARM: dts: rockchip: enable sdmmc for rv1108 evb ARM: dts: rockchip: add efuse device node for rk3228 ARM: dts: rockchip: add gpio power-key for rk3229-evb ARM: dts: rockchip: enable tsadc for rk3229-evb ARM: dts: rockchip: enable eMMC for rk3229-evb ARM: dts: rockchip: enable io-domain for rk3229-evb ARM: dts: rockchip: add cpu-supply property for cpu node of rk3229-evb ARM: dts: rockchip: add regulator nodes for rk3229-evb ARM: dts: rockchip: add sdmmc and sdio nodes for rk3228 SoC ARM: dts: rockchip: fix compatible string for eMMC node of rk3228 SoC ARM: dts: rockchip: add cpu enable method for rk3228 SoC ARM: dts: rockchip: remove num-slots from all platforms ARM: dts: rockchip: Add io-domain node for rk3228 ARM: dts: rockchip: add basic dtsi file for RK3229 SoC ARM: dts: rockchip: enable adc key for rk3288-evb ARM: dts: rockchip: enable saradc for rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-fennec ARM: dts: rockchip: enable ARM Mali GPU on rk3288-evb ARM: dts: rockchip: enable ARM Mali GPU on rk3288-tinker ... [ upstream commit: fc244e3 ]

jenswi-linaro reviewed Jul 7, 2017

View reviewed changes

jforissier reviewed Jul 7, 2017

View reviewed changes

jforissier approved these changes Jul 10, 2017

View reviewed changes

jenswi-linaro reviewed Jul 13, 2017

View reviewed changes

jforissier approved these changes Jul 24, 2017

View reviewed changes

JosephChen2017 added 3 commits July 25, 2017 17:24

jforissier merged commit 7176a0b into OP-TEE:master Jul 25, 2017

TonyXie06 mentioned this pull request Jul 26, 2017

Update description files for RK322X #1718

Merged


		val = read32(va_base + CRU_SOFTRST_CON(0));

		return !!(val & CORE_HELD_IN_RESET(core));

Rk322x #1666

Rk322x #1666

Conversation

TonyXie06 commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Jul 7, 2017

jforissier commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenb-xlnx commented Jul 7, 2017

jforissier commented Jul 7, 2017

igoropaniuk commented Jul 7, 2017 • edited Loading

igoropaniuk commented Jul 7, 2017 • edited Loading

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Jul 10, 2017

igoropaniuk commented Jul 10, 2017 • edited Loading

TonyXie06 commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

jenswi-linaro commented Jul 13, 2017

jenswi-linaro commented Jul 14, 2017

TonyXie06 commented Jul 14, 2017

jenswi-linaro commented Jul 14, 2017

TonyXie06 commented Jul 17, 2017

TonyXie06 commented Jul 20, 2017

jenswi-linaro commented Jul 20, 2017

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyXie06 commented Jul 25, 2017

jenswi-linaro commented Jul 25, 2017

TonyXie06 commented Jul 25, 2017

jforissier commented Jul 25, 2017

igoropaniuk commented Jul 7, 2017 •

edited

Loading

igoropaniuk commented Jul 7, 2017 •

edited

Loading

igoropaniuk commented Jul 10, 2017 •

edited

Loading

jenswi-linaro Jul 13, 2017 •

edited

Loading