RE: [PATCH v3 1/7] ARM: S5PV210: Add a Kconfig entry S5PC110_EVT0_WORKAROUND

2010-07-19 Thread Kukjin Kim
MyungJoo Ham wrote:
 
 Early S5PC110 (EVT0) chip had some issues required workaround from a
 kernel. We can add such workaround codes with this Kconfig entry.
 
 Signed-off-by: MyungJoo Ham myungjoo@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  arch/arm/mach-s5pv210/Kconfig |7 +++
  1 files changed, 7 insertions(+), 0 deletions(-)
 
 diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
 index 631019a..18802e7 100644
 --- a/arch/arm/mach-s5pv210/Kconfig
 +++ b/arch/arm/mach-s5pv210/Kconfig
 @@ -101,4 +101,11 @@ config MACH_SMDKC110
 Machine support for Samsung SMDKC110
 S5PC110(MCP) is one of package option of S5PV210
 
 +config S5PC110_EVT0_WORKAROUND
 + bool S5PC110 Early Chip Workaround (EVT0)
 + help
 +   Early S5PC110 (so called EVT0) has errata items that should be
 +   addressed; otherwise the kernel may panic or be locked up. Enable
 +   this option to execute workaround instructions.
 +
  endif
 --

As I said earlier, EVT0 is not real chip and not for mass production.

Why do you submit the EVT0 patch which can only available for you?
It is better to not add code into mainline that is not going to be used.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim kgene@samsung.com, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/7] ARM: S5PV210: Add a Kconfig entry S5PC110_EVT0_WORKAROUND

2010-07-19 Thread Kyungmin Park
On Mon, Jul 19, 2010 at 4:59 PM, Kukjin Kim kgene@samsung.com wrote:
 MyungJoo Ham wrote:

 Early S5PC110 (EVT0) chip had some issues required workaround from a
 kernel. We can add such workaround codes with this Kconfig entry.

 Signed-off-by: MyungJoo Ham myungjoo@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  arch/arm/mach-s5pv210/Kconfig |    7 +++
  1 files changed, 7 insertions(+), 0 deletions(-)

 diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
 index 631019a..18802e7 100644
 --- a/arch/arm/mach-s5pv210/Kconfig
 +++ b/arch/arm/mach-s5pv210/Kconfig
 @@ -101,4 +101,11 @@ config MACH_SMDKC110
         Machine support for Samsung SMDKC110
         S5PC110(MCP) is one of package option of S5PV210

 +config S5PC110_EVT0_WORKAROUND
 +     bool S5PC110 Early Chip Workaround (EVT0)
 +     help
 +       Early S5PC110 (so called EVT0) has errata items that should be
 +       addressed; otherwise the kernel may panic or be locked up. Enable
 +       this option to execute workaround instructions.
 +
  endif
 --

 As I said earlier, EVT0 is not real chip and not for mass production.

 Why do you submit the EVT0 patch which can only available for you?
 It is better to not add code into mainline that is not going to be used.

Did you read the previous mail? I explain that.
The LSI focus the latest SoCs. but we got the early chip and used it.
You see the chip itself but I see the product which used the chips
whether for mass production or not.

Actually it's management and maintenance problem. you only show the
latest codes and chips to outside.
but I want to maintain our board at mainline.

Thank you,
Kyungmin Park
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/4] ARM: S5PV210: Clock Framework: powerdomain, block-gating, flags

2010-07-19 Thread MyungJoo Ham
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(-)

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/4] ARM: SAMSUNG SoC: Powerdomain/Block-gating Support

2010-07-19 Thread MyungJoo Ham
Even when all the clocks of a domain are turned off, turning off the
power for the domain saves (leakage) current. Thus, we need to control
powerdomain accordingly to save even more power when we do clock gating.

Block-gating is a similar feature with powerdomain, but, it controls
block, which is smaller than power-domain, and the saved current is
not so significant.

This patch enables powerdomain/block-gating support for Samsung SoC.

Signed-off-by: MyungJoo Ham myungjoo@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 arch/arm/plat-samsung/Kconfig  |   19 +++
 arch/arm/plat-samsung/clock.c  |   76 ++-
 arch/arm/plat-samsung/include/plat/clock.h |   32 
 3 files changed, 124 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
index bd007e3..1fddb04 100644
--- a/arch/arm/plat-samsung/Kconfig
+++ b/arch/arm/plat-samsung/Kconfig
@@ -298,4 +298,23 @@ config SAMSUNG_WAKEMASK
  and above. This code allows a set of interrupt to wakeup-mask
  mappings. See plat/wakeup-mask.h
 
