Re: [PATCH v5 2/3] arm: exynos5: Add Suspend-to-RAM support for 5420

[Vikas Sajjan <vikas.sajjan@samsung.com>]

Hi Tomasz,


On Mon, Jun 30, 2014 at 11:27 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Vikas, Abhilash,
>
> Please see my comments inline.
>
> On 26.06.2014 13:12, Vikas Sajjan wrote:
>> From: Abhilash Kesavan <a.kesavan@samsung.com>
>>
>> Adds Suspend-to-RAM support for EXYNOS5420
>>
>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@samsung.com>
>> ---
>>  arch/arm/mach-exynos/pm.c |  150 ++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 134 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> index de61d48..bf8564a 100644
>> --- a/arch/arm/mach-exynos/pm.c
>> +++ b/arch/arm/mach-exynos/pm.c
>> @@ -38,9 +38,13 @@
>>  #include "regs-pmu.h"
>>  #include "regs-sys.h"
>>
>> +#define EXYNOS5420_CPU_STATE 0x28
>> +
>>  #define pmu_raw_writel(val, offset) \
>>               __raw_writel(val, pmu_base_addr + offset)
>>
>> +static int exynos5420_cpu_state;
>> +
>>  /**
>>   * struct exynos_wkup_irq - Exynos GIC to PMU IRQ mapping
>>   * @hwirq: Hardware IRQ signal of the GIC
>> @@ -64,6 +68,10 @@ static struct sleep_save exynos_core_save[] = {
>>       SAVE_ITEM(S5P_SROM_BC3),
>>  };
>>
>> +static struct sleep_save exynos5420_pmu_reg_save[] = {
>> +     SAVE_ITEM((void __iomem *)S5P_PMU_SPARE3),
>> +};
>
> Do you need a whole array for this single register?
>
>> +
>>  /*
>>   * GIC wake-up support
>>   */
>> @@ -86,7 +94,7 @@ static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
>>  {
>>       const struct exynos_wkup_irq *wkup_irq;
>>
>> -     if (soc_is_exynos5250())
>> +     if (soc_is_exynos5250() || soc_is_exynos5420())
>
> You should rework this to eliminate the need to use any SoC-specific
> checks. For example:
>
> 1) create a struct where any SoC/family specific data are stored, e.g.
>
>         struct exynos_pm_data {
>                 const struct exynos_wkup_irq *wkup_irq;
>                 /* arrays, flags, values, function pointers, etc. */
>         };
>
> 2) describe supported variants using instances of this struct, e.g.
>
>         static const struct exynos_pm_data exynos5250_pm_data {
>                 .wkup_irq = exynos5250_wkup_irq,
>                 /* ... */
>         };
>
>         static const struct exynos_pm_data exynos5420_pm_data {
>                 .wkup_irq = exynos5250_wkup_irq,
>                 /* ... */
>         };
>
> 3) put pointers to those structs in DT match table:
>
>         static const struct of_device_id exynos_pm_matches[] = {
>                 { .compatible = "samsung,exynos5250-pmu",
>                         .data = &exynos5250_pm_data },
>                 { .compatible = "samsung,exynos5420-pmu",
>                         .data = &exynos5420_pm_data },
>         };
>
> 4) find a matching node in DT and use the struct pointed by match data.
>
> Also certain checks could probably be replaced with non-SoC-specific
> checks, e.g. by CPU part checks.
>
>>               wkup_irq = exynos5250_wkup_irq;
>>       else
>>               wkup_irq = exynos4_wkup_irq;
>> @@ -250,7 +258,16 @@ static int exynos_cpu_suspend(unsigned long arg)
>>       outer_flush_all();
>>  #endif
>>
>> -     if (soc_is_exynos5250())
>> +     /*
>> +      * Clear sysram register for cpu state so that primary CPU does
>> +      * not enter low power start in U-Boot.
>> +      * This is specific to exynos5420 SoC only.
>> +      */
>> +     if (soc_is_exynos5420())
>> +             __raw_writel(0x0,
>> +                     sysram_base_addr + EXYNOS5420_CPU_STATE);
>> +
>> +     if (soc_is_exynos5250() || soc_is_exynos5420())
>
> This probably can be replaced with a check for Cortex A15 or A7.
>
>>               flush_cache_all();
>>
>>       /* issue the standby signal into the pm unit. */
>> @@ -276,6 +293,22 @@ static void exynos_pm_prepare(void)
>>               tmp = __raw_readl(pmu_base_addr + EXYNOS5_JPEG_MEM_OPTION);
>>               tmp &= ~EXYNOS5_OPTION_USE_RETENTION;
>>               pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
>> +     } else if (soc_is_exynos5420()) {
>> +             unsigned int i;

I have a question here.

I can come up with a function like exynos5420_pm_prepare(), which does
the same as the code under this if condition.

call it here like below,

static void exynos_pm_prepare(void)
{
 /* common to all SoCs */
              ......
                  .....
                    ......
 /* specific to given SoC */
 exynos_pm_data->pm_prepare();
                   ........
                .......
           .......
}

but at line   -286,6 +319,27 @@ static void exynos_pm_prepare(void)
also we have something specific to 5420.

how do we handle such cases. Because in future there may be new a SoC
which does SoC specific things at 3 different

location in same function  exynos_pm_prepare().

This will also apply to function like exynos_pm_suspend()
exynos_pm_resume() etc.,

>> +
>> +             for (i = 0; i < ARRAY_SIZE(exynos5420_pmu_reg_save); i++)
>> +                     exynos5420_pmu_reg_save[i].val =
>> +                             __raw_readl(pmu_base_addr +
>> +                             (unsigned int)exynos5420_pmu_reg_save[i].reg);
>> +             /*
>> +              * The cpu state needs to be saved and restored so that the
>> +              * secondary CPUs will enter low power start. Though the U-Boot
>> +              * is setting the cpu state with low power flag, the kernel
>> +              * needs to restore it back in case, the primary cpu fails to
>> +              * suspend for any reason.
>> +              */
>> +             exynos5420_cpu_state =
>> +             __raw_readl(sysram_base_addr + EXYNOS5420_CPU_STATE);
>>       }
>>
>>       /* Set value of power down register for sleep mode */
>> @@ -286,6 +319,27 @@ static void exynos_pm_prepare(void)
>>       /* ensure at least INFORM0 has the resume address */
>>
>>       pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
>> +
>> +     if (soc_is_exynos5420()) {
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5_ARM_L2_OPTION);
>> +             tmp &= ~EXYNOS5_USE_RETENTION;
>> +             pmu_raw_writel(tmp, EXYNOS5_ARM_L2_OPTION);
>> +
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5420_SFR_AXI_CGDIS1);
>> +             tmp |= EXYNOS5420_UFS;
>> +             pmu_raw_writel(tmp, EXYNOS5420_SFR_AXI_CGDIS1);
>> +
>> +             tmp = __raw_readl(pmu_base_addr +
>> +                             EXYNOS5420_ARM_COMMON_OPTION);
>> +             tmp &= ~EXYNOS5420_L2RSTDISABLE_VALUE;
>> +             pmu_raw_writel(tmp, EXYNOS5420_ARM_COMMON_OPTION);
>
> nit: A blank line here could improve readability.
>
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5420_FSYS2_OPTION);
>> +             tmp |= EXYNOS5420_EMULATION;
>> +             pmu_raw_writel(tmp, EXYNOS5420_FSYS2_OPTION);
>
> nit: A blank line here could improve readability.
>
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5420_PSGEN_OPTION);
>> +             tmp |= EXYNOS5420_EMULATION;
>> +             pmu_raw_writel(tmp, EXYNOS5420_PSGEN_OPTION);
>> +     }
>>  }
>>
>>  static void exynos_pm_central_suspend(void)
>> @@ -301,13 +355,24 @@ static void exynos_pm_central_suspend(void)
>>  static int exynos_pm_suspend(void)
>>  {
>>       unsigned long tmp;
>> +     unsigned int  this_cluster;
>
> nit: Two spaces between "int" and "this_cluster". Also it might be a
> better idea to use explicitly sized types with same size as register width.
>
>>
>>       exynos_pm_central_suspend();
>>
>>       /* Setting SEQ_OPTION register */
>>
>> -     tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
>> -     pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
>> +     if (soc_is_exynos5420()) {
>
> I believe this is not Exynos5420-specific, but rather specific to any
> multi-cluster Exynos SoC. I think there might be a way to check the
> number of clusters.
>
>> +             this_cluster = MPIDR_AFFINITY_LEVEL(read_cpuid_mpidr(), 1);
>> +             if (!this_cluster)
>> +                     pmu_raw_writel(EXYNOS5420_ARM_USE_STANDBY_WFI0,
>> +                                     S5P_CENTRAL_SEQ_OPTION);
>> +             else
>> +                     pmu_raw_writel(EXYNOS5420_KFC_USE_STANDBY_WFI0,
>> +                                     S5P_CENTRAL_SEQ_OPTION);
>
> By the way, is it even possible to boot this SoC using other CPU than
> first Cortex A15?
>
>> +     } else {
>> +             tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
>> +             pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
>> +     }
>>
>>       if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
>>               exynos_cpu_save_register();
>> @@ -340,6 +405,17 @@ static int exynos_pm_central_resume(void)
>>
>>  static void exynos_pm_resume(void)
>>  {
>> +     unsigned int tmp;
>> +
>> +     if (soc_is_exynos5420()) {
>> +             /* Restore the sysram cpu state register */
>> +             __raw_writel(exynos5420_cpu_state,
>> +             sysram_base_addr + EXYNOS5420_CPU_STATE);
>> +
>> +             pmu_raw_writel(EXYNOS5420_USE_STANDBY_WFI_ALL,
>> +                             S5P_CENTRAL_SEQ_OPTION);
>> +     }
>> +
>>       if (exynos_pm_central_resume())
>>               goto early_wakeup;
>>
>> @@ -347,18 +423,42 @@ static void exynos_pm_resume(void)
>>               exynos_cpu_restore_register();
>>
>>       /* For release retention */
>> +     if (soc_is_exynos5420()) {
>> +             pmu_raw_writel((1 << 28), EXYNOS_PAD_RET_DRAM_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS_PAD_RET_MAUDIO_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS_PAD_RET_JTAG_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_GPIO_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_UART_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_MMCA_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_MMCB_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_MMCC_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_HSI_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS_PAD_RET_EBIA_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS_PAD_RET_EBIB_OPTION);
>> +             pmu_raw_writel((1 << 28), EXYNOS5420_PAD_RET_SPI_OPTION);
>> +             pmu_raw_writel((1 << 28),
>> +                             EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION);
>
> This could be replaced with an array of registers that is specified in
> the exynos_pm_data struct. Also while at it, the (1 << 28) could be
> replaced with proper macro.
>
>> +     } else {
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_MAUDIO_OPTION);
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_GPIO_OPTION);
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_UART_OPTION);
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_MMCA_OPTION);
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_MMCB_OPTION);
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_EBIA_OPTION);
>> +             pmu_raw_writel((1 << 28), S5P_PAD_RET_EBIB_OPTION);
>> +     }
>>
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_MAUDIO_OPTION);
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_GPIO_OPTION);
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_UART_OPTION);
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_MMCA_OPTION);
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_MMCB_OPTION);
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_EBIA_OPTION);
>> -     pmu_raw_writel((1 << 28), S5P_PAD_RET_EBIB_OPTION);
>> -
>> -     if (soc_is_exynos5250())
>> +     if (soc_is_exynos5250()) {
>>               s3c_pm_do_restore(exynos5_sys_save,
>>                       ARRAY_SIZE(exynos5_sys_save));
>> +     } else if (soc_is_exynos5420()) {
>> +             unsigned int i;
>> +
>> +             for (i = 0; i < ARRAY_SIZE(exynos5420_pmu_reg_save); i++)
>> +                     pmu_raw_writel(
>> +                     (unsigned int)exynos5420_pmu_reg_save[i].val,
>> +                     (unsigned int)exynos5420_pmu_reg_save[i].reg);
>> +     }
>>
>>       s3c_pm_do_restore_core(exynos_core_save, ARRAY_SIZE(exynos_core_save));
>>
>> @@ -367,6 +467,18 @@ static void exynos_pm_resume(void)
>>
>>  early_wakeup:
>>
>> +     if (soc_is_exynos5420()) {
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5420_SFR_AXI_CGDIS1);
>> +             tmp &= ~EXYNOS5420_UFS;
>> +             pmu_raw_writel(tmp, EXYNOS5420_SFR_AXI_CGDIS1);
>
> nit: Spacing here would be nice.
>
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5420_FSYS2_OPTION);
>> +             tmp &= ~EXYNOS5420_EMULATION;
>> +             pmu_raw_writel(tmp, EXYNOS5420_FSYS2_OPTION);
>
> Ditto.
>
>> +             tmp = __raw_readl(pmu_base_addr + EXYNOS5420_PSGEN_OPTION);
>> +             tmp &= ~EXYNOS5420_EMULATION;
>> +             pmu_raw_writel(tmp, EXYNOS5420_PSGEN_OPTION);
>> +     }
>> +
>>       /* Clear SLEEP mode set in INFORM1 */
>>       pmu_raw_writel(0x0, S5P_INFORM1);
>>
>> @@ -483,9 +595,15 @@ void __init exynos_pm_init(void)
>>       gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
>>
>>       /* All wakeup disable */
>> -     tmp = __raw_readl(pmu_base_addr + S5P_WAKEUP_MASK);
>> -     tmp |= ((0xFF << 8) | (0x1F << 1));
>> -     pmu_raw_writel(tmp, S5P_WAKEUP_MASK);
>> +     if (soc_is_exynos5420()) {
>> +             tmp = __raw_readl(pmu_base_addr + S5P_WAKEUP_MASK);
>> +             tmp |= ((0x7F << 7) | (0x1F << 1));
>
> This mask could be also moved to the struct.
>
> Best regards,
> Tomasz