linux-kernel - Re: [2/3] soc: samsung: Do not build ARMv7 PMU drivers on ARMv8

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-id: <0ae4c538-a3af-a1b7-418d-4bccbf09faf4@samsung.com>
Date:   Tue, 14 Mar 2017 14:10:02 +0530
From:   Alim Akhtar <alim.akhtar@...sung.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Kukjin Kim <kgene@...nel.org>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Arnd Bergmann <arnd@...db.de>,
        Kevin Hilman <khilman@...libre.com>,
        Olof Johansson <olof@...om.net>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org
Subject: Re: [2/3] soc: samsung: Do not build ARMv7 PMU drivers on ARMv8



On 03/14/2017 01:32 PM, Krzysztof Kozlowski wrote:
> On Tue, Mar 14, 2017 at 9:51 AM, Alim Akhtar <alim.akhtar@...sung.com> wrote:
>> Hi Krzysztof,
>>
>> On 03/12/2017 03:08 AM, Krzysztof Kozlowski wrote:
>>> The Exynos Power Management Unit (PMU) drivers contain quite large
>>> static arrays of register values necessary for given Exynos SoC to enter
>>> low power mode.  All this data is useless for ARMv8 SoC like
>>> Exynos5433, because the image will not be shared between ARMv7 and
>>> ARMv8.
>>>
>>> Add additional Kconfig symbol for selecting the SoC-specific driver
>>> addons thus skipping the useless data in the final image (this is
>>> similar approach to chosen for Exynos clock controller drivers):
>>>  - exynos-pmu driver will be compiled on both architectures ARMv7
>>>    and ARMv8,
>>>  - additional driver_data for ARMv7 SoCs will not be built on ARMv8
>>>    and a macro will return NULL for them in of_device_id - this should
>>>    be safe as these compatibles cannot match on ARMv7 and driver
>>>    anyway handles NULL driver_data,
>>>  - on ARMv8 compile only exynos-pmu driver which exposes the
>>>    syscon-regmap for PMU address space.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzk@...nel.org>
>>> ---
>>>  drivers/soc/samsung/Kconfig      |  8 +++++++-
>>>  drivers/soc/samsung/Makefile     |  4 +++-
>>>  drivers/soc/samsung/exynos-pmu.c | 22 ++++++++++++++++------
>>>  drivers/soc/samsung/exynos-pmu.h |  3 +++
>>>  4 files changed, 29 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>>> index 245533907d1b..8b25bd55e648 100644
>>> --- a/drivers/soc/samsung/Kconfig
>>> +++ b/drivers/soc/samsung/Kconfig
>>> @@ -8,7 +8,13 @@ if SOC_SAMSUNG
>>>
>>>  config EXYNOS_PMU
>>>       bool "Exynos PMU controller driver" if COMPILE_TEST
>>> -     depends on (ARM && ARCH_EXYNOS) || ((ARM || ARM64) && COMPILE_TEST)
>>> +     depends on ARCH_EXYNOS || ((ARM || ARM64) && COMPILE_TEST)
>>> +     select EXYNOS_PMU_ARM_DRIVERS if ARM && ARCH_EXYNOS
>>> +
>>
>> In general this patch look ok, but I was think we should make these
>> configs configurable via _menuconfig_. Currently these are visible only
>> if COMPILE_TEST is enabled.
>> Recently I was working on adding PMU support for Exynos7 and I face
>> issues when I want to disable this option and re-enable it for testing
>> purpose.
>
> These drivers are not available in menuconfig on purpose - these are
> essential parts of SoC. Without them usually something will not work
> so user should not be able to disable them. For all of such drivers,
> we use the SELECT from mach approach.
>
Well, what you are saying is very subjective. In past I have face issues 
where to isolate or narrow down some issue, we do need to play with PMU 
and power domains. So, having a configurable option won't hurt here.
Anyway if you (or anyone else) strongly feel we should be following 
"SELECT from MACH approach" then lets follow it.


>>> +# There is no need to enable these drivers for ARMv8
>>> +config EXYNOS_PMU_ARM_DRIVERS
>>> +     bool "Exynos PMU ARMv7-specific driver extensions" if COMPILE_TEST
>>> +     depends on EXYNOS_PMU
>>>
>>>  config EXYNOS_PM_DOMAINS
>>>       bool "Exynos PM domains" if COMPILE_TEST
>>> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
>>> index 3619f2ecddaa..4d7694a4e7a4 100644
>>> --- a/drivers/soc/samsung/Makefile
>>> +++ b/drivers/soc/samsung/Makefile
>>> @@ -1,3 +1,5 @@
>>> -obj-$(CONFIG_EXYNOS_PMU)     += exynos-pmu.o exynos3250-pmu.o exynos4-pmu.o \
>>> +obj-$(CONFIG_EXYNOS_PMU)     += exynos-pmu.o
>>> +
>>> +obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS) += exynos3250-pmu.o exynos4-pmu.o \
>>>                                       exynos5250-pmu.o exynos5420-pmu.o
>>>  obj-$(CONFIG_EXYNOS_PM_DOMAINS) += pm_domains.o
>>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>>> index 56d9244ff981..bd4a76f27bc2 100644
>>> --- a/drivers/soc/samsung/exynos-pmu.c
>>> +++ b/drivers/soc/samsung/exynos-pmu.c
>>> @@ -69,27 +69,37 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
>>>  }
>>>
>>>  /*
>>> + * Split the data between ARM architectures because it is relatively big
>>> + * and useless on other arch.
>>> + */
>>> +#ifdef CONFIG_EXYNOS_PMU_ARM_DRIVERS
>>> +#define exynos_pmu_data_arm_ptr(data)        (&data)
>>> +#else
>>> +#define exynos_pmu_data_arm_ptr(data)        NULL
>>> +#endif
>>> +
>>> +/*
>>>   * PMU platform driver and devicetree bindings.
>>>   */
>>>  static const struct of_device_id exynos_pmu_of_device_ids[] = {
>>>       {
>>>               .compatible = "samsung,exynos3250-pmu",
>>> -             .data = &exynos3250_pmu_data,
>>> +             .data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
>>>       }, {
>>>               .compatible = "samsung,exynos4210-pmu",
>>> -             .data = &exynos4210_pmu_data,
>>> +             .data = exynos_pmu_data_arm_ptr(exynos4210_pmu_data),
>>>       }, {
>>>               .compatible = "samsung,exynos4212-pmu",
>>> -             .data = &exynos4212_pmu_data,
>>> +             .data = exynos_pmu_data_arm_ptr(exynos4212_pmu_data),
>>>       }, {
>>>               .compatible = "samsung,exynos4412-pmu",
>>> -             .data = &exynos4412_pmu_data,
>>> +             .data = exynos_pmu_data_arm_ptr(exynos4412_pmu_data),
>>>       }, {
>>>               .compatible = "samsung,exynos5250-pmu",
>>> -             .data = &exynos5250_pmu_data,
>>> +             .data = exynos_pmu_data_arm_ptr(exynos5250_pmu_data),
>>>       }, {
>>>               .compatible = "samsung,exynos5420-pmu",
>>> -             .data = &exynos5420_pmu_data,
>>> +             .data = exynos_pmu_data_arm_ptr(exynos5420_pmu_data),
>>>       }, {
>>>               .compatible = "samsung,exynos5433-pmu",
>> So, as I understand, the idea here to use something like
>>         .data = &exynos5433_pmu_data or so in case ARMv8?
>
> In the future, yes. Now there is no low power mode support for
> Exynos5433 so the data is left empty.
>
Ok, this is fine.

> Best regards,
> Krzysztof
>
>
>