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

[PATCH 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.

The OLDI configuration should happen before the video-port configuration takes
place in tidss_crtc_atomic_enable hook. I have posted a patch allowing DRM
bridges to get enabled before the CRTC of that bridge is enabled[0]. The patch
4/4 of this series uses the bridge hooks introduced in [0], and hence will not
compile without [0].

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 AM625 DSS was kept (and eventually
merged)[2].

These patches have been tested on AM625 based platforms, SK-AM625 EVM with a
Microptis dual-lvds panel (SK-LCD1), and Beagleplay with a Lincolntech dual-lvds
panel (LCD-185T). The patches with complete support including the expected
devicetree configuration of the OLDI TXes can be found in the
"next_oldi_finals-v1-tests" branch of my github fork[3].

Thanks,
Aradhya

[0]: Dependency Patch: Introduce early_enable / late_disable drm bridge APIs
https://lore.kernel.org/all/20240511153051.1355825-7-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_finals-v1-tests


Aradhya Bhatia (4):
  dt-bindings: display: ti,am65x-dss: Minor Cleanup
  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     | 178 +++++-
 MAINTAINERS                                   |   1 +
 drivers/gpu/drm/tidss/Makefile                |   3 +-
 drivers/gpu/drm/tidss/tidss_dispc.c           |  11 +-
 drivers/gpu/drm/tidss/tidss_dispc.h           |   4 +
 drivers/gpu/drm/tidss/tidss_drv.c             |  13 +-
 drivers/gpu/drm/tidss/tidss_drv.h             |   4 +
 drivers/gpu/drm/tidss/tidss_oldi.c            | 568 ++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_oldi.h            |  73 +++
 10 files changed, 983 insertions(+), 25 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: 75fa778d74b786a1608d55d655d42b480a6fa8bd
-- 
2.34.1

[PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup

[Aradhya Bhatia]

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

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

Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup

[Laurent Pinchart]

Hi Aradhya,

Thank you for the patch.

On Sun, May 12, 2024 at 01:00:52AM +0530, Aradhya Bhatia wrote:
> Reduce tab size from 8 spaces to 4 spaces to make the bindings
> consistent, and easy to expand.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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>;
> +                };
>              };
> +        };
>      };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup

[Rob Herring]

On Sun, May 12, 2024 at 01:00:52AM +0530, Aradhya Bhatia wrote:
> Reduce tab size from 8 spaces to 4 spaces to make the bindings
> consistent, and easy to expand.

"Re-indent the example" would be more specific than "minor cleanups" in 
the subject.

Otherwise,

Acked-by: Rob Herring (Arm) <robh@kernel.org>

> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 54 +++++++++----------
>  1 file changed, 27 insertions(+), 27 deletions(-)

Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup

[Aradhya Bhatia]

On 14/05/24 00:49, Rob Herring wrote:
> On Sun, May 12, 2024 at 01:00:52AM +0530, Aradhya Bhatia wrote:
>> Reduce tab size from 8 spaces to 4 spaces to make the bindings
>> consistent, and easy to expand.
> 
> "Re-indent the example" would be more specific than "minor cleanups" in 
> the subject.

That would be better. Will reword this in v2.
Thank you!

Regards
Aradhya

> 
> Otherwise,
> 
> Acked-by: Rob Herring (Arm) <robh@kernel.org>
> 
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  .../bindings/display/ti/ti,am65x-dss.yaml     | 54 +++++++++----------
>>  1 file changed, 27 insertions(+), 27 deletions(-)
> 

Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup

[Tomi Valkeinen]

On 11/05/2024 22:30, Aradhya Bhatia wrote:
> Reduce tab size from 8 spaces to 4 spaces to make the bindings
> consistent, and easy to expand.
> 
> 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>;
> +                };
>               };
> +        };
>       };

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

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

[Aradhya Bhatia]

Add devicetree binding schema for AM625 OLDI Transmitters.

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 c675fc296b19..4426c4d41a7f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7480,6 +7480,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

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

[Laurent Pinchart]

Hi Aradhya,

Thank you for the patch.

On Sun, May 12, 2024 at 01:00:53AM +0530, Aradhya Bhatia wrote:
> Add devicetree binding schema for AM625 OLDI Transmitters.
> 
> 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>;

What bus does this device live on ? Couldn't the I/O register space be
referenced by the reg property ?

> +            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 c675fc296b19..4426c4d41a7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7480,6 +7480,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

-- 
Regards,

Laurent Pinchart

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

[Aradhya Bhatia]

Hi Laurent,

Thank you for reviewing the patches!

On 13-May-24 01:04, Laurent Pinchart wrote:
> Hi Aradhya,
> 
> Thank you for the patch.
> 
> On Sun, May 12, 2024 at 01:00:53AM +0530, Aradhya Bhatia wrote:
>> Add devicetree binding schema for AM625 OLDI Transmitters.
>>
>> 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>;
> 
> What bus does this device live on ? Couldn't the I/O register space be
> referenced by the reg property ?.
> 

These registers are a part of the system-controller register space
(ctrl_mmr0). The whole register set is owned by the main_conf[0]
devicetree node, with sub-nodes pointing to specific regions. That's why
I cannot reference these registers directly.

The IO control node for OLDI will look like this though[1].

[0]:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/ti/k3-am62-main.dtsi#n45

[1]:
https://github.com/aradhya07/linux-ab/commit/7d7184fb36dc22c67cc2704fe708e885f300860c


