[PATCH] Platform fixes for s5p-fimc

[PATCH] Platform fixes for s5p-fimc

[Marek Szyprowski]

Hello,

This patch series fixes a bunch of bugs in s5p-fimc platform support.
Mainly it adds missing clocks, dma coherent masks and clock setup code
required on Aquila and Goni targets.

This is a pure bugfix series to get fimc driver working. I hope to get
it merged for 2.6.36-rc4.

The complete list of patches:

[PATCH 1/4] ARM: S5PC110: add fimc clocks.
[PATCH 2/4] ARM: Samsung: s5p-fimc: fix dma coherent mask
[PATCH 3/4] ARM: S5PC110: add common FIMC setup code
[PATCH 4/4] ARM: S5PC110: use common FIMC clock setup code

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 1/4] ARM: S5PC110: add fimc clocks.

[Marek Szyprowski]

These clocks enables FIMC driver to operate on machines, which bootloader
power gated FIMC devices to save power on boot.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv210/clock.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
index af91fef..9c199ef 100644
--- a/arch/arm/mach-s5pv210/clock.c
+++ b/arch/arm/mach-s5pv210/clock.c
@@ -281,6 +281,24 @@ static struct clk init_clocks_disable[] = {
 		.enable		= s5pv210_clk_ip0_ctrl,
 		.ctrlbit	= (1<<29),
 	}, {
+		.name		= "fimc",
+		.id		= 0,
+		.parent		= &clk_hclk_dsys.clk,
+		.enable		= s5pv210_clk_ip0_ctrl,
+		.ctrlbit	= (1 << 24),
+	}, {
+		.name		= "fimc",
+		.id		= 1,
+		.parent		= &clk_hclk_dsys.clk,
+		.enable		= s5pv210_clk_ip0_ctrl,
+		.ctrlbit	= (1 << 25),
+	}, {
+		.name		= "fimc",
+		.id		= 2,
+		.parent		= &clk_hclk_dsys.clk,
+		.enable		= s5pv210_clk_ip0_ctrl,
+		.ctrlbit	= (1 << 26),
+	}, {
 		.name		= "otg",
 		.id		= -1,
 		.parent		= &clk_hclk_psys.clk,
-- 
1.7.2.2

[PATCH 1/4] ARM: S5PC110: add fimc clocks.

[Kukjin Kim]

Marek Szyprowski wrote:
> 
> These clocks enables FIMC driver to operate on machines, which bootloader
> power gated FIMC devices to save power on boot.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-s5pv210/clock.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> index af91fef..9c199ef 100644
> --- a/arch/arm/mach-s5pv210/clock.c
> +++ b/arch/arm/mach-s5pv210/clock.c
> @@ -281,6 +281,24 @@ static struct clk init_clocks_disable[] = {
>  		.enable		= s5pv210_clk_ip0_ctrl,
>  		.ctrlbit	= (1<<29),
>  	}, {
> +		.name		= "fimc",
> +		.id		= 0,
> +		.parent		= &clk_hclk_dsys.clk,
> +		.enable		= s5pv210_clk_ip0_ctrl,
> +		.ctrlbit	= (1 << 24),
> +	}, {
> +		.name		= "fimc",
> +		.id		= 1,
> +		.parent		= &clk_hclk_dsys.clk,
> +		.enable		= s5pv210_clk_ip0_ctrl,
> +		.ctrlbit	= (1 << 25),
> +	}, {
> +		.name		= "fimc",
> +		.id		= 2,
> +		.parent		= &clk_hclk_dsys.clk,
> +		.enable		= s5pv210_clk_ip0_ctrl,
> +		.ctrlbit	= (1 << 26),
> +	}, {
>  		.name		= "otg",
>  		.id		= -1,
>  		.parent		= &clk_hclk_psys.clk,
> --
> 1.7.2.2

Ok..will apply.

Thanks.

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

[PATCH 2/4] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Marek Szyprowski]

FIMC driver uses DMA_coherent allocator, which requires proper dma mask
to be set.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/plat-s5p/dev-fimc0.c |    2 ++
 arch/arm/plat-s5p/dev-fimc1.c |    2 ++
 arch/arm/plat-s5p/dev-fimc2.c |    2 ++
 3 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-fimc0.c
index d3f1a9b..9f26096 100644
--- a/arch/arm/plat-s5p/dev-fimc0.c
+++ b/arch/arm/plat-s5p/dev-fimc0.c
@@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc0 = {
 	.id		= 0,
 	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
 	.resource	= s5p_fimc0_resource,
+	.dev.dma_mask	  = &s5p_device_fimc0.dev.coherent_dma_mask,
+	.dev.coherent_dma_mask = 0xffffffffUL,
 };
diff --git a/arch/arm/plat-s5p/dev-fimc1.c b/arch/arm/plat-s5p/dev-fimc1.c
index 41bd698..4149c49 100644
--- a/arch/arm/plat-s5p/dev-fimc1.c
+++ b/arch/arm/plat-s5p/dev-fimc1.c
@@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc1 = {
 	.id		= 1,
 	.num_resources	= ARRAY_SIZE(s5p_fimc1_resource),
 	.resource	= s5p_fimc1_resource,
+	.dev.dma_mask	  = &s5p_device_fimc1.dev.coherent_dma_mask,
+	.dev.coherent_dma_mask = 0xffffffffUL,
 };
diff --git a/arch/arm/plat-s5p/dev-fimc2.c b/arch/arm/plat-s5p/dev-fimc2.c
index dfddeda..ced9097 100644
--- a/arch/arm/plat-s5p/dev-fimc2.c
+++ b/arch/arm/plat-s5p/dev-fimc2.c
@@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc2 = {
 	.id		= 2,
 	.num_resources	= ARRAY_SIZE(s5p_fimc2_resource),
 	.resource	= s5p_fimc2_resource,
+	.dev.dma_mask	  = &s5p_device_fimc2.dev.coherent_dma_mask,
+	.dev.coherent_dma_mask = 0xffffffffUL,
 };
-- 
1.7.2.2

[PATCH 2/4] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Kukjin Kim]

Marek Szyprowski wrote:
> 
> FIMC driver uses DMA_coherent allocator, which requires proper dma mask
> to be set.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/plat-s5p/dev-fimc0.c |    2 ++
>  arch/arm/plat-s5p/dev-fimc1.c |    2 ++
>  arch/arm/plat-s5p/dev-fimc2.c |    2 ++
>  3 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-fimc0.c
> index d3f1a9b..9f26096 100644
> --- a/arch/arm/plat-s5p/dev-fimc0.c
> +++ b/arch/arm/plat-s5p/dev-fimc0.c
> @@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc0 = {
>  	.id		= 0,
>  	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
>  	.resource	= s5p_fimc0_resource,
> +	.dev.dma_mask	  = &s5p_device_fimc0.dev.coherent_dma_mask,
> +	.dev.coherent_dma_mask = 0xffffffffUL,

Ok...but I prefer following style :-)
Could you please re-submit your this patch?

diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-fimc0.c
index d3f1a9b..cb1ff4d 100644
--- a/arch/arm/plat-s5p/dev-fimc0.c
+++ b/arch/arm/plat-s5p/dev-fimc0.c
@@ -28,9 +28,15 @@ static struct resource s5p_fimc0_resource[] = {
        },
 };

