| 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
| ||
|
Date: Wed, 25 Feb 2015 10:26:12 +0000
From: "Suzuki K. Poulose" <Suzuki.Poulose@....com>
To: Nicolas Pitre <nicolas.pitre@...aro.org>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Kukjin Kim <kgene@...nel.org>,
Abhilash Kesavan <a.kesavan@...sung.com>,
Arnd Bergmann <arnd@...db.de>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Liviu Dudau <Liviu.Dudau@....com>,
Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
Olof Johansson <olof@...om.net>,
Pawel Moll <Pawel.Moll@....com>,
Punit Agrawal <Punit.Agrawal@....com>,
Sudeep Holla <Sudeep.Holla@....com>,
Will Deacon <Will.Deacon@....com>,
Catalin Marinas <Catalin.Marinas@....com>
Subject: Re: [PATCH 3/4] arm-cci: Split the code for PMU vs driver support
On 24/02/15 22:17, Nicolas Pitre wrote:
> On Tue, 24 Feb 2015, Suzuki K. Poulose wrote:
>
>> From: "Suzuki K. Poulose" <suzuki.poulose@....com>
>>
>> This patch separates the PMU driver code from the low level
>> CCI driver code, and enables the CCI400-PMU for ARM64.
>>
>> Introduces config options for both.
>>
>> - ARM_CCI400_MCPM - controls the low level MCPM driver code for CCI
>> - ARM_CCI400_PMU - controls the PMU driver code
>> - ARM_CCI400_COMMON - CCI400 specific details shared by MCPM
>> and PMU
>> Changes:
>> - ARM_CCI - common code for probing the CCI devices
>>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
>> Cc: Kukjin Kim <kgene@...nel.org>
>> Cc: Abhilash Kesavan <a.kesavan@...sung.com>
>> Cc: Liviu Dudau <liviu.dudau@....com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
>> Cc: Sudeep Holla <sudeep.holla@....com>
>> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@....com>
>
> Comments inline.
>
>> ---
>> arch/arm/mach-exynos/Kconfig | 2 +-
>> arch/arm/mach-vexpress/Kconfig | 4 ++--
>> drivers/bus/Kconfig | 28 +++++++++++++++++++++++-----
>> drivers/bus/arm-cci.c | 25 +++++++++++++++++++++----
>> include/linux/arm-cci.h | 7 ++++++-
>> 5 files changed, 53 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
>> index 603820e..9bc8b4d 100644
>> --- a/arch/arm/mach-exynos/Kconfig
>> +++ b/arch/arm/mach-exynos/Kconfig
>> @@ -123,7 +123,7 @@ config SOC_EXYNOS5800
>> config EXYNOS5420_MCPM
>> bool "Exynos5420 Multi-Cluster PM support"
>> depends on MCPM && SOC_EXYNOS5420
>> - select ARM_CCI
>> + select ARM_CCI400_MCPM
>> select ARM_CPU_SUSPEND
>> help
>> This is needed to provide CPU and cluster power management
>> diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
>> index d6b16d9..097912f 100644
>> --- a/arch/arm/mach-vexpress/Kconfig
>> +++ b/arch/arm/mach-vexpress/Kconfig
>> @@ -53,7 +53,7 @@ config ARCH_VEXPRESS_CORTEX_A5_A9_ERRATA
>> config ARCH_VEXPRESS_DCSCB
>> bool "Dual Cluster System Control Block (DCSCB) support"
>> depends on MCPM
>> - select ARM_CCI
>> + select ARM_CCI400_MCPM
>> help
>> Support for the Dual Cluster System Configuration Block (DCSCB).
>> This is needed to provide CPU and cluster power management
>> @@ -71,7 +71,7 @@ config ARCH_VEXPRESS_SPC
>> config ARCH_VEXPRESS_TC2_PM
>> bool "Versatile Express TC2 power management"
>> depends on MCPM
>> - select ARM_CCI
>> + select ARM_CCI400_MCPM
>> select ARCH_VEXPRESS_SPC
>> help
>> Support for CPU and cluster power management on Versatile Express
>> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
>> index b99729e..91dd013 100644
>> --- a/drivers/bus/Kconfig
>> +++ b/drivers/bus/Kconfig
>> @@ -43,12 +43,30 @@ config OMAP_INTERCONNECT
>> help
>> Driver to enable OMAP interconnect error handling driver.
>>
>> -config ARM_CCI
>> - bool "ARM CCI driver support"
>> - depends on ARM && OF && CPU_V7
>> +config ARM_CCI400_MCPM
>> + bool
>> + depends on ARM && OF && CPU_V7 && MCPM
>
> MCPM is not an actual dependency and therefore should probably not be
> added here.
OK, will remove that.
> You removed the prompt string therefore this will only be
> selectable explicitly as needed.
This was intentional, I missed mentioning about it. Do you think we
need to change it back ?
>
> Also, shouldn't it select ARM_CCI400_COMMON ?
Thanks for that, yes it should.
>
>> + help
>> + Low level power management driver for CCI400 cache coherent
>> + interconnect for ARM platforms.
>> +
>> +config ARM_CCI400_PMU
>> + bool "ARM CCI400 PMU support"
>> + depends on ARM || ARM64
>> + depends on HW_PERF_EVENTS
>> + select ARM_CCI400_COMMON
>> help
>> - Driver supporting the CCI cache coherent interconnect for ARM
>> - platforms.
>> + Support for PMU events monitoring on the ARM CCI cache coherent
>> + interconnect.
>> +
>> + If unsure, say N
>> +
>> +config ARM_CCI400_COMMON
>> + bool
>> + select ARM_CCI
>> +
>> +config ARM_CCI
>> + bool
>
> Surely you could do with only one of ARM_CCI or ARM_CCI400_COMMON?
> Personally I'd go with the later as it is more precise.
The ARM_CCI now stands for CCI version agnostic code. This can be used
for adding support for the newer versions, e.g CCI-500, which I am
planning to post, after this series gets sorted out.
>
>> config ARM_CCN
>> bool "ARM CCN driver support"
>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>> index fe9fa46..7e330fe 100644
>> --- a/drivers/bus/arm-cci.c
>> +++ b/drivers/bus/arm-cci.c
>> @@ -32,6 +32,7 @@
>> static void __iomem *cci_ctrl_base;
>> static unsigned long cci_ctrl_phys;
>>
>> +#ifdef CONFIG_ARM_CCI400_MCPM
>> struct cci_nb_ports {
>> unsigned int nb_ace;
>> unsigned int nb_ace_lite;
>> @@ -42,12 +43,19 @@ static const struct cci_nb_ports cci400_ports = {
>> .nb_ace_lite = 3
>> };
>>
>> +#define CCI400_MCPM_PORTS_DATA (&cci400_ports)
>
> I'm a bit uneasy with the conflation of MCPM in here. Sure (most) MCPM
> backends are the only users of this code, but that doesn't mean MCPM has
> to have exclusive access. Having "MCPM" entranched into the code and
> config symbols like that is misrepresenting this code somewhat.
So, would you like to change the ARM_CCI400_MCPM as well, to something like:
ARM_CCI400_DRIVER or even ARM_CCI400_LL_DRIVER ?
>
>> +#else
>> +#define CCI400_MCPM_PORTS_DATA (NULL)
>> +#endif
>> +
>> static const struct of_device_id arm_cci_matches[] = {
>> - {.compatible = "arm,cci-400", .data = &cci400_ports },
>> +#ifdef CONFIG_ARM_CCI400_COMMON
>> + {.compatible = "arm,cci-400", .data = CCI400_MCPM_PORTS_DATA },
>> +#endif
>> {},
>> };
>>
>> -#ifdef CONFIG_HW_PERF_EVENTS
>> +#ifdef CONFIG_ARM_CCI400_PMU
>>
>> #define DRIVER_NAME "CCI-400"
>> #define DRIVER_NAME_PMU DRIVER_NAME " PMU"
>> @@ -981,6 +989,7 @@ static int cci_pmu_probe(struct platform_device *pdev)
>> if (ret)
>> return ret;
>>
>> + pr_info("ARM %s PMU driver probed", pmu->model->name);
>
> Wouldn't this addition fit better in one of the previous patches?
Yes, it could have been moved to the previous one, will fix it in the
next revision.
Thanks
Suzuki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists