diff mbox series

[5/5] ARM: dts: qcom: msm8974: add interconnect nodes

Message ID 20191013080804.10231-6-masneyb@onstation.org (mailing list archive)
State Superseded
Headers show
Series ARM: qcom: add defconfig items and dts nodes | expand

Commit Message

Brian Masney Oct. 13, 2019, 8:08 a.m. UTC
Add interconnect nodes that's needed to support bus scaling.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Georgi Djakov Oct. 23, 2019, 11:50 a.m. UTC | #1
Hi Brian,

Thanks for the patch!

On 13.10.19 г. 11:08 ч., Brian Masney wrote:
> Add interconnect nodes that's needed to support bus scaling.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> index bdbde5125a56..ed98d14a88b1 100644
> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /dts-v1/;
>  
> +#include <dt-bindings/interconnect/qcom,msm8974.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-msm8974.h>
>  #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> @@ -1106,6 +1107,60 @@
>  			};
>  		};
>  
> +		bimc: interconnect@fc380000 {
> +			reg = <0xfc380000 0x6a000>;
> +			compatible = "qcom,msm8974-bimc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
> +			         <&rpmcc RPM_SMD_BIMC_A_CLK>;
> +		};
> +
> +		cnoc: interconnect@fc480000 {
> +			reg = <0xfc480000 0x4000>;
> +			compatible = "qcom,msm8974-cnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_CNOC_CLK>,
> +			         <&rpmcc RPM_SMD_CNOC_A_CLK>;
> +		};
> +
> +		mmssnoc: interconnect@fc478000 {
> +			reg = <0xfc478000 0x4000>;
> +			compatible = "qcom,msm8974-mmssnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&mmcc MMSS_S0_AXI_CLK>,
> +			         <&mmcc MMSS_S0_AXI_CLK>;
> +		};
> +
> +		ocmemnoc: interconnect@fc470000 {
> +			reg = <0xfc470000 0x4000>;
> +			compatible = "qcom,msm8974-ocmemnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>,
> +			         <&rpmcc RPM_SMD_OCMEMGX_A_CLK>;
> +		};
> +
> +		pnoc: interconnect@fc468000 {
> +			reg = <0xfc468000 0x4000>;
> +			compatible = "qcom,msm8974-pnoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
> +			         <&rpmcc RPM_SMD_PNOC_A_CLK>;
> +		};
> +
> +		snoc: interconnect@fc460000 {
> +			reg = <0xfc460000 0x4000>;
> +			compatible = "qcom,msm8974-snoc";
> +			#interconnect-cells = <1>;
> +			clock-names = "bus", "bus_a";
> +			clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
> +			         <&rpmcc RPM_SMD_SNOC_A_CLK>;
> +		};

It would have been nice to have the DT nodes sorted by address, but i suppose it
doesn't make much difference, as the rest of the nodes in this file are unsorted
anyway.

