diff mbox series

[v3,07/15] arm64: dts: qcom: sc8180x: Add interconnects and lmh

Message ID 20230530162454.51708-8-vkoul@kernel.org
State Accepted
Commit f3be8a111d7eaf4e291b6c2d51dd0adb39934b32
Headers show
Series Introduce the SC8180x devices | expand

Commit Message

Vinod Koul May 30, 2023, 4:24 p.m. UTC
This add interconnect nodes and add LMH to sc8180x SoC dtsi

Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8180x.dtsi | 126 ++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

Comments

Vinod Koul June 1, 2023, 7:17 a.m. UTC | #1
On 31-05-23, 10:26, Krzysztof Kozlowski wrote:
> On 30/05/2023 18:24, Vinod Koul wrote:
> > This add interconnect nodes and add LMH to sc8180x SoC dtsi
> > 
> > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> 
> I don't understand why this was split. We talked on IRC many times on
> this - artificial splits are not "release early, release often". Your
> previous patchset was correct in that approach, but why this is separate
> patch?

Coz the patch was big to review. This is usual Linux approach to break a
change into smaller chunks for review!
Bjorn Andersson June 1, 2023, 1:27 p.m. UTC | #2
On Thu, Jun 01, 2023 at 12:47:03PM +0530, Vinod Koul wrote:
> On 31-05-23, 10:26, Krzysztof Kozlowski wrote:
> > On 30/05/2023 18:24, Vinod Koul wrote:
> > > This add interconnect nodes and add LMH to sc8180x SoC dtsi
> > > 
> > > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > 
> > I don't understand why this was split. We talked on IRC many times on
> > this - artificial splits are not "release early, release often". Your
> > previous patchset was correct in that approach, but why this is separate
> > patch?
> 
> Coz the patch was big to review. This is usual Linux approach to break a
> change into smaller chunks for review!
> 

We break patches into small, logical units so that it's easy to follow
the thought through each step in the process of introducing a change.

This is not the same thing as splitting one logical change into multiple
smaller patches to keep the line count of each patch down. This just
forces the reviewer to jump between emails to get the full picture of
the logical change.

And it's not until patch 14 that any of this content is introduced to
the build system, so it's not split to ensure bisectability etc.


I'd be perfectly happy to have received the dts changes as 3-4 patches.

Regards,
Bjorn
Krzysztof Kozlowski June 1, 2023, 3:27 p.m. UTC | #3
On 01/06/2023 15:27, Bjorn Andersson wrote:
> On Thu, Jun 01, 2023 at 12:47:03PM +0530, Vinod Koul wrote:
>> On 31-05-23, 10:26, Krzysztof Kozlowski wrote:
>>> On 30/05/2023 18:24, Vinod Koul wrote:
>>>> This add interconnect nodes and add LMH to sc8180x SoC dtsi
>>>>
>>>> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>>> ---
>>>
>>> I don't understand why this was split. We talked on IRC many times on
>>> this - artificial splits are not "release early, release often". Your
>>> previous patchset was correct in that approach, but why this is separate
>>> patch?
>>
>> Coz the patch was big to review. This is usual Linux approach to break a
>> change into smaller chunks for review!
>>
> 
> We break patches into small, logical units so that it's easy to follow
> the thought through each step in the process of introducing a change.

For example splitting interconnects which are essential part of several
IP blocks is not making it easy. One patch introduces incomplete block
which is then fixed (completed) in next patch.

> 
> This is not the same thing as splitting one logical change into multiple
> smaller patches to keep the line count of each patch down. This just
> forces the reviewer to jump between emails to get the full picture of
> the logical change.

Reviewer has to jump here to see full picture of UART or some other IP
block.

