Hi, Ryan,
On 14.02.2025 20:09, Ryan.Wanner@xxxxxxxxxxxxx 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@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.
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)
>>
>
