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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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