[PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Aradhya Bhatia]

Hello all,

This patch series add support for the dual OLDI TXes supported in Texas
Instruments' AM62x and AM62Px family of SoCs. The OLDI TXes support
single-lvds, lvds-clone, and dual-lvds modes. These have now been
represented through DRM bridges within TI-DSS.

 - Some history and hardware description for this patch series.

This patch series is a complete re-vamp from the previously posted
series[1] and hence, the version index has been reset to v1. The OLDI
support from that series was dropped and only the base support for AM62x
DSS was kept (and eventually merged)[2].

The OLDI display that the tidss driver today supports, could not be
extended for the newer SoCs. The OLDI display in tidss is modelled after
the DSS and OLDI hardware in the AM65x SoC. The DSS in AM65x SoC, has
two video-ports. Both these video-ports (VP) output DPI video signals.
One of the DPI output (from VP1) from the DSS connects to a singular
OLDI TX present inside the SoC. There is no other way for the DPI from
VP1 to be taken out of the SoC. The other DPI output however - the one
from VP2 - is taken out of the SoC as is. Hence we have an OLDI bus
output and a DPI bus output from the SoC. Since the VP1 and OLDI are
tightly coupled, the tidss driver considers them as a single entity.
That is why, any OLDI sink connects directly to the DSS ports in the
OF graphs.

The newer SoCs have varying DSS and OLDI integrations.

The AM62x DSS also has 2 VPs. The 2nd VP, VP2, outputs DPI signals which
are taken out of the SoC - similar to the AM65x above. For the VP1,
there are 2 OLDI TXes. These OLDI TXes can only receive DPI signals from
VP1, and don't connect to VP2 at all.

The AM62Px SoC has 2 OLDI TXes like AM62x SoC. However, the AM62Px SoC
also has 2 separate DSSes. The 2 OLDI TXes can now be shared between the
2 VPs of the 2 DSSes.

The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need
for some major changes for a full feature experience.

1. The OF graph needs to be updated to accurately show the data flow.
2. The tidss and OLDI drivers now need to support the dual-link and the
   cloned single-link OLDI video signals.
3. The drivers also need to support the case where 2 OLDI TXes are
   connected to 2 different VPs - thereby creating 2 independent streams
   of single-link OLDI outputs.

Note that the OLDI does not have registers of its own. Its still
dependent on the parent VP. The VP that provides the DPI video signals
to the OLDI TXes, also gives the OLDI TXes all the config data. That is
to say, the hardware doesn't sit on the data bus directly - but does so
via DSS.

In light of all of these hardware variations, it was decided to have
a separate OLDI driver (unlike AM65x) but not entirely separate so as to
be a platform device. The OLDI TXes are now being represented as DRM
bridges under the tidss.

Also, since the DRM framework only really supports a linear
encoder-bridge chain, the OLDI driver creates a DRM bridge ONLY for the
primary OLDI TX in cases of dual-link or cloned single-link OLDI modes.
That bridge then attaches to the tidss's display core - which consists
of a CRTC, an Encoder (dummy) and a bridge (dummy). On the other end,
it attaches to OLDI sinks (panels or other bridges).

Since the OLDI TX have a hardware dependency with the VP, the OLDI
configuration needs to happen before that VP is enabled for streaming.
VP stream enable takes place in tidss_crtc_atomic_enable hook. I have
posted a patch allowing DRM bridges to get pre-enabled before the CRTC
of that bridge is enabled[0]. Without that patch, some warnings or
glitches can be seen.

These patches have been tested on AM625 based SK-AM625 EVM with a
Microptis dual-lvds panel (SK-LCD1). The patches with complete support
including the expected devicetree configuration of the OLDI TXes can be
found in the "next_oldi-v3-tests" branch of my github fork[3].

Thanks,
Aradhya


Change Log:
V3:
  - Fix the dt_binding_check warning in patch 3/4[4] by adding
    "additionalProperties" constraint.

V2:
  - Add all the R-b and A-b tags from Laurent Pinchart, Rob Herring, and
    Tomi Valkeinen.
  - Reword the subject for patch 1/4.
  - Reword the commit descriptions to add proper hardware detail.
  - Drop the change in schema reference for port@0 in patch 3/4.
  - Lots of improvements for patch 4/4.
    * Refactor OLDI selection logic in tidss_oldi_tx_power().
    * Add "companion_instance" support to identify the OLDI index in
      dual-link or cloned sinle-link modes.
    * De-initialize tidss_oldi during tidss removal.
    * Use dev_err_probe() instead of dev_err().
    * Drop OLDI(n) macro.
    * Move OLDI Config register bits to tidss_dispc_regs.h.
    * Drop oldi bridge atomic_check().
    * s/%d/%u for all print instances of "oldi_instance".
    * Move OLDI init after DISPC init in tidss_probe.
    * Use devm_drm_of_get_bridge() instead of
      drm_of_find_panel_or_bridge() to find the next bridge and drop all
      the drm_panel support from tidss_oldi.

Previous revisions:
V2: https://lore.kernel.org/all/20240715200953.1213284-1-a-bhatia1@ti.com/
V1: https://lore.kernel.org/all/20240511193055.1686149-1-a-bhatia1@ti.com/

[0]: Dependency Patch: 
("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
https://lore.kernel.org/all/20240622110929.3115714-11-a-bhatia1@ti.com/

[1]: AM62 OLDI Series - v7
https://lore.kernel.org/all/20230125113529.13952-1-a-bhatia1@ti.com/

[2]: AM62 DSS Series - v9
https://lore.kernel.org/all/20230616150900.6617-1-a-bhatia1@ti.com/

[3]: GitHub Fork for OLDI tests
https://github.com/aradhya07/linux-ab/tree/next_oldi-v3-tests/

[4]: ("ti,am65x-dss.yaml: oldi-txes: Missing additionalProperties/
      unevaluatedProperties constraint")
https://lore.kernel.org/all/172107979988.1595945.9666141982402158422.robh@kernel.org/

Aradhya Bhatia (4):
  dt-bindings: display: ti,am65x-dss: Re-indent the example
  dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter
  dt-bindings: display: ti,am65x-dss: Add OLDI properties for AM625 DSS
  drm/tidss: Add OLDI bridge support

 .../bindings/display/ti/ti,am625-oldi.yaml    | 153 +++++
 .../bindings/display/ti/ti,am65x-dss.yaml     | 177 +++++-
 MAINTAINERS                                   |   1 +
 drivers/gpu/drm/tidss/Makefile                |   3 +-
 drivers/gpu/drm/tidss/tidss_dispc.c           |  20 +-
 drivers/gpu/drm/tidss/tidss_dispc.h           |   4 +
 drivers/gpu/drm/tidss/tidss_dispc_regs.h      |  14 +
 drivers/gpu/drm/tidss/tidss_drv.c             |   9 +
 drivers/gpu/drm/tidss/tidss_drv.h             |   5 +
 drivers/gpu/drm/tidss/tidss_oldi.c            | 537 ++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_oldi.h            |  51 ++
 11 files changed, 951 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
 create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c
 create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h


base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
-- 
2.34.1

[PATCH v3 1/4] dt-bindings: display: ti,am65x-dss: Re-indent the example

[Aradhya Bhatia]

Reduce tab size from 8 spaces to 4 spaces to make the bindings
consistent, and easy to expand.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 54 +++++++++----------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 55e3e490d0e6..399d68986326 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -142,32 +142,32 @@ examples:
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
 
     dss: dss@4a00000 {
-            compatible = "ti,am65x-dss";
-            reg =   <0x04a00000 0x1000>, /* common */
-                    <0x04a02000 0x1000>, /* vidl1 */
-                    <0x04a06000 0x1000>, /* vid */
-                    <0x04a07000 0x1000>, /* ovr1 */
-                    <0x04a08000 0x1000>, /* ovr2 */
-                    <0x04a0a000 0x1000>, /* vp1 */
-                    <0x04a0b000 0x1000>, /* vp2 */
-                    <0x04a01000 0x1000>; /* common1 */
-            reg-names = "common", "vidl1", "vid",
-                    "ovr1", "ovr2", "vp1", "vp2", "common1";
-            ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
-            power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
-            clocks =        <&k3_clks 67 1>,
-                            <&k3_clks 216 1>,
-                            <&k3_clks 67 2>;
-            clock-names = "fck", "vp1", "vp2";
-            interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
-            ports {
-                    #address-cells = <1>;
-                    #size-cells = <0>;
-                    port@0 {
-                            reg = <0>;
-                            oldi_out0: endpoint {
-                                    remote-endpoint = <&lcd_in0>;
-                            };
-                    };
+        compatible = "ti,am65x-dss";
+        reg = <0x04a00000 0x1000>, /* common */
+              <0x04a02000 0x1000>, /* vidl1 */
+              <0x04a06000 0x1000>, /* vid */
+              <0x04a07000 0x1000>, /* ovr1 */
+              <0x04a08000 0x1000>, /* ovr2 */
+              <0x04a0a000 0x1000>, /* vp1 */
+              <0x04a0b000 0x1000>, /* vp2 */
+              <0x04a01000 0x1000>; /* common1 */
+        reg-names = "common", "vidl1", "vid",
+                "ovr1", "ovr2", "vp1", "vp2", "common1";
+        ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
+        power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
+        clocks =        <&k3_clks 67 1>,
+                        <&k3_clks 216 1>,
+                        <&k3_clks 67 2>;
+        clock-names = "fck", "vp1", "vp2";
+        interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            port@0 {
+                reg = <0>;
+                oldi_out0: endpoint {
+                    remote-endpoint = <&lcd_in0>;
+                };
             };
+        };
     };
-- 
2.34.1

[PATCH v3 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter

[Aradhya Bhatia]

The OLDI (transmitters) TXes do not have registers of their own, and are
dependent on the source video-ports from the DSS to provide
configuration data. This hardware doesn't directly sit on the internal
bus of the SoC, but does so via the DSS. Hence, the OLDI TXes are
supposed to be child nodes under the DSS, and not independent devices.

Two of the OLDI TXes can function in tandem to output dual-link OLDI
output, or cloned single-link outputs. In these cases, one OLDI will be
the primary OLDI, and the other one, a companion.

The OLDI functionality is further supported by a system-control module,
which contains a few registers to control OLDI IO power and
characteristics.

Add devicetree binding schema for AM625 OLDI TXes.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../bindings/display/ti/ti,am625-oldi.yaml    | 153 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 154 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
new file mode 100644
index 000000000000..0a96e600bc0b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
@@ -0,0 +1,153 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ti/ti,am625-oldi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments AM625 OLDI Transmitter
+
+maintainers:
+  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
+  - Aradhya Bhatia <a-bhatia1@ti.com>
+
+description: |
+  The AM625 TI Keystone OpenLDI transmitter (OLDI TX) supports serialized RGB
+  pixel data transmission between host and flat panel display over LVDS (Low
+  Voltage Differential Sampling) interface. The OLDI TX consists of 7-to-1 data
+  serializers, and 4-data and 1-clock LVDS outputs. It supports the LVDS output
+  formats "jeida-18", "jeida-24" and "vesa-18", and can accept 24-bit RGB or
+  padded and un-padded 18-bit RGB bus formats as input.
+
+properties:
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: serial clock input for the OLDI transmitters
+
+  clock-names:
+    const: s_clk
+
+  ti,companion-oldi:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to companion OLDI transmitter. This property is mandatory for the
+      primarty OLDI TX if the OLDI TXes are expected to work either in dual-lvds
+      mode or in clone mode. This property should point to the secondary OLDI
+      TX.
+
+  ti,secondary-oldi:
+    type: boolean
+    description: Boolean property to mark an OLDI TX as secondary node.
+
+  ti,oldi-io-ctrl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to syscon device node mapping OLDI IO_CTRL registers found in the
+      control MMR region. This property is needed for OLDI interface to work.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Parallel RGB input port
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: LVDS output port
+
+    required:
+      - port@0
+      - port@1
+
+allOf:
+  - if:
+      properties:
+        ti,secondary-oldi: true
+    then:
+      properties:
+        ti,companion-oldi: false
+        ti,oldi-io-ctrl: false
+        clocks: false
+        clock-names: false
+
+    else:
+      required:
+        - ti,oldi-io-ctrl
+        - clocks
+        - clock-names
+
+required:
+  - reg
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    oldi_txes {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        oldi: oldi@0 {
+            reg = <0>;
+            clocks = <&k3_clks 186 0>;
+            clock-names = "s_clk";
+            ti,oldi-io-ctrl = <&dss_oldi_io_ctrl>;
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    oldi_in: endpoint {
+                        remote-endpoint = <&dpi0_out>;
+                    };
+                };
+            };
+        };
+    };
+
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    oldi_txes {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        oldi0: oldi@0 {
+            reg = <0>;
+            clocks = <&k3_clks 186 0>;
+            clock-names = "s_clk";
+            ti,companion-oldi = <&oldi1>;
+            ti,oldi-io-ctrl = <&dss_oldi_io_ctrl>;
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    oldi0_in: endpoint {
+                        remote-endpoint = <&dpi0_out0>;
+                    };
+                };
+            };
+        };
+        oldi1: oldi@1 {
+            reg = <1>;
+            ti,secondary-oldi;
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    oldi1_in: endpoint {
+                        remote-endpoint = <&dpi0_out1>;
+                    };
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index fb1df8c29f5a..26b1dd3367c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7642,6 +7642,7 @@ M:	Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
 L:	dri-devel@lists.freedesktop.org
 S:	Maintained
 T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
+F:	Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
 F:	Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
 F:	Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
 F:	Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
-- 
2.34.1

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

[Aradhya Bhatia]

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.
+
+        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
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      '^oldi_tx@[0-1]$':
+        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 {
+                    reg = <0>;
+                    clocks = <&k3_clks 186 0>;
+                    clock-names = "s_clk";
+                    ti,companion-oldi = <&oldi1>;
+                    ti,oldi-io-ctrl = <&dss_oldi_io_ctrl>;
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            oldi0_in: endpoint {
+                                remote-endpoint = <&dpi0_out0>;
+                            };
+                        };
+                    };
+                };
+                oldi1: oldi@1 {
+                    reg = <1>;
+                    ti,secondary-oldi;
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            oldi1_in: endpoint {
+                                remote-endpoint = <&dpi0_out1>;
+                            };
+                        };
+                    };
+                };
+            };
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+                    reg = <0>;
+                    dpi0_out0: endpoint@0 {
+                        reg = <0>;
+                        remote-endpoint = <&oldi0_in>;
+                    };
+                    dpi0_out1: endpoint@1 {
+                        reg = <1>;
+                        remote-endpoint = <&oldi1_in>;
+                    };
+                };
+                port@1 {
+                    reg = <1>;
+                    dpi1_out: endpoint {
+                        remote-endpoint = <&hdmi_bridge>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.34.1

[PATCH v3 4/4] drm/tidss: Add OLDI bridge support

[Aradhya Bhatia]

The AM62x and AM62Px SoCs feature 2 OLDI TXes each, which makes it
possible to connect them in dual-link or cloned single-link OLDI display
modes. The current OLDI support in tidss_dispc.c can only support for
a single OLDI TX, connected to a VP and doesn't really support
configuration of OLDIs in the other modes. The current OLDI support in
tidss_dispc.c also works on the principle that the OLDI output can only
be served by one, and only one, DSS video-port. This isn't the case in
the AM62Px SoC, where there are 2 DSS controllers present that share the
OLDI TXes.

Having their own devicetree and their own bridge entity will help
support the various display modes and sharing possiblilities of the OLDI
hardware.

For all these reasons, add support for the OLDI TXes as DRM bridges.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/Makefile           |   3 +-
 drivers/gpu/drm/tidss/tidss_dispc.c      |  20 +-
 drivers/gpu/drm/tidss/tidss_dispc.h      |   4 +
 drivers/gpu/drm/tidss/tidss_dispc_regs.h |  14 +
 drivers/gpu/drm/tidss/tidss_drv.c        |   9 +
 drivers/gpu/drm/tidss/tidss_drv.h        |   5 +
 drivers/gpu/drm/tidss/tidss_oldi.c       | 537 +++++++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_oldi.h       |  51 +++
 8 files changed, 641 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c
 create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h

diff --git a/drivers/gpu/drm/tidss/Makefile b/drivers/gpu/drm/tidss/Makefile
index 312645271014..b6d6becf1683 100644
--- a/drivers/gpu/drm/tidss/Makefile
+++ b/drivers/gpu/drm/tidss/Makefile
@@ -7,6 +7,7 @@ tidss-y := tidss_crtc.o \
 	tidss_irq.o \
 	tidss_plane.o \
 	tidss_scale_coefs.o \
-	tidss_dispc.o
+	tidss_dispc.o \
+	tidss_oldi.o
 
 obj-$(CONFIG_DRM_TIDSS) += tidss.o
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 1ad711f8d2a8..8631a89e6155 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -466,6 +466,25 @@ static u32 dispc_vp_read(struct dispc_device *dispc, u32 hw_videoport, u16 reg)
 	return ioread32(base + reg);
 }
 
+void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport,
+			  u32 oldi_cfg)
+{
+	u32 count = 0;
+	u32 oldi_reset_bit = BIT(5 + hw_videoport);
+
+	dispc_vp_write(tidss->dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+
+	if (oldi_cfg != 0) {
+		while (!(oldi_reset_bit & dispc_read(tidss->dispc, DSS_SYSSTATUS)) &&
+		       count < 10000)
+			count++;
+
+		if (!(oldi_reset_bit & dispc_read(tidss->dispc, DSS_SYSSTATUS)))
+			dev_warn(tidss->dispc->dev, "%s: timeout waiting OLDI reset done\n",
+				 __func__);
+	}
+}
+
 /*
  * TRM gives bitfields as start:end, where start is the higher bit
  * number. For example 7:0
@@ -1310,7 +1329,6 @@ void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport)
  * Calculate the percentage difference between the requested pixel clock rate
  * and the effective rate resulting from calculating the clock divider value.
  */
-static
 unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
 {
 	int r = rate / 100, rr = real_rate / 100;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 086327d51a90..fab248f2055a 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -94,6 +94,10 @@ extern const struct dispc_features dispc_am62a7_feats;
 extern const struct dispc_features dispc_am65x_feats;
 extern const struct dispc_features dispc_j721e_feats;
 
+void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport,
+			  u32 oldi_cfg);
+unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate);
+
 void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
 dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