+static u64 s5p_fimc_dma_mask = DMA_BIT_MASK(32);
+
 struct platform_device s5p_device_fimc0 = {
 	.name		= "s5p-fimc",
 	.id		= 0,
 	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
 	.resource	= s5p_fimc0_resource,
+	.dev		= {
+		.dma_mask		= &s5p_fimc_dma_mask,
+		.coherent_dma_mask	= DMA_BIT_MASK(32),
+	},
 };

>  };
> diff --git a/arch/arm/plat-s5p/dev-fimc1.c b/arch/arm/plat-s5p/dev-fimc1.c
> index 41bd698..4149c49 100644
> --- a/arch/arm/plat-s5p/dev-fimc1.c
> +++ b/arch/arm/plat-s5p/dev-fimc1.c
> @@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc1 = {
>  	.id		= 1,
>  	.num_resources	= ARRAY_SIZE(s5p_fimc1_resource),
>  	.resource	= s5p_fimc1_resource,
> +	.dev.dma_mask	  = &s5p_device_fimc1.dev.coherent_dma_mask,
> +	.dev.coherent_dma_mask = 0xffffffffUL,
>  };
> diff --git a/arch/arm/plat-s5p/dev-fimc2.c b/arch/arm/plat-s5p/dev-fimc2.c
> index dfddeda..ced9097 100644
> --- a/arch/arm/plat-s5p/dev-fimc2.c
> +++ b/arch/arm/plat-s5p/dev-fimc2.c
> @@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc2 = {
>  	.id		= 2,
>  	.num_resources	= ARRAY_SIZE(s5p_fimc2_resource),
>  	.resource	= s5p_fimc2_resource,
> +	.dev.dma_mask	  = &s5p_device_fimc2.dev.coherent_dma_mask,
> +	.dev.coherent_dma_mask = 0xffffffffUL,
>  };
> --
> 1.7.2.2

Thanks.

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

[PATCH 2/4] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Marek Szyprowski]

W dniu 2010-09-09 09:41, Kukjin Kim pisze:
> Marek Szyprowski wrote:
>>
>> FIMC driver uses DMA_coherent allocator, which requires proper dma mask
>> to be set.
>>
>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   arch/arm/plat-s5p/dev-fimc0.c |    2 ++
>>   arch/arm/plat-s5p/dev-fimc1.c |    2 ++
>>   arch/arm/plat-s5p/dev-fimc2.c |    2 ++
>>   3 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-fimc0.c
>> index d3f1a9b..9f26096 100644
>> --- a/arch/arm/plat-s5p/dev-fimc0.c
>> +++ b/arch/arm/plat-s5p/dev-fimc0.c
>> @@ -33,4 +33,6 @@ struct platform_device s5p_device_fimc0 = {
>>   	.id		= 0,
>>   	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
>>   	.resource	= s5p_fimc0_resource,
>> +	.dev.dma_mask	  =&s5p_device_fimc0.dev.coherent_dma_mask,
>> +	.dev.coherent_dma_mask = 0xffffffffUL,
>
> Ok...but I prefer following style :-)
> Could you please re-submit your this patch?

OK, I will update the style and resubmit the patch.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH v2] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Marek Szyprowski]

FIMC driver uses DMA_coherent allocator, which requires proper dma mask
to be set.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Chnages since v1:
- use DMA_BIT_MASK() macro, minor coding style change

---
 arch/arm/plat-s5p/dev-fimc0.c |    7 +++++++
 arch/arm/plat-s5p/dev-fimc1.c |    7 +++++++
 arch/arm/plat-s5p/dev-fimc2.c |    7 +++++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-fimc0.c
index d3f1a9b..f9203fa 100644
--- a/arch/arm/plat-s5p/dev-fimc0.c
+++ b/arch/arm/plat-s5p/dev-fimc0.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
@@ -28,9 +29,15 @@ static struct resource s5p_fimc0_resource[] = {
 	},
 };
 
+static u64 s5p_fimc0_dma_mask = DMA_BIT_MASK(32);
+
 struct platform_device s5p_device_fimc0 = {
 	.name		= "s5p-fimc",
 	.id		= 0,
 	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
 	.resource	= s5p_fimc0_resource,
+	.dev		= {
+		.dma_mask		= &s5p_fimc0_dma_mask,
+		.coherent_dma_mask	= DMA_BIT_MASK(32),
+	},
 };
diff --git a/arch/arm/plat-s5p/dev-fimc1.c b/arch/arm/plat-s5p/dev-fimc1.c
index 41bd698..7ed91a3 100644
--- a/arch/arm/plat-s5p/dev-fimc1.c
+++ b/arch/arm/plat-s5p/dev-fimc1.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
@@ -28,9 +29,15 @@ static struct resource s5p_fimc1_resource[] = {
 	},
 };
 
+static u64 s5p_fimc1_dma_mask = DMA_BIT_MASK(32);
+
 struct platform_device s5p_device_fimc1 = {
 	.name		= "s5p-fimc",
 	.id		= 1,
 	.num_resources	= ARRAY_SIZE(s5p_fimc1_resource),
 	.resource	= s5p_fimc1_resource,
+	.dev		= {
+		.dma_mask		= &s5p_fimc1_dma_mask,
+		.coherent_dma_mask	= DMA_BIT_MASK(32),
+	},
 };
diff --git a/arch/arm/plat-s5p/dev-fimc2.c b/arch/arm/plat-s5p/dev-fimc2.c
index dfddeda..5543d6373 100644
--- a/arch/arm/plat-s5p/dev-fimc2.c
+++ b/arch/arm/plat-s5p/dev-fimc2.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
@@ -28,9 +29,15 @@ static struct resource s5p_fimc2_resource[] = {
 	},
 };
 
+static u64 s5p_fimc2_dma_mask = DMA_BIT_MASK(32);
+
 struct platform_device s5p_device_fimc2 = {
 	.name		= "s5p-fimc",
 	.id		= 2,
 	.num_resources	= ARRAY_SIZE(s5p_fimc2_resource),
 	.resource	= s5p_fimc2_resource,
+	.dev		= {
+		.dma_mask		= &s5p_fimc2_dma_mask,
+		.coherent_dma_mask	= DMA_BIT_MASK(32),
+	},
 };
-- 
1.7.2.2

[PATCH v2] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Kukjin Kim]

