[v1] dt-bindings: msm:disp: update dsi and dpu bindings

[v1] dt-bindings: msm:disp: update dsi and dpu bindings

[Harigovindan P]

Updating bindings of dsi and dpu by adding and removing certain
properties.

Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
---

Changes in v1:
        - Adding "ahb" clock as a required property.
        - Adding "bus", "rot", "lut" as optional properties for sc7180 device.
        - Removing properties from dsi bindings that are unused.
	- Removing power-domain property since DSI is the child node of MDSS
	  and it will inherit supply from its parent.

 Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++
 Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
index 551ae26..dd58472a 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
@@ -19,6 +19,7 @@ Required properties:
   The following clocks are required:
   * "iface"
   * "bus"
+  * "ahb"
   * "core"
 - interrupts: interrupt signal from MDSS.
 - interrupt-controller: identifies the node as an interrupt controller.
@@ -50,6 +51,8 @@ Required properties:
 - clock-names: device clock names, must be in same order as clocks property.
   The following clocks are required.
   * "bus"
+  For the device "qcom,sc7180-dpu":
+  * "bus" - is an optional property due to architecture change.
   * "iface"
   * "core"
   * "vsync"
@@ -70,6 +73,10 @@ Optional properties:
 - assigned-clocks: list of clock specifiers for clocks needing rate assignment
 - assigned-clock-rates: list of clock frequencies sorted in the same order as
   the assigned-clocks property.
+- For the device "qcom,sc7180-dpu":
+  clock-names: optional device clocks, needed for accessing LUT blocks.
+  * "rot"
+  * "lut"
 
 Example:
 
diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index af95586..61d659a 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -8,13 +8,10 @@ Required properties:
 - reg-names: The names of register regions. The following regions are required:
   * "dsi_ctrl"
 - interrupts: The interrupt signal from the DSI block.
-- power-domains: Should be <&mmcc MDSS_GDSC>.
 - clocks: Phandles to device clocks.
 - clock-names: the following clocks are required:
-  * "mdp_core"
   * "iface"
   * "bus"
-  * "core_mmss"
   * "byte"
   * "pixel"
   * "core"
@@ -156,7 +153,6 @@ Example:
 			"core",
 			"core_mmss",
 			"iface",
-			"mdp_core",
 			"pixel";
 		clocks =
 			<&mmcc MDSS_AXI_CLK>,
@@ -164,7 +160,6 @@ Example:
 			<&mmcc MDSS_ESC0_CLK>,
 			<&mmcc MMSS_MISC_AHB_CLK>,
 			<&mmcc MDSS_AHB_CLK>,
-			<&mmcc MDSS_MDP_CLK>,
 			<&mmcc MDSS_PCLK0_CLK>;
 
 		assigned-clocks =
-- 
2.7.4

Re: [Freedreno] [v1] dt-bindings: msm:disp: update dsi and dpu bindings

[Jeffrey Hugo]

On Tue, Feb 4, 2020 at 7:15 AM Harigovindan P <harigovi@codeaurora.org> wrote:
>
> Updating bindings of dsi and dpu by adding and removing certain
> properties.
>
> Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
> ---
>
> Changes in v1:
>         - Adding "ahb" clock as a required property.
>         - Adding "bus", "rot", "lut" as optional properties for sc7180 device.
>         - Removing properties from dsi bindings that are unused.
>         - Removing power-domain property since DSI is the child node of MDSS
>           and it will inherit supply from its parent.
>
>  Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index af95586..61d659a 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -8,13 +8,10 @@ Required properties:
>  - reg-names: The names of register regions. The following regions are required:
>    * "dsi_ctrl"
>  - interrupts: The interrupt signal from the DSI block.
> -- power-domains: Should be <&mmcc MDSS_GDSC>.
>  - clocks: Phandles to device clocks.
>  - clock-names: the following clocks are required:
> -  * "mdp_core"
>    * "iface"
>    * "bus"
> -  * "core_mmss"

Why do you think these are unused?  I see them used in the driver, and
as far as I can tell these get routed to the hardware, therefore they
should be described in DT.

>    * "byte"
>    * "pixel"
>    * "core"
> @@ -156,7 +153,6 @@ Example:
>                         "core",
>                         "core_mmss",
>                         "iface",
> -                       "mdp_core",
>                         "pixel";
>                 clocks =
>                         <&mmcc MDSS_AXI_CLK>,
> @@ -164,7 +160,6 @@ Example:
>                         <&mmcc MDSS_ESC0_CLK>,
>                         <&mmcc MMSS_MISC_AHB_CLK>,
>                         <&mmcc MDSS_AHB_CLK>,
> -                       <&mmcc MDSS_MDP_CLK>,
>                         <&mmcc MDSS_PCLK0_CLK>;
>
>                 assigned-clocks =
> --
> 2.7.4
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings

[Doug Anderson]

Hi,