index 13feedfe5d6d..03f7098029e6 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
@@ -226,6 +226,20 @@ enum dispc_common_regs {
 #define DISPC_VP_DSS_DMA_THREADSIZE		0x170 /* J721E */
 #define DISPC_VP_DSS_DMA_THREADSIZE_STATUS	0x174 /* J721E */
 
+/* OLDI Config Bits (DISPC_VP_DSS_OLDI_CFG) */
+#define OLDI_ENABLE		BIT(0)
+#define OLDI_MAP		(BIT(1) | BIT(2) | BIT(3))
+#define OLDI_SRC		BIT(4)
+#define OLDI_CLONE_MODE		BIT(5)
+#define OLDI_MASTERSLAVE	BIT(6)
+#define OLDI_DEPOL		BIT(7)
+#define OLDI_MSB		BIT(8)
+#define OLDI_LBEN		BIT(9)
+#define OLDI_LBDATA		BIT(10)
+#define OLDI_DUALMODESYNC	BIT(11)
+#define OLDI_SOFTRST		BIT(12)
+#define OLDI_TPATCFG		BIT(13)
+
 /*
  * OLDI IO_CTRL register offsets. On AM654 the registers are found
  * from CTRL_MMR0, there the syscon regmap should map 0x14 bytes from
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index d15f836dca95..56eb81602363 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -23,6 +23,7 @@
 #include "tidss_drv.h"
 #include "tidss_kms.h"
 #include "tidss_irq.h"
+#include "tidss_oldi.h"
 
 /* Power management */
 
@@ -146,6 +147,10 @@ static int tidss_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = tidss_oldi_init(tidss);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init OLDI\n");
+
 	pm_runtime_enable(dev);
 
 	pm_runtime_set_autosuspend_delay(dev, 1000);
@@ -202,6 +207,8 @@ static int tidss_probe(struct platform_device *pdev)
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
+	tidss_oldi_deinit(tidss);
+
 	return ret;
 }
 
@@ -226,6 +233,8 @@ static void tidss_remove(struct platform_device *pdev)
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
+	tidss_oldi_deinit(tidss);
+
 	/* devm allocated dispc goes away with the dev so mark it NULL */
 	dispc_remove(tidss);
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..6c0fe1d989ee 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -11,8 +11,10 @@
 
 #define TIDSS_MAX_PORTS 4
 #define TIDSS_MAX_PLANES 4
+#define TIDSS_MAX_OLDI_TXES 2
 
 typedef u32 dispc_irq_t;