> +
>  		mdss: mdss@fd900000 {
>  			status = "disabled";
>  
> @@ -1152,6 +1207,11 @@
>  				              "core",
>  				              "vsync";
>  
> +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
> +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;

Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
or both? The above seem like GPU-related interconnects, so maybe these
properties should be in the GPU DT node.

> +				interconnect-names = "mdp0-mem",
> +				                     "mdp1-mem";

As the second path is not to DDR, but to ocmem, it might be better to call it
something like "gpu-ocmem".

Thanks,
Georgi

> +
>  				ports {
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>
Brian Masney Oct. 23, 2019, 12:47 p.m. UTC | #2
On Wed, Oct 23, 2019 at 02:50:19PM +0300, Georgi Djakov wrote:
> On 13.10.19 г. 11:08 ч., Brian Masney wrote:
> > Add interconnect nodes that's needed to support bus scaling.
> > 
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > ---
> >  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> > @@ -1152,6 +1207,11 @@
> >  				              "core",
> >  				              "vsync";
> >  
> > +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
> > +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;
> 
> Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
> or both? The above seem like GPU-related interconnects, so maybe these
> properties should be in the GPU DT node.

The display is what currently requests the interconnect path,
specifically mdp5_setup_interconnect() in
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c. The Freedreno GPU bindings
currently don't have interconnect support. Maybe this is something that
I should add to that driver as well?

> > +				interconnect-names = "mdp0-mem",
> > +				                     "mdp1-mem";
> 
> As the second path is not to DDR, but to ocmem, it might be better to call it
> something like "gpu-ocmem".

I used what mdp5_kms.c expected.

Brian
Georgi Djakov Oct. 23, 2019, 1:39 p.m. UTC | #3
On 23.10.19 г. 15:47 ч., Brian Masney wrote:
> On Wed, Oct 23, 2019 at 02:50:19PM +0300, Georgi Djakov wrote:
>> On 13.10.19 г. 11:08 ч., Brian Masney wrote:
>>> Add interconnect nodes that's needed to support bus scaling.
>>>
>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>>> ---
>>>  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
>>>  1 file changed, 60 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
>>> @@ -1152,6 +1207,11 @@
>>>  				              "core",
>>>  				              "vsync";
>>>  
>>> +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
>>> +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;
>>
>> Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
>> or both? The above seem like GPU-related interconnects, so maybe these
>> properties should be in the GPU DT node.
> 
> The display is what currently requests the interconnect path,
> specifically mdp5_setup_interconnect() in
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c. The Freedreno GPU bindings
> currently don't have interconnect support. Maybe this is something that
> I should add to that driver as well?

The "mdp0-mem" and "mdp1-mem" paths mentioned in the mdp5_kms.c are the two
interconnects between the display and DDR memory. There is actually a patch [1]
to add to GPU bindings, but it seems that we missed to pick it up.

> 
>>> +				interconnect-names = "mdp0-mem",
>>> +				                     "mdp1-mem";
>>
>> As the second path is not to DDR, but to ocmem, it might be better to call it
>> something like "gpu-ocmem".
> 
> I used what mdp5_kms.c expected.

This is for the display and here [2] are some patches for the GPU. Not sure how
the NoCs are configured on 8974 by default, but if you notice any blue/black
screens, you may need to request bandwidth for display too.

Thanks,
Georgi

[1]
https://lore.kernel.org/r/1555703787-10897-1-git-send-email-jcrouse@codeaurora.org
[2] https://lore.kernel.org/r/20181220173026.3857-1-jcrouse@codeaurora.org
Georgi Djakov Oct. 23, 2019, 1:51 p.m. UTC | #4
On 23.10.19 г. 16:39 ч., Georgi Djakov wrote:
> On 23.10.19 г. 15:47 ч., Brian Masney wrote:
>> On Wed, Oct 23, 2019 at 02:50:19PM +0300, Georgi Djakov wrote:
>>> On 13.10.19 г. 11:08 ч., Brian Masney wrote:
>>>> Add interconnect nodes that's needed to support bus scaling.
>>>>
>>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>>>> ---
>>>>  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
>>>>  1 file changed, 60 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
>>>> @@ -1152,6 +1207,11 @@
>>>>  				              "core",
>>>>  				              "vsync";
>>>>  
>>>> +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
>>>> +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;
>>>
>>> Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
>>> or both? The above seem like GPU-related interconnects, so maybe these
>>> properties should be in the GPU DT node.
>>
>> The display is what currently requests the interconnect path,
>> specifically mdp5_setup_interconnect() in
>> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c. The Freedreno GPU bindings
>> currently don't have interconnect support. Maybe this is something that
>> I should add to that driver as well?
> 
> The "mdp0-mem" and "mdp1-mem" paths mentioned in the mdp5_kms.c are the two
> interconnects between the display and DDR memory. There is actually a patch [1]
> to add to GPU bindings, but it seems that we missed to pick it up.

It was actually merged in v5.2.

BR,
Georgi

>>>> +				interconnect-names = "mdp0-mem",
>>>> +				                     "mdp1-mem";
>>>
>>> As the second path is not to DDR, but to ocmem, it might be better to call it
>>> something like "gpu-ocmem".
>>
>> I used what mdp5_kms.c expected.
> 
> This is for the display and here [2] are some patches for the GPU. Not sure how
> the NoCs are configured on 8974 by default, but if you notice any blue/black
> screens, you may need to request bandwidth for display too.
> 
> Thanks,
> Georgi
> 
> [1]
> https://lore.kernel.org/r/1555703787-10897-1-git-send-email-jcrouse@codeaurora.org
> [2] https://lore.kernel.org/r/20181220173026.3857-1-jcrouse@codeaurora.org
>
Brian Masney Oct. 24, 2019, 7:07 a.m. UTC | #5
On Wed, Oct 23, 2019 at 04:39:21PM +0300, Georgi Djakov wrote:
> On 23.10.19 г. 15:47 ч., Brian Masney wrote:
> > On Wed, Oct 23, 2019 at 02:50:19PM +0300, Georgi Djakov wrote:
> >> On 13.10.19 г. 11:08 ч., Brian Masney wrote:
> >>> Add interconnect nodes that's needed to support bus scaling.
> >>>
> >>> Signed-off-by: Brian Masney <masneyb@onstation.org>
> >>> ---
> >>>  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
> >>>  1 file changed, 60 insertions(+)
> >>>
> >>> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
> >>> @@ -1152,6 +1207,11 @@
> >>>  				              "core",
> >>>  				              "vsync";
> >>>  
> >>> +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
> >>> +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;
> >>
> >> Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
> >> or both? The above seem like GPU-related interconnects, so maybe these
> >> properties should be in the GPU DT node.
> > 
> > The display is what currently requests the interconnect path,
> > specifically mdp5_setup_interconnect() in
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c. The Freedreno GPU bindings
> > currently don't have interconnect support. Maybe this is something that
> > I should add to that driver as well?
> 
> The "mdp0-mem" and "mdp1-mem" paths mentioned in the mdp5_kms.c are the two
> interconnects between the display and DDR memory.

OK, I see. Most of the interconnect paths in the downstream MSM 3.4
sources are configured in device tree using the
qcom,msm-bus,vectors-KBps property, which is what I was only looking at
before. The interconnect path for the display is configured directly in
code (drivers/video/msm/mdss/mdss_mdp.c) to setup a path between
MSM_BUS_MASTER_MDP_PORT0 and MSM_BUS_SLAVE_EBI_CH0.

In the upstream kernel, it looks like I'll need to

  1) add support for an optional second interconnect path for ocmem to
     drivers/gpu/drm/msm/adreno/adreno_gpu.c.

  2) add implementations of gpu_get_freq and gpu_get_freq to the
     adreno_gpu_funcs struct in drivers/gpu/drm/msm/adreno/a3xx_gpu.c.

