plat/synquacer: enable SCMI support #1856

masahisak · 2019-03-05T06:13:33Z

Enable the SCMI protocol support in SynQuacer platform.
Aside from power domain, system power and apcore management protocol,
this commit adds the vendor specific protocol(0x80).
This vendor specific protocol is used to get the dram mapping information
from SCP.

This commit also exposes scmi_handle to outside of drivers/arm/css/scmi,
to issue the SCMI vendor specific procotol from platform dependent code.

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

ssg-bot · 2019-03-05T06:14:40Z

Can one of the admins verify this patch?

arm-tf-bot · 2019-03-05T06:16:07Z

This pull request modifies subsystem(s) maintained by @b49020. Can you please review the changes?

ghost · 2019-03-05T10:57:14Z

IP Review +1

ghost · 2019-03-05T10:57:30Z

jenkins: test this please

ghost · 2019-03-05T11:39:15Z

The CI failure is due to a MISRA defect we can ignore.

soby-mathew · 2019-03-05T15:05:08Z

plat/socionext/synquacer/drivers/scmi/sq_scmi_private.h

+#include <lib/mmio.h>
+
+/* SCMI vendor specific protocol */
+#define SCMI_SYS_VENDOR_EXT_PROTO_ID		0x80


This is the only difference from scmi_private.h. Would suggest to add this to the scmi_private.h and remove this file.

soby-mathew · 2019-03-05T15:26:06Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.h

+
+#include <sq_common.h>
+
+int scmi_get_draminfo(void *p, struct draminfo *info);


This can go at the end of scmi.h. Create a section for vendor extensions using code comments and add this there.

soby-mathew · 2019-03-05T15:27:21Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.c

+	int status;
+	int reserved;
+	struct draminfo info;
+};


This structure can go in scmi.h at the end where a section for "vendor-specific" extensions can be created.

I have submitted updated commit.
I really hesitate to add this definitions into include/drivers/arm/css/scmi.h.
I also need to add very vendor specific "struct draminfo" definition here.

Instead of modifying scmi.h, I added drivers/arm/css/scmi/vendor/scmi_sq.h.
This file contains synquacer specific protocol definition.

If you prefer to modify scmi.h, I will do so.

ok, what you have done is fine.

soby-mathew · 2019-03-05T15:29:42Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.c

+/*
+ * API to set the SCMI vendor extension protocol
+ */
+int scmi_get_draminfo(void *p, struct draminfo *info)


This can go into a new file within the following folder drivers/arm/css/scmi/vendor/scmi_sq.c.

soby-mathew · 2019-03-05T15:53:22Z

plat/socionext/synquacer/include/platform_def.h

+/* Local power state for retention. Valid only for CPU power domains */
+#define ARM_LOCAL_STATE_RET	U(1)
+/* Local power state for OFF/power-down. Valid for CPU and cluster power domains */
+#define ARM_LOCAL_STATE_OFF	U(2)


This is introducing ARM platform layer dependency which might be a maintenance overhead in future. Currently the Power management for SCMI goes through a css_pm_scmi which introduces this dependency. How about removing that dependancy by implementing the functionality in css_pm_scmi.c in synquancer which can then directly invoke SCMI driver ?

I think that would be clean solution.

It seems like duplicating stuff. Shouldn't we try to make files under drivers/arm/css/scp/ generic for reuse in other platforms? Like renaming these macros to PLAT_* rather than ARM_* in common css_pm_scmi.c. Or else if they need to be platform specific then move them to plat/arm.

The css_scp layer will have platform implementation details like the power levels available on the platform , the mapping between PSCI power state to SCP power state which makes it difficult to make it generic. The SCMI driver is generic where as css-scp is ARM platform specific.

Or else if they need to be platform specific then move them to plat/arm.

Hmm, I see your point. It used to be within plat/arm previously and was not intended to be generic. But I agree atleast the scp folder should should be within platform directory. The scmi folder can go into drivers/ folder. I will create a task to do that internally.

Sounds good.

soby-mathew · 2019-03-05T15:54:54Z

plat/socionext/synquacer/sq_psci.c

