[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Sylwester Nawrocki]

The default mux and divider clocks are specified in device tree
so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
clocked from recommended clock source and with maximum supported
frequency. If needed these settings could be overrode in board
specific dts files, however they are in practice optimal in most
cases.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 arch/arm/boot/dts/exynos4210.dtsi |   16 ++++++++++++++++
 arch/arm/boot/dts/exynos4x12.dtsi |   16 ++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index 807bb5b..0969d2e 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -154,18 +154,30 @@
 			samsung,pix-limits = <4224 8192 1920 4224>;
 			samsung,mainscaler-ext;
 			samsung,cam-if;
+			assigned-clocks = <&clock CLK_MOUT_FIMC0>,
+					<&clock CLK_SCLK_FIMC0>;
+			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
+			assigned-clock-rates = <0>, <160000000>;
 		};
 
 		fimc_1: fimc at 11810000 {
 			samsung,pix-limits = <4224 8192 1920 4224>;
 			samsung,mainscaler-ext;
 			samsung,cam-if;
+			assigned-clocks = <&clock CLK_MOUT_FIMC1>,
+					<&clock CLK_SCLK_FIMC1>;
+			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
+			assigned-clock-rates = <0>, <160000000>;
 		};
 
 		fimc_2: fimc at 11820000 {
 			samsung,pix-limits = <4224 8192 1920 4224>;
 			samsung,mainscaler-ext;
 			samsung,lcd-wb;
+			assigned-clocks = <&clock CLK_MOUT_FIMC2>,
+					<&clock CLK_SCLK_FIMC2>;
+			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
+			assigned-clock-rates = <0>, <160000000>;
 		};
 
 		fimc_3: fimc at 11830000 {
@@ -173,6 +185,10 @@
 			samsung,rotators = <0>;
 			samsung,mainscaler-ext;
 			samsung,lcd-wb;
+			assigned-clocks = <&clock CLK_MOUT_FIMC3>,
+					<&clock CLK_SCLK_FIMC3>;
+			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
+			assigned-clock-rates = <0>, <160000000>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 861bb91..38ba14f 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -162,6 +162,10 @@
 			samsung,mainscaler-ext;
 			samsung,isp-wb;
 			samsung,cam-if;
+			assigned-clocks = <&clock CLK_MOUT_FIMC0>,
+					<&clock CLK_SCLK_FIMC0>;
+			assigned-clock-parents = <&clock CLK_MOUT_MPLL_USER_T>;
+			assigned-clock-rates = <0>, <176000000>;
 		};
 
 		fimc_1: fimc at 11810000 {
@@ -170,6 +174,10 @@
 			samsung,mainscaler-ext;
 			samsung,isp-wb;
 			samsung,cam-if;
+			assigned-clocks = <&clock CLK_MOUT_FIMC1>,
+					<&clock CLK_SCLK_FIMC1>;
+			assigned-clock-parents = <&clock CLK_MOUT_MPLL_USER_T>;
+			assigned-clock-rates = <0>, <176000000>;
 		};
 
 		fimc_2: fimc at 11820000 {
@@ -179,6 +187,10 @@
 			samsung,isp-wb;
 			samsung,lcd-wb;
 			samsung,cam-if;
+			assigned-clocks = <&clock CLK_MOUT_FIMC2>,
+					<&clock CLK_SCLK_FIMC2>;
+			assigned-clock-parents = <&clock CLK_MOUT_MPLL_USER_T>;
+			assigned-clock-rates = <0>, <176000000>;
 		};
 
 		fimc_3: fimc at 11830000 {
@@ -188,6 +200,10 @@
 			samsung,mainscaler-ext;
 			samsung,isp-wb;
 			samsung,lcd-wb;
+			assigned-clocks = <&clock CLK_MOUT_FIMC3>,
+					<&clock CLK_SCLK_FIMC3>;
+			assigned-clock-parents = <&clock CLK_MOUT_MPLL_USER_T>;
+			assigned-clock-rates = <0>, <176000000>;
 		};
 
 		fimc_lite_0: fimc-lite at 12390000 {
-- 
1.7.9.5

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Daniel Drake]

Hi Sylwester,

