| Message ID | 20190715124417.4787-38-l.luba@partner.samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,01/50] clk: samsung: add new IDs for Exynos5420 clocks | expand |
On Mon, 15 Jul 2019 at 14:45, Lukasz Luba <l.luba@partner.samsung.com> wrote: > > The FSYS bus OPP table has been aligned to the new parent rate. This patch > sets the proper parent and picks the init frequency before the devfreq > governor starts working. It sets also parent rate (DPLL to 1200MHz). 1. I see what the patch is doing, but please write why you are doing this. What problem are you solving here? 2. Commit title is wrong - it is not Exynos 5422 but Odroid XU3/XU4 family of boards. > > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi > index d460041f716c..6a82dd175b8a 100644 > --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi > +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi > @@ -72,6 +72,11 @@ > > &bus_fsys { > devfreq = <&bus_wcore>; > + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, > + <&clock CLK_DOUT_ACLK200_FSYS>, > + <&clock CLK_FOUT_DPLL>; > + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; > + assigned-clock-rates = <0>, <240000000>,<1200000000>; Here and in all other patches: I am not entirely sure that this should be here. It looks like property of the SoC. Do we expect that buses will be configured to different clock rates between different boards? Since the OPP tables are shared (they are property of the SoC, not board) then I would assume that default frequency is shared as well. Best regards, Krzysztof
On 7/17/19 12:25 PM, Krzysztof Kozlowski wrote: > On Mon, 15 Jul 2019 at 14:45, Lukasz Luba <l.luba@partner.samsung.com> wrote: >> >> The FSYS bus OPP table has been aligned to the new parent rate. This patch >> sets the proper parent and picks the init frequency before the devfreq >> governor starts working. It sets also parent rate (DPLL to 1200MHz). > > 1. I see what the patch is doing, but please write why you are doing > this. What problem are you solving here? > 2. Commit title is wrong - it is not Exynos 5422 but Odroid XU3/XU4 > family of boards. OK, I will rewrite it when during the work on squashing the patches. > >> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi >> index d460041f716c..6a82dd175b8a 100644 >> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi >> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi >> @@ -72,6 +72,11 @@ >> >> &bus_fsys { >> devfreq = <&bus_wcore>; >> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, >> + <&clock CLK_DOUT_ACLK200_FSYS>, >> + <&clock CLK_FOUT_DPLL>; >> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; >> + assigned-clock-rates = <0>, <240000000>,<1200000000>; > > Here and in all other patches: > I am not entirely sure that this should be here. It looks like > property of the SoC. Do we expect that buses will be configured to > different clock rates between different boards? Since the OPP tables > are shared (they are property of the SoC, not board) then I would > assume that default frequency is shared as well. These clocks they all relay on some bootloader configuration. It depends which version of the bootloader you have, then you might get different default configuration in the clocks. The pattern of changing the parent or even rate is known in the DT files (or I am missing something). When you grep for it, you get 168 hits (38 for exynos*): git grep -n "assigned-clock-rates" ./arch/arm/boot/dts/ | wc -l Regards, Lukasz > > Best regards, > Krzysztof > >
On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote: > >> > >> &bus_fsys { > >> devfreq = <&bus_wcore>; > >> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, > >> + <&clock CLK_DOUT_ACLK200_FSYS>, > >> + <&clock CLK_FOUT_DPLL>; > >> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; > >> + assigned-clock-rates = <0>, <240000000>,<1200000000>; > > > > Here and in all other patches: > > I am not entirely sure that this should be here. It looks like > > property of the SoC. Do we expect that buses will be configured to > > different clock rates between different boards? Since the OPP tables > > are shared (they are property of the SoC, not board) then I would > > assume that default frequency is shared as well. > These clocks they all relay on some bootloader configuration. It depends > which version of the bootloader you have, then you might get different > default configuration in the clocks. I do not agree here. This configuration is not dependent on bootloader. Although one bootloader might set the clocks to X and other to Y, but still you provide here valid configuration setting them, e.g. to Y (or to Z). What bootloader set before does not matter because you always override it. > The pattern of changing the parent > or even rate is known in the DT files (or I am missing something). > When you grep for it, you get 168 hits (38 for exynos*): > git grep -n "assigned-clock-rates" ./arch/arm/boot/dts/ | wc -l Yeah, and if you grep per type you got: DTSI: 114 DTS: 54 so what do you want to say? My thinking is that all the boards have buses configured to the same initial frequency. I am not questioning the use of assigned-clock-rates at all. Just the place... BR, Krzysztof
On 7/17/19 12:45 PM, Krzysztof Kozlowski wrote: > On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote: >>>> >>>> &bus_fsys { >>>> devfreq = <&bus_wcore>; >>>> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, >>>> + <&clock CLK_DOUT_ACLK200_FSYS>, >>>> + <&clock CLK_FOUT_DPLL>; >>>> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; >>>> + assigned-clock-rates = <0>, <240000000>,<1200000000>; >>> >>> Here and in all other patches: >>> I am not entirely sure that this should be here. It looks like >>> property of the SoC. Do we expect that buses will be configured to >>> different clock rates between different boards? Since the OPP tables >>> are shared (they are property of the SoC, not board) then I would >>> assume that default frequency is shared as well. >> These clocks they all relay on some bootloader configuration. It depends >> which version of the bootloader you have, then you might get different >> default configuration in the clocks. > > I do not agree here. This configuration is not dependent on > bootloader. Although one bootloader might set the clocks to X and > other to Y, but still you provide here valid configuration setting > them, e.g. to Y (or to Z). What bootloader set before does not matter > because you always override it. This exactly the patch set is aim to do: overwrite any bootloader configuration which could be wrong set after boot. I don't know for how long it is left in such 'bootloader-default-clock-settings' but it is not accurate configuration. The pattern in the DT to change the clock rates is there. > >> The pattern of changing the parent >> or even rate is known in the DT files (or I am missing something). >> When you grep for it, you get 168 hits (38 for exynos*): >> git grep -n "assigned-clock-rates" ./arch/arm/boot/dts/ | wc -l > > Yeah, and if you grep per type you got: > DTSI: 114 > DTS: 54 > so what do you want to say? Thus, It could be changed in DT. > > My thinking is that all the boards have buses configured to the same > initial frequency. I am not questioning the use of > assigned-clock-rates at all. Just the place... It is not only 'initial frequency' as you name it. It has three changes: - re-parent to proper PLL - changing this PLL rate - change the OPPs frequency values to integer values derived from PLL The initial frequencies will be changed by devfreq governor using OPP tables and the load after the whole system boots. Regards, Lukasz > > BR, > Krzysztof > >
On Wed, 17 Jul 2019 at 13:06, Lukasz Luba <l.luba@partner.samsung.com> wrote: > > > > On 7/17/19 12:45 PM, Krzysztof Kozlowski wrote: > > On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote: > >>>> > >>>> &bus_fsys { > >>>> devfreq = <&bus_wcore>; > >>>> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, > >>>> + <&clock CLK_DOUT_ACLK200_FSYS>, > >>>> + <&clock CLK_FOUT_DPLL>; > >>>> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; > >>>> + assigned-clock-rates = <0>, <240000000>,<1200000000>; > >>> > >>> Here and in all other patches: > >>> I am not entirely sure that this should be here. It looks like > >>> property of the SoC. Do we expect that buses will be configured to > >>> different clock rates between different boards? Since the OPP tables > >>> are shared (they are property of the SoC, not board) then I would > >>> assume that default frequency is shared as well. > >> These clocks they all relay on some bootloader configuration. It depends > >> which version of the bootloader you have, then you might get different > >> default configuration in the clocks. > > > > I do not agree here. This configuration is not dependent on > > bootloader. Although one bootloader might set the clocks to X and > > other to Y, but still you provide here valid configuration setting > > them, e.g. to Y (or to Z). What bootloader set before does not matter > > because you always override it. > This exactly the patch set is aim to do: overwrite any bootloader > configuration which could be wrong set after boot. > I don't know for how long it is left in such > 'bootloader-default-clock-settings' but it is not accurate > configuration. The pattern in the DT to change the clock rates is > there. Still it is not the answer to my concerns and questions. > > > >> The pattern of changing the parent > >> or even rate is known in the DT files (or I am missing something). > >> When you grep for it, you get 168 hits (38 for exynos*): > >> git grep -n "assigned-clock-rates" ./arch/arm/boot/dts/ | wc -l > > > > Yeah, and if you grep per type you got: > > DTSI: 114 > > DTS: 54 > > so what do you want to say? > Thus, It could be changed in DT. Of course, why not. But how this relevant to my question? > > My thinking is that all the boards have buses configured to the same > > initial frequency. I am not questioning the use of > > assigned-clock-rates at all. Just the place... > It is not only 'initial frequency' as you name it. It has three changes: > - re-parent to proper PLL > - changing this PLL rate > - change the OPPs frequency values to integer values derived from PLL > > The initial frequencies will be changed by devfreq governor using OPP > tables and the load after the whole system boots. I simplified with "initial frequency" but it does not matter. Let me try to raise my concerns again, different wording: All this looks like property of the SoC, not the board, because: 1. the OPPs are already properties of the SoC, not the board (XU3 Lite is kind of exception but in fact it uses different flavor of Exynos5422 SoC which we do not model here as separate DTSI), 2. I expect all boards to have the same properties. Best regards, Krzysztof
On 7/17/19 1:11 PM, Krzysztof Kozlowski wrote: > On Wed, 17 Jul 2019 at 13:06, Lukasz Luba <l.luba@partner.samsung.com> wrote: >> >> >> >> On 7/17/19 12:45 PM, Krzysztof Kozlowski wrote: >>> On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote: >>>>>> >>>>>> &bus_fsys { >>>>>> devfreq = <&bus_wcore>; >>>>>> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, >>>>>> + <&clock CLK_DOUT_ACLK200_FSYS>, >>>>>> + <&clock CLK_FOUT_DPLL>; >>>>>> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; >>>>>> + assigned-clock-rates = <0>, <240000000>,<1200000000>; >>>>> >>>>> Here and in all other patches: >>>>> I am not entirely sure that this should be here. It looks like >>>>> property of the SoC. Do we expect that buses will be configured to >>>>> different clock rates between different boards? This is the board file for Exynos5420/5422/5800 which enables buses. Thus, I have change them here. Patch 49/50 adds these buses to Exynos5800 (Peach Pi). In Exynos5420 there is no clock tree for bus_isp266. The parents for different devices could be also different. It is because i.e. in 5420 there is 2 bit in the WCORE 1st mux while in 5422 there is 3 bits (6 parents possible). That's why I have picked exynos5422-odroid-core.dtsi to reference the bus devices and pinned them into proper parent and changed rate. When you check patch 49/50 for 5800 not all the parents are the same. (1) I could create a dedicated files like: exynos5422-bus.dtsi, exynos5420-bus.dtsi, exynos5800-bus.dtsi which would include some base file with the basic &bus_X and set the right parent, rate. Then these files would be included into proper board file like: exynos5800-peach-pi.dts. Is this something that you would like to see? Since the OPP tables >>>>> are shared (they are property of the SoC, not board) then I would >>>>> assume that default frequency is shared as well. >>>> These clocks they all relay on some bootloader configuration. It depends >>>> which version of the bootloader you have, then you might get different >>>> default configuration in the clocks. >>> >>> I do not agree here. This configuration is not dependent on >>> bootloader. Although one bootloader might set the clocks to X and >>> other to Y, but still you provide here valid configuration setting >>> them, e.g. to Y (or to Z). What bootloader set before does not matter >>> because you always override it. >> This exactly the patch set is aim to do: overwrite any bootloader >> configuration which could be wrong set after boot. >> I don't know for how long it is left in such >> 'bootloader-default-clock-settings' but it is not accurate >> configuration. The pattern in the DT to change the clock rates is >> there. > > Still it is not the answer to my concerns and questions. Please look at my answer above. > >>> >>>> The pattern of changing the parent >>>> or even rate is known in the DT files (or I am missing something). >>>> When you grep for it, you get 168 hits (38 for exynos*): >>>> git grep -n "assigned-clock-rates" ./arch/arm/boot/dts/ | wc -l >>> >>> Yeah, and if you grep per type you got: >>> DTSI: 114 >>> DTS: 54 >>> so what do you want to say? >> Thus, It could be changed in DT. > > Of course, why not. But how this relevant to my question? Please see above. > >>> My thinking is that all the boards have buses configured to the same >>> initial frequency. I am not questioning the use of >>> assigned-clock-rates at all. Just the place... >> It is not only 'initial frequency' as you name it. It has three changes: >> - re-parent to proper PLL >> - changing this PLL rate >> - change the OPPs frequency values to integer values derived from PLL >> >> The initial frequencies will be changed by devfreq governor using OPP >> tables and the load after the whole system boots. > > I simplified with "initial frequency" but it does not matter. Let me > try to raise my concerns again, different wording: > All this looks like property of the SoC, not the board, because: > 1. the OPPs are already properties of the SoC, not the board (XU3 Lite > is kind of exception but in fact it uses different flavor of > Exynos5422 SoC which we do not model here as separate DTSI), Please see above at (1). > 2. I expect all boards to have the same properties. All boards which have the same SoC, i.e. Exysno5422 <- then I agree. Thank you for the comments. Regards, Lukasz > > Best regards, > Krzysztof > >
On Wed, 17 Jul 2019 at 14:56, Lukasz Luba <l.luba@partner.samsung.com> wrote: > > > On 7/17/19 1:11 PM, Krzysztof Kozlowski wrote: > > On Wed, 17 Jul 2019 at 13:06, Lukasz Luba <l.luba@partner.samsung.com> wrote: > >> > >> > >> > >> On 7/17/19 12:45 PM, Krzysztof Kozlowski wrote: > >>> On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote: > >>>>>> > >>>>>> &bus_fsys { > >>>>>> devfreq = <&bus_wcore>; > >>>>>> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, > >>>>>> + <&clock CLK_DOUT_ACLK200_FSYS>, > >>>>>> + <&clock CLK_FOUT_DPLL>; > >>>>>> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; > >>>>>> + assigned-clock-rates = <0>, <240000000>,<1200000000>; > >>>>> > >>>>> Here and in all other patches: > >>>>> I am not entirely sure that this should be here. It looks like > >>>>> property of the SoC. Do we expect that buses will be configured to > >>>>> different clock rates between different boards? > This is the board file for Exynos5420/5422/5800 which enables buses. > Thus, I have change them here. Patch 49/50 adds these buses to > Exynos5800 (Peach Pi). In Exynos5420 there is no clock tree for > bus_isp266. The parents for different devices could be also different. > It is because i.e. in 5420 there is 2 bit in the WCORE 1st mux while in > 5422 there is 3 bits (6 parents possible). > That's why I have picked exynos5422-odroid-core.dtsi to reference > the bus devices and pinned them into proper parent and changed rate. > When you check patch 49/50 for 5800 not all the parents are the same. > > (1) I could create a dedicated files like: exynos5422-bus.dtsi, > exynos5420-bus.dtsi, exynos5800-bus.dtsi which would include some > base file with the basic &bus_X and set the right parent, rate. > Then these files would be included into proper board file like: > exynos5800-peach-pi.dts. > Is this something that you would like to see? I see now. Are there any differences in all these properties between Peach Pi and Odroids? Both of them are using exynos5800.dtsi so that could be a place for all clock assignments. Best regards, Krzysztof
diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi index d460041f716c..6a82dd175b8a 100644 --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi @@ -72,6 +72,11 @@ &bus_fsys { devfreq = <&bus_wcore>; + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>, + <&clock CLK_DOUT_ACLK200_FSYS>, + <&clock CLK_FOUT_DPLL>; + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>; + assigned-clock-rates = <0>, <240000000>,<1200000000>; status = "okay"; };
The FSYS bus OPP table has been aligned to the new parent rate. This patch sets the proper parent and picks the init frequency before the devfreq governor starts working. It sets also parent rate (DPLL to 1200MHz). Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 5 +++++ 1 file changed, 5 insertions(+)