Brian
Georgi Djakov Oct. 24, 2019, 8:27 a.m. UTC | #6
On 24.10.19 г. 10:07 ч., Brian Masney wrote:
> On Wed, Oct 23, 2019 at 04:39:21PM +0300, Georgi Djakov wrote:
>> On 23.10.19 г. 15:47 ч., Brian Masney wrote:
>>> On Wed, Oct 23, 2019 at 02:50:19PM +0300, Georgi Djakov wrote:
>>>> On 13.10.19 г. 11:08 ч., Brian Masney wrote:
>>>>> Add interconnect nodes that's needed to support bus scaling.
>>>>>
>>>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>>>>> ---
>>>>>  arch/arm/boot/dts/qcom-msm8974.dtsi | 60 +++++++++++++++++++++++++++++
>>>>>  1 file changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
>>>>> @@ -1152,6 +1207,11 @@
>>>>>  				              "core",
>>>>>  				              "vsync";
>>>>>  
>>>>> +				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
>>>>> +				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;
>>>>
>>>> Who will be the requesting bandwidth to DDR and ocmem? Is it the display or GPU
>>>> or both? The above seem like GPU-related interconnects, so maybe these
>>>> properties should be in the GPU DT node.
>>>
>>> The display is what currently requests the interconnect path,
>>> specifically mdp5_setup_interconnect() in
>>> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c. The Freedreno GPU bindings
>>> currently don't have interconnect support. Maybe this is something that
>>> I should add to that driver as well?
>>
>> The "mdp0-mem" and "mdp1-mem" paths mentioned in the mdp5_kms.c are the two
>> interconnects between the display and DDR memory.
> 
> OK, I see. Most of the interconnect paths in the downstream MSM 3.4
> sources are configured in device tree using the
> qcom,msm-bus,vectors-KBps property, which is what I was only looking at
> before. The interconnect path for the display is configured directly in
> code (drivers/video/msm/mdss/mdss_mdp.c) to setup a path between
> MSM_BUS_MASTER_MDP_PORT0 and MSM_BUS_SLAVE_EBI_CH0.