>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +                port@0 {
>> +                    reg = <0>;
>> +                    oldi_in: endpoint {
>> +                        remote-endpoint = <&dpi0_out>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
>> +
> 

[ ... ]

-- 
Regards
Aradhya

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

[Rob Herring]

On Mon, May 13, 2024 at 02:07:44PM +0530, Aradhya Bhatia wrote:
> Hi Laurent,
> 
> Thank you for reviewing the patches!
> 
> On 13-May-24 01:04, Laurent Pinchart wrote:
> > Hi Aradhya,
> > 
> > Thank you for the patch.
> > 
> > On Sun, May 12, 2024 at 01:00:53AM +0530, Aradhya Bhatia wrote:
> >> Add devicetree binding schema for AM625 OLDI Transmitters.
> >>
> >> 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>;
> > 
> > What bus does this device live on ? Couldn't the I/O register space be
> > referenced by the reg property ?.
> > 
> 
> These registers are a part of the system-controller register space
> (ctrl_mmr0). The whole register set is owned by the main_conf[0]
> devicetree node, with sub-nodes pointing to specific regions. That's why
> I cannot reference these registers directly.

Then what does 'reg' represent? Looks like you just made up an index. If 
so, then this should probably be a child of &dss_oldi_io_ctrl instead. 
Or it should just be merged into that node.

Rob

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

[Aradhya Bhatia]

Hi Rob,

Thank you for reviewing the patches!

On 14/05/24 01:00, Rob Herring wrote:
> On Mon, May 13, 2024 at 02:07:44PM +0530, Aradhya Bhatia wrote:
>> Hi Laurent,
>>
>> Thank you for reviewing the patches!
>>
>> On 13-May-24 01:04, Laurent Pinchart wrote:
>>> Hi Aradhya,
>>>
>>> Thank you for the patch.
>>>
>>> On Sun, May 12, 2024 at 01:00:53AM +0530, Aradhya Bhatia wrote:
>>>> Add devicetree binding schema for AM625 OLDI Transmitters.
>>>>
>>>> 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>;
>>>
>>> What bus does this device live on ? Couldn't the I/O register space be
>>> referenced by the reg property ?.
>>>
>>
>> These registers are a part of the system-controller register space
>> (ctrl_mmr0). The whole register set is owned by the main_conf[0]
>> devicetree node, with sub-nodes pointing to specific regions. That's why
>> I cannot reference these registers directly.
> 
> Then what does 'reg' represent? Looks like you just made up an index. If 
> so, then this should probably be a child of &dss_oldi_io_ctrl instead. 
> Or it should just be merged into that node.
> 

I did make up an index when I used the 'reg' property. Similar to how
ports can be indexed. The AM65 has 1 OLDI TX. AM62 and AM62P have 2 OLDI
TXes each. The index is to help the driver parse through each of them.

If I push these OLDI TX nodes as child nodes under &dss_oldi_io_ctrl,
then that would be an inaccurate representation of the hardware.

The OLDI TXes are very well a part of the DSS hardware. As such, the
(three) registers that control the functionality have been made a part
of the DSS video-port (VP) register space, leaving OLDI TXes with no
direct access to the primary bus (cbass_main) where the DSS sits.

The IO control registers, on the other hand, do not affect OLDI
functionality in any way. These are just helper registers that merely
control the power characteristics of the OLDI data and clock lanes.

Regards
Aradhya

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

[Tomi Valkeinen]

On 14/05/2024 08:08, Aradhya Bhatia wrote:
> Hi Rob,
> 
> Thank you for reviewing the patches!
> 
> On 14/05/24 01:00, Rob Herring wrote:
>> On Mon, May 13, 2024 at 02:07:44PM +0530, Aradhya Bhatia wrote:
>>> Hi Laurent,
>>>
>>> Thank you for reviewing the patches!
>>>
>>> On 13-May-24 01:04, Laurent Pinchart wrote:
>>>> Hi Aradhya,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Sun, May 12, 2024 at 01:00:53AM +0530, Aradhya Bhatia wrote:
>>>>> Add devicetree binding schema for AM625 OLDI Transmitters.
>>>>>
>>>>> 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>;
>>>>
>>>> What bus does this device live on ? Couldn't the I/O register space be
>>>> referenced by the reg property ?.
>>>>
>>>
>>> These registers are a part of the system-controller register space
>>> (ctrl_mmr0). The whole register set is owned by the main_conf[0]
>>> devicetree node, with sub-nodes pointing to specific regions. That's why
>>> I cannot reference these registers directly.
>>
>> Then what does 'reg' represent? Looks like you just made up an index. If
>> so, then this should probably be a child of &dss_oldi_io_ctrl instead.
>> Or it should just be merged into that node.
>>
> 
> I did make up an index when I used the 'reg' property. Similar to how
> ports can be indexed. The AM65 has 1 OLDI TX. AM62 and AM62P have 2 OLDI
> TXes each. The index is to help the driver parse through each of them.
> 
> If I push these OLDI TX nodes as child nodes under &dss_oldi_io_ctrl,
> then that would be an inaccurate representation of the hardware.
> 
> The OLDI TXes are very well a part of the DSS hardware. As such, the
> (three) registers that control the functionality have been made a part
> of the DSS video-port (VP) register space, leaving OLDI TXes with no
> direct access to the primary bus (cbass_main) where the DSS sits.
> 
> The IO control registers, on the other hand, do not affect OLDI
> functionality in any way. These are just helper registers that merely
> control the power characteristics of the OLDI data and clock lanes.

Just summarizing my understanding from today's code and ref manual (TRM) 
reading, which mostly repeats what Aradhya said above:

There may be 0, 1 or 2 OLDI instances, depending on the SoC. With two 
instances, they may be (or are currently always?) connected to the same 
display controller videoport, and thus can be used for cloning or 
dual-link lvds.

In the TRM the OLDI instances are depicted to be inside DSS (display 
subsystem), but outside DISPC (display controller). However, the 
registers to configure and control OLDI are in the DISPC register block. 
More specifically, in the DISPC VP1 block, which is the videoport to 
which the OLDIs are connected to, and additionally one bit in the 
DSS_SYSSTATUS register.

The oldi-io-ctrl points to a system-control module, which contains a 
register to control OLDI power up/down. But why is it called 
"oldi-io-ctrl"? The register in question is OLDI_PD_CTRL.

So... How to represent this in the DT?

I think we do want have separate nodes for the OLDIs, so that we can use 
the of_graph to represent how the data flows. OLDI doesn't have its own 
register block, and is controlled via DSS. If the OLDI nodes are not 
children of the DSS node, where could they be?

I think the DT design in these patches matches my understanding of the 
hardware.

  Tomi

[PATCH 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
properties.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 136 +++++++++++++++++-
 1 file changed, 135 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 399d68986326..4aa2de59b32b 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -85,12 +85,30 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
         description:
           For AM65x DSS, the OLDI output port node from video port 1.
           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,22 @@ properties:
       Input memory (from main memory to dispc) bandwidth limit in
       bytes per second
 
+  oldi-txes:
+    type: object
+    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 +157,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 +218,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

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

[Rob Herring]

On Sun, May 12, 2024 at 01:00:54AM +0530, Aradhya Bhatia wrote:
> The DSS in AM625 SoC has 2 OLDI TXes. Refer the OLDI schema to add the
> properties.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 136 +++++++++++++++++-
>  1 file changed, 135 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 399d68986326..4aa2de59b32b 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -85,12 +85,30 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base

You don't need this change. You aren't adding any extra properties.

>          description:
>            For AM65x DSS, the OLDI output port node from video port 1.
>            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,22 @@ properties:
>        Input memory (from main memory to dispc) bandwidth limit in
>        bytes per second
>  
> +  oldi-txes:
> +    type: object
> +    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

Connected to is not part of the DSS. I don't think these nodes belong 
here as mentioned in the other patch.

Rob

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

[Aradhya Bhatia]

On 14/05/24 01:05, Rob Herring wrote:
> On Sun, May 12, 2024 at 01:00:54AM +0530, Aradhya Bhatia wrote:
>> The DSS in AM625 SoC has 2 OLDI TXes. Refer the OLDI schema to add the
>> properties.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  .../bindings/display/ti/ti,am65x-dss.yaml     | 136 +++++++++++++++++-
>>  1 file changed, 135 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 399d68986326..4aa2de59b32b 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -85,12 +85,30 @@ properties:
>>  
>>      properties:
>>        port@0:
>> -        $ref: /schemas/graph.yaml#/properties/port
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
> 
> You don't need this change. You aren't adding any extra properties.

Alright. I will make the change.

> 
>>          description:
>>            For AM65x DSS, the OLDI output port node from video port 1.
>>            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,22 @@ properties:
>>        Input memory (from main memory to dispc) bandwidth limit in
>>        bytes per second
>>  
>> +  oldi-txes:
>> +    type: object
>> +    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
> 
> Connected to is not part of the DSS. I don't think these nodes belong 
> here as mentioned in the other patch.
> 

Replied to this in patch 2/4 to keep the discussion in one thread.


Regards
Aradhya

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

[Aradhya Bhatia]

Up till now, the OLDI support in tidss was integrated within the tidss dispc.
This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone
modes.

Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
configurations.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
Note:

The OLDI configuration should happen before the video-port configuration takes
place in tidss_crtc_atomic_enable hook. I have posted a patch allowing DRM
bridges to get enabled before the CRTC of that bridge is enabled[0]. This patch
uses the bridge hooks introduced in [0], and hence will not compile without [0].

[0]: Dependency Patch: Introduce early_enable / late_disable drm bridge APIs
https://lore.kernel.org/all/20240511153051.1355825-7-a-bhatia1@ti.com/

---
 drivers/gpu/drm/tidss/Makefile      |   3 +-
 drivers/gpu/drm/tidss/tidss_dispc.c |  11 +-
 drivers/gpu/drm/tidss/tidss_dispc.h |   4 +
 drivers/gpu/drm/tidss/tidss_drv.c   |  13 +-
 drivers/gpu/drm/tidss/tidss_drv.h   |   4 +
 drivers/gpu/drm/tidss/tidss_oldi.c  | 568 ++++++++++++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_oldi.h  |  73 ++++
 7 files changed, 673 insertions(+), 3 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..4961da3989c0 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -466,6 +466,16 @@ static u32 dispc_vp_read(struct dispc_device *dispc, u32 hw_videoport, u16 reg)
 	return ioread32(base + reg);
 }
 
+u32 tidss_get_status(struct tidss_device *tidss)
+{
+	return dispc_read(tidss->dispc, DSS_SYSSTATUS);
+}
+
+void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, u32 val)
+{
+	return dispc_vp_write(tidss->dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, val);
+}
+
 /*
  * TRM gives bitfields as start:end, where start is the higher bit
  * number. For example 7:0
@@ -1310,7 +1320,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..800a73457aff 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;
 
+u32 tidss_get_status(struct tidss_device *tidss);
+void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, u32 val);
+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_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index d15f836dca95..fd90e8498cc2 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 */
 
@@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev)
 
 	spin_lock_init(&tidss->wait_lock);
 
+	ret = tidss_oldi_init(tidss);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to init OLDI (%d)\n", ret);
+		return ret;
+	}
+
 	ret = dispc_init(tidss);
 	if (ret) {
 		dev_err(dev, "failed to initialize dispc: %d\n", ret);
-		return ret;
+		goto err_oldi_deinit;
 	}
 
 	pm_runtime_enable(dev);
@@ -202,6 +210,9 @@ static int tidss_probe(struct platform_device *pdev)
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
+err_oldi_deinit:
+	tidss_oldi_deinit(tidss);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..4ccdc177d171 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -11,6 +11,7 @@
 
 #define TIDSS_MAX_PORTS 4
 #define TIDSS_MAX_PLANES 4
+#define TIDSS_MAX_OLDI_TXES 2
 
 typedef u32 dispc_irq_t;
 