@@ -33,12 +35,16 @@ uintptr_t sq_sec_entrypoint;

 int sq_pwr_domain_on(u_register_t mpidr)
 {
+#if SQ_USE_SCMI_DRIVER
+	css_scp_on(mpidr);


As stated in my previous comment, instead of calling this function, implement a synq_scmi_on() function. This function can directly call the required SCMI driver.

soby-mathew

I think all reference to CSS in macro names and functions can be removed. See my comments.

soby-mathew · 2019-03-06T17:06:59Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.c

+
+#include <sq_common.h>
+
+const uint32_t plat_css_core_pos_to_scmi_dmn_id_map[PLATFORM_CORE_COUNT] = {


I think all the contents of this file can move to sq_pm_scmi.c

This array can just be called sq_core_pos_to_scmi_dmn_id[]

soby-mathew · 2019-03-06T17:08:00Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.c

+
+static scmi_channel_plat_info_t sq_scmi_plat_info = {
+		.scmi_mbx_mem = SQ_SCMI_PAYLOAD_BASE,
+		.db_reg_addr = PLAT_CSS_MHU_BASE + MHU_CPU_INTR_S_SET_OFFSET,


I dont think you need to use any CSS macro anymore. This can be PLAT_SQ_MHU_BASE

PLAT_CSS_MHU_BASE is used in include/drivers/arm/css/css_mhu_doorbell.h, and I still need
this macro to use generic MHU driver.
So, I submit separate commit to rename PLAT_CSS_MHU_BASE to PLAT_MHUV2_BASE.
I think it is not the best solution, but it is minimal modification without modifying much under plat/arm.

soby-mathew · 2019-03-06T17:09:41Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.c

+		.ring_doorbell = &mhu_ring_doorbell,
+};
+
+scmi_channel_plat_info_t *plat_css_get_scmi_info()


You dont need this wrapper function anymore.

void __init plat_arm_pwrc_setup(void) { channel.info = &sq_scmi_plat_info; ...

soby-mathew · 2019-03-06T17:10:28Z

plat/socionext/synquacer/drivers/scmi/sq_scmi.c

+	return &sq_scmi_plat_info;
+}
+
+void plat_arm_gic_cpuif_disable(void)


This function is not needed anymore

soby-mathew · 2019-03-06T17:11:24Z

plat/socionext/synquacer/drivers/scp/sq_pm_scmi.c

+	return 0;
+}
+
+void __init plat_arm_pwrc_setup(void)


This function can be plat_sq_pwrc_setup()

soby-mathew · 2019-03-06T17:12:04Z

plat/socionext/synquacer/drivers/mhu/sq_mhu.c

@@ -41,27 +41,27 @@ void mhu_secure_message_start(unsigned int slot_id)
 	bakery_lock_get(&sq_lock);

 	/* Make sure any previous command has finished */
-	while (mmio_read_32(PLAT_SQ_MHU_BASE + CPU_INTR_S_STAT) &
+	while (mmio_read_32(PLAT_CSS_MHU_BASE + CPU_INTR_S_STAT) &


This change is not required as per my comments below.

soby-mathew · 2019-03-06T17:12:42Z

plat/socionext/synquacer/drivers/scp/sq_pm_scmi.c

+		panic();
+	}
+	wfi();
+	ERROR("CSS set power state: operation not handled.\n");


remove CSS from print messages.

soby-mathew · 2019-03-06T17:13:14Z

plat/socionext/synquacer/drivers/scp/sq_pm_scmi.c

+ * The global handle for invoking the SCMI driver APIs after the driver
+ * has been initialized.
+ */
+void *sq_scmi_handle;


The handle can be static now.

soby-mathew · 2019-03-06T17:13:50Z

plat/socionext/synquacer/include/platform_def.h

 #define PLAT_MAX_PWR_LVL		U(1)
+/* System power domain level */
+#define CSS_SYSTEM_PWR_DMN_LVL	SQ_PWR_LVL2


This macro is not required I think.

soby-mathew · 2019-03-06T17:14:07Z

plat/socionext/synquacer/include/platform_def.h

@@ -69,7 +81,7 @@

 #define DRAMINFO_BASE			0x2E00FFC0

-#define PLAT_SQ_MHU_BASE		0x45000000
+#define PLAT_CSS_MHU_BASE		0x45000000


No need to change the name of macro.

arm-tf-bot · 2019-03-07T02:45:49Z

This pull request modifies subsystem(s) maintained by @npoushin, @thomas-arm. Can you please review the changes?

masahisak · 2019-03-07T02:48:26Z

Thank you for your comment.
Could you kindly review the new patches?

npoushin

Build tested, looks ok for sgi575

b49020

Apart from few nitpicks, this patch-set looks good to me. So:
Acked-by: Sumit Garg <sumit.garg@linaro.org>

b49020 · 2019-03-12T05:31:14Z

plat/socionext/synquacer/sq_psci.c

@@ -11,6 +11,8 @@

 #include <arch_helpers.h>
 #include <common/debug.h>
+#include <drivers/arm/css/css_scp.h>
+#include <drivers/arm/css/scmi.h>


Do we need these includes now?

b49020 · 2019-03-12T05:33:17Z

plat/socionext/synquacer/platform.mk

-				-I$(PLAT_PATH)/drivers/mhu
+				-I$(PLAT_PATH)/drivers/mhu \
+                -Idrivers/arm/css/scmi \
+                -Idrivers/arm/css/scmi/vendor


Use tabs for indentation similar to other places.

b49020 · 2019-03-12T05:43:24Z

plat/socionext/synquacer/include/platform_def.h

+#define SQ_CORE_PWR_STATE(state)	(state)->pwr_domain_state[SQ_PWR_LVL0]
+#define SQ_CLUSTER_PWR_STATE(state)	(state)->pwr_domain_state[SQ_PWR_LVL1]
+#define SQ_SYSTEM_PWR_STATE(state)	((PLAT_MAX_PWR_LVL > SQ_PWR_LVL1) ?\
+				(state)->pwr_domain_state[SQ_PWR_LVL2] : 0)


Since these macros are defined here, we can remove their duplicate definition from sq_psci.c and rather include this header file.

soby-mathew

The changes look fine. Please let us know if you intend to fix the nits mentioned by @b49020 .

soby-mathew · 2019-03-12T09:59:33Z

jenkins: test this please

Enable the SCMI protocol support in SynQuacer platform. Aside from power domain, system power and apcore management protocol, this commit adds the vendor specific protocol(0x80). This vendor specific protocol is used to get the dram mapping information from SCP. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

MHU doorbell driver requires arm platform specific macro "PLAT_CSS_MHU_BASE". Rename it to "PLAT_MHUV2_BASE", so that platforms other than arm can use generic MHU doorbell driver. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

masahisak · 2019-03-13T01:06:00Z

The changes look fine. Please let us know if you intend to fix the nits mentioned by @b49020 .

I addressed comments, please kindly check.

soby-mathew requested changes Mar 5, 2019

View reviewed changes

masahisak force-pushed the synquacer-scmi-support branch from 5bf9729 to 18099e2 Compare March 6, 2019 06:24

soby-mathew reviewed Mar 6, 2019

View reviewed changes

soby-mathew mentioned this pull request Mar 6, 2019

Reorganize drivers/arm/css folder ARM-software/tf-issues#683

Open

masahisak force-pushed the synquacer-scmi-support branch from 18099e2 to 287aab0 Compare March 7, 2019 02:41

npoushin approved these changes Mar 11, 2019

View reviewed changes

b49020 reviewed Mar 12, 2019

View reviewed changes

soby-mathew approved these changes Mar 12, 2019

View reviewed changes

Masahisa Kojima added 2 commits March 13, 2019 09:54

plat/arm: mhu: make mhu driver generic

cf6c30e

MHU doorbell driver requires arm platform specific macro "PLAT_CSS_MHU_BASE". Rename it to "PLAT_MHUV2_BASE", so that platforms other than arm can use generic MHU doorbell driver. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

masahisak force-pushed the synquacer-scmi-support branch from 287aab0 to cf6c30e Compare March 13, 2019 01:03

b49020 approved these changes Mar 13, 2019

View reviewed changes

soby-mathew merged commit eb9da9e into ARM-software:integration Mar 13, 2019

masahisak deleted the synquacer-scmi-support branch March 13, 2019 23:49


		#include <sq_common.h>

		int scmi_get_draminfo(void p, struct draminfo info);


		#include <sq_common.h>

		const uint32_t plat_css_core_pos_to_scmi_dmn_id_map[PLATFORM_CORE_COUNT] = {

plat/synquacer: enable SCMI support #1856

plat/synquacer: enable SCMI support #1856

Conversation

masahisak commented Mar 5, 2019

ssg-bot commented Mar 5, 2019

arm-tf-bot commented Mar 5, 2019

ghost commented Mar 5, 2019

ghost commented Mar 5, 2019

ghost commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masahisak Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soby-mathew left a comment

Choose a reason for hiding this comment

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

arm-tf-bot commented Mar 7, 2019

masahisak commented Mar 7, 2019

npoushin left a comment

Choose a reason for hiding this comment

b49020 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

soby-mathew left a comment

Choose a reason for hiding this comment

soby-mathew commented Mar 12, 2019

masahisak commented Mar 13, 2019

masahisak Mar 6, 2019 •

edited

Loading