On 10/01/18 01:46, Chanwoo Choi wrote:
> On 2018년 01월 09일 23:11, Sudeep Holla wrote:
>>
>>
>> On 09/01/18 07:59, Chanwoo Choi wrote:
>>> This patch adds the PMU (Power Management Unit) data of exynos5433 SoC
>>> in order to support the various power modes. Each power mode has
>>> the different value for reducing the power-consumption.
>>>
>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@xxxxxxxxxxx>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>> ---
>>> arch/arm/mach-exynos/common.h | 2 -
>>> drivers/soc/samsung/Makefile | 3 +-
>>> drivers/soc/samsung/exynos-pmu.c | 1 +
>>> drivers/soc/samsung/exynos-pmu.h | 2 +
>>> drivers/soc/samsung/exynos5433-pmu.c | 286 ++++++++++++++++++++++++++++
>>> include/linux/soc/samsung/exynos-regs-pmu.h | 148 ++++++++++++++
>>> 6 files changed, 439 insertions(+), 3 deletions(-)
>>> create mode 100644 drivers/soc/samsung/exynos5433-pmu.c
>>>
>>
>>> diff --git a/drivers/soc/samsung/exynos5433-pmu.c b/drivers/soc/samsung/exynos5433-pmu.c
>>> new file mode 100644
>>> index 000000000000..2571e61522f0
>>> --- /dev/null
>>> +++ b/drivers/soc/samsung/exynos5433-pmu.c
>>> @@ -0,0 +1,286 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +//
>>> +// Copyright (c) 2018 Samsung Electronics Co., Ltd.
>>> +// Copyright (c) Jonghwa Lee <jonghwa3.lee@xxxxxxxxxxx>
>>> +// Copyright (c) Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>> +//
>>> +// EXYNOS5433 - CPU PMU (Power Management Unit) support
>>> +
>>> +#include <linux/soc/samsung/exynos-regs-pmu.h>
>>> +#include <linux/soc/samsung/exynos-pmu.h>
>>> +
>>> +#include "exynos-pmu.h"
>>> +
>>> +static struct exynos_pmu_conf exynos5433_pmu_config[] = {
>>> + /* { .offset = address, .val = { AFTR, LPA, SLEEP } } */
>>> + { EXYNOS5433_ATLAS_CPU0_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU0_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_ATLAS_CPU1_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU1_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_ATLAS_CPU2_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU2_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_ATLAS_CPU3_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_ATLAS_CPU3_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_APOLLO_CPU0_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU0_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_APOLLO_CPU1_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU1_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_APOLLO_CPU2_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU2_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_APOLLO_CPU3_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_DIS_IRQ_APOLLO_CPU3_CENTRAL_SYS_PWR_REG, { 0x0, 0x0, 0x0 } },
>>> + { EXYNOS5433_ATLAS_NONCPU_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>> + { EXYNOS5433_APOLLO_NONCPU_SYS_PWR_REG, { 0x0, 0x0, 0x8 } },
>>
>>
>> 1. First of all why do you need any of these CPU related PMU config
>> registers in kernel ? From the information I gathered this is ARM64
>> SoC using PSCI. These are needed just in PSCI implementation and not
>> in kernel. So can you elaborate on why there are present here ?
>
> The 32bit Exynos used the 'smc' call to enter the suspend mode
> and need to handle the PMU registers.
>
OK
> Even if PSCI replaces the 'smc' call on the Exynos5433,
> the Exynos5433's document requires the handling of PMU config
> related to CPU for the suspend mode.
>
OK but does it state it needs to be in kernel ?(i.e. in EL1 NS)
> IMHO, If the secure OS implemented the all something related to CPU,
> it might be unnecessary to handle the PMU registers. I think that
> it depend on how to design the SoC by H/W Architect. This is just my opinion.
>
Even if it can be accessed in kernel, better to keep all these in PSCI
implementation that does the actual power state management. Mixing them
in both kernel and PSCI implementation is just a mess TBH and not easy
to maintain the platform if some issues pop up.
>>
>> 2. Are there any public documents that these names map to ?
>
> There is no public document. It is confidential.
>
I guessed so and hence raised to remove it.
>> If there is none, please replace these codenames(ATLAS, APOLLO) with
>> appropriately.
>
> In the Exynos5433, 'apollo' indicates the LITTLE cores (cpu0-3, cortex-a53)
> and 'atlas' indicates the big cores (cpu4-7, cortex-a57)
>
> Exynos5433 already used the 'apollo' and 'atlas' on clk-exynos5433.c driver
> and thermal device-tree node. It is better to use the original register name
> in the document in order to reduce the confusion of the change of register name
> even if document is not public.
>
> Also, exynos7 used the 'atlas' word for big cores.
>
I disagree. If you don't have any public documents, then better use
names other developers are used to and not some code names which
otherwise is not seen in any other part of the code base. Grepping
just shows Exynos using these code names which are generally restricted
for internal use.
--
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
