Re: [PATCH v5 9/9] drivers: soc: Add support for Exynos PMU driver

[Krzysztof Kozlowski]

On 17.11.2015 12:44, pankaj.dubey wrote:
> 
> 
> On Saturday 14 November 2015 05:00 PM, Krzysztof Kozlowski wrote:
>> W dniu 14.11.2015 o 00:36, Pankaj Dubey pisze:
>>> On 13 November 2015 at 15:58, Krzysztof Kozlowski
>>> <k.kozlowski@xxxxxxxxxxx> wrote:
>>>>
>>>> On 13.11.2015 18:29, Pankaj Dubey wrote:
>>>>> This patch moves Exynos PMU driver implementation from
>>>>> "arm/mach-exynos"
>>>>> to "drivers/soc/samsung". This driver is mainly used for setting misc
>>>>> bits of register from PMU IP of Exynos SoC which will be required to
>>>>> configure before Suspend/Resume. Currently all these settings are done
>>>>> in "arch/arm/mach-exynos/pmu.c" but moving ahead for ARM64 based SoC
>>>>> support, there is a need of this PMU driver in driver/* folder.
>>>>>
>>>>> This driver uses existing DT binding information and there should
>>>>> be no functionality change in the supported platforms.
>>>>>
>>>>> Signed-off-by: Amit Daniel Kachhap <amitdanielk@xxxxxxxxx>
>>>>> Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
>>>>> ---
>>>>>   arch/arm/mach-exynos/Kconfig                                   | 1 +
>>>>>   arch/arm/mach-exynos/Makefile                                  |
>>>>> 4 +---
>>>>>   drivers/soc/samsung/Kconfig                                    |
>>>>> 4 ++++
>>>>>   drivers/soc/samsung/Makefile                                   |
>>>>> 4 ++++
>>>>>   arch/arm/mach-exynos/pmu.c => drivers/soc/samsung/exynos-pmu.c | 0
>>>>>   {arch/arm/mach-exynos => drivers/soc/samsung}/exynos-pmu.h     | 0
>>>>>   {arch/arm/mach-exynos => drivers/soc/samsung}/exynos3250-pmu.c | 0
>>>>>   {arch/arm/mach-exynos => drivers/soc/samsung}/exynos4-pmu.c    | 0
>>>>>   {arch/arm/mach-exynos => drivers/soc/samsung}/exynos5250-pmu.c | 0
>>>>>   {arch/arm/mach-exynos => drivers/soc/samsung}/exynos5420-pmu.c | 0
>>>>>   10 files changed, 10 insertions(+), 3 deletions(-)
>>>>>   rename arch/arm/mach-exynos/pmu.c =>
>>>>> drivers/soc/samsung/exynos-pmu.c (100%)
>>>>>   rename {arch/arm/mach-exynos => drivers/soc/samsung}/exynos-pmu.h
>>>>> (100%)
>>>>>   rename {arch/arm/mach-exynos =>
>>>>> drivers/soc/samsung}/exynos3250-pmu.c (100%)
>>>>>   rename {arch/arm/mach-exynos =>
>>>>> drivers/soc/samsung}/exynos4-pmu.c (100%)
>>>>>   rename {arch/arm/mach-exynos =>
>>>>> drivers/soc/samsung}/exynos5250-pmu.c (100%)
>>>>>   rename {arch/arm/mach-exynos =>
>>>>> drivers/soc/samsung}/exynos5420-pmu.c (100%)
>>>>>
>>>>> diff --git a/arch/arm/mach-exynos/Kconfig
>>>>> b/arch/arm/mach-exynos/Kconfig
>>>>> index 83c85f5..874cb38 100644
>>>>> --- a/arch/arm/mach-exynos/Kconfig
>>>>> +++ b/arch/arm/mach-exynos/Kconfig
>>>>> @@ -16,6 +16,7 @@ menuconfig ARCH_EXYNOS
>>>>>        select ARM_GIC
>>>>>        select COMMON_CLK_SAMSUNG
>>>>>        select EXYNOS_THERMAL
>>>>> +     select EXYNOS_PMU
>>>>>        select EXYNOS_SROM if PM
>>>>>        select HAVE_ARM_SCU if SMP
>>>>>        select HAVE_S3C2410_I2C if I2C
>>>>> diff --git a/arch/arm/mach-exynos/Makefile
>>>>> b/arch/arm/mach-exynos/Makefile
>>>>> index 2d58063..34d29df 100644
>>>>> --- a/arch/arm/mach-exynos/Makefile
>>>>> +++ b/arch/arm/mach-exynos/Makefile
>>>>> @@ -9,9 +9,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) +=
>>>>> -I$(srctree)/$(src)/include -I$(srctree)
>>>>>
>>>>>   # Core
>>>>>
>>>>> -obj-$(CONFIG_ARCH_EXYNOS)    += exynos.o pmu.o exynos-smc.o
>>>>> firmware.o \
>>>>> -                                     exynos3250-pmu.o exynos4-pmu.o \
>>>>> -                                     exynos5250-pmu.o
>>>>> exynos5420-pmu.o
>>>>> +obj-$(CONFIG_ARCH_EXYNOS)    += exynos.o exynos-smc.o firmware.o
>>>>>
>>>>>   obj-$(CONFIG_EXYNOS_CPU_SUSPEND) += pm.o sleep.o
>>>>>   obj-$(CONFIG_PM_SLEEP)               += suspend.o
>>>>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>>>>> index 2833b5b..f545d6c 100644
>>>>> --- a/drivers/soc/samsung/Kconfig
>>>>> +++ b/drivers/soc/samsung/Kconfig
>>>>> @@ -10,4 +10,8 @@ config EXYNOS_SROM
>>>>>        bool
>>>>>        depends on ARM && ARCH_EXYNOS && PM
>>>>>
>>>>> +config EXYNOS_PMU
>>>>> +     bool
>>>>> +     depends on ARCH_EXYNOS
>>>>> +
>>>>>   endmenu
>>>>> diff --git a/drivers/soc/samsung/Makefile
>>>>> b/drivers/soc/samsung/Makefile
>>>>> index 9c554d5..26fb489 100644
>>>>> --- a/drivers/soc/samsung/Makefile
>>>>> +++ b/drivers/soc/samsung/Makefile
>>>>> @@ -1 +1,5 @@
>>>>>   obj-$(CONFIG_EXYNOS_SROM)    += exynos-srom.o
>>>>> +ifdef CONFIG_ARM
>>>>> +obj-$(CONFIG_EXYNOS_PMU)     += exynos-pmu.o exynos3250-pmu.o
>>>>> exynos4-pmu.o \
>>>>> +                                     exynos5250-pmu.o
>>>>> exynos5420-pmu.o
>>>>> +endif
>>>>
>>>> Why ifdef CONFIG_ARM? This already depends on ARCH_EXYNOS. If you want
>>>> to limit to ARMv7 then add the dependency to Kconfig (just like
>>>> EXYNOS_SROM).
>>>>
>>>
>>> This is required, so that 32-bit based Exynos SoC's PMU should not get
>>> compiled when we are compiling for ARM64 and vice-versa.
>>>
>>> For example: In future, I have plan to add exynos7 PMU support as -
>>>
>>> ifdef CONFIG_ARM64
>>> obj-$(CONFIG_EXYNOS_PMU) += exynos-pmu.o exynos7-pmu.o
>>> endif
>>>
>>> Thus preventing compilation of ARM64 based SoCs PMU data files when we
>>> are compiling for ARM.
>>>
>>> Only exynos-pmu.c will be shared and compiled in both cases.
>>
>> But this actually spreads Kconfig configuration over different places:
>> Kconfig and Makefile. It confuses and makes it more difficult in the
>> future to maintain and extend.
>>
> 
> I could not understand your point here. Will you please explain what
> issue you can see in this approach?

You are spreading Kconfig symbols over different places. These should be
put only in one - in Kconfig.

Some time later someone will add COMPILE_TEST to Kconfig and wonder why
it does not work... oh yeah, because dependency was put not only in
Kconfig but also in Makefile. One place for dependency: Kconfig.

> 
>> This should be in Kconfig. If you want to limit this per architecture
>> (is it really needed?) then maybe define different EXYNOS_PMU config
>> symbols?
>>
> 
> Do you mean we need to have separate configs for enabling PMU on ARM and
> ARM64 based Exynos SoCs?

Yes. If you want to separate ARMv7 from ARMv8 then yes. Just like we
have for other drivers - usually per SoC but this can be per architecture.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/