| Message ID | 20190715124417.4787-21-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:44, Lukasz Luba <l.luba@partner.samsung.com> wrote: > > The FSYS and FSYS2 buses have similar characteristics and both have max > frequency 240MHz. The old OPP table bus_fsys_apb_opp_table should be used > only to FSYS APB bus because APB max frequency is 200MHz. > The new OPPs for FSYS should increase its performance and related devices. I do not understand the explanation. You say that there are two buses - FSYS and FSYS2 - and old OPP table should be used for FSYS APB but you remove the old one (by renaming). Or which one is the 'old one' here? The reason is speed... wait, what? Usually DTS should describe the HW so I imagine that proper opp table should be used for proper bus. It surprised me that we switch a bus to different OPP table just because of speed concerns. It should be correctness concern. Please clarify and reword all this. I am also not sure how this relates with previous patch - whether you are fixing independent issues. Maybe because I do not see the issue fixed... change the commit title and adjust the messages to focus WHY you are doing it. For small fixes WHAT you are doing is rather obvious so commit msg (and title) should not focus on it. Best regards, Krzysztof > > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > arch/arm/boot/dts/exynos5420.dtsi | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi > index 941c58bdd809..c7fc4b829b2a 100644 > --- a/arch/arm/boot/dts/exynos5420.dtsi > +++ b/arch/arm/boot/dts/exynos5420.dtsi > @@ -995,7 +995,7 @@ > compatible = "samsung,exynos-bus"; > clocks = <&clock CLK_DOUT_ACLK200_FSYS>; > clock-names = "bus"; > - operating-points-v2 = <&bus_fsys_apb_opp_table>; > + operating-points-v2 = <&bus_fsys_opp_table>; > status = "disabled"; > }; > > @@ -1003,7 +1003,7 @@ > compatible = "samsung,exynos-bus"; > clocks = <&clock CLK_DOUT_ACLK200_FSYS2>; > clock-names = "bus"; > - operating-points-v2 = <&bus_fsys2_opp_table>; > + operating-points-v2 = <&bus_fsys_opp_table>; > status = "disabled"; > }; > > @@ -1157,7 +1157,7 @@ > }; > }; > > - bus_fsys2_opp_table: opp_table5 { > + bus_fsys_opp_table: opp_table5 { > compatible = "operating-points-v2"; > > opp00 { > -- > 2.17.1 >
Hi Krzysztof, On 7/17/19 10:39 AM, Krzysztof Kozlowski wrote: > On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote: >> >> The FSYS and FSYS2 buses have similar characteristics and both have max >> frequency 240MHz. The old OPP table bus_fsys_apb_opp_table should be used >> only to FSYS APB bus because APB max frequency is 200MHz. >> The new OPPs for FSYS should increase its performance and related devices. > > I do not understand the explanation. You say that there are two buses > - FSYS and FSYS2 - and old OPP table should be used for FSYS APB but > you remove the old one (by renaming). Or which one is the 'old one' > here? The reason is speed... wait, what? Usually DTS should describe > the HW so I imagine that proper opp table should be used for proper > bus. It surprised me that we switch a bus to different OPP table just > because of speed concerns. It should be correctness concern. > > Please clarify and reword all this. > > I am also not sure how this relates with previous patch - whether you > are fixing independent issues. Maybe because I do not see the issue > fixed... change the commit title and adjust the messages to focus WHY > you are doing it. For small fixes WHAT you are doing is rather obvious > so commit msg (and title) should not focus on it. I don't know how familiar you are with AMBA standard or general concept of NoC, so I am not sure if the explanation below would be sufficient. There are 3 buses: FSYS, FSYS2, FSYS APB. The first two are connecting AXI Slave/Master interfaces of the IP blocks. They are dedicated to transfer the data i.e. to MMC block using 128 bit bus width and 240MHz clock. The 3rd is dedicated for accessing peripheral registers - connecting to IP block interfaces called APB3 slave. As I mentioned in the comment the FSYS and FSYS2 are able to run faster than the APB bus. Thus, changing the old implementation which pinned FSYS and FSYS APB to the same OPP table is wrong. The right connection made by OPP reference should be FSYS and FSYS2 with also 240MHz max freq inside. I have discussed offline with Bartek and I will squash DT patches to an atomic-change-with-OPPs-and-PLL-rate-for-all-children, with more detailed comment in the commit message describing the old state and the new one. Thank you for the review. Regards, Lukasz > > Best regards, > Krzysztof > >> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> arch/arm/boot/dts/exynos5420.dtsi | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi >> index 941c58bdd809..c7fc4b829b2a 100644 >> --- a/arch/arm/boot/dts/exynos5420.dtsi >> +++ b/arch/arm/boot/dts/exynos5420.dtsi >> @@ -995,7 +995,7 @@ >> compatible = "samsung,exynos-bus"; >> clocks = <&clock CLK_DOUT_ACLK200_FSYS>; >> clock-names = "bus"; >> - operating-points-v2 = <&bus_fsys_apb_opp_table>; >> + operating-points-v2 = <&bus_fsys_opp_table>; >> status = "disabled"; >> }; >> >> @@ -1003,7 +1003,7 @@ >> compatible = "samsung,exynos-bus"; >> clocks = <&clock CLK_DOUT_ACLK200_FSYS2>; >> clock-names = "bus"; >> - operating-points-v2 = <&bus_fsys2_opp_table>; >> + operating-points-v2 = <&bus_fsys_opp_table>; >> status = "disabled"; >> }; >> >> @@ -1157,7 +1157,7 @@ >> }; >> }; >> >> - bus_fsys2_opp_table: opp_table5 { >> + bus_fsys_opp_table: opp_table5 { >> compatible = "operating-points-v2"; >> >> opp00 { >> -- >> 2.17.1 >> > >
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 941c58bdd809..c7fc4b829b2a 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -995,7 +995,7 @@ compatible = "samsung,exynos-bus"; clocks = <&clock CLK_DOUT_ACLK200_FSYS>; clock-names = "bus"; - operating-points-v2 = <&bus_fsys_apb_opp_table>; + operating-points-v2 = <&bus_fsys_opp_table>; status = "disabled"; }; @@ -1003,7 +1003,7 @@ compatible = "samsung,exynos-bus"; clocks = <&clock CLK_DOUT_ACLK200_FSYS2>; clock-names = "bus"; - operating-points-v2 = <&bus_fsys2_opp_table>; + operating-points-v2 = <&bus_fsys_opp_table>; status = "disabled"; }; @@ -1157,7 +1157,7 @@ }; }; - bus_fsys2_opp_table: opp_table5 { + bus_fsys_opp_table: opp_table5 { compatible = "operating-points-v2"; opp00 {
The FSYS and FSYS2 buses have similar characteristics and both have max frequency 240MHz. The old OPP table bus_fsys_apb_opp_table should be used only to FSYS APB bus because APB max frequency is 200MHz. The new OPPs for FSYS should increase its performance and related devices. Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- arch/arm/boot/dts/exynos5420.dtsi | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)