| Message ID | 1445864143-25695-8-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26.10.2015 21:55, 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@gmail.com> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > 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 > diff --git a/arch/arm/mach-exynos/pmu.c b/drivers/soc/samsung/exynos-pmu.c > similarity index 100% > rename from arch/arm/mach-exynos/pmu.c > rename to drivers/soc/samsung/exynos-pmu.c > diff --git a/arch/arm/mach-exynos/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h 1. Please reorder the exynos_sys_powerdown_conf() to be after the statics. I am thinking also about adding EXPORT_SYMBOL... but maybe this would be over-thinking. 2. I think the proper location of everything is drivers/power/reset/. Although I don't have strong opinion. 3. Please cc linux-pm and arm-soc guys (Arnd, Olof, Kevin) on next iteration. Best regards, Krzysztof > similarity index 100% > rename from arch/arm/mach-exynos/exynos-pmu.h > rename to drivers/soc/samsung/exynos-pmu.h > diff --git a/arch/arm/mach-exynos/exynos3250-pmu.c b/drivers/soc/samsung/exynos3250-pmu.c > similarity index 100% > rename from arch/arm/mach-exynos/exynos3250-pmu.c > rename to drivers/soc/samsung/exynos3250-pmu.c > diff --git a/arch/arm/mach-exynos/exynos4-pmu.c b/drivers/soc/samsung/exynos4-pmu.c > similarity index 100% > rename from arch/arm/mach-exynos/exynos4-pmu.c > rename to drivers/soc/samsung/exynos4-pmu.c > diff --git a/arch/arm/mach-exynos/exynos5250-pmu.c b/drivers/soc/samsung/exynos5250-pmu.c > similarity index 100% > rename from arch/arm/mach-exynos/exynos5250-pmu.c > rename to drivers/soc/samsung/exynos5250-pmu.c > diff --git a/arch/arm/mach-exynos/exynos5420-pmu.c b/drivers/soc/samsung/exynos5420-pmu.c > similarity index 100% > rename from arch/arm/mach-exynos/exynos5420-pmu.c > rename to drivers/soc/samsung/exynos5420-pmu.c >
Hi Krzysztof, On Tuesday 03 November 2015 07:52 AM, Krzysztof Kozlowski wrote: > On 26.10.2015 21:55, 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@gmail.com> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> 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%) >> > > 1. Please reorder the exynos_sys_powerdown_conf() to be after the > statics. I am thinking also about adding EXPORT_SYMBOL... but maybe this > would be over-thinking. > I could not understand your point of reordering, will you please explain this. > 2. I think the proper location of everything is drivers/power/reset/. > Although I don't have strong opinion. > There has been discussion about the proper location for this driver, initial attempt was done in "drivers/mfd" folder but then we realized that this driver is not exactly fitting in MFD category. There was suggestion from Catalin Marinas [1], [2] to move it to "drivers/power" or a more suitable place other than mfd. As I received comments from Bartlomiej [3] and other members also (sorry I could not produce all links as it was quite more than a year back), I feel driver is very much SoC specific and hence decided to move it here. 1: https://lkml.org/lkml/2014/4/28/879 2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/252018.html 3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/244690.html > 3. Please cc linux-pm and arm-soc guys (Arnd, Olof, Kevin) on next > iteration. > Ok will keep them in CC in next revision. Thanks, Pankaj Dubey > Best regards, > Krzysztof >
On 05.11.2015 14:31, Pankaj Dubey wrote: > Hi Krzysztof, > > On Tuesday 03 November 2015 07:52 AM, Krzysztof Kozlowski wrote: > >> >> 1. Please reorder the exynos_sys_powerdown_conf() to be after the >> statics. I am thinking also about adding EXPORT_SYMBOL... but maybe this >> would be over-thinking. >> > > I could not understand your point of reordering, will you please explain > this. Usually static functions are put at beginning of a file and the externally linkable ones (including exportable) are at the end. This allows quick finding of what is exported by this unit. Mixing static-nonstatic-static brings confusion/ This is a driver so everything except it is static, so I see two choices: 1. Put it in front, just after pmu_context definition. 2. Put it in back, just before the probe. > >> 2. I think the proper location of everything is drivers/power/reset/. >> Although I don't have strong opinion. >> > > There has been discussion about the proper location for this driver, > initial attempt was done in "drivers/mfd" folder but then we realized > that this driver is not exactly fitting in MFD category. > There was suggestion from Catalin Marinas [1], [2] to move it to > "drivers/power" or a more suitable place other than mfd. As I received > comments from Bartlomiej [3] and other members also (sorry I could not > produce all links as it was quite more than a year back), I feel driver > is very much SoC specific and hence decided to move it here. > > 1: https://lkml.org/lkml/2014/4/28/879 > 2: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/252018.html > > 3: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/244690.html In drivers/power/reset there are already very-SoC-specific reset handlers. All of them are non-reusable outside of some SoC family. However I don't think it really matters - both locations (soc and power) seem fine to me. Looking at Bart's comments I see that you did not resolve all of them. One was left: "what happens if exynos_sys_powerdown_conf() is called while there are no platform devices binded to a driver but driver itself is loaded." Looking at the code NULL pointer exception will happen on pmu_context dereference. Best regards, Krzysztof
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