[PATCH 0/4] dt-bindings: msm/dp: add support for pixel clock to driver another stream

[PATCH 0/4] dt-bindings: msm/dp: add support for pixel clock to driver another stream

[Abhinav Kumar]

On some MSM chipsets, the display port controller is capable of supporting
two streams. To drive the second stream, the pixel clock for the corresponding
stream needs to be enabled. In order to add the bindings for the pixel clock
for the second stream, fixup the documentation of some of the bindings to
clarify exactly which stream they correspond to, then add the new bindings.

In addition, to help out with reviews for dp-controller bindings, add myself
as the maintainter.

This change was made on top of [1] which fixes a warning on the sa8775p
bindings.

[1]: https://patchwork.freedesktop.org/patch/624068/

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
Abhinav Kumar (4):
      dt-bindings: display: msm: dp-controller: document pixel clock stream
      dt-bindings: display: msm: dp-controller: document clock parents better
      dt-bindings: display/msm: add stream 1 pixel clock binding
      dt-bindings: display: msm: dp: update maintainer entry

 .../bindings/display/msm/dp-controller.yaml        | 41 +++++++++++++++++++---
 .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 +++--
 2 files changed, 43 insertions(+), 7 deletions(-)
---
base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
change-id: 20241202-dp_mst_bindings-7536ffc9ae2f

Best regards,
-- 
Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 1/4] dt-bindings: display: msm: dp-controller: document pixel clock stream

[Abhinav Kumar]

Display port controller on some MSM chipsets are capable of supporting
multiple streams. In order to distinguish the streams better, describe
the current pixel clock better to emphasize that it drives the stream 0.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index a212f335d5ff..35ae2630c2b3 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -55,7 +55,7 @@ properties:
       - description: Display Port AUX clock
       - description: Display Port Link clock
       - description: Link interface clock between DP and PHY
-      - description: Display Port Pixel clock
+      - description: Display Port stream 0 Pixel clock
 
   clock-names:
     items:
@@ -68,7 +68,7 @@ properties:
   assigned-clocks:
     items:
       - description: link clock source
-      - description: pixel clock source
+      - description: stream 0 pixel clock source
 
   assigned-clock-parents:
     items:

-- 
2.34.1

[PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Abhinav Kumar]

Document the assigned-clock-parents better for the DP controller node
to indicate its functionality better.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 35ae2630c2b3..9fe2bf0484d8 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -72,8 +72,8 @@ properties:
 
   assigned-clock-parents:
     items:
-      - description: phy 0 parent
-      - description: phy 1 parent
+      - description: Link clock PLL output provided by PHY block
+      - description: Stream 0 pixel clock PLL output provided by PHY block
 
   phys:
     maxItems: 1

-- 
2.34.1

[PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Abhinav Kumar]

On some chipsets the display port controller can support more
than one pixel stream (multi-stream transport). To support MST
on such chipsets, add the binding for stream 1 pixel clock for
display port controller. Since this mode is not supported on all
chipsets, add exception rules and min/max items to clearly mark
which chipsets support only SST mode (single stream) and which ones
support MST.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
 .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 9fe2bf0484d8..650d19e58277 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -50,30 +50,38 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 5
     items:
       - description: AHB clock to enable register access
       - description: Display Port AUX clock
       - description: Display Port Link clock
       - description: Link interface clock between DP and PHY
       - description: Display Port stream 0 Pixel clock
+      - description: Display Port stream 1 Pixel clock
 
   clock-names:
+    minItems: 5
     items:
       - const: core_iface
       - const: core_aux
       - const: ctrl_link
       - const: ctrl_link_iface
       - const: stream_pixel
+      - const: stream_1_pixel
 
   assigned-clocks:
+    minItems: 2
     items:
       - description: link clock source
       - description: stream 0 pixel clock source
+      - description: stream 1 pixel clock source
 
   assigned-clock-parents:
+    minItems: 2
     items:
       - description: Link clock PLL output provided by PHY block
       - description: Stream 0 pixel clock PLL output provided by PHY block
+      - description: Stream 1 pixel clock PLL output provided by PHY block
 
   phys:
     maxItems: 1
@@ -175,6 +183,30 @@ allOf:
       required:
         - "#sound-dai-cells"
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sa8775p-dp
+
+    then:
+      properties:
+        clocks:
+          maxItems: 6
+        clock-names:
+          items:
+            - const: core_iface
+            - const: core_aux
+            - const: ctrl_link
+            - const: ctrl_link_iface
+            - const: stream_pixel
+            - const: stream_1_pixel
+        assigned-clocks:
+          maxItems: 3
+        assigned-clock-parents:
+          maxItems: 3
+
 additionalProperties: false
 
 examples:
diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
index 58f8a01f29c7..7f10e6ad8f63 100644
--- a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
+++ b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
@@ -177,16 +177,19 @@ examples:
                      <&dispcc_dptx0_aux_clk>,
                      <&dispcc_dptx0_link_clk>,
                      <&dispcc_dptx0_link_intf_clk>,
-                     <&dispcc_dptx0_pixel0_clk>;
+                     <&dispcc_dptx0_pixel0_clk>,
+                     <&dispcc_dptx0_pixel1_clk>;
             clock-names = "core_iface",
                           "core_aux",
                           "ctrl_link",
                           "ctrl_link_iface",
-                          "stream_pixel";
+                          "stream_pixel",
+                          "stream_1_pixel";
 
             assigned-clocks = <&dispcc_mdss_dptx0_link_clk_src>,
+                              <&dispcc_mdss_dptx0_pixel1_clk_src>,
                               <&dispcc_mdss_dptx0_pixel0_clk_src>;
-            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>;
+            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>, <&mdss0_edp_phy 1>;
 
             phys = <&mdss0_edp_phy>;
             phy-names = "dp";

-- 
2.34.1

[PATCH 4/4] dt-bindings: display: msm: dp: update maintainer entry

[Abhinav Kumar]

Add myself as maintainer for dp controller yaml as to support
review of the incoming changes.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 650d19e58277..9867eb5133ab 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -8,6 +8,7 @@ title: MSM Display Port Controller
 
 maintainers:
   - Kuogee Hsieh <quic_khsieh@quicinc.com>
+  - Abhinav Kumar <quic_abhinavk@quicinc.com>
 
 description: |
   Device tree bindings for DisplayPort host controller for MSM targets