@@ -27,6 +28,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..fd96ca815542
--- /dev/null
+++ b/drivers/gpu/drm/tidss/tidss_oldi.c
@@ -0,0 +1,568 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 - Texas Instruments Incorporated
+ *
+ * Aradhya Bhatia <a-bhati1@ti.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.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_panel.h>
+#include <drm/drm_of.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_probe_helper.h>
+
+#include "tidss_drv.h"
+#include "tidss_oldi.h"
+
+struct tidss_oldi {
+	struct tidss_device	*tidss;
+	struct device		*dev;
+
+	struct drm_bridge	bridge;
+	struct drm_bridge	*next_bridge;
+
+	struct drm_panel *panel;
+
+	enum tidss_oldi_link_type link_type;
+	const struct oldi_bus_format *bus_format;
+	u32 oldi_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 },
+};
+
+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%d Failure attach next bridge\n",
+			__func__, oldi->oldi_instance);
+		return -ENODEV;
+	}
+
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		dev_err(oldi->dev,
+			"%s: OLDI%d 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
+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%d: 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%d Clock rate %lu differs over 5%% from requested %lu\n",
+			 oldi->oldi_instance, new_rate, rate);
+
+	dev_dbg(oldi->dev, "OLDI%d: 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 power)
+{
+	u32 val = 0, mask;
+
+	if (WARN_ON(!oldi->io_ctrl))
+		return;
+
+	/*
+	 * 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 (power) {
+		val = 0;
+		switch (oldi->link_type) {
+		case OLDI_MODE_SINGLE_LINK:
+			if (oldi->oldi_instance == OLDI(0))
+				mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_BG;
+			else if (oldi->oldi_instance == OLDI(1))
+				mask = OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
+
+			break;
+		case OLDI_MODE_CLONE_SINGLE_LINK:
+		case OLDI_MODE_DUAL_LINK:
+			mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
+			break;
+		default:
+			if (oldi->oldi_instance == OLDI(0))
+				mask = OLDI_PWRDN_TX(0);
+			else if (oldi->oldi_instance == OLDI(1))
+				mask = OLDI_PWRDN_TX(1);
+
+			val = mask;
+			break;
+		}
+	} else {
+		switch (oldi->link_type) {
+		case OLDI_MODE_CLONE_SINGLE_LINK:
+		case OLDI_MODE_DUAL_LINK:
+			mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
+			break;
+		case OLDI_MODE_SINGLE_LINK:
+		default:
+			if (oldi->oldi_instance == OLDI(0))
+				mask = OLDI_PWRDN_TX(0);
+			else if (oldi->oldi_instance == OLDI(1))
+				mask = OLDI_PWRDN_TX(1);
+
+			break;
+		}
+		val = mask;
+	}
+
+	regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, val);
+}
+
+static int tidss_oldi_config(struct tidss_oldi *oldi)
+{
+	const struct oldi_bus_format *bus_fmt = NULL;
+	u32 oldi_reset_bit = BIT(5);
+	int count = 0;
+	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%d: 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%d: Unsupported mode.\n",
+			oldi->oldi_instance);
+		return -EINVAL;
+	}
+
+	tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg);
+	while (!(oldi_reset_bit & tidss_get_status(oldi->tidss)) &&
+	       count < 10000)
+		count++;
+
+	if (!(oldi_reset_bit & tidss_get_status(oldi->tidss)))
+		dev_warn(oldi->dev, "%s: OLDI%d timeout waiting OLDI reset done\n",
+			 __func__, oldi->oldi_instance);
+
+	return 0;
+}
+
+static void tidss_oldi_atomic_early_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);
+
+	/* Enable the OLDI serial clock (7 times the pixel clock) */
+	oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
+
+	/* Enable OLDI IO power */
+	tidss_oldi_tx_power(oldi, true);
+}
+
+static void tidss_oldi_atomic_late_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);
+
+	/* Disable OLDI clock by setting IDLE Frequency */
+	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 int tidss_oldi_atomic_check(struct drm_bridge *bridge,
+				   struct drm_bridge_state *bridge_state,
+				   struct drm_crtc_state *crtc_state,
+				   struct drm_connector_state *conn_state)
+{
+	/*
+	 * There might be flags negotiation supported in future but
+	 * set the bus flags in atomic_check statically for now.
+	 */
+
+	/* Not sure what this is required for, at the moment */
+	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
+
+	return 0;
+}
+
+static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
+	.attach		= tidss_oldi_bridge_attach,
+	.atomic_check	= tidss_oldi_atomic_check,
+	.atomic_early_enable = tidss_oldi_atomic_early_enable,
+	.atomic_late_disable = tidss_oldi_atomic_late_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)
+{
+	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) {
+		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;
+	}
+
+	/*
+	 * 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 u32 get_parent_dss_vp(struct device_node *oldi_tx, u32 *parent_vp)
+{
+	struct device_node *ep, *dss_port;
+	int ret = 0;
+
+	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;
+	struct drm_panel *panel;
+	u32 parent_vp, oldi_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.
+		 */
+		ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
+						  &panel, &bridge);
+		if (ret) {
+			/*
+			 * 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.
+			 */
+			if (ret != -EPROBE_DEFER)
+				dev_err(tidss->dev,
+					"no panel/bridge for OLDI%d. Error %d\n",
+					oldi_instance, ret);
+			goto err_put_node;
+		}
+
+		if (panel) {
+			bridge = devm_drm_panel_bridge_add(tidss->dev, panel);
+			if (IS_ERR(bridge)) {
+				ret = PTR_ERR(bridge);
+				goto err_put_node;
+			}
+		}
+
+		link_type = get_oldi_mode(child);
+		if (link_type == OLDI_MODE_UNSUPPORTED) {
+			dev_err(tidss->dev,
+				"OLDI%d: Unsupported OLDI connection.\n",
+				oldi_instance);
+			ret = -EINVAL;
+			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->link_type = link_type;
+		oldi->oldi_instance = oldi_instance;
+		oldi->dev = tidss->dev;
+		oldi->next_bridge = bridge;
+		oldi->panel = panel;
+
+		oldi->io_ctrl = syscon_regmap_lookup_by_phandle(child,
+								"ti,oldi-io-ctrl");
+		if (IS_ERR(oldi->io_ctrl)) {
+			dev_err(oldi->dev,
+				"%s: oldi%d syscon_regmap_lookup_by_phandle failed %ld\n",
+			       __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
+			ret = PTR_ERR(oldi->io_ctrl);
+			goto err_put_node;
+		}
+
+		oldi->s_clk = of_clk_get_by_name(child, "s_clk");
+		if (IS_ERR(oldi->s_clk)) {
+			dev_err(oldi->dev,
+				"%s: oldi%d Failed to get s_clk: %ld\n",
+				__func__, oldi_instance, PTR_ERR(oldi->s_clk));
+			ret = PTR_ERR(oldi->s_clk);
+			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);
+	}
+
+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..5ad02ddea11a
--- /dev/null
+++ b/drivers/gpu/drm/tidss/tidss_oldi.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2023 - Texas Instruments Incorporated
+ *
+ * Aradhya Bhatia <a-bhati1@ti.com>
+ */
+
+#ifndef __TIDSS_OLDI_H__
+#define __TIDSS_OLDI_H__
+
+#include <linux/media-bus-format.h>
+
+#include "tidss_drv.h"
+#include "tidss_dispc.h"
+
+struct tidss_oldi;
+
+/* OLDI Instances */
+#define OLDI(n)		n
+
+/* OLDI PORTS */
+#define OLDI_INPUT_PORT		0
+#define OLDI_OURPUT_PORT	1
+
+/* OLDI Config Bits */
+#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)
+
+/* Control MMR Register */
+
+/* Register offsets */
+#define OLDI_PD_CTRL            0x100
+#define OLDI_LB_CTRL            0x104
+
+/* Power control bits */
+#define OLDI_PWRDN_TX(n)	BIT(n)
+
+/* LVDS Bandgap reference Enable/Disable */
+#define OLDI_PWRDN_BG		BIT(8)
+
+#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
+
+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 4/4] drm/tidss: Add OLDI bridge support

[Francesco Dolcini]

Hello Aradhya, thanks for you patch, I should be able to test your patch on my
hardware in the coming days.

On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote:
> Up till now, the OLDI support in tidss was integrated within the tidss dispc.
> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone
> modes.
> 
> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
> configurations.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  drivers/gpu/drm/tidss/Makefile      |   3 +-
>  drivers/gpu/drm/tidss/tidss_dispc.c |  11 +-
>  drivers/gpu/drm/tidss/tidss_dispc.h |   4 +
>  drivers/gpu/drm/tidss/tidss_drv.c   |  13 +-
>  drivers/gpu/drm/tidss/tidss_drv.h   |   4 +
>  drivers/gpu/drm/tidss/tidss_oldi.c  | 568 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/tidss/tidss_oldi.h  |  73 ++++
>  7 files changed, 673 insertions(+), 3 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/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index d15f836dca95..fd90e8498cc2 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 */
>  
> @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&tidss->wait_lock);
>  
> +	ret = tidss_oldi_init(tidss);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to init OLDI (%d)\n", ret);
> +		return ret;
> +	}

