Re: [RFC PATCH v2 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Artur ÅwigoÅ]

Hi,

On Mon, 2019-12-16 at 11:59 +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 12/16/19 9:51 AM, Chanwoo Choi wrote:
> > On 9/19/19 11:22 PM, Artur ÅwigoÅ wrote:
> > > From: Artur ÅwigoÅ <a.swigon@xxxxxxxxxxxxxxxxxxx>
> > > 
> > > This patch adds two fields to the Exynos4412 DTS:
> > >   - parent: to declare connections between nodes that are not in a
> > >     parent-child relation in devfreq;
> > >   - #interconnect-cells: required by the interconnect framework.
> > > 
> > > Please note that #interconnect-cells is always zero and node IDs are not
> > > hardcoded anywhere. The above-mentioned parent-child relation in devfreq
> > > means that there is a shared power line ('devfreq' property). The 'parent'
> > > property only signifies an interconnect connection.
> > > 
> > > Signed-off-by: Artur ÅwigoÅ <a.swigon@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
> > >  arch/arm/boot/dts/exynos4412.dtsi               | 9 +++++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > index ea55f377d17c..bdd61ae86103 100644
> > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > @@ -106,6 +106,7 @@
> > >  &bus_leftbus {
> > >  	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
> > >  	vdd-supply = <&buck3_reg>;
> > > +	parent = <&bus_dmc>;
> > 
> > As I mentioned on other reply,
> > I'm not sure to use the specific 'parent' property to make
> > the connection between buses. If possible, you better to
> > use the standard way like OF graph. Except for making
> > the connection between buses by 'parent' property,
> > looks good to me.
> 
> I tried to think it continuously. I withdraw the my opinion
> using OF graph. If you make the property name like the following
> example, it is possible for exynos.
> - exynos,interconnect-parent-node = <&bus_dmc>; or other proper name.
> 
> Regardless of existing 'devfreq' property, I think you better to
> make the connection between buses for only interconnect as following
> example: This make it possible user can draw the correct tree by tracking
> the 'exynos,interconnect-parent-node' value.

OK, for v3 I will add 'exynos,interconnect-parent-node' to bus_dmc,
bus_leftbus and bus_display as you suggested below and change the code
so that the 'devfreq' (or the upcoming 'exynos,parent-bus') property is
not taken into account.

> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..53f87f46e161 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -90,6 +90,7 @@
>  &bus_dmc {
>         devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>         vdd-supply = <&buck1_reg>;
> +       #interconnect-cells = <0>;
>         status = "okay";
>  };
>  
> @@ -106,6 +107,8 @@
>  &bus_leftbus {
>         devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>         vdd-supply = <&buck3_reg>;
> +       exynos,interconnect-parent-node = <&bus_dmc>;
> +       #interconnect-cells = <0>;
>         status = "okay";
>  };
>  
> @@ -116,6 +119,8 @@
>  
>  &bus_display {
>         devfreq = <&bus_leftbus>;
> +       exynos,interconnect-parent-node = <&bus_leftbus>;
> +       #interconnect-cells = <0>;
>         status = "okay";
>  };
> 
> 
> > 
> > 
> > >  	status = "okay";
> > >  };
> > >  
> > > diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> > > index d20db2dfe8e2..a70a671acacd 100644
> > > --- a/arch/arm/boot/dts/exynos4412.dtsi
> > > +++ b/arch/arm/boot/dts/exynos4412.dtsi
> > > @@ -390,6 +390,7 @@
> > >  			clocks = <&clock CLK_DIV_DMC>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_dmc_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -398,6 +399,7 @@
> > >  			clocks = <&clock CLK_DIV_ACP>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_acp_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -406,6 +408,7 @@
> > >  			clocks = <&clock CLK_DIV_C2C>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_dmc_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -459,6 +462,7 @@
> > >  			clocks = <&clock CLK_DIV_GDL>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_leftbus_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -467,6 +471,7 @@
> > >  			clocks = <&clock CLK_DIV_GDR>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_leftbus_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -475,6 +480,7 @@
> > >  			clocks = <&clock CLK_ACLK160>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_display_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -483,6 +489,7 @@
> > >  			clocks = <&clock CLK_ACLK133>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_fsys_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -491,6 +498,7 @@
> > >  			clocks = <&clock CLK_ACLK100>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_peri_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > @@ -499,6 +507,7 @@
> > >  			clocks = <&clock CLK_SCLK_MFC>;
> > >  			clock-names = "bus";
> > >  			operating-points-v2 = <&bus_leftbus_opp_table>;
> > > +			#interconnect-cells = <0>;
> > >  			status = "disabled";
> > >  		};
> > >  
> > > 
> > 
> > 
> 

Best regards,
-- 
Artur ÅwigoÅ
Samsung R&D Institute Poland
Samsung Electronics