Rk322x adds PSCI version, features and system suspend #1720

TonyXie06 · 2017-07-26T13:10:21Z

RK322X adds PSCI version, features and system suspend implementations.
PSCI version and features must be implemented, because Linux Kernel will check whether optee supports PSCI system suspend or not by getting status from PSCI version and features.

Signed-off-by: Joseph Chen chenjh@rock-chips.com

jforissier

Please find some minor comments on the "system suspend" patch. The other ones LGTM.
@etienne-lms any comment?

jforissier · 2017-07-27T11:19:57Z

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

+ * you needs set the write-mask bits at the same time, the write-mask bits is
+ * in high 16-bits.
+ * The fllowing macro definition helps access write-mask bits reg efficient.
+ ******************************************************************************/


No ********* please (open comment block with /* and close with */)

Spelling

jforissier · 2017-07-27T12:02:04Z

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

+#define BITS_SHIFT(bits, shift)	SHIFT_U32(bits, shift)
+#define BITS_WMSK(msk, shift)	SHIFT_U32(msk, (shift) + REG_MSK_SHIFT)
+#define BITS_WITH_WMASK(bits, msk, shift)\
+	(BITS_SHIFT(bits, shift) | BITS_SHIFT(msk, ((shift) + REG_MSK_SHIFT)))


This can be rewritten in a simpler way:

#define BITS_WMSK(msk, shift) SHIFT_U32(msk, (shift) + 16) #define BITS_WITH_WMASK(bits, msk, shift) \ (SHIFT_U32(bits, shift) | BITS_WMSK(msk, shift))

jforissier · 2017-07-27T12:37:08Z

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

+	0x000f,
+};
+
+static void clks_gating(void)


It's usually preferred to use a verb for functions that perform some action, what about clks_disable(void)?

MrVan · 2017-07-28T01:51:24Z

@TonyXie06 I just have a common question. rx322x is armv7 based soc, and you enabled CONFIG_ARM_PSCI_FW and suspend/low power idle are all moved to psci firmware, then in linux side, only need to smc trap to psci firmware and no touch clock and cpu_pm_cluster_entry/exit and else, right?

TonyXie06 · 2017-07-31T01:37:59Z

@MrVan On armv7 based soc, we don't move cpuidle to psci firmware, only cpu on/off and system supend are moved. Cpuidle is still left in linux side, it will not trap into secure world by cpu_suspend().

MrVan · 2017-07-31T02:36:15Z

@TonyXie06 ok, then you platform do not support cpu off when low power cpu idle.

TonyXie06 · 2017-08-09T00:37:21Z

Hi, @jforissier any comments ?

jforissier · 2017-08-09T08:21:58Z

Hi @TonyXie06,

Thanks for the updates. You may add my:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

...to all patches, and I think we're good to have this merged.

TonyXie06 · 2017-08-10T01:14:52Z

tags applied

etienne-lms · 2017-08-10T07:29:13Z

Look fine to me regarding the generic psci layer.
Crosscheck comment about timeout handling in platform specifc code.
Sorry for such late comments.

jforissier · 2017-08-10T08:14:36Z

@etienne-lms did you post a comment? I can't see it...

etienne-lms · 2017-07-28T13:43:34Z

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

+		loop++;
+	}
+
+	assert(read32(va_base + CRU_PLL_CON1(pll)) & PLL_LOCK);


assert or panic ?

You prefer panic ? Approved, because assert doesn't work if NDEBUG is defined.

etienne-lms · 2017-08-10T08:17:22Z

Sorry, here it is. It was still pending upon an opened review.

etienne-lms

Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
with minor comments addressed or not.

etienne-lms · 2017-08-15T22:07:44Z

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

+ * Some register has write-mask bits, it means if you want to set the bits,
+ * you need set the write-mask bits at the same time, the write-mask bits is
+ * in high 16-bits. The following macro definition helps you access register
+ * efficient.


minor: enficiently ?

etienne-lms · 2017-08-15T22:12:19Z

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