Marek Szyprowski wrote:
> 
> FIMC driver uses DMA_coherent allocator, which requires proper dma mask
> to be set.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 
> Chnages since v1:
> - use DMA_BIT_MASK() macro, minor coding style change
> 
> ---
>  arch/arm/plat-s5p/dev-fimc0.c |    7 +++++++
>  arch/arm/plat-s5p/dev-fimc1.c |    7 +++++++
>  arch/arm/plat-s5p/dev-fimc2.c |    7 +++++++
>  3 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-fimc0.c
> index d3f1a9b..f9203fa 100644
> --- a/arch/arm/plat-s5p/dev-fimc0.c
> +++ b/arch/arm/plat-s5p/dev-fimc0.c
> @@ -10,6 +10,7 @@
>   */
> 
>  #include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
> @@ -28,9 +29,15 @@ static struct resource s5p_fimc0_resource[] = {
>  	},
>  };
> 
> +static u64 s5p_fimc0_dma_mask = DMA_BIT_MASK(32);
> +
>  struct platform_device s5p_device_fimc0 = {
>  	.name		= "s5p-fimc",
>  	.id		= 0,
>  	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
>  	.resource	= s5p_fimc0_resource,
> +	.dev		= {
> +		.dma_mask		= &s5p_fimc0_dma_mask,
> +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> +	},
>  };
> diff --git a/arch/arm/plat-s5p/dev-fimc1.c b/arch/arm/plat-s5p/dev-fimc1.c
> index 41bd698..7ed91a3 100644
> --- a/arch/arm/plat-s5p/dev-fimc1.c
> +++ b/arch/arm/plat-s5p/dev-fimc1.c
> @@ -10,6 +10,7 @@
>   */
> 
>  #include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
> @@ -28,9 +29,15 @@ static struct resource s5p_fimc1_resource[] = {
>  	},
>  };
> 
> +static u64 s5p_fimc1_dma_mask = DMA_BIT_MASK(32);
> +
>  struct platform_device s5p_device_fimc1 = {
>  	.name		= "s5p-fimc",
>  	.id		= 1,
>  	.num_resources	= ARRAY_SIZE(s5p_fimc1_resource),
>  	.resource	= s5p_fimc1_resource,
> +	.dev		= {
> +		.dma_mask		= &s5p_fimc1_dma_mask,
> +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> +	},
>  };
> diff --git a/arch/arm/plat-s5p/dev-fimc2.c b/arch/arm/plat-s5p/dev-fimc2.c
> index dfddeda..5543d6373 100644
> --- a/arch/arm/plat-s5p/dev-fimc2.c
> +++ b/arch/arm/plat-s5p/dev-fimc2.c
> @@ -10,6 +10,7 @@
>   */
> 
>  #include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
> @@ -28,9 +29,15 @@ static struct resource s5p_fimc2_resource[] = {
>  	},
>  };
> 
> +static u64 s5p_fimc2_dma_mask = DMA_BIT_MASK(32);
> +
>  struct platform_device s5p_device_fimc2 = {
>  	.name		= "s5p-fimc",
>  	.id		= 2,
>  	.num_resources	= ARRAY_SIZE(s5p_fimc2_resource),
>  	.resource	= s5p_fimc2_resource,
> +	.dev		= {
> +		.dma_mask		= &s5p_fimc2_dma_mask,
> +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> +	},
>  };
> --
> 1.7.2.2

looks good to me..will apply :-)

Thanks.

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

[PATCH v2] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Marek Szyprowski]

Hello,

On Thursday, September 09, 2010 1:20 PM Kukjin Kim wrote:

> Marek Szyprowski wrote:
> >
> > FIMC driver uses DMA_coherent allocator, which requires proper dma mask
> > to be set.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >
> > Chnages since v1:
> > - use DMA_BIT_MASK() macro, minor coding style change
> >
> > ---
> >  arch/arm/plat-s5p/dev-fimc0.c |    7 +++++++
> >  arch/arm/plat-s5p/dev-fimc1.c |    7 +++++++
> >  arch/arm/plat-s5p/dev-fimc2.c |    7 +++++++
> >  3 files changed, 21 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-
> fimc0.c
> > index d3f1a9b..f9203fa 100644
> > --- a/arch/arm/plat-s5p/dev-fimc0.c
> > +++ b/arch/arm/plat-s5p/dev-fimc0.c
> > @@ -10,6 +10,7 @@
> >   */
> >
> >  #include <linux/kernel.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/ioport.h>
> > @@ -28,9 +29,15 @@ static struct resource s5p_fimc0_resource[] = {
> >  	},
> >  };
> >
> > +static u64 s5p_fimc0_dma_mask = DMA_BIT_MASK(32);
> > +
> >  struct platform_device s5p_device_fimc0 = {
> >  	.name		= "s5p-fimc",
> >  	.id		= 0,
> >  	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
> >  	.resource	= s5p_fimc0_resource,
> > +	.dev		= {
> > +		.dma_mask		= &s5p_fimc0_dma_mask,
> > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > +	},
> >  };
> > diff --git a/arch/arm/plat-s5p/dev-fimc1.c b/arch/arm/plat-s5p/dev-
> fimc1.c
> > index 41bd698..7ed91a3 100644
> > --- a/arch/arm/plat-s5p/dev-fimc1.c
> > +++ b/arch/arm/plat-s5p/dev-fimc1.c
> > @@ -10,6 +10,7 @@
> >   */
> >
> >  #include <linux/kernel.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/ioport.h>
> > @@ -28,9 +29,15 @@ static struct resource s5p_fimc1_resource[] = {
> >  	},
> >  };
> >
> > +static u64 s5p_fimc1_dma_mask = DMA_BIT_MASK(32);
> > +
> >  struct platform_device s5p_device_fimc1 = {
> >  	.name		= "s5p-fimc",
> >  	.id		= 1,
> >  	.num_resources	= ARRAY_SIZE(s5p_fimc1_resource),
> >  	.resource	= s5p_fimc1_resource,
> > +	.dev		= {
> > +		.dma_mask		= &s5p_fimc1_dma_mask,
> > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > +	},
> >  };
> > diff --git a/arch/arm/plat-s5p/dev-fimc2.c b/arch/arm/plat-s5p/dev-
> fimc2.c
> > index dfddeda..5543d6373 100644
> > --- a/arch/arm/plat-s5p/dev-fimc2.c
> > +++ b/arch/arm/plat-s5p/dev-fimc2.c
> > @@ -10,6 +10,7 @@
> >   */
> >
> >  #include <linux/kernel.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/ioport.h>
> > @@ -28,9 +29,15 @@ static struct resource s5p_fimc2_resource[] = {
> >  	},
> >  };
> >
> > +static u64 s5p_fimc2_dma_mask = DMA_BIT_MASK(32);
> > +
> >  struct platform_device s5p_device_fimc2 = {
> >  	.name		= "s5p-fimc",
> >  	.id		= 2,
> >  	.num_resources	= ARRAY_SIZE(s5p_fimc2_resource),
> >  	.resource	= s5p_fimc2_resource,
> > +	.dev		= {
> > +		.dma_mask		= &s5p_fimc2_dma_mask,
> > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > +	},
> >  };
> > --
> > 1.7.2.2
> 
> looks good to me..will apply :-)

What about merging this patch also to the kgene/s5p-fixes-for-linus branch?
IMHO this and the fimc clocks patch are pure bug fixes that should be merged
into final 2.6.36 kernel. Without these two patches fimc driver is useless.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH v2] ARM: Samsung: s5p-fimc: fix dma coherent mask

[Kukjin Kim]

Marek Szyprowski wrote:
> 
> Hello,
> 
Hi ;-)