-- 
2.34.1

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Krzysztof Kozlowski]

On 03/12/2024 04:31, Abhinav Kumar wrote:
> Document the assigned-clock-parents better for the DP controller node
> to indicate its functionality better.


You change the clocks entirely, not "document". I would say that's an
ABI break if it really is a Linux requirement. You could avoid any
problems by just dropping the property from binding.

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 35ae2630c2b3..9fe2bf0484d8 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -72,8 +72,8 @@ properties:
>  
>    assigned-clock-parents:
>      items:
> -      - description: phy 0 parent
> -      - description: phy 1 parent
> +      - description: Link clock PLL output provided by PHY block
> +      - description: Stream 0 pixel clock PLL output provided by PHY block


Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: display: msm: dp-controller: document pixel clock stream

[Krzysztof Kozlowski]

On 03/12/2024 04:31, Abhinav Kumar wrote:
> Display port controller on some MSM chipsets are capable of supporting
> multiple streams. In order to distinguish the streams better, describe
> the current pixel clock better to emphasize that it drives the stream 0.
> 
This should be squashed with patch adding stream 1.

Best regards,
Krzysztof

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Krzysztof Kozlowski]

On 03/12/2024 04:31, Abhinav Kumar wrote:
> On some chipsets the display port controller can support more

Which chipsets?

> than one pixel stream (multi-stream transport). To support MST
> on such chipsets, add the binding for stream 1 pixel clock for
> display port controller. Since this mode is not supported on all
> chipsets, add exception rules and min/max items to clearly mark
> which chipsets support only SST mode (single stream) and which ones
> support MST.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>  .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 9fe2bf0484d8..650d19e58277 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -50,30 +50,38 @@ properties:
>      maxItems: 1
>  
>    clocks:
> +    minItems: 5
>      items:
>        - description: AHB clock to enable register access
>        - description: Display Port AUX clock
>        - description: Display Port Link clock
>        - description: Link interface clock between DP and PHY
>        - description: Display Port stream 0 Pixel clock
> +      - description: Display Port stream 1 Pixel clock
>  
>    clock-names:
> +    minItems: 5
>      items:
>        - const: core_iface
>        - const: core_aux
>        - const: ctrl_link
>        - const: ctrl_link_iface
>        - const: stream_pixel
> +      - const: stream_1_pixel
>  
>    assigned-clocks:
> +    minItems: 2
>      items:
>        - description: link clock source
>        - description: stream 0 pixel clock source
> +      - description: stream 1 pixel clock source
>  
>    assigned-clock-parents:
> +    minItems: 2
>      items:
>        - description: Link clock PLL output provided by PHY block
>        - description: Stream 0 pixel clock PLL output provided by PHY block
> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>  
>    phys:
>      maxItems: 1
> @@ -175,6 +183,30 @@ allOf:
>        required:
>          - "#sound-dai-cells"
>  

Missing if: narrowing this to 5 items for other devices.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sa8775p-dp
> +
> +    then:
> +      properties:
> +        clocks:

Missing minItems, otherwise it is pointless.

> +          maxItems: 6
> +        clock-names:
> +          items:
> +            - const: core_iface
> +            - const: core_aux
> +            - const: ctrl_link
> +            - const: ctrl_link_iface
> +            - const: stream_pixel
> +            - const: stream_1_pixel
> +        assigned-clocks:
> +          maxItems: 3

Missing minItems... or just drop, it's not accurate or not even correct.
I can assign 4 clocks, why not? Or rather: why do you stop users from
assigning 4 clocks?


> +        assigned-clock-parents:
> +          maxItems: 3
> +
>  additionalProperties: false



Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Dmitry Baryshkov]

On Tue, Dec 03, 2024 at 09:01:31AM +0100, Krzysztof Kozlowski wrote:
> On 03/12/2024 04:31, Abhinav Kumar wrote:
> > Document the assigned-clock-parents better for the DP controller node
> > to indicate its functionality better.
> 
> 
> You change the clocks entirely, not "document". I would say that's an
> ABI break if it really is a Linux requirement. You could avoid any
> problems by just dropping the property from binding.

But if you take a look at the existing usage, the proposed change
matches the behaviour. So, I'd say, it's really a change that makes
documentation follow the actual hardware.

> 
> > 
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > ---
> >  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > index 35ae2630c2b3..9fe2bf0484d8 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > @@ -72,8 +72,8 @@ properties:
> >  
> >    assigned-clock-parents:
> >      items:
> > -      - description: phy 0 parent
> > -      - description: phy 1 parent
> > +      - description: Link clock PLL output provided by PHY block
> > +      - description: Stream 0 pixel clock PLL output provided by PHY block
> 
> 
> Best regards,
> Krzysztof

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Dmitry Baryshkov]

On Mon, Dec 02, 2024 at 07:31:41PM -0800, Abhinav Kumar wrote:
> On some chipsets the display port controller can support more
> than one pixel stream (multi-stream transport). To support MST
> on such chipsets, add the binding for stream 1 pixel clock for
> display port controller. Since this mode is not supported on all
> chipsets, add exception rules and min/max items to clearly mark
> which chipsets support only SST mode (single stream) and which ones
> support MST.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>  .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 9fe2bf0484d8..650d19e58277 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -50,30 +50,38 @@ properties:
>      maxItems: 1
>  
>    clocks:
> +    minItems: 5
>      items:
>        - description: AHB clock to enable register access
>        - description: Display Port AUX clock
>        - description: Display Port Link clock
>        - description: Link interface clock between DP and PHY
>        - description: Display Port stream 0 Pixel clock
> +      - description: Display Port stream 1 Pixel clock
>  
>    clock-names:
> +    minItems: 5
>      items:
>        - const: core_iface
>        - const: core_aux
>        - const: ctrl_link
>        - const: ctrl_link_iface
>        - const: stream_pixel
> +      - const: stream_1_pixel
>  
>    assigned-clocks:
> +    minItems: 2
>      items:
>        - description: link clock source
>        - description: stream 0 pixel clock source
> +      - description: stream 1 pixel clock source
>  
>    assigned-clock-parents:
> +    minItems: 2
>      items:
>        - description: Link clock PLL output provided by PHY block
>        - description: Stream 0 pixel clock PLL output provided by PHY block
> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>  
>    phys:
>      maxItems: 1
> @@ -175,6 +183,30 @@ allOf:
>        required:
>          - "#sound-dai-cells"
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sa8775p-dp

