On Tue, Jul 20, 2010 at 10:04 AM, Kukjin Kim <kgene.kim@xxxxxxxxxxx> wrote: > MyungJoo Ham wrote: >> >> This patches add support for powerdomain, block-gating, and flags in > struct clk. >> >> Blockgating re-uses powerdomain support scheme and depends on powerdomain >> support. >> >> Flags support is independent from powerdomain; however, powerdomain > support >> is NOT stable without flags support. Without flags support, powerdomain > may lock >> up the system with some conditions although they are rare. Thus, when >> powerdomain or block-gating is used, flags support should be turned on for > the >> system stability. (and that's why I'm sending flags support with >> powerdomain/block-gating support). >> >> Although powerdomain support is requred for blockgating, blockgating is > NOT >> required for powerdomain. Besides, powerdomain support is observed to > reduce >> power consumption significantly (with 2.6.29 kernel); however, blockgating > support >> didn't show any significant improvement. >> >> >> MyungJoo Ham (4): >> ARM: SAMSUNG SoC: Powerdomain/Block-gating Support >> ARM: S5PV210: Powerdomain/Clock-gating Support >> ARM: SAMSUNG SoC: Clock Framework: Flag Support for struct clk. >> ARM: S5PV210: Clock Framework: Flag Support for struct clk. >> >> arch/arm/mach-s5pv210/Kconfig | 17 + >> arch/arm/mach-s5pv210/clock.c | 906 >> ++++++++++++++++++++++------ >> arch/arm/plat-samsung/Kconfig | 19 + >> arch/arm/plat-samsung/clock.c | 146 +++++- >> arch/arm/plat-samsung/include/plat/clock.h | 44 ++ >> 5 files changed, 935 insertions(+), 197 deletions(-) > > Already we discussed about power domain... > And actually your code is similar with my member's 2.6.29 powerdomain > scheme. > So Ben's code review about that is available to your this patch. > > Following is from Ben Dooks note. > > === > > Powerdomain control notes > ========================= > > Copyright 2010 Simtec Electronics > Ben Dooks <ben@xxxxxxxxxxxx> > > Code Review > ----------- > > The current implementation makes a number of #ifdef based additions to > the clock.c which currently lies in plat-s3c/clock.c (but to be moved > to plat-samsung/clock.c). The following are the observations from reviewing > the code as presented: > > 1) The use of #ifdef is not recommended, as noted before in reviews it > makes the code hard to read, allows bugs to creep in due to code > either being skipped, and problems when we get to the stage several > conflicting configurations could live in the kernel. I've been using #ifdef's for enabling/disabling powerdomain and blockgating control (especially for machines that do not have support for powerdomain/blockgating). However, it seems to be reasonable that the #ifdef's are not needed and to be removed. > > 2) The code is changing the functionality of the clk_enable() and the > clk_disable() calls, which is bad for the following reasons: > > A) Anyone reading or modifying a driver using this API may not see > what is going on underneath. > > B) It ties the clock to the powerdomain, if any of the current drivers > wish to temporarily stop the clock to reduce the dynamic power > consumption they cannot (see part A, if all drivers are resting > in the power domain the whole domain may shutdown with a resulting > cost to the work resumption). > > C) It ties the drivers to the specific clock implementation and thus > restricts future use of standard drivers on future devices. > > D) It ties policy into the kernel, as it forces the power domains to > shut down if the driver is not in use. The developer or end user > may want to change this depending on either a device wide or usage > case. Policy decisions are best left out of the kernel. > Um... yes, putting powerdomain/blockgating code at clk_* code is too invasive to the clk_* code. And it'd be more problematic if we move on to the common struct clk. I'll implement powerdomain/blockgating control code at arch/arm/mach-s5pv210/clock.c:s5pv210_clk_*_ctrl. For B) and D), we may need to leave an option not to control powerdomain. How about leaving an option at sysfs if not using #ifdef's? > > Part (1) is a definite barrier to merging, the code whilst functional is > both ugly and the use of #ifdefs like that are contrary to a number of > developers beliefs. > > The second part is also a stumbling block, as it is likely to be commented > upon by a number of interested parties if it did come up for review. I am > certainly not happy to see this sort of invasive change to the clock system > merged. I expect others would also raise objections. > > ... > > === > > So need to another approach for it. > > Thanks. > > Best regards, > Kgene. > -- > Kukjin Kim <kgene.kim@xxxxxxxxxxx>, Senior Engineer, > SW Solution Development Team, Samsung Electronics Co., Ltd. > > -- MyungJoo Ham (함명주), Ph.D. Mobile Software Platform Lab, Digital Media and Communications (DMC) Business Samsung Electronics cell: 82-10-6714-2858 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |