Re: [PATCH v3 3/4] dt-bindings: display: ti,am65x-dss: Add OLDI properties for AM625 DSS

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

Hi Krzysztof,


On 7/21/24 21:09, Krzysztof Kozlowski wrote:
> On 16/07/2024 10:42, Aradhya Bhatia wrote:
>> The DSS in AM625 SoC has 2 OLDI TXes. Refer the OLDI schema to add the
>> support for the OLDI TXes.
>>
>> The AM625 DSS VP1 (port@0) can connect and control 2 OLDI TXes, to use
>> them in dual-link or cloned single-link OLDI modes. Add support for an
>> additional endpoint under the port@0 to accurately depict the data flow
>> path.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  .../bindings/display/ti/ti,am65x-dss.yaml     | 135 ++++++++++++++++++
>>  1 file changed, 135 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 399d68986326..249597455d34 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -91,6 +91,24 @@ properties:
>>            For AM625 DSS, the internal DPI output port node from video
>>            port 1.
>>            For AM62A7 DSS, the port is tied off inside the SoC.
>> +        properties:
>> +          endpoint@0:
>> +            $ref: /schemas/graph.yaml#/properties/endpoint
>> +            description:
>> +              For AM625 DSS, VP Connection to OLDI0.
>> +              For AM65X DSS, OLDI output from the SoC.
>> +
>> +          endpoint@1:
>> +            $ref: /schemas/graph.yaml#/properties/endpoint
>> +            description:
>> +              For AM625 DSS, VP Connection to OLDI1.
> 
> Eh, that's confusing. Why do you have graph to your children? Isn't this
> entirely pointless?

I am not sure I fully understand. The same display source video port can
connect up to 2 OLDI TXes - hence 2 endpoints which connect to the OLDI
that were described in the previous patch. The idea has been to
accurately depict the connections of the hardware.

What am I missing here?


side-note: I do realize, as I write this, that it has been quite a while
since you reviewed, and that you may have, rightfully, lost context.
I apologize for that.

> 
>> +
>> +        anyOf:
>> +          - required:
>> +              - endpoint
>> +          - required:
>> +              - endpoint@0
>> +              - endpoint@1
>>  
>>        port@1:
>>          $ref: /schemas/graph.yaml#/properties/port
>> @@ -112,6 +130,23 @@ properties:
>>        Input memory (from main memory to dispc) bandwidth limit in
>>        bytes per second
>>  
>> +  oldi-txes:
>> +    type: object
>> +    additionalProperties: true
> 
> Why? This looks wrong.

This, I will admit, was a shot in the dark. The binding check asked me
that I was missing either this or unevaluatedProperties. I tried to make
sense of the two, but with little luck. Eventually, I went with this.

I could change it to unevaluatedProperties if that is indeed correct. I
could also use some comprehensive resource to understand this, if you
have something to recommend. =)

> 
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      '^oldi_tx@[0-1]$':
> 
> Please follow DTS coding style for naming.

Okay!

> 
>> +        type: object
>> +        $ref: ti,am625-oldi.yaml#
>> +        unevaluatedProperties: false
>> +        description: OLDI transmitters connected to the DSS VPs
>> +
>>  allOf:
>>    - if:
>>        properties:
>> @@ -123,6 +158,19 @@ allOf:
>>          ports:
>>            properties:
>>              port@0: false
>> +            oldi_txes: false
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: ti,am65x-dss
>> +    then:
>> +      properties:
>> +        oldi_txes: false
>> +        port@0:
>> +          properties:
>> +            endpoint@1: false
>>  
>>  required:
>>    - compatible
>> @@ -171,3 +219,90 @@ examples:
>>              };
>>          };
>>      };
>> +
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
>> +
>> +    bus {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        dss1: dss@30200000 {
>> +            compatible = "ti,am625-dss";
>> +            reg = <0x00 0x30200000 0x00 0x1000>, /* common */
>> +                  <0x00 0x30202000 0x00 0x1000>, /* vidl1 */
>> +                  <0x00 0x30206000 0x00 0x1000>, /* vid */
>> +                  <0x00 0x30207000 0x00 0x1000>, /* ovr1 */
>> +                  <0x00 0x30208000 0x00 0x1000>, /* ovr2 */
>> +                  <0x00 0x3020a000 0x00 0x1000>, /* vp1 */
>> +                  <0x00 0x3020b000 0x00 0x1000>, /* vp2 */
>> +                  <0x00 0x30201000 0x00 0x1000>; /* common1 */
>> +            reg-names = "common", "vidl1", "vid",
>> +                        "ovr1", "ovr2", "vp1", "vp2", "common1";
>> +            power-domains = <&k3_pds 186 TI_SCI_PD_EXCLUSIVE>;
>> +            clocks =        <&k3_clks 186 6>,
>> +                            <&vp1_clock>,
>> +                            <&k3_clks 186 2>;
>> +            clock-names = "fck", "vp1", "vp2";
>> +            interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
>> +            oldi-txes {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +                oldi0: oldi@0 {
> 
> You are duplicating the example from previous schema. No need. Keep
> only, one complete example.

Sure!


Regards
Aradhya