drivers: sam: add SCMI reset domain management protocol support #7136

TonyHan11 · 2024-11-20T01:16:14Z

No description provided.

etienne-lms

some minor comments

etienne-lms · 2024-11-20T07:16:58Z

core/drivers/atmel_rstc.c

@@ -12,6 +12,7 @@
 #include <tee_api_defines.h>
 #include <tee_api_types.h>
 #include <types_ext.h>
+#include <drivers/rstctrl.h>


sort in alphabetical order and add #include <sys/queue.h>

etienne-lms · 2024-11-20T07:19:31Z

core/include/drivers/atmel_rstc.h

@@ -8,6 +8,12 @@
 #include <compiler.h>
 #include <stdbool.h>


add #include <util.h>

Please address the above comment.
(commit "drivers: atmel_rstc: new data and functions to handle reset assert/deassert")

etienne-lms · 2024-11-20T07:25:20Z

core/drivers/atmel_rstc.c

@@ -24,6 +24,11 @@

 static vaddr_t rstc_base;

+struct sam_reset_data {
+	bool rstc_always_secure;


add #include <stdbool.h>

Please address the above comment.
(commit "drivers: atmel_rstc: add functions to allocate/get reset controller/lines")

etienne-lms · 2024-11-20T07:29:00Z

core/include/drivers/atmel_rstc.h

@@ -30,4 +30,6 @@ static inline void sam_rstc_usb_por(unsigned char id __unused,
 				    bool enable __unused) {}
 #endif

+struct rstctrl *sam_get_rstctrl(unsigned int reset_id);


maybe consider #if defined(CFG_ATMEL_RSTC) to declare/define this function?

TonyHan11 · 2024-11-21T05:50:38Z

@etienne-lms updated according to the comments, thank you very much.

etienne-lms

1 comment to address for commit
"drivers: atmel_rstc: new data and functions to handle reset assert/deassert".

Few comments to address for commit
"drivers: atmel_rstc: add functions to allocate/get reset controller/lines".

Acked-by: Etienne Carriere <[email protected]> for commit
"drivers: atmel_rstc: probe RSTC with reset controller and reset lines" and
"plat-sam: enable SCMI reset domain management protocol and rstctrl driver".

1 comment for "plat-sam: scmi_server: use SCMI reset to manage USB reset for sama7g5". If not applicable, you can add my Acked-by tag to that commit.

etienne-lms · 2024-12-12T19:13:51Z

core/include/drivers/atmel_rstc.h

@@ -28,6 +30,7 @@ static inline bool atmel_rstc_available(void)
 static inline void atmel_rstc_reset(void) {}
 static inline void sam_rstc_usb_por(unsigned char id __unused,
 				    bool enable __unused) {}
+struct rstctrl *sam_get_rstctrl(unsigned int reset_id) { return NULL; }


static inline + __unused for the function argument.

core/include/drivers/atmel_rstc.h

+#define RESET_ID_MASK		GENMASK_32(31, 5)
+#define RESET_ID_SHIFT		U(5)
+#define RESET_BIT_POS_MASK	GENMASK_32(4, 0)
+#define RESET_OFFSET(id)	(((id) & RESET_ID_MASK) >> RESET_ID_SHIFT)


etienne-lms · 2024-12-12T19:45:08Z

core/arch/arm/plat-sam/scmi_server.c

+
+	if (!rd)
+		return SCMI_NOT_FOUND;
+


if (!rd->rstctrl) return SCMI_DENIED ?
(to match rstctrl_get_exclusive() test at line 1141)

If reset controller access is not exclusive t line 1141 then rd->rstctrl is NULL and OP-TEE core will segfault at line 1094/1097.

TonyHan11 · 2024-12-13T09:38:42Z

@etienne-lms update according to your comments and add part of the review tags, thank you.

etienne-lms

Acked-by: Etienne Carriere <[email protected]> fro commit
"drivers: clk: sam: rename the sama7g5 UTMI clocks for USB PHY".

Few comments still to address.

etienne-lms · 2024-12-13T09:41:23Z

core/include/drivers/atmel_rstc.h

@@ -8,6 +8,12 @@
 #include <compiler.h>
 #include <stdbool.h>


Please address the above comment.
(commit "drivers: atmel_rstc: new data and functions to handle reset assert/deassert")

etienne-lms · 2024-12-13T09:42:23Z

core/drivers/atmel_rstc.c

@@ -24,6 +24,11 @@

 static vaddr_t rstc_base;

+struct sam_reset_data {
+	bool rstc_always_secure;


Please address the above comment.
(commit "drivers: atmel_rstc: add functions to allocate/get reset controller/lines")

TonyHan11 · 2025-01-02T07:43:11Z

#include <stdbool.h> already added in previous update: from ac07e4e to 9e05490

update with the following changes:

add Acked-byfor commit "drivers: clk: sam: rename the sama7g5 UTMI clocks for USB PHY".
add #include <util.h>

etienne-lms

There is a comment not address on commit "plat-sam: scmi_server: use SCMI reset to manage USB reset for sama7g5".
Could you rebase this P-R to fix the merge conflicts?

etienne-lms · 2025-01-06T16:49:59Z

core/arch/arm/plat-sam/scmi_server.c

+
+	if (!rd)
+		return SCMI_NOT_FOUND;
+


If reset controller access is not exclusive t line 1141 then rd->rstctrl is NULL and OP-TEE core will segfault at line 1094/1097.

etienne-lms · 2025-01-06T16:50:33Z

core/arch/arm/plat-sam/scmi_server.c

+{
+	const struct channel_resources *resource = find_resource(channel_id);
+
+	if (resource &&  scmi_id < resource->rd_count)


nitpicking: by the way:, remove the extra space char before scmi_id.

TonyHan11 · 2025-01-07T01:39:12Z

redundant space char before scmi_id removed and fixed the check for rd and rd->rstctrl, thank you.

TonyHan11 · 2025-01-07T01:42:45Z

rebased on the latest master branch with the merge conflict fixed.

TonyHan11 · 2025-01-07T03:30:21Z

Add a commit to fix the compile error caused by the wrong path of the header file included in core/drivers/microchip_pit.c.

TonyHan11 · 2025-01-07T08:12:53Z

Sorry for adding another fix for correcting the length used for comparing the clock names in core/drivers/microchip_pit.c.

etienne-lms

I think you should squash commits
"drivers: atmel_rstc: new data and functions to handle reset assert/deassert"
and "drivers: atmel_rstc: add functions to allocate/get reset controller/lines"
unless what to former likely triggers a build warning on unused sam_rst_list.

Acked-by: Etienne Carriere <[email protected]> for commits
"drivers: atmel_rstc: add functions to allocate/get reset controller/lines" (with malloc.h inclusion fixed)
and "plat-sam: scmi_server: use SCMI reset to manage USB reset for sama7g5".

For commit "drivers: microchip_pit: fix the path of "dt_driver.h" included", could you add a Fixes: tag to the commit message. With that addressed,
Acked-by: Etienne Carriere <[email protected]> for that commit.

etienne-lms · 2025-01-07T11:15:52Z

core/drivers/atmel_rstc.c

+	if (sam_rstline)
+		return sam_rstline;
+
+	sam_rstline = calloc(1, sizeof(*sam_rstline));


#include <malloc.h>

The UTMI clocks for USB PHY are handled in OP-TEE due to they are controlled by the registers from RSTC (reset controller) which is always-secured. SCMI "reset domain management protocol" makes it prossible to handle the resets from the kernel running in normal world. So the code in kernel for these clocks need to be enabled. Here renaming the clocks to avoid registering them failed from the kernel. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

TonyHan11 · 2025-01-08T02:26:17Z

Commits "drivers: atmel_rstc: new data and functions to handle reset assert/deassert"
and "drivers: atmel_rstc: add functions to allocate/get reset controller/lines" not
squashed. The definition of sam_rst_list is moved to the 2nd commit as it is not
used in the 1st commit. Acked-by added for the 2nd commit.

Acked-by added for commits
"drivers: atmel_rstc: add functions to allocate/get reset controller/lines" (#include <malloc.h> added)
and "plat-sam: scmi_server: use SCMI reset to manage USB reset for sama7g5".

Fixes added to the commit messages for the last 2 commits.
Acked-by added for commit "drivers: microchip_pit: fix the path of "dt_driver.h" included".

Thank you very much.

etienne-lms

Acked-by: Etienne Carriere <[email protected]> for commit
"drivers: microchip_pit: fix the length used for comparing the clock names".
I've added a comment for another potential debug inconvenience.

etienne-lms · 2025-01-08T07:32:09Z

core/drivers/microchip_pit.c

@@ -34,7 +34,8 @@ static TEE_Result microchip_pit_probe(const void *fdt, int node,

 	do {
 		parent = clk_get_parent_by_index(gclk, i++);
-		if (!memcmp("syspll", clk_get_name(parent), sizeof("syspll")))
+		if (!memcmp("syspll", clk_get_name(parent),
+			    sizeof("syspll") - 1))


For consistency, i think you also should also test parent is not NULL (at least with assert()) to get a better trace message instead of a segfault from clk_get_name(). No strong issue here since anyway you expect a panic at line 42 but this line would never be reached.

TonyHan11 · 2025-01-09T02:40:07Z

Acked-by added and also moved the test of parent into the while loop to improve debug convenience.

etienne-lms

Acked-by: Etienne Carriere <[email protected]> for commit
"drivers: atmel_rstc: new data and functions to handle reset assert/deassert".

The whole series looks good to me.

…assert Define new struct and functions for handling the reset controller, reset lines and the reset operations (assert, deassert). Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

…ines Define new functions for getting the reset controller, find or allocate the reset lines. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Update the type of RSTC driver to DT_DRIVER_RSTCTRL and probe RSTC with the concept of controller and lines. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Enable "reset domain management protocol", add reset domains to SCMI channel and add functions for SCMI reset domain. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

…iver Enable CFG_SCMI_MSG_RESET_DOMAIN and CFG_DRIVERS_RSTCTRL. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

change "#include <dt_driver.h>" to "#include <kernel/dt_driver.h>" due to "dt_driver.h" is located at "core/include/kernel/" and the path is not in the include paths which would cause compile errors. Fixes: 8796ab4 ("drivers: microchip_pit: add driver for sama7g54's pit64b") Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

…ames sizeof("syspll") equals to 7 with a character '\0' in count, adjust the length used for the comparison to obtain the expected result. Move the test of "parent" into the while loop to improve debug convenience. Fixes: 8796ab4 ("drivers: microchip_pit: add driver for sama7g54's pit64b") Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

TonyHan11 · 2025-01-09T08:44:54Z

@etienne-lms The review tag added, thanks a lot.

etienne-lms reviewed Nov 20, 2024

View reviewed changes

TonyHan11 force-pushed the scmi_reset branch from ac07e4e to 9e05490 Compare November 21, 2024 05:45

etienne-lms reviewed Dec 13, 2024

View reviewed changes

TonyHan11 force-pushed the scmi_reset branch from 9e05490 to 122c571 Compare December 13, 2024 09:36

etienne-lms reviewed Dec 13, 2024

View reviewed changes

TonyHan11 force-pushed the scmi_reset branch from 122c571 to a727022 Compare January 2, 2025 07:40

etienne-lms reviewed Jan 6, 2025

View reviewed changes

TonyHan11 force-pushed the scmi_reset branch from a727022 to bee1b25 Compare January 7, 2025 01:36

TonyHan11 force-pushed the scmi_reset branch from bee1b25 to 7960ff3 Compare January 7, 2025 01:41

TonyHan11 force-pushed the scmi_reset branch from 4a10495 to a087c30 Compare January 7, 2025 03:34

etienne-lms reviewed Jan 7, 2025

View reviewed changes

TonyHan11 force-pushed the scmi_reset branch from e4095f0 to 0c89893 Compare January 8, 2025 02:23

etienne-lms reviewed Jan 8, 2025

View reviewed changes

TonyHan11 force-pushed the scmi_reset branch from 0c89893 to 0d1229b Compare January 9, 2025 02:39

etienne-lms reviewed Jan 9, 2025

View reviewed changes

TonyHan11 added 5 commits January 9, 2025 16:41

drivers: atmel_rstc: add functions to allocate/get reset controller/l…

5a650d0

…ines Define new functions for getting the reset controller, find or allocate the reset lines. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

drivers: atmel_rstc: probe RSTC with reset controller and reset lines

1474323

Update the type of RSTC driver to DT_DRIVER_RSTCTRL and probe RSTC with the concept of controller and lines. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

plat-sam: scmi_server: use SCMI reset to manage USB reset for sama7g5

c805917

Enable "reset domain management protocol", add reset domains to SCMI channel and add functions for SCMI reset domain. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

plat-sam: enable SCMI reset domain management protocol and rstctrl dr…

3dc94a9

…iver Enable CFG_SCMI_MSG_RESET_DOMAIN and CFG_DRIVERS_RSTCTRL. Signed-off-by: Tony Han <[email protected]> Acked-by: Etienne Carriere <[email protected]>

TonyHan11 added 2 commits January 9, 2025 16:42

TonyHan11 force-pushed the scmi_reset branch from 0d1229b to b9cf6cb Compare January 9, 2025 08:43

jforissier merged commit 9145b71 into OP-TEE:master Jan 9, 2025
10 checks passed

TonyHan11 deleted the scmi_reset branch January 10, 2025 02:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sam: add SCMI reset domain management protocol support #7136

drivers: sam: add SCMI reset domain management protocol support #7136

TonyHan11 commented Nov 20, 2024

etienne-lms left a comment

etienne-lms Nov 20, 2024

etienne-lms Nov 20, 2024

etienne-lms Dec 13, 2024

etienne-lms Nov 20, 2024

etienne-lms Dec 13, 2024

etienne-lms Nov 20, 2024

TonyHan11 commented Nov 21, 2024

etienne-lms left a comment

etienne-lms Dec 12, 2024

This comment was marked as resolved.

etienne-lms Dec 12, 2024

etienne-lms Jan 6, 2025

TonyHan11 commented Dec 13, 2024

etienne-lms left a comment

etienne-lms Dec 13, 2024

etienne-lms Dec 13, 2024

TonyHan11 commented Jan 2, 2025

etienne-lms left a comment

etienne-lms Jan 6, 2025

etienne-lms Jan 6, 2025

TonyHan11 commented Jan 7, 2025

TonyHan11 commented Jan 7, 2025

TonyHan11 commented Jan 7, 2025

TonyHan11 commented Jan 7, 2025

etienne-lms left a comment

etienne-lms Jan 7, 2025

TonyHan11 commented Jan 8, 2025

etienne-lms left a comment

etienne-lms Jan 8, 2025

TonyHan11 commented Jan 9, 2025

etienne-lms left a comment

TonyHan11 commented Jan 9, 2025

drivers: sam: add SCMI reset domain management protocol support #7136

drivers: sam: add SCMI reset domain management protocol support #7136

Conversation

TonyHan11 commented Nov 20, 2024

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

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

TonyHan11 commented Nov 21, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyHan11 commented Dec 13, 2024

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

TonyHan11 commented Jan 2, 2025

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

TonyHan11 commented Jan 7, 2025

TonyHan11 commented Jan 7, 2025

TonyHan11 commented Jan 7, 2025

TonyHan11 commented Jan 7, 2025

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyHan11 commented Jan 8, 2025

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyHan11 commented Jan 9, 2025

etienne-lms left a comment

Choose a reason for hiding this comment

TonyHan11 commented Jan 9, 2025