Correct!

> 
> In the upstream kernel, it looks like I'll need to
> 
>   1) add support for an optional second interconnect path for ocmem to
>      drivers/gpu/drm/msm/adreno/adreno_gpu.c.

Yes, just check if there is a "gpu-ocmem" path in DT and scale it when needed.

> 
>   2) add implementations of gpu_get_freq and gpu_get_freq to the
>      adreno_gpu_funcs struct in drivers/gpu/drm/msm/adreno/a3xx_gpu.c.
> 

Maybe, i am not very familiar with adreno stuff. It might be good to CC the
freedreno guys.

Thanks,
Georgi
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index bdbde5125a56..ed98d14a88b1 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /dts-v1/;
 
+#include <dt-bindings/interconnect/qcom,msm8974.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,gcc-msm8974.h>
 #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
@@ -1106,6 +1107,60 @@ 
 			};
 		};
 
+		bimc: interconnect@fc380000 {
+			reg = <0xfc380000 0x6a000>;
+			compatible = "qcom,msm8974-bimc";
+			#interconnect-cells = <1>;
+			clock-names = "bus", "bus_a";
+			clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
+			         <&rpmcc RPM_SMD_BIMC_A_CLK>;
+		};
+
+		cnoc: interconnect@fc480000 {
+			reg = <0xfc480000 0x4000>;
+			compatible = "qcom,msm8974-cnoc";
+			#interconnect-cells = <1>;
+			clock-names = "bus", "bus_a";
+			clocks = <&rpmcc RPM_SMD_CNOC_CLK>,
+			         <&rpmcc RPM_SMD_CNOC_A_CLK>;
+		};
+
+		mmssnoc: interconnect@fc478000 {
+			reg = <0xfc478000 0x4000>;
+			compatible = "qcom,msm8974-mmssnoc";
+			#interconnect-cells = <1>;
+			clock-names = "bus", "bus_a";
+			clocks = <&mmcc MMSS_S0_AXI_CLK>,
+			         <&mmcc MMSS_S0_AXI_CLK>;
+		};
+
+		ocmemnoc: interconnect@fc470000 {
+			reg = <0xfc470000 0x4000>;
+			compatible = "qcom,msm8974-ocmemnoc";
+			#interconnect-cells = <1>;
+			clock-names = "bus", "bus_a";
+			clocks = <&rpmcc RPM_SMD_OCMEMGX_CLK>,
+			         <&rpmcc RPM_SMD_OCMEMGX_A_CLK>;
+		};
+
+		pnoc: interconnect@fc468000 {
+			reg = <0xfc468000 0x4000>;
+			compatible = "qcom,msm8974-pnoc";
+			#interconnect-cells = <1>;
+			clock-names = "bus", "bus_a";
+			clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
+			         <&rpmcc RPM_SMD_PNOC_A_CLK>;
+		};
+
+		snoc: interconnect@fc460000 {
+			reg = <0xfc460000 0x4000>;
+			compatible = "qcom,msm8974-snoc";
+			#interconnect-cells = <1>;
+			clock-names = "bus", "bus_a";
+			clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
+			         <&rpmcc RPM_SMD_SNOC_A_CLK>;
+		};
+
 		mdss: mdss@fd900000 {
 			status = "disabled";
 
@@ -1152,6 +1207,11 @@ 
 				              "core",
 				              "vsync";
 
+				interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
+				                <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc OCMEM_SLV_OCMEM>;
+				interconnect-names = "mdp0-mem",
+				                     "mdp1-mem";
+
 				ports {
 					#address-cells = <1>;
 					#size-cells = <0>;