On Wed, Sep 10, 2014 at 10:37 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> The default mux and divider clocks are specified in device tree
> so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
> clocked from recommended clock source and with maximum supported
> frequency. If needed these settings could be overrode in board
> specific dts files, however they are in practice optimal in most
> cases.

Just curious about the Exynos4x12 situation here.
You set the FIMC clocks as 176MHz, child of MPLL, which works for
ODROID with a divider:

880MHz MPLL / 5 = 176MHz

However, talking of recommended frequencies... Is 880MHz really the
standard there?
Isn't 800MHz the more common one?

Also, if you happen to know, I would be curious about the equivalent
and recommended situation for the sclk_mfc clock. On the vendor kernel
it is clocked at 880/4 = 220MHz. When booting mainline on an odroid it
is 880/16 = 55MHz :/

Thanks
Daniel

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Sylwester Nawrocki]

Hi Daniel,

On 18/09/14 21:27, Daniel Drake wrote:
> On Wed, Sep 10, 2014 at 10:37 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> > The default mux and divider clocks are specified in device tree
>> > so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
>> > clocked from recommended clock source and with maximum supported
>> > frequency. If needed these settings could be overrode in board
>> > specific dts files, however they are in practice optimal in most
>> > cases.
>
> Just curious about the Exynos4x12 situation here.
> You set the FIMC clocks as 176MHz, child of MPLL, which works for
> ODROID with a divider:
> 
> 880MHz MPLL / 5 = 176MHz
> 
> However, talking of recommended frequencies... Is 880MHz really the
> standard there?
> Isn't 800MHz the more common one?

AFAIK 880 MHz is recommended MPLL frequency for Exynos4412 EVT2.0, which
is revision of the Exynos4412 SoC the Odroid U3 boards are populated with.
You can read the main/sub revision information from the chip ID register
(at 0x10000000).
The frequencies can always be overwritten in board specific dts or DTB
could be amended by bootloader if needed.

> Also, if you happen to know, I would be curious about the equivalent
> and recommended situation for the sclk_mfc clock. On the vendor kernel
> it is clocked at 880/4 = 220MHz. When booting mainline on an odroid it
> is 880/16 = 55MHz :/

I think we should add similar entry in device tree for the MFC devices.
AFAIR now the frequency has fixed value in the driver. I saw some changes
in s5p-mfc driver WRT to clock handling recently though, possibly Jacek
or Kamil could explain what current situation is.

--
Regards,
Sylwester

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Daniel Drake]

On Thu, Sep 25, 2014 at 12:05 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>> Just curious about the Exynos4x12 situation here.
>> You set the FIMC clocks as 176MHz, child of MPLL, which works for
>> ODROID with a divider:
>>
>> 880MHz MPLL / 5 = 176MHz
>>
>> However, talking of recommended frequencies... Is 880MHz really the
>> standard there?
>> Isn't 800MHz the more common one?
>
> AFAIK 880 MHz is recommended MPLL frequency for Exynos4412 EVT2.0, which
> is revision of the Exynos4412 SoC the Odroid U3 boards are populated with.
> You can read the main/sub revision information from the chip ID register
> (at 0x10000000).

Thanks for the information. Might be worth a quick comment in
exynos4x12.dtsi then, saying that the defaults here are for Exynos4412
EVT2.0 and might want to be overridden for other versions of the SoC.

Thanks
Daniel

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Daniel Drake]

On Thu, Sep 25, 2014 at 1:44 PM, Daniel Drake <drake@endlessm.com> wrote:
>> AFAIK 880 MHz is recommended MPLL frequency for Exynos4412 EVT2.0, which
>> is revision of the Exynos4412 SoC the Odroid U3 boards are populated with.
>> You can read the main/sub revision information from the chip ID register
>> (at 0x10000000).
>
> Thanks for the information. Might be worth a quick comment in
> exynos4x12.dtsi then, saying that the defaults here are for Exynos4412
> EVT2.0 and might want to be overridden for other versions of the SoC.

Also can you double check this?
The old ODROID-X is Exynos4412 EVT1.0 and runs 880MHz MPLL.

Daniel

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Daniel Drake]

On Wed, Sep 10, 2014 at 10:37 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> The default mux and divider clocks are specified in device tree
> so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
> clocked from recommended clock source and with maximum supported
> frequency. If needed these settings could be overrode in board
> specific dts files, however they are in practice optimal in most
> cases.