return dev_err_probe()

> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
> new file mode 100644
> index 000000000000..fd96ca815542
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
> @@ -0,0 +1,568 @@

...

> +		ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
> +						  &panel, &bridge);
> +		if (ret) {
> +			/*
> +			 * 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.
> +			 */
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(tidss->dev,
> +					"no panel/bridge for OLDI%d. Error %d\n",
> +					oldi_instance, ret);

just dev_err_probe

> +			goto err_put_node;
> +		}

...

> +		if (IS_ERR(oldi->io_ctrl)) {
> +			dev_err(oldi->dev,
> +				"%s: oldi%d syscon_regmap_lookup_by_phandle failed %ld\n",
> +			       __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
> +			ret = PTR_ERR(oldi->io_ctrl);

dev_err_probe 

> +			goto err_put_node;
> +		}
> +
> +		oldi->s_clk = of_clk_get_by_name(child, "s_clk");
> +		if (IS_ERR(oldi->s_clk)) {
> +			dev_err(oldi->dev,
> +				"%s: oldi%d Failed to get s_clk: %ld\n",
> +				__func__, oldi_instance, PTR_ERR(oldi->s_clk));
> +			ret = PTR_ERR(oldi->s_clk);

dev_err_probe

In general, in this function, sometime you print an error and goto
err_put_node, sometime you just goto err_put_node.  Not sure what's the
rationale on this.

> +			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);
> +	}
> +
> +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..5ad02ddea11a
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2023 - Texas Instruments Incorporated
> + *
> + * Aradhya Bhatia <a-bhati1@ti.com>
> + */
> +
> +#ifndef __TIDSS_OLDI_H__
> +#define __TIDSS_OLDI_H__
> +
> +#include <linux/media-bus-format.h>
> +
> +#include "tidss_drv.h"
> +#include "tidss_dispc.h"
> +
> +struct tidss_oldi;

why do you need this here? 

> +
> +/* OLDI Instances */
> +#define OLDI(n)		n
> +
> +/* OLDI PORTS */
> +#define OLDI_INPUT_PORT		0
> +#define OLDI_OURPUT_PORT	1
> +
> +/* OLDI Config Bits */
> +#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)
> +
> +/* Control MMR Register */
> +
> +/* Register offsets */
> +#define OLDI_PD_CTRL            0x100
> +#define OLDI_LB_CTRL            0x104
> +
> +/* Power control bits */
> +#define OLDI_PWRDN_TX(n)	BIT(n)
> +
> +/* LVDS Bandgap reference Enable/Disable */
> +#define OLDI_PWRDN_BG		BIT(8)
> +
> +#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
this is used only on a single C files, move it there?

I would consider this comment in general for this header file,
from a quick check most of this is used only in tidss_oldi.c.

Francesco

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

[Aradhya Bhatia]

Hi Francesco,

On 12/05/24 17:18, Francesco Dolcini wrote:
> Hello Aradhya, thanks for you patch, I should be able to test your patch on my
> hardware in the coming days.

That's appreciated. Thank you! =)

> 
> On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote:
>> Up till now, the OLDI support in tidss was integrated within the tidss dispc.
>> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
>> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone
>> modes.
>>
>> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
>> configurations.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  drivers/gpu/drm/tidss/Makefile      |   3 +-
>>  drivers/gpu/drm/tidss/tidss_dispc.c |  11 +-
>>  drivers/gpu/drm/tidss/tidss_dispc.h |   4 +
>>  drivers/gpu/drm/tidss/tidss_drv.c   |  13 +-
>>  drivers/gpu/drm/tidss/tidss_drv.h   |   4 +
>>  drivers/gpu/drm/tidss/tidss_oldi.c  | 568 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/tidss/tidss_oldi.h  |  73 ++++
>>  7 files changed, 673 insertions(+), 3 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/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>> index d15f836dca95..fd90e8498cc2 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 */
>>  
>> @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev)
>>  
>>  	spin_lock_init(&tidss->wait_lock);
>>  
>> +	ret = tidss_oldi_init(tidss);
>> +	if (ret) {
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to init OLDI (%d)\n", ret);
>> +		return ret;
>> +	}
> 
> return dev_err_probe()
> 
>> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
>> new file mode 100644
>> index 000000000000..fd96ca815542
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
>> @@ -0,0 +1,568 @@
> 
> ...
> 
>> +		ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
>> +						  &panel, &bridge);
>> +		if (ret) {
>> +			/*
>> +			 * 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.
>> +			 */
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err(tidss->dev,
>> +					"no panel/bridge for OLDI%d. Error %d\n",
>> +					oldi_instance, ret);
> 
> just dev_err_probe
> 
>> +			goto err_put_node;
>> +		}
> 
> ...
> 
>> +		if (IS_ERR(oldi->io_ctrl)) {
>> +			dev_err(oldi->dev,
>> +				"%s: oldi%d syscon_regmap_lookup_by_phandle failed %ld\n",
>> +			       __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
>> +			ret = PTR_ERR(oldi->io_ctrl);
> 
> dev_err_probe 
> 
>> +			goto err_put_node;
>> +		}
>> +
>> +		oldi->s_clk = of_clk_get_by_name(child, "s_clk");
>> +		if (IS_ERR(oldi->s_clk)) {
>> +			dev_err(oldi->dev,
>> +				"%s: oldi%d Failed to get s_clk: %ld\n",
>> +				__func__, oldi_instance, PTR_ERR(oldi->s_clk));
>> +			ret = PTR_ERR(oldi->s_clk);
> 
> dev_err_probe

Got it. Will update in all the 4 places.

> 
> In general, in this function, sometime you print an error and goto
> err_put_node, sometime you just goto err_put_node.  Not sure what's the
> rationale on this.

There hasn't been any real logic behind the prints, except that I have
added them whenever there was something (specifc) to be explained. Other
times, for example, if the error is -ENOMEM, or any other systemic API
failure, there isn't any print required.

If this function does exit in an error, however, tidss_probe will always
throw a print (except in the case of -EPROBE_DEFER).

> 
>> +			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);
>> +	}
>> +
>> +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..5ad02ddea11a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
>> @@ -0,0 +1,73 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2023 - Texas Instruments Incorporated
>> + *
>> + * Aradhya Bhatia <a-bhati1@ti.com>
>> + */
>> +
>> +#ifndef __TIDSS_OLDI_H__
>> +#define __TIDSS_OLDI_H__
>> +
>> +#include <linux/media-bus-format.h>
>> +
>> +#include "tidss_drv.h"
>> +#include "tidss_dispc.h"
>> +
>> +struct tidss_oldi;
> 
> why do you need this here? 

So that struct tidss_device can store pointers to struct tidss_oldi
instances.

> 
>> +
>> +/* OLDI Instances */
>> +#define OLDI(n)		n
>> +
>> +/* OLDI PORTS */
>> +#define OLDI_INPUT_PORT		0
>> +#define OLDI_OURPUT_PORT	1
>> +
>> +/* OLDI Config Bits */
>> +#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)
>> +
>> +/* Control MMR Register */
>> +
>> +/* Register offsets */
>> +#define OLDI_PD_CTRL            0x100
>> +#define OLDI_LB_CTRL            0x104
>> +
>> +/* Power control bits */
>> +#define OLDI_PWRDN_TX(n)	BIT(n)
>> +
>> +/* LVDS Bandgap reference Enable/Disable */
>> +#define OLDI_PWRDN_BG		BIT(8)
>> +
>> +#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
> this is used only on a single C files, move it there?
> 
> I would consider this comment in general for this header file,
> from a quick check most of this is used only in tidss_oldi.c.

Apart from struct tidss_device being able to access struct tidss_oldi,
there is no direct access to any of the above.

Perhaps I can move the idle clock definition into the C file.

However, before tidss_oldi.h, all the register definitions have been
stored in tidss_dispc_regs.h. It just seemed right to keep them out in
the header file and maintain the status quo.


Regards
Aradhya

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

[Francesco Dolcini]

Hello Aradhya,