+struct tidss_oldi;
 
 struct tidss_device {
 	struct drm_device ddev;		/* DRM device for DSS */
@@ -27,6 +29,9 @@ struct tidss_device {
 	unsigned int num_planes;
 	struct drm_plane *planes[TIDSS_MAX_PLANES];
 
+	unsigned int num_oldis;
+	struct tidss_oldi *oldis[TIDSS_MAX_OLDI_TXES];
+
 	unsigned int irq;
 
 	spinlock_t wait_lock;	/* protects the irq masks */
diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
new file mode 100644
index 000000000000..7509aeae15c7
--- /dev/null
+++ b/drivers/gpu/drm/tidss/tidss_oldi.c
@@ -0,0 +1,537 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 - Texas Instruments Incorporated
+ *
+ * Aradhya Bhatia <a-bhatia1@ti.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/mfd/syscon.h>
+#include <linux/media-bus-format.h>
+#include <linux/regmap.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_of.h>
+
+#include "tidss_dispc.h"
+#include "tidss_dispc_regs.h"
+#include "tidss_oldi.h"
+
+struct tidss_oldi {
+	struct tidss_device	*tidss;
+	struct device		*dev;
+
+	struct drm_bridge	bridge;
+	struct drm_bridge	*next_bridge;
+
+	enum tidss_oldi_link_type link_type;
+	const struct oldi_bus_format *bus_format;
+	u32 oldi_instance;
+	u32 companion_instance;
+	u32 parent_vp;
+
+	struct clk *s_clk;
+	struct regmap *io_ctrl;
+};
+
+static const struct oldi_bus_format oldi_bus_formats[] = {
+	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, SPWG_18,	MEDIA_BUS_FMT_RGB666_1X18 },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, SPWG_24,	MEDIA_BUS_FMT_RGB888_1X24 },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, JEIDA_24,	MEDIA_BUS_FMT_RGB888_1X24 },
+};
+
+#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
+
+static inline struct tidss_oldi *
+drm_bridge_to_tidss_oldi(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct tidss_oldi, bridge);
+}
+
+static int tidss_oldi_bridge_attach(struct drm_bridge *bridge,
+				    enum drm_bridge_attach_flags flags)
+{
+	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
+
+	if (!oldi->next_bridge) {
+		dev_err(oldi->dev,
+			"%s: OLDI%u Failure attach next bridge\n",
+			__func__, oldi->oldi_instance);
+		return -ENODEV;
+	}
+
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		dev_err(oldi->dev,
+			"%s: OLDI%u DRM_BRIDGE_ATTACH_NO_CONNECTOR is mandatory.\n",
+			__func__, oldi->oldi_instance);
+		return -EINVAL;
+	}
+
+	return drm_bridge_attach(bridge->encoder, oldi->next_bridge,
+				 bridge, flags);
+}
+
+static int
+tidss_oldi_set_serial_clk(struct tidss_oldi *oldi, unsigned long rate)
+{
+	unsigned long new_rate;
+	int ret;
+
+	ret = clk_set_rate(oldi->s_clk, rate);
+	if (ret) {
+		dev_err(oldi->dev,
+			"OLDI%u: failed to set serial clk rate to %lu Hz\n",
+			 oldi->oldi_instance, rate);
+		return ret;
+	}
+
+	new_rate = clk_get_rate(oldi->s_clk);
+
+	if (dispc_pclk_diff(rate, new_rate) > 5)
+		dev_warn(oldi->dev,
+			 "OLDI%u Clock rate %lu differs over 5%% from requested %lu\n",
+			 oldi->oldi_instance, new_rate, rate);
+
+	dev_dbg(oldi->dev, "OLDI%u: new rate %lu Hz (requested %lu Hz)\n",
+		oldi->oldi_instance, clk_get_rate(oldi->s_clk), rate);
+
+	return 0;
+}
+
+static void tidss_oldi_tx_power(struct tidss_oldi *oldi, bool enable)
+{
+	u32 mask;
+
+	/*
+	 * The power control bits are Active Low, and remain powered off by
+	 * default. That is, the bits are set to 1. To power on the OLDI TXes,
+	 * the bits must be cleared to 0. Since there are cases where not all
+	 * OLDI TXes are being used, the power logic selectively powers them
+	 * on.
+	 * Setting the variable 'val' to particular bit masks, makes sure that
+	 * the unrequired OLDI TXes remain powered off.
+	 */
+
+	if (enable) {
+		switch (oldi->link_type) {
+		case OLDI_MODE_SINGLE_LINK:
+			/* Power-on only the required OLDI TX's IO*/
+			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) | OLDI_PWRDN_BG;
+			break;
+		case OLDI_MODE_CLONE_SINGLE_LINK:
+		case OLDI_MODE_DUAL_LINK:
+			/* Power-on both the OLDI TXes' IOs */
+			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) |
+			       OLDI_PWRDOWN_TX(oldi->companion_instance) |
+			       OLDI_PWRDN_BG;
+			break;
+		default:
+			/*
+			 * This code execution should never reach here as any
+			 * OLDI with an unsupported OLDI mode would never get
+			 * registered in the first place.
+			 * However, power-off the OLDI in concern just in case.
+			 */
+			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance);
+			enable = false;
+			break;
+		}
+	} else {
+		switch (oldi->link_type) {
+		case OLDI_MODE_CLONE_SINGLE_LINK:
+		case OLDI_MODE_DUAL_LINK:
+			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance) |
+			       OLDI_PWRDOWN_TX(oldi->companion_instance) |
+			       OLDI_PWRDN_BG;
+			break;
+		case OLDI_MODE_SINGLE_LINK:
+		default:
+			mask = OLDI_PWRDOWN_TX(oldi->oldi_instance);
+			break;
+		}
+	}
+
+	regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, enable ? 0 : mask);
+}
+
+static int tidss_oldi_config(struct tidss_oldi *oldi)
+{
+	const struct oldi_bus_format *bus_fmt = NULL;
+	u32 oldi_cfg = 0;
+
+	bus_fmt = oldi->bus_format;
+
+	/*
+	 * MASTERSLAVE and SRC bits of OLDI Config are always set to 0.
+	 */
+
+	if (bus_fmt->data_width == 24)
+		oldi_cfg |= OLDI_MSB;
+	else if (bus_fmt->data_width != 18)
+		dev_warn(oldi->dev,
+			 "OLDI%u: DSS port width %d not supported\n",
+			 oldi->oldi_instance, bus_fmt->data_width);
+
+	oldi_cfg |= OLDI_DEPOL;
+
+	oldi_cfg = (oldi_cfg & (~OLDI_MAP)) | (bus_fmt->oldi_mode_reg_val << 1);
+
+	oldi_cfg |= OLDI_SOFTRST;
+
+	oldi_cfg |= OLDI_ENABLE;
+
+	switch (oldi->link_type) {
+	case OLDI_MODE_SINGLE_LINK:
+		/* All configuration is done for this mode.  */
+		break;
+
+	case OLDI_MODE_CLONE_SINGLE_LINK:
+		oldi_cfg |= OLDI_CLONE_MODE;
+		break;
+
+	case OLDI_MODE_DUAL_LINK:
+		/* data-mapping field also indicates dual-link mode */
+		oldi_cfg |= BIT(3);
+		oldi_cfg |= OLDI_DUALMODESYNC;
+		break;
+
+	default:
+		dev_err(oldi->dev, "OLDI%u: Unsupported mode.\n",
+			oldi->oldi_instance);
+		return -EINVAL;
+	}
+
+	tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg);
+
+	return 0;
+}
+
+static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
+					 struct drm_bridge_state *old_bridge_state)
+{
+	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
+	struct drm_atomic_state *state = old_bridge_state->base.state;
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_display_mode *mode;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (WARN_ON(!crtc_state))
+		return;
+
+	mode = &crtc_state->adjusted_mode;
+
+	/* Configure the OLDI params*/
+	tidss_oldi_config(oldi);
+
+	/* Set the OLDI serial clock (7 times the pixel clock) */
+	tidss_oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
+
+	/* Enable OLDI IO power */
+	tidss_oldi_tx_power(oldi, true);
+}
+
+static void tidss_oldi_atomic_post_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
+{
+	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
+
+	/* Disable OLDI IO power */
+	tidss_oldi_tx_power(oldi, false);
+
+	/* Set the OLDI serial clock to IDLE Frequency */
+	tidss_oldi_set_serial_clk(oldi, OLDI_IDLE_CLK_HZ);
+
+	/* Clear OLDI Config */
+	tidss_configure_oldi(oldi->tidss, oldi->parent_vp, 0);
+}
+
+#define MAX_INPUT_SEL_FORMATS	1
+
+static u32 *tidss_oldi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+						 struct drm_bridge_state *bridge_state,
+						 struct drm_crtc_state *crtc_state,
+						 struct drm_connector_state *conn_state,
+						 u32 output_fmt,
+						 unsigned int *num_input_fmts)
+{
+	struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
+	u32 *input_fmts;
+	int i;
+
+	*num_input_fmts = 0;
+
+	for (i = 0; i < ARRAY_SIZE(oldi_bus_formats); i++)
+		if (oldi_bus_formats[i].bus_fmt == output_fmt)
+			break;
+
+	if (i == ARRAY_SIZE(oldi_bus_formats))
+		return NULL;
+
+	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
+			     GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	*num_input_fmts = 1;
+	input_fmts[0] = oldi_bus_formats[i].input_bus_fmt;
+	oldi->bus_format = &oldi_bus_formats[i];
+
+	return input_fmts;
+}
+
+static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
+	.attach		= tidss_oldi_bridge_attach,
+	.atomic_pre_enable = tidss_oldi_atomic_pre_enable,
+	.atomic_post_disable = tidss_oldi_atomic_post_disable,
+	.atomic_get_input_bus_fmts = tidss_oldi_atomic_get_input_bus_fmts,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+};
+
+static int get_oldi_mode(struct device_node *oldi_tx, u32 *companion_instance)
+{
+	struct device_node *companion;
+	struct device_node *port0, *port1;
+	int pixel_order;
+
+	/*
+	 * Find if the OLDI is paired with another OLDI for combined OLDI
+	 * operation (dual-lvds or clone).
+	 */
+	companion = of_parse_phandle(oldi_tx, "ti,companion-oldi", 0);
+	if (!companion) {
+		/*
+		 * OLDI TXes in Single Link mode do not have companion
+		 * OLDI TXes and, Secondary OLDI nodes don't need this
+		 * information.
+		 */
+		*companion_instance = -1;
+
+		if (of_property_read_bool(oldi_tx, "ti,secondary-oldi"))
+			return OLDI_MODE_SECONDARY;
+
+		/*
+		 * The OLDI TX does not have a companion, nor is it a
+		 * secondary OLDI. It will operate independently.
+		 */
+		return OLDI_MODE_SINGLE_LINK;
+	}
+
+	if (of_property_read_u32(companion, "reg", companion_instance))
+		return OLDI_MODE_UNSUPPORTED;
+
+	/*
+	 * We need to work out if the sink is expecting us to function in
+	 * dual-link mode. We do this by looking at the DT port nodes we are
+	 * connected to, if they are marked as expecting even pixels and
+	 * odd pixels than we need to enable vertical stripe output.
+	 */
+	port0 = of_graph_get_port_by_id(oldi_tx, 1);
+	port1 = of_graph_get_port_by_id(companion, 1);
+	pixel_order = drm_of_lvds_get_dual_link_pixel_order(port0, port1);
+	of_node_put(port0);
+	of_node_put(port1);
+	of_node_put(companion);
+
+	switch (pixel_order) {
+	case -EINVAL:
+		/*
+		 * The dual link properties were not found in at least
+		 * one of the sink nodes. Since 2 OLDI ports are present
+		 * in the DT, it can be safely assumed that the required
+		 * configuration is Clone Mode.
+		 */
+		return OLDI_MODE_CLONE_SINGLE_LINK;
+
+	case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
+		return OLDI_MODE_DUAL_LINK;
+
+	/* Unsupported OLDI Modes */
+	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
+	default:
+		return OLDI_MODE_UNSUPPORTED;
+	}
+}
+
+static int get_parent_dss_vp(struct device_node *oldi_tx, u32 *parent_vp)
+{
+	struct device_node *ep, *dss_port;
+	int ret;
+
+	ep = of_graph_get_endpoint_by_regs(oldi_tx, OLDI_INPUT_PORT, -1);
+	if (ep) {
+		dss_port = of_graph_get_remote_port(ep);
+		if (!dss_port) {
+			ret = -ENODEV;
+			goto err_return_ep_port;
+		}
+
+		ret = of_property_read_u32(dss_port, "reg", parent_vp);
+
+		of_node_put(dss_port);
+err_return_ep_port:
+		of_node_put(ep);
+		return ret;
+	}
+
+	return -ENODEV;
+}
+
+static const struct drm_bridge_timings default_tidss_oldi_timings = {
+	.input_bus_flags = DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
+			 | DRM_BUS_FLAG_DE_HIGH,
+};
+
+void tidss_oldi_deinit(struct tidss_device *tidss)
+{
+	for (int i = 0; i < tidss->num_oldis; i++) {
+		if (tidss->oldis[i]) {
+			drm_bridge_remove(&tidss->oldis[i]->bridge);
+			tidss->oldis[i] = NULL;
+		}
+	}
+}
+
+int tidss_oldi_init(struct tidss_device *tidss)
+{
+	struct tidss_oldi *oldi;
+	struct device_node *child;
+	struct drm_bridge *bridge;
+	u32 parent_vp, oldi_instance, companion_instance;
+	enum tidss_oldi_link_type link_type = OLDI_MODE_UNSUPPORTED;
+	struct device_node *oldi_parent;
+	int ret = 0;
+
+	tidss->num_oldis = 0;
+
+	oldi_parent = of_get_child_by_name(tidss->dev->of_node, "oldi-txes");
+	if (!oldi_parent)
+		/* Return gracefully */
+		return 0;
+
+	for_each_child_of_node(oldi_parent, child) {
+		ret = get_parent_dss_vp(child, &parent_vp);
+		if (ret) {
+			if (ret == -ENODEV) {
+				/*
+				 * ENODEV means that this particular OLDI node
+				 * is not connected with the DSS, which is not
+				 * a harmful case. There could be another OLDI
+				 * which may still be connected.
+				 * Continue to search for that.
+				 */
+				ret = 0;
+				continue;
+			}
+			goto err_put_node;
+		}
+
+		ret = of_property_read_u32(child, "reg", &oldi_instance);
+		if (ret)
+			goto err_put_node;
+
+		/*
+		 * Now that its confirmed that OLDI is connected with DSS, let's
+		 * continue getting the OLDI sinks ahead and other OLDI
+		 * properties.
+		 */
+		bridge = devm_drm_of_get_bridge(tidss->dev, child,
+						OLDI_OURPUT_PORT, 0);
+		if (IS_ERR(bridge)) {
+			/*
+			 * Either there was no OLDI sink in the devicetree, or
+			 * the OLDI sink has not been added yet. In any case,
+			 * return.
+			 * We don't want to have an OLDI node connected to DSS
+			 * but not to any sink.
+			 */
+			ret = dev_err_probe(tidss->dev, PTR_ERR(bridge),
+					    "no panel/bridge for OLDI%u.\n",
+					    oldi_instance);
+			goto err_put_node;
+		}
+
+		link_type = get_oldi_mode(child, &companion_instance);
+		if (link_type == OLDI_MODE_UNSUPPORTED) {
+			ret = dev_err_probe(tidss->dev, -EINVAL,
+					    "OLDI%u: Unsupported OLDI connection.\n",
+					    oldi_instance);
+			goto err_put_node;
+		} else if (link_type == OLDI_MODE_SECONDARY) {
+			/*
+			 * This is the secondary OLDI node, which serves as a
+			 * companinon to the primary OLDI, when it is configured
+			 * for the dual-lvds mode. Since the primary OLDI will
+			 * be a part of bridge chain, no need to put this one
+			 * too. Continue onto the next OLDI node.
+			 */
+			continue;
+		}
+
+		oldi = devm_kzalloc(tidss->dev, sizeof(*oldi), GFP_KERNEL);
+		if (!oldi) {
+			ret = -ENOMEM;
+			goto err_put_node;
+		}
+
+		oldi->parent_vp = parent_vp;
+		oldi->oldi_instance = oldi_instance;
+		oldi->companion_instance = companion_instance;
+		oldi->link_type = link_type;
+		oldi->dev = tidss->dev;
+		oldi->next_bridge = bridge;
+
+		oldi->io_ctrl = syscon_regmap_lookup_by_phandle(child,
+								"ti,oldi-io-ctrl");
+		if (IS_ERR(oldi->io_ctrl)) {
+			ret = dev_err_probe(oldi->dev, PTR_ERR(oldi->io_ctrl),
+					    "OLDI%u: syscon_regmap_lookup_by_phandle failed.\n",
+					    oldi_instance);
+			goto err_put_node;
+		}
+
+		oldi->s_clk = of_clk_get_by_name(child, "s_clk");
+		if (IS_ERR(oldi->s_clk)) {
+			ret = dev_err_probe(oldi->dev, PTR_ERR(oldi->s_clk),
+					    "OLDI%u: Failed to get serial clock (s_clk).\n",
+					    oldi_instance);
+			goto err_put_node;
+		}
+
+		/* Register the bridge. */
+		oldi->bridge.of_node = child;
+		oldi->bridge.driver_private = oldi;
+		oldi->bridge.funcs = &tidss_oldi_bridge_funcs;
+		oldi->bridge.timings = &default_tidss_oldi_timings;
+
+		tidss->oldis[tidss->num_oldis++] = oldi;
+		oldi->tidss = tidss;
+
+		drm_bridge_add(&oldi->bridge);
+	}
+
+	of_node_put(child);
+	of_node_put(oldi_parent);
+
+	return 0;
+
+err_put_node:
+	of_node_put(child);
+	of_node_put(oldi_parent);
+	return ret;
+}
diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h b/drivers/gpu/drm/tidss/tidss_oldi.h
new file mode 100644
index 000000000000..e0bdd199d128
--- /dev/null
+++ b/drivers/gpu/drm/tidss/tidss_oldi.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2024 - Texas Instruments Incorporated
+ *
+ * Aradhya Bhatia <a-bhatia1@ti.com>
+ */
+
+#ifndef __TIDSS_OLDI_H__
+#define __TIDSS_OLDI_H__
+
+#include "tidss_drv.h"
+
+struct tidss_oldi;
+
+/* OLDI PORTS */
+#define OLDI_INPUT_PORT		0
+#define OLDI_OURPUT_PORT	1
+
+/* Control MMR Registers */
+
+/* Register offsets */
+#define OLDI_PD_CTRL            0x100
+#define OLDI_LB_CTRL            0x104
+
+/* Power control bits */
+#define OLDI_PWRDOWN_TX(n)	BIT(n)
+
+/* LVDS Bandgap reference Enable/Disable */
+#define OLDI_PWRDN_BG		BIT(8)
+
+enum tidss_oldi_link_type {
+	OLDI_MODE_UNSUPPORTED,
+	OLDI_MODE_SINGLE_LINK,
+	OLDI_MODE_CLONE_SINGLE_LINK,
+	OLDI_MODE_DUAL_LINK,
+	OLDI_MODE_SECONDARY,
+};
+
+enum oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
+
+struct oldi_bus_format {
+	u32 bus_fmt;
+	u32 data_width;
+	enum oldi_mode_reg_val oldi_mode_reg_val;
+	u32 input_bus_fmt;
+};
+
+int tidss_oldi_init(struct tidss_device *tidss);
+void tidss_oldi_deinit(struct tidss_device *tidss);
+
+#endif /* __TIDSS_OLDI_H__ */
-- 
2.34.1

