Re: [PATCH v2 12/15] ARM: at91: pm: Enable ULP0 for SAMA7D65

[<Ryan.Wanner@microchip.com>]

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)
>>>
>>
>