On Sun, May 12, 2024 at 08:53:12PM +0530, Aradhya Bhatia wrote:
> On 12/05/24 17:18, Francesco Dolcini wrote:
> > On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote:
> >> Up till now, the OLDI support in tidss was integrated within the tidss dispc.
> >> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
> >> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone
> >> modes.
> >>
> >> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
> >> configurations.
> >>
> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >> ---
> >>  drivers/gpu/drm/tidss/Makefile      |   3 +-
> >>  drivers/gpu/drm/tidss/tidss_dispc.c |  11 +-
> >>  drivers/gpu/drm/tidss/tidss_dispc.h |   4 +
> >>  drivers/gpu/drm/tidss/tidss_drv.c   |  13 +-
> >>  drivers/gpu/drm/tidss/tidss_drv.h   |   4 +
> >>  drivers/gpu/drm/tidss/tidss_oldi.c  | 568 ++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/tidss/tidss_oldi.h  |  73 ++++
> >>  7 files changed, 673 insertions(+), 3 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/tidss_oldi.h b/drivers/gpu/drm/tidss/tidss_oldi.h
> >> new file mode 100644
> >> index 000000000000..5ad02ddea11a
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
> >> @@ -0,0 +1,73 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2023 - Texas Instruments Incorporated
> >> + *
> >> + * Aradhya Bhatia <a-bhati1@ti.com>
> >> + */
> >> +
> >> +#ifndef __TIDSS_OLDI_H__
> >> +#define __TIDSS_OLDI_H__
> >> +
> >> +#include <linux/media-bus-format.h>
> >> +
> >> +#include "tidss_drv.h"
> >> +#include "tidss_dispc.h"
> >> +
> >> +struct tidss_oldi;
> > 
> > why do you need this here? 
> 
> So that struct tidss_device can store pointers to struct tidss_oldi
> instances.

on this and ...

> >> +#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
> > this is used only on a single C files, move it there?
> > 
> > I would consider this comment in general for this header file,
> > from a quick check most of this is used only in tidss_oldi.c.
> 
> Apart from struct tidss_device being able to access struct tidss_oldi,
> there is no direct access to any of the above.
> 
> Perhaps I can move the idle clock definition into the C file.
> 
> However, before tidss_oldi.h, all the register definitions have been
> stored in tidss_dispc_regs.h. It just seemed right to keep them out in
> the header file and maintain the status quo.

... this they are probably more of a personal taste topic, just go
for whatever you and the actual maintainer (tomi?) prefer.

Thanks,
Francesco

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

[Tomi Valkeinen]

On 11/05/2024 22:30, Aradhya Bhatia wrote:
> Up till now, the OLDI support in tidss was integrated within the tidss dispc.
> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone
> modes.
> 
> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
> configurations.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
> Note:
> 
> The OLDI configuration should happen before the video-port configuration takes
> place in tidss_crtc_atomic_enable hook. I have posted a patch allowing DRM
> bridges to get enabled before the CRTC of that bridge is enabled[0]. This patch
> uses the bridge hooks introduced in [0], and hence will not compile without [0].
> 
> [0]: Dependency Patch: Introduce early_enable / late_disable drm bridge APIs
> https://lore.kernel.org/all/20240511153051.1355825-7-a-bhatia1@ti.com/
> 
> ---
>   drivers/gpu/drm/tidss/Makefile      |   3 +-
>   drivers/gpu/drm/tidss/tidss_dispc.c |  11 +-
>   drivers/gpu/drm/tidss/tidss_dispc.h |   4 +
>   drivers/gpu/drm/tidss/tidss_drv.c   |  13 +-
>   drivers/gpu/drm/tidss/tidss_drv.h   |   4 +
>   drivers/gpu/drm/tidss/tidss_oldi.c  | 568 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/tidss/tidss_oldi.h  |  73 ++++
>   7 files changed, 673 insertions(+), 3 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..4961da3989c0 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -466,6 +466,16 @@ static u32 dispc_vp_read(struct dispc_device *dispc, u32 hw_videoport, u16 reg)
>   	return ioread32(base + reg);
>   }
>   
> +u32 tidss_get_status(struct tidss_device *tidss)
> +{
> +	return dispc_read(tidss->dispc, DSS_SYSSTATUS);
> +}
> +
> +void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, u32 val)
> +{
> +	return dispc_vp_write(tidss->dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, val);
> +}
> +
>   /*
>    * TRM gives bitfields as start:end, where start is the higher bit
>    * number. For example 7:0
> @@ -1310,7 +1320,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..800a73457aff 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;
>   
> +u32 tidss_get_status(struct tidss_device *tidss);
> +void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, u32 val);
> +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_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index d15f836dca95..fd90e8498cc2 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 */
>   
> @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev)
>   
>   	spin_lock_init(&tidss->wait_lock);
>   
> +	ret = tidss_oldi_init(tidss);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to init OLDI (%d)\n", ret);
> +		return ret;
> +	}
> +
>   	ret = dispc_init(tidss);
>   	if (ret) {
>   		dev_err(dev, "failed to initialize dispc: %d\n", ret);
> -		return ret;
> +		goto err_oldi_deinit;
>   	}
>   
>   	pm_runtime_enable(dev);
> @@ -202,6 +210,9 @@ static int tidss_probe(struct platform_device *pdev)
>   	pm_runtime_dont_use_autosuspend(dev);
>   	pm_runtime_disable(dev);
>   
> +err_oldi_deinit:
> +	tidss_oldi_deinit(tidss);

Shouldn't this be called tidss_remove()?

> +
>   	return ret;
>   }
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index d7f27b0b0315..4ccdc177d171 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -11,6 +11,7 @@
>   
>   #define TIDSS_MAX_PORTS 4
>   #define TIDSS_MAX_PLANES 4
> +#define TIDSS_MAX_OLDI_TXES 2
>   
>   typedef u32 dispc_irq_t;
>   
> @@ -27,6 +28,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];

You should probably have "struct tidss_oldi;" in this file.

> +
>   	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..fd96ca815542
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
> @@ -0,0 +1,568 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 - Texas Instruments Incorporated
> + *
> + * Aradhya Bhatia <a-bhati1@ti.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.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_panel.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include "tidss_drv.h"
> +#include "tidss_oldi.h"
> +
> +struct tidss_oldi {
> +	struct tidss_device	*tidss;
> +	struct device		*dev;
> +
> +	struct drm_bridge	bridge;
> +	struct drm_bridge	*next_bridge;
> +
> +	struct drm_panel *panel;
> +
> +	enum tidss_oldi_link_type link_type;
> +	const struct oldi_bus_format *bus_format;
> +	u32 oldi_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 },
> +};
> +
> +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%d Failure attach next bridge\n",
> +			__func__, oldi->oldi_instance);

Here and in other places: the oldi_instance is unsigned, so %u.