Why do you need an extra platform conditional?

> +
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 6
> +        clock-names:
> +          items:
> +            - const: core_iface
> +            - const: core_aux
> +            - const: ctrl_link
> +            - const: ctrl_link_iface
> +            - const: stream_pixel
> +            - const: stream_1_pixel
> +        assigned-clocks:
> +          maxItems: 3
> +        assigned-clock-parents:
> +          maxItems: 3
> +
>  additionalProperties: false
>  
>  examples:
> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
> index 58f8a01f29c7..7f10e6ad8f63 100644
> --- a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
> @@ -177,16 +177,19 @@ examples:
>                       <&dispcc_dptx0_aux_clk>,
>                       <&dispcc_dptx0_link_clk>,
>                       <&dispcc_dptx0_link_intf_clk>,
> -                     <&dispcc_dptx0_pixel0_clk>;
> +                     <&dispcc_dptx0_pixel0_clk>,
> +                     <&dispcc_dptx0_pixel1_clk>;
>              clock-names = "core_iface",
>                            "core_aux",
>                            "ctrl_link",
>                            "ctrl_link_iface",
> -                          "stream_pixel";
> +                          "stream_pixel",
> +                          "stream_1_pixel";
>  
>              assigned-clocks = <&dispcc_mdss_dptx0_link_clk_src>,
> +                              <&dispcc_mdss_dptx0_pixel1_clk_src>,
>                                <&dispcc_mdss_dptx0_pixel0_clk_src>;
> -            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>;
> +            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>, <&mdss0_edp_phy 1>;
>  
>              phys = <&mdss0_edp_phy>;
>              phy-names = "dp";
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Krzysztof Kozlowski]

On Tue, Dec 03, 2024 at 03:41:48PM +0200, Dmitry Baryshkov wrote:
> On Tue, Dec 03, 2024 at 09:01:31AM +0100, Krzysztof Kozlowski wrote:
> > On 03/12/2024 04:31, Abhinav Kumar wrote:
> > > Document the assigned-clock-parents better for the DP controller node
> > > to indicate its functionality better.
> > 
> > 
> > You change the clocks entirely, not "document". I would say that's an
> > ABI break if it really is a Linux requirement. You could avoid any
> > problems by just dropping the property from binding.
> 
> But if you take a look at the existing usage, the proposed change
> matches the behaviour. So, I'd say, it's really a change that makes
> documentation follow the actual hardware.

First, this should be in the commit msg, instead of "document better to
indicate functionality better".

Second, what is the point of documenting it in the first place if you
can change it and changing has no impact? So maybe just drop?

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Dmitry Baryshkov]

On Wed, Dec 04, 2024 at 09:02:18AM +0100, Krzysztof Kozlowski wrote:
> On Tue, Dec 03, 2024 at 03:41:48PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Dec 03, 2024 at 09:01:31AM +0100, Krzysztof Kozlowski wrote:
> > > On 03/12/2024 04:31, Abhinav Kumar wrote:
> > > > Document the assigned-clock-parents better for the DP controller node
> > > > to indicate its functionality better.
> > > 
> > > 
> > > You change the clocks entirely, not "document". I would say that's an
> > > ABI break if it really is a Linux requirement. You could avoid any
> > > problems by just dropping the property from binding.
> > 
> > But if you take a look at the existing usage, the proposed change
> > matches the behaviour. So, I'd say, it's really a change that makes
> > documentation follow the actual hardware.
> 
> First, this should be in the commit msg, instead of "document better to
> indicate functionality better".
> 
> Second, what is the point of documenting it in the first place if you
> can change it and changing has no impact? So maybe just drop?

So, do you suggest setting both of the property descriptions to true? Or
dropping them completely and using unevaluatedProperties instead of
additionalProperties?

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Krzysztof Kozlowski]

On 04/12/2024 11:09, Dmitry Baryshkov wrote:
> On Wed, Dec 04, 2024 at 09:02:18AM +0100, Krzysztof Kozlowski wrote:
>> On Tue, Dec 03, 2024 at 03:41:48PM +0200, Dmitry Baryshkov wrote:
>>> On Tue, Dec 03, 2024 at 09:01:31AM +0100, Krzysztof Kozlowski wrote:
>>>> On 03/12/2024 04:31, Abhinav Kumar wrote:
>>>>> Document the assigned-clock-parents better for the DP controller node
>>>>> to indicate its functionality better.
>>>>
>>>>
>>>> You change the clocks entirely, not "document". I would say that's an
>>>> ABI break if it really is a Linux requirement. You could avoid any
>>>> problems by just dropping the property from binding.
>>>
>>> But if you take a look at the existing usage, the proposed change
>>> matches the behaviour. So, I'd say, it's really a change that makes
>>> documentation follow the actual hardware.
>>
>> First, this should be in the commit msg, instead of "document better to
>> indicate functionality better".
>>
>> Second, what is the point of documenting it in the first place if you
>> can change it and changing has no impact? So maybe just drop?
> 
> So, do you suggest setting both of the property descriptions to true? Or
> dropping them completely and using unevaluatedProperties instead of
> additionalProperties?
> 

Dropping them entirely, without any changes of additionalProperties.
Unless this property was added due to limitation of dtschema?


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Dmitry Baryshkov]

On Thu, 5 Dec 2024 at 09:33, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 04/12/2024 11:09, Dmitry Baryshkov wrote:
> > On Wed, Dec 04, 2024 at 09:02:18AM +0100, Krzysztof Kozlowski wrote:
> >> On Tue, Dec 03, 2024 at 03:41:48PM +0200, Dmitry Baryshkov wrote:
> >>> On Tue, Dec 03, 2024 at 09:01:31AM +0100, Krzysztof Kozlowski wrote:
> >>>> On 03/12/2024 04:31, Abhinav Kumar wrote:
> >>>>> Document the assigned-clock-parents better for the DP controller node
> >>>>> to indicate its functionality better.
> >>>>
> >>>>
> >>>> You change the clocks entirely, not "document". I would say that's an
> >>>> ABI break if it really is a Linux requirement. You could avoid any
> >>>> problems by just dropping the property from binding.
> >>>
> >>> But if you take a look at the existing usage, the proposed change
> >>> matches the behaviour. So, I'd say, it's really a change that makes
> >>> documentation follow the actual hardware.
> >>
> >> First, this should be in the commit msg, instead of "document better to
> >> indicate functionality better".
> >>
> >> Second, what is the point of documenting it in the first place if you
> >> can change it and changing has no impact? So maybe just drop?
> >
> > So, do you suggest setting both of the property descriptions to true? Or
> > dropping them completely and using unevaluatedProperties instead of
> > additionalProperties?
> >
>
> Dropping them entirely, without any changes of additionalProperties.
> Unless this property was added due to limitation of dtschema?