On Tue, Feb 4, 2020 at 6:15 AM Harigovindan P <harigovi@codeaurora.org> wrote:
>
> Updating bindings of dsi and dpu by adding and removing certain
> properties.
>
> Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
> ---
>
> Changes in v1:
>         - Adding "ahb" clock as a required property.
>         - Adding "bus", "rot", "lut" as optional properties for sc7180 device.
>         - Removing properties from dsi bindings that are unused.
>         - Removing power-domain property since DSI is the child node of MDSS
>           and it will inherit supply from its parent.
>
>  Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -----
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26..dd58472a 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -19,6 +19,7 @@ Required properties:
>    The following clocks are required:
>    * "iface"
>    * "bus"
> +  * "ahb"

This is only required for sc7180?  ...or old SoCs should have had it
all along too?


>    * "core"
>  - interrupts: interrupt signal from MDSS.
>  - interrupt-controller: identifies the node as an interrupt controller.
> @@ -50,6 +51,8 @@ Required properties:
>  - clock-names: device clock names, must be in same order as clocks property.
>    The following clocks are required.
>    * "bus"
> +  For the device "qcom,sc7180-dpu":
> +  * "bus" - is an optional property due to architecture change.

This is a really odd way to write it for two reasons:
* You're breaking up the flow of the list.
* This shouldn't be listed as "optional" in sc7180 but unless there is
some reason to ever provide it on sc7180.  It should simply be
disallowed.

Maybe instead just:

   The following clocks are required.
-  * "bus"
+  * "bus" (anything other than qcom,sc7180-dpu)

We really need to get this into yaml ASAP but that'd probably be OK to
tide us over.

