Re: [PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-08-06 Thread Vikas Sajjan
Hi Tomasz,

On Wed, Aug 6, 2014 at 1:35 AM, Tomasz Figa tomasz.f...@gmail.com wrote:
 Hi Vikas,

 Please see my comments inline.

 On 25.07.2014 13:49, Vikas Sajjan wrote:
 Refactoring the pm.c to avoid using soc_is_exynos checks,
 instead use the DT based lookup.

 While at it, consolidate the common code across SoCs
 and create a static helper functions.

 [snip]

 @@ -241,13 +266,13 @@ static void exynos_cpu_restore_register(void)

  static int exynos_cpu_suspend(unsigned long arg)
  {
 -#ifdef CONFIG_CACHE_L2X0
 - outer_flush_all();
 -#endif
 -
 - if (soc_is_exynos5250())
 - flush_cache_all();
 + if (pm_data-cpu_suspend)
 + return pm_data-cpu_suspend(arg);
 + return -1;
 +}

 I believe you could just pass pm_data-cpu_suspend to cpu_suspend(),
 without this three-liner.


OK.


 +static int exynos_cpu_do_idle(void)
 +{
   /* issue the standby signal into the pm unit. */
   cpu_do_idle();

 @@ -257,32 +282,73 @@ static int exynos_cpu_suspend(unsigned long arg)

  static void exynos_pm_prepare(void)
  {
 - unsigned int tmp;
 + if (pm_data-pm_prepare)
 + pm_data-pm_prepare();

 You could just directly insert this code into exynos_suspend_enter()
 instead of adding this two-liner.

OK.


 +}

 +static int exynos4_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 + outer_flush_all();
 +#endif
 + return exynos_cpu_do_idle();
 +}
 +
 +static int exynos5250_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 + outer_flush_all();
 +#endif
 + flush_cache_all();
 + return exynos_cpu_do_idle();
 +}

 I believe both can be merged safely into the same single function. A
 call to flush_cache_all() will not hurt on Exynos4, but it should be
 moved before outer_flush_all().

 Moreover, the #ifdef CONFIG_CACHE_L2X0 is superfluous, because the
 existence of outer_flush_all() symbol doesn't depend on this Kconfig
 symbol (it depends on CONFIG_OUTER_CACHE_SYNC, but a stub is provided if
 it is disabled).

 +
 +static void exynos_pm_set_wakeup_mask(void)
 +{
   /* Set wake-up mask registers */
   pmu_raw_writel(exynos_get_eint_wake_mask(), S5P_EINT_WAKEUP_MASK);
   pmu_raw_writel(exynos_irqwake_intmask  ~(1  31), S5P_WAKEUP_MASK);
 +}

 - s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 -
 - if (soc_is_exynos5250()) {
 - s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
 - /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 - tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 - tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 - pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
 - }
 -
 +static void exynos_pm_enter_sleep_mode(void)
 +{
   /* Set value of power down register for sleep mode */
 -
   exynos_sys_powerdown_conf(SYS_SLEEP);
   pmu_raw_writel(S5P_CHECK_SLEEP, S5P_INFORM1);

   /* ensure at least INFORM0 has the resume address */
 -
   pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
  }

 +static void exynos5250_pm_prepare(void)
 +{
 + unsigned int tmp;
 +
 + /* Set wake-up mask registers */
 + exynos_pm_set_wakeup_mask();
 +
 + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 +
 + s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));

 You could store a pointer to this array in new .extra_save field of the
 variant struct and handle this with generic code, like below:

 if (pm_data-extra_save)
 s3c_pm_do_save(pm_data-extra_save,
 pm_data-num_extra_save);


OK.

 +
 + /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 + tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 + tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 + pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);

 This looks completely like a copy paste from a vendor tree needed to
 workaround some issues in early revisions of the SoC. Are you sure this
 is still needed in production versions of the silicon used on boards
 supported in mainline?

This piece of code is NOT copy paste from my side, it is an already
existing code. I just moved it into the function
exynos5250_pm_prepare().  However I removed this piece of code and
made a common function for exynos4 and exynos5250, S2R works on 5250
well even without this piece of code.


 Even if yes, Exynos4 handles such registers in PMU register array in
 pmu.c, so I wonder whether it shouldn't be moved there and allow
 handling of all Exynos SoCs uniformly in this file.

 +
 + exynos_pm_enter_sleep_mode();
 +}
 +
 +static void exynos4_pm_prepare(void)
 +{
 + /* Set wake-up mask registers */
 + exynos_pm_set_wakeup_mask();
 +
 + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 +
 + exynos_pm_enter_sleep_mode();
 +}

 In fact, exynos4_pm_prepare() is a direct subset of
 exynos5250_pm_prepare(). This just confirms that it might be a good idea
 to 

Re: [PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-08-06 Thread Tomasz Figa
On 06.08.2014 14:58, Vikas Sajjan wrote:
 On Wed, Aug 6, 2014 at 1:35 AM, Tomasz Figa tomasz.f...@gmail.com wrote:
 On 25.07.2014 13:49, Vikas Sajjan wrote:

[snip]

 
 +
 + /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 + tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 + tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 + pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);

 This looks completely like a copy paste from a vendor tree needed to
 workaround some issues in early revisions of the SoC. Are you sure this
 is still needed in production versions of the silicon used on boards
 supported in mainline?
 
 This piece of code is NOT copy paste from my side, it is an already
 existing code. I just moved it into the function
 exynos5250_pm_prepare().

Sure, I'm aware of this. It might be good to know though if this is
really necessary, as I don't think we want useless code.

 However I removed this piece of code and
 made a common function for exynos4 and exynos5250, S2R works on 5250
 well even without this piece of code.

Thanks for checking this. I wonder if we could get some clarification
about this from hardware guys. Kukjin, Jingoo, any ideas or people that
might know what this is about?

For now, I believe it could be handled the same way as Exynos4, in PMU
register array as I suggested in my previous reply (quoted below).

 

 Even if yes, Exynos4 handles such registers in PMU register array in
 pmu.c, so I wonder whether it shouldn't be moved there and allow
 handling of all Exynos SoCs uniformly in this file.

 +
 + exynos_pm_enter_sleep_mode();
 +}
 +
 +static void exynos4_pm_prepare(void)
 +{
 + /* Set wake-up mask registers */
 + exynos_pm_set_wakeup_mask();
 +
 + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 +
 + exynos_pm_enter_sleep_mode();
 +}

 In fact, exynos4_pm_prepare() is a direct subset of
 exynos5250_pm_prepare(). This just confirms that it might be a good idea
 to just merge both functions into a single generic one.
 
 Right, can be merged into a common function which can be used for
 exynos4 and exynos5250.
 But I am afraid we still need specific functions in case of 5420.