I  don't remember at this point. I think it's worth trying to drop them.

-- 
With best wishes
Dmitry

Re: [PATCH 4/4] dt-bindings: display: msm: dp: update maintainer entry

[Krzysztof Kozlowski]

On Mon, Dec 02, 2024 at 07:31:42PM -0800, Abhinav Kumar wrote:
> Add myself as maintainer for dp controller yaml as to support
> review of the incoming changes.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 +
>  1 file changed, 1 insertion(+)

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

Best regards,
Krzysztof

Re: [PATCH 0/4] dt-bindings: msm/dp: add support for pixel clock to driver another stream

[Marijn Suijten]

On 2024-12-02 19:31:38, Abhinav Kumar wrote:
> On some MSM chipsets, the display port controller is capable of supporting
> two streams. To drive the second stream, the pixel clock for the corresponding
> stream needs to be enabled. In order to add the bindings for the pixel clock
> for the second stream, fixup the documentation of some of the bindings to
> clarify exactly which stream they correspond to, then add the new bindings.
> 
> In addition, to help out with reviews for dp-controller bindings, add myself
> as the maintainter.
> 
> This change was made on top of [1] which fixes a warning on the sa8775p
> bindings.
> 
> [1]: https://patchwork.freedesktop.org/patch/624068/
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> Abhinav Kumar (4):
>       dt-bindings: display: msm: dp-controller: document pixel clock stream
>       dt-bindings: display: msm: dp-controller: document clock parents better
>       dt-bindings: display/msm: add stream 1 pixel clock binding
>       dt-bindings: display: msm: dp: update maintainer entry

Simple nit: any reason why the third patch uses display/msm as subject prefix
while the other 3 patches separate with a colon and space?

- Marijn

>  .../bindings/display/msm/dp-controller.yaml        | 41 +++++++++++++++++++---
>  .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 +++--
>  2 files changed, 43 insertions(+), 7 deletions(-)
> ---
> base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
> change-id: 20241202-dp_mst_bindings-7536ffc9ae2f
> 
> Best regards,
> -- 
> Abhinav Kumar <quic_abhinavk@quicinc.com>
> 

Re: [PATCH 0/4] dt-bindings: msm/dp: add support for pixel clock to driver another stream

[Dmitry Baryshkov]

On Mon, 02 Dec 2024 19:31:38 -0800, Abhinav Kumar wrote:
> On some MSM chipsets, the display port controller is capable of supporting
> two streams. To drive the second stream, the pixel clock for the corresponding
> stream needs to be enabled. In order to add the bindings for the pixel clock
> for the second stream, fixup the documentation of some of the bindings to
> clarify exactly which stream they correspond to, then add the new bindings.
> 
> In addition, to help out with reviews for dp-controller bindings, add myself
> as the maintainter.
> 
> [...]

Applied, thanks!

[4/4] dt-bindings: display: msm: dp: update maintainer entry
      https://gitlab.freedesktop.org/lumag/msm/-/commit/c36c60d1f742

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH 1/4] dt-bindings: display: msm: dp-controller: document pixel clock stream

[Abhinav Kumar]

Hi Krzysztof

Sorry for the delayed response.

On 12/3/2024 12:01 AM, Krzysztof Kozlowski wrote:
> On 03/12/2024 04:31, Abhinav Kumar wrote:
>> Display port controller on some MSM chipsets are capable of supporting
>> multiple streams. In order to distinguish the streams better, describe
>> the current pixel clock better to emphasize that it drives the stream 0.
>>
> This should be squashed with patch adding stream 1.
> 

Sure, I can squash this with patch 3.

> Best regards,
> Krzysztof

Re: [PATCH 2/4] dt-bindings: display: msm: dp-controller: document clock parents better

[Abhinav Kumar]

Hi Krzysztof and Dmitry

Sorry for the late response.


On 12/5/2024 3:32 AM, Dmitry Baryshkov wrote:
> On Thu, 5 Dec 2024 at 09:33, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 04/12/2024 11:09, Dmitry Baryshkov wrote:
>>> On Wed, Dec 04, 2024 at 09:02:18AM +0100, Krzysztof Kozlowski wrote:
>>>> On Tue, Dec 03, 2024 at 03:41:48PM +0200, Dmitry Baryshkov wrote:
>>>>> On Tue, Dec 03, 2024 at 09:01:31AM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 03/12/2024 04:31, Abhinav Kumar wrote:
>>>>>>> Document the assigned-clock-parents better for the DP controller node
>>>>>>> to indicate its functionality better.
>>>>>>
>>>>>>
>>>>>> You change the clocks entirely, not "document". I would say that's an
>>>>>> ABI break if it really is a Linux requirement. You could avoid any
>>>>>> problems by just dropping the property from binding.
>>>>>
>>>>> But if you take a look at the existing usage, the proposed change
>>>>> matches the behaviour. So, I'd say, it's really a change that makes
>>>>> documentation follow the actual hardware.
>>>>

Yes, I was trying to document it better and that way adding/extending 
more streams becomes easier for the next patches in the series.

>>>> First, this should be in the commit msg, instead of "document better to
>>>> indicate functionality better".
>>>>
>>>> Second, what is the point of documenting it in the first place if you
>>>> can change it and changing has no impact? So maybe just drop?
>>>
>>> So, do you suggest setting both of the property descriptions to true? Or
>>> dropping them completely and using unevaluatedProperties instead of
>>> additionalProperties?
>>>
>>
>> Dropping them entirely, without any changes of additionalProperties.
>> Unless this property was added due to limitation of dtschema?
> 
> I  don't remember at this point. I think it's worth trying to drop them.
> 