Another consideration for this patch.

fimc-core.c already has probe-time code that tries to set the clock
frequency, using a clock-frequency property in the DT, and falling
back to a hardcoded value in the driver. Maybe it is best to just set
the clock rate from one place.

Daniel

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Sylwester Nawrocki]

On 19/09/14 01:53, Daniel Drake wrote:
> On Wed, Sep 10, 2014 at 10:37 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> The default mux and divider clocks are specified in device tree
>> so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
>> clocked from recommended clock source and with maximum supported
>> frequency. If needed these settings could be overrode in board
>> specific dts files, however they are in practice optimal in most
>> cases.
> 
> Another consideration for this patch.
> 
> fimc-core.c already has probe-time code that tries to set the clock
> frequency, using a clock-frequency property in the DT, and falling
> back to a hardcoded value in the driver. Maybe it is best to just set
> the clock rate from one place.

Indeed, thanks for pointing this out. I will submit a patch to remove
the fallback in fimc-core.c. So the driver doesn't touch both: clock
rate and parent setting if 'clock-rates' property is specified in DT.
I think clock-frequency property in the FIMC DT binding should be
deprecated. So the initial parent clock and frequency settings are
handled similarly for all relevant IP blocks, and there is no need
to code such things in each individual driver.

--
Thanks,
Sylwester

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Tomasz Figa]

Hi Sylwester,

On 10.09.2014 18:37, Sylwester Nawrocki wrote:
> The default mux and divider clocks are specified in device tree
> so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
> clocked from recommended clock source and with maximum supported
> frequency. If needed these settings could be overrode in board
> specific dts files, however they are in practice optimal in most
> cases.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4210.dtsi |   16 ++++++++++++++++
>  arch/arm/boot/dts/exynos4x12.dtsi |   16 ++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index 807bb5b..0969d2e 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -154,18 +154,30 @@
>  			samsung,pix-limits = <4224 8192 1920 4224>;
>  			samsung,mainscaler-ext;
>  			samsung,cam-if;
> +			assigned-clocks = <&clock CLK_MOUT_FIMC0>,
> +					<&clock CLK_SCLK_FIMC0>;
> +			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
> +			assigned-clock-rates = <0>, <160000000>;

I wonder whether such settings shouldn't really be set up on a per-board
basis.

As Daniel already pointed, we have cases when MPLL frequency differs
across boards, but we might also have boards that differ in power budget
and so having different desired operating frequencies for various IP blocks.

What do you think?

Best regards,
Tomasz

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Sylwester Nawrocki]

Hi Tomasz,

On 25/09/14 23:58, Tomasz Figa wrote:
> On 10.09.2014 18:37, Sylwester Nawrocki wrote:
>> > The default mux and divider clocks are specified in device tree
>> > so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
>> > clocked from recommended clock source and with maximum supported
>> > frequency. If needed these settings could be overrode in board
>> > specific dts files, however they are in practice optimal in most
>> > cases.
>> > 
>> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> > ---
>> >  arch/arm/boot/dts/exynos4210.dtsi |   16 ++++++++++++++++
>> >  arch/arm/boot/dts/exynos4x12.dtsi |   16 ++++++++++++++++
>> >  2 files changed, 32 insertions(+)
>> > 
>> > diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
>> > index 807bb5b..0969d2e 100644
>> > --- a/arch/arm/boot/dts/exynos4210.dtsi
>> > +++ b/arch/arm/boot/dts/exynos4210.dtsi
>> > @@ -154,18 +154,30 @@
>> >  			samsung,pix-limits = <4224 8192 1920 4224>;
>> >  			samsung,mainscaler-ext;
>> >  			samsung,cam-if;
>> > +			assigned-clocks = <&clock CLK_MOUT_FIMC0>,
>> > +					<&clock CLK_SCLK_FIMC0>;
>> > +			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
>> > +			assigned-clock-rates = <0>, <160000000>;
>
> I wonder whether such settings shouldn't really be set up on a per-board
> basis.
> 
> As Daniel already pointed, we have cases when MPLL frequency differs
> across boards, but we might also have boards that differ in power budget
> and so having different desired operating frequencies for various IP blocks.
> 
> What do you think?