+config SAMSUNG_POWERDOMAIN
+   bool Powerdomain Control Support
+   depends on CPU_S5PV210
+   select S5PV210_POWERDOMAIN
+   help
+ Compile support for powerdomain controls. This code allows
+ enabling and diabling powerdomain according to its clocks,
+ which often saves significant amount of power.
+
+config SAMSUNG_BLOCKGATING
+   bool Block Gating Control Support
+   depends on SAMSUNG_POWERDOMAIN  CPU_S5PV210
+   select S5PV210_BLOCKGATING
+   help
+ Compile support for block gating controls. This code allows
+ enabling and diabling blocks according to its clocks.
+ However, at least in S5PV210, this is not as effective as
+ powerdomain control.
+
 endif
diff --git a/arch/arm/plat-samsung/clock.c b/arch/arm/plat-samsung/clock.c
index 8bf79f3..f8a30f7 100644
--- a/arch/arm/plat-samsung/clock.c
+++ b/arch/arm/plat-samsung/clock.c
@@ -113,6 +113,10 @@ void clk_put(struct clk *clk)
 
 int clk_enable(struct clk *clk)
 {
+#if defined(CONFIG_SAMSUNG_BLOCKGATING) || defined(CONFIG_SAMSUNG_POWERDOMAIN)
+   struct clk *p;
+#endif
+
if (IS_ERR(clk) || clk == NULL)
return -EINVAL;
 
@@ -120,8 +124,38 @@ int clk_enable(struct clk *clk)
 
spin_lock(clocks_lock);
 
-   if ((clk-usage++) == 0)
-   (clk-enable)(clk, 1);
+   if ((clk-usage++) == 0) {
+#ifdef CONFIG_SAMSUNG_BLOCKGATING
+   if (clk-bd  clk-bd-ref_count == 0) {
+   if (clk-bd-pd_set)
+   clk-bd-pd_set(clk-bd, 1);
+   clk-bd-ref_count++;
+   }
+#endif
+#ifdef CONFIG_SAMSUNG_POWERDOMAIN
+   if (clk-pd) {
+   if (clk-pd-ref_count == 0 
+   clk-pd-num_clks_boot_on = 0) {
+   list_for_each_entry(p, clk-pd-clocks,
+   powerdomain_list) {
+   if (p-enable)
+   p-enable(p, 1);
+   }
+
+   clk-pd-pd_set(clk-pd, 1);
+
+   list_for_each_entry(p, clk-pd-clocks,
+   powerdomain_list) {
+   if (p-enable)
+   p-enable(p, 0);
+   }
+   }
+   clk-pd-ref_count++;
+   }
+#endif
+   if (clk-enable)
+   (clk-enable)(clk, 1);
+   }
 
spin_unlock(clocks_lock);
return 0;
@@ -134,8 +168,29 @@ void clk_disable(struct clk *clk)
 
spin_lock(clocks_lock);
 
-   if ((--clk-usage) == 0)
+   if ((--clk-usage) == 0) {
(clk-enable)(clk, 0);
+#ifdef CONFIG_SAMSUNG_POWERDOMAIN
+   if (clk-pd) {
+   if (clk-pd-ref_count == 1 
+   clk-pd-num_clks_boot_on = 0 
+   clk-pd-pd_set)
+   clk-pd-pd_set(clk-pd, 0);
+   if (clk-pd-ref_count  0)
+   clk-pd-ref_count--;
+   }
+#endif
+#ifdef CONFIG_SAMSUNG_BLOCKGATING
+   if (clk-bd) {
+   if (clk-bd-ref_count == 1 
+   clk-bd-num_clks_boot_on = 0 
+   clk-bd-pd_set)
+   clk-bd-pd_set(clk-bd, 0);
+   if (clk-bd-ref_count  0)
+   clk-bd-ref_count--;
+   }
+#endif
+   }
 
spin_unlock(clocks_lock);

[PATCH 3/4] ARM: SAMSUNG SoC: Clock Framework: Flag Support for struct clk.

2010-07-19 Thread MyungJoo Ham
Add a .flag property to the struct clk. The flag can have the following
information:

The clock is enabled at the boot time.
The clock cannot be disabled.
The clock is enabled at the boot time and cannot be disabled.
The clock is disabled at the boot time. Note that both
CLKFLAGS_BOOT_ON and CLKFLAGS_CANNOT_DISABLE can override
CLKFLAGS_BOOT_OFF.
Please stop using this clock. When a DEPRECATED clock is
clk_get'd, clk_get function will printk a warning message.

The previous patch related with powerdomain / blcok-gating control
requires this patch for the stability related with clocks that are
turned on at the boot time.

Note that clocks without both BOOT_ON and BOOT_OFF keep the previous
states; however, powerdomain / block-gating controls do NOT have any
information about the states of such clocks, which in turn, may incur
instable kernel behaviors. For example, a powerdomain may be turned off
while a clock of the domain is still being used. With the flag support
feature, we highly recommend to define .flag fields fully for clocks
related to powerdomain and block-gating. Clocks not connected to
powerdomain and block-gating are ok without flag field.

Signed-off-by: MyungJoo Ham myungjoo@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 arch/arm/plat-samsung/clock.c  |   70 ++-
 arch/arm/plat-samsung/include/plat/clock.h |   12 +
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-samsung/clock.c b/arch/arm/plat-samsung/clock.c
index f8a30f7..9ff9d63 100644
--- a/arch/arm/plat-samsung/clock.c
+++ b/arch/arm/plat-samsung/clock.c
@@ -103,6 +103,14 @@ struct clk *clk_get(struct device *dev, const char *id)
}
 