I tried dropping assigned-clock-parents altogether and it seems fine, No 
schema/errors of warnings if I also fixup the examples in 
qcom,sc7180-mdss.yaml and qcom,sa8775p-mdss.yaml to drop the usage.

So I can go ahead with dropping it.

Thanks

Abhinav

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Abhinav Kumar]

Hi Krzysztof

On 12/3/2024 12:04 AM, Krzysztof Kozlowski wrote:
> On 03/12/2024 04:31, Abhinav Kumar wrote:
>> On some chipsets the display port controller can support more
> 
> Which chipsets?
> 

 From the current list of chipsets which support DP, the following can 
support more than one stream.

qcom,sa8775p-dp
qcom,sc7280-dp
qcom,sc8180x-dp
qcom,sc8280xp-dp
qcom,sm8350-dp
qcom,sm8650-dp
qcom,sm8550-dp
qcom,sm8450-dp
qcom,sm8250-dp
qcom,sm8150-dp

So do you also want all of these to be added in the same if block as
qcom,sa8775p-dp?

>> than one pixel stream (multi-stream transport). To support MST
>> on such chipsets, add the binding for stream 1 pixel clock for
>> display port controller. Since this mode is not supported on all
>> chipsets, add exception rules and min/max items to clearly mark
>> which chipsets support only SST mode (single stream) and which ones
>> support MST.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>>   .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>>   2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index 9fe2bf0484d8..650d19e58277 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -50,30 +50,38 @@ properties:
>>       maxItems: 1
>>   
>>     clocks:
>> +    minItems: 5
>>       items:
>>         - description: AHB clock to enable register access
>>         - description: Display Port AUX clock
>>         - description: Display Port Link clock
>>         - description: Link interface clock between DP and PHY
>>         - description: Display Port stream 0 Pixel clock
>> +      - description: Display Port stream 1 Pixel clock
>>   
>>     clock-names:
>> +    minItems: 5
>>       items:
>>         - const: core_iface
>>         - const: core_aux
>>         - const: ctrl_link
>>         - const: ctrl_link_iface
>>         - const: stream_pixel
>> +      - const: stream_1_pixel
>>   
>>     assigned-clocks:
>> +    minItems: 2
>>       items:
>>         - description: link clock source
>>         - description: stream 0 pixel clock source
>> +      - description: stream 1 pixel clock source
>>   
>>     assigned-clock-parents:
>> +    minItems: 2
>>       items:
>>         - description: Link clock PLL output provided by PHY block
>>         - description: Stream 0 pixel clock PLL output provided by PHY block
>> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>>   
>>     phys:
>>       maxItems: 1
>> @@ -175,6 +183,30 @@ allOf:
>>         required:
>>           - "#sound-dai-cells"
>>   
> 
> Missing if: narrowing this to 5 items for other devices.
> 

OR would an else be better?

+    else:
+      properties:
+        clocks:
+          maxItems: 5
+        clock-names:
+          items:
+            - const: core_iface
+            - const: core_aux
+            - const: ctrl_link
+            - const: ctrl_link_iface
+            - const: stream_pixel

>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,sa8775p-dp
>> +
>> +    then:
>> +      properties:
>> +        clocks:
> 
> Missing minItems, otherwise it is pointless.
> 

I thought that since I have already specified the minItems as 5
in the clocks in the section above, I need to specify only the maxItems 
here?

    clocks:
+    minItems: 5
      items:
        - description: AHB clock to enable register access
        - description: Display Port AUX clock
        - description: Display Port Link clock
        - description: Link interface clock between DP and PHY
        - description: Display Port stream 0 Pixel clock
+      - description: Display Port stream 1 Pixel clock


>> +          maxItems: 6
>> +        clock-names:
>> +          items:
>> +            - const: core_iface
>> +            - const: core_aux
>> +            - const: ctrl_link
>> +            - const: ctrl_link_iface
>> +            - const: stream_pixel
>> +            - const: stream_1_pixel
>> +        assigned-clocks:
>> +          maxItems: 3
> 
> Missing minItems... or just drop, it's not accurate or not even correct.
> I can assign 4 clocks, why not? Or rather: why do you stop users from
> assigning 4 clocks?
> 

Sure, I can drop this.

> 
>> +        assigned-clock-parents:
>> +          maxItems: 3
>> +
>>   additionalProperties: false
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Abhinav Kumar]

On 12/3/2024 5:43 AM, Dmitry Baryshkov wrote:
> On Mon, Dec 02, 2024 at 07:31:41PM -0800, Abhinav Kumar wrote:
>> On some chipsets the display port controller can support more
>> than one pixel stream (multi-stream transport). To support MST
>> on such chipsets, add the binding for stream 1 pixel clock for
>> display port controller. Since this mode is not supported on all
>> chipsets, add exception rules and min/max items to clearly mark
>> which chipsets support only SST mode (single stream) and which ones
>> support MST.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>>   .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>>   2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index 9fe2bf0484d8..650d19e58277 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -50,30 +50,38 @@ properties:
>>       maxItems: 1
>>   
>>     clocks:
>> +    minItems: 5
>>       items:
>>         - description: AHB clock to enable register access
>>         - description: Display Port AUX clock
>>         - description: Display Port Link clock
>>         - description: Link interface clock between DP and PHY
>>         - description: Display Port stream 0 Pixel clock
>> +      - description: Display Port stream 1 Pixel clock
>>   
>>     clock-names:
>> +    minItems: 5
>>       items:
>>         - const: core_iface
>>         - const: core_aux
>>         - const: ctrl_link
>>         - const: ctrl_link_iface
>>         - const: stream_pixel
>> +      - const: stream_1_pixel
>>   
>>     assigned-clocks:
>> +    minItems: 2
>>       items:
>>         - description: link clock source
>>         - description: stream 0 pixel clock source
>> +      - description: stream 1 pixel clock source
>>   
>>     assigned-clock-parents:
>> +    minItems: 2
>>       items:
>>         - description: Link clock PLL output provided by PHY block
>>         - description: Stream 0 pixel clock PLL output provided by PHY block
>> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>>   
>>     phys:
>>       maxItems: 1
>> @@ -175,6 +183,30 @@ allOf:
>>         required:
>>           - "#sound-dai-cells"
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,sa8775p-dp
> 
> Why do you need an extra platform conditional?
> 