> On Thursday, September 09, 2010 1:20 PM Kukjin Kim wrote:
> 
> > Marek Szyprowski wrote:
> > >
> > > FIMC driver uses DMA_coherent allocator, which requires proper dma
mask
> > > to be set.
> > >
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > ---
> > >
> > > Chnages since v1:
> > > - use DMA_BIT_MASK() macro, minor coding style change
> > >
> > > ---
> > >  arch/arm/plat-s5p/dev-fimc0.c |    7 +++++++
> > >  arch/arm/plat-s5p/dev-fimc1.c |    7 +++++++
> > >  arch/arm/plat-s5p/dev-fimc2.c |    7 +++++++
> > >  3 files changed, 21 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/arm/plat-s5p/dev-fimc0.c b/arch/arm/plat-s5p/dev-
> > fimc0.c
> > > index d3f1a9b..f9203fa 100644
> > > --- a/arch/arm/plat-s5p/dev-fimc0.c
> > > +++ b/arch/arm/plat-s5p/dev-fimc0.c
> > > @@ -10,6 +10,7 @@
> > >   */
> > >
> > >  #include <linux/kernel.h>
> > > +#include <linux/dma-mapping.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/ioport.h>
> > > @@ -28,9 +29,15 @@ static struct resource s5p_fimc0_resource[] = {
> > >  	},
> > >  };
> > >
> > > +static u64 s5p_fimc0_dma_mask = DMA_BIT_MASK(32);
> > > +
> > >  struct platform_device s5p_device_fimc0 = {
> > >  	.name		= "s5p-fimc",
> > >  	.id		= 0,
> > >  	.num_resources	= ARRAY_SIZE(s5p_fimc0_resource),
> > >  	.resource	= s5p_fimc0_resource,
> > > +	.dev		= {
> > > +		.dma_mask		= &s5p_fimc0_dma_mask,
> > > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > > +	},
> > >  };
> > > diff --git a/arch/arm/plat-s5p/dev-fimc1.c b/arch/arm/plat-s5p/dev-
> > fimc1.c
> > > index 41bd698..7ed91a3 100644
> > > --- a/arch/arm/plat-s5p/dev-fimc1.c
> > > +++ b/arch/arm/plat-s5p/dev-fimc1.c
> > > @@ -10,6 +10,7 @@
> > >   */
> > >
> > >  #include <linux/kernel.h>
> > > +#include <linux/dma-mapping.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/ioport.h>
> > > @@ -28,9 +29,15 @@ static struct resource s5p_fimc1_resource[] = {
> > >  	},
> > >  };
> > >
> > > +static u64 s5p_fimc1_dma_mask = DMA_BIT_MASK(32);
> > > +
> > >  struct platform_device s5p_device_fimc1 = {
> > >  	.name		= "s5p-fimc",
> > >  	.id		= 1,
> > >  	.num_resources	= ARRAY_SIZE(s5p_fimc1_resource),
> > >  	.resource	= s5p_fimc1_resource,
> > > +	.dev		= {
> > > +		.dma_mask		= &s5p_fimc1_dma_mask,
> > > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > > +	},
> > >  };
> > > diff --git a/arch/arm/plat-s5p/dev-fimc2.c b/arch/arm/plat-s5p/dev-
> > fimc2.c
> > > index dfddeda..5543d6373 100644
> > > --- a/arch/arm/plat-s5p/dev-fimc2.c
> > > +++ b/arch/arm/plat-s5p/dev-fimc2.c
> > > @@ -10,6 +10,7 @@
> > >   */
> > >
> > >  #include <linux/kernel.h>
> > > +#include <linux/dma-mapping.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/ioport.h>
> > > @@ -28,9 +29,15 @@ static struct resource s5p_fimc2_resource[] = {
> > >  	},
> > >  };
> > >
> > > +static u64 s5p_fimc2_dma_mask = DMA_BIT_MASK(32);
> > > +
> > >  struct platform_device s5p_device_fimc2 = {
> > >  	.name		= "s5p-fimc",
> > >  	.id		= 2,
> > >  	.num_resources	= ARRAY_SIZE(s5p_fimc2_resource),
> > >  	.resource	= s5p_fimc2_resource,
> > > +	.dev		= {
> > > +		.dma_mask		= &s5p_fimc2_dma_mask,
> > > +		.coherent_dma_mask	= DMA_BIT_MASK(32),
> > > +	},
> > >  };
> > > --
> > > 1.7.2.2
> >
> > looks good to me..will apply :-)
> 
> What about merging this patch also to the kgene/s5p-fixes-for-linus
branch?
> IMHO this and the fimc clocks patch are pure bug fixes that should be
merged
> into final 2.6.36 kernel. Without these two patches fimc driver is
useless.
> 
Hmm...ok...will sort out with other fixes.

Thanks.

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

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Szyprowski]

Add common clocks setup code for FIMC devices on S5PV210 SoCs.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv210/Kconfig             |    6 ++++
 arch/arm/mach-s5pv210/Makefile            |    1 +
 arch/arm/mach-s5pv210/setup-fimc.c        |   46 +++++++++++++++++++++++++++++
 arch/arm/plat-samsung/include/plat/fimc.h |   24 +++++++++++++++
 4 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-s5pv210/setup-fimc.c
 create mode 100644 arch/arm/plat-samsung/include/plat/fimc.h

diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index d3a3895..48489bb 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -37,6 +37,12 @@ config S5PV210_SETUP_FB_24BPP
 	help
           Common setup code for S5PV210 with an 24bpp RGB display helper.
 
+config S5PV210_SETUP_FIMC
+	bool
+	help
+	  Compile common code for S5PV210 based machines to setup correct
+	  FIMC clock parameters.
+
 config S5PV210_SETUP_KEYPAD
 	bool
 	help
diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
index 05048c5..c13aef1 100644
--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
 obj-$(CONFIG_S5PC110_DEV_ONENAND) += dev-onenand.o
 
 obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
+obj-$(CONFIG_S5PV210_SETUP_FIMC)	+= setup-fimc.o
 obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
 obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
 obj-$(CONFIG_S5PV210_SETUP_IDE)		+= setup-ide.o
diff --git a/arch/arm/mach-s5pv210/setup-fimc.c b/arch/arm/mach-s5pv210/setup-fimc.c
new file mode 100644
index 0000000..80c1ffe
--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-fimc.c
@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2010 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * S5PV210 clock setup code for FIMC devices
+ *
+ * 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.
+ */
+#include <linux/device.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <plat/devs.h>
+#include <plat/fimc.h>
+
+int __init s5pv210_fimc_setup_clks(void)
+{
+	int err = 0;
+	int i;
+	struct clk *clk_fimc, *parent;
+
+	struct device *fimc_devs[] = {
+		&s5p_device_fimc0.dev,
+		&s5p_device_fimc1.dev,
+		&s5p_device_fimc2.dev
+	};
+
+	parent = clk_get(NULL, "mout_epll");
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	for (i = 0; err == 0 && i < ARRAY_SIZE(fimc_devs); i++) {
+		if (fimc_devs[i]) {
+			clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
+			if (IS_ERR(clk_fimc)) {
+				err = PTR_ERR(clk_fimc);
+				break;
+			}
+			clk_set_parent(clk_fimc, parent);
+			clk_put(clk_fimc);
+		}
+	}
+	clk_put(parent);
+	return err;
+}
diff --git a/arch/arm/plat-samsung/include/plat/fimc.h b/arch/arm/plat-samsung/include/plat/fimc.h
new file mode 100644
index 0000000..2c06f37
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/fimc.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2010 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * Common FIMC devices definitions and helper functions
+ *
+ * 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.
+*/
+
+#ifndef __PLAT_SAMSUNG_FIMC_H
+#define __PLAT_SAMSUNG_FIMC_H __FILE__
+
+/**
+ * s5pv210_fimc_setup_clks() - S5PV210/S5PC110 fimc clocks setup function
+ *
+ * Set correct parent clocks on machines which bootloaded did not configured
+ * fimc clocks correctly. FIMC devices works properly only if sourced from
+ * certain clock sources. "mout_epll" clock is the recommended one.
+ */
+extern int s5pv210_fimc_setup_clks(void);
+
+#endif /* __PLAT_SAMSUNG_FIMC_H */
-- 
1.7.2.2

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Vasut]