Best regards,
Krzysztof
Bjorn Andersson June 10, 2023, 4:24 p.m. UTC | #4
On Thu, Jun 01, 2023 at 05:27:21PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2023 15:27, Bjorn Andersson wrote:
> > On Thu, Jun 01, 2023 at 12:47:03PM +0530, Vinod Koul wrote:
> >> On 31-05-23, 10:26, Krzysztof Kozlowski wrote:
> >>> On 30/05/2023 18:24, Vinod Koul wrote:
> >>>> This add interconnect nodes and add LMH to sc8180x SoC dtsi
> >>>>
> >>>> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >>>> ---
> >>>
> >>> I don't understand why this was split. We talked on IRC many times on
> >>> this - artificial splits are not "release early, release often". Your
> >>> previous patchset was correct in that approach, but why this is separate
> >>> patch?
> >>
> >> Coz the patch was big to review. This is usual Linux approach to break a
> >> change into smaller chunks for review!
> >>
> > 
> > We break patches into small, logical units so that it's easy to follow
> > the thought through each step in the process of introducing a change.
> 
> For example splitting interconnects which are essential part of several
> IP blocks is not making it easy. One patch introduces incomplete block
> which is then fixed (completed) in next patch.
> 
> > 
> > This is not the same thing as splitting one logical change into multiple
> > smaller patches to keep the line count of each patch down. This just
> > forces the reviewer to jump between emails to get the full picture of
> > the logical change.
> 
> Reviewer has to jump here to see full picture of UART or some other IP
> block.
> 

Sorry if it wasn't clear, I'm trying to make the same argument as you,
Krzysztof.


The way to split such series would be to introduce some minimal bootable
board, and then extend that in a bisectable fashion. But given that both
contributor and maintainer primarily care about the whole set at this
stage, I'd generally prefer longer patches.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8180x.dtsi b/arch/arm64/boot/dts/qcom/sc8180x.dtsi
index e38ea2545654..08f5ad65efd5 100644
--- a/arch/arm64/boot/dts/qcom/sc8180x.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8180x.dtsi
@@ -6,6 +6,8 @@ 
 
 #include <dt-bindings/clock/qcom,gcc-sc8180x.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
