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@xxxxxxxxxxxxx wrote:
>> From: Ryan Wanner <Ryan.Wanner@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> Acked-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
>> ---
>> 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)
>
