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
pm_data->pm_suspend directly to exynos_pm_syscore_ops.
> +
> +static int exynos4_pm_suspend(void)
> +{
> unsigned long tmp;
>
> exynos_pm_central_suspend();
> @@ -304,9 +377,20 @@ static int exynos_pm_suspend(void)
> 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();
> + exynos_cpu_save_register();
> + return 0;
> +}
> +
> +static int exynos5250_pm_suspend(void)
> +{
> + unsigned long tmp;
> +
> + 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);
> return 0;
> }
This function is exactly the same as exynos4_pm_suspend, but without the
call to exynos_cpu_save_register(). Since originally this call was
conditional, depending on CPU part number, is there any reason to split
this function into SoC specific ones?
>
> @@ -333,39 +417,57 @@ static int exynos_pm_central_resume(void)
> return 0;
> }
>
> +static void exynos_pm_set_release_retention(void)
nit: exynos_pm_release_retention() would sound better.
> +{
> + unsigned int i;
> +
> + for (i = 0; (pm_data->release_ret_regs[i] != REG_TABLE_END); i++)
> + pmu_raw_writel(EXYNOS_WAKEUP_FROM_LOWPWR,
> + pm_data->release_ret_regs[i]);
> +}
> +
> static void exynos_pm_resume(void)
> {
> + if (pm_data->pm_resume)
> + pm_data->pm_resume();
> +}
Similarly as with exynos_pm_suspend().
> +
> +static void exynos4_pm_resume(void)
> +{
> if (exynos_pm_central_resume())
> goto early_wakeup;
>
> - if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
> - exynos_cpu_restore_register();
> + exynos_cpu_restore_register();
>
> /* For release retention */
> -
> - 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())
> - s3c_pm_do_restore(exynos5_sys_save,
> - ARRAY_SIZE(exynos5_sys_save));
> + exynos_pm_set_release_retention();
>
> s3c_pm_do_restore_core(exynos_core_save, ARRAY_SIZE(exynos_core_save));
>
> - if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
> - scu_enable(S5P_VA_SCU);
> + scu_enable(S5P_VA_SCU);
>
> early_wakeup:
>
> /* Clear SLEEP mode set in INFORM1 */
> pmu_raw_writel(0x0, S5P_INFORM1);
> +}
> +
> +static void exynos5250_pm_resume(void)
> +{
> + if (exynos_pm_central_resume())
> + goto early_wakeup;
>
> - return;
> + /* For release retention */
> + exynos_pm_set_release_retention();
> +
> + s3c_pm_do_restore(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save));
> +
> + s3c_pm_do_restore_core(exynos_core_save, ARRAY_SIZE(exynos_core_save));
> +
> +early_wakeup:
> +
> + /* Clear SLEEP mode set in INFORM1 */
> + pmu_raw_writel(0x0, S5P_INFORM1);
> }
Similar to exynos{4,5250}_pm_suspend(), this could be kept as a single
generic function. exynos5_sys_save would be handled by .extra_save field
of the pm_data struct and scu_enable() and exynos_cpu_restore_register()
was already handled in a generic way, based on ARM core type, not
soc_is_*().
>
> static struct syscore_ops exynos_pm_syscore_ops = {
> @@ -468,18 +570,64 @@ static struct notifier_block exynos_cpu_pm_notifier_block = {
> .notifier_call = exynos_cpu_pm_notifier,
> };
>
> +static struct exynos_pm_data exynos4_pm_data = {
static const
> + .wkup_irq = exynos4_wkup_irq,
> + .wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
> + .release_ret_regs = exynos_release_ret_regs,
> + .pm_suspend = exynos4_pm_suspend,
> + .pm_resume = exynos4_pm_resume,
> + .pm_prepare = exynos4_pm_prepare,
> + .cpu_suspend = exynos4_cpu_suspend,
> +};
> +
> +static struct exynos_pm_data exynos5250_pm_data = {
static const
> + .wkup_irq = exynos5250_wkup_irq,
> + .wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
> + .release_ret_regs = exynos_release_ret_regs,
> + .pm_suspend = exynos5250_pm_suspend,
> + .pm_resume = exynos5250_pm_resume,
> + .pm_prepare = exynos5250_pm_prepare,
> + .cpu_suspend = exynos5250_cpu_suspend,
> +};
> +
> +static struct of_device_id exynos_pmu_of_device_ids[] = {
> + {
> + .compatible = "samsung,exynos4210-pmu",
> + .data = (void *)&exynos4_pm_data,
No need to cast if const pointers are used. + 3 more below.
> + }, {
> + .compatible = "samsung,exynos4212-pmu",
> + .data = (void *)&exynos4_pm_data,
> + }, {
> + .compatible = "samsung,exynos4412-pmu",
> + .data = (void *)&exynos4_pm_data,
> + }, {
> + .compatible = "samsung,exynos5250-pmu",
> + .data = (void *)&exynos5250_pm_data,
> + },
> + { /*sentinel*/ },
> +};
> +
> void __init exynos_pm_init(void)
> {
> u32 tmp;
> + const struct of_device_id *match;
nit: it would look better if this variable was added above tmp.
>
> cpu_pm_register_notifier(&exynos_cpu_pm_notifier_block);
>
> + of_find_matching_node_and_match(NULL, exynos_pmu_of_device_ids, &match);
> +
nit: superfluous blank line
> + if (!match) {
> + pr_err("Failed to find PMU node\n");
> + return;
> + }
> + pm_data = (struct exynos_pm_data *) match->data;
No need to cast if const pointers are used.
> +
> /* 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?
> pmu_raw_writel(tmp, S5P_WAKEUP_MASK);
>
> register_syscore_ops(&exynos_pm_syscore_ops);
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index 96a1569..30c0301 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -21,6 +21,7 @@
> #define S5P_USE_STANDBY_WFI0 (1 << 16)
> #define S5P_USE_STANDBY_WFE0 (1 << 24)
>
> +#define EXYNOS_WAKEUP_FROM_LOWPWR (1 << 28)
Is this really the real name of this bit, as specified in the datasheet?
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
