diff mbox series

[v3,6/8] arm64: dts: qcom: sdm630: use defined symbols for interconnects

Message ID 20220513234518.3068480-7-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series arm64: dts: qcom: initial Inforce IFC6560 board support | expand

Commit Message

Dmitry Baryshkov May 13, 2022, 11:45 p.m. UTC 
Replace numeric values with the symbolic names defined in the bindings
header.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm630.dtsi | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Marijn Suijten May 15, 2022, 2:44 p.m. UTC | #1 
On 2022-05-14 15:51:55, Dmitry Baryshkov wrote:
> On Sat, 14 May 2022 at 12:45, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > On 2022-05-14 02:45:16, Dmitry Baryshkov wrote:
> > > Replace numeric values with the symbolic names defined in the bindings
> > > header.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > Seems there is one off-by-one copy-paste error.  With that addressed:
> >
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >
> > > ---
> > >  arch/arm64/boot/dts/qcom/sdm630.dtsi | 23 ++++++++++++-----------
> > >  1 file changed, 12 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> > > index 17a1877587cf..01a1a1703568 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> > > @@ -8,6 +8,7 @@
> > >  #include <dt-bindings/clock/qcom,gpucc-sdm660.h>
> > >  #include <dt-bindings/clock/qcom,mmcc-sdm660.h>
> > >  #include <dt-bindings/clock/qcom,rpmcc.h>
> > > +#include <dt-bindings/interconnect/qcom,sdm660.h>
> > >  #include <dt-bindings/power/qcom-rpmpd.h>
> > >  #include <dt-bindings/gpio/gpio.h>
> > >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > @@ -1045,7 +1046,7 @@ adreno_gpu: gpu@5000000 {
> > >                       nvmem-cells = <&gpu_speed_bin>;
> > >                       nvmem-cell-names = "speed_bin";
> > >
> > > -                     interconnects = <&gnoc 1 &bimc 5>;
> > > +                     interconnects = <&gnoc MASTER_APSS_PROC &bimc SLAVE_EBI>;
> >
> > From qcom,sdm660.h:
> >
> >     /* GNOC */
> >     #define MASTER_APSS_PROC            0
> >     #define SLAVE_GNOC_BIMC                     1
> >     #define SLAVE_GNOC_SNOC                     2
> >
> > Seems like the left side should be SLAVE_GNOC_BIMC?  Unless this
> > semantic change is intended, in which case it should be clearly
> > documented in its own commit with a Fixes tag.
> 
> I don't think there can be a slave on the left side of the ICC path.
> But nice catch anyway. Downstream uses MSM_BUS_MASTER_GRAPHICS_3D
> here, which corresponds to <&bimc MASTER_OXILI>.
> Could you please double check this?

Agreed, my downstream source for this device also uses
MSM_BUS_MASTER_GRAPHICS_3D=26 with mas_rpm_id=ICBID_MASTER_GFX3D=6, and 
on the right-side MSM_BUS_SLAVE_EBI_CH0=512 which resolves to
slv_rpm_id=ICBID_SLAVE_EBI1=0.  Both on &bimc.

Have you double-checked all the other interconnect paths in this file?

- Marijn

> >
> > The rest looks correct.
> >
> > - Marijn
> [..]
Dmitry Baryshkov May 15, 2022, 6:05 p.m. UTC | #2 
On Sun, 15 May 2022 at 17:44, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2022-05-14 15:51:55, Dmitry Baryshkov wrote:
> > On Sat, 14 May 2022 at 12:45, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > On 2022-05-14 02:45:16, Dmitry Baryshkov wrote:
> > > > Replace numeric values with the symbolic names defined in the bindings
> > > > header.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > Seems there is one off-by-one copy-paste error.  With that addressed:
> > >
> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sdm630.dtsi | 23 ++++++++++++-----------
> > > >  1 file changed, 12 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> > > > index 17a1877587cf..01a1a1703568 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> > > > @@ -8,6 +8,7 @@
> > > >  #include <dt-bindings/clock/qcom,gpucc-sdm660.h>
> > > >  #include <dt-bindings/clock/qcom,mmcc-sdm660.h>
> > > >  #include <dt-bindings/clock/qcom,rpmcc.h>
> > > > +#include <dt-bindings/interconnect/qcom,sdm660.h>
> > > >  #include <dt-bindings/power/qcom-rpmpd.h>
> > > >  #include <dt-bindings/gpio/gpio.h>
> > > >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > @@ -1045,7 +1046,7 @@ adreno_gpu: gpu@5000000 {
> > > >                       nvmem-cells = <&gpu_speed_bin>;
> > > >                       nvmem-cell-names = "speed_bin";
> > > >
> > > > -                     interconnects = <&gnoc 1 &bimc 5>;
> > > > +                     interconnects = <&gnoc MASTER_APSS_PROC &bimc SLAVE_EBI>;
> > >
> > > From qcom,sdm660.h:
> > >
> > >     /* GNOC */
> > >     #define MASTER_APSS_PROC            0
> > >     #define SLAVE_GNOC_BIMC                     1
> > >     #define SLAVE_GNOC_SNOC                     2
> > >
> > > Seems like the left side should be SLAVE_GNOC_BIMC?  Unless this
> > > semantic change is intended, in which case it should be clearly
> > > documented in its own commit with a Fixes tag.
> >
> > I don't think there can be a slave on the left side of the ICC path.
> > But nice catch anyway. Downstream uses MSM_BUS_MASTER_GRAPHICS_3D
> > here, which corresponds to <&bimc MASTER_OXILI>.
> > Could you please double check this?
>
> Agreed, my downstream source for this device also uses
> MSM_BUS_MASTER_GRAPHICS_3D=26 with mas_rpm_id=ICBID_MASTER_GFX3D=6, and
> on the right-side MSM_BUS_SLAVE_EBI_CH0=512 which resolves to
> slv_rpm_id=ICBID_SLAVE_EBI1=0.  Both on &bimc.
>
> Have you double-checked all the other interconnect paths in this file?

Hmmmmm.

SDHCs also seem to be incorrect. For sdhc_1 downstream uses
<MSM_BUS_MASTER_SDCC_1 MSM_BUS_SLAVE_EBI_CH0>,
<MSM_BUS_MASTER_AMPSS_M0 MSM_BUS_SLAVE_SDCC_1>.
For the upstream kernel this translates to <&a2noc MASTER_SDCC_1 &bimc
SLAVE_EBI>, <&gnoc MASTER_APSS_PROC &cnoc SLAVE_SDCC_1> (while we have
<&a2noc MASTER_SDCC_1 &a2noc SLAVE_A2NOC_SNOC>, <&gnoc
MASTER_APSS_PROC &cnoc SLAVE_SDCC_1>). Same applies for the sdhc2.

AngeloGioacchino, since SDHC interconnects were added by you, could
you please check?

For camss I suppose that downstream uses MASTER_CPP rather than
MASTER_VFE. If I'm not mistaken.

For venus the downstream path is equivalent <&mnoc MASTER_VENUS &bimc
SLAVE_EBI> (which we have here). vidc describes <&gnoc
MASTER_APSS_PROC &mnoc SLAVE_VENUS_CFG>. So venus cfg looks correct.

>
> - Marijn
>
> > >
> > > The rest looks correct.
> > >
> > > - Marijn
> > [..]
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
index 17a1877587cf..01a1a1703568 100644
--- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
@@ -8,6 +8,7 @@ 
 #include <dt-bindings/clock/qcom,gpucc-sdm660.h>
 #include <dt-bindings/clock/qcom,mmcc-sdm660.h>
 #include <dt-bindings/clock/qcom,rpmcc.h>
+#include <dt-bindings/interconnect/qcom,sdm660.h>
 #include <dt-bindings/power/qcom-rpmpd.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -1045,7 +1046,7 @@  adreno_gpu: gpu@5000000 {
 			nvmem-cells = <&gpu_speed_bin>;
 			nvmem-cell-names = "speed_bin";
 
-			interconnects = <&gnoc 1 &bimc 5>;
+			interconnects = <&gnoc MASTER_APSS_PROC &bimc SLAVE_EBI>;
 			interconnect-names = "gfx-mem";
 
 			operating-points-v2 = <&gpu_sdm630_opp_table>;
@@ -1299,8 +1300,8 @@  sdhc_2: sdhci@c084000 {
 					<&xo_board>;
 			clock-names = "core", "iface", "xo";
 
-			interconnects = <&a2noc 3 &a2noc 10>,
-					<&gnoc 0 &cnoc 28>;
+			interconnects = <&a2noc MASTER_SDCC_2 &a2noc SLAVE_A2NOC_SNOC>,
+					<&gnoc MASTER_APSS_PROC &cnoc SLAVE_SDCC_2>;
 			operating-points-v2 = <&sdhc2_opp_table>;
 
 			pinctrl-names = "default", "sleep";
@@ -1351,8 +1352,8 @@  sdhc_1: sdhci@c0c4000 {
 				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
 			clock-names = "core", "iface", "xo", "ice";
 
-			interconnects = <&a2noc 2 &a2noc 10>,
-					<&gnoc 0 &cnoc 27>;
+			interconnects = <&a2noc MASTER_SDCC_1 &a2noc SLAVE_A2NOC_SNOC>,
+					<&gnoc MASTER_APSS_PROC &cnoc SLAVE_SDCC_1>;
 			interconnect-names = "sdhc1-ddr", "cpu-sdhc1";
 			operating-points-v2 = <&sdhc1_opp_table>;
 			pinctrl-names = "default", "sleep";
@@ -1525,9 +1526,9 @@  mdp: mdp@c901000 {
 					      "core",
 					      "vsync";
 
-				interconnects = <&mnoc 2 &bimc 5>,
-						<&mnoc 3 &bimc 5>,
-						<&gnoc 0 &mnoc 17>;
+				interconnects = <&mnoc MASTER_MDP_P0 &bimc SLAVE_EBI>,
+						<&mnoc MASTER_MDP_P1 &bimc SLAVE_EBI>,
+						<&gnoc MASTER_APSS_PROC &mnoc SLAVE_DISPLAY_CFG>;
 				interconnect-names = "mdp0-mem",
 						     "mdp1-mem",
 						     "rotator-mem";
@@ -2034,7 +2035,7 @@  camss: camss@ca00000 {
 				"cphy_csid1",
 				"cphy_csid2",
 				"cphy_csid3";
-			interconnects = <&mnoc 5 &bimc 5>;
+			interconnects = <&mnoc MASTER_VFE &bimc SLAVE_EBI>;
 			interconnect-names = "vfe-mem";
 			iommus = <&mmss_smmu 0xc00>,
 				 <&mmss_smmu 0xc01>,
@@ -2097,8 +2098,8 @@  venus: video-codec@cc00000 {
 				 <&mmcc VIDEO_AXI_CLK>,
 				 <&mmcc THROTTLE_VIDEO_AXI_CLK>;
 			clock-names = "core", "iface", "bus", "bus_throttle";
-			interconnects = <&gnoc 0 &mnoc 13>,
-					<&mnoc 4 &bimc 5>;
+			interconnects = <&gnoc MASTER_APSS_PROC &mnoc SLAVE_VENUS_CFG>,
+					<&mnoc MASTER_VENUS &bimc SLAVE_EBI>;
 			interconnect-names = "cpu-cfg", "video-mem";
 			interrupts = <GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>;
 			iommus = <&mmss_smmu 0x400>,