Dne Po 6. z??? 2010 05:50:43 Marek Szyprowski napsal(a):
> Add common clocks setup code for FIMC devices on S5PV210 SoCs.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-s5pv210/Kconfig             |    6 ++++
>  arch/arm/mach-s5pv210/Makefile            |    1 +
>  arch/arm/mach-s5pv210/setup-fimc.c        |   46
> +++++++++++++++++++++++++++++ arch/arm/plat-samsung/include/plat/fimc.h | 
>  24 +++++++++++++++
>  4 files changed, 77 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-s5pv210/setup-fimc.c
>  create mode 100644 arch/arm/plat-samsung/include/plat/fimc.h
> 
> diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
> index d3a3895..48489bb 100644
> --- a/arch/arm/mach-s5pv210/Kconfig
> +++ b/arch/arm/mach-s5pv210/Kconfig
> @@ -37,6 +37,12 @@ config S5PV210_SETUP_FB_24BPP
>  	help
>            Common setup code for S5PV210 with an 24bpp RGB display helper.
> 
> +config S5PV210_SETUP_FIMC
> +	bool
> +	help
> +	  Compile common code for S5PV210 based machines to setup correct
> +	  FIMC clock parameters.
> +
>  config S5PV210_SETUP_KEYPAD
>  	bool
>  	help
> diff --git a/arch/arm/mach-s5pv210/Makefile
> b/arch/arm/mach-s5pv210/Makefile index 05048c5..c13aef1 100644
> --- a/arch/arm/mach-s5pv210/Makefile
> +++ b/arch/arm/mach-s5pv210/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
>  obj-$(CONFIG_S5PC110_DEV_ONENAND) += dev-onenand.o
> 
>  obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
> +obj-$(CONFIG_S5PV210_SETUP_FIMC)	+= setup-fimc.o
>  obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
>  obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
>  obj-$(CONFIG_S5PV210_SETUP_IDE)		+= setup-ide.o
> diff --git a/arch/arm/mach-s5pv210/setup-fimc.c
> b/arch/arm/mach-s5pv210/setup-fimc.c new file mode 100644
> index 0000000..80c1ffe
> --- /dev/null
> +++ b/arch/arm/mach-s5pv210/setup-fimc.c
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com/
> + *
> + * S5PV210 clock setup code for FIMC devices
> + *
> + * 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.
> + */
> +#include <linux/device.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <plat/devs.h>
> +#include <plat/fimc.h>
> +
> +int __init s5pv210_fimc_setup_clks(void)
> +{
> +	int err = 0;
> +	int i;
> +	struct clk *clk_fimc, *parent;
> +
> +	struct device *fimc_devs[] = {
> +		&s5p_device_fimc0.dev,
> +		&s5p_device_fimc1.dev,
> +		&s5p_device_fimc2.dev
> +	};
> +
> +	parent = clk_get(NULL, "mout_epll");
> +	if (IS_ERR(parent))
> +		return PTR_ERR(parent);
> +
> +	for (i = 0; err == 0 && i < ARRAY_SIZE(fimc_devs); i++) {
> +		if (fimc_devs[i]) {
> +			clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
> +			if (IS_ERR(clk_fimc)) {
> +				err = PTR_ERR(clk_fimc);

I believe you should clk_put() the clocks that were already clk_get()-ed here.

Cheers
> +				break;
> +			}
> +			clk_set_parent(clk_fimc, parent);
> +			clk_put(clk_fimc);
> +		}
> +	}
> +	clk_put(parent);
> +	return err;
> +}
> diff --git a/arch/arm/plat-samsung/include/plat/fimc.h
> b/arch/arm/plat-samsung/include/plat/fimc.h new file mode 100644
> index 0000000..2c06f37
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/fimc.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com/
> + *
> + * Common FIMC devices definitions and helper functions
> + *
> + * 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.
> +*/
> +
> +#ifndef __PLAT_SAMSUNG_FIMC_H
> +#define __PLAT_SAMSUNG_FIMC_H __FILE__
> +
> +/**
> + * s5pv210_fimc_setup_clks() - S5PV210/S5PC110 fimc clocks setup function
> + *
> + * Set correct parent clocks on machines which bootloaded did not
> configured + * fimc clocks correctly. FIMC devices works properly only if
> sourced from + * certain clock sources. "mout_epll" clock is the
> recommended one. + */
> +extern int s5pv210_fimc_setup_clks(void);
> +
> +#endif /* __PLAT_SAMSUNG_FIMC_H */

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Szyprowski]

Hello,

On 2010-09-06 13:16 Marek Vasut wrote:

> Dne Po 6. z??? 2010 05:50:43 Marek Szyprowski napsal(a):
>> Add common clocks setup code for FIMC devices on S5PV210 SoCs.
>>
>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   arch/arm/mach-s5pv210/Kconfig             |    6 ++++
>>   arch/arm/mach-s5pv210/Makefile            |    1 +
>>   arch/arm/mach-s5pv210/setup-fimc.c        |   46
>> +++++++++++++++++++++++++++++ arch/arm/plat-samsung/include/plat/fimc.h |
>>   24 +++++++++++++++
>>   4 files changed, 77 insertions(+), 0 deletions(-)
>>   create mode 100644 arch/arm/mach-s5pv210/setup-fimc.c
>>   create mode 100644 arch/arm/plat-samsung/include/plat/fimc.h
>>
>> diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
>> index d3a3895..48489bb 100644
>> --- a/arch/arm/mach-s5pv210/Kconfig
>> +++ b/arch/arm/mach-s5pv210/Kconfig
>> @@ -37,6 +37,12 @@ config S5PV210_SETUP_FB_24BPP
>>   	help
>>             Common setup code for S5PV210 with an 24bpp RGB display helper.
>>
>> +config S5PV210_SETUP_FIMC
>> +	bool
>> +	help
>> +	  Compile common code for S5PV210 based machines to setup correct
>> +	  FIMC clock parameters.
>> +
>>   config S5PV210_SETUP_KEYPAD
>>   	bool
>>   	help
>> diff --git a/arch/arm/mach-s5pv210/Makefile
>> b/arch/arm/mach-s5pv210/Makefile index 05048c5..c13aef1 100644
>> --- a/arch/arm/mach-s5pv210/Makefile
>> +++ b/arch/arm/mach-s5pv210/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
>>   obj-$(CONFIG_S5PC110_DEV_ONENAND) += dev-onenand.o
>>
>>   obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
>> +obj-$(CONFIG_S5PV210_SETUP_FIMC)	+= setup-fimc.o
>>   obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
>>   obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
>>   obj-$(CONFIG_S5PV210_SETUP_IDE)		+= setup-ide.o
>> diff --git a/arch/arm/mach-s5pv210/setup-fimc.c
>> b/arch/arm/mach-s5pv210/setup-fimc.c new file mode 100644
>> index 0000000..80c1ffe
>> --- /dev/null
>> +++ b/arch/arm/mach-s5pv210/setup-fimc.c
>> @@ -0,0 +1,46 @@
>> +/*
>> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
>> + *		http://www.samsung.com/
>> + *
>> + * S5PV210 clock setup code for FIMC devices
>> + *
>> + * 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.
>> + */
>> +#include<linux/device.h>
>> +#include<linux/clk.h>
>> +#include<linux/err.h>
>> +#include<plat/devs.h>
>> +#include<plat/fimc.h>
>> +
>> +int __init s5pv210_fimc_setup_clks(void)
>> +{
>> +	int err = 0;
>> +	int i;
>> +	struct clk *clk_fimc, *parent;
>> +
>> +	struct device *fimc_devs[] = {
>> +		&s5p_device_fimc0.dev,
>> +		&s5p_device_fimc1.dev,
>> +		&s5p_device_fimc2.dev
>> +	};
>> +
>> +	parent = clk_get(NULL, "mout_epll");
>> +	if (IS_ERR(parent))
>> +		return PTR_ERR(parent);
>> +
>> +	for (i = 0; err == 0&&  i<  ARRAY_SIZE(fimc_devs); i++) {
>> +		if (fimc_devs[i]) {
>> +			clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
>> +			if (IS_ERR(clk_fimc)) {
>> +				err = PTR_ERR(clk_fimc);
>
> I believe you should clk_put() the clocks that were already clk_get()-ed here.

They are, see clk_put after clk_set_parent.

> Cheers
>> +				break;
>> +			}
>> +			clk_set_parent(clk_fimc, parent);
>> +			clk_put(clk_fimc);
>> +		}
>> +	}
>> +	clk_put(parent);
>> +	return err;
>> +}
>> diff --git a/arch/arm/plat-samsung/include/plat/fimc.h
>> b/arch/arm/plat-samsung/include/plat/fimc.h new file mode 100644
>> index 0000000..2c06f37
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/include/plat/fimc.h
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
>> + *		http://www.samsung.com/
>> + *
>> + * Common FIMC devices definitions and helper functions
>> + *
>> + * 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.
>> +*/
>> +
>> +#ifndef __PLAT_SAMSUNG_FIMC_H
>> +#define __PLAT_SAMSUNG_FIMC_H __FILE__
>> +
>> +/**
>> + * s5pv210_fimc_setup_clks() - S5PV210/S5PC110 fimc clocks setup function
>> + *
>> + * Set correct parent clocks on machines which bootloaded did not
>> configured + * fimc clocks correctly. FIMC devices works properly only if
>> sourced from + * certain clock sources. "mout_epll" clock is the
>> recommended one. + */
>> +extern int s5pv210_fimc_setup_clks(void);
>> +
>> +#endif /* __PLAT_SAMSUNG_FIMC_H */
>

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Vasut]

Dne Po 6. z??? 2010 06:34:28 Marek Szyprowski napsal(a):
> Hello,
> 
> On 2010-09-06 13:16 Marek Vasut wrote:
> > Dne Po 6. z??? 2010 05:50:43 Marek Szyprowski napsal(a):
> >> Add common clocks setup code for FIMC devices on S5PV210 SoCs.
> >> 
> >> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> ---
> >> 
> >>   arch/arm/mach-s5pv210/Kconfig             |    6 ++++
> >>   arch/arm/mach-s5pv210/Makefile            |    1 +
> >>   arch/arm/mach-s5pv210/setup-fimc.c        |   46
> >> 
> >> +++++++++++++++++++++++++++++ arch/arm/plat-samsung/include/plat/fimc.h
> >> |
> >> 
> >>   24 +++++++++++++++
> >>   4 files changed, 77 insertions(+), 0 deletions(-)
> >>   create mode 100644 arch/arm/mach-s5pv210/setup-fimc.c
> >>   create mode 100644 arch/arm/plat-samsung/include/plat/fimc.h
> >> 
> >> diff --git a/arch/arm/mach-s5pv210/Kconfig
> >> b/arch/arm/mach-s5pv210/Kconfig index d3a3895..48489bb 100644
> >> --- a/arch/arm/mach-s5pv210/Kconfig
> >> +++ b/arch/arm/mach-s5pv210/Kconfig
> >> @@ -37,6 +37,12 @@ config S5PV210_SETUP_FB_24BPP
> >> 
> >>   	help
> >>   	
> >>             Common setup code for S5PV210 with an 24bpp RGB display
> >>             helper.
> >> 
> >> +config S5PV210_SETUP_FIMC
> >> +	bool
> >> +	help
> >> +	  Compile common code for S5PV210 based machines to setup correct
> >> +	  FIMC clock parameters.
> >> +
> >> 
> >>   config S5PV210_SETUP_KEYPAD
> >>   
> >>   	bool
> >>   	help
> >> 
> >> diff --git a/arch/arm/mach-s5pv210/Makefile
> >> b/arch/arm/mach-s5pv210/Makefile index 05048c5..c13aef1 100644
> >> --- a/arch/arm/mach-s5pv210/Makefile
> >> +++ b/arch/arm/mach-s5pv210/Makefile
> >> @@ -29,6 +29,7 @@ obj-$(CONFIG_S3C64XX_DEV_SPI)	+= dev-spi.o
> >> 
> >>   obj-$(CONFIG_S5PC110_DEV_ONENAND) += dev-onenand.o
> >>   
> >>   obj-$(CONFIG_S5PV210_SETUP_FB_24BPP)	+= setup-fb-24bpp.o
> >> 
> >> +obj-$(CONFIG_S5PV210_SETUP_FIMC)	+= setup-fimc.o
> >> 
> >>   obj-$(CONFIG_S5PV210_SETUP_I2C1) 	+= setup-i2c1.o
> >>   obj-$(CONFIG_S5PV210_SETUP_I2C2) 	+= setup-i2c2.o
> >>   obj-$(CONFIG_S5PV210_SETUP_IDE)		+= setup-ide.o
> >> 
> >> diff --git a/arch/arm/mach-s5pv210/setup-fimc.c
> >> b/arch/arm/mach-s5pv210/setup-fimc.c new file mode 100644
> >> index 0000000..80c1ffe
> >> --- /dev/null
> >> +++ b/arch/arm/mach-s5pv210/setup-fimc.c
> >> @@ -0,0 +1,46 @@
> >> +/*
> >> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> >> + *		http://www.samsung.com/
> >> + *
> >> + * S5PV210 clock setup code for FIMC devices
> >> + *
> >> + * 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.
> >> + */
> >> +#include<linux/device.h>
> >> +#include<linux/clk.h>
> >> +#include<linux/err.h>
> >> +#include<plat/devs.h>
> >> +#include<plat/fimc.h>
> >> +
> >> +int __init s5pv210_fimc_setup_clks(void)
> >> +{
> >> +	int err = 0;
> >> +	int i;
> >> +	struct clk *clk_fimc, *parent;
> >> +
> >> +	struct device *fimc_devs[] = {
> >> +		&s5p_device_fimc0.dev,
> >> +		&s5p_device_fimc1.dev,
> >> +		&s5p_device_fimc2.dev
> >> +	};
> >> +
> >> +	parent = clk_get(NULL, "mout_epll");
> >> +	if (IS_ERR(parent))
> >> +		return PTR_ERR(parent);
> >> +
> >> +	for (i = 0; err == 0&&  i<  ARRAY_SIZE(fimc_devs); i++) {
> >> +		if (fimc_devs[i]) {
> >> +			clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
> >> +			if (IS_ERR(clk_fimc)) {
> >> +				err = PTR_ERR(clk_fimc);
> > 
> > I believe you should clk_put() the clocks that were already clk_get()-ed
> > here.

Ah, please ignore my stupid comment then, I missed this one.
> 
> They are, see clk_put after clk_set_parent.
> 
> > Cheers
> > 
> >> +				break;

btw. can the array fimc_devs[] be sparse ? So you'd replace this break; with 
continue; ?

Well ... it's ok :)

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

> >> +			}
> >> +			clk_set_parent(clk_fimc, parent);
> >> +			clk_put(clk_fimc);
> >> +		}
> >> +	}
> >> +	clk_put(parent);
> >> +	return err;
> >> +}
> >> diff --git a/arch/arm/plat-samsung/include/plat/fimc.h
> >> b/arch/arm/plat-samsung/include/plat/fimc.h new file mode 100644
> >> index 0000000..2c06f37
> >> --- /dev/null
> >> +++ b/arch/arm/plat-samsung/include/plat/fimc.h
> >> @@ -0,0 +1,24 @@
> >> +/*
> >> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> >> + *		http://www.samsung.com/
> >> + *
> >> + * Common FIMC devices definitions and helper functions
> >> + *
> >> + * 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.
> >> +*/
> >> +
> >> +#ifndef __PLAT_SAMSUNG_FIMC_H
> >> +#define __PLAT_SAMSUNG_FIMC_H __FILE__
> >> +
> >> +/**
> >> + * s5pv210_fimc_setup_clks() - S5PV210/S5PC110 fimc clocks setup
> >> function + *
> >> + * Set correct parent clocks on machines which bootloaded did not
> >> configured + * fimc clocks correctly. FIMC devices works properly only
> >> if sourced from + * certain clock sources. "mout_epll" clock is the
> >> recommended one. + */
> >> +extern int s5pv210_fimc_setup_clks(void);
> >> +
> >> +#endif /* __PLAT_SAMSUNG_FIMC_H */
> 
> Best regards

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Jassi Brar]

