From: Ryan Wanner <Ryan.Wanner@microchip.com>
New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
total of 10 main clocks that need to be saved for ULP0 mode.
Add mck_count member to at91_pm_data, this will be used to determine
how many mcks need to be saved. In the mck_count member will also make
sure that no unnecessary clock settings are written during
mck_ps_restore.
Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
power modes.
Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
arch/arm/mach-at91/pm.c | 19 +++++-
arch/arm/mach-at91/pm.h | 1 +
arch/arm/mach-at91/pm_data-offsets.c | 2 +
arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
4 files changed, 110 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 55cab31ce1ecb..50bada544eede 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -1337,6 +1337,7 @@ struct pmc_info {
unsigned long uhp_udp_mask;
unsigned long mckr;
unsigned long version;
+ unsigned long mck_count;
};
static const struct pmc_info pmc_infos[] __initconst = {
@@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
.uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
.mckr = 0x30,
.version = AT91_PMC_V1,
+ .mck_count = 1,
},
{
.uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
.mckr = 0x30,
.version = AT91_PMC_V1,
+ .mck_count = 1,
},
{
.uhp_udp_mask = AT91SAM926x_PMC_UHP,
.mckr = 0x30,
.version = AT91_PMC_V1,
+ .mck_count = 1,
},
{ .uhp_udp_mask = 0,
.mckr = 0x30,
.version = AT91_PMC_V1,
+ .mck_count = 1,
},
{
.uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
.mckr = 0x28,
.version = AT91_PMC_V2,
+ .mck_count = 1,
},
{
.mckr = 0x28,
.version = AT91_PMC_V2,
+ .mck_count = 5,
+ },
+ {
+ .uhp_udp_mask = AT91SAM926x_PMC_UHP,
+ .mckr = 0x28,
+ .version = AT91_PMC_V2,
+ .mck_count = 10,
},
};
@@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
{ .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
{ .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
{ .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
- { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
+ { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
{ .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
{ /* sentinel */ },
};
@@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
soc_pm.data.pmc_mckr_offset = pmc->mckr;
soc_pm.data.pmc_version = pmc->version;
+ soc_pm.data.pmc_mck_count = pmc->mck_count;
if (pm_idle)
arm_pm_idle = pm_idle;
@@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
};
static const u32 iomaps[] __initconst = {
- [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
+ [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
+ AT91_PM_IOMAP(SHDWC),
[AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
AT91_PM_IOMAP(SHDWC) |
AT91_PM_IOMAP(ETHC),
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 53bdc9000e447..ccde9c8728c27 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -39,6 +39,7 @@ struct at91_pm_data {
unsigned int suspend_mode;
unsigned int pmc_mckr_offset;
unsigned int pmc_version;
+ unsigned int pmc_mck_count;
};
#endif
diff --git a/arch/arm/mach-at91/pm_data-offsets.c b/arch/arm/mach-at91/pm_data-offsets.c
index 40bd4e8fe40a5..59a4838038381 100644
--- a/arch/arm/mach-at91/pm_data-offsets.c
+++ b/arch/arm/mach-at91/pm_data-offsets.c
@@ -18,6 +18,8 @@ int main(void)
pmc_mckr_offset));
DEFINE(PM_DATA_PMC_VERSION, offsetof(struct at91_pm_data,
pmc_version));
+ DEFINE(PM_DATA_PMC_MCK_COUNT, offsetof(struct at91_pm_data,
+ pmc_mck_count));
return 0;
}
diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index e5869cca5e791..2bbcbb26adb28 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -814,17 +814,19 @@ sr_dis_exit:
.endm
/**
- * at91_mckx_ps_enable: save MCK1..4 settings and switch it to main clock
+ * at91_mckx_ps_enable: save MCK settings and switch it to main clock
*
- * Side effects: overwrites tmp1, tmp2
+ * Side effects: overwrites tmp1, tmp2, tmp3
*/
.macro at91_mckx_ps_enable
#ifdef CONFIG_SOC_SAMA7
ldr pmc, .pmc_base
+ ldr tmp3, .mck_count
- /* There are 4 MCKs we need to handle: MCK1..4 */
+ /* Start at MCK1 and go until MCK_count */
mov tmp1, #1
-e_loop: cmp tmp1, #5
+e_loop:
+ cmp tmp1, tmp3
beq e_done
/* Write MCK ID to retrieve the settings. */
@@ -850,7 +852,37 @@ e_save_mck3:
b e_ps
e_save_mck4:
+ cmp tmp1, #4
+ bne e_save_mck5
str tmp2, .saved_mck4
+ b e_ps
+
+e_save_mck5:
+ cmp tmp1, #5
+ bne e_save_mck6
+ str tmp2, .saved_mck5
+ b e_ps
+
+e_save_mck6:
+ cmp tmp1, #6
+ bne e_save_mck7
+ str tmp2, .saved_mck6
+ b e_ps
+
+e_save_mck7:
+ cmp tmp1, #7
+ bne e_save_mck8
+ str tmp2, .saved_mck7
+ b e_ps
+
+e_save_mck8:
+ cmp tmp1, #8
+ bne e_save_mck9
+ str tmp2, .saved_mck8
+ b e_ps
+
+e_save_mck9:
+ str tmp2, .saved_mck9
e_ps:
/* Use CSS=MAINCK and DIV=1. */
@@ -870,17 +902,19 @@ e_done:
.endm
/**
- * at91_mckx_ps_restore: restore MCK1..4 settings
+ * at91_mckx_ps_restore: restore MCKx settings
*
* Side effects: overwrites tmp1, tmp2
*/
.macro at91_mckx_ps_restore
#ifdef CONFIG_SOC_SAMA7
ldr pmc, .pmc_base
+ ldr tmp2, .mck_count
- /* There are 4 MCKs we need to handle: MCK1..4 */
+ /* Start from MCK1 and go up to MCK_count */
mov tmp1, #1
-r_loop: cmp tmp1, #5
+r_loop:
+ cmp tmp1, tmp2
beq r_done
r_save_mck1:
@@ -902,7 +936,37 @@ r_save_mck3:
b r_ps
r_save_mck4:
+ cmp tmp1, #4
+ bne r_save_mck5
ldr tmp2, .saved_mck4
+ b r_ps
+
+r_save_mck5:
+ cmp tmp1, #5
+ bne r_save_mck6
+ ldr tmp2, .saved_mck5
+ b r_ps
+
+r_save_mck6:
+ cmp tmp1, #6
+ bne r_save_mck7
+ ldr tmp2, .saved_mck6
+ b r_ps
+
+r_save_mck7:
+ cmp tmp1, #7
+ bne r_save_mck8
+ ldr tmp2, .saved_mck7
+ b r_ps
+
+r_save_mck8:
+ cmp tmp1, #8
+ bne r_save_mck9
+ ldr tmp2, .saved_mck8
+ b r_ps
+
+r_save_mck9:
+ ldr tmp2, .saved_mck9
r_ps:
/* Write MCK ID to retrieve the settings. */
@@ -921,6 +985,7 @@ r_ps:
wait_mckrdy tmp1
add tmp1, tmp1, #1
+ ldr tmp2, .mck_count
b r_loop
r_done:
#endif
@@ -1045,6 +1110,10 @@ ENTRY(at91_pm_suspend_in_sram)
str tmp1, .memtype
ldr tmp1, [r0, #PM_DATA_MODE]
str tmp1, .pm_mode
+#ifdef CONFIG_SOC_SAMA7
+ ldr tmp1, [r0, #PM_DATA_PMC_MCK_COUNT]
+ str tmp1, .mck_count
+#endif
/*
* ldrne below are here to preload their address in the TLB as access
@@ -1132,6 +1201,10 @@ ENDPROC(at91_pm_suspend_in_sram)
.word 0
.pmc_version:
.word 0
+#ifdef CONFIG_SOC_SAMA7
+.mck_count:
+ .word 0
+#endif
.saved_mckr:
.word 0
.saved_pllar:
@@ -1155,6 +1228,16 @@ ENDPROC(at91_pm_suspend_in_sram)
.word 0
.saved_mck4:
.word 0
+.saved_mck5:
+ .word 0
+.saved_mck6:
+ .word 0
+.saved_mck7:
+ .word 0
+.saved_mck8:
+ .word 0
+.saved_mck9:
+ .word 0
#endif
ENTRY(at91_pm_suspend_in_sram_sz)
--
2.43.0Hi, Ryan,
On 10.02.2025 23:13, Ryan.Wanner@microchip.com wrote:
> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>
> New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
> total of 10 main clocks that need to be saved for ULP0 mode.
Isn't 9 the total number of MCKs that are handled in the last/first phase
of suspend/resume?
Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.
>
> Add mck_count member to at91_pm_data, this will be used to determine
> how many mcks need to be saved. In the mck_count member will also make
> sure that no unnecessary clock settings are written during
> mck_ps_restore.
>
> Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
> power modes.
Can you explain why this clear need to be done? The commit message should
answer to the "what?" and "why?" questions.
>
> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
> arch/arm/mach-at91/pm.c | 19 +++++-
> arch/arm/mach-at91/pm.h | 1 +
> arch/arm/mach-at91/pm_data-offsets.c | 2 +
> arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
> 4 files changed, 110 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index 55cab31ce1ecb..50bada544eede 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -1337,6 +1337,7 @@ struct pmc_info {
> unsigned long uhp_udp_mask;
> unsigned long mckr;
> unsigned long version;
> + unsigned long mck_count;> };
>
> static const struct pmc_info pmc_infos[] __initconst = {
> @@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
> .uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
> .mckr = 0x30,
> .version = AT91_PMC_V1,
> + .mck_count = 1,
As this member is used only for SAMA7 SoCs I would drop it here and above
(where initialized with 1).
> },
>
> {
> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
> .mckr = 0x30,
> .version = AT91_PMC_V1,
> + .mck_count = 1,
> },
> {
> .uhp_udp_mask = AT91SAM926x_PMC_UHP,
> .mckr = 0x30,
> .version = AT91_PMC_V1,
> + .mck_count = 1,
> },
> { .uhp_udp_mask = 0,
> .mckr = 0x30,
> .version = AT91_PMC_V1,
> + .mck_count = 1,
> },
> {
> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
> .mckr = 0x28,
> .version = AT91_PMC_V2,
> + .mck_count = 1,
> },
> {
> .mckr = 0x28,
> .version = AT91_PMC_V2,
> + .mck_count = 5,
I'm not sure mck_count is a good name when used like proposed in this
patch. We know that only 4 MCKs need to be handled for SAMA7G5 and 9 for
SAMA7D65.
Maybe, better change it here to 4 (.mck_count = 4) and to 9 above
(.mck_count = 9) and adjust properly the assembly macros (see below)? What
do you think?
> + },
> + {
> + .uhp_udp_mask = AT91SAM926x_PMC_UHP,
> + .mckr = 0x28,
> + .version = AT91_PMC_V2,
> + .mck_count = 10,
> },
>
> };
> @@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
> { .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
> { .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
> { .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
> - { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
> + { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
> { .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
> { /* sentinel */ },
> };
> @@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
> soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
> soc_pm.data.pmc_mckr_offset = pmc->mckr;
> soc_pm.data.pmc_version = pmc->version;
> + soc_pm.data.pmc_mck_count = pmc->mck_count;
>
> if (pm_idle)
> arm_pm_idle = pm_idle;
> @@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
> AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
> };
> static const u32 iomaps[] __initconst = {
> - [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
> + [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
> + AT91_PM_IOMAP(SHDWC),
In theory, as the wakeup sources can also resumes the system from standby
(WFI), the shdwc should be mapped for standby, too. Unless I'm wrong and
the wakeup sources covered by the SHDWC_SR register don't apply to standby
(WFI).
> [AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
> AT91_PM_IOMAP(SHDWC) |
> AT91_PM_IOMAP(ETHC),
> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
> index 53bdc9000e447..ccde9c8728c27 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -39,6 +39,7 @@ struct at91_pm_data {
> unsigned int suspend_mode;
> unsigned int pmc_mckr_offset;
> unsigned int pmc_version;
> + unsigned int pmc_mck_count;
> };
> #endif
>
> diff --git a/arch/arm/mach-at91/pm_data-offsets.c b/arch/arm/mach-at91/pm_data-offsets.c
> index 40bd4e8fe40a5..59a4838038381 100644
> --- a/arch/arm/mach-at91/pm_data-offsets.c
> +++ b/arch/arm/mach-at91/pm_data-offsets.c
> @@ -18,6 +18,8 @@ int main(void)
> pmc_mckr_offset));
> DEFINE(PM_DATA_PMC_VERSION, offsetof(struct at91_pm_data,
> pmc_version));
> + DEFINE(PM_DATA_PMC_MCK_COUNT, offsetof(struct at91_pm_data,
> + pmc_mck_count));
>
> return 0;
> }
> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
> index e5869cca5e791..2bbcbb26adb28 100644
> --- a/arch/arm/mach-at91/pm_suspend.S
> +++ b/arch/arm/mach-at91/pm_suspend.S
> @@ -814,17 +814,19 @@ sr_dis_exit:
> .endm
>
> /**
> - * at91_mckx_ps_enable: save MCK1..4 settings and switch it to main clock
> + * at91_mckx_ps_enable: save MCK settings and switch it to main clock
> *
> - * Side effects: overwrites tmp1, tmp2
> + * Side effects: overwrites tmp1, tmp2, tmp3
> */
> .macro at91_mckx_ps_enable
> #ifdef CONFIG_SOC_SAMA7
> ldr pmc, .pmc_base
> + ldr tmp3, .mck_count
>
> - /* There are 4 MCKs we need to handle: MCK1..4 */
> + /* Start at MCK1 and go until MCK_count */
s/MCK_count/mck_count to align with the mck_count above.
> mov tmp1, #1
> -e_loop: cmp tmp1, #5
> +e_loop:
> + cmp tmp1, tmp3
> beq e_done
If providing mck_count = 4 (for SAMA7G5) and mck_count = 9 (for SAMA7D65)
you can change this to:
bqt e_done
>
> /* Write MCK ID to retrieve the settings. */
> @@ -850,7 +852,37 @@ e_save_mck3:
> b e_ps
>
> e_save_mck4:
> + cmp tmp1, #4
> + bne e_save_mck5
> str tmp2, .saved_mck4
> + b e_ps
> +
> +e_save_mck5:
> + cmp tmp1, #5
> + bne e_save_mck6
> + str tmp2, .saved_mck5
> + b e_ps
> +
> +e_save_mck6:
> + cmp tmp1, #6
> + bne e_save_mck7
> + str tmp2, .saved_mck6
> + b e_ps
> +
> +e_save_mck7:
> + cmp tmp1, #7
> + bne e_save_mck8
> + str tmp2, .saved_mck7
> + b e_ps
> +
> +e_save_mck8:
> + cmp tmp1, #8
> + bne e_save_mck9
> + str tmp2, .saved_mck8
> + b e_ps
> +
> +e_save_mck9:
> + str tmp2, .saved_mck9
>
> e_ps:
> /* Use CSS=MAINCK and DIV=1. */
> @@ -870,17 +902,19 @@ e_done:
> .endm
>
> /**
> - * at91_mckx_ps_restore: restore MCK1..4 settings
> + * at91_mckx_ps_restore: restore MCKx settings
s/MCKx/MCK to align with the description from at91_mckx_ps_enable
> *
> * Side effects: overwrites tmp1, tmp2
> */
> .macro at91_mckx_ps_restore
> #ifdef CONFIG_SOC_SAMA7
> ldr pmc, .pmc_base
> + ldr tmp2, .mck_count
>
> - /* There are 4 MCKs we need to handle: MCK1..4 */
> + /* Start from MCK1 and go up to MCK_count */
> mov tmp1, #1
> -r_loop: cmp tmp1, #5
> +r_loop:
> + cmp tmp1, tmp2
> beq r_done
Same here:
bgt r_done
should be enough if providing mck_count = 4 or 9
>
> r_save_mck1:
> @@ -902,7 +936,37 @@ r_save_mck3:
> b r_ps
>
> r_save_mck4:
> + cmp tmp1, #4
> + bne r_save_mck5
> ldr tmp2, .saved_mck4
> + b r_ps
> +
> +r_save_mck5:
> + cmp tmp1, #5
> + bne r_save_mck6
> + ldr tmp2, .saved_mck5
> + b r_ps
> +
> +r_save_mck6:
> + cmp tmp1, #6
> + bne r_save_mck7
> + ldr tmp2, .saved_mck6
> + b r_ps
> +
> +r_save_mck7:
> + cmp tmp1, #7
> + bne r_save_mck8
> + ldr tmp2, .saved_mck7
> + b r_ps
> +
> +r_save_mck8:
> + cmp tmp1, #8
> + bne r_save_mck9
> + ldr tmp2, .saved_mck8
> + b r_ps
> +
> +r_save_mck9:
> + ldr tmp2, .saved_mck9
>
> r_ps:
> /* Write MCK ID to retrieve the settings. */
> @@ -921,6 +985,7 @@ r_ps:
> wait_mckrdy tmp1
>
> add tmp1, tmp1, #1
> + ldr tmp2, .mck_count
Or you can add tmp4 for this
> b r_loop
> r_done:
> #endif
> @@ -1045,6 +1110,10 @@ ENTRY(at91_pm_suspend_in_sram)
> str tmp1, .memtype
> ldr tmp1, [r0, #PM_DATA_MODE]
> str tmp1, .pm_mode
> +#ifdef CONFIG_SOC_SAMA7
> + ldr tmp1, [r0, #PM_DATA_PMC_MCK_COUNT]
> + str tmp1, .mck_count
> +#endif
>
> /*
> * ldrne below are here to preload their address in the TLB as access
> @@ -1132,6 +1201,10 @@ ENDPROC(at91_pm_suspend_in_sram)
> .word 0
> .pmc_version:
> .word 0
> +#ifdef CONFIG_SOC_SAMA7
> +.mck_count:
> + .word 0
> +#endif
> .saved_mckr:
> .word 0
> .saved_pllar:
> @@ -1155,6 +1228,16 @@ ENDPROC(at91_pm_suspend_in_sram)
> .word 0
> .saved_mck4:
> .word 0
> +.saved_mck5:
> + .word 0
> +.saved_mck6:
> + .word 0
> +.saved_mck7:
> + .word 0
> +.saved_mck8:
> + .word 0
> +.saved_mck9:
> + .word 0
> #endif
>
> ENTRY(at91_pm_suspend_in_sram_sz)
On 2/13/25 01:20, Claudiu Beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Ryan,
>
>
> On 10.02.2025 23:13, Ryan.Wanner@microchip.com wrote:
>> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>>
>> New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
>> total of 10 main clocks that need to be saved for ULP0 mode.
>
> Isn't 9 the total number of MCKs that are handled in the last/first phase
> of suspend/resume?
Yes I was including 10 to match the indexing in the mck_count variable.
Since bgt instruction was suggested I will correct this to reflect the
true behavior of the change.
>
> Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.
>
>>
>> Add mck_count member to at91_pm_data, this will be used to determine
>> how many mcks need to be saved. In the mck_count member will also make
>> sure that no unnecessary clock settings are written during
>> mck_ps_restore.
>>
>> Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
>> power modes.
>
> Can you explain why this clear need to be done? The commit message should
> answer to the "what?" and "why?" questions.
>
>>
>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> ---
>> arch/arm/mach-at91/pm.c | 19 +++++-
>> arch/arm/mach-at91/pm.h | 1 +
>> arch/arm/mach-at91/pm_data-offsets.c | 2 +
>> arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
>> 4 files changed, 110 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index 55cab31ce1ecb..50bada544eede 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -1337,6 +1337,7 @@ struct pmc_info {
>> unsigned long uhp_udp_mask;
>> unsigned long mckr;
>> unsigned long version;
>> + unsigned long mck_count;> };
>>
>> static const struct pmc_info pmc_infos[] __initconst = {
>> @@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
>> .uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
>> .mckr = 0x30,
>> .version = AT91_PMC_V1,
>> + .mck_count = 1,
>
> As this member is used only for SAMA7 SoCs I would drop it here and above
> (where initialized with 1).
>
>> },
>>
>> {
>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>> .mckr = 0x30,
>> .version = AT91_PMC_V1,
>> + .mck_count = 1,
>> },
>> {
>> .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>> .mckr = 0x30,
>> .version = AT91_PMC_V1,
>> + .mck_count = 1,
>> },
>> { .uhp_udp_mask = 0,
>> .mckr = 0x30,
>> .version = AT91_PMC_V1,
>> + .mck_count = 1,
>> },
>> {
>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>> .mckr = 0x28,
>> .version = AT91_PMC_V2,
>> + .mck_count = 1,
>> },
>> {
>> .mckr = 0x28,
>> .version = AT91_PMC_V2,
>> + .mck_count = 5,
>
> I'm not sure mck_count is a good name when used like proposed in this
> patch. We know that only 4 MCKs need to be handled for SAMA7G5 and 9 for
> SAMA7D65.
>
> Maybe, better change it here to 4 (.mck_count = 4) and to 9 above
> (.mck_count = 9) and adjust properly the assembly macros (see below)? What
> do you think?
Yes I think this is better and cleaner to read. Should this mck_count
match the pmc_mck_count variable name? Or should this be more
descriptive or would mcks be sufficient.
>
>> + },
>> + {
>> + .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>> + .mckr = 0x28,
>> + .version = AT91_PMC_V2,
>> + .mck_count = 10,
>> },
>>
>> };
>> @@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
>> { .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
>> { .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
>> { .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
>> - { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
>> + { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
>> { .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
>> { /* sentinel */ },
>> };
>> @@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
>> soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
>> soc_pm.data.pmc_mckr_offset = pmc->mckr;
>> soc_pm.data.pmc_version = pmc->version;
>> + soc_pm.data.pmc_mck_count = pmc->mck_count;
>>
>> if (pm_idle)
>> arm_pm_idle = pm_idle;
>> @@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
>> AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
>> };
>> static const u32 iomaps[] __initconst = {
>> - [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
>> + [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
>> + AT91_PM_IOMAP(SHDWC),
>
> In theory, as the wakeup sources can also resumes the system from standby
> (WFI), the shdwc should be mapped for standby, too. Unless I'm wrong and
> the wakeup sources covered by the SHDWC_SR register don't apply to standby
> (WFI).
The device can wake up from an RTT or RTC alarm event on both the
standby power mode and the ULP0 power mode, since the RTT/RTC are
included in the SHDWC_SR I think it is safe to have this.
If I understand what you are asking correctly.
>
>
>> [AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
>> AT91_PM_IOMAP(SHDWC) |
>> AT91_PM_IOMAP(ETHC),
>> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
>> index 53bdc9000e447..ccde9c8728c27 100644
>> --- a/arch/arm/mach-at91/pm.h
>> +++ b/arch/arm/mach-at91/pm.h
>> @@ -39,6 +39,7 @@ struct at91_pm_data {
>> unsigned int suspend_mode;
>> unsigned int pmc_mckr_offset;
>> unsigned int pmc_version;
>> + unsigned int pmc_mck_count;
>> };
>> #endif
>>
>> diff --git a/arch/arm/mach-at91/pm_data-offsets.c b/arch/arm/mach-at91/pm_data-offsets.c
>> index 40bd4e8fe40a5..59a4838038381 100644
>> --- a/arch/arm/mach-at91/pm_data-offsets.c
>> +++ b/arch/arm/mach-at91/pm_data-offsets.c
>> @@ -18,6 +18,8 @@ int main(void)
>> pmc_mckr_offset));
>> DEFINE(PM_DATA_PMC_VERSION, offsetof(struct at91_pm_data,
>> pmc_version));
>> + DEFINE(PM_DATA_PMC_MCK_COUNT, offsetof(struct at91_pm_data,
>> + pmc_mck_count));
>>
>> return 0;
>> }
>> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
>> index e5869cca5e791..2bbcbb26adb28 100644
>> --- a/arch/arm/mach-at91/pm_suspend.S
>> +++ b/arch/arm/mach-at91/pm_suspend.S
>> @@ -814,17 +814,19 @@ sr_dis_exit:
>> .endm
>>
>> /**
>> - * at91_mckx_ps_enable: save MCK1..4 settings and switch it to main clock
>> + * at91_mckx_ps_enable: save MCK settings and switch it to main clock
>> *
>> - * Side effects: overwrites tmp1, tmp2
>> + * Side effects: overwrites tmp1, tmp2, tmp3
>> */
>> .macro at91_mckx_ps_enable
>> #ifdef CONFIG_SOC_SAMA7
>> ldr pmc, .pmc_base
>> + ldr tmp3, .mck_count
>>
>> - /* There are 4 MCKs we need to handle: MCK1..4 */
>> + /* Start at MCK1 and go until MCK_count */
>
> s/MCK_count/mck_count to align with the mck_count above.
>
>> mov tmp1, #1
>> -e_loop: cmp tmp1, #5
>> +e_loop:
>> + cmp tmp1, tmp3
>> beq e_done
>
> If providing mck_count = 4 (for SAMA7G5) and mck_count = 9 (for SAMA7D65)
> you can change this to:
>
> bqt e_done
>
>>
>> /* Write MCK ID to retrieve the settings. */
>> @@ -850,7 +852,37 @@ e_save_mck3:
>> b e_ps
>>
>> e_save_mck4:
>> + cmp tmp1, #4
>> + bne e_save_mck5
>> str tmp2, .saved_mck4
>> + b e_ps
>> +
>> +e_save_mck5:
>> + cmp tmp1, #5
>> + bne e_save_mck6
>> + str tmp2, .saved_mck5
>> + b e_ps
>> +
>> +e_save_mck6:
>> + cmp tmp1, #6
>> + bne e_save_mck7
>> + str tmp2, .saved_mck6
>> + b e_ps
>> +
>> +e_save_mck7:
>> + cmp tmp1, #7
>> + bne e_save_mck8
>> + str tmp2, .saved_mck7
>> + b e_ps
>> +
>> +e_save_mck8:
>> + cmp tmp1, #8
>> + bne e_save_mck9
>> + str tmp2, .saved_mck8
>> + b e_ps
>> +
>> +e_save_mck9:
>> + str tmp2, .saved_mck9
>>
>> e_ps:
>> /* Use CSS=MAINCK and DIV=1. */
>> @@ -870,17 +902,19 @@ e_done:
>> .endm
>>
>> /**
>> - * at91_mckx_ps_restore: restore MCK1..4 settings
>> + * at91_mckx_ps_restore: restore MCKx settings
>
> s/MCKx/MCK to align with the description from at91_mckx_ps_enable
>
>> *
>> * Side effects: overwrites tmp1, tmp2
>> */
>> .macro at91_mckx_ps_restore
>> #ifdef CONFIG_SOC_SAMA7
>> ldr pmc, .pmc_base
>> + ldr tmp2, .mck_count
>>
>> - /* There are 4 MCKs we need to handle: MCK1..4 */
>> + /* Start from MCK1 and go up to MCK_count */
>> mov tmp1, #1
>> -r_loop: cmp tmp1, #5
>> +r_loop:
>> + cmp tmp1, tmp2
>> beq r_done
>
> Same here:
> bgt r_done
>
> should be enough if providing mck_count = 4 or 9
>
>>
>> r_save_mck1:
>> @@ -902,7 +936,37 @@ r_save_mck3:
>> b r_ps
>>
>> r_save_mck4:
>> + cmp tmp1, #4
>> + bne r_save_mck5
>> ldr tmp2, .saved_mck4
>> + b r_ps
>> +
>> +r_save_mck5:
>> + cmp tmp1, #5
>> + bne r_save_mck6
>> + ldr tmp2, .saved_mck5
>> + b r_ps
>> +
>> +r_save_mck6:
>> + cmp tmp1, #6
>> + bne r_save_mck7
>> + ldr tmp2, .saved_mck6
>> + b r_ps
>> +
>> +r_save_mck7:
>> + cmp tmp1, #7
>> + bne r_save_mck8
>> + ldr tmp2, .saved_mck7
>> + b r_ps
>> +
>> +r_save_mck8:
>> + cmp tmp1, #8
>> + bne r_save_mck9
>> + ldr tmp2, .saved_mck8
>> + b r_ps
>> +
>> +r_save_mck9:
>> + ldr tmp2, .saved_mck9
>>
>> r_ps:
>> /* Write MCK ID to retrieve the settings. */
>> @@ -921,6 +985,7 @@ r_ps:
>> wait_mckrdy tmp1
>>
>> add tmp1, tmp1, #1
>> + ldr tmp2, .mck_count
>
> Or you can add tmp4 for this
>
>> b r_loop
>> r_done:
>> #endif
>> @@ -1045,6 +1110,10 @@ ENTRY(at91_pm_suspend_in_sram)
>> str tmp1, .memtype
>> ldr tmp1, [r0, #PM_DATA_MODE]
>> str tmp1, .pm_mode
>> +#ifdef CONFIG_SOC_SAMA7
>> + ldr tmp1, [r0, #PM_DATA_PMC_MCK_COUNT]
>> + str tmp1, .mck_count
>> +#endif
>>
>> /*
>> * ldrne below are here to preload their address in the TLB as access
>> @@ -1132,6 +1201,10 @@ ENDPROC(at91_pm_suspend_in_sram)
>> .word 0
>> .pmc_version:
>> .word 0
>> +#ifdef CONFIG_SOC_SAMA7
>> +.mck_count:
>> + .word 0
>> +#endif
>> .saved_mckr:
>> .word 0
>> .saved_pllar:
>> @@ -1155,6 +1228,16 @@ ENDPROC(at91_pm_suspend_in_sram)
>> .word 0
>> .saved_mck4:
>> .word 0
>> +.saved_mck5:
>> + .word 0
>> +.saved_mck6:
>> + .word 0
>> +.saved_mck7:
>> + .word 0
>> +.saved_mck8:
>> + .word 0
>> +.saved_mck9:
>> + .word 0
>> #endif
>>
>> ENTRY(at91_pm_suspend_in_sram_sz)
>
Hi, Ryan,
On 14.02.2025 20:09, Ryan.Wanner@microchip.com wrote:
> On 2/13/25 01:20, Claudiu Beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi, Ryan,
>>
>>
>> On 10.02.2025 23:13, Ryan.Wanner@microchip.com wrote:
>>> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>
>>> New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
>>> total of 10 main clocks that need to be saved for ULP0 mode.
>>
>> Isn't 9 the total number of MCKs that are handled in the last/first phase
>> of suspend/resume?
> Yes I was including 10 to match the indexing in the mck_count variable.
> Since bgt instruction was suggested I will correct this to reflect the
> true behavior of the change.
>>
>> Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.
>>
>>>
>>> Add mck_count member to at91_pm_data, this will be used to determine
>>> how many mcks need to be saved. In the mck_count member will also make
>>> sure that no unnecessary clock settings are written during
>>> mck_ps_restore.
>>>
>>> Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
>>> power modes.
>>
>> Can you explain why this clear need to be done? The commit message should
>> answer to the "what?" and "why?" questions.
>>
>>>
>>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>> ---
>>> arch/arm/mach-at91/pm.c | 19 +++++-
>>> arch/arm/mach-at91/pm.h | 1 +
>>> arch/arm/mach-at91/pm_data-offsets.c | 2 +
>>> arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
>>> 4 files changed, 110 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>>> index 55cab31ce1ecb..50bada544eede 100644
>>> --- a/arch/arm/mach-at91/pm.c
>>> +++ b/arch/arm/mach-at91/pm.c
>>> @@ -1337,6 +1337,7 @@ struct pmc_info {
>>> unsigned long uhp_udp_mask;
>>> unsigned long mckr;
>>> unsigned long version;
>>> + unsigned long mck_count;> };
>>>
>>> static const struct pmc_info pmc_infos[] __initconst = {
>>> @@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
>>> .uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
>>> .mckr = 0x30,
>>> .version = AT91_PMC_V1,
>>> + .mck_count = 1,
>>
>> As this member is used only for SAMA7 SoCs I would drop it here and above
>> (where initialized with 1).
>>
>>> },
>>>
>>> {
>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>> .mckr = 0x30,
>>> .version = AT91_PMC_V1,
>>> + .mck_count = 1,
>>> },
>>> {
>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>> .mckr = 0x30,
>>> .version = AT91_PMC_V1,
>>> + .mck_count = 1,
>>> },
>>> { .uhp_udp_mask = 0,
>>> .mckr = 0x30,
>>> .version = AT91_PMC_V1,
>>> + .mck_count = 1,
>>> },
>>> {
>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>> .mckr = 0x28,
>>> .version = AT91_PMC_V2,
>>> + .mck_count = 1,
>>> },
>>> {
>>> .mckr = 0x28,
>>> .version = AT91_PMC_V2,
>>> + .mck_count = 5,
>>
>> I'm not sure mck_count is a good name when used like proposed in this
>> patch. We know that only 4 MCKs need to be handled for SAMA7G5 and 9 for
>> SAMA7D65.
>>
>> Maybe, better change it here to 4 (.mck_count = 4) and to 9 above
>> (.mck_count = 9) and adjust properly the assembly macros (see below)? What
>> do you think?
>
> Yes I think this is better and cleaner to read. Should this mck_count
> match the pmc_mck_count variable name? Or should this be more
> descriptive or would mcks be sufficient.
mck_count/mcks should be enough. These will be anyway in the context of
pmc_info.
>>
>>> + },
>>> + {
>>> + .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>> + .mckr = 0x28,
>>> + .version = AT91_PMC_V2,
>>> + .mck_count = 10,
>>> },
>>>
>>> };
>>> @@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
>>> { .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
>>> { .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
>>> { .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
>>> - { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
>>> + { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
>>> { .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
>>> { /* sentinel */ },
>>> };
>>> @@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
>>> soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
>>> soc_pm.data.pmc_mckr_offset = pmc->mckr;
>>> soc_pm.data.pmc_version = pmc->version;
>>> + soc_pm.data.pmc_mck_count = pmc->mck_count;
>>>
>>> if (pm_idle)
>>> arm_pm_idle = pm_idle;
>>> @@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
>>> AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
>>> };
>>> static const u32 iomaps[] __initconst = {
>>> - [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
>>> + [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
>>> + AT91_PM_IOMAP(SHDWC),
>>
>> In theory, as the wakeup sources can also resumes the system from standby
>> (WFI), the shdwc should be mapped for standby, too. Unless I'm wrong and
>> the wakeup sources covered by the SHDWC_SR register don't apply to standby
>> (WFI).
> The device can wake up from an RTT or RTC alarm event on both the
> standby power mode and the ULP0 power mode, since the RTT/RTC are
> included in the SHDWC_SR I think it is safe to have this.
> If I understand what you are asking correctly.
I was asking if the SHDWC should also be mapped for standby like:
static const u32 iomaps[] __initconst = {
[AT91_PM_STANDBY] = AT91_PM_IOMAP(SHDWC) |
[AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
AT91_PM_IOMAP(SHDWC),
[AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
AT91_PM_IOMAP(SHDWC) |
AT91_PM_IOMAP(ETHC),
[AT91_PM_BACKUP] = AT91_PM_IOMAP(SFRBU) |
AT91_PM_IOMAP(SHDWC),
};
>>
>>
>>> [AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
>>> AT91_PM_IOMAP(SHDWC) |
>>> AT91_PM_IOMAP(ETHC),
>>> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
>>> index 53bdc9000e447..ccde9c8728c27 100644
>>> --- a/arch/arm/mach-at91/pm.h
>>> +++ b/arch/arm/mach-at91/pm.h
>>> @@ -39,6 +39,7 @@ struct at91_pm_data {
>>> unsigned int suspend_mode;
>>> unsigned int pmc_mckr_offset;
>>> unsigned int pmc_version;
>>> + unsigned int pmc_mck_count;
>>> };
>>> #endif
>>>
>>> diff --git a/arch/arm/mach-at91/pm_data-offsets.c b/arch/arm/mach-at91/pm_data-offsets.c
>>> index 40bd4e8fe40a5..59a4838038381 100644
>>> --- a/arch/arm/mach-at91/pm_data-offsets.c
>>> +++ b/arch/arm/mach-at91/pm_data-offsets.c
>>> @@ -18,6 +18,8 @@ int main(void)
>>> pmc_mckr_offset));
>>> DEFINE(PM_DATA_PMC_VERSION, offsetof(struct at91_pm_data,
>>> pmc_version));
>>> + DEFINE(PM_DATA_PMC_MCK_COUNT, offsetof(struct at91_pm_data,
>>> + pmc_mck_count));
>>>
>>> return 0;
>>> }
>>> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
>>> index e5869cca5e791..2bbcbb26adb28 100644
>>> --- a/arch/arm/mach-at91/pm_suspend.S
>>> +++ b/arch/arm/mach-at91/pm_suspend.S
>>> @@ -814,17 +814,19 @@ sr_dis_exit:
>>> .endm
>>>
>>> /**
>>> - * at91_mckx_ps_enable: save MCK1..4 settings and switch it to main clock
>>> + * at91_mckx_ps_enable: save MCK settings and switch it to main clock
>>> *
>>> - * Side effects: overwrites tmp1, tmp2
>>> + * Side effects: overwrites tmp1, tmp2, tmp3
>>> */
>>> .macro at91_mckx_ps_enable
>>> #ifdef CONFIG_SOC_SAMA7
>>> ldr pmc, .pmc_base
>>> + ldr tmp3, .mck_count
>>>
>>> - /* There are 4 MCKs we need to handle: MCK1..4 */
>>> + /* Start at MCK1 and go until MCK_count */
>>
>> s/MCK_count/mck_count to align with the mck_count above.
>>
>>> mov tmp1, #1
>>> -e_loop: cmp tmp1, #5
>>> +e_loop:
>>> + cmp tmp1, tmp3
>>> beq e_done
>>
>> If providing mck_count = 4 (for SAMA7G5) and mck_count = 9 (for SAMA7D65)
>> you can change this to:
>>
>> bqt e_done
>>
>>>
>>> /* Write MCK ID to retrieve the settings. */
>>> @@ -850,7 +852,37 @@ e_save_mck3:
>>> b e_ps
>>>
>>> e_save_mck4:
>>> + cmp tmp1, #4
>>> + bne e_save_mck5
>>> str tmp2, .saved_mck4
>>> + b e_ps
>>> +
>>> +e_save_mck5:
>>> + cmp tmp1, #5
>>> + bne e_save_mck6
>>> + str tmp2, .saved_mck5
>>> + b e_ps
>>> +
>>> +e_save_mck6:
>>> + cmp tmp1, #6
>>> + bne e_save_mck7
>>> + str tmp2, .saved_mck6
>>> + b e_ps
>>> +
>>> +e_save_mck7:
>>> + cmp tmp1, #7
>>> + bne e_save_mck8
>>> + str tmp2, .saved_mck7
>>> + b e_ps
>>> +
>>> +e_save_mck8:
>>> + cmp tmp1, #8
>>> + bne e_save_mck9
>>> + str tmp2, .saved_mck8
>>> + b e_ps
>>> +
>>> +e_save_mck9:
>>> + str tmp2, .saved_mck9
>>>
>>> e_ps:
>>> /* Use CSS=MAINCK and DIV=1. */
>>> @@ -870,17 +902,19 @@ e_done:
>>> .endm
>>>
>>> /**
>>> - * at91_mckx_ps_restore: restore MCK1..4 settings
>>> + * at91_mckx_ps_restore: restore MCKx settings
>>
>> s/MCKx/MCK to align with the description from at91_mckx_ps_enable
>>
>>> *
>>> * Side effects: overwrites tmp1, tmp2
>>> */
>>> .macro at91_mckx_ps_restore
>>> #ifdef CONFIG_SOC_SAMA7
>>> ldr pmc, .pmc_base
>>> + ldr tmp2, .mck_count
>>>
>>> - /* There are 4 MCKs we need to handle: MCK1..4 */
>>> + /* Start from MCK1 and go up to MCK_count */
>>> mov tmp1, #1
>>> -r_loop: cmp tmp1, #5
>>> +r_loop:
>>> + cmp tmp1, tmp2
>>> beq r_done
>>
>> Same here:
>> bgt r_done
>>
>> should be enough if providing mck_count = 4 or 9
>>
>>>
>>> r_save_mck1:
>>> @@ -902,7 +936,37 @@ r_save_mck3:
>>> b r_ps
>>>
>>> r_save_mck4:
>>> + cmp tmp1, #4
>>> + bne r_save_mck5
>>> ldr tmp2, .saved_mck4
>>> + b r_ps
>>> +
>>> +r_save_mck5:
>>> + cmp tmp1, #5
>>> + bne r_save_mck6
>>> + ldr tmp2, .saved_mck5
>>> + b r_ps
>>> +
>>> +r_save_mck6:
>>> + cmp tmp1, #6
>>> + bne r_save_mck7
>>> + ldr tmp2, .saved_mck6
>>> + b r_ps
>>> +
>>> +r_save_mck7:
>>> + cmp tmp1, #7
>>> + bne r_save_mck8
>>> + ldr tmp2, .saved_mck7
>>> + b r_ps
>>> +
>>> +r_save_mck8:
>>> + cmp tmp1, #8
>>> + bne r_save_mck9
>>> + ldr tmp2, .saved_mck8
>>> + b r_ps
>>> +
>>> +r_save_mck9:
>>> + ldr tmp2, .saved_mck9
>>>
>>> r_ps:
>>> /* Write MCK ID to retrieve the settings. */
>>> @@ -921,6 +985,7 @@ r_ps:
>>> wait_mckrdy tmp1
>>>
>>> add tmp1, tmp1, #1
>>> + ldr tmp2, .mck_count
>>
>> Or you can add tmp4 for this
>>
>>> b r_loop
>>> r_done:
>>> #endif
>>> @@ -1045,6 +1110,10 @@ ENTRY(at91_pm_suspend_in_sram)
>>> str tmp1, .memtype
>>> ldr tmp1, [r0, #PM_DATA_MODE]
>>> str tmp1, .pm_mode
>>> +#ifdef CONFIG_SOC_SAMA7
>>> + ldr tmp1, [r0, #PM_DATA_PMC_MCK_COUNT]
>>> + str tmp1, .mck_count
>>> +#endif
>>>
>>> /*
>>> * ldrne below are here to preload their address in the TLB as access
>>> @@ -1132,6 +1201,10 @@ ENDPROC(at91_pm_suspend_in_sram)
>>> .word 0
>>> .pmc_version:
>>> .word 0
>>> +#ifdef CONFIG_SOC_SAMA7
>>> +.mck_count:
>>> + .word 0
>>> +#endif
>>> .saved_mckr:
>>> .word 0
>>> .saved_pllar:
>>> @@ -1155,6 +1228,16 @@ ENDPROC(at91_pm_suspend_in_sram)
>>> .word 0
>>> .saved_mck4:
>>> .word 0
>>> +.saved_mck5:
>>> + .word 0
>>> +.saved_mck6:
>>> + .word 0
>>> +.saved_mck7:
>>> + .word 0
>>> +.saved_mck8:
>>> + .word 0
>>> +.saved_mck9:
>>> + .word 0
>>> #endif
>>>
>>> ENTRY(at91_pm_suspend_in_sram_sz)
>>
>
On 2/17/25 00:18, Claudiu Beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Ryan,
>
> On 14.02.2025 20:09, Ryan.Wanner@microchip.com wrote:
>> On 2/13/25 01:20, Claudiu Beznea wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi, Ryan,
>>>
>>>
>>> On 10.02.2025 23:13, Ryan.Wanner@microchip.com wrote:
>>>> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>
>>>> New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
>>>> total of 10 main clocks that need to be saved for ULP0 mode.
>>>
>>> Isn't 9 the total number of MCKs that are handled in the last/first phase
>>> of suspend/resume?
>> Yes I was including 10 to match the indexing in the mck_count variable.
>> Since bgt instruction was suggested I will correct this to reflect the
>> true behavior of the change.
>>>
>>> Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.
>>>
>>>>
>>>> Add mck_count member to at91_pm_data, this will be used to determine
>>>> how many mcks need to be saved. In the mck_count member will also make
>>>> sure that no unnecessary clock settings are written during
>>>> mck_ps_restore.
>>>>
>>>> Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
>>>> power modes.
>>>
>>> Can you explain why this clear need to be done? The commit message should
>>> answer to the "what?" and "why?" questions.
>>>
>>>>
>>>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>>> ---
>>>> arch/arm/mach-at91/pm.c | 19 +++++-
>>>> arch/arm/mach-at91/pm.h | 1 +
>>>> arch/arm/mach-at91/pm_data-offsets.c | 2 +
>>>> arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
>>>> 4 files changed, 110 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>>>> index 55cab31ce1ecb..50bada544eede 100644
>>>> --- a/arch/arm/mach-at91/pm.c
>>>> +++ b/arch/arm/mach-at91/pm.c
>>>> @@ -1337,6 +1337,7 @@ struct pmc_info {
>>>> unsigned long uhp_udp_mask;
>>>> unsigned long mckr;
>>>> unsigned long version;
>>>> + unsigned long mck_count;> };
>>>>
>>>> static const struct pmc_info pmc_infos[] __initconst = {
>>>> @@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
>>>> .uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
>>>> .mckr = 0x30,
>>>> .version = AT91_PMC_V1,
>>>> + .mck_count = 1,
>>>
>>> As this member is used only for SAMA7 SoCs I would drop it here and above
>>> (where initialized with 1).
>>>
>>>> },
>>>>
>>>> {
>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>>> .mckr = 0x30,
>>>> .version = AT91_PMC_V1,
>>>> + .mck_count = 1,
>>>> },
>>>> {
>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>>> .mckr = 0x30,
>>>> .version = AT91_PMC_V1,
>>>> + .mck_count = 1,
>>>> },
>>>> { .uhp_udp_mask = 0,
>>>> .mckr = 0x30,
>>>> .version = AT91_PMC_V1,
>>>> + .mck_count = 1,
>>>> },
>>>> {
>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>>> .mckr = 0x28,
>>>> .version = AT91_PMC_V2,
>>>> + .mck_count = 1,
>>>> },
>>>> {
>>>> .mckr = 0x28,
>>>> .version = AT91_PMC_V2,
>>>> + .mck_count = 5,
>>>
>>> I'm not sure mck_count is a good name when used like proposed in this
>>> patch. We know that only 4 MCKs need to be handled for SAMA7G5 and 9 for
>>> SAMA7D65.
>>>
>>> Maybe, better change it here to 4 (.mck_count = 4) and to 9 above
>>> (.mck_count = 9) and adjust properly the assembly macros (see below)? What
>>> do you think?
>>
>> Yes I think this is better and cleaner to read. Should this mck_count
>> match the pmc_mck_count variable name? Or should this be more
>> descriptive or would mcks be sufficient.
>
> mck_count/mcks should be enough. These will be anyway in the context of
> pmc_info.
>
>>>
>>>> + },
>>>> + {
>>>> + .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>>> + .mckr = 0x28,
>>>> + .version = AT91_PMC_V2,
>>>> + .mck_count = 10,
>>>> },
>>>>
>>>> };
>>>> @@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
>>>> { .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
>>>> { .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
>>>> { .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
>>>> - { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
>>>> + { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
>>>> { .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
>>>> { /* sentinel */ },
>>>> };
>>>> @@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
>>>> soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
>>>> soc_pm.data.pmc_mckr_offset = pmc->mckr;
>>>> soc_pm.data.pmc_version = pmc->version;
>>>> + soc_pm.data.pmc_mck_count = pmc->mck_count;
>>>>
>>>> if (pm_idle)
>>>> arm_pm_idle = pm_idle;
>>>> @@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
>>>> AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
>>>> };
>>>> static const u32 iomaps[] __initconst = {
>>>> - [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
>>>> + [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
>>>> + AT91_PM_IOMAP(SHDWC),
>>>
>>> In theory, as the wakeup sources can also resumes the system from standby
>>> (WFI), the shdwc should be mapped for standby, too. Unless I'm wrong and
>>> the wakeup sources covered by the SHDWC_SR register don't apply to standby
>>> (WFI).
>> The device can wake up from an RTT or RTC alarm event on both the
>> standby power mode and the ULP0 power mode, since the RTT/RTC are
>> included in the SHDWC_SR I think it is safe to have this.
>> If I understand what you are asking correctly.
>
> I was asking if the SHDWC should also be mapped for standby like:
Ok I see. I have a better understanding now of wake up sources table
like you showed below. I think for readability of code I should not have
SHDWC set as ULP0 and STANDBY source because in at91_pm_config_ws()
SHDWC is only configured as a wake up source in ULP1 power mode.
So removing SHDWC from the ULP0 wake up source would reflect more
accurately what is configured as a wake up source in the code. What do
you think?
Best
Ryan
>
> static const u32 iomaps[] __initconst = {
>
> [AT91_PM_STANDBY] = AT91_PM_IOMAP(SHDWC) |
>
> [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
>
> AT91_PM_IOMAP(SHDWC),
>
> [AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
>
> AT91_PM_IOMAP(SHDWC) |
>
> AT91_PM_IOMAP(ETHC),
>
> [AT91_PM_BACKUP] = AT91_PM_IOMAP(SFRBU) |
>
> AT91_PM_IOMAP(SHDWC),
>
> };
>
>
>
>>>
>>>
>>>> [AT91_PM_ULP1] = AT91_PM_IOMAP(SFRBU) |
>>>> AT91_PM_IOMAP(SHDWC) |
>>>> AT91_PM_IOMAP(ETHC),
>>>> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
>>>> index 53bdc9000e447..ccde9c8728c27 100644
>>>> --- a/arch/arm/mach-at91/pm.h
>>>> +++ b/arch/arm/mach-at91/pm.h
>>>> @@ -39,6 +39,7 @@ struct at91_pm_data {
>>>> unsigned int suspend_mode;
>>>> unsigned int pmc_mckr_offset;
>>>> unsigned int pmc_version;
>>>> + unsigned int pmc_mck_count;
>>>> };
>>>> #endif
>>>>
>>>> diff --git a/arch/arm/mach-at91/pm_data-offsets.c b/arch/arm/mach-at91/pm_data-offsets.c
>>>> index 40bd4e8fe40a5..59a4838038381 100644
>>>> --- a/arch/arm/mach-at91/pm_data-offsets.c
>>>> +++ b/arch/arm/mach-at91/pm_data-offsets.c
>>>> @@ -18,6 +18,8 @@ int main(void)
>>>> pmc_mckr_offset));
>>>> DEFINE(PM_DATA_PMC_VERSION, offsetof(struct at91_pm_data,
>>>> pmc_version));
>>>> + DEFINE(PM_DATA_PMC_MCK_COUNT, offsetof(struct at91_pm_data,
>>>> + pmc_mck_count));
>>>>
>>>> return 0;
>>>> }
>>>> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
>>>> index e5869cca5e791..2bbcbb26adb28 100644
>>>> --- a/arch/arm/mach-at91/pm_suspend.S
>>>> +++ b/arch/arm/mach-at91/pm_suspend.S
>>>> @@ -814,17 +814,19 @@ sr_dis_exit:
>>>> .endm
>>>>
>>>> /**
>>>> - * at91_mckx_ps_enable: save MCK1..4 settings and switch it to main clock
>>>> + * at91_mckx_ps_enable: save MCK settings and switch it to main clock
>>>> *
>>>> - * Side effects: overwrites tmp1, tmp2
>>>> + * Side effects: overwrites tmp1, tmp2, tmp3
>>>> */
>>>> .macro at91_mckx_ps_enable
>>>> #ifdef CONFIG_SOC_SAMA7
>>>> ldr pmc, .pmc_base
>>>> + ldr tmp3, .mck_count
>>>>
>>>> - /* There are 4 MCKs we need to handle: MCK1..4 */
>>>> + /* Start at MCK1 and go until MCK_count */
>>>
>>> s/MCK_count/mck_count to align with the mck_count above.
>>>
>>>> mov tmp1, #1
>>>> -e_loop: cmp tmp1, #5
>>>> +e_loop:
>>>> + cmp tmp1, tmp3
>>>> beq e_done
>>>
>>> If providing mck_count = 4 (for SAMA7G5) and mck_count = 9 (for SAMA7D65)
>>> you can change this to:
>>>
>>> bqt e_done
>>>
>>>>
>>>> /* Write MCK ID to retrieve the settings. */
>>>> @@ -850,7 +852,37 @@ e_save_mck3:
>>>> b e_ps
>>>>
>>>> e_save_mck4:
>>>> + cmp tmp1, #4
>>>> + bne e_save_mck5
>>>> str tmp2, .saved_mck4
>>>> + b e_ps
>>>> +
>>>> +e_save_mck5:
>>>> + cmp tmp1, #5
>>>> + bne e_save_mck6
>>>> + str tmp2, .saved_mck5
>>>> + b e_ps
>>>> +
>>>> +e_save_mck6:
>>>> + cmp tmp1, #6
>>>> + bne e_save_mck7
>>>> + str tmp2, .saved_mck6
>>>> + b e_ps
>>>> +
>>>> +e_save_mck7:
>>>> + cmp tmp1, #7
>>>> + bne e_save_mck8
>>>> + str tmp2, .saved_mck7
>>>> + b e_ps
>>>> +
>>>> +e_save_mck8:
>>>> + cmp tmp1, #8
>>>> + bne e_save_mck9
>>>> + str tmp2, .saved_mck8
>>>> + b e_ps
>>>> +
>>>> +e_save_mck9:
>>>> + str tmp2, .saved_mck9
>>>>
>>>> e_ps:
>>>> /* Use CSS=MAINCK and DIV=1. */
>>>> @@ -870,17 +902,19 @@ e_done:
>>>> .endm
>>>>
>>>> /**
>>>> - * at91_mckx_ps_restore: restore MCK1..4 settings
>>>> + * at91_mckx_ps_restore: restore MCKx settings
>>>
>>> s/MCKx/MCK to align with the description from at91_mckx_ps_enable
>>>
>>>> *
>>>> * Side effects: overwrites tmp1, tmp2
>>>> */
>>>> .macro at91_mckx_ps_restore
>>>> #ifdef CONFIG_SOC_SAMA7
>>>> ldr pmc, .pmc_base
>>>> + ldr tmp2, .mck_count
>>>>
>>>> - /* There are 4 MCKs we need to handle: MCK1..4 */
>>>> + /* Start from MCK1 and go up to MCK_count */
>>>> mov tmp1, #1
>>>> -r_loop: cmp tmp1, #5
>>>> +r_loop:
>>>> + cmp tmp1, tmp2
>>>> beq r_done
>>>
>>> Same here:
>>> bgt r_done
>>>
>>> should be enough if providing mck_count = 4 or 9
>>>
>>>>
>>>> r_save_mck1:
>>>> @@ -902,7 +936,37 @@ r_save_mck3:
>>>> b r_ps
>>>>
>>>> r_save_mck4:
>>>> + cmp tmp1, #4
>>>> + bne r_save_mck5
>>>> ldr tmp2, .saved_mck4
>>>> + b r_ps
>>>> +
>>>> +r_save_mck5:
>>>> + cmp tmp1, #5
>>>> + bne r_save_mck6
>>>> + ldr tmp2, .saved_mck5
>>>> + b r_ps
>>>> +
>>>> +r_save_mck6:
>>>> + cmp tmp1, #6
>>>> + bne r_save_mck7
>>>> + ldr tmp2, .saved_mck6
>>>> + b r_ps
>>>> +
>>>> +r_save_mck7:
>>>> + cmp tmp1, #7
>>>> + bne r_save_mck8
>>>> + ldr tmp2, .saved_mck7
>>>> + b r_ps
>>>> +
>>>> +r_save_mck8:
>>>> + cmp tmp1, #8
>>>> + bne r_save_mck9
>>>> + ldr tmp2, .saved_mck8
>>>> + b r_ps
>>>> +
>>>> +r_save_mck9:
>>>> + ldr tmp2, .saved_mck9
>>>>
>>>> r_ps:
>>>> /* Write MCK ID to retrieve the settings. */
>>>> @@ -921,6 +985,7 @@ r_ps:
>>>> wait_mckrdy tmp1
>>>>
>>>> add tmp1, tmp1, #1
>>>> + ldr tmp2, .mck_count
>>>
>>> Or you can add tmp4 for this
>>>
>>>> b r_loop
>>>> r_done:
>>>> #endif
>>>> @@ -1045,6 +1110,10 @@ ENTRY(at91_pm_suspend_in_sram)
>>>> str tmp1, .memtype
>>>> ldr tmp1, [r0, #PM_DATA_MODE]
>>>> str tmp1, .pm_mode
>>>> +#ifdef CONFIG_SOC_SAMA7
>>>> + ldr tmp1, [r0, #PM_DATA_PMC_MCK_COUNT]
>>>> + str tmp1, .mck_count
>>>> +#endif
>>>>
>>>> /*
>>>> * ldrne below are here to preload their address in the TLB as access
>>>> @@ -1132,6 +1201,10 @@ ENDPROC(at91_pm_suspend_in_sram)
>>>> .word 0
>>>> .pmc_version:
>>>> .word 0
>>>> +#ifdef CONFIG_SOC_SAMA7
>>>> +.mck_count:
>>>> + .word 0
>>>> +#endif
>>>> .saved_mckr:
>>>> .word 0
>>>> .saved_pllar:
>>>> @@ -1155,6 +1228,16 @@ ENDPROC(at91_pm_suspend_in_sram)
>>>> .word 0
>>>> .saved_mck4:
>>>> .word 0
>>>> +.saved_mck5:
>>>> + .word 0
>>>> +.saved_mck6:
>>>> + .word 0
>>>> +.saved_mck7:
>>>> + .word 0
>>>> +.saved_mck8:
>>>> + .word 0
>>>> +.saved_mck9:
>>>> + .word 0
>>>> #endif
>>>>
>>>> ENTRY(at91_pm_suspend_in_sram_sz)
>>>
>>
>
On 19.02.2025 17:24, Ryan.Wanner@microchip.com wrote:
> On 2/17/25 00:18, Claudiu Beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi, Ryan,
>>
>> On 14.02.2025 20:09, Ryan.Wanner@microchip.com wrote:
>>> On 2/13/25 01:20, Claudiu Beznea wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hi, Ryan,
>>>>
>>>>
>>>> On 10.02.2025 23:13, Ryan.Wanner@microchip.com wrote:
>>>>> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>>
>>>>> New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
>>>>> total of 10 main clocks that need to be saved for ULP0 mode.
>>>>
>>>> Isn't 9 the total number of MCKs that are handled in the last/first phase
>>>> of suspend/resume?
>>> Yes I was including 10 to match the indexing in the mck_count variable.
>>> Since bgt instruction was suggested I will correct this to reflect the
>>> true behavior of the change.
>>>>
>>>> Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.
>>>>
>>>>>
>>>>> Add mck_count member to at91_pm_data, this will be used to determine
>>>>> how many mcks need to be saved. In the mck_count member will also make
>>>>> sure that no unnecessary clock settings are written during
>>>>> mck_ps_restore.
>>>>>
>>>>> Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
>>>>> power modes.
>>>>
>>>> Can you explain why this clear need to be done? The commit message should
>>>> answer to the "what?" and "why?" questions.
>>>>
>>>>>
>>>>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>>>> ---
>>>>> arch/arm/mach-at91/pm.c | 19 +++++-
>>>>> arch/arm/mach-at91/pm.h | 1 +
>>>>> arch/arm/mach-at91/pm_data-offsets.c | 2 +
>>>>> arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
>>>>> 4 files changed, 110 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>>>>> index 55cab31ce1ecb..50bada544eede 100644
>>>>> --- a/arch/arm/mach-at91/pm.c
>>>>> +++ b/arch/arm/mach-at91/pm.c
>>>>> @@ -1337,6 +1337,7 @@ struct pmc_info {
>>>>> unsigned long uhp_udp_mask;
>>>>> unsigned long mckr;
>>>>> unsigned long version;
>>>>> + unsigned long mck_count;> };
>>>>>
>>>>> static const struct pmc_info pmc_infos[] __initconst = {
>>>>> @@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
>>>>> .uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>
>>>> As this member is used only for SAMA7 SoCs I would drop it here and above
>>>> (where initialized with 1).
>>>>
>>>>> },
>>>>>
>>>>> {
>>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> {
>>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> { .uhp_udp_mask = 0,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> {
>>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>>>> .mckr = 0x28,
>>>>> .version = AT91_PMC_V2,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> {
>>>>> .mckr = 0x28,
>>>>> .version = AT91_PMC_V2,
>>>>> + .mck_count = 5,
>>>>
>>>> I'm not sure mck_count is a good name when used like proposed in this
>>>> patch. We know that only 4 MCKs need to be handled for SAMA7G5 and 9 for
>>>> SAMA7D65.
>>>>
>>>> Maybe, better change it here to 4 (.mck_count = 4) and to 9 above
>>>> (.mck_count = 9) and adjust properly the assembly macros (see below)? What
>>>> do you think?
>>>
>>> Yes I think this is better and cleaner to read. Should this mck_count
>>> match the pmc_mck_count variable name? Or should this be more
>>> descriptive or would mcks be sufficient.
>>
>> mck_count/mcks should be enough. These will be anyway in the context of
>> pmc_info.
>>
>>>>
>>>>> + },
>>>>> + {
>>>>> + .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>>>> + .mckr = 0x28,
>>>>> + .version = AT91_PMC_V2,
>>>>> + .mck_count = 10,
>>>>> },
>>>>>
>>>>> };
>>>>> @@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
>>>>> { .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
>>>>> { .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
>>>>> { .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
>>>>> - { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
>>>>> + { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
>>>>> { .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
>>>>> { /* sentinel */ },
>>>>> };
>>>>> @@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
>>>>> soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
>>>>> soc_pm.data.pmc_mckr_offset = pmc->mckr;
>>>>> soc_pm.data.pmc_version = pmc->version;
>>>>> + soc_pm.data.pmc_mck_count = pmc->mck_count;
>>>>>
>>>>> if (pm_idle)
>>>>> arm_pm_idle = pm_idle;
>>>>> @@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
>>>>> AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
>>>>> };
>>>>> static const u32 iomaps[] __initconst = {
>>>>> - [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
>>>>> + [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
>>>>> + AT91_PM_IOMAP(SHDWC),
>>>>
>>>> In theory, as the wakeup sources can also resumes the system from standby
>>>> (WFI), the shdwc should be mapped for standby, too. Unless I'm wrong and
>>>> the wakeup sources covered by the SHDWC_SR register don't apply to standby
>>>> (WFI).
>>> The device can wake up from an RTT or RTC alarm event on both the
>>> standby power mode and the ULP0 power mode, since the RTT/RTC are
>>> included in the SHDWC_SR I think it is safe to have this.
>>> If I understand what you are asking correctly.
>>
>> I was asking if the SHDWC should also be mapped for standby like:
> Ok I see. I have a better understanding now of wake up sources table
> like you showed below. I think for readability of code I should not have
> SHDWC set as ULP0 and STANDBY source because in at91_pm_config_ws()
> SHDWC is only configured as a wake up source in ULP1 power mode.
>
> So removing SHDWC from the ULP0 wake up source would reflect more
> accurately what is configured as a wake up source in the code. What do
> you think?
Sounds good.
Thank you,
Claudiu
© 2016 - 2026 Red Hat, Inc.