I expect this list to grow and also there can be chipsets which support 
4 streams as well, so an extra platform conditional was needed.

>> +
>> +    then:
>> +      properties:
>> +        clocks:
>> +          maxItems: 6
>> +        clock-names:
>> +          items:
>> +            - const: core_iface
>> +            - const: core_aux
>> +            - const: ctrl_link
>> +            - const: ctrl_link_iface
>> +            - const: stream_pixel
>> +            - const: stream_1_pixel
>> +        assigned-clocks:
>> +          maxItems: 3
>> +        assigned-clock-parents:
>> +          maxItems: 3
>> +
>>   additionalProperties: false
>>   
>>   examples:
>> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
>> index 58f8a01f29c7..7f10e6ad8f63 100644
>> --- a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
>> @@ -177,16 +177,19 @@ examples:
>>                        <&dispcc_dptx0_aux_clk>,
>>                        <&dispcc_dptx0_link_clk>,
>>                        <&dispcc_dptx0_link_intf_clk>,
>> -                     <&dispcc_dptx0_pixel0_clk>;
>> +                     <&dispcc_dptx0_pixel0_clk>,
>> +                     <&dispcc_dptx0_pixel1_clk>;
>>               clock-names = "core_iface",
>>                             "core_aux",
>>                             "ctrl_link",
>>                             "ctrl_link_iface",
>> -                          "stream_pixel";
>> +                          "stream_pixel",
>> +                          "stream_1_pixel";
>>   
>>               assigned-clocks = <&dispcc_mdss_dptx0_link_clk_src>,
>> +                              <&dispcc_mdss_dptx0_pixel1_clk_src>,
>>                                 <&dispcc_mdss_dptx0_pixel0_clk_src>;
>> -            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>;
>> +            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>, <&mdss0_edp_phy 1>;
>>   
>>               phys = <&mdss0_edp_phy>;
>>               phy-names = "dp";
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Dmitry Baryshkov]

On Tue, Apr 22, 2025 at 07:46:57PM -0700, Abhinav Kumar wrote:
> 
> 
> On 12/3/2024 5:43 AM, Dmitry Baryshkov wrote:
> > On Mon, Dec 02, 2024 at 07:31:41PM -0800, Abhinav Kumar wrote:
> > > On some chipsets the display port controller can support more
> > > than one pixel stream (multi-stream transport). To support MST
> > > on such chipsets, add the binding for stream 1 pixel clock for
> > > display port controller. Since this mode is not supported on all
> > > chipsets, add exception rules and min/max items to clearly mark
> > > which chipsets support only SST mode (single stream) and which ones
> > > support MST.
> > > 
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > ---
> > >   .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
> > >   .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
> > >   2 files changed, 38 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > > index 9fe2bf0484d8..650d19e58277 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > > @@ -50,30 +50,38 @@ properties:
> > >       maxItems: 1
> > >     clocks:
> > > +    minItems: 5
> > >       items:
> > >         - description: AHB clock to enable register access
> > >         - description: Display Port AUX clock
> > >         - description: Display Port Link clock
> > >         - description: Link interface clock between DP and PHY
> > >         - description: Display Port stream 0 Pixel clock
> > > +      - description: Display Port stream 1 Pixel clock
> > >     clock-names:
> > > +    minItems: 5
> > >       items:
> > >         - const: core_iface
> > >         - const: core_aux
> > >         - const: ctrl_link
> > >         - const: ctrl_link_iface
> > >         - const: stream_pixel
> > > +      - const: stream_1_pixel
> > >     assigned-clocks:
> > > +    minItems: 2
> > >       items:
> > >         - description: link clock source
> > >         - description: stream 0 pixel clock source
> > > +      - description: stream 1 pixel clock source
> > >     assigned-clock-parents:
> > > +    minItems: 2
> > >       items:
> > >         - description: Link clock PLL output provided by PHY block
> > >         - description: Stream 0 pixel clock PLL output provided by PHY block
> > > +      - description: Stream 1 pixel clock PLL output provided by PHY block
> > >     phys:
> > >       maxItems: 1
> > > @@ -175,6 +183,30 @@ allOf:
> > >         required:
> > >           - "#sound-dai-cells"
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - qcom,sa8775p-dp
> > 
> > Why do you need an extra platform conditional?
> > 
> 
> I expect this list to grow and also there can be chipsets which support 4
> streams as well, so an extra platform conditional was needed.

Ack

> 
> > > +
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          maxItems: 6
> > > +        clock-names:
> > > +          items:
> > > +            - const: core_iface
> > > +            - const: core_aux
> > > +            - const: ctrl_link
> > > +            - const: ctrl_link_iface
> > > +            - const: stream_pixel
> > > +            - const: stream_1_pixel

You don't need to ducplicate the list. Just specify min/maxItems.

> > > +        assigned-clocks:
> > > +          maxItems: 3
> > > +        assigned-clock-parents:
> > > +          maxItems: 3
> > > +
> > >   additionalProperties: false
> > >   examples:
> > > diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
> > > index 58f8a01f29c7..7f10e6ad8f63 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
> > > +++ b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
> > > @@ -177,16 +177,19 @@ examples:
> > >                        <&dispcc_dptx0_aux_clk>,
> > >                        <&dispcc_dptx0_link_clk>,
> > >                        <&dispcc_dptx0_link_intf_clk>,
> > > -                     <&dispcc_dptx0_pixel0_clk>;
> > > +                     <&dispcc_dptx0_pixel0_clk>,
> > > +                     <&dispcc_dptx0_pixel1_clk>;
> > >               clock-names = "core_iface",
> > >                             "core_aux",
> > >                             "ctrl_link",
> > >                             "ctrl_link_iface",
> > > -                          "stream_pixel";
> > > +                          "stream_pixel",
> > > +                          "stream_1_pixel";
> > >               assigned-clocks = <&dispcc_mdss_dptx0_link_clk_src>,
> > > +                              <&dispcc_mdss_dptx0_pixel1_clk_src>,
> > >                                 <&dispcc_mdss_dptx0_pixel0_clk_src>;
> > > -            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>;
> > > +            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>, <&mdss0_edp_phy 1>;
> > >               phys = <&mdss0_edp_phy>;
> > >               phy-names = "dp";
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Krzysztof Kozlowski]