On Mon, Sep 6, 2010 at 12:50 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
....
> + ? ? ? parent = clk_get(NULL, "mout_epll");
> + ? ? ? if (IS_ERR(parent))
> + ? ? ? ? ? ? ? return PTR_ERR(parent);
> +
> + ? ? ? for (i = 0; err == 0 && i < ARRAY_SIZE(fimc_devs); i++) {
> + ? ? ? ? ? ? ? if (fimc_devs[i]) {
> + ? ? ? ? ? ? ? ? ? ? ? clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
> + ? ? ? ? ? ? ? ? ? ? ? if (IS_ERR(clk_fimc)) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? err = PTR_ERR(clk_fimc);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? clk_set_parent(clk_fimc, parent);
> + ? ? ? ? ? ? ? ? ? ? ? clk_put(clk_fimc);
> + ? ? ? ? ? ? ? }
> + ? ? ? }

The sclk_fimc could source clock from a number of options out of a mux.
mout_epll is just one of them. Different machines may want to source the
clock differently.
So, IMO the parent selection should not be done in platform code, but rather
in machine init code.
Not the best, but a better solution can be found in
arch/arm/mach-s5pc100/dev-spi.c
Give it a thought.

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Szyprowski]

Hello,

On 2010-09-06 13:52, Jassi Brar wrote:
> On Mon, Sep 6, 2010 at 12:50 PM, Marek Szyprowski
> <m.szyprowski@samsung.com>  wrote:
> ....
>> +       parent = clk_get(NULL, "mout_epll");
>> +       if (IS_ERR(parent))
>> +               return PTR_ERR(parent);
>> +
>> +       for (i = 0; err == 0&&  i<  ARRAY_SIZE(fimc_devs); i++) {
>> +               if (fimc_devs[i]) {
>> +                       clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
>> +                       if (IS_ERR(clk_fimc)) {
>> +                               err = PTR_ERR(clk_fimc);
>> +                               break;
>> +                       }
>> +                       clk_set_parent(clk_fimc, parent);
>> +                       clk_put(clk_fimc);
>> +               }
>> +       }
>
> The sclk_fimc could source clock from a number of options out of a mux.
> mout_epll is just one of them. Different machines may want to source the
> clock differently.

Right, I forgot about this case.

> So, IMO the parent selection should not be done in platform code, but rather
> in machine init code.
> Not the best, but a better solution can be found in
> arch/arm/mach-s5pc100/dev-spi.c
> Give it a thought.

I'm thinking of making the parent clock an argument to the 
s5pv210_fimc_setup_clks().

I really don't like the idea of passing clock name through the platform 
data and letting driver to mess with clock's parents. Machine startup 
code is the last place where such things should be changed.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Jassi Brar]

On Mon, Sep 6, 2010 at 5:46 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On 2010-09-06 13:52, Jassi Brar wrote:
>>
>> On Mon, Sep 6, 2010 at 12:50 PM, Marek Szyprowski
>> <m.szyprowski@samsung.com> ?wrote:
>> ....
>>>
>>> + ? ? ? parent = clk_get(NULL, "mout_epll");
>>> + ? ? ? if (IS_ERR(parent))
>>> + ? ? ? ? ? ? ? return PTR_ERR(parent);
>>> +
>>> + ? ? ? for (i = 0; err == 0&& ?i< ?ARRAY_SIZE(fimc_devs); i++) {
>>> + ? ? ? ? ? ? ? if (fimc_devs[i]) {
>>> + ? ? ? ? ? ? ? ? ? ? ? clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
>>> + ? ? ? ? ? ? ? ? ? ? ? if (IS_ERR(clk_fimc)) {
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? err = PTR_ERR(clk_fimc);
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>>> + ? ? ? ? ? ? ? ? ? ? ? }
>>> + ? ? ? ? ? ? ? ? ? ? ? clk_set_parent(clk_fimc, parent);
>>> + ? ? ? ? ? ? ? ? ? ? ? clk_put(clk_fimc);
>>> + ? ? ? ? ? ? ? }
>>> + ? ? ? }
>>
>> The sclk_fimc could source clock from a number of options out of a mux.
>> mout_epll is just one of them. Different machines may want to source the
>> clock differently.
>
> Right, I forgot about this case.
>
>> So, IMO the parent selection should not be done in platform code, but
>> rather
>> in machine init code.
>> Not the best, but a better solution can be found in
>> arch/arm/mach-s5pc100/dev-spi.c
>> Give it a thought.
>
> I'm thinking of making the parent clock an argument to the
> s5pv210_fimc_setup_clks().
Yes, that's better since the relevant clock is managed by the CMU

> I really don't like the idea of passing clock name through the platform data
> and letting driver to mess with clock's parents.
In case of SPI the clock mux and scalar is present _within_ the SPI
controller and having to touch SPI regs from outside the driver isn't
what I prefer.

> Machine startup code is the
> last place where such things should be changed.
Until I am enlightened, I'd like to think otherwise.
I think the board designer would already have thought out the clock sourcing
hierarchy. Setting appropriate parents once at boot-time and having drivers
not worry about it, should be better.

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Szyprowski]