+#include <dt-bindings/interconnect/qcom,osm-l3.h>
+#include <dt-bindings/interconnect/qcom,sc8180x.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/power/qcom-rpmpd.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
@@ -44,6 +46,8 @@  CPU0: cpu@0 {
 			next-level-cache = <&L2_0>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD0>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -70,6 +74,8 @@  CPU1: cpu@100 {
 			next-level-cache = <&L2_100>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD1>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -93,6 +99,8 @@  CPU2: cpu@200 {
 			next-level-cache = <&L2_200>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD2>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -115,6 +123,8 @@  CPU3: cpu@300 {
 			next-level-cache = <&L2_300>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			operating-points-v2 = <&cpu0_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD3>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -137,6 +147,8 @@  CPU4: cpu@400 {
 			next-level-cache = <&L2_400>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD4>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -159,6 +171,8 @@  CPU5: cpu@500 {
 			next-level-cache = <&L2_500>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD5>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -181,6 +195,8 @@  CPU6: cpu@600 {
 			next-level-cache = <&L2_600>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD6>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -203,6 +219,8 @@  CPU7: cpu@700 {
 			next-level-cache = <&L2_700>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			operating-points-v2 = <&cpu4_opp_table>;
+			interconnects = <&gem_noc MASTER_AMPSS_M0 3 &mc_virt SLAVE_EBI_CH0 3>,
+					<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;
 			power-domains = <&CPU_PD7>;
 			power-domain-names = "psci";
 			#cooling-cells = <2>;
@@ -476,6 +494,24 @@  scm: scm {
 		};
 	};
 
+	camnoc_virt: interconnect-camnoc-virt {
+		compatible = "qcom,sc8180x-camnoc-virt";
+		#interconnect-cells = <2>;
+		qcom,bcm-voters = <&apps_bcm_voter>;
+	};
+
+	mc_virt: interconnect-mc-virt {
+		compatible = "qcom,sc8180x-mc-virt";
+		#interconnect-cells = <2>;
+		qcom,bcm-voters = <&apps_bcm_voter>;
+	};
+
+	qup_virt: interconnect-qup-virt {
+		compatible = "qcom,sc8180x-qup-virt";
+		#interconnect-cells = <2>;
+		qcom,bcm-voters = <&apps_bcm_voter>;
+	};
+
 	memory@80000000 {
 		device_type = "memory";
 		/* We expect the bootloader to fill in the size */
@@ -743,6 +779,48 @@  gcc: clock-controller@100000 {
 				      "sleep_clk";
 		};
 
+		config_noc: interconnect@1500000 {
+			compatible = "qcom,sc8180x-config-noc";
+			reg = <0 0x01500000 0 0x7400>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
+		system_noc: interconnect@1620000 {
+			compatible = "qcom,sc8180x-system-noc";
+			reg = <0 0x01620000 0 0x19400>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
+		aggre1_noc: interconnect@16e0000 {
+			compatible = "qcom,sc8180x-aggre1-noc";
+			reg = <0 0x016e0000 0 0xd080>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
+		aggre2_noc: interconnect@1700000 {
+			compatible = "qcom,sc8180x-aggre2-noc";
+			reg = <0 0x01700000 0 0x20000>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
+		compute_noc: interconnect@1720000 {
+			compatible = "qcom,sc8180x-compute-noc";
+			reg = <0 0x01720000 0 0x7000>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
+		mmss_noc: interconnect@1740000 {
+			compatible = "qcom,sc8180x-mmss-noc";
+			reg = <0 0x01740000 0 0x1c100>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
 		ufs_mem_hc: ufshc@1d84000 {
 			compatible = "qcom,sc8180x-ufshc", "qcom,ufshc",
 				     "jedec,ufs-2.0";
@@ -810,6 +888,13 @@  ufs_mem_phy_lanes: phy@1d87400 {
 			};
 		};
 
+		ipa_virt: interconnect@1e00000 {
+			compatible = "qcom,sc8180x-ipa-virt";
+			reg = <0 0x01e00000 0 0x1000>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
 		tcsr_mutex: hwlock@1f40000 {
 			compatible = "qcom,tcsr-mutex";
 			reg = <0x0 0x01f40000 0x0 0x40000>;
@@ -860,6 +945,13 @@  system-cache-controller@9200000 {
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		gem_noc: interconnect@9680000 {
+			compatible = "qcom,sc8180x-gem-noc";
+			reg = <0 0x09680000 0 0x58200>;
+			#interconnect-cells = <2>;
+			qcom,bcm-voters = <&apps_bcm_voter>;
+		};
+
 		pdc: interrupt-controller@b220000 {
 			compatible = "qcom,sc8180x-pdc", "qcom,pdc";
 			reg = <0 0x0b220000 0 0x30000>;
@@ -1166,6 +1258,40 @@  rpmhpd_opp_turbo_l1: opp10 {
 			};
 		};
 
+		osm_l3: interconnect@18321000 {
+			compatible = "qcom,sc8180x-osm-l3";
+			reg = <0 0x18321000 0 0x1400>;
+
+			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+			clock-names = "xo", "alternate";
+
+			#interconnect-cells = <1>;
+		};
+
+		lmh@18350800 {
+			compatible = "qcom,sc8180x-lmh";
+			reg = <0 0x18350800 0 0x400>;
+			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+			cpus = <&CPU4>;
+			qcom,lmh-temp-arm-millicelsius = <65000>;
+			qcom,lmh-temp-low-millicelsius = <94500>;
+			qcom,lmh-temp-high-millicelsius = <95000>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		lmh@18358800 {
+			compatible = "qcom,sc8180x-lmh";
+			reg = <0 0x18358800 0 0x400>;
+			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+			cpus = <&CPU0>;
+			qcom,lmh-temp-arm-millicelsius = <65000>;
+			qcom,lmh-temp-low-millicelsius = <94500>;
+			qcom,lmh-temp-high-millicelsius = <95000>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		cpufreq_hw: cpufreq@18323000 {
 			compatible = "qcom,cpufreq-hw";
 			reg = <0 0x18323000 0 0x1400>, <0 0x18325800 0 0x1400>;