> +		return -ENODEV;
> +	}
> +
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +		dev_err(oldi->dev,
> +			"%s: OLDI%d 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
> +oldi_set_serial_clk(struct tidss_oldi *oldi, unsigned long rate)

This should be "tidss_oldi_set_serial_clk".

> +{
> +	unsigned long new_rate;
> +	int ret;
> +
> +	ret = clk_set_rate(oldi->s_clk, rate);
> +	if (ret) {
> +		dev_err(oldi->dev,
> +			"OLDI%d: 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%d Clock rate %lu differs over 5%% from requested %lu\n",
> +			 oldi->oldi_instance, new_rate, rate);
> +
> +	dev_dbg(oldi->dev, "OLDI%d: 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 power)
> +{
> +	u32 val = 0, mask;

Unnecessary initialization.

Isn't the val variable unnecessary? You could just:

regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, power ? 0 : mask);

Oh, hmm, wait... You sometimes turn off the oldis, even if power == true 
(the default cases)? I guess that's for the OLDI_MODE_UNSUPPORTED case. 
But why is tidss_oldi_tx_power() even called in that scenario?

Also, I think usually "power" would be named "enable".

> +
> +	if (WARN_ON(!oldi->io_ctrl))

if oldi->io_ctrl == NULL, hasn't the probe already failed and this can 
never be called? The WARN_ON(), and the whole if() feels unnecessary to me.

> +		return;
> +
> +	/*
> +	 * 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 (power) {
> +		val = 0;
> +		switch (oldi->link_type) {
> +		case OLDI_MODE_SINGLE_LINK:
> +			if (oldi->oldi_instance == OLDI(0))
> +				mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_BG;
> +			else if (oldi->oldi_instance == OLDI(1))
> +				mask = OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
> +
> +			break;
> +		case OLDI_MODE_CLONE_SINGLE_LINK:
> +		case OLDI_MODE_DUAL_LINK:
> +			mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
> +			break;
> +		default:
> +			if (oldi->oldi_instance == OLDI(0))
> +				mask = OLDI_PWRDN_TX(0);
> +			else if (oldi->oldi_instance == OLDI(1))
> +				mask = OLDI_PWRDN_TX(1);
> +
> +			val = mask;
> +			break;
> +		}
> +	} else {
> +		switch (oldi->link_type) {
> +		case OLDI_MODE_CLONE_SINGLE_LINK:
> +		case OLDI_MODE_DUAL_LINK:
> +			mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
> +			break;
> +		case OLDI_MODE_SINGLE_LINK:
> +		default:
> +			if (oldi->oldi_instance == OLDI(0))
> +				mask = OLDI_PWRDN_TX(0);
> +			else if (oldi->oldi_instance == OLDI(1))
> +				mask = OLDI_PWRDN_TX(1);
> +
> +			break;
> +		}
> +		val = mask;
> +	}
> +
> +	regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, val);
> +}
> +
> +static int tidss_oldi_config(struct tidss_oldi *oldi)
> +{
> +	const struct oldi_bus_format *bus_fmt = NULL;
> +	u32 oldi_reset_bit = BIT(5);
> +	int count = 0;
> +	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%d: 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%d: Unsupported mode.\n",
> +			oldi->oldi_instance);
> +		return -EINVAL;
> +	}
> +
> +	tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg);

If you expose this kind of function, I think you might as well move the 
below while loop into that function, and return an error code from 
tidss_configure_oldi. Then you don't need to expose tidss_get_status().

To handle the other call to tidss_configure_oldi() where you pass 0 as 
the oldi_cfg, you could either skip the while loop for oldi_cfg == 0 
(are you sure you don't need to wait in that case too?), or have a 
separate function in tidss for disabling oldi.

But looking at the AM62a TRM, this doesn't seem to follow the steps. It has:

4. Software checks that the PLLs are stable and locked.

5. Release OLDITX_SW_RST = 1 (by configuring DSS0_VP_DSS_OLDI_CFG[12] 
SOFTRST register bit).

But you set the OLDITX_SW_RST while configuring.

> +	while (!(oldi_reset_bit & tidss_get_status(oldi->tidss)) &&
> +	       count < 10000)
> +		count++;
> +
> +	if (!(oldi_reset_bit & tidss_get_status(oldi->tidss)))
> +		dev_warn(oldi->dev, "%s: OLDI%d timeout waiting OLDI reset done\n",
> +			 __func__, oldi->oldi_instance);
> +
> +	return 0;
> +}
> +
> +static void tidss_oldi_atomic_early_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);
> +
> +	/* Enable the OLDI serial clock (7 times the pixel clock) */

This doesn't enable the clk, just sets the rate.

> +	oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
> +
> +	/* Enable OLDI IO power */
> +	tidss_oldi_tx_power(oldi, true);
> +}
> +
> +static void tidss_oldi_atomic_late_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);
> +
> +	/* Disable OLDI clock by setting IDLE Frequency */

And this doesn't disable the clock...

What is the reason to change the freq at disable time anyway, if the 
power and OLDI are disabled?

> +	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 int tidss_oldi_atomic_check(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *bridge_state,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
> +{
> +	/*
> +	 * There might be flags negotiation supported in future but
> +	 * set the bus flags in atomic_check statically for now.
> +	 */
> +
> +	/* Not sure what this is required for, at the moment */

Do you mean that this is required (for OLDI to work) but you don't know 
why, or that this is not required?

> +	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> +
> +	return 0;
> +}
> +
> +static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
> +	.attach		= tidss_oldi_bridge_attach,
> +	.atomic_check	= tidss_oldi_atomic_check,
> +	.atomic_early_enable = tidss_oldi_atomic_early_enable,
> +	.atomic_late_disable = tidss_oldi_atomic_late_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)
> +{
> +	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) {
> +		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;
> +	}
> +
> +	/*
> +	 * 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 u32 get_parent_dss_vp(struct device_node *oldi_tx, u32 *parent_vp)

The return value should be int.

> +{
> +	struct device_node *ep, *dss_port;
> +	int ret = 0;

Unnecessary initialization.

> +
> +	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;
> +	struct drm_panel *panel;
> +	u32 parent_vp, oldi_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.
> +		 */
> +		ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
> +						  &panel, &bridge);
> +		if (ret) {
> +			/*
> +			 * 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.
> +			 */
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(tidss->dev,
> +					"no panel/bridge for OLDI%d. Error %d\n",
> +					oldi_instance, ret);
> +			goto err_put_node;
> +		}
> +
> +		if (panel) {
> +			bridge = devm_drm_panel_bridge_add(tidss->dev, panel);
> +			if (IS_ERR(bridge)) {
> +				ret = PTR_ERR(bridge);
> +				goto err_put_node;
> +			}
> +		}
> +
> +		link_type = get_oldi_mode(child);
> +		if (link_type == OLDI_MODE_UNSUPPORTED) {
> +			dev_err(tidss->dev,
> +				"OLDI%d: Unsupported OLDI connection.\n",
> +				oldi_instance);
> +			ret = -EINVAL;
> +			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->link_type = link_type;
> +		oldi->oldi_instance = oldi_instance;
> +		oldi->dev = tidss->dev;
> +		oldi->next_bridge = bridge;
> +		oldi->panel = panel;
> +
> +		oldi->io_ctrl = syscon_regmap_lookup_by_phandle(child,
> +								"ti,oldi-io-ctrl");
> +		if (IS_ERR(oldi->io_ctrl)) {
> +			dev_err(oldi->dev,
> +				"%s: oldi%d syscon_regmap_lookup_by_phandle failed %ld\n",
> +			       __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
> +			ret = PTR_ERR(oldi->io_ctrl);
> +			goto err_put_node;
> +		}
> +
> +		oldi->s_clk = of_clk_get_by_name(child, "s_clk");
> +		if (IS_ERR(oldi->s_clk)) {
> +			dev_err(oldi->dev,
> +				"%s: oldi%d Failed to get s_clk: %ld\n",
> +				__func__, oldi_instance, PTR_ERR(oldi->s_clk));
> +			ret = PTR_ERR(oldi->s_clk);
> +			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);
> +	}
> +
> +err_put_node:
> +	of_node_put(child);
> +	of_node_put(oldi_parent);
> +	return ret;

I usually have the non-error code path at the end of the main function 
code, with "return 0", and the error cases separately below. Reading the 
above, it makes me think that if we reach the end of the 
for_each_child_of_node block, we're in an error state as we pass the 
err_put_node...

> +}
> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h b/drivers/gpu/drm/tidss/tidss_oldi.h
> new file mode 100644
> index 000000000000..5ad02ddea11a
> --- /dev/null
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2023 - Texas Instruments Incorporated
> + *
> + * Aradhya Bhatia <a-bhati1@ti.com>
> + */
> +
> +#ifndef __TIDSS_OLDI_H__
> +#define __TIDSS_OLDI_H__
> +
> +#include <linux/media-bus-format.h>
> +
> +#include "tidss_drv.h"
> +#include "tidss_dispc.h"
> +
> +struct tidss_oldi;
> +
> +/* OLDI Instances */
> +#define OLDI(n)		n

This is an odd macro... At least the n should be inside parenthesis, 
but... Isn't a plain number just fine, why do you need a macro?

> +
> +/* OLDI PORTS */
> +#define OLDI_INPUT_PORT		0
> +#define OLDI_OURPUT_PORT	1
> +
> +/* OLDI Config Bits */
> +#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)

These are bits of the DISPC_VP_DSS_OLDI_CFG register, so it would be 
nicer to have these in the tidss_dispc_regs.h.

> +
> +/* Control MMR Register */
> +
> +/* Register offsets */
> +#define OLDI_PD_CTRL            0x100
> +#define OLDI_LB_CTRL            0x104
> +
> +/* Power control bits */
> +#define OLDI_PWRDN_TX(n)	BIT(n)
> +
> +/* LVDS Bandgap reference Enable/Disable */
> +#define OLDI_PWRDN_BG		BIT(8)
> +
> +#define OLDI_IDLE_CLK_HZ	25000000 /*25 MHz */
> +
> +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__ */

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

[Aradhya Bhatia]

Hi Tomi,

Thank you for reviewing the patches!

On 27/06/24 18:41, Tomi Valkeinen wrote:
> On 11/05/2024 22:30, Aradhya Bhatia wrote:
>> Up till now, the OLDI support in tidss was integrated within the tidss
>> dispc.
>> This was fine till the OLDI was one-to-mapped with the DSS video-port
>> (VP).
>> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds /
>> lvds-clone
>> modes.
>>
>> Add OLDI TXes as separate DRM bridge entities to better support the
>> new LVDS
>> configurations.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>> Note:
>>
>> The OLDI configuration should happen before the video-port
>> configuration takes
>> place in tidss_crtc_atomic_enable hook. I have posted a patch allowing
>> DRM
>> bridges to get enabled before the CRTC of that bridge is enabled[0].
>> This patch
>> uses the bridge hooks introduced in [0], and hence will not compile
>> without [0].
>>
>> [0]: Dependency Patch: Introduce early_enable / late_disable drm
>> bridge APIs
>> https://lore.kernel.org/all/20240511153051.1355825-7-a-bhatia1@ti.com/
>>
>> ---
>>   drivers/gpu/drm/tidss/Makefile      |   3 +-
>>   drivers/gpu/drm/tidss/tidss_dispc.c |  11 +-
>>   drivers/gpu/drm/tidss/tidss_dispc.h |   4 +
>>   drivers/gpu/drm/tidss/tidss_drv.c   |  13 +-
>>   drivers/gpu/drm/tidss/tidss_drv.h   |   4 +
>>   drivers/gpu/drm/tidss/tidss_oldi.c  | 568 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tidss/tidss_oldi.h  |  73 ++++
>>   7 files changed, 673 insertions(+), 3 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..4961da3989c0 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -466,6 +466,16 @@ static u32 dispc_vp_read(struct dispc_device
>> *dispc, u32 hw_videoport, u16 reg)
>>       return ioread32(base + reg);
>>   }
>>   +u32 tidss_get_status(struct tidss_device *tidss)
>> +{
>> +    return dispc_read(tidss->dispc, DSS_SYSSTATUS);
>> +}
>> +
>> +void tidss_configure_oldi(struct tidss_device *tidss, u32
>> hw_videoport, u32 val)
>> +{
>> +    return dispc_vp_write(tidss->dispc, hw_videoport,
>> DISPC_VP_DSS_OLDI_CFG, val);
>> +}
>> +
>>   /*
>>    * TRM gives bitfields as start:end, where start is the higher bit
>>    * number. For example 7:0
>> @@ -1310,7 +1320,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..800a73457aff 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;
>>   +u32 tidss_get_status(struct tidss_device *tidss);
>> +void tidss_configure_oldi(struct tidss_device *tidss, u32
>> hw_videoport, u32 val);
>> +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_drv.c
>> b/drivers/gpu/drm/tidss/tidss_drv.c
>> index d15f836dca95..fd90e8498cc2 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 */
>>   @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device
>> *pdev)
>>         spin_lock_init(&tidss->wait_lock);
>>   +    ret = tidss_oldi_init(tidss);
>> +    if (ret) {
>> +        if (ret != -EPROBE_DEFER)
>> +            dev_err(dev, "failed to init OLDI (%d)\n", ret);
>> +        return ret;
>> +    }
>> +
>>       ret = dispc_init(tidss);
>>       if (ret) {
>>           dev_err(dev, "failed to initialize dispc: %d\n", ret);
>> -        return ret;
>> +        goto err_oldi_deinit;
>>       }
>>         pm_runtime_enable(dev);
>> @@ -202,6 +210,9 @@ static int tidss_probe(struct platform_device *pdev)
>>       pm_runtime_dont_use_autosuspend(dev);
>>       pm_runtime_disable(dev);
>>   +err_oldi_deinit:
>> +    tidss_oldi_deinit(tidss);
> 
> Shouldn't this be called tidss_remove()?