Hello,

On 2010-09-06 18:17, Jassi Brar wrote:

>> I'm thinking of making the parent clock an argument to the
>> s5pv210_fimc_setup_clks().
> Yes, that's better since the relevant clock is managed by the CMU

What do you mean by the CMU? This function is intended to be called from 
board startup code.

>> I really don't like the idea of passing clock name through the platform data
>> and letting driver to mess with clock's parents.
> In case of SPI the clock mux and scalar is present _within_ the SPI
> controller and having to touch SPI regs from outside the driver isn't
> what I prefer.

I know. It is the same case as with SDHCI and UART controllers. I have 
an idea how to solve this in a bit more cleaner way. I hope to post a 
proposition soon.

>> Machine startup code is the
>> last place where such things should be changed.
> Until I am enlightened, I'd like to think otherwise.
> I think the board designer would already have thought out the clock sourcing
> hierarchy. Setting appropriate parents once at boot-time and having drivers
> not worry about it, should be better.

Definitely, but in our case kernel the default fimc_sclk parent points 
to non-existing clock.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Jassi Brar]

On Mon, Sep 6, 2010 at 7:21 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On 2010-09-06 18:17, Jassi Brar wrote:
>
>>> I'm thinking of making the parent clock an argument to the
>>> s5pv210_fimc_setup_clks().
>>
>> Yes, that's better since the relevant clock is managed by the CMU
>
> What do you mean by the CMU? This function is intended to be called from
> board startup code.
Clock-Management-Unit block of the SoC.

>>> I really don't like the idea of passing clock name through the platform
>>> data
>>> and letting driver to mess with clock's parents.
>>
>> In case of SPI the clock mux and scalar is present _within_ the SPI
>> controller and having to touch SPI regs from outside the driver isn't
>> what I prefer.
>
> I know. It is the same case as with SDHCI and UART controllers. I have an
> idea how to solve this in a bit more cleaner way. I hope to post a
> proposition soon.
I'll be waiting.


>>> Machine startup code is the
>>> last place where such things should be changed.
>>
>> Until I am enlightened, I'd like to think otherwise.
>> I think the board designer would already have thought out the clock
>> sourcing
>> hierarchy. Setting appropriate parents once at boot-time and having
>> drivers
>> not worry about it, should be better.
>
> Definitely, but in our case kernel the default fimc_sclk parent points to
> non-existing clock.
You mean the default value in mux field of some CMU register points to
absent source
of clock on the board?
Yes, that's why it's machine specific and should be setup by the board
init code.
Rather we should never count upon defaults and always set parents explicitly.

[PATCH 3/4] ARM: S5PC110: add common FIMC setup code

[Marek Szyprowski]

Hello,

On 2010-09-06 13:52, Jassi Brar wrote:
> On Mon, Sep 6, 2010 at 12:50 PM, Marek Szyprowski
> <m.szyprowski@samsung.com>  wrote:
> ....
>> +       parent = clk_get(NULL, "mout_epll");
>> +       if (IS_ERR(parent))
>> +               return PTR_ERR(parent);
>> +
>> +       for (i = 0; err == 0&&  i<  ARRAY_SIZE(fimc_devs); i++) {
>> +               if (fimc_devs[i]) {
>> +                       clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
>> +                       if (IS_ERR(clk_fimc)) {
>> +                               err = PTR_ERR(clk_fimc);
>> +                               break;
>> +                       }
>> +                       clk_set_parent(clk_fimc, parent);
>> +                       clk_put(clk_fimc);
>> +               }
>> +       }
>
> The sclk_fimc could source clock from a number of options out of a mux.
> mout_epll is just one of them. Different machines may want to source the
> clock differently.

Right, I forgot about this case.

> So, IMO the parent selection should not be done in platform code, but rather
> in machine init code.
> Not the best, but a better solution can be found in
> arch/arm/mach-s5pc100/dev-spi.c
> Give it a thought.

I'm thinking of making the parent clock an argument to the 
s5pv210_fimc_setup_clks().

I really don't like the idea of passing clock name through the platform 
data and letting driver to mess with clock's parents. Machine startup 
code is the last place where such things should be changed.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 4/4] ARM: S5PC110: use common FIMC clock setup code

[Marek Szyprowski]

On Aquila and GONI the bootloader doesn't set FIMC parent clocks
correctly. Use common FIMC setup code to fix this during machne boot.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-s5pv210/Kconfig       |    2 ++
 arch/arm/mach-s5pv210/mach-aquila.c |    3 +++
 arch/arm/mach-s5pv210/mach-goni.c   |    2 ++
 3 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 48489bb..7b5e816 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -79,6 +79,7 @@ config MACH_AQUILA
 	select S3C_DEV_HSMMC2
 	select S5PC110_DEV_ONENAND
 	select S5PV210_SETUP_FB_24BPP
+	select S5PV210_SETUP_FIMC
 	select S5PV210_SETUP_SDHCI
 	help
 	  Machine support for the Samsung Aquila target based on S5PC110 SoC
@@ -96,6 +97,7 @@ config MACH_GONI
 	select S3C_DEV_HSMMC2
 	select S5PC110_DEV_ONENAND
 	select S5PV210_SETUP_FB_24BPP
+	select S5PV210_SETUP_FIMC
 	select S5PV210_SETUP_SDHCI
 	help
 	  Machine support for Samsung GONI board
diff --git a/arch/arm/mach-s5pv210/mach-aquila.c b/arch/arm/mach-s5pv210/mach-aquila.c
index 0dda801..65f616b 100644
--- a/arch/arm/mach-s5pv210/mach-aquila.c
+++ b/arch/arm/mach-s5pv210/mach-aquila.c
@@ -35,7 +35,9 @@
 #include <plat/devs.h>
 #include <plat/cpu.h>
 #include <plat/fb.h>
+#include <plat/fimc.h>
 #include <plat/fimc-core.h>
+
 #include <plat/sdhci.h>
 
 /* Following are default values for UCON, ULCON and UFCON UART registers */
@@ -510,6 +512,7 @@ static void __init aquila_machine_init(void)
 	s3c_fb_set_platdata(&aquila_lcd_pdata);
 
 	platform_add_devices(aquila_devices, ARRAY_SIZE(aquila_devices));
+	s5pv210_fimc_setup_clks();
 }
 
 MACHINE_START(AQUILA, "Aquila")
diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-s5pv210/mach-goni.c
index 53754d7..9f3b52d 100644
--- a/arch/arm/mach-s5pv210/mach-goni.c
+++ b/arch/arm/mach-s5pv210/mach-goni.c
@@ -35,6 +35,7 @@
 #include <plat/devs.h>
 #include <plat/cpu.h>
 #include <plat/fb.h>
+#include <plat/fimc.h>
 #include <plat/sdhci.h>
 
 /* Following are default values for UCON, ULCON and UFCON UART registers */
@@ -487,6 +488,7 @@ static void __init goni_machine_init(void)
 	s3c_fb_set_platdata(&goni_lcd_pdata);
 
 	platform_add_devices(goni_devices, ARRAY_SIZE(goni_devices));
+	s5pv210_fimc_setup_clks();
 }
 
 MACHINE_START(GONI, "GONI")
-- 
1.7.2.2