Could you provide some details about Exynos5420 specific things?

 

 +
  static void exynos_pm_central_suspend(void)
  {
   unsigned long tmp;

[snip]

 +
   /* Platform-specific GIC callback */
   gic_arch_extn.irq_set_wake = exynos_irq_set_wake;

   /* All wakeup disable */
   tmp = pmu_raw_readl(S5P_WAKEUP_MASK);
 - tmp |= ((0xFF  8) | (0x1F  1));
 + tmp |= pm_data-wake_disable_mask;

 Does this vary between SoCs?
 
 Yes, It is different in case of 5420.

Could you provide more information about the difference?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-08-06 Thread Vikas Sajjan
Hi Tomasz,

On Wed, Aug 6, 2014 at 6:57 PM, Tomasz Figa t.f...@samsung.com wrote:
 On 06.08.2014 14:58, Vikas Sajjan wrote:
 On Wed, Aug 6, 2014 at 1:35 AM, Tomasz Figa tomasz.f...@gmail.com wrote:
 On 25.07.2014 13:49, Vikas Sajjan wrote:

 [snip]


 +
 + /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 + tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 + tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 + pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);

 This looks completely like a copy paste from a vendor tree needed to
 workaround some issues in early revisions of the SoC. Are you sure this
 is still needed in production versions of the silicon used on boards
 supported in mainline?

 This piece of code is NOT copy paste from my side, it is an already
 existing code. I just moved it into the function
 exynos5250_pm_prepare().

 Sure, I'm aware of this. It might be good to know though if this is
 really necessary, as I don't think we want useless code.

 However I removed this piece of code and
 made a common function for exynos4 and exynos5250, S2R works on 5250
 well even without this piece of code.

 Thanks for checking this. I wonder if we could get some clarification
 about this from hardware guys. Kukjin, Jingoo, any ideas or people that
 might know what this is about?

 For now, I believe it could be handled the same way as Exynos4, in PMU
 register array as I suggested in my previous reply (quoted below).



 Even if yes, Exynos4 handles such registers in PMU register array in
 pmu.c, so I wonder whether it shouldn't be moved there and allow
 handling of all Exynos SoCs uniformly in this file.

 +
 + exynos_pm_enter_sleep_mode();
 +}
 +
 +static void exynos4_pm_prepare(void)
 +{
 + /* Set wake-up mask registers */
 + exynos_pm_set_wakeup_mask();
 +
 + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 +
 + exynos_pm_enter_sleep_mode();
 +}

 In fact, exynos4_pm_prepare() is a direct subset of
 exynos5250_pm_prepare(). This just confirms that it might be a good idea
 to just merge both functions into a single generic one.

 Right, can be merged into a common function which can be used for
 exynos4 and exynos5250.
 But I am afraid we still need specific functions in case of 5420.

 Could you provide some details about Exynos5420 specific things?