This patch provides sane default values for Exynos4210, MPLL is recommended
clock source for FIMC devices. If any other clock frequency is needed for 
selected boards the clocks setup could be simply overwritten in board dts 
file. Otherwise similar changes would have to be done in each board dts.

Alternatively I could split it and leave only parent clock assignment in 
SoC dts, moving assigned-clock-rates properties to board dts. I'm going 
to leave the functional clock frequency setting in the driver as it is done 
now, and to just modify the fallback to driver data, to have also 
'assigned-clock-rates' considered in the driver.

So parent clock assignment independently of the IP block driver in dts, 
and the functional clock frequency set in the driver from driver data.

-- 
Regards,
Sylwester

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Tomasz Figa]

On 26.09.2014 13:01, Sylwester Nawrocki wrote:
> Hi Tomasz,
> 
> On 25/09/14 23:58, Tomasz Figa wrote:
>> On 10.09.2014 18:37, Sylwester Nawrocki wrote:
>>>> The default mux and divider clocks are specified in device tree
>>>> so that the FIMC devices in Exynos4210 and Exynos4x12 SoCs are
>>>> clocked from recommended clock source and with maximum supported
>>>> frequency. If needed these settings could be overrode in board
>>>> specific dts files, however they are in practice optimal in most
>>>> cases.
>>>>
>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>> ---
>>>>  arch/arm/boot/dts/exynos4210.dtsi |   16 ++++++++++++++++
>>>>  arch/arm/boot/dts/exynos4x12.dtsi |   16 ++++++++++++++++
>>>>  2 files changed, 32 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
>>>> index 807bb5b..0969d2e 100644
>>>> --- a/arch/arm/boot/dts/exynos4210.dtsi
>>>> +++ b/arch/arm/boot/dts/exynos4210.dtsi
>>>> @@ -154,18 +154,30 @@
>>>>  			samsung,pix-limits = <4224 8192 1920 4224>;
>>>>  			samsung,mainscaler-ext;
>>>>  			samsung,cam-if;
>>>> +			assigned-clocks = <&clock CLK_MOUT_FIMC0>,
>>>> +					<&clock CLK_SCLK_FIMC0>;
>>>> +			assigned-clock-parents = <&clock CLK_SCLK_MPLL>;
>>>> +			assigned-clock-rates = <0>, <160000000>;
>>
>> I wonder whether such settings shouldn't really be set up on a per-board
>> basis.
>>
>> As Daniel already pointed, we have cases when MPLL frequency differs
>> across boards, but we might also have boards that differ in power budget
>> and so having different desired operating frequencies for various IP blocks.
>>
>> What do you think?
> 
> This patch provides sane default values for Exynos4210, MPLL is recommended
> clock source for FIMC devices. If any other clock frequency is needed for 
> selected boards the clocks setup could be simply overwritten in board dts 
> file. Otherwise similar changes would have to be done in each board dts.

I'm not concerned specifically with Exynos4210, but with placing such
kind of data in common dtsi files.

Notice that even on boards which have correct initialization done by
firmware, this will cause the settings to be overwritten, even if the
firmware sets correct, but different values, regardless of them being
clock parents or rates.

To me, even if this would mean duplicating some data, making this per
board and present only in dts files of boards that actually need this
(i.e. are known to have broken firmware) sounds more reasonable.

Best regards,
Tomasz

[PATCH] ARM: dts: Specify default clocks for Exynos4 FIMC devices

[Sylwester Nawrocki]

On 26/09/14 15:24, Tomasz Figa wrote:
> I'm not concerned specifically with Exynos4210, but with placing such
> kind of data in common dtsi files.
> 
> Notice that even on boards which have correct initialization done by
> firmware, this will cause the settings to be overwritten, even if the
> firmware sets correct, but different values, regardless of them being
> clock parents or rates.
> 
> To me, even if this would mean duplicating some data, making this per
> board and present only in dts files of boards that actually need this
> (i.e. are known to have broken firmware) sounds more reasonable.

OTOH by having those settings in device tree would ensure the clock
tree is set correctly, regardless of the firmware. There is only one
correct clock parent for these devices, so the kernel couldn't do any
harm. I'd say it never worked in practice to rely on the bootloader
to configure these things, and one could say the firmware has been
broken in general with regards to this issue.

--
Regards,
Sylwester