Hi, Ryan,

On 14.02.2025 20:09, Ryan.Wanner@microchip.com wrote:

On 2/13/25 01:20, Claudiu Beznea wrote:

quoted

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:

quoted

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.

quoted

Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.

quoted

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.

quoted

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

quoted

      },

      {
              .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.

quoted

quoted

+     },
+     {
+             .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),

        };

quoted

quoted

              [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.

quoted

      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

quoted

      /* 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

quoted

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

quoted

 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

quoted

      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)