On Wed, 25 Feb 2015, Suzuki K. Poulose wrote:
> On 24/02/15 22:17, Nicolas Pitre wrote:
> > On Tue, 24 Feb 2015, Suzuki K. Poulose wrote:
> >
> > > From: "Suzuki K. Poulose" <suzuki.poulose@xxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > Cc: Kukjin Kim <kgene@xxxxxxxxxx>
> > > Cc: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
> > > Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > > Signed-off-by: Suzuki K. Poulose <suzuki.poulose@xxxxxxx>
> >
> > 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 ?
No. I'm perfectly fine with those platforms needing it for proper
operation to explicitly select this. I don't see much value in having
this user configurable.
> >
> > 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.
OK. Please add a note to that effect in the commit log.
> >
> > > 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 ?
That would make more sense. Or even ARM_CCI400_PORT_CTRL or the like.
Nicolas
--
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
