[U-Boot] [PATCH] video, ipu: make ldb clock frequenz overwriteable through board code

[U-Boot] [PATCH] video, ipu: make ldb clock frequenz overwriteable through board code

[Heiko Schocher]

the ldb clock can be setup in board code (for example set through PLL5).
Update the ldb_clock rate also through board code.

This should be removed, if a clock framework is availiable.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 drivers/video/ipu.h        |  1 +
 drivers/video/ipu_common.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/video/ipu.h b/drivers/video/ipu.h
index 091b58f..f13934f 100644
--- a/drivers/video/ipu.h
+++ b/drivers/video/ipu.h
@@ -266,4 +266,5 @@ void ipu_dp_uninit(ipu_channel_t channel);
 void ipu_dp_dc_disable(ipu_channel_t channel, unsigned char swap);
 ipu_color_space_t format_to_colorspace(uint32_t fmt);
 
+int ipu_set_ldb_clock(int rate);
 #endif
diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c
index 1a209d4..dc054bc 100644
--- a/drivers/video/ipu_common.c
+++ b/drivers/video/ipu_common.c
@@ -1198,3 +1198,14 @@ ipu_color_space_t format_to_colorspace(uint32_t fmt)
 	}
 	return RGB;
 }
+
+/* should removed when clk framework is availiable */
+int ipu_set_ldb_clock(int rate)
+{
+	if (g_ldb_clk == NULL)
+		return -ENOENT;
+
+	g_ldb_clk->rate = rate;
+
+	return 0;
+}
-- 
2.1.0

[U-Boot] [PATCH] video, ipu: make ldb clock frequenz overwriteable through board code

[Eric Nelson]

Hi Heiko,

On 04/12/2015 01:19 AM, Heiko Schocher wrote:
> the ldb clock can be setup in board code (for example set through PLL5).
> Update the ldb_clock rate also through board code.
> 
> This should be removed, if a clock framework is availiable.
> 
Any chance you're up to the task?

Searching for 'clk_get' in the sources shows a proliferation
of attempts for various SOCs and drivers.

> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> 
>  drivers/video/ipu.h        |  1 +
>  drivers/video/ipu_common.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/video/ipu.h b/drivers/video/ipu.h
> index 091b58f..f13934f 100644
> --- a/drivers/video/ipu.h
> +++ b/drivers/video/ipu.h
> @@ -266,4 +266,5 @@ void ipu_dp_uninit(ipu_channel_t channel);
>  void ipu_dp_dc_disable(ipu_channel_t channel, unsigned char swap);
>  ipu_color_space_t format_to_colorspace(uint32_t fmt);
>  

I'd rather see this in imx-common/video.h to avoid #includes
out of the driver tree.

> +int ipu_set_ldb_clock(int rate);
>  #endif
> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c
> index 1a209d4..dc054bc 100644
> --- a/drivers/video/ipu_common.c
> +++ b/drivers/video/ipu_common.c
> @@ -1198,3 +1198,14 @@ ipu_color_space_t format_to_colorspace(uint32_t fmt)
>  	}
>  	return RGB;
>  }
> +
> +/* should removed when clk framework is availiable */
> +int ipu_set_ldb_clock(int rate)
> +{

This forces a tight dependency on when this is called that
using ldb_clk directly would prevent.

> +	if (g_ldb_clk == NULL)
> +		return -ENOENT;
> +
> +	g_ldb_clk->rate = rate;
> +
> +	return 0;
> +}
> 

The use of g_ldb_clk seems pretty dodgy everywhere in the
IPU driver.

Otherwise:

Tested-by: Eric Nelson <eric.nelson@boundarydevices.com>

[U-Boot] [PATCH] video, ipu: make ldb clock frequenz overwriteable through board code

[Heiko Schocher]

Hello Eric,

Am 16.04.2015 02:27, schrieb Eric Nelson:
> Hi Heiko,
>
> On 04/12/2015 01:19 AM, Heiko Schocher wrote:
>> the ldb clock can be setup in board code (for example set through PLL5).
>> Update the ldb_clock rate also through board code.
>>
>> This should be removed, if a clock framework is availiable.
>>
> Any chance you're up to the task?
>
> Searching for 'clk_get' in the sources shows a proliferation
> of attempts for various SOCs and drivers.

Yes ... but it seems, I get no time for such a job :-(

>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>
>>   drivers/video/ipu.h        |  1 +
>>   drivers/video/ipu_common.c | 11 +++++++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/video/ipu.h b/drivers/video/ipu.h
>> index 091b58f..f13934f 100644
>> --- a/drivers/video/ipu.h
>> +++ b/drivers/video/ipu.h
>> @@ -266,4 +266,5 @@ void ipu_dp_uninit(ipu_channel_t channel);
>>   void ipu_dp_dc_disable(ipu_channel_t channel, unsigned char swap);
>>   ipu_color_space_t format_to_colorspace(uint32_t fmt);
>>
>
> I'd rather see this in imx-common/video.h to avoid #includes
> out of the driver tree.

ok, moved.

>> +int ipu_set_ldb_clock(int rate);
>>   #endif
>> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c
>> index 1a209d4..dc054bc 100644
>> --- a/drivers/video/ipu_common.c
>> +++ b/drivers/video/ipu_common.c
>> @@ -1198,3 +1198,14 @@ ipu_color_space_t format_to_colorspace(uint32_t fmt)
>>   	}
>>   	return RGB;
>>   }
>> +
>> +/* should removed when clk framework is availiable */
>> +int ipu_set_ldb_clock(int rate)
>> +{
>
> This forces a tight dependency on when this is called that
> using ldb_clk directly would prevent.

Sorry, did not understand you here ...

>> +	if (g_ldb_clk == NULL)
>> +		return -ENOENT;
>> +
>> +	g_ldb_clk->rate = rate;
>> +
>> +	return 0;
>> +}
>>
>
> The use of g_ldb_clk seems pretty dodgy everywhere in the
> IPU driver.
>
> Otherwise:
>
> Tested-by: Eric Nelson <eric.nelson@boundarydevices.com>

Thanks for testing.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] video, ipu: make ldb clock frequenz overwriteable through board code

[Eric Nelson]

Hi Heiko,

On 04/17/2015 12:53 AM, Heiko Schocher wrote:

>>> +int ipu_set_ldb_clock(int rate);
>>>   #endif
>>> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c
>>> index 1a209d4..dc054bc 100644
>>> --- a/drivers/video/ipu_common.c
>>> +++ b/drivers/video/ipu_common.c
>>> @@ -1198,3 +1198,14 @@ ipu_color_space_t
>>> format_to_colorspace(uint32_t fmt)
>>>       }
>>>       return RGB;
>>>   }
>>> +
>>> +/* should removed when clk framework is availiable */
>>> +int ipu_set_ldb_clock(int rate)
>>> +{
>>
>> This forces a tight dependency on when this is called that
>> using ldb_clk directly would prevent.
> 
> Sorry, did not understand you here ...
> 

The variable g_ldb is initialized in ipu_probe(), so this routine
must be called at a very particular time.

You must have noticed it, since you put in a check for NULL.

>>> +    if (g_ldb_clk == NULL)
>>> +        return -ENOENT;
>>> +

If you replace this with ldb_clk.rate, the dependency goes
away.

>>> +    g_ldb_clk->rate = rate;
>>> +
>>> +    return 0;
>>> +}
>>>

Separately, I don't see any reason not to have g_ldb_clk
statically initialized... i.e.

struct clk *g_ldb_clk = &ldb_clk;