drivers: sam: update sama7g5 configurations for wakeup from low-power #6831
Conversation
core/drivers/pm/sam/at91_pm.c
Outdated
| * configure regions with write permission with not forced to | ||
| * XN (Execute-never) attribute. | ||
| */ | ||
| if (read_id_pfr1() & GENMASK_32(15, 12)) |
There was a problem hiding this comment.
Please add and use proper defines for both the mask and the bit below.
There was a problem hiding this comment.
Where do you restore the XN bit?
core/drivers/pm/sam/at91_pm.c
Outdated
| @@ -395,7 +395,7 @@ static TEE_Result at91_pm_sram_init(const void *fdt) | |||
| at91_suspend_sram_pbase = virt_to_phys((void *)at91_suspend_sram_base); | |||
|
|
|||
| /* Map the secure ram suspend code to be executable */ | |||
There was a problem hiding this comment.
Please update this comment to match what you're doing.
There was a problem hiding this comment.
This comment is not addressed.
core/drivers/pm/sam/at91_pm.c
Outdated
| @@ -236,6 +236,7 @@ static int at91_pm_config_ws_ulp1(bool set) | |||
| */ | |||
| static bool at91_pm_verify_clocks(void) | |||
| { | |||
| int pck_cnt = 4; | |||
There was a problem hiding this comment.
Please add and use an AT91_PMC_PKC_COUNT define depending on CFG_SAMA7G5 instead of using magic numbers here.
|
Thank you @jenswi-linaro . Code & commit message updated according to the comments. |
core/drivers/pm/sam/at91_pm.c
Outdated
| @@ -298,6 +311,7 @@ static TEE_Result at91_enter_backup(void) | |||
| TEE_Result atmel_pm_suspend(uintptr_t entry, struct sm_nsec_ctx *nsec) | |||
| { | |||
| TEE_Result res = TEE_ERROR_GENERIC; | |||
| int sctlr = 0; | |||
There was a problem hiding this comment.
This should be of an unsigned type, either uint32_t or perhaps unsigned long.
core/drivers/pm/sam/at91_pm.c
Outdated
| * configure regions with write permission with not forced to | ||
| * XN (Execute-never) attribute. | ||
| */ | ||
| if (read_id_pfr1() & IDPFR1_VIRT_MASK) { |
There was a problem hiding this comment.
Wouldn't it make sense to read SCTLR and if SCTLR_WXN is set clear it, regardless of ID_PFR1?
The bit is reserved as 0 if not supported.
core/drivers/pm/sam/at91_pm.c
Outdated
| */ | ||
| if (read_id_pfr1() & IDPFR1_VIRT_MASK) { | ||
| sctlr = read_sctlr(); | ||
| write_sctlr(sctlr & ~SCTLR_WXN); |
There was a problem hiding this comment.
From the Arm ARM it looks like you need to invalidate the TLB also.
core/drivers/pm/sam/at91_pm.c
Outdated
| if (soc_pm.mode == AT91_PM_BACKUP) { | ||
| res = at91_enter_backup(); | ||
| } else { | ||
| at91_suspend_sram_fn(&soc_pm); | ||
| res = TEE_SUCCESS; | ||
| } | ||
|
|
||
| /* Restore the XN attribute */ | ||
| if (read_id_pfr1() & IDPFR1_VIRT_MASK) |
There was a problem hiding this comment.
How about checking if SCTLR_WXN is set in the sctlr variable and only write it then?
I believe the TLB should be invalidated if SCTLR_WXN is changed in SCTLR here too.
core/drivers/pm/sam/at91_pm.c
Outdated
| @@ -395,7 +395,7 @@ static TEE_Result at91_pm_sram_init(const void *fdt) | |||
| at91_suspend_sram_pbase = virt_to_phys((void *)at91_suspend_sram_base); | |||
|
|
|||
| /* Map the secure ram suspend code to be executable */ | |||
There was a problem hiding this comment.
This comment is not addressed.
|
Updated according to the comments, thank you. |
| sctlr = read_sctlr(); | ||
| if (sctlr & SCTLR_WXN) { | ||
| write_sctlr(sctlr & ~SCTLR_WXN); | ||
| tlbi_all(); |
There was a problem hiding this comment.
Given that we restore the WXN bit below without changing ASID we could optimize this by using tlbi_asid() with the current ASID instead. But it may be useless depending on what the CPU does with the TLB during suspend, I leave it to you to decide if it's worth the effort.
There was a problem hiding this comment.
Rebased with the review tags added and thank you for you suggestion. Currently we do not consider about the ASID and just use tlbi_all() as the simplest way to make it work.
Configure regions with write permission with not forced to XN (Execute-never) attribute when implementation includes the Virtualization Extensions. Signed-off-by: Tony Han <tony.han@microchip.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Mapping with memory area type MEM_AREA_TEE_RAM would enable cacheable attribute, it might cause abort in same cases. Here change the memory area type to MEM_AREA_TEE_COHERENT to map SRAM with non-cacheable attribute to avoid the aborts in the low-power process. Signed-off-by: Tony Han <tony.han@microchip.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Rename the names of functions for adding the configuration of sama7g5 wakeup sources. Add and configure the wakeup sources for sama7g5. Signed-off-by: Tony Han <tony.han@microchip.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Add definition "AT91_PMC_PCK_COUNT", in sama7g5 there're 8 PCK clocks and for sama5d2 there're 4 PCK clocks. Signed-off-by: Tony Han <tony.han@microchip.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
The offset of "PMC CPU Clock Register" for sama7g5 is different from the one for sama5d2. Signed-off-by: Tony Han <tony.han@microchip.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
No description provided.