Please refer my post [1] for 5420 PMU and S2R Support

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/269956.html




 +
  static void exynos_pm_central_suspend(void)
  {
   unsigned long tmp;

 [snip]

 +
   /* Platform-specific GIC callback */
   gic_arch_extn.irq_set_wake = exynos_irq_set_wake;

   /* All wakeup disable */
   tmp = pmu_raw_readl(S5P_WAKEUP_MASK);
 - tmp |= ((0xFF  8) | (0x1F  1));
 + tmp |= pm_data-wake_disable_mask;

 Does this vary between SoCs?

 Yes, It is different in case of 5420.

 Could you provide more information about the difference?

In case of 5420, it is wake_disable_mask = (0x7F  7) | (0x1F  1)



 Best regards,
 Tomasz
 --
 To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 
 in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-08-05 Thread Thomas Abraham
Hi Vikas,

On Fri, Jul 25, 2014 at 5:19 PM, Vikas Sajjan vikas.saj...@samsung.com wrote:
 Refactoring the pm.c to avoid using soc_is_exynos checks,
 instead use the DT based lookup.

 While at it, consolidate the common code across SoCs
 and create a static helper functions.

 Signed-off-by: Vikas Sajjan vikas.saj...@samsung.com
 ---
 changes since v1:
 - Address Kukjin Kim comments to respin this patch separately from
 
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272574.html
 - removed panic, returned if no PMU node found and added check in 
 exynos_wkup_irq.

 Rebased on Kukjin Kim's tree, for-next branch
 
 https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=for-next
 ---
  arch/arm/mach-exynos/pm.c   |  234 
 ---
  arch/arm/mach-exynos/regs-pmu.h |1 +
  2 files changed, 192 insertions(+), 43 deletions(-)

 diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
 index c4c6d98..1c875e5 100644
 --- a/arch/arm/mach-exynos/pm.c
 +++ b/arch/arm/mach-exynos/pm.c
 @@ -36,6 +36,8 @@
  #include regs-pmu.h
  #include regs-sys.h

 +#define REG_TABLE_END (-1U)
 +
  /**
   * struct exynos_wkup_irq - Exynos GIC to PMU IRQ mapping
   * @hwirq: Hardware IRQ signal of the GIC
 @@ -59,6 +61,19 @@ static struct sleep_save exynos_core_save[] = {
 SAVE_ITEM(S5P_SROM_BC3),
  };

 +struct exynos_pm_data {
 +   const struct exynos_wkup_irq *wkup_irq;
 +   unsigned int wake_disable_mask;
 +   unsigned int *release_ret_regs;
 +
 +   void (*pm_prepare)(void);
 +   void (*pm_resume)(void);
 +   int (*pm_suspend)(void);
 +   int (*cpu_suspend)(unsigned long);
 +};
 +
 +struct exynos_pm_data *pm_data;
 +
  /*
   * GIC wake-up support
   */
 @@ -77,14 +92,24 @@ static const struct exynos_wkup_irq exynos5250_wkup_irq[] 
 = {
 { /* sentinel */ },
  };

 +unsigned int exynos_release_ret_regs[] = {
 +   S5P_PAD_RET_MAUDIO_OPTION,
 +   S5P_PAD_RET_GPIO_OPTION,
 +   S5P_PAD_RET_UART_OPTION,
 +   S5P_PAD_RET_MMCA_OPTION,
 +   S5P_PAD_RET_MMCB_OPTION,
 +   S5P_PAD_RET_EBIA_OPTION,
 +   S5P_PAD_RET_EBIB_OPTION,
 +   REG_TABLE_END,
 +};
 +
  static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
  {
 const struct exynos_wkup_irq *wkup_irq;

 -   if (soc_is_exynos5250())
 -   wkup_irq = exynos5250_wkup_irq;
 -   else
 -   wkup_irq = exynos4_wkup_irq;
 +   if (!pm_data-wkup_irq)
 +   return -ENOENT;
 +   wkup_irq = pm_data-wkup_irq;

 while (wkup_irq-mask) {
 if (wkup_irq-hwirq == data-hwirq) {
 @@ -241,13 +266,13 @@ static void exynos_cpu_restore_register(void)

  static int exynos_cpu_suspend(unsigned long arg)
  {
 -#ifdef CONFIG_CACHE_L2X0
 -   outer_flush_all();
 -#endif
 -
 -   if (soc_is_exynos5250())
 -   flush_cache_all();
 +   if (pm_data-cpu_suspend)
 +   return pm_data-cpu_suspend(arg);
 +   return -1;
 +}

 +static int exynos_cpu_do_idle(void)
 +{
 /* issue the standby signal into the pm unit. */
 cpu_do_idle();

 @@ -257,32 +282,73 @@ static int exynos_cpu_suspend(unsigned long arg)

  static void exynos_pm_prepare(void)
  {
 -   unsigned int tmp;
 +   if (pm_data-pm_prepare)
 +   pm_data-pm_prepare();
 +}

 +static int exynos4_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 +   outer_flush_all();
 +#endif
 +   return exynos_cpu_do_idle();
 +}
 +
 +static int exynos5250_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 +   outer_flush_all();
 +#endif

Exynos5 SoCs do not use an additional outer cache controller. So the
above #ifdef block can be dropped.

 +   flush_cache_all();
 +   return exynos_cpu_do_idle();
 +}
 +
 +static void exynos_pm_set_wakeup_mask(void)
 +{
 /* Set wake-up mask registers */
 pmu_raw_writel(exynos_get_eint_wake_mask(), S5P_EINT_WAKEUP_MASK);
 pmu_raw_writel(exynos_irqwake_intmask  ~(1  31), S5P_WAKEUP_MASK);
 +}

 -   s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 -
 -   if (soc_is_exynos5250()) {
 -   s3c_pm_do_save(exynos5_sys_save, 
 ARRAY_SIZE(exynos5_sys_save));
 -   /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 -   tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 -   tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 -   pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
 -   }
 -
 +static void exynos_pm_enter_sleep_mode(void)
 +{
 /* Set value of power down register for sleep mode */
 -
 exynos_sys_powerdown_conf(SYS_SLEEP);
 pmu_raw_writel(S5P_CHECK_SLEEP, S5P_INFORM1);

 /* ensure at least INFORM0 has the resume address */
 -
 pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
  }

 +static void 

Re: [PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-08-05 Thread Tomasz Figa
Hi Vikas,

Please see my comments inline.

On 25.07.2014 13:49, Vikas Sajjan wrote:
 Refactoring the pm.c to avoid using soc_is_exynos checks,
 instead use the DT based lookup.
 
 While at it, consolidate the common code across SoCs
 and create a static helper functions.

[snip]

 @@ -241,13 +266,13 @@ static void exynos_cpu_restore_register(void)
  
  static int exynos_cpu_suspend(unsigned long arg)
  {
 -#ifdef CONFIG_CACHE_L2X0
 - outer_flush_all();
 -#endif
 -
 - if (soc_is_exynos5250())
 - flush_cache_all();
 + if (pm_data-cpu_suspend)
 + return pm_data-cpu_suspend(arg);
 + return -1;
 +}

I believe you could just pass pm_data-cpu_suspend to cpu_suspend(),
without this three-liner.

  
 +static int exynos_cpu_do_idle(void)
 +{
   /* issue the standby signal into the pm unit. */
   cpu_do_idle();
  
 @@ -257,32 +282,73 @@ static int exynos_cpu_suspend(unsigned long arg)
  
  static void exynos_pm_prepare(void)
  {
 - unsigned int tmp;
 + if (pm_data-pm_prepare)
 + pm_data-pm_prepare();

You could just directly insert this code into exynos_suspend_enter()
instead of adding this two-liner.

 +}
  
 +static int exynos4_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 + outer_flush_all();
 +#endif
 + return exynos_cpu_do_idle();
 +}
 +
 +static int exynos5250_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 + outer_flush_all();
 +#endif
 + flush_cache_all();
 + return exynos_cpu_do_idle();
 +}

I believe both can be merged safely into the same single function. A
call to flush_cache_all() will not hurt on Exynos4, but it should be
moved before outer_flush_all().

Moreover, the #ifdef CONFIG_CACHE_L2X0 is superfluous, because the
existence of outer_flush_all() symbol doesn't depend on this Kconfig
symbol (it depends on CONFIG_OUTER_CACHE_SYNC, but a stub is provided if
it is disabled).

 +
 +static void exynos_pm_set_wakeup_mask(void)
 +{
   /* Set wake-up mask registers */
   pmu_raw_writel(exynos_get_eint_wake_mask(), S5P_EINT_WAKEUP_MASK);
   pmu_raw_writel(exynos_irqwake_intmask  ~(1  31), S5P_WAKEUP_MASK);
 +}
  
 - s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 -
 - if (soc_is_exynos5250()) {
 - s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
 - /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 - tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 - tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 - pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
 - }
 -
 +static void exynos_pm_enter_sleep_mode(void)
 +{
   /* Set value of power down register for sleep mode */
 -
   exynos_sys_powerdown_conf(SYS_SLEEP);
   pmu_raw_writel(S5P_CHECK_SLEEP, S5P_INFORM1);
  
   /* ensure at least INFORM0 has the resume address */
 -
   pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
  }
  
 +static void exynos5250_pm_prepare(void)
 +{
 + unsigned int tmp;
 +
 + /* Set wake-up mask registers */
 + exynos_pm_set_wakeup_mask();
 +
 + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 +
 + s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));

You could store a pointer to this array in new .extra_save field of the
variant struct and handle this with generic code, like below:

if (pm_data-extra_save)
s3c_pm_do_save(pm_data-extra_save,
pm_data-num_extra_save);

 +
 + /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 + tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 + tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 + pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);

This looks completely like a copy paste from a vendor tree needed to
workaround some issues in early revisions of the SoC. Are you sure this
is still needed in production versions of the silicon used on boards
supported in mainline?

Even if yes, Exynos4 handles such registers in PMU register array in
pmu.c, so I wonder whether it shouldn't be moved there and allow
handling of all Exynos SoCs uniformly in this file.

 +
 + exynos_pm_enter_sleep_mode();
 +}
 +
 +static void exynos4_pm_prepare(void)
 +{
 + /* Set wake-up mask registers */
 + exynos_pm_set_wakeup_mask();
 +
 + s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 +
 + exynos_pm_enter_sleep_mode();
 +}

