On Thu, Jan 26, 2017 at 04:20:22PM +0100, Andrzej Hajda wrote:
> On 26.01.2017 15:49, Chanwoo Choi wrote:
> > Hi Marek
> >
(...)
> >> + assigned-clock-rates = <278000000>, <400000000>;
> > Except for setting the assigned-clock-rate for CLK_FOUT_DISP_PLL,
> > tm2.dts and tm2e.dts has the same dt node of cmu_disp.
> > If there is same value between tm2 and tm2e, you should keep the same value
> > in exynos5433-tm2-common.dtsi.
> >
> > So, I think that exynos5433-tm2-common.dtsi include the relationships
> > between clocks and parent clocks as following without assigning the clocks.
> >
> > And then, each tm2 and tm2e.dts can assign the clock rate for CLK_FOUT_DISP_PLL
> > and CLK_DIV_SCLK_DECON_TV_ECLK.
> >
> > For example,
> > In exynos5433-tm2-common.dtsi
> >
> > &cmu_disp {
> > assigned-clocks = <&cmu_disp CLK_FOUT_DISP_PLL>,
> > <&cmu_mif CLK_DIV_SCLK_DECON_TV_ECLK>,
> > <&cmu_disp CLK_MOUT_ACLK_DISP_333_USER>,
> > <&cmu_disp CLK_MOUT_SCLK_DSIM0_USER>,
> > <&cmu_disp CLK_MOUT_SCLK_DSIM0>,
> > <&cmu_disp CLK_MOUT_SCLK_DECON_ECLK_USER>,
> > <&cmu_disp CLK_MOUT_SCLK_DECON_ECLK>,
> > <&cmu_disp CLK_MOUT_PHYCLK_MIPIDPHY0_RXCLKESC0_USER>,
> > <&cmu_disp CLK_MOUT_PHYCLK_MIPIDPHY0_BITCLKDIV8_USER>,
> > <&cmu_disp CLK_MOUT_DISP_PLL>,
> > <&cmu_mif CLK_MOUT_SCLK_DECON_TV_ECLK_A>,
> > <&cmu_disp CLK_MOUT_SCLK_DECON_TV_ECLK_USER>,
> > <&cmu_disp CLK_MOUT_SCLK_DECON_TV_ECLK>;
> > assigned-clock-parents = <0>, <0>,
> > <&cmu_mif CLK_ACLK_DISP_333>,
> > <&cmu_mif CLK_SCLK_DSIM0_DISP>,
> > <&cmu_disp CLK_MOUT_SCLK_DSIM0_USER>,
> > <&cmu_mif CLK_SCLK_DECON_ECLK_DISP>,
> > <&cmu_disp CLK_MOUT_SCLK_DECON_ECLK_USER>,
> > <&cmu_disp CLK_PHYCLK_MIPIDPHY0_RXCLKESC0_PHY>,
> > <&cmu_disp CLK_PHYCLK_MIPIDPHY0_BITCLKDIV8_PHY>,
> > <&cmu_disp CLK_FOUT_DISP_PLL>,
> > <&cmu_mif CLK_MOUT_BUS_PLL_DIV2>,
> > <&cmu_mif CLK_SCLK_DECON_TV_ECLK_DISP>,
> > <&cmu_disp CLK_MOUT_SCLK_DECON_TV_ECLK_USER>;
> > };
> >
> >
> > In exynos5433-tm2.dts
> > &cmu_disp {
> > assigned-clocks = <&cmu_disp CLK_FOUT_DISP_PLL>,
> > <&cmu_mif CLK_DIV_SCLK_DECON_TV_ECLK>;
> > assigned-clock-rates = <250000000>, <400000000>;
> > };
> >
> >
> > In exynos5433-tm2e.dts
> > &cmu_disp {
> > assigned-clocks = <&cmu_disp CLK_FOUT_DISP_PLL>,
> > <&cmu_mif CLK_DIV_SCLK_DECON_TV_ECLK>;
> > assigned-clock-rates = <278000000>, <400000000>;
> > };
> >
> I guess in such case common properties will be overwritten.
>
Indeed.
> If one really want to put as much as possible into common part it could
> be done this way:
>
> exynos5433-tm2.dts before '#include "exynos5433-tm2-common.dtsi"':
>
> #define CLK_FOUT_DISP_PLL_RATE 250000000
>
> exynos5433-tm2e.dts before '#include "exynos5433-tm2-common.dtsi"':
>
> #define CLK_FOUT_DISP_PLL_RATE 278000000
>
> and in exynos5433-tm2-common.dtsi:
>
> +&cmu_disp {
> + assigned-clocks = <&cmu_disp CLK_FOUT_DISP_PLL>,
> + <&cmu_mif CLK_DIV_SCLK_DECON_TV_ECLK>,
> + <&cmu_disp CLK_MOUT_ACLK_DISP_333_USER>,
> + <&cmu_disp CLK_MOUT_SCLK_DSIM0_USER>,
> + <&cmu_disp CLK_MOUT_SCLK_DSIM0>,
> + <&cmu_disp CLK_MOUT_SCLK_DECON_ECLK_USER>,
> + <&cmu_disp CLK_MOUT_SCLK_DECON_ECLK>,
> + <&cmu_disp CLK_MOUT_PHYCLK_MIPIDPHY0_RXCLKESC0_USER>,
> + <&cmu_disp CLK_MOUT_PHYCLK_MIPIDPHY0_BITCLKDIV8_USER>,
> + <&cmu_disp CLK_MOUT_DISP_PLL>,
> + <&cmu_mif CLK_MOUT_SCLK_DECON_TV_ECLK_A>,
> + <&cmu_disp CLK_MOUT_SCLK_DECON_TV_ECLK_USER>,
> + <&cmu_disp CLK_MOUT_SCLK_DECON_TV_ECLK>;
> + assigned-clock-parents = <0>, <0>,
> + <&cmu_mif CLK_ACLK_DISP_333>,
> + <&cmu_mif CLK_SCLK_DSIM0_DISP>,
> + <&cmu_disp CLK_MOUT_SCLK_DSIM0_USER>,
> + <&cmu_mif CLK_SCLK_DECON_ECLK_DISP>,
> + <&cmu_disp CLK_MOUT_SCLK_DECON_ECLK_USER>,
> + <&cmu_disp CLK_PHYCLK_MIPIDPHY0_RXCLKESC0_PHY>,
> + <&cmu_disp CLK_PHYCLK_MIPIDPHY0_BITCLKDIV8_PHY>,
> + <&cmu_disp CLK_FOUT_DISP_PLL>,
> + <&cmu_mif CLK_MOUT_BUS_PLL_DIV2>,
> + <&cmu_mif CLK_SCLK_DECON_TV_ECLK_DISP>,
> + <&cmu_disp CLK_MOUT_SCLK_DECON_TV_ECLK_USER>;
> + assigned-clock-rates = <CLK_FOUT_DISP_PLL_RATE>, <400000000>;
> +};
> +
>
> The question is if such macro games will be accepted? :)
Interesting idea... The DTSI would look nice but the real problem is
hidden by a dependency on #define. In the same time one would have two
definitions of this value - when looking at DTSI, one would have to
look at DTS as well. This would be rather a new way of overriding
things (reading DTSI and then jumping to DTS) and I prefer
consistency...
assigned-clocks and assigned-clock-parents are the same, right? Only
assigned-clock-rates differ?
Ideas:
1. Make common assigned-clocks+assigned-clock-parents in DTSI and customize
only the rates in DTS.
Mention the reason behind the split for assigned-* properties in a
comment in all files (common + boards). Anyone looking at final DTS
will find that assigned-clocks/parents are somewhere else (and
vice-versa). Also anyone looking at DTSI will see a comment that one
part is missing - rates are somewhere else.
2. Duplicate the nodes like it is in Marek's patch but please mention in
a comment that assigned-clocks+assigned-clock-parents are expected to be
the same between boards.
The goal behind the comment is to make reader's life easier:
- no need to figure out why this is duplicated,
- if there would be a mistake (difference between boards), then
easily understand that this was a mistake, not a feature.
The benefit of Andrzej's approach is that there will be compile time
dependency - missing define causes build failure. In my approach, there
will be only comments. Anyway in all cases the reader have to look at
both DTSI and DTS to find the final answer.
What do you think?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