+
+int psci_features(uint32_t psci_fid)
+{
+	if ((psci_fid == PSCI_PSCI_FEATURES) ||


very minor: a switch(psci_fid) looks better.

Add __weak property for the function, developers could have their own implementation. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

Improve PSCI version to PSCI_VERSION_1_0. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

Add currently implemented PSCI functions. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

TonyXie06 · 2017-08-16T01:30:20Z

update with etienne-lms' minor comments addressed.
fixup commits;
apply tags for all commits: Acked-by: Etienne Carriere etienne.carriere@linaro.org

etienne-lms

some late re-review

etienne-lms · 2017-08-16T07:37:28Z

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

+#define WMSK_BIT(nr)		BIT((nr) + REG_MSK_SHIFT)
+#define BIT_WITH_WMSK(nr)	(BIT(nr) | WMSK_BIT(nr))
+#define BITS_WMSK(msk, shift)	SHIFT_U32(msk, (shift) + REG_MSK_SHIFT)
+#define BITS_WITH_WMASK(bits, msk, shift)\


minor: a space or tab before the \.

etienne-lms · 2017-08-16T07:38:33Z

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

+
+static struct dram_data dram_d;
+
+static uint32_t clks_gating_table[CRU_CLKGATE_CON_CNT] = {


Support gating clks and power down PLLs. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

TonyXie06 · 2017-08-16T09:35:46Z

fix 2 minors that you commented.

TonyXie06 · 2017-08-21T01:08:40Z

Any comments?

jforissier · 2017-08-21T06:29:38Z

@TonyXie06 sorry, somehow I was thinking we were waiting for @etienne-lms's Ack but I realize all is good. So I'm merging this PR. Thanks!

jforissier reviewed Jul 27, 2017

View reviewed changes

TonyXie06 force-pushed the rk322x-system-suspend branch from 48687c0 to 488f7fd Compare July 31, 2017 01:40

TonyXie06 force-pushed the rk322x-system-suspend branch 2 times, most recently from 1cae9c0 to 4820eba Compare August 9, 2017 13:21

etienne-lms reviewed Aug 10, 2017

View reviewed changes

etienne-lms approved these changes Aug 15, 2017

View reviewed changes

JosephChen2017 added 3 commits August 16, 2017 09:21

plat-rockchip: rk322x: add PSCI version

00d47cf

Improve PSCI version to PSCI_VERSION_1_0. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

plat-rockchip: rk322x: add PSCI features

7922149

Add currently implemented PSCI functions. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

TonyXie06 force-pushed the rk322x-system-suspend branch from 1cd9ade to cdf5f93 Compare August 16, 2017 01:25

etienne-lms reviewed Aug 16, 2017

View reviewed changes

plat-rockchip: rk322x: add PSCI system suspend

0892b0a

Support gating clks and power down PLLs. Signed-off-by: Joseph Chen <chenjh@rock-chips.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

TonyXie06 force-pushed the rk322x-system-suspend branch from cdf5f93 to 0892b0a Compare August 16, 2017 09:31

jforissier merged commit 110da4b into OP-TEE:master Aug 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rk322x adds PSCI version, features and system suspend #1720

Rk322x adds PSCI version, features and system suspend #1720

TonyXie06 commented Jul 26, 2017

jforissier left a comment

jforissier Jul 27, 2017

jforissier Jul 27, 2017

jforissier Jul 27, 2017

MrVan commented Jul 28, 2017

TonyXie06 commented Jul 31, 2017

MrVan commented Jul 31, 2017

TonyXie06 commented Aug 9, 2017

jforissier commented Aug 9, 2017

TonyXie06 commented Aug 10, 2017

etienne-lms commented Aug 10, 2017

jforissier commented Aug 10, 2017

etienne-lms Jul 28, 2017

TonyXie06 Aug 14, 2017

etienne-lms commented Aug 10, 2017

etienne-lms left a comment

etienne-lms Aug 15, 2017

etienne-lms Aug 15, 2017

TonyXie06 commented Aug 16, 2017

etienne-lms left a comment

etienne-lms Aug 16, 2017

etienne-lms Aug 16, 2017

TonyXie06 commented Aug 16, 2017

TonyXie06 commented Aug 21, 2017

jforissier commented Aug 21, 2017


		static struct dram_data dram_d;

		static uint32_t clks_gating_table[CRU_CLKGATE_CON_CNT] = {

Rk322x adds PSCI version, features and system suspend #1720

Rk322x adds PSCI version, features and system suspend #1720

Conversation

TonyXie06 commented Jul 26, 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

Choose a reason for hiding this comment

MrVan commented Jul 28, 2017

TonyXie06 commented Jul 31, 2017

MrVan commented Jul 31, 2017

TonyXie06 commented Aug 9, 2017

jforissier commented Aug 9, 2017

TonyXie06 commented Aug 10, 2017

etienne-lms commented Aug 10, 2017

jforissier commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Aug 10, 2017

etienne-lms 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 Aug 16, 2017

etienne-lms 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 Aug 16, 2017

TonyXie06 commented Aug 21, 2017

jforissier commented Aug 21, 2017