spin_unlock(clocks_lock);
+
+   if (!IS_ERR(clk)  clk)
+   if (clk-flags  CLKFLAGS_DEPRECATED)
+   printk(KERN_WARNING [%s:%d] clk %s:%d is deprecated. 
+   Please reconsider using it.\n,
+   __FILE__, __LINE__, clk-name,
+   clk-id);
+
return clk;
 }
 
@@ -169,7 +177,25 @@ void clk_disable(struct clk *clk)
spin_lock(clocks_lock);
 
if ((--clk-usage) == 0) {
-   (clk-enable)(clk, 0);
+#ifdef CONFIG_SAMSUNG_POWERDOMAIN
+   if ((clk-flags | CLKFLAGS_BOOT_ON) 
+   !(clk-flags | CLKFLAGS_CANNOT_DISABLE)) {
+   /* BOOT_ON became NO EFFECT. Let PD/BD be able
+* to turn themselves off */
+
+   if (clk-pd)
+   clk-pd-num_clks_boot_on--;
+#ifdef CONFIG_SAMSUNG_BLOCKGATING
+   if (clk-bd)
+   clk-bd-num_clks_boot_on--;
+#endif
+   clk-flags = ~CLKFLAGS_BOOT_ON;
+   }
+#endif
+
+   if (clk-enable  !(clk-flags | CLKFLAGS_CANNOT_DISABLE))
+   (clk-enable)(clk, 0);
+
 #ifdef CONFIG_SAMSUNG_POWERDOMAIN
if (clk-pd) {
if (clk-pd-ref_count == 1 
@@ -193,7 +219,9 @@ void clk_disable(struct clk *clk)
}
 
spin_unlock(clocks_lock);
-   clk_disable(clk-parent);
+
+   if (!(clk-flags | CLKFLAGS_CANNOT_DISABLE))
+   clk_disable(clk-parent);
 }
 
 
@@ -370,6 +398,13 @@ struct clk s3c24xx_uclk = {
  */
 int s3c24xx_register_clock(struct clk *clk)
 {
+   int ret = 0;
+
+   if (clk == NULL)
+   return -EINVAL;
+   if (clk-name == NULL)
+   return -EINVAL;
+
if (clk-enable == NULL)
clk-enable = clk_null_enable;
 
@@ -382,6 +417,21 @@ int s3c24xx_register_clock(struct clk *clk)
list_add(clk-list, clocks);
spin_unlock(clocks_lock);
 
+   if (clk-flags  CLKFLAGS_BOOT_ON) {
+   /* Use clk_enable, not clk-enable as clk's parent is
+* also required to be turned on */
+   clk_enable(clk);
+
+#ifdef CONFIG_SAMSUNG_POWERDOMAIN
+   if (clk-pd)
+   clk-pd-num_clks_boot_on++;
+#endif
+#ifdef CONFIG_SAMSUNG_BLOCKGATING
+   if (clk-bd)
+   clk-bd-num_clks_boot_on++;
+#endif
+   }
+
 #ifdef CONFIG_SAMSUNG_POWERDOMAIN
if (clk-pd) {
spin_lock(clocks_lock);
@@ -397,7 +447,21 @@ int s3c24xx_register_clock(struct clk *clk)
}
 #endif
 
-   return 0;
+   if (clk-flags  CLKFLAGS_BOOT_OFF) {
+   if ((clk-flags  CLKFLAGS_BOOT_ON) ||
+   (clk-flags  CLKFLAGS_CANNOT_DISABLE)) {
+   printk(KERN_WARNING [%s:%d] clk %s:%d has incompatible
+initilization flags: %x.\n,
+   __FILE__, __LINE__, clk-name, clk-id,
+   clk-flags);
+   

[PATCH 4/4] ARM: S5PV210: Clock Framework: Flag Support for struct clk.

2010-07-19 Thread MyungJoo Ham
Added .flags property to struct clk init_clocks[] and removed
init_clocks_disabled[], which became useless by BOOT_OFF bit of .flags.

Signed-off-by: MyungJoo Ham myungjoo@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 arch/arm/mach-s5pv210/clock.c |  477 +++--
 1 files changed, 270 insertions(+), 207 deletions(-)

diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
index af2af5e..bafba6f 100644
--- a/arch/arm/mach-s5pv210/clock.c
+++ b/arch/arm/mach-s5pv210/clock.c
@@ -660,8 +660,51 @@ static struct clk_ops clk_hclk_imem_ops = {
.get_rate   = s5pv210_clk_imem_get_rate,
 };
 
-static struct clk init_clocks_disable[] = {
+static struct clk init_clocks[] = {
{
+   .name   = hclk_imem,
+   .id = -1,
+   .parent = clk_hclk_msys.clk,
+   .enable = s5pv210_clk_ip0_ctrl,
+   .ctrlbit= (1  5),
+   .flags  = CLKFLAGS_ALWAYS_ON,
+   .ops= clk_hclk_imem_ops,
+   }, {
+   .name   = pdma,
+   .id = 0,
+   .parent = clk_hclk_psys.clk,
+   .enable = s5pv210_clk_ip0_ctrl,
+   .ctrlbit= (1  3),
+   .flags  = CLKFLAGS_BOOT_ON,
+   }, {
+   .name   = pdma,
+   .id = 1,
+   .parent = clk_hclk_psys.clk,
+   .enable = s5pv210_clk_ip0_ctrl,
+   .ctrlbit= (1  4),
+   .flags  = CLKFLAGS_BOOT_ON,
+   }, {
+   .name   = mdma,
+   .id = -1,
+   .parent = clk_hclk_dsys.clk,
+   .enable = s5pv210_clk_ip0_ctrl,
+   .ctrlbit= (1  2),
+   .flags  = CLKFLAGS_BOOT_OFF,
+   }, {
+   .name   = dmc,
+   .id = 0,
+   .parent = clk_hclk_msys.clk,
+   .enable = s5pv210_clk_ip0_ctrl,
+   .ctrlbit= (1  0),
+   .flags  = CLKFLAGS_ALWAYS_ON,
+   }, {
+   .name   = dmc,
+   .id = 1,
+   .parent = clk_hclk_msys.clk,
+   .enable = s5pv210_clk_ip0_ctrl,
+   .ctrlbit= (1  1),
+   .flags  = CLKFLAGS_ALWAYS_ON,
+   }, {
.name   = csis,
.id = -1,
.parent = clk_pclk_dsys.clk,
@@ -669,6 +712,7 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= (1  31),
PD_SET(cam)
BD_SET(img)
+   .flags  = CLKFLAGS_BOOT_OFF,
}, {
.name   = rot,
.id = -1,
@@ -677,6 +721,7 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= (129),
PD_SET(cam)
BD_SET(img)
+   .flags  = CLKFLAGS_BOOT_OFF,
}, {
.name   = jpeg,
.id = -1,
@@ -685,6 +730,7 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= (1  28),
PD_SET(cam)
BD_SET(img)
+   .flags  = CLKFLAGS_BOOT_OFF,
}, {
.name   = fimc,
.id = 0,
@@ -693,6 +739,7 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= (1  24),
PD_SET(cam)
BD_SET(img)
+   .flags  = CLKFLAGS_BOOT_OFF,
}, {
.name   = fimc,
.id = 1,
@@ -701,6 +748,7 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= (1  25),
PD_SET(cam)
BD_SET(img)
+   .flags  = CLKFLAGS_BOOT_OFF,
}, {
.name   = fimc,
.id = 2,
@@ -709,6 +757,15 @@ static struct clk init_clocks_disable[] = {
.ctrlbit= (1  26),
PD_SET(cam)
BD_SET(img)
+   .flags  = CLKFLAGS_BOOT_OFF,
+   }, {
+   .name   = nandxl,
+   .id = -1,
+   .parent = clk_hclk_psys.clk,
+   .enable = s5pv210_clk_ip1_ctrl,
+   .ctrlbit= (1  24),
+   BD_SET(memory)
+   .flags  = CLKFLAGS_BOOT_ON,
}, {
.name   = nfcon,
.id = -1,
@@ -716,6 +773,15 @@ static struct clk init_clocks_disable[] = {
.enable = 

Re: [PATCH v3 7/7] ARM: S5PV210: Initial CPUFREQ Support

2010-07-19 Thread Mark Brown
On Mon, Jul 19, 2010 at 02:31:23PM +0900, MyungJoo Ham wrote:
 S5PV210 CPUFREQ Support.
 
 This CPUFREQ may work without PMIC's DVS support. However, it is not
 as effective without DVS support as supposed. AVS is not supported in
 this version.
 
 Note that CLK_SRC of some clocks including ARMCLK, G3D, G2D, MFC,
 and ONEDRAM are modified directly without updating clksrc_src
 representations of the clocks.  Because CPUFREQ reverts the CLK_SRC
 to the supposed values, this does not affect the consistency as long as
 there are no other modules that modifies clock sources of ARMCLK, G3D,
 G2D, MFC, and ONEDRAM (and only CPUFREQ should have the control). As
 soon as clock framework is settled, we may update source clocks
 (.parent field) of those clocks accordingly later.
 
 
 Signed-off-by: MyungJoo Ham myungjoo@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

Still:

Acked-by: Mark Brown broo...@opensource.wolfsonmicro.com
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] ARM: SAMSUNG SoC: Clock Framework: Flag Support for struct clk.

2010-07-19 Thread Russell King - ARM Linux
On Mon, Jul 19, 2010 at 05:30:35PM +0900, MyungJoo Ham wrote:
 +#ifdef CONFIG_SAMSUNG_POWERDOMAIN
 + if ((clk-flags | CLKFLAGS_BOOT_ON) 

This is always true if CLKFLAGS_BOOT_ON is non-zero.

 + !(clk-flags | CLKFLAGS_CANNOT_DISABLE)) {

This is always false if CLKFLAGS_CANNOT_DISABLE is non-zero.

 + if (clk-enable  !(clk-flags | CLKFLAGS_CANNOT_DISABLE))

Always false...
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/4] ARM: S5PV210: Powerdomain/Clock-gating Support

2010-07-19 Thread Russell King - ARM Linux
On Mon, Jul 19, 2010 at 05:30:34PM +0900, MyungJoo Ham wrote:
 +static int powerdomain_set(struct powerdomain *pd, int enable)
 +{
 + unsigned long ctrlbit;
 + void __iomem *reg;
 + void __iomem *stable_reg;
 + unsigned long reg_dat;
 +
 + if (pd == NULL)
 + return -EINVAL;

If someone calls this function with a NULL pd argument, is it better
to ignore it, or better to use WARN_ON and get a backtrace so it can
be fixed?  I don't see much reason for this function to ever be called
with a NULL powerdomain argument.

 +
 + ctrlbit = pd-pd_ctrlbit;
 + reg = (void __iomem *)pd-pd_reg;
 + stable_reg = (void __iomem *)pd-pd_stable_reg;

Why not ensure that the element in the structure is correctly typed to
start with?

It's good practice to avoid casts whenever-possible - they hide bugs.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 03/11] USB: s3c-hsotg: Avoid overwriting contents of perodic in 'fifo'

2010-07-19 Thread Ben Dooks
In shared fifo mode (used on older SoCs) the periodic in fifo beahves
much more like a packet buffer, discarding old data when writing new
data. Avoid this by ensuring that we do not load new transactions in
when there is data sitting already in the FIFO.

Note, this may not be an observed bug, we are fixing the case that this
may happen.

Signed-off-by: Ben Dooks ben-li...@fluff.org
---
 drivers/usb/gadget/s3c-hsotg.c |   12 +++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 4a25145..354fd45 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -91,7 +91,9 @@ struct s3c_hsotg_req;
  * For periodic IN endpoints, we have fifo_size and fifo_load to try
  * and keep track of the amount of data in the periodic FIFO for each
  * of these as we don't have a status register that tells us how much
- * is in each of them.
+ * is in each of them. (note, this may actually be useless information
+ * as in shared-fifo mode periodic in acts like a single-frame packet
+ * buffer than a fifo)
  */
 struct s3c_hsotg_ep {
struct usb_ep   ep;
@@ -474,6 +476,14 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
 
size_left = S3C_DxEPTSIZ_XferSize_GET(epsize);
 
+   /* if shared fifo, we cannot write anything until the
+* previous data has been completely sent.
+*/
+   if (hs_ep-fifo_load != 0) {
+   s3c_hsotg_en_gsint(hsotg, S3C_GINTSTS_PTxFEmp);
+   return -ENOSPC;
+   }
+
dev_dbg(hsotg-dev, %s: left=%d, load=%d, fifo=%d, size %d\n,
__func__, size_left,
hs_ep-size_loaded, hs_ep-fifo_load, hs_ep-fifo_size);
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 11/11] USB: s3c-hsotg: Add support for external USB clock

2010-07-19 Thread Ben Dooks
From: Maurus Cuelenaere mcuelena...@gmail.com

The PLL that drives the USB clock supports 3 input clocks: 12, 24 and 48Mhz.
This patch adds support to the USB driver for setting the correct register bit
according to the given clock.

This depends on the following patch:
[PATCH] ARM: S3C64XX: Add USB external clock definition

Signed-off-by: Maurus Cuelenaere mcuelena...@gmail.com
Signed-off-by: Ben Dooks ben-li...@fluff.org
---
 drivers/usb/gadget/s3c-hsotg.c |   19 +++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 825b6ca..a4e0b0f 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -25,6 +25,7 @@
 #include linux/delay.h
 #include linux/io.h
 #include linux/slab.h
+#include linux/clk.h
 
 #include linux/usb/ch9.h
 #include linux/usb/gadget.h
@@ -2798,6 +2799,7 @@ static void __devinit s3c_hsotg_initep(struct s3c_hsotg 
*hsotg,
  */
 static void s3c_hsotg_otgreset(struct s3c_hsotg *hsotg)
 {
+   struct clk *xusbxti;
u32 osc;
 
writel(0, S3C_PHYPWR);
@@ -2805,6 +2807,23 @@ static void s3c_hsotg_otgreset(struct s3c_hsotg *hsotg)
 
osc = hsotg-plat-is_osc ? S3C_PHYCLK_EXT_OSC : 0;
 
+   xusbxti = clk_get(hsotg-dev, xusbxti);
+   if (xusbxti  !IS_ERR(xusbxti)) {
+   switch (clk_get_rate(xusbxti)) {
+   case 12*MHZ:
+   osc |= S3C_PHYCLK_CLKSEL_12M;
+   break;
+   case 24*MHZ:
+   osc |= S3C_PHYCLK_CLKSEL_24M;
+   break;
+   default:
+   case 48*MHZ:
+   /* default reference clock */
+   break;
+   }
+   clk_put(xusbxti);
+   }
+
writel(osc | 0x10, S3C_PHYCLK);
 
/* issue a full set of resets to the otg and core */
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 07/11] USB: s3c-hsotg: Check for new request before enqueing new setup

2010-07-19 Thread Ben Dooks
Before trying a new setup transaction after getting an EP0 in complete
interrupt, check that the driver did not try and send more EP0 IN data
before enqueing a new setup transaction.

This fixes a bug where we cannot send all of the IN data in one go
so split the transfer, but then fail to send all the data as we start
waiting for a new OUT transaction

Signed-off-by: Ben Dooks ben-li...@fluff.org
---
 drivers/usb/gadget/s3c-hsotg.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index df6a39d..10aeee1 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -1790,7 +1790,7 @@ static void s3c_hsotg_epint(struct s3c_hsotg *hsotg, 
unsigned int idx,
if (dir_in) {
s3c_hsotg_complete_in(hsotg, hs_ep);
 
-   if (idx == 0)
+   if (idx == 0  !hs_ep-req)
s3c_hsotg_enqueue_setup(hsotg);
} else if (using_dma(hsotg)) {
/* We're using DMA, we need to fire an OutDone here
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 08/11] USB: s3c-hsotg: Fix max EP0 IN request length

2010-07-19 Thread Ben Dooks
The maximum length for any EP0 IN request on EP0 is 127 bytes, not 128
as the driver currently has it.

Signed-off-by: Ben Dooks ben-li...@fluff.org
---
 drivers/usb/gadget/s3c-hsotg.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 10aeee1..1020006 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -612,8 +612,7 @@ static unsigned get_ep_limit(struct s3c_hsotg_ep *hs_ep)
maxpkt = S3C_DxEPTSIZ_PktCnt_LIMIT + 1;
} else {
if (hs_ep-dir_in) {
-   /* maxsize = S3C_DIEPTSIZ0_XferSize_LIMIT + 1; */
-   maxsize = 64+64+1;
+   maxsize = 64+64;
maxpkt = S3C_DIEPTSIZ0_PktCnt_LIMIT + 1;
} else {
maxsize = 0x3f;
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 01/11] USB: s3c-hsotg: Increase TX fifo limit

2010-07-19 Thread Ben Dooks
Up the FIFO size for the TX to 1024 entries, as this now seems to work
with all the cores. This fixes a problem when using large packets on
a core with MPS set to 512 can hang due to insufficient space for the
writes.

The hang arises due to getting the non-periodic FIFO empty IRQ but
not being able to satisfy any requests since there is never enough
space to write 512 bytes into the buffer. This means we end up with
a stream of interrupt requests.

It is easier to up the TX FIFO to fill the space we left for it
than to try and fix the positions in the code where we should have
limited the max-packet size to  TXFIFOSIZE, since the TXFIFOSIZE
depends on how the TX FIFOs have been setup.

Signed-off-by: Ben Dooks ben-li...@fluff.org
---
 drivers/usb/gadget/s3c-hsotg.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 26193ec..81f62da 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -310,11 +310,11 @@ static void s3c_hsotg_init_fifo(struct s3c_hsotg *hsotg)
hsotg-regs + S3C_GNPTXFSIZ);
*/
 
-   /* set FIFO sizes to 2048/0x1C0 */
+   /* set FIFO sizes to 2048/1024 */
 
writel(2048, hsotg-regs + S3C_GRXFSIZ);
writel(S3C_GNPTXFSIZ_NPTxFStAddr(2048) |
-  S3C_GNPTXFSIZ_NPTxFDep(0x1C0),
+  S3C_GNPTXFSIZ_NPTxFDep(1024),
   hsotg-regs + S3C_GNPTXFSIZ);
 
/* arange all the rest of the TX FIFOs, as some versions of this
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


S3C HSOTG fixes

2010-07-19 Thread Ben Dooks
Repost of the series as requested by Greg. Fixes a number of problems
with the hsotg block on Samsung devices, including FIFO reset issues,
data sizes, dealing with core compilation options, et al.

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/3] USB: s3c-hsotg: modify only selected bits in S3C_PHYPWR register

2010-07-19 Thread Marek Szyprowski
S5PV210 SoCs has 2 USB PHY interfaces, both enabled by writing zero to
S3C_PHYPWR register. HS/OTG driver uses only PHY0, so do not touch bits
related to PHY1.

Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 drivers/usb/gadget/s3c-hsotg.c |6 --
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index ce272b4..258ca01 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -2801,9 +2801,11 @@ static void __devinit s3c_hsotg_initep(struct s3c_hsotg 
*hsotg,
 static void s3c_hsotg_otgreset(struct s3c_hsotg *hsotg)
 {
struct clk *xusbxti;
-   u32 osc;
+   u32 pwr, osc;
 
-   writel(0, S3C_PHYPWR);
+   pwr = readl(S3C_PHYPWR);
+   pwr = ~0x19;
+   writel(pwr, S3C_PHYPWR);
mdelay(1);
 
osc = hsotg-plat-is_osc ? S3C_PHYCLK_EXT_OSC : 0;
-- 
1.7.1.569.g6f426

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v3 7/7] ARM: S5PV210: Initial CPUFREQ Support

2010-07-19 Thread Kukjin Kim
MyungJoo Ham wrote:
 
 S5PV210 CPUFREQ Support.
 
 This CPUFREQ may work without PMIC's DVS support. However, it is not
 as effective without DVS support as supposed. AVS is not supported in
 this version.
 
 Note that CLK_SRC of some clocks including ARMCLK, G3D, G2D, MFC,
 and ONEDRAM are modified directly without updating clksrc_src
 representations of the clocks.  Because CPUFREQ reverts the CLK_SRC
 to the supposed values, this does not affect the consistency as long as
 there are no other modules that modifies clock sources of ARMCLK, G3D,
 G2D, MFC, and ONEDRAM (and only CPUFREQ should have the control). As
 soon as clock framework is settled, we may update source clocks
 (.parent field) of those clocks accordingly later.
 
As you know memory composition such as mDDR, (mDDR, OneDRAM), DDR2 and so on
differs in MCP type of each S5PC110, and single type of S5PV210.
So basically, this code can available only for some S5PC110 MCP types...only
some machines.
It means can occur problem on some boards...if you use ARCH dependency
configuration...
And need to add comment about tested boards.

 
 Signed-off-by: MyungJoo Ham myungjoo@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 --
 v2:
   - Ramp-up delay is removed. (let regulator framework do the job)
   - Provide proper max values for regulator_set_voltage
   - Removed unneccesary #ifdef's.
   - Removed unnecessary initialiser for CLK_OUT
 v3:
   - Style corrections (pr_info/pr_err, ...)
   - Revised dvs_conf struct
 
 ---
  arch/arm/Kconfig  |1 +
  arch/arm/mach-s5pv210/Makefile|3 +
  arch/arm/mach-s5pv210/cpufreq-s5pv210.c   |  766
 +
  arch/arm/mach-s5pv210/include/mach/cpu-freq.h |   51 ++
  4 files changed, 821 insertions(+), 0 deletions(-)
  create mode 100644 arch/arm/mach-s5pv210/cpufreq-s5pv210.c
  create mode 100644 arch/arm/mach-s5pv210/include/mach/cpu-freq.h
 
 diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
 index 98922f7..d5a5916 100644
 --- a/arch/arm/Kconfig
 +++ b/arch/arm/Kconfig
 @@ -701,6 +701,7 @@ config ARCH_S5PV210
   select HAVE_CLK
   select ARM_L1_CACHE_SHIFT_6
   select ARCH_USES_GETTIMEOFFSET
 + select ARCH_HAS_CPUFREQ
   help
 Samsung S5PV210/S5PC110 series based systems
 
 diff --git a/arch/arm/mach-s5pv210/Makefile
b/arch/arm/mach-s5pv210/Makefile
 index aae592a..293dbac 100644
 --- a/arch/arm/mach-s5pv210/Makefile
 +++ b/arch/arm/mach-s5pv210/Makefile
 @@ -34,3 +34,6 @@ obj-$(CONFIG_S5PV210_SETUP_I2C2)+= setup-i2c2.o
  obj-$(CONFIG_S5PV210_SETUP_KEYPAD)   += setup-keypad.o
  obj-$(CONFIG_S5PV210_SETUP_SDHCI)   += setup-sdhci.o
  obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO)   += setup-sdhci-gpio.o
 +
 +# CPUFREQ (DVFS)

No need above comment.

 +obj-$(CONFIG_CPU_FREQ)   += cpufreq-s5pv210.o

As I said, should be having machine dependency configuration...

 diff --git a/arch/arm/mach-s5pv210/cpufreq-s5pv210.c b/arch/arm/mach-
 s5pv210/cpufreq-s5pv210.c
 new file mode 100644
 index 000..38de3ac
 --- /dev/null
 +++ b/arch/arm/mach-s5pv210/cpufreq-s5pv210.c

'cpufreq.c' is enough.

 @@ -0,0 +1,766 @@
 +/*  linux/arch/arm/plat-s5pc11x/s5pc11x-cpufreq.c

Please correct above comment.

 + *
 + *  Copyright (C) 2010 Samsung Electronics Co., Ltd.
 + *
 + *  CPU frequency scaling for S5PC110
 + *  Based on cpu-sa1110.c
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + */

Please add empty line here like others.

 +#include linux/types.h
 +#include linux/kernel.h
 +#include linux/sched.h
 +#include linux/delay.h
 +#include linux/init.h
 +#include linux/err.h
 +#include linux/clk.h
 +#include linux/io.h
 +#include linux/regulator/consumer.h
 +#include linux/gpio.h
 +#include asm/system.h
 +
 +#include mach/hardware.h
 +#include mach/map.h
 +#include mach/regs-clock.h
 +#include mach/regs-gpio.h
 +#include mach/cpu-freq.h
 +
 +#include plat/cpu-freq.h
 +#include plat/pll.h
 +#include plat/clock.h
 +#include plat/gpio-cfg.h
 +#include plat/regs-fb.h
 +#ifdef CONFIG_PM
 +#include plat/pm.h
 +#endif
 +
 +static struct clk *mpu_clk;
 +static struct regulator *arm_regulator;
 +static struct regulator *internal_regulator;
 +
 +struct s3c_cpufreq_freqs s3c_freqs;
 +
 +#ifdef CONFIG_S5PC110_EVT0_WORKAROUND

As I said, please don't add EVT0 code in here.

 +#define  CPUFREQ_DISABLE_1GHZ
 +#endif
 +/* #define   CPUFREQ_DISABLE_100MHZ */
 +
 +static unsigned long previous_arm_volt;
 +
 +/* frequency */
 +static struct cpufreq_frequency_table s5pv210_freq_table[] = {
 + {L0, 1000*1000},
 + {L1, 800*1000},
 + {L2, 400*1000},
 + {L3, 200*1000},
 + {L4, 100*1000},
 + {0, CPUFREQ_TABLE_END},
 +};
 +
 +struct s5pv210_dvs_conf {
 + unsigned long   arm_volt;   /* uV */
 + unsigned long 

RE: [PATCH 0/4] ARM: S5PV210: Clock Framework: powerdomain, block-gating, flags

2010-07-19 Thread Kukjin Kim
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 b...@simtec.co.uk

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.

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.


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@samsung.com, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 0/4] ARM: S5PV210: Clock Framework: powerdomain, block-gating, flags

2010-07-19 Thread MyungJoo Ham
On Tue, Jul 20, 2010 at 10:04 AM, Kukjin Kim kgene@samsung.com 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 b...@simtec.co.uk

 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@samsung.com, 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