On 23/04/2025 04:46, Abhinav Kumar wrote:
> Hi Krzysztof
> 
> On 12/3/2024 12:04 AM, Krzysztof Kozlowski wrote:
>> On 03/12/2024 04:31, Abhinav Kumar wrote:
>>> On some chipsets the display port controller can support more
>>
>> Which chipsets?
>>
> 
>  From the current list of chipsets which support DP, the following can 
> support more than one stream.
> 
> qcom,sa8775p-dp
> qcom,sc7280-dp
> qcom,sc8180x-dp
> qcom,sc8280xp-dp
> qcom,sm8350-dp
> qcom,sm8650-dp
> qcom,sm8550-dp
> qcom,sm8450-dp
> qcom,sm8250-dp
> qcom,sm8150-dp
> 
> So do you also want all of these to be added in the same if block as
> qcom,sa8775p-dp?

That was talk in 2024. Entire context is gone if you reply after three
months. I do not have even that emails in my inbox anymore.

Probably I expected commit msg to mention at least some, so everyone
knows which chipsets are affected here and one can verify the statements
from commit msg.

> 
>>> than one pixel stream (multi-stream transport). To support MST
>>> on such chipsets, add the binding for stream 1 pixel clock for
>>> display port controller. Since this mode is not supported on all
>>> chipsets, add exception rules and min/max items to clearly mark
>>> which chipsets support only SST mode (single stream) and which ones
>>> support MST.
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>>>   .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>>>   2 files changed, 38 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> index 9fe2bf0484d8..650d19e58277 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>> @@ -50,30 +50,38 @@ properties:
>>>       maxItems: 1
>>>   
>>>     clocks:
>>> +    minItems: 5
>>>       items:
>>>         - description: AHB clock to enable register access
>>>         - description: Display Port AUX clock
>>>         - description: Display Port Link clock
>>>         - description: Link interface clock between DP and PHY
>>>         - description: Display Port stream 0 Pixel clock
>>> +      - description: Display Port stream 1 Pixel clock
>>>   
>>>     clock-names:
>>> +    minItems: 5
>>>       items:
>>>         - const: core_iface
>>>         - const: core_aux
>>>         - const: ctrl_link
>>>         - const: ctrl_link_iface
>>>         - const: stream_pixel
>>> +      - const: stream_1_pixel
>>>   
>>>     assigned-clocks:
>>> +    minItems: 2
>>>       items:
>>>         - description: link clock source
>>>         - description: stream 0 pixel clock source
>>> +      - description: stream 1 pixel clock source
>>>   
>>>     assigned-clock-parents:
>>> +    minItems: 2
>>>       items:
>>>         - description: Link clock PLL output provided by PHY block
>>>         - description: Stream 0 pixel clock PLL output provided by PHY block
>>> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>>>   
>>>     phys:
>>>       maxItems: 1
>>> @@ -175,6 +183,30 @@ allOf:
>>>         required:
>>>           - "#sound-dai-cells"
>>>   
>>
>> Missing if: narrowing this to 5 items for other devices.
>>
> 
> OR would an else be better?

Usually not, although depends how this binding is written.


> 
> +    else:
> +      properties:
> +        clocks:
> +          maxItems: 5
> +        clock-names:
> +          items:
> +            - const: core_iface
> +            - const: core_aux
> +            - const: ctrl_link
> +            - const: ctrl_link_iface
> +            - const: stream_pixel
> 
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,sa8775p-dp
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>
>> Missing minItems, otherwise it is pointless.
>>
> 
> I thought that since I have already specified the minItems as 5
> in the clocks in the section above, I need to specify only the maxItems 
> here?

No, you need explicit constraints here.



Best regards,
Krzysztof

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Abhinav Kumar]

On 4/23/2025 7:23 AM, Dmitry Baryshkov wrote:
> On Tue, Apr 22, 2025 at 07:46:57PM -0700, Abhinav Kumar wrote:
>>
>>
>> On 12/3/2024 5:43 AM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 02, 2024 at 07:31:41PM -0800, Abhinav Kumar wrote:
>>>> On some chipsets the display port controller can support more
>>>> than one pixel stream (multi-stream transport). To support MST
>>>> on such chipsets, add the binding for stream 1 pixel clock for
>>>> display port controller. Since this mode is not supported on all
>>>> chipsets, add exception rules and min/max items to clearly mark
>>>> which chipsets support only SST mode (single stream) and which ones
>>>> support MST.
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>>>>    .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>>>>    2 files changed, 38 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> index 9fe2bf0484d8..650d19e58277 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> @@ -50,30 +50,38 @@ properties:
>>>>        maxItems: 1
>>>>      clocks:
>>>> +    minItems: 5
>>>>        items:
>>>>          - description: AHB clock to enable register access
>>>>          - description: Display Port AUX clock
>>>>          - description: Display Port Link clock
>>>>          - description: Link interface clock between DP and PHY
>>>>          - description: Display Port stream 0 Pixel clock
>>>> +      - description: Display Port stream 1 Pixel clock
>>>>      clock-names:
>>>> +    minItems: 5
>>>>        items:
>>>>          - const: core_iface
>>>>          - const: core_aux
>>>>          - const: ctrl_link
>>>>          - const: ctrl_link_iface
>>>>          - const: stream_pixel
>>>> +      - const: stream_1_pixel
>>>>      assigned-clocks:
>>>> +    minItems: 2
>>>>        items:
>>>>          - description: link clock source
>>>>          - description: stream 0 pixel clock source
>>>> +      - description: stream 1 pixel clock source
>>>>      assigned-clock-parents:
>>>> +    minItems: 2
>>>>        items:
>>>>          - description: Link clock PLL output provided by PHY block
>>>>          - description: Stream 0 pixel clock PLL output provided by PHY block
>>>> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>>>>      phys:
>>>>        maxItems: 1
>>>> @@ -175,6 +183,30 @@ allOf:
>>>>          required:
>>>>            - "#sound-dai-cells"
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - qcom,sa8775p-dp
>>>
>>> Why do you need an extra platform conditional?
>>>
>>
>> I expect this list to grow and also there can be chipsets which support 4
>> streams as well, so an extra platform conditional was needed.
> 
> Ack
> 
>>
>>>> +
>>>> +    then:
>>>> +      properties:
>>>> +        clocks:
>>>> +          maxItems: 6
>>>> +        clock-names:
>>>> +          items:
>>>> +            - const: core_iface
>>>> +            - const: core_aux
>>>> +            - const: ctrl_link
>>>> +            - const: ctrl_link_iface
>>>> +            - const: stream_pixel
>>>> +            - const: stream_1_pixel
> 
> You don't need to ducplicate the list. Just specify min/maxItems.
> 

