[PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node

Krzysztof Kozlowski posted 1 patch 1 year, 1 month ago
arch/arm64/boot/dts/qcom/sdm630.dtsi | 38 ++++++++++++++--------------
1 file changed, 19 insertions(+), 19 deletions(-)
[PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Krzysztof Kozlowski 1 year, 1 month ago
The soc node is supposed to have only device nodes with MMIO addresses,
so move the DSI OPP out of it (it is used also by second DSI1 on
SDM660):

  sda660-inforce-ifc6560.dtb: soc: opp-table-dsi: {'compatible': ['operating-points-v2'], ... should not be valid under {'type': 'object'}
    From schema: dtschema/schemas/simple-bus.yaml

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes since v1:
1. Move the node out of soc. Don't add Konrad's review tag.
---
 arch/arm64/boot/dts/qcom/sdm630.dtsi | 38 ++++++++++++++--------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
index 72d9a12b5e9c..b91e423a3cfc 100644
--- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
@@ -328,6 +328,25 @@ memory@80000000 {
 		reg = <0x0 0x80000000 0x0 0x0>;
 	};
 
+	dsi_opp_table: opp-table-dsi {
+		compatible = "operating-points-v2";
+
+		opp-131250000 {
+			opp-hz = /bits/ 64 <131250000>;
+			required-opps = <&rpmpd_opp_svs>;
+		};
+
+		opp-210000000 {
+			opp-hz = /bits/ 64 <210000000>;
+			required-opps = <&rpmpd_opp_svs_plus>;
+		};
+
+		opp-262500000 {
+			opp-hz = /bits/ 64 <262500000>;
+			required-opps = <&rpmpd_opp_nom>;
+		};
+	};
+
 	pmu {
 		compatible = "arm,armv8-pmuv3";
 		interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
@@ -1450,25 +1469,6 @@ mmcc: clock-controller@c8c0000 {
 					<0>;
 		};
 
-		dsi_opp_table: opp-table-dsi {
-			compatible = "operating-points-v2";
-
-			opp-131250000 {
-				opp-hz = /bits/ 64 <131250000>;
-				required-opps = <&rpmpd_opp_svs>;
-			};
-
-			opp-210000000 {
-				opp-hz = /bits/ 64 <210000000>;
-				required-opps = <&rpmpd_opp_svs_plus>;
-			};
-
-			opp-262500000 {
-				opp-hz = /bits/ 64 <262500000>;
-				required-opps = <&rpmpd_opp_nom>;
-			};
-		};
-
 		mdss: display-subsystem@c900000 {
 			compatible = "qcom,mdss";
 			reg = <0x0c900000 0x1000>,
-- 
2.34.1
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Bjorn Andersson 1 year ago
On Sun, Mar 26, 2023 at 11:16:05AM +0200, Krzysztof Kozlowski wrote:
> The soc node is supposed to have only device nodes with MMIO addresses,
> so move the DSI OPP out of it (it is used also by second DSI1 on
> SDM660):
> 

This node has been moved into the dsi node, so if we still want this,
could you please update the commit message.

Regards,
Bjorn

>   sda660-inforce-ifc6560.dtb: soc: opp-table-dsi: {'compatible': ['operating-points-v2'], ... should not be valid under {'type': 'object'}
>     From schema: dtschema/schemas/simple-bus.yaml
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Changes since v1:
> 1. Move the node out of soc. Don't add Konrad's review tag.
> ---
>  arch/arm64/boot/dts/qcom/sdm630.dtsi | 38 ++++++++++++++--------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> index 72d9a12b5e9c..b91e423a3cfc 100644
> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> @@ -328,6 +328,25 @@ memory@80000000 {
>  		reg = <0x0 0x80000000 0x0 0x0>;
>  	};
>  
> +	dsi_opp_table: opp-table-dsi {
> +		compatible = "operating-points-v2";
> +
> +		opp-131250000 {
> +			opp-hz = /bits/ 64 <131250000>;
> +			required-opps = <&rpmpd_opp_svs>;
> +		};
> +
> +		opp-210000000 {
> +			opp-hz = /bits/ 64 <210000000>;
> +			required-opps = <&rpmpd_opp_svs_plus>;
> +		};
> +
> +		opp-262500000 {
> +			opp-hz = /bits/ 64 <262500000>;
> +			required-opps = <&rpmpd_opp_nom>;
> +		};
> +	};
> +
>  	pmu {
>  		compatible = "arm,armv8-pmuv3";
>  		interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
> @@ -1450,25 +1469,6 @@ mmcc: clock-controller@c8c0000 {
>  					<0>;
>  		};
>  
> -		dsi_opp_table: opp-table-dsi {
> -			compatible = "operating-points-v2";
> -
> -			opp-131250000 {
> -				opp-hz = /bits/ 64 <131250000>;
> -				required-opps = <&rpmpd_opp_svs>;
> -			};
> -
> -			opp-210000000 {
> -				opp-hz = /bits/ 64 <210000000>;
> -				required-opps = <&rpmpd_opp_svs_plus>;
> -			};
> -
> -			opp-262500000 {
> -				opp-hz = /bits/ 64 <262500000>;
> -				required-opps = <&rpmpd_opp_nom>;
> -			};
> -		};
> -
>  		mdss: display-subsystem@c900000 {
>  			compatible = "qcom,mdss";
>  			reg = <0x0c900000 0x1000>,
> -- 
> 2.34.1
>
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Krzysztof Kozlowski 1 year ago
On 27/03/2023 21:39, Bjorn Andersson wrote:
> On Sun, Mar 26, 2023 at 11:16:05AM +0200, Krzysztof Kozlowski wrote:
>> The soc node is supposed to have only device nodes with MMIO addresses,
>> so move the DSI OPP out of it (it is used also by second DSI1 on
>> SDM660):
>>
> 
> This node has been moved into the dsi node, so if we still want this,
> could you please update the commit message.

The OPP table has been moved *out of* DSI node. The v1 was moving
inside, but this was not good approach, thus v2 moves it out.

I don't understand what shall be updated here.



Best regards,
Krzysztof
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Bjorn Andersson 1 year ago
On Mon, Mar 27, 2023 at 09:39:00PM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 21:39, Bjorn Andersson wrote:
> > On Sun, Mar 26, 2023 at 11:16:05AM +0200, Krzysztof Kozlowski wrote:
> >> The soc node is supposed to have only device nodes with MMIO addresses,
> >> so move the DSI OPP out of it (it is used also by second DSI1 on
> >> SDM660):
> >>
> > 
> > This node has been moved into the dsi node, so if we still want this,
> > could you please update the commit message.
> 
> The OPP table has been moved *out of* DSI node. The v1 was moving
> inside, but this was not good approach, thus v2 moves it out.
> 
> I don't understand what shall be updated here.
> 

The commit message doesn't reflect what's in linux-next today and the
patch doesn't apply.

Regards,
Bjorn
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Krzysztof Kozlowski 1 year ago
On 07/04/2023 18:34, Bjorn Andersson wrote:
> On Mon, Mar 27, 2023 at 09:39:00PM +0200, Krzysztof Kozlowski wrote:
>> On 27/03/2023 21:39, Bjorn Andersson wrote:
>>> On Sun, Mar 26, 2023 at 11:16:05AM +0200, Krzysztof Kozlowski wrote:
>>>> The soc node is supposed to have only device nodes with MMIO addresses,
>>>> so move the DSI OPP out of it (it is used also by second DSI1 on
>>>> SDM660):
>>>>
>>>
>>> This node has been moved into the dsi node, so if we still want this,
>>> could you please update the commit message.
>>
>> The OPP table has been moved *out of* DSI node. The v1 was moving
>> inside, but this was not good approach, thus v2 moves it out.
>>
>> I don't understand what shall be updated here.
>>
> 
> The commit message doesn't reflect what's in linux-next today and the
> patch doesn't apply.
> 

It seems you applied v1. It's okay, yet would be nice to clean up so I
will send a follow-up patch.

Best regards,
Krzysztof
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Konrad Dybcio 1 year ago

On 26.03.2023 11:16, Krzysztof Kozlowski wrote:
> The soc node is supposed to have only device nodes with MMIO addresses,
> so move the DSI OPP out of it (it is used also by second DSI1 on
> SDM660):
> 
>   sda660-inforce-ifc6560.dtb: soc: opp-table-dsi: {'compatible': ['operating-points-v2'], ... should not be valid under {'type': 'object'}
>     From schema: dtschema/schemas/simple-bus.yaml
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> 
> Changes since v1:
> 1. Move the node out of soc. Don't add Konrad's review tag.
> ---
>  arch/arm64/boot/dts/qcom/sdm630.dtsi | 38 ++++++++++++++--------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> index 72d9a12b5e9c..b91e423a3cfc 100644
> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> @@ -328,6 +328,25 @@ memory@80000000 {
>  		reg = <0x0 0x80000000 0x0 0x0>;
>  	};
>  
> +	dsi_opp_table: opp-table-dsi {
> +		compatible = "operating-points-v2";
> +
> +		opp-131250000 {
> +			opp-hz = /bits/ 64 <131250000>;
> +			required-opps = <&rpmpd_opp_svs>;
> +		};
> +
> +		opp-210000000 {
> +			opp-hz = /bits/ 64 <210000000>;
> +			required-opps = <&rpmpd_opp_svs_plus>;
> +		};
> +
> +		opp-262500000 {
> +			opp-hz = /bits/ 64 <262500000>;
> +			required-opps = <&rpmpd_opp_nom>;
> +		};
> +	};
> +
>  	pmu {
>  		compatible = "arm,armv8-pmuv3";
>  		interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
> @@ -1450,25 +1469,6 @@ mmcc: clock-controller@c8c0000 {
>  					<0>;
>  		};
>  
> -		dsi_opp_table: opp-table-dsi {
> -			compatible = "operating-points-v2";
> -
> -			opp-131250000 {
> -				opp-hz = /bits/ 64 <131250000>;
> -				required-opps = <&rpmpd_opp_svs>;
> -			};
> -
> -			opp-210000000 {
> -				opp-hz = /bits/ 64 <210000000>;
> -				required-opps = <&rpmpd_opp_svs_plus>;
> -			};
> -
> -			opp-262500000 {
> -				opp-hz = /bits/ 64 <262500000>;
> -				required-opps = <&rpmpd_opp_nom>;
> -			};
> -		};
> -
>  		mdss: display-subsystem@c900000 {
>  			compatible = "qcom,mdss";
>  			reg = <0x0c900000 0x1000>,
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Dmitry Baryshkov 1 year, 1 month ago
On Sun, 26 Mar 2023 at 12:16, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> The soc node is supposed to have only device nodes with MMIO addresses,
> so move the DSI OPP out of it (it is used also by second DSI1 on
> SDM660):

This raises a question: would it make sense to add /opps to handle all
opp tables?

>
>   sda660-inforce-ifc6560.dtb: soc: opp-table-dsi: {'compatible': ['operating-points-v2'], ... should not be valid under {'type': 'object'}
>     From schema: dtschema/schemas/simple-bus.yaml
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Changes since v1:
> 1. Move the node out of soc. Don't add Konrad's review tag.
> ---
>  arch/arm64/boot/dts/qcom/sdm630.dtsi | 38 ++++++++++++++--------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> index 72d9a12b5e9c..b91e423a3cfc 100644
> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> @@ -328,6 +328,25 @@ memory@80000000 {
>                 reg = <0x0 0x80000000 0x0 0x0>;
>         };
>
> +       dsi_opp_table: opp-table-dsi {
> +               compatible = "operating-points-v2";
> +
> +               opp-131250000 {
> +                       opp-hz = /bits/ 64 <131250000>;
> +                       required-opps = <&rpmpd_opp_svs>;
> +               };
> +
> +               opp-210000000 {
> +                       opp-hz = /bits/ 64 <210000000>;
> +                       required-opps = <&rpmpd_opp_svs_plus>;
> +               };
> +
> +               opp-262500000 {
> +                       opp-hz = /bits/ 64 <262500000>;
> +                       required-opps = <&rpmpd_opp_nom>;
> +               };
> +       };
> +
>         pmu {
>                 compatible = "arm,armv8-pmuv3";
>                 interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
> @@ -1450,25 +1469,6 @@ mmcc: clock-controller@c8c0000 {
>                                         <0>;
>                 };
>
> -               dsi_opp_table: opp-table-dsi {
> -                       compatible = "operating-points-v2";
> -
> -                       opp-131250000 {
> -                               opp-hz = /bits/ 64 <131250000>;
> -                               required-opps = <&rpmpd_opp_svs>;
> -                       };
> -
> -                       opp-210000000 {
> -                               opp-hz = /bits/ 64 <210000000>;
> -                               required-opps = <&rpmpd_opp_svs_plus>;
> -                       };
> -
> -                       opp-262500000 {
> -                               opp-hz = /bits/ 64 <262500000>;
> -                               required-opps = <&rpmpd_opp_nom>;
> -                       };
> -               };
> -
>                 mdss: display-subsystem@c900000 {
>                         compatible = "qcom,mdss";
>                         reg = <0x0c900000 0x1000>,
> --
> 2.34.1
>


-- 
With best wishes
Dmitry
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Krzysztof Kozlowski 1 year, 1 month ago
On 26/03/2023 11:21, Dmitry Baryshkov wrote:
> On Sun, 26 Mar 2023 at 12:16, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> The soc node is supposed to have only device nodes with MMIO addresses,
>> so move the DSI OPP out of it (it is used also by second DSI1 on
>> SDM660):
> 
> This raises a question: would it make sense to add /opps to handle all
> opp tables?

We didn't add it to any other cases like this (and we already fixed all
other boards), so why now? We can but it is a bit late for it.

Best regards,
Krzysztof
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Dmitry Baryshkov 1 year, 1 month ago
On Sun, 26 Mar 2023 at 12:22, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/03/2023 11:21, Dmitry Baryshkov wrote:
> > On Sun, 26 Mar 2023 at 12:16, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> The soc node is supposed to have only device nodes with MMIO addresses,
> >> so move the DSI OPP out of it (it is used also by second DSI1 on
> >> SDM660):
> >
> > This raises a question: would it make sense to add /opps to handle all
> > opp tables?
>
> We didn't add it to any other cases like this (and we already fixed all
> other boards), so why now? We can but it is a bit late for it.

Because nobody expressed this idea beforehand? I'm not insisting here,
you have a better understanding of DT. Just wondering if it makes
sense.

-- 
With best wishes
Dmitry
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Krzysztof Kozlowski 1 year, 1 month ago
On 26/03/2023 12:03, Dmitry Baryshkov wrote:
> On Sun, 26 Mar 2023 at 12:22, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 26/03/2023 11:21, Dmitry Baryshkov wrote:
>>> On Sun, 26 Mar 2023 at 12:16, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> The soc node is supposed to have only device nodes with MMIO addresses,
>>>> so move the DSI OPP out of it (it is used also by second DSI1 on
>>>> SDM660):
>>>
>>> This raises a question: would it make sense to add /opps to handle all
>>> opp tables?
>>
>> We didn't add it to any other cases like this (and we already fixed all
>> other boards), so why now? We can but it is a bit late for it.
> 
> Because nobody expressed this idea beforehand? I'm not insisting here,
> you have a better understanding of DT. Just wondering if it makes
> sense.

It will not change much of ordering - all nodes will be close to each
other anyway (opp-table-XYZ), thus is rather a matter of readability and
subjective preference. No other platforms have "opps" or "opp-tables".

Best regards,
Krzysztof
Re: [PATCH v2] arm64: dts: qcom: sdm630: move DSI opp-table out of soc node
Posted by Dmitry Baryshkov 1 year, 1 month ago
On Sun, 26 Mar 2023 at 13:13, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/03/2023 12:03, Dmitry Baryshkov wrote:
> > On Sun, 26 Mar 2023 at 12:22, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 26/03/2023 11:21, Dmitry Baryshkov wrote:
> >>> On Sun, 26 Mar 2023 at 12:16, Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> The soc node is supposed to have only device nodes with MMIO addresses,
> >>>> so move the DSI OPP out of it (it is used also by second DSI1 on
> >>>> SDM660):
> >>>
> >>> This raises a question: would it make sense to add /opps to handle all
> >>> opp tables?
> >>
> >> We didn't add it to any other cases like this (and we already fixed all
> >> other boards), so why now? We can but it is a bit late for it.
> >
> > Because nobody expressed this idea beforehand? I'm not insisting here,
> > you have a better understanding of DT. Just wondering if it makes
> > sense.
>
> It will not change much of ordering - all nodes will be close to each
> other anyway (opp-table-XYZ), thus is rather a matter of readability and
> subjective preference. No other platforms have "opps" or "opp-tables".

Ack, thanks for the explanation.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>



-- 
With best wishes
Dmitry