Yes!

> 
>> +
>>       return ret;
>>   }
>>   diff --git a/drivers/gpu/drm/tidss/tidss_drv.h
>> b/drivers/gpu/drm/tidss/tidss_drv.h
>> index d7f27b0b0315..4ccdc177d171 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -11,6 +11,7 @@
>>     #define TIDSS_MAX_PORTS 4
>>   #define TIDSS_MAX_PLANES 4
>> +#define TIDSS_MAX_OLDI_TXES 2
>>     typedef u32 dispc_irq_t;
>>   @@ -27,6 +28,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];
> 
> You should probably have "struct tidss_oldi;" in this file.

Okay! I will add that.


> 
>> +
>>       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..fd96ca815542
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
>> @@ -0,0 +1,568 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023 - Texas Instruments Incorporated
>> + *
>> + * Aradhya Bhatia <a-bhati1@ti.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.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_panel.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_probe_helper.h>
>> +
>> +#include "tidss_drv.h"
>> +#include "tidss_oldi.h"
>> +
>> +struct tidss_oldi {
>> +    struct tidss_device    *tidss;
>> +    struct device        *dev;
>> +
>> +    struct drm_bridge    bridge;
>> +    struct drm_bridge    *next_bridge;
>> +
>> +    struct drm_panel *panel;
>> +
>> +    enum tidss_oldi_link_type link_type;
>> +    const struct oldi_bus_format *bus_format;
>> +    u32 oldi_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 },
>> +};
>> +
>> +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%d Failure attach next bridge\n",
>> +            __func__, oldi->oldi_instance);
> 
> Here and in other places: the oldi_instance is unsigned, so %u.

Okay!