Re: [PATCH v3 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter

[Krzysztof Kozlowski]

On 16/07/2024 10:42, Aradhya Bhatia wrote:
> The OLDI (transmitters) TXes do not have registers of their own, and are
> dependent on the source video-ports from the DSS to provide
> configuration data. This hardware doesn't directly sit on the internal
> bus of the SoC, but does so via the DSS. Hence, the OLDI TXes are
> supposed to be child nodes under the DSS, and not independent devices.
> 
> Two of the OLDI TXes can function in tandem to output dual-link OLDI
> output, or cloned single-link outputs. In these cases, one OLDI will be
> the primary OLDI, and the other one, a companion.
> 
> The OLDI functionality is further supported by a system-control module,
> which contains a few registers to control OLDI IO power and
> characteristics.
> 
> Add devicetree binding schema for AM625 OLDI TXes.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  .../bindings/display/ti/ti,am625-oldi.yaml    | 153 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 154 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
> new file mode 100644
> index 000000000000..0a96e600bc0b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
> @@ -0,0 +1,153 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/ti/ti,am625-oldi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments AM625 OLDI Transmitter
> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> +  - Aradhya Bhatia <a-bhatia1@ti.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The AM625 TI Keystone OpenLDI transmitter (OLDI TX) supports serialized RGB
> +  pixel data transmission between host and flat panel display over LVDS (Low
> +  Voltage Differential Sampling) interface. The OLDI TX consists of 7-to-1 data
> +  serializers, and 4-data and 1-clock LVDS outputs. It supports the LVDS output
> +  formats "jeida-18", "jeida-24" and "vesa-18", and can accept 24-bit RGB or
> +  padded and un-padded 18-bit RGB bus formats as input.
> +
> +properties:
> +  reg:
> +    maxItems: 1
> +

How does it even work without compatible? How is this schema selected?
If by part of your next patch, then this is not a proper split - this
patch itself is noop. Squash the patches.

> +  clocks:
> +    maxItems: 1
> +    description: serial clock input for the OLDI transmitters
> +
> +  clock-names:
> +    const: s_clk

Drop _clk or name it correctly.

> +
> +  ti,companion-oldi:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to companion OLDI transmitter. This property is mandatory for the
> +      primarty OLDI TX if the OLDI TXes are expected to work either in dual-lvds
> +      mode or in clone mode. This property should point to the secondary OLDI
> +      TX.
> +
> +  ti,secondary-oldi:
> +    type: boolean
> +    description: Boolean property to mark an OLDI TX as secondary node.

Why? Lack companion tells it, doesn't it?

> +
> +  ti,oldi-io-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to syscon device node mapping OLDI IO_CTRL registers found in the
> +      control MMR region. This property is needed for OLDI interface to work.

"This property is needed for OLDI interface to work." tells nothing.
Everything is needed for everything to work. Be specific.


> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Parallel RGB input port
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: LVDS output port
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +allOf:
> +  - if:
> +      properties:
> +        ti,secondary-oldi: true

This does not work... Test your schema.

> +    then:
> +      properties:
> +        ti,companion-oldi: false
> +        ti,oldi-io-ctrl: false
> +        clocks: false
> +        clock-names: false
> +
> +    else:
> +      required:
> +        - ti,oldi-io-ctrl
> +        - clocks
> +        - clock-names
> +
> +required:
> +  - reg
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +
> +    oldi_txes {

No underscores in node names.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        oldi: oldi@0 {
What is the "reg" for?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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?

> +
> +        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.

> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    patternProperties:
> +      '^oldi_tx@[0-1]$':

Please follow DTS coding style for naming.

> +        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.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter

[Aradhya Bhatia]

Hi Krzysztof,

Thank you for the reviewing the patches.


On 7/21/24 21:06, Krzysztof Kozlowski wrote:
> On 16/07/2024 10:42, Aradhya Bhatia wrote:
>> The OLDI (transmitters) TXes do not have registers of their own, and are
>> dependent on the source video-ports from the DSS to provide
>> configuration data. This hardware doesn't directly sit on the internal
>> bus of the SoC, but does so via the DSS. Hence, the OLDI TXes are
>> supposed to be child nodes under the DSS, and not independent devices.
>>
>> Two of the OLDI TXes can function in tandem to output dual-link OLDI
>> output, or cloned single-link outputs. In these cases, one OLDI will be
>> the primary OLDI, and the other one, a companion.
>>
>> The OLDI functionality is further supported by a system-control module,
>> which contains a few registers to control OLDI IO power and
>> characteristics.
>>
>> Add devicetree binding schema for AM625 OLDI TXes.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  .../bindings/display/ti/ti,am625-oldi.yaml    | 153 ++++++++++++++++++
>>  MAINTAINERS                                   |   1 +
>>  2 files changed, 154 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>> new file mode 100644
>> index 000000000000..0a96e600bc0b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>> @@ -0,0 +1,153 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/ti/ti,am625-oldi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments AM625 OLDI Transmitter
>> +
>> +maintainers:
>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> +  - Aradhya Bhatia <a-bhatia1@ti.com>
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.

Okay!

> 
>> +  The AM625 TI Keystone OpenLDI transmitter (OLDI TX) supports serialized RGB
>> +  pixel data transmission between host and flat panel display over LVDS (Low
>> +  Voltage Differential Sampling) interface. The OLDI TX consists of 7-to-1 data
>> +  serializers, and 4-data and 1-clock LVDS outputs. It supports the LVDS output
>> +  formats "jeida-18", "jeida-24" and "vesa-18", and can accept 24-bit RGB or
>> +  padded and un-padded 18-bit RGB bus formats as input.
>> +
>> +properties:
>> +  reg:
>> +    maxItems: 1
>> +
> 
> How does it even work without compatible? How is this schema selected?
> If by part of your next patch, then this is not a proper split - this
> patch itself is noop. Squash the patches.
> 

Yes, it is supposed to be picked like the next patch does it. I can
squash these both.

>> +  clocks:
>> +    maxItems: 1
>> +    description: serial clock input for the OLDI transmitters
>> +
>> +  clock-names:
>> +    const: s_clk
> 
> Drop _clk or name it correctly.

Alright!

> 
>> +
>> +  ti,companion-oldi:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      phandle to companion OLDI transmitter. This property is mandatory for the
>> +      primarty OLDI TX if the OLDI TXes are expected to work either in dual-lvds
>> +      mode or in clone mode. This property should point to the secondary OLDI
>> +      TX.
>> +
>> +  ti,secondary-oldi:
>> +    type: boolean
>> +    description: Boolean property to mark an OLDI TX as secondary node.
> 
> Why? Lack companion tells it, doesn't it?

A lack of companion doesn't mean secondary-OLDI automatically, actually.

There is also a possible configuration where 2 OLDI TXes could be
individually connected to 2 different sources => 2x single Link
configuration. The OLDI TXes would then work independently.

> 
>> +
>> +  ti,oldi-io-ctrl:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      phandle to syscon device node mapping OLDI IO_CTRL registers found in the
>> +      control MMR region. This property is needed for OLDI interface to work.
> 
> "This property is needed for OLDI interface to work." tells nothing.
> Everything is needed for everything to work. Be specific.
> 

Yes! Will fix this.

>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: Parallel RGB input port
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: LVDS output port
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        ti,secondary-oldi: true
> 
> This does not work... Test your schema.
> 

I tested again just now. At least the schema check didn't report any
error. I used the v2024.05 dtschema too.

This github gist[0] captures all details of this test.

Could you instead please elaborate what maybe wrong here, and I will try
to fix that.


>> +    then:
>> +      properties:
>> +        ti,companion-oldi: false
>> +        ti,oldi-io-ctrl: false
>> +        clocks: false
>> +        clock-names: false
>> +
>> +    else:
>> +      required:
>> +        - ti,oldi-io-ctrl
>> +        - clocks
>> +        - clock-names
>> +
>> +required:
>> +  - reg
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
>> +
>> +    oldi_txes {
> 
> No underscores in node names.
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Right. Will make the name generic.

> 
> 
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        oldi: oldi@0 {
> What is the "reg" for?

The reg is for indexing purposes so that the driver can distinguish
between which OLDI TX is under question. Since, the syscon controller
has different power control registers and bits for different OLDIs - its
important for the driver to be able to tell one from another.


Regards
Aradhya


[0]: Github Gist of schema testing for patch 2/4.
https://gist.github.com/aradhya07/e3776cb10ee64c33405db5609cbd2e4f

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

[Aradhya Bhatia]

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

Re: [PATCH v3 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter

[Krzysztof Kozlowski]

On 26/08/2024 07:47, Aradhya Bhatia wrote:
> Hi Krzysztof,
> 
> Thank you for the reviewing the patches.
> 
> 
> On 7/21/24 21:06, Krzysztof Kozlowski wrote:
>> On 16/07/2024 10:42, Aradhya Bhatia wrote:
>>> The OLDI (transmitters) TXes do not have registers of their own, and are
>>> dependent on the source video-ports from the DSS to provide
>>> configuration data. This hardware doesn't directly sit on the internal
>>> bus of the SoC, but does so via the DSS. Hence, the OLDI TXes are
>>> supposed to be child nodes under the DSS, and not independent devices.
>>>
>>> Two of the OLDI TXes can function in tandem to output dual-link OLDI
>>> output, or cloned single-link outputs. In these cases, one OLDI will be
>>> the primary OLDI, and the other one, a companion.
>>>
>>> The OLDI functionality is further supported by a system-control module,
>>> which contains a few registers to control OLDI IO power and
>>> characteristics.
>>>
>>> Add devicetree binding schema for AM625 OLDI TXes.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>  .../bindings/display/ti/ti,am625-oldi.yaml    | 153 ++++++++++++++++++
>>>  MAINTAINERS                                   |   1 +
>>>  2 files changed, 154 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>> new file mode 100644
>>> index 000000000000..0a96e600bc0b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>> @@ -0,0 +1,153 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/ti/ti,am625-oldi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Texas Instruments AM625 OLDI Transmitter
>>> +
>>> +maintainers:
>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> +  - Aradhya Bhatia <a-bhatia1@ti.com>
>>> +
>>> +description: |
>>
>> Do not need '|' unless you need to preserve formatting.
> 
> Okay!
> 
>>
>>> +  The AM625 TI Keystone OpenLDI transmitter (OLDI TX) supports serialized RGB
>>> +  pixel data transmission between host and flat panel display over LVDS (Low
>>> +  Voltage Differential Sampling) interface. The OLDI TX consists of 7-to-1 data
>>> +  serializers, and 4-data and 1-clock LVDS outputs. It supports the LVDS output
>>> +  formats "jeida-18", "jeida-24" and "vesa-18", and can accept 24-bit RGB or
>>> +  padded and un-padded 18-bit RGB bus formats as input.
>>> +
>>> +properties:
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>
>> How does it even work without compatible? How is this schema selected?
>> If by part of your next patch, then this is not a proper split - this
>> patch itself is noop. Squash the patches.
>>
> 
> Yes, it is supposed to be picked like the next patch does it. I can
> squash these both.
> 
>>> +  clocks:
>>> +    maxItems: 1
>>> +    description: serial clock input for the OLDI transmitters
>>> +
>>> +  clock-names:
>>> +    const: s_clk
>>
>> Drop _clk or name it correctly.
> 
> Alright!
> 
>>
>>> +
>>> +  ti,companion-oldi:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      phandle to companion OLDI transmitter. This property is mandatory for the
>>> +      primarty OLDI TX if the OLDI TXes are expected to work either in dual-lvds
>>> +      mode or in clone mode. This property should point to the secondary OLDI
>>> +      TX.
>>> +
>>> +  ti,secondary-oldi:
>>> +    type: boolean
>>> +    description: Boolean property to mark an OLDI TX as secondary node.
>>
>> Why? Lack companion tells it, doesn't it?
> 
> A lack of companion doesn't mean secondary-OLDI automatically, actually.
> 
> There is also a possible configuration where 2 OLDI TXes could be
> individually connected to 2 different sources => 2x single Link
> configuration. The OLDI TXes would then work independently.

You are responding for something month old. I am not in the context anymore.

Probably you miss proper graphs here, not such property.

> 
>>
>>> +
>>> +  ti,oldi-io-ctrl:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      phandle to syscon device node mapping OLDI IO_CTRL registers found in the
>>> +      control MMR region. This property is needed for OLDI interface to work.
>>
>> "This property is needed for OLDI interface to work." tells nothing.
>> Everything is needed for everything to work. Be specific.
>>
> 
> Yes! Will fix this.
> 
>>> +
>>> +  ports:
>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>> +
>>> +    properties:
>>> +      port@0:
>>> +        $ref: /schemas/graph.yaml#/properties/port
>>> +        description: Parallel RGB input port
>>> +
>>> +      port@1:
>>> +        $ref: /schemas/graph.yaml#/properties/port
>>> +        description: LVDS output port
>>> +
>>> +    required:
>>> +      - port@0
>>> +      - port@1
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        ti,secondary-oldi: true
>>
>> This does not work... Test your schema.
>>
> 
> I tested again just now. At least the schema check didn't report any
> error. I used the v2024.05 dtschema too.

No, test your condition. Come with DTS with exercises this if. You will
see this DOES NOT WORK. This is just no-op, does not perform any useful
work. So test the code that it actually performs what you want it to do.

> 
> This github gist[0] captures all details of this test.
> 
> Could you instead please elaborate what maybe wrong here, and I will try
> to fix that.

Look at example-schema or any of my talks with useful references.

> 
> 
>>> +    then:
>>> +      properties:
>>> +        ti,companion-oldi: false
>>> +        ti,oldi-io-ctrl: false
>>> +        clocks: false
>>> +        clock-names: false
>>> +
>>> +    else:
>>> +      required:
>>> +        - ti,oldi-io-ctrl
>>> +        - clocks
>>> +        - clock-names
>>> +
>>> +required:
>>> +  - reg
>>> +  - ports
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
>>> +
>>> +    oldi_txes {
>>
>> No underscores in node names.
>>
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Right. Will make the name generic.
> 
>>
>>
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        oldi: oldi@0 {
>> What is the "reg" for?
> 
> The reg is for indexing purposes so that the driver can distinguish
> between which OLDI TX is under question. Since, the syscon controller
> has different power control registers and bits for different OLDIs - its
> important for the driver to be able to tell one from another.

Again, not sure, not in context. Patch is not even in the inbox anymore.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 26/08/2024 09:32, Aradhya Bhatia wrote:
> 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?

You are missing the explanation: why do you need to represent internal
parts of a device with graph. Where does this endpoint point?

Provide some diagram showing the architecture, because either it is
wrong or I do not understand what hardware you want to represent 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. =)

This must be additionalProperties false. See example schema or writing
schema.


Best regards,
Krzysztof

Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Francesco Dolcini]

+Max

Hello Aradhya,

On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote:
> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need
> for some major changes for a full feature experience.
> 
> 1. The OF graph needs to be updated to accurately show the data flow.
> 2. The tidss and OLDI drivers now need to support the dual-link and the
>    cloned single-link OLDI video signals.
> 3. The drivers also need to support the case where 2 OLDI TXes are
>    connected to 2 different VPs - thereby creating 2 independent streams
>    of single-link OLDI outputs.

Have you considered/tested the use case in which only single link is used?
You do not mention it here and to me this is a relevant use case.

There is a workaround for this (use option 2, cloned, even if nothing is
connected to the second link), but this seems not correct.

We (Max in Cc here) noticed that this specific use case is broken on
your downstream v6.6 TI branch.

Francesco

Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Tomi Valkeinen]

Hi,

On 06/09/2024 14:43, Francesco Dolcini wrote:
> +Max
> 
> Hello Aradhya,
> 
> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote:
>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need
>> for some major changes for a full feature experience.
>>
>> 1. The OF graph needs to be updated to accurately show the data flow.
>> 2. The tidss and OLDI drivers now need to support the dual-link and the
>>     cloned single-link OLDI video signals.
>> 3. The drivers also need to support the case where 2 OLDI TXes are
>>     connected to 2 different VPs - thereby creating 2 independent streams
>>     of single-link OLDI outputs.
> 
> Have you considered/tested the use case in which only single link is used?
> You do not mention it here and to me this is a relevant use case.
> 
> There is a workaround for this (use option 2, cloned, even if nothing is
> connected to the second link), but this seems not correct.
> 
> We (Max in Cc here) noticed that this specific use case is broken on
> your downstream v6.6 TI branch.

What if you set "fw_devlink=off" kernel boot parameter?

  Tomi

Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Aradhya Bhatia]

Hi,

Thank you, Francesco and Max, for testing and reporting this!

On 09/09/24 13:45, Tomi Valkeinen wrote:
> Hi,
> 
> On 06/09/2024 14:43, Francesco Dolcini wrote:
>> +Max
>>
>> Hello Aradhya,
>>
>> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote:
>>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need
>>> for some major changes for a full feature experience.
>>>
>>> 1. The OF graph needs to be updated to accurately show the data flow.
>>> 2. The tidss and OLDI drivers now need to support the dual-link and the
>>>     cloned single-link OLDI video signals.
>>> 3. The drivers also need to support the case where 2 OLDI TXes are
>>>     connected to 2 different VPs - thereby creating 2 independent
>>> streams
>>>     of single-link OLDI outputs.
>>
>> Have you considered/tested the use case in which only single link is
>> used?
>> You do not mention it here and to me this is a relevant use case.
>>
>> There is a workaround for this (use option 2, cloned, even if nothing is
>> connected to the second link), but this seems not correct.

I agree. The whole idea behind the series is to be able to accurately
describe the hardware. Putting the devicetree in clone mode in order to
get the single-link mode working is far from ideal.

>>
>> We (Max in Cc here) noticed that this specific use case is broken on
>> your downstream v6.6 TI branch.

Yes, it was been brought to my attention that the single-link usecase is
not working over the downstream ti-6.6 kernel. As I have since
discovered, it's not working on this series either.

For some reason, the supplier-consumer dependency between the OLDI and
the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE`
in cases of single-link configuration.

This flag allows the panel driver to continue to probe without waiting
for the OLDI driver (panel's supplier). Absence of the flag getting set
is causing these drivers to keep deferring probe in an endless cycle.

Even with the flag, the OLDI (and tidss) cannot complete probe until the
panel driver is probed and ready. That is because the OLDI (and tidss)
need the panel to continue with the bridge-chain creation.

However, over with the dual-lvds configuration (and as Francesco has
now mentioned the clone configuration as well), the flag gets set by
default, and everything works.

This is what the debug has led to, so far.

> 
> What if you set "fw_devlink=off" kernel boot parameter?
> 

Yes! I haven't tested it, but it seems that this will bypass the
supplier check and let the panel probe continue.


Tomi, any idea, why is this issue happening only for single-link in the
first place? It seems as if having 2 ports inside the panel devicetree
lets the probe mechanism recognize the circular dependency and ignore
the supplier OLDIs?

This is the function where the difference comes down to, by the way -
__fw_devlink_relax_cycles(), per my knowledge. I am still on my way to
understand what exactly it is doing and why is it not relaxing the
single-link case.


Regards
Aradhya

Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Max Krummenacher]

On Mon, Sep 09, 2024 at 11:15:28AM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 06/09/2024 14:43, Francesco Dolcini wrote:
> > +Max
> > 
> > Hello Aradhya,
> > 
> > On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote:
> > > The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need
> > > for some major changes for a full feature experience.
> > > 
> > > 1. The OF graph needs to be updated to accurately show the data flow.
> > > 2. The tidss and OLDI drivers now need to support the dual-link and the
> > >     cloned single-link OLDI video signals.
> > > 3. The drivers also need to support the case where 2 OLDI TXes are
> > >     connected to 2 different VPs - thereby creating 2 independent streams
> > >     of single-link OLDI outputs.
> > 
> > Have you considered/tested the use case in which only single link is used?
> > You do not mention it here and to me this is a relevant use case.
> > 
> > There is a workaround for this (use option 2, cloned, even if nothing is
> > connected to the second link), but this seems not correct.
> > 
> > We (Max in Cc here) noticed that this specific use case is broken on
> > your downstream v6.6 TI branch.
> 
> What if you set "fw_devlink=off" kernel boot parameter?

Hi Tomi

My overlay specifing a single link which did not work by default
results in a working panel with "fw_devlink=off" on the cmdline.

Max
> 
>  Tomi
> 

Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Tomi Valkeinen]

On 09/09/2024 12:31, Aradhya Bhatia wrote:
> Hi,
> 
> Thank you, Francesco and Max, for testing and reporting this!
> 
> On 09/09/24 13:45, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 06/09/2024 14:43, Francesco Dolcini wrote:
>>> +Max
>>>
>>> Hello Aradhya,
>>>
>>> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote:
>>>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need
>>>> for some major changes for a full feature experience.
>>>>
>>>> 1. The OF graph needs to be updated to accurately show the data flow.
>>>> 2. The tidss and OLDI drivers now need to support the dual-link and the
>>>>      cloned single-link OLDI video signals.
>>>> 3. The drivers also need to support the case where 2 OLDI TXes are
>>>>      connected to 2 different VPs - thereby creating 2 independent
>>>> streams
>>>>      of single-link OLDI outputs.
>>>
>>> Have you considered/tested the use case in which only single link is
>>> used?
>>> You do not mention it here and to me this is a relevant use case.
>>>
>>> There is a workaround for this (use option 2, cloned, even if nothing is
>>> connected to the second link), but this seems not correct.
> 
> I agree. The whole idea behind the series is to be able to accurately
> describe the hardware. Putting the devicetree in clone mode in order to
> get the single-link mode working is far from ideal.

Btw, with the fw_devlink=off hack, and removing the second link from 
k3-am625-sk-microtips-mf101hie-panel.dtso, is still not enough, as the 
k3-am62-main.dtsi contains the ti,companion-oldi property which makes 
the driver think it's a cloning case.

So, I think, either the ti,companion-oldi and ti,secondary-oldi should 
only be set in the overlay when setting up cloning/dual-link, or the 
companion-oldi property shouldn't actually make a difference, and the 
selection between clone and single-link should be done via some other means.

>>> We (Max in Cc here) noticed that this specific use case is broken on
>>> your downstream v6.6 TI branch.
> 
> Yes, it was been brought to my attention that the single-link usecase is
> not working over the downstream ti-6.6 kernel. As I have since
> discovered, it's not working on this series either.
> 
> For some reason, the supplier-consumer dependency between the OLDI and
> the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE`
> in cases of single-link configuration.
> 
> This flag allows the panel driver to continue to probe without waiting
> for the OLDI driver (panel's supplier). Absence of the flag getting set
> is causing these drivers to keep deferring probe in an endless cycle.
> 
> Even with the flag, the OLDI (and tidss) cannot complete probe until the
> panel driver is probed and ready. That is because the OLDI (and tidss)
> need the panel to continue with the bridge-chain creation.
> 
> However, over with the dual-lvds configuration (and as Francesco has
> now mentioned the clone configuration as well), the flag gets set by
> default, and everything works.
> 
> This is what the debug has led to, so far.

Yes, I came to the same conclusion with my debug.

>>
>> What if you set "fw_devlink=off" kernel boot parameter?
>>
> 
> Yes! I haven't tested it, but it seems that this will bypass the
> supplier check and let the panel probe continue.
> 
> 
> Tomi, any idea, why is this issue happening only for single-link in the
> first place? It seems as if having 2 ports inside the panel devicetree
> lets the probe mechanism recognize the circular dependency and ignore
> the supplier OLDIs?

I have to say I have no idea...

I don't really understand the devlink code here, but I'm guessing that 
the "cycle" part comes from the fact that with a media graph we have 
links (remote-endpoint) both ways in the DT data. So it's not possible 
to say which side is the consumer, which one is the supplier. Thus, it's 
marked as a cycle and, I think, basically ignored for the probing purpose.

But why not here? I can see the links being created both ways:

/display Linked as a fwnode consumer to 
/bus@f0000/dss@30200000/oldi-txes/oldi@0
/bus@f0000/dss@30200000/oldi-txes/oldi@0 Linked as a fwnode consumer to 
/display

but it's never marked as a cycle.

> This is the function where the difference comes down to, by the way -
> __fw_devlink_relax_cycles(), per my knowledge. I am still on my way to
> understand what exactly it is doing and why is it not relaxing the
> single-link case.

Yep. The answer is probably somewhere there =).

  Tomi

Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support

[Aradhya Bhatia]

On 09/09/24 15:20, Tomi Valkeinen wrote:
> On 09/09/2024 12:31, Aradhya Bhatia wrote:
>> Hi,
>>
>> Thank you, Francesco and Max, for testing and reporting this!
>>
>> On 09/09/24 13:45, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 06/09/2024 14:43, Francesco Dolcini wrote:
>>>> +Max
>>>>
>>>> Hello Aradhya,
>>>>
>>>> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote:
>>>>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a
>>>>> need
>>>>> for some major changes for a full feature experience.
>>>>>
>>>>> 1. The OF graph needs to be updated to accurately show the data flow.
>>>>> 2. The tidss and OLDI drivers now need to support the dual-link and
>>>>> the
>>>>>      cloned single-link OLDI video signals.
>>>>> 3. The drivers also need to support the case where 2 OLDI TXes are
>>>>>      connected to 2 different VPs - thereby creating 2 independent
>>>>> streams
>>>>>      of single-link OLDI outputs.
>>>>
>>>> Have you considered/tested the use case in which only single link is
>>>> used?
>>>> You do not mention it here and to me this is a relevant use case.
>>>>
>>>> There is a workaround for this (use option 2, cloned, even if
>>>> nothing is
>>>> connected to the second link), but this seems not correct.
>>
>> I agree. The whole idea behind the series is to be able to accurately
>> describe the hardware. Putting the devicetree in clone mode in order to
>> get the single-link mode working is far from ideal.
> 
> Btw, with the fw_devlink=off hack, and removing the second link from k3-
> am625-sk-microtips-mf101hie-panel.dtso, is still not enough, as the k3-
> am62-main.dtsi contains the ti,companion-oldi property which makes the
> driver think it's a cloning case.

Yes!

> 
> So, I think, either the ti,companion-oldi and ti,secondary-oldi should
> only be set in the overlay when setting up cloning/dual-link, or the
> companion-oldi property shouldn't actually make a difference, and the
> selection between clone and single-link should be done via some other
> means.

Yep, those properties need to be set in the overlay file, and not in the
k3-am62-main.dtsi like it is the case in ti-6.6.

> 
>>>> We (Max in Cc here) noticed that this specific use case is broken on
>>>> your downstream v6.6 TI branch.
>>
>> Yes, it was been brought to my attention that the single-link usecase is
>> not working over the downstream ti-6.6 kernel. As I have since
>> discovered, it's not working on this series either.
>>
>> For some reason, the supplier-consumer dependency between the OLDI and
>> the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE`
>> in cases of single-link configuration.
>>
>> This flag allows the panel driver to continue to probe without waiting
>> for the OLDI driver (panel's supplier). Absence of the flag getting set
>> is causing these drivers to keep deferring probe in an endless cycle.
>>
>> Even with the flag, the OLDI (and tidss) cannot complete probe until the
>> panel driver is probed and ready. That is because the OLDI (and tidss)
>> need the panel to continue with the bridge-chain creation.
>>
>> However, over with the dual-lvds configuration (and as Francesco has
>> now mentioned the clone configuration as well), the flag gets set by
>> default, and everything works.
>>
>> This is what the debug has led to, so far.
> 
> Yes, I came to the same conclusion with my debug.
> 
>>>
>>> What if you set "fw_devlink=off" kernel boot parameter?
>>>
>>
>> Yes! I haven't tested it, but it seems that this will bypass the
>> supplier check and let the panel probe continue.
>>
>>
>> Tomi, any idea, why is this issue happening only for single-link in the
>> first place? It seems as if having 2 ports inside the panel devicetree
>> lets the probe mechanism recognize the circular dependency and ignore
>> the supplier OLDIs?
> 
> I have to say I have no idea...
> 
> I don't really understand the devlink code here, but I'm guessing that
> the "cycle" part comes from the fact that with a media graph we have
> links (remote-endpoint) both ways in the DT data. So it's not possible
> to say which side is the consumer, which one is the supplier. Thus, it's
> marked as a cycle and, I think, basically ignored for the probing purpose.

Okay! I am not too sure about the devlink code either, but this
reasoning makes sense.

> 
> But why not here? I can see the links being created both ways:
> 
> /display Linked as a fwnode consumer to /bus@f0000/dss@30200000/oldi-
> txes/oldi@0
> /bus@f0000/dss@30200000/oldi-txes/oldi@0 Linked as a fwnode consumer
> to /display
> 
> but it's never marked as a cycle.

Yes, this matches my observations.

> 
>> This is the function where the difference comes down to, by the way -
>> __fw_devlink_relax_cycles(), per my knowledge. I am still on my way to
>> understand what exactly it is doing and why is it not relaxing the
>> single-link case.
> 
> Yep. The answer is probably somewhere there =).
> 

Alright! We have an interesting problem in our hands now. =)


Regards
Aradhya