Ack

>>>> +        assigned-clocks:
>>>> +          maxItems: 3
>>>> +        assigned-clock-parents:
>>>> +          maxItems: 3
>>>> +
>>>>    additionalProperties: false
>>>>    examples:
>>>> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
>>>> index 58f8a01f29c7..7f10e6ad8f63 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sa8775p-mdss.yaml
>>>> @@ -177,16 +177,19 @@ examples:
>>>>                         <&dispcc_dptx0_aux_clk>,
>>>>                         <&dispcc_dptx0_link_clk>,
>>>>                         <&dispcc_dptx0_link_intf_clk>,
>>>> -                     <&dispcc_dptx0_pixel0_clk>;
>>>> +                     <&dispcc_dptx0_pixel0_clk>,
>>>> +                     <&dispcc_dptx0_pixel1_clk>;
>>>>                clock-names = "core_iface",
>>>>                              "core_aux",
>>>>                              "ctrl_link",
>>>>                              "ctrl_link_iface",
>>>> -                          "stream_pixel";
>>>> +                          "stream_pixel",
>>>> +                          "stream_1_pixel";
>>>>                assigned-clocks = <&dispcc_mdss_dptx0_link_clk_src>,
>>>> +                              <&dispcc_mdss_dptx0_pixel1_clk_src>,
>>>>                                  <&dispcc_mdss_dptx0_pixel0_clk_src>;
>>>> -            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>;
>>>> +            assigned-clock-parents = <&mdss0_edp_phy 0>, <&mdss0_edp_phy 1>, <&mdss0_edp_phy 1>;
>>>>                phys = <&mdss0_edp_phy>;
>>>>                phy-names = "dp";
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>
> 

Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Abhinav Kumar]

On 5/7/2025 11:18 PM, Krzysztof Kozlowski wrote:
> On 23/04/2025 04:46, Abhinav Kumar wrote:
>> Hi Krzysztof
>>
>> On 12/3/2024 12:04 AM, Krzysztof Kozlowski wrote:
>>> On 03/12/2024 04:31, Abhinav Kumar wrote:
>>>> On some chipsets the display port controller can support more
>>>
>>> Which chipsets?
>>>
>>
>>   From the current list of chipsets which support DP, the following can
>> support more than one stream.
>>
>> qcom,sa8775p-dp
>> qcom,sc7280-dp
>> qcom,sc8180x-dp
>> qcom,sc8280xp-dp
>> qcom,sm8350-dp
>> qcom,sm8650-dp
>> qcom,sm8550-dp
>> qcom,sm8450-dp
>> qcom,sm8250-dp
>> qcom,sm8150-dp
>>
>> So do you also want all of these to be added in the same if block as
>> qcom,sa8775p-dp?
> 
> That was talk in 2024. Entire context is gone if you reply after three
> months. I do not have even that emails in my inbox anymore.
> 
> Probably I expected commit msg to mention at least some, so everyone
> knows which chipsets are affected here and one can verify the statements
> from commit msg.
> 

Sure will do.

>>
>>>> than one pixel stream (multi-stream transport). To support MST
>>>> on such chipsets, add the binding for stream 1 pixel clock for
>>>> display port controller. Since this mode is not supported on all
>>>> chipsets, add exception rules and min/max items to clearly mark
>>>> which chipsets support only SST mode (single stream) and which ones
>>>> support MST.
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>>>>    .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>>>>    2 files changed, 38 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> index 9fe2bf0484d8..650d19e58277 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> @@ -50,30 +50,38 @@ properties:
>>>>        maxItems: 1
>>>>    
>>>>      clocks:
>>>> +    minItems: 5
>>>>        items:
>>>>          - description: AHB clock to enable register access
>>>>          - description: Display Port AUX clock
>>>>          - description: Display Port Link clock
>>>>          - description: Link interface clock between DP and PHY
>>>>          - description: Display Port stream 0 Pixel clock
>>>> +      - description: Display Port stream 1 Pixel clock
>>>>    
>>>>      clock-names:
>>>> +    minItems: 5
>>>>        items:
>>>>          - const: core_iface
>>>>          - const: core_aux
>>>>          - const: ctrl_link
>>>>          - const: ctrl_link_iface
>>>>          - const: stream_pixel
>>>> +      - const: stream_1_pixel
>>>>    
>>>>      assigned-clocks:
>>>> +    minItems: 2
>>>>        items:
>>>>          - description: link clock source
>>>>          - description: stream 0 pixel clock source
>>>> +      - description: stream 1 pixel clock source
>>>>    
>>>>      assigned-clock-parents:
>>>> +    minItems: 2
>>>>        items:
>>>>          - description: Link clock PLL output provided by PHY block
>>>>          - description: Stream 0 pixel clock PLL output provided by PHY block
>>>> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>>>>    
>>>>      phys:
>>>>        maxItems: 1
>>>> @@ -175,6 +183,30 @@ allOf:
>>>>          required:
>>>>            - "#sound-dai-cells"
>>>>    
>>>
>>> Missing if: narrowing this to 5 items for other devices.
>>>
>>
>> OR would an else be better?
> 
> Usually not, although depends how this binding is written.
> 

Ok, let me try it.

> 
>>
>> +    else:
>> +      properties:
>> +        clocks:
>> +          maxItems: 5
>> +        clock-names:
>> +          items:
>> +            - const: core_iface
>> +            - const: core_aux
>> +            - const: ctrl_link
>> +            - const: ctrl_link_iface
>> +            - const: stream_pixel
>>
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - qcom,sa8775p-dp
>>>> +
>>>> +    then:
>>>> +      properties:
>>>> +        clocks:
>>>
>>> Missing minItems, otherwise it is pointless.
>>>
>>
>> I thought that since I have already specified the minItems as 5
>> in the clocks in the section above, I need to specify only the maxItems
>> here?
> 
> No, you need explicit constraints here.
> 

Ack

> 
> 
> Best regards,
> Krzysztof