In fact, exynos4_pm_prepare() is a direct subset of
exynos5250_pm_prepare(). This just confirms that it might be a good idea
to just merge both functions into a single generic one.

 +
  static void exynos_pm_central_suspend(void)
  {
   unsigned long tmp;
 @@ -295,6 +361,13 @@ static void exynos_pm_central_suspend(void)
  
  static int exynos_pm_suspend(void)
  {
 + if (pm_data-pm_suspend)
 + return pm_data-pm_suspend();
 + return -1;
 +}

Do you need this three-liner? I believe you could just assign

Re: [PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-08-03 Thread Vikas Sajjan
Hi Kukjin,


On Fri, Jul 25, 2014 at 5:34 PM, Vikas Sajjan vikas.saj...@samsung.com wrote:
 Refactoring the pm.c to avoid using soc_is_exynos checks,
 instead use the DT based lookup.

 While at it, consolidate the common code across SoCs
 and create a static helper functions.

 Signed-off-by: Vikas Sajjan vikas.saj...@samsung.com
 ---
 changes since v1:
 - Address Kukjin Kim comments to respin this patch separately from
 
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272574.html
 - removed panic, returned if no PMU node found and added check in 
 exynos_wkup_irq.


Do you have any more comments.


 Rebased on Kukjin Kim's tree, for-next branch
 
 https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=for-next
 ---
  arch/arm/mach-exynos/pm.c   |  234 
 ---
  arch/arm/mach-exynos/regs-pmu.h |1 +
  2 files changed, 192 insertions(+), 43 deletions(-)

 diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
 index c4c6d98..1c875e5 100644
 --- a/arch/arm/mach-exynos/pm.c
 +++ b/arch/arm/mach-exynos/pm.c
 @@ -36,6 +36,8 @@
  #include regs-pmu.h
  #include regs-sys.h

 +#define REG_TABLE_END (-1U)
 +
  /**
   * struct exynos_wkup_irq - Exynos GIC to PMU IRQ mapping
   * @hwirq: Hardware IRQ signal of the GIC
 @@ -59,6 +61,19 @@ static struct sleep_save exynos_core_save[] = {
 SAVE_ITEM(S5P_SROM_BC3),
  };

 +struct exynos_pm_data {
 +   const struct exynos_wkup_irq *wkup_irq;
 +   unsigned int wake_disable_mask;
 +   unsigned int *release_ret_regs;
 +
 +   void (*pm_prepare)(void);
 +   void (*pm_resume)(void);
 +   int (*pm_suspend)(void);
 +   int (*cpu_suspend)(unsigned long);
 +};
 +
 +struct exynos_pm_data *pm_data;
 +
  /*
   * GIC wake-up support
   */
 @@ -77,14 +92,24 @@ static const struct exynos_wkup_irq exynos5250_wkup_irq[] 
 = {
 { /* sentinel */ },
  };

 +unsigned int exynos_release_ret_regs[] = {
 +   S5P_PAD_RET_MAUDIO_OPTION,
 +   S5P_PAD_RET_GPIO_OPTION,
 +   S5P_PAD_RET_UART_OPTION,
 +   S5P_PAD_RET_MMCA_OPTION,
 +   S5P_PAD_RET_MMCB_OPTION,
 +   S5P_PAD_RET_EBIA_OPTION,
 +   S5P_PAD_RET_EBIB_OPTION,
 +   REG_TABLE_END,
 +};
 +
  static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
  {
 const struct exynos_wkup_irq *wkup_irq;

 -   if (soc_is_exynos5250())
 -   wkup_irq = exynos5250_wkup_irq;
 -   else
 -   wkup_irq = exynos4_wkup_irq;
 +   if (!pm_data-wkup_irq)
 +   return -ENOENT;
 +   wkup_irq = pm_data-wkup_irq;

 while (wkup_irq-mask) {
 if (wkup_irq-hwirq == data-hwirq) {
 @@ -241,13 +266,13 @@ static void exynos_cpu_restore_register(void)

  static int exynos_cpu_suspend(unsigned long arg)
  {
 -#ifdef CONFIG_CACHE_L2X0
 -   outer_flush_all();
 -#endif
 -
 -   if (soc_is_exynos5250())
 -   flush_cache_all();
 +   if (pm_data-cpu_suspend)
 +   return pm_data-cpu_suspend(arg);
 +   return -1;
 +}

 +static int exynos_cpu_do_idle(void)
 +{
 /* issue the standby signal into the pm unit. */
 cpu_do_idle();

 @@ -257,32 +282,73 @@ static int exynos_cpu_suspend(unsigned long arg)

  static void exynos_pm_prepare(void)
  {
 -   unsigned int tmp;
 +   if (pm_data-pm_prepare)
 +   pm_data-pm_prepare();
 +}

 +static int exynos4_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 +   outer_flush_all();
 +#endif
 +   return exynos_cpu_do_idle();
 +}
 +
 +static int exynos5250_cpu_suspend(unsigned long arg)
 +{
 +#ifdef CONFIG_CACHE_L2X0
 +   outer_flush_all();
 +#endif
 +   flush_cache_all();
 +   return exynos_cpu_do_idle();
 +}
 +
 +static void exynos_pm_set_wakeup_mask(void)
 +{
 /* Set wake-up mask registers */
 pmu_raw_writel(exynos_get_eint_wake_mask(), S5P_EINT_WAKEUP_MASK);
 pmu_raw_writel(exynos_irqwake_intmask  ~(1  31), S5P_WAKEUP_MASK);
 +}

 -   s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 -
 -   if (soc_is_exynos5250()) {
 -   s3c_pm_do_save(exynos5_sys_save, 
 ARRAY_SIZE(exynos5_sys_save));
 -   /* Disable USE_RETENTION of JPEG_MEM_OPTION */
 -   tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
 -   tmp = ~EXYNOS5_OPTION_USE_RETENTION;
 -   pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
 -   }
 -
 +static void exynos_pm_enter_sleep_mode(void)
 +{
 /* Set value of power down register for sleep mode */
 -
 exynos_sys_powerdown_conf(SYS_SLEEP);
 pmu_raw_writel(S5P_CHECK_SLEEP, S5P_INFORM1);

 /* ensure at least INFORM0 has the resume address */
 -
 pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
  }

 +static void exynos5250_pm_prepare(void)
 +{
 +   unsigned int tmp;
 +
 +   /* Set wake-up mask 

[PATCH v2] ARM: EXYNOS: Refactor the pm code to use DT based lookup

2014-07-25 Thread Vikas Sajjan
Refactoring the pm.c to avoid using soc_is_exynos checks,
instead use the DT based lookup.

While at it, consolidate the common code across SoCs
and create a static helper functions.

Signed-off-by: Vikas Sajjan vikas.saj...@samsung.com
---
changes since v1:
- Address Kukjin Kim comments to respin this patch separately from

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272574.html
- removed panic, returned if no PMU node found and added check in 
exynos_wkup_irq.

Rebased on Kukjin Kim's tree, for-next branch 

https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=for-next
---
 arch/arm/mach-exynos/pm.c   |  234 ---
 arch/arm/mach-exynos/regs-pmu.h |1 +
 2 files changed, 192 insertions(+), 43 deletions(-)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index c4c6d98..1c875e5 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -36,6 +36,8 @@
 #include regs-pmu.h
 #include regs-sys.h
 
+#define REG_TABLE_END (-1U)
+
 /**
  * struct exynos_wkup_irq - Exynos GIC to PMU IRQ mapping
  * @hwirq: Hardware IRQ signal of the GIC
@@ -59,6 +61,19 @@ static struct sleep_save exynos_core_save[] = {
SAVE_ITEM(S5P_SROM_BC3),
 };
 
+struct exynos_pm_data {
+   const struct exynos_wkup_irq *wkup_irq;
+   unsigned int wake_disable_mask;
+   unsigned int *release_ret_regs;
+
+   void (*pm_prepare)(void);
+   void (*pm_resume)(void);
+   int (*pm_suspend)(void);
+   int (*cpu_suspend)(unsigned long);
+};
+
+struct exynos_pm_data *pm_data;
+
 /*
  * GIC wake-up support
  */
@@ -77,14 +92,24 @@ static const struct exynos_wkup_irq exynos5250_wkup_irq[] = 
{
{ /* sentinel */ },
 };
 
+unsigned int exynos_release_ret_regs[] = {
+   S5P_PAD_RET_MAUDIO_OPTION,
+   S5P_PAD_RET_GPIO_OPTION,
+   S5P_PAD_RET_UART_OPTION,
+   S5P_PAD_RET_MMCA_OPTION,
+   S5P_PAD_RET_MMCB_OPTION,
+   S5P_PAD_RET_EBIA_OPTION,
+   S5P_PAD_RET_EBIB_OPTION,
+   REG_TABLE_END,
+};
+
 static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
 {
const struct exynos_wkup_irq *wkup_irq;
 
-   if (soc_is_exynos5250())
-   wkup_irq = exynos5250_wkup_irq;
-   else
-   wkup_irq = exynos4_wkup_irq;
+   if (!pm_data-wkup_irq)
+   return -ENOENT;
+   wkup_irq = pm_data-wkup_irq;
 
while (wkup_irq-mask) {
if (wkup_irq-hwirq == data-hwirq) {
@@ -241,13 +266,13 @@ static void exynos_cpu_restore_register(void)
 
 static int exynos_cpu_suspend(unsigned long arg)
 {
-#ifdef CONFIG_CACHE_L2X0
-   outer_flush_all();
-#endif
-
-   if (soc_is_exynos5250())
-   flush_cache_all();
+   if (pm_data-cpu_suspend)
+   return pm_data-cpu_suspend(arg);
+   return -1;
+}
 
+static int exynos_cpu_do_idle(void)
+{
/* issue the standby signal into the pm unit. */
cpu_do_idle();
 
@@ -257,32 +282,73 @@ static int exynos_cpu_suspend(unsigned long arg)
 
 static void exynos_pm_prepare(void)
 {
-   unsigned int tmp;
+   if (pm_data-pm_prepare)
+   pm_data-pm_prepare();
+}
 
+static int exynos4_cpu_suspend(unsigned long arg)
+{
+#ifdef CONFIG_CACHE_L2X0
+   outer_flush_all();
+#endif
+   return exynos_cpu_do_idle();
+}
+
+static int exynos5250_cpu_suspend(unsigned long arg)
+{
+#ifdef CONFIG_CACHE_L2X0
+   outer_flush_all();
+#endif
+   flush_cache_all();
+   return exynos_cpu_do_idle();
+}
+
+static void exynos_pm_set_wakeup_mask(void)
+{
/* Set wake-up mask registers */
pmu_raw_writel(exynos_get_eint_wake_mask(), S5P_EINT_WAKEUP_MASK);
pmu_raw_writel(exynos_irqwake_intmask  ~(1  31), S5P_WAKEUP_MASK);
+}
 
-   s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
-
-   if (soc_is_exynos5250()) {
-   s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
-   /* Disable USE_RETENTION of JPEG_MEM_OPTION */
-   tmp = pmu_raw_readl(EXYNOS5_JPEG_MEM_OPTION);
-   tmp = ~EXYNOS5_OPTION_USE_RETENTION;
-   pmu_raw_writel(tmp, EXYNOS5_JPEG_MEM_OPTION);
-   }
-
+static void exynos_pm_enter_sleep_mode(void)
+{
/* Set value of power down register for sleep mode */
-
exynos_sys_powerdown_conf(SYS_SLEEP);
pmu_raw_writel(S5P_CHECK_SLEEP, S5P_INFORM1);
 
/* ensure at least INFORM0 has the resume address */
-
pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
 }
 
+static void exynos5250_pm_prepare(void)
+{
+   unsigned int tmp;
+
+   /* Set wake-up mask registers */
+   exynos_pm_set_wakeup_mask();
+
+   s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save));
+
+   s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
+
+   /* Disable USE_RETENTION of JPEG_MEM_OPTION */
+