> 
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>> +        dev_err(oldi->dev,
>> +            "%s: OLDI%d 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
>> +oldi_set_serial_clk(struct tidss_oldi *oldi, unsigned long rate)
> 
> This should be "tidss_oldi_set_serial_clk".

Okay!

> 
>> +{
>> +    unsigned long new_rate;
>> +    int ret;
>> +
>> +    ret = clk_set_rate(oldi->s_clk, rate);
>> +    if (ret) {
>> +        dev_err(oldi->dev,
>> +            "OLDI%d: 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%d Clock rate %lu differs over 5%% from requested
>> %lu\n",
>> +             oldi->oldi_instance, new_rate, rate);
>> +
>> +    dev_dbg(oldi->dev, "OLDI%d: 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 power)
>> +{
>> +    u32 val = 0, mask;
> 
> Unnecessary initialization.
> 
> Isn't the val variable unnecessary? You could just:
> 
> regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, power ? 0 : mask);
> 
> Oh, hmm, wait... You sometimes turn off the oldis, even if power == true
> (the default cases)? I guess that's for the OLDI_MODE_UNSUPPORTED case.
> But why is tidss_oldi_tx_power() even called in that scenario?
> 
> Also, I think usually "power" would be named "enable".

Ideally, it would never go in the default case. It's only supposed to be
a defensive programming.

I will add a comment mentioning the same. I am also planning to make
some changes around the OLDI selection logic for different OLDI modes,
which will help me drop the val variable entirely.

> 
>> +
>> +    if (WARN_ON(!oldi->io_ctrl))
> 
> if oldi->io_ctrl == NULL, hasn't the probe already failed and this can
> never be called? The WARN_ON(), and the whole if() feels unnecessary to me.

I will drop this.

> 
>> +        return;
>> +
>> +    /*
>> +     * 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 (power) {
>> +        val = 0;
>> +        switch (oldi->link_type) {
>> +        case OLDI_MODE_SINGLE_LINK:
>> +            if (oldi->oldi_instance == OLDI(0))
>> +                mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_BG;
>> +            else if (oldi->oldi_instance == OLDI(1))
>> +                mask = OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
>> +
>> +            break;
>> +        case OLDI_MODE_CLONE_SINGLE_LINK:
>> +        case OLDI_MODE_DUAL_LINK:
>> +            mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
>> +            break;
>> +        default:
>> +            if (oldi->oldi_instance == OLDI(0))
>> +                mask = OLDI_PWRDN_TX(0);
>> +            else if (oldi->oldi_instance == OLDI(1))
>> +                mask = OLDI_PWRDN_TX(1);
>> +
>> +            val = mask;
>> +            break;
>> +        }
>> +    } else {
>> +        switch (oldi->link_type) {
>> +        case OLDI_MODE_CLONE_SINGLE_LINK:
>> +        case OLDI_MODE_DUAL_LINK:
>> +            mask = OLDI_PWRDN_TX(0) | OLDI_PWRDN_TX(1) | OLDI_PWRDN_BG;
>> +            break;
>> +        case OLDI_MODE_SINGLE_LINK:
>> +        default:
>> +            if (oldi->oldi_instance == OLDI(0))
>> +                mask = OLDI_PWRDN_TX(0);
>> +            else if (oldi->oldi_instance == OLDI(1))
>> +                mask = OLDI_PWRDN_TX(1);
>> +
>> +            break;
>> +        }
>> +        val = mask;
>> +    }
>> +
>> +    regmap_update_bits(oldi->io_ctrl, OLDI_PD_CTRL, mask, val);
>> +}
>> +
>> +static int tidss_oldi_config(struct tidss_oldi *oldi)
>> +{
>> +    const struct oldi_bus_format *bus_fmt = NULL;
>> +    u32 oldi_reset_bit = BIT(5);
>> +    int count = 0;
>> +    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%d: 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%d: Unsupported mode.\n",
>> +            oldi->oldi_instance);
>> +        return -EINVAL;
>> +    }
>> +
>> +    tidss_configure_oldi(oldi->tidss, oldi->parent_vp, oldi_cfg);
> 
> If you expose this kind of function, I think you might as well move the
> below while loop into that function, and return an error code from
> tidss_configure_oldi. Then you don't need to expose tidss_get_status()>
> To handle the other call to tidss_configure_oldi() where you pass 0 as
> the oldi_cfg, you could either skip the while loop for oldi_cfg == 0
> (are you sure you don't need to wait in that case too?), or have a
> separate function in tidss for disabling oldi.

Yeah, we don't need to wait in the OLDI reset status when we are
disabling. Writing 0 to OLDI CFG during disable will assert the reset.
Since we are not de-asserting it until we enable, waiting is not required.

I will skip the wait when oldi_cfg == 0 as you suggested.


> 
> But looking at the AM62a TRM, this doesn't seem to follow the steps. It
> has:
> 
> 4. Software checks that the PLLs are stable and locked.
> 
> 5. Release OLDITX_SW_RST = 1 (by configuring DSS0_VP_DSS_OLDI_CFG[12]
> SOFTRST register bit).
> 
> But you set the OLDITX_SW_RST while configuring.

Hmm, I followed convention from the dispc_enable_oldi() function in
tidss_dispc.c.

I think the TRM there asks for bits to be written one by one, but
functionally it hasn't ever been an issue.

> 
>> +    while (!(oldi_reset_bit & tidss_get_status(oldi->tidss)) &&
>> +           count < 10000)
>> +        count++;
>> +
>> +    if (!(oldi_reset_bit & tidss_get_status(oldi->tidss)))
>> +        dev_warn(oldi->dev, "%s: OLDI%d timeout waiting OLDI reset
>> done\n",
>> +             __func__, oldi->oldi_instance);
>> +
>> +    return 0;
>> +}
>> +
>> +static void tidss_oldi_atomic_early_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);
>> +
>> +    /* Enable the OLDI serial clock (7 times the pixel clock) */
> 
> This doesn't enable the clk, just sets the rate.

Okay!

> 
>> +    oldi_set_serial_clk(oldi, mode->clock * 7 * 1000);
>> +
>> +    /* Enable OLDI IO power */
>> +    tidss_oldi_tx_power(oldi, true);
>> +}
>> +
>> +static void tidss_oldi_atomic_late_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);
>> +
>> +    /* Disable OLDI clock by setting IDLE Frequency */
> 
> And this doesn't disable the clock...

Okay!

> 
> What is the reason to change the freq at disable time anyway, if the
> power and OLDI are disabled?

There is a clock-divider is present between the serial PLL, and the DSS
VP clock input. When the PLL generates the serial frequency (s_clk)
(s_clk = 7x pixel frequency), the clock-divider divides the frequency by
7, thereby ensuring that the DSS VP clock input gets s_clk / 7 value.

((s_clk / 7) => ((7x pixel freq / 7) => (pixel freq))

Once the OLDI is disabled, the by-7 clock divider also gets disabled.
This puts the clock divider in the by-pass mode.

The DSS VP then directly receives serial clock from the PLL (instead
of the 1/7th serial clock).

The HW design team has mentioned that receving such a high frequency
clock can be damaging to the DSS hardware. Hence we drop it down to
lower levels before disbaling OLDI.

> 
>> +    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 int tidss_oldi_atomic_check(struct drm_bridge *bridge,
>> +                   struct drm_bridge_state *bridge_state,
>> +                   struct drm_crtc_state *crtc_state,
>> +                   struct drm_connector_state *conn_state)
>> +{
>> +    /*
>> +     * There might be flags negotiation supported in future but
>> +     * set the bus flags in atomic_check statically for now.
>> +     */
>> +
>> +    /* Not sure what this is required for, at the moment */
> 
> Do you mean that this is required (for OLDI to work) but you don't know
> why, or that this is not required?

That is a stray comment that got in. Since DSS is doing most checks, we
don't need an atomic check yet.

I will drop the atomic_check part from tidss oldi completely.

> 
>> +    bridge_state->input_bus_cfg.flags =
>> bridge->timings->input_bus_flags;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
>> +    .attach        = tidss_oldi_bridge_attach,
>> +    .atomic_check    = tidss_oldi_atomic_check,
>> +    .atomic_early_enable = tidss_oldi_atomic_early_enable,
>> +    .atomic_late_disable = tidss_oldi_atomic_late_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)
>> +{
>> +    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) {
>> +        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;
>> +    }
>> +
>> +    /*
>> +     * 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 u32 get_parent_dss_vp(struct device_node *oldi_tx, u32
>> *parent_vp)
> 
> The return value should be int.

Okay!

> 
>> +{
>> +    struct device_node *ep, *dss_port;
>> +    int ret = 0;
> 
> Unnecessary initialization.

Okay!

> 
>> +
>> +    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;
>> +    struct drm_panel *panel;
>> +    u32 parent_vp, oldi_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.
>> +         */
>> +        ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1,
>> +                          &panel, &bridge);
>> +        if (ret) {
>> +            /*
>> +             * 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.
>> +             */
>> +            if (ret != -EPROBE_DEFER)
>> +                dev_err(tidss->dev,
>> +                    "no panel/bridge for OLDI%d. Error %d\n",
>> +                    oldi_instance, ret);
>> +            goto err_put_node;
>> +        }
>> +
>> +        if (panel) {
>> +            bridge = devm_drm_panel_bridge_add(tidss->dev, panel);
>> +            if (IS_ERR(bridge)) {
>> +                ret = PTR_ERR(bridge);
>> +                goto err_put_node;
>> +            }
>> +        }
>> +
>> +        link_type = get_oldi_mode(child);
>> +        if (link_type == OLDI_MODE_UNSUPPORTED) {
>> +            dev_err(tidss->dev,
>> +                "OLDI%d: Unsupported OLDI connection.\n",
>> +                oldi_instance);
>> +            ret = -EINVAL;
>> +            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->link_type = link_type;
>> +        oldi->oldi_instance = oldi_instance;
>> +        oldi->dev = tidss->dev;
>> +        oldi->next_bridge = bridge;
>> +        oldi->panel = panel;
>> +
>> +        oldi->io_ctrl = syscon_regmap_lookup_by_phandle(child,
>> +                                "ti,oldi-io-ctrl");
>> +        if (IS_ERR(oldi->io_ctrl)) {
>> +            dev_err(oldi->dev,
>> +                "%s: oldi%d syscon_regmap_lookup_by_phandle failed
>> %ld\n",
>> +                   __func__, oldi_instance, PTR_ERR(oldi->io_ctrl));
>> +            ret = PTR_ERR(oldi->io_ctrl);
>> +            goto err_put_node;
>> +        }
>> +
>> +        oldi->s_clk = of_clk_get_by_name(child, "s_clk");
>> +        if (IS_ERR(oldi->s_clk)) {
>> +            dev_err(oldi->dev,
>> +                "%s: oldi%d Failed to get s_clk: %ld\n",
>> +                __func__, oldi_instance, PTR_ERR(oldi->s_clk));
>> +            ret = PTR_ERR(oldi->s_clk);
>> +            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);
>> +    }
>> +
>> +err_put_node:
>> +    of_node_put(child);
>> +    of_node_put(oldi_parent);
>> +    return ret;
> 
> I usually have the non-error code path at the end of the main function
> code, with "return 0", and the error cases separately below. Reading the
> above, it makes me think that if we reach the end of the
> for_each_child_of_node block, we're in an error state as we pass the
> err_put_node...
> 

Alright! I will put the nodes and return 0 in case everything is
successful.

>> +}
>> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h
>> b/drivers/gpu/drm/tidss/tidss_oldi.h
>> new file mode 100644
>> index 000000000000..5ad02ddea11a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h
>> @@ -0,0 +1,73 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2023 - Texas Instruments Incorporated
>> + *
>> + * Aradhya Bhatia <a-bhati1@ti.com>
>> + */
>> +
>> +#ifndef __TIDSS_OLDI_H__
>> +#define __TIDSS_OLDI_H__
>> +
>> +#include <linux/media-bus-format.h>
>> +
>> +#include "tidss_drv.h"
>> +#include "tidss_dispc.h"
>> +
>> +struct tidss_oldi;
>> +
>> +/* OLDI Instances */
>> +#define OLDI(n)        n
> 
> This is an odd macro... At least the n should be inside parenthesis,
> but... Isn't a plain number just fine, why do you need a macro?

Just for better readability.

(oldi_instance == 0) OR (oldi_instance == 1) might have confused people.

However, the only place I needed to check oldi_instance values was TX
power enable/disable. I have decided to refactor the logic there
slightly and that won't require the checks anymore. I can drop this
macro now.

> 
>> +
>> +/* OLDI PORTS */
>> +#define OLDI_INPUT_PORT        0
>> +#define OLDI_OURPUT_PORT    1
>> +
>> +/* OLDI Config Bits */
>> +#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)
> 
> These are bits of the DISPC_VP_DSS_OLDI_CFG register, so it would be
> nicer to have these in the tidss_dispc_regs.h.

Okay!

> 
>> +
>> +/* Control MMR Register */
>> +
>> +/* Register offsets */
>> +#define OLDI_PD_CTRL            0x100
>> +#define OLDI_LB_CTRL            0x104
>> +
>> +/* Power control bits */
>> +#define OLDI_PWRDN_TX(n)    BIT(n)
>> +
>> +/* LVDS Bandgap reference Enable/Disable */
>> +#define OLDI_PWRDN_BG        BIT(8)
>> +
>> +#define OLDI_IDLE_CLK_HZ    25000000 /*25 MHz */
>> +
>> +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__ */
> 


Regards
Aradhya