NOTE: when converting to yaml, ideally we'll have a separate file per
SoC to avoid crazy spaghetti, see commit 2a8aa18c1131 ("dt-bindings:
clk: qcom: Fix self-validation, split, and clean cruft") in clk-next
for an example of starting the transition to one yaml per SoC (at
least for anything majorly different).


>    * "iface"
>    * "core"
>    * "vsync"
> @@ -70,6 +73,10 @@ Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate assignment
>  - assigned-clock-rates: list of clock frequencies sorted in the same order as
>    the assigned-clocks property.
> +- For the device "qcom,sc7180-dpu":
> +  clock-names: optional device clocks, needed for accessing LUT blocks.
> +  * "rot"
> +  * "lut"
>
>  Example:
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index af95586..61d659a 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -8,13 +8,10 @@ Required properties:
>  - reg-names: The names of register regions. The following regions are required:
>    * "dsi_ctrl"
>  - interrupts: The interrupt signal from the DSI block.
> -- power-domains: Should be <&mmcc MDSS_GDSC>.

Is this supposed to be removed from all SoCs using this bindings, or
just yours?

I'll also note that you left it in the "Example:" below.


>  - clocks: Phandles to device clocks.
>  - clock-names: the following clocks are required:
> -  * "mdp_core"
>    * "iface"
>    * "bus"
> -  * "core_mmss"

As Jeffrey pointed out, you shouldn't be removing these from old SoCs.
In "drivers/gpu/drm/msm/dsi/dsi_cfg.c" you can clearly see them used.
Maybe it's time for you to do the yaml conversion and handle this
correctly per-SoC.

-Doug

Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings

[Rob Herring]

On Tue, Feb 04, 2020 at 07:45:37PM +0530, Harigovindan P wrote:
> Updating bindings of dsi and dpu by adding and removing certain
> properties.

Yes, the diff tells me that. The commit message should say why.

This change breaks compatibility as well.

> 
> Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
> ---
> 
> Changes in v1:
>         - Adding "ahb" clock as a required property.
>         - Adding "bus", "rot", "lut" as optional properties for sc7180 device.
>         - Removing properties from dsi bindings that are unused.
> 	- Removing power-domain property since DSI is the child node of MDSS
> 	  and it will inherit supply from its parent.
> 
>  Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26..dd58472a 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -19,6 +19,7 @@ Required properties:
>    The following clocks are required:
>    * "iface"
>    * "bus"
> +  * "ahb"

You can't just add new clocks...

>    * "core"
>  - interrupts: interrupt signal from MDSS.
>  - interrupt-controller: identifies the node as an interrupt controller.
> @@ -50,6 +51,8 @@ Required properties:
>  - clock-names: device clock names, must be in same order as clocks property.
>    The following clocks are required.
>    * "bus"
> +  For the device "qcom,sc7180-dpu":
> +  * "bus" - is an optional property due to architecture change.
>    * "iface"
>    * "core"
>    * "vsync"
> @@ -70,6 +73,10 @@ Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate assignment
>  - assigned-clock-rates: list of clock frequencies sorted in the same order as
>    the assigned-clocks property.
> +- For the device "qcom,sc7180-dpu":
> +  clock-names: optional device clocks, needed for accessing LUT blocks.
> +  * "rot"
> +  * "lut"
>  
>  Example:
>  
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index af95586..61d659a 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -8,13 +8,10 @@ Required properties:
>  - reg-names: The names of register regions. The following regions are required:
>    * "dsi_ctrl"
>  - interrupts: The interrupt signal from the DSI block.
> -- power-domains: Should be <&mmcc MDSS_GDSC>.
>  - clocks: Phandles to device clocks.
>  - clock-names: the following clocks are required:
> -  * "mdp_core"
>    * "iface"
>    * "bus"
> -  * "core_mmss"
>    * "byte"
>    * "pixel"
>    * "core"
> @@ -156,7 +153,6 @@ Example:
>  			"core",
>  			"core_mmss",
>  			"iface",
> -			"mdp_core",
>  			"pixel";
>  		clocks =
>  			<&mmcc MDSS_AXI_CLK>,
> @@ -164,7 +160,6 @@ Example:
>  			<&mmcc MDSS_ESC0_CLK>,
>  			<&mmcc MMSS_MISC_AHB_CLK>,
>  			<&mmcc MDSS_AHB_CLK>,
> -			<&mmcc MDSS_MDP_CLK>,
>  			<&mmcc MDSS_PCLK0_CLK>;
>  
>  		assigned-clocks =
> -- 
> 2.7.4
> 

Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings

[harigovi]

On 2020-02-04 23:52, Doug Anderson wrote:
> Hi,
> 
> On Tue, Feb 4, 2020 at 6:15 AM Harigovindan P <harigovi@codeaurora.org> 
> wrote:
>> 
>> Updating bindings of dsi and dpu by adding and removing certain
>> properties.
>> 
>> Signed-off-by: Harigovindan P <harigovi@codeaurora.org>
>> ---
>> 
>> Changes in v1:
>>         - Adding "ahb" clock as a required property.
>>         - Adding "bus", "rot", "lut" as optional properties for sc7180 
>> device.
>>         - Removing properties from dsi bindings that are unused.
>>         - Removing power-domain property since DSI is the child node 
>> of MDSS
>>           and it will inherit supply from its parent.
>> 
>>  Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++
>>  Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -----
>>  2 files changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt 
>> b/Documentation/devicetree/bindings/display/msm/dpu.txt
>> index 551ae26..dd58472a 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
>> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
>> @@ -19,6 +19,7 @@ Required properties:
>>    The following clocks are required:
>>    * "iface"
>>    * "bus"
>> +  * "ahb"
> 
> This is only required for sc7180?  ...or old SoCs should have had it
> all along too?
> 
> 
>>    * "core"
>>  - interrupts: interrupt signal from MDSS.
>>  - interrupt-controller: identifies the node as an interrupt 
>> controller.
>> @@ -50,6 +51,8 @@ Required properties:
>>  - clock-names: device clock names, must be in same order as clocks 
>> property.
>>    The following clocks are required.
>>    * "bus"
>> +  For the device "qcom,sc7180-dpu":
>> +  * "bus" - is an optional property due to architecture change.
> 
> This is a really odd way to write it for two reasons:
> * You're breaking up the flow of the list.
> * This shouldn't be listed as "optional" in sc7180 but unless there is
> some reason to ever provide it on sc7180.  It should simply be
> disallowed.
> 
> Maybe instead just:
> 
>    The following clocks are required.
> -  * "bus"
> +  * "bus" (anything other than qcom,sc7180-dpu)
> 
> We really need to get this into yaml ASAP but that'd probably be OK to
> tide us over.
> 
> NOTE: when converting to yaml, ideally we'll have a separate file per
> SoC to avoid crazy spaghetti, see commit 2a8aa18c1131 ("dt-bindings:
> clk: qcom: Fix self-validation, split, and clean cruft") in clk-next
> for an example of starting the transition to one yaml per SoC (at
> least for anything majorly different).
> 
> 
>>    * "iface"
>>    * "core"
>>    * "vsync"
>> @@ -70,6 +73,10 @@ Optional properties:
>>  - assigned-clocks: list of clock specifiers for clocks needing rate 
>> assignment
>>  - assigned-clock-rates: list of clock frequencies sorted in the same 
>> order as
>>    the assigned-clocks property.
>> +- For the device "qcom,sc7180-dpu":
>> +  clock-names: optional device clocks, needed for accessing LUT 
>> blocks.
>> +  * "rot"
>> +  * "lut"
>> 
>>  Example:
>> 
>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
>> b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> index af95586..61d659a 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
>> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> @@ -8,13 +8,10 @@ Required properties:
>>  - reg-names: The names of register regions. The following regions are 
>> required:
>>    * "dsi_ctrl"
>>  - interrupts: The interrupt signal from the DSI block.
>> -- power-domains: Should be <&mmcc MDSS_GDSC>.
> 
> Is this supposed to be removed from all SoCs using this bindings, or
> just yours?
> 
> I'll also note that you left it in the "Example:" below.
> 
> 
>>  - clocks: Phandles to device clocks.
>>  - clock-names: the following clocks are required:
>> -  * "mdp_core"
>>    * "iface"
>>    * "bus"
>> -  * "core_mmss"
> 
> As Jeffrey pointed out, you shouldn't be removing these from old SoCs.
> In "drivers/gpu/drm/msm/dsi/dsi_cfg.c" you can clearly see them used.
> Maybe it's time for you to do the yaml conversion and handle this
> correctly per-SoC.
> 
> -Doug


Hi,

yaml files have been created and new patchset has been created for that.
https://patchwork.kernel.org/patch/11423653/