sFrom b69208b75f7ae8e223c81783afb04fecd2f5faf8 Mon Sep 17 00:00:00 2001

sFrom b69208b75f7ae8e223c81783afb04fecd2f5faf8 Mon Sep 17 00:00:00 2001

[Aradhya Bhatia]

This patch series adds the required support for enabling OLDI display on
boards having SoCs with am625 compatible Display SubSystem.

The 2 OLDI TXes on am625-dss allow a 3 different types of panel
connections with the board.

1. Single Link / Single Mode on OLDI TX 0 OR 1.
2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
3. Dual Link / Single Mode on OLDI TX 0 and 1.

The 3rd combination allows the DSS to be connected to LVDS panels of
resolutions upto 2K.

This patch series further adds support for boards, with HDMI bridges
supporting only 24bit RGB color space, to be able to explicitly output
16bit RGB data when only 16 DPI output pins have been connected from SoC
to bridge.

Note:
These patches require the new compatible "ti,am625-dss" and its support
in tidss driver. Those patches have been previously posted and can be
found on the following link:

https://patchwork.kernel.org/project/dri-devel/list/?series=654214&archive=both


Aradhya Bhatia (8):
  dt-bindings: display: ti,am65x-dss: Add port properties for DSS
  dt-bindings: display: ti,am65x-dss: Add IO CTRL property for AM625
    OLDI
  drm/tidss: Add support for DSS port properties
  drm/tidss: Add support for Dual Link LVDS Bus Format
  drm/tidss: dt property to force 16bit VP output to a 24bit bridge
  drm/tidss: Add IO CTRL and Power support for OLDI TX in am625
  drm/tidss: Fix clock request value for OLDI videoports
  drm/tidss: Enable Dual and Duplicate Modes for OLDI

 .../bindings/display/ti/ti,am65x-dss.yaml     |  46 +++-
 drivers/gpu/drm/tidss/tidss_dispc.c           | 220 +++++++++++++++---
 drivers/gpu/drm/tidss/tidss_dispc.h           |   8 +
 drivers/gpu/drm/tidss/tidss_dispc_regs.h      |   6 +
 4 files changed, 246 insertions(+), 34 deletions(-)

-- 
2.37.0

[PATCH 1/8] dt-bindings: display: ti, am65x-dss: Add port properties for DSS

[Aradhya Bhatia]

Add "ti,oldi-mode" property to indicate the tidss driver the OLDI output
mode. The 2 OLDI TXes on am625-dss allow a 3 different types of panel
connections with the board.

1. Single Link / Single Mode on OLDI TX 0 OR 1.
2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
3. Dual Link / Single Mode on OLDI TX 0 and 1.

Add "ti,rgb565-to-888" property to override 16bit output from a videoport
for a bridge that only accepts 24bit RGB888 DPI input.

On some boards the HDMI bridge takes a 24bit DPI input, but only 16 data
pins are actually enabled from the SoC.  This new property forces the
output to be RGB565 on a specific video port if the bridge requests a
24bit RGB color space.

This assumes that the video port is connected like so:

SoC : Bridge
R0 ->   R3
R1 ->   R4
R2 ->   R5
R3 ->   R6
R4 ->   R7
G0 ->   G2
G1 ->   G3
G2 ->   G4
G3 ->   G5
G4 ->   G6
G5 ->   G7
B0 ->   B3
B1 ->   B4
B2 ->   B5
B3 ->   B6
B4 ->   B7

On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
The bridge sees 24bits of data,  but the lsb's are always zero.

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

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 6bbce921479d..11d9b3821409 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -80,15 +80,35 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
         description:
           The DSS OLDI output port node form video port 1
 
+        properties:
+          ti,oldi-mode:
+            description: TI specific property to indicate the mode the OLDI TXes
+              and the display panel are connected in.
+              0 -> OLDI TXes OFF (driver default for am625-dss)
+              1 -> Single link, Single Mode (OLDI0) (driver default for am65x-dss)
+              2 -> Single link, Single Mode (OLDI1)
+              3 -> Single link, Duplicate Mode
+              4 -> Dual link (Only Single Mode)
+            $ref: /schemas/types.yaml#/definitions/uint32
+            enum: [0, 1, 2, 3, 4]
+
       port@1:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
         description:
           The DSS DPI output port node from video port 2
 
+        properties:
+          ti,rgb565-to-888:
+            description:
+              property to override DPI output to 16bit for 24bit bridge
+            type: boolean
+
   ti,am65x-oldi-io-ctrl:
     $ref: "/schemas/types.yaml#/definitions/phandle"
     description:
@@ -144,6 +164,7 @@ examples:
                     #size-cells = <0>;
                     port@0 {
                             reg = <0>;
+                            ti,oldi-mode = <1>;
                             oldi_out0: endpoint {
                                     remote-endpoint = <&lcd_in0>;
                             };
-- 
2.37.0

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Rob Herring]

On Tue, Jul 19, 2022 at 01:38:38PM +0530, Aradhya Bhatia wrote:
> Add "ti,oldi-mode" property to indicate the tidss driver the OLDI output
> mode. The 2 OLDI TXes on am625-dss allow a 3 different types of panel
> connections with the board.
> 
> 1. Single Link / Single Mode on OLDI TX 0 OR 1.
> 2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
> 3. Dual Link / Single Mode on OLDI TX 0 and 1.
> 
> Add "ti,rgb565-to-888" property to override 16bit output from a videoport
> for a bridge that only accepts 24bit RGB888 DPI input.
> 
> On some boards the HDMI bridge takes a 24bit DPI input, but only 16 data
> pins are actually enabled from the SoC.  This new property forces the
> output to be RGB565 on a specific video port if the bridge requests a
> 24bit RGB color space.
> 
> This assumes that the video port is connected like so:
> 
> SoC : Bridge
> R0 ->   R3
> R1 ->   R4
> R2 ->   R5
> R3 ->   R6
> R4 ->   R7
> G0 ->   G2
> G1 ->   G3
> G2 ->   G4
> G3 ->   G5
> G4 ->   G6
> G5 ->   G7
> B0 ->   B3
> B1 ->   B4
> B2 ->   B5
> B3 ->   B6
> B4 ->   B7
> 
> On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
> The bridge sees 24bits of data,  but the lsb's are always zero.

Unless the bridge ignores the LSBs, that's not the right way to do 16 to 
24 bit. The LSBs should be connected to the MSB of the color component 
to get full color range.

> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 6bbce921479d..11d9b3821409 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -80,15 +80,35 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
>          description:
>            The DSS OLDI output port node form video port 1
>  
> +        properties:
> +          ti,oldi-mode:
> +            description: TI specific property to indicate the mode the OLDI TXes
> +              and the display panel are connected in.
> +              0 -> OLDI TXes OFF (driver default for am625-dss)
> +              1 -> Single link, Single Mode (OLDI0) (driver default for am65x-dss)
> +              2 -> Single link, Single Mode (OLDI1)
> +              3 -> Single link, Duplicate Mode
> +              4 -> Dual link (Only Single Mode)
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3, 4]

Wouldn't 'data-lanes' property work for this purpose.

Generally, we don't put properties in port nodes.

> +
>        port@1:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
>          description:
>            The DSS DPI output port node from video port 2
>  
> +        properties:
> +          ti,rgb565-to-888:
> +            description:
> +              property to override DPI output to 16bit for 24bit bridge
> +            type: boolean

There's work underway for standard way to handle interface formats[1]. 
Please help/comment on that to make sure it works for you. 

Rob

[1] https://lore.kernel.org/all/20220628181838.2031-3-max.oss.09@gmail.com/

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Nishanth Menon]

On 17:28-20220720, Rob Herring wrote:
> > On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
> > The bridge sees 24bits of data,  but the lsb's are always zero.
> 
> Unless the bridge ignores the LSBs, that's not the right way to do 16 to 
> 24 bit. The LSBs should be connected to the MSB of the color component 
> to get full color range.

I unfortunately cannot point specifics without violating NDAs, so
will just give a broad perspective.

Correct, this is not ideal, but in certain scenarios with limited
pins (due to iovoltage groups), we are indeed starting to see this
kind of usage model starting to pop up. Tradeoff is in a limit on
image quality, but that tends to be acceptable in certain lower cost
solutions.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Tomi Valkeinen]

On 22/07/2022 19:16, Nishanth Menon wrote:
> On 17:28-20220720, Rob Herring wrote:
>>> On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
>>> The bridge sees 24bits of data,  but the lsb's are always zero.
>>
>> Unless the bridge ignores the LSBs, that's not the right way to do 16 to
>> 24 bit. The LSBs should be connected to the MSB of the color component
>> to get full color range.
> 
> I unfortunately cannot point specifics without violating NDAs, so
> will just give a broad perspective.
> 
> Correct, this is not ideal, but in certain scenarios with limited
> pins (due to iovoltage groups), we are indeed starting to see this
> kind of usage model starting to pop up. Tradeoff is in a limit on
> image quality, but that tends to be acceptable in certain lower cost
> solutions.

It doesn't require more pins. If the lowest bits are tied to ground the 
image is always a bit darker than it should, and you do not get the full 
brightness. But if you wire e.g. the red component:

SoC : Bridge
R2 ->   R0
R3 ->   R1
R4 ->   R2
R0 ->   R3
R1 ->   R4
R2 ->   R5
R3 ->   R6
R4 ->   R7

or

R4 ->   R0
R4 ->   R1
R4 ->   R2
R0 ->   R3
R1 ->   R4
R2 ->   R5
R3 ->   R6
R4 ->   R7

You'll get the full range.

  Tomi

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Aradhya Bhatia]

On 21-Jul-22 04:58, Rob Herring wrote:
> On Tue, Jul 19, 2022 at 01:38:38PM +0530, Aradhya Bhatia wrote:
>> Add "ti,oldi-mode" property to indicate the tidss driver the OLDI output
>> mode. The 2 OLDI TXes on am625-dss allow a 3 different types of panel
>> connections with the board.
>>
>> 1. Single Link / Single Mode on OLDI TX 0 OR 1.
>> 2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
>> 3. Dual Link / Single Mode on OLDI TX 0 and 1.
>>
>> Add "ti,rgb565-to-888" property to override 16bit output from a videoport
>> for a bridge that only accepts 24bit RGB888 DPI input.
>>
>> On some boards the HDMI bridge takes a 24bit DPI input, but only 16 data
>> pins are actually enabled from the SoC.  This new property forces the
>> output to be RGB565 on a specific video port if the bridge requests a
>> 24bit RGB color space.
>>
>> This assumes that the video port is connected like so:
>>
>> SoC : Bridge
>> R0 ->   R3
>> R1 ->   R4
>> R2 ->   R5
>> R3 ->   R6
>> R4 ->   R7
>> G0 ->   G2
>> G1 ->   G3
>> G2 ->   G4
>> G3 ->   G5
>> G4 ->   G6
>> G5 ->   G7
>> B0 ->   B3
>> B1 ->   B4
>> B2 ->   B5
>> B3 ->   B6
>> B4 ->   B7
>>
>> On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
>> The bridge sees 24bits of data,  but the lsb's are always zero.
> 
> Unless the bridge ignores the LSBs, that's not the right way to do 16 to
> 24 bit. The LSBs should be connected to the MSB of the color component
> to get full color range.
> 
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++--
>>   1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 6bbce921479d..11d9b3821409 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -80,15 +80,35 @@ properties:
>>   
>>       properties:
>>         port@0:
>> -        $ref: /schemas/graph.yaml#/properties/port
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>>           description:
>>             The DSS OLDI output port node form video port 1
>>   
>> +        properties:
>> +          ti,oldi-mode:
>> +            description: TI specific property to indicate the mode the OLDI TXes
>> +              and the display panel are connected in.
>> +              0 -> OLDI TXes OFF (driver default for am625-dss)
>> +              1 -> Single link, Single Mode (OLDI0) (driver default for am65x-dss)
>> +              2 -> Single link, Single Mode (OLDI1)
>> +              3 -> Single link, Duplicate Mode
>> +              4 -> Dual link (Only Single Mode)
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            enum: [0, 1, 2, 3, 4]
> 
> Wouldn't 'data-lanes' property work for this purpose.
> 
> Generally, we don't put properties in port nodes.
> 
Thank you for the suggestions Rob!

I looked into the "data-lanes" property and it seems that the property
alone would not be able to help distinguish between the "Single link,
Duplicate mode" (Mode 3) and "Dual link, Single mode" (Mode 4). For both
the cases, the property will look like "data-lanes = <0 1>;" in the DT
node.

I have an idea on what the driver could use along with the data-lanes
property to ascertain the OLDI mode.

By means of number of remote-endpoints in DTS.
The OLDI output port of DSS can be made to have 2 remote endpoints when
2 panels are connected as "Single link, Duplicate Mode" vs only 1 remote
endpoint for "Dual Link, Single Mode". Based on the count, the driver
can distinguish between the two when both the data-lanes are activated
in DT node.

Let me know if you think this method would be appropriate.
>> +
>>         port@1:
>> -        $ref: /schemas/graph.yaml#/properties/port
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>>           description:
>>             The DSS DPI output port node from video port 2
>>   
>> +        properties:
>> +          ti,rgb565-to-888:
>> +            description:
>> +              property to override DPI output to 16bit for 24bit bridge
>> +            type: boolean
> 
> There's work underway for standard way to handle interface formats[1].
> Please help/comment on that to make sure it works for you.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20220628181838.2031-3-max.oss.09@gmail.com/

I also followed what this patch series is implementing. This seems to be
applicable for cases where the DPI pins are drawn and forwarded towards
a simple panel capable of accepting the raw parallel data.

It does not cover for the bridges with lesser number of formats to
support.


Regards
Aradhya

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Francesco Dolcini]

On Mon, Jul 25, 2022 at 04:56:15PM +0530, Aradhya Bhatia wrote:
> On 21-Jul-22 04:58, Rob Herring wrote:
> > On Tue, Jul 19, 2022 at 01:38:38PM +0530, Aradhya Bhatia wrote:
> > > +
> > >         port@1:
> > > -        $ref: /schemas/graph.yaml#/properties/port
> > > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > > +        unevaluatedProperties: false
> > >           description:
> > >             The DSS DPI output port node from video port 2
> > > +        properties:
> > > +          ti,rgb565-to-888:
> > > +            description:
> > > +              property to override DPI output to 16bit for 24bit bridge
> > > +            type: boolean
> > 
> > There's work underway for standard way to handle interface formats[1].
> > Please help/comment on that to make sure it works for you.
> > 
> > Rob
> > 
> > [1] https://lore.kernel.org/all/20220628181838.2031-3-max.oss.09@gmail.com/
> 
> I also followed what this patch series is implementing. This seems to be
> applicable for cases where the DPI pins are drawn and forwarded towards
> a simple panel capable of accepting the raw parallel data.

It would be great to have that patch move forward and eventually get it
merged, only Rob commented weeks ago.

Francesco

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Rob Herring]

On Mon, Jul 25, 2022 at 04:56:15PM +0530, Aradhya Bhatia wrote:
> 
> 
> On 21-Jul-22 04:58, Rob Herring wrote:
> > On Tue, Jul 19, 2022 at 01:38:38PM +0530, Aradhya Bhatia wrote:
> > > Add "ti,oldi-mode" property to indicate the tidss driver the OLDI output
> > > mode. The 2 OLDI TXes on am625-dss allow a 3 different types of panel
> > > connections with the board.
> > > 
> > > 1. Single Link / Single Mode on OLDI TX 0 OR 1.
> > > 2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
> > > 3. Dual Link / Single Mode on OLDI TX 0 and 1.
> > > 
> > > Add "ti,rgb565-to-888" property to override 16bit output from a videoport
> > > for a bridge that only accepts 24bit RGB888 DPI input.
> > > 
> > > On some boards the HDMI bridge takes a 24bit DPI input, but only 16 data
> > > pins are actually enabled from the SoC.  This new property forces the
> > > output to be RGB565 on a specific video port if the bridge requests a
> > > 24bit RGB color space.
> > > 
> > > This assumes that the video port is connected like so:
> > > 
> > > SoC : Bridge
> > > R0 ->   R3
> > > R1 ->   R4
> > > R2 ->   R5
> > > R3 ->   R6
> > > R4 ->   R7
> > > G0 ->   G2
> > > G1 ->   G3
> > > G2 ->   G4
> > > G3 ->   G5
> > > G4 ->   G6
> > > G5 ->   G7
> > > B0 ->   B3
> > > B1 ->   B4
> > > B2 ->   B5
> > > B3 ->   B6
> > > B4 ->   B7
> > > 
> > > On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
> > > The bridge sees 24bits of data,  but the lsb's are always zero.
> > 
> > Unless the bridge ignores the LSBs, that's not the right way to do 16 to
> > 24 bit. The LSBs should be connected to the MSB of the color component
> > to get full color range.
> > 
> > > 
> > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> > > ---
> > >   .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++--
> > >   1 file changed, 23 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> > > index 6bbce921479d..11d9b3821409 100644
> > > --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> > > +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> > > @@ -80,15 +80,35 @@ properties:
> > >       properties:
> > >         port@0:
> > > -        $ref: /schemas/graph.yaml#/properties/port
> > > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > > +        unevaluatedProperties: false
> > >           description:
> > >             The DSS OLDI output port node form video port 1
> > > +        properties:
> > > +          ti,oldi-mode:
> > > +            description: TI specific property to indicate the mode the OLDI TXes
> > > +              and the display panel are connected in.
> > > +              0 -> OLDI TXes OFF (driver default for am625-dss)
> > > +              1 -> Single link, Single Mode (OLDI0) (driver default for am65x-dss)
> > > +              2 -> Single link, Single Mode (OLDI1)
> > > +              3 -> Single link, Duplicate Mode
> > > +              4 -> Dual link (Only Single Mode)
> > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > > +            enum: [0, 1, 2, 3, 4]
> > 
> > Wouldn't 'data-lanes' property work for this purpose.
> > 
> > Generally, we don't put properties in port nodes.
> > 
> Thank you for the suggestions Rob!
> 
> I looked into the "data-lanes" property and it seems that the property
> alone would not be able to help distinguish between the "Single link,
> Duplicate mode" (Mode 3) and "Dual link, Single mode" (Mode 4). For both
> the cases, the property will look like "data-lanes = <0 1>;" in the DT
> node.
> 
> I have an idea on what the driver could use along with the data-lanes
> property to ascertain the OLDI mode.
> 
> By means of number of remote-endpoints in DTS.
> The OLDI output port of DSS can be made to have 2 remote endpoints when
> 2 panels are connected as "Single link, Duplicate Mode" vs only 1 remote
> endpoint for "Dual Link, Single Mode". Based on the count, the driver
> can distinguish between the two when both the data-lanes are activated
> in DT node.

You can only have 1 'remote-endpoint'. However, you can have multiple 
endpoint nodes which is generally used for fanout (output) or muxed 
(input) cases. Your case is fanout as it is the same data sent to 
multiple connections.

data-lanes would be kind of redundant in that case since it would be 1 
lane per endpoint.

> 
> Let me know if you think this method would be appropriate.
> > > +
> > >         port@1:
> > > -        $ref: /schemas/graph.yaml#/properties/port
> > > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > > +        unevaluatedProperties: false
> > >           description:
> > >             The DSS DPI output port node from video port 2
> > > +        properties:
> > > +          ti,rgb565-to-888:
> > > +            description:
> > > +              property to override DPI output to 16bit for 24bit bridge
> > > +            type: boolean
> > 
> > There's work underway for standard way to handle interface formats[1].
> > Please help/comment on that to make sure it works for you.
> > 
> > Rob
> > 
> > [1] https://lore.kernel.org/all/20220628181838.2031-3-max.oss.09@gmail.com/
> 
> I also followed what this patch series is implementing. This seems to be
> applicable for cases where the DPI pins are drawn and forwarded towards
> a simple panel capable of accepting the raw parallel data.
> 
> It does not cover for the bridges with lesser number of formats to
> support.

Not sure what you mean here, but raise it on that thread.

Rob

Re: [PATCH 1/8] dt-bindings: display: ti,am65x-dss: Add port properties for DSS

[Tomi Valkeinen]

On 19/07/2022 11:08, Aradhya Bhatia wrote:
> Add "ti,oldi-mode" property to indicate the tidss driver the OLDI output
> mode. The 2 OLDI TXes on am625-dss allow a 3 different types of panel
> connections with the board.
> 
> 1. Single Link / Single Mode on OLDI TX 0 OR 1.
> 2. Single Link / Duplicate Mode on OLDI TX 0 and 1.
> 3. Dual Link / Single Mode on OLDI TX 0 and 1.
> 
> Add "ti,rgb565-to-888" property to override 16bit output from a videoport
> for a bridge that only accepts 24bit RGB888 DPI input.
> 
> On some boards the HDMI bridge takes a 24bit DPI input, but only 16 data
> pins are actually enabled from the SoC.  This new property forces the
> output to be RGB565 on a specific video port if the bridge requests a
> 24bit RGB color space.
> 
> This assumes that the video port is connected like so:
> 
> SoC : Bridge
> R0 ->   R3
> R1 ->   R4
> R2 ->   R5
> R3 ->   R6
> R4 ->   R7
> G0 ->   G2
> G1 ->   G3
> G2 ->   G4
> G3 ->   G5
> G4 ->   G6
> G5 ->   G7
> B0 ->   B3
> B1 ->   B4
> B2 ->   B5
> B3 ->   B6
> B4 ->   B7
> 
> On the bridge side R0->R2, G0->G1, B0->B2 would be tied to ground.
> The bridge sees 24bits of data,  but the lsb's are always zero.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   .../bindings/display/ti/ti,am65x-dss.yaml     | 25 +++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 6bbce921479d..11d9b3821409 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -80,15 +80,35 @@ properties:
>   
>       properties:
>         port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
>           description:
>             The DSS OLDI output port node form video port 1
>   
> +        properties:
> +          ti,oldi-mode:
> +            description: TI specific property to indicate the mode the OLDI TXes
> +              and the display panel are connected in.
> +              0 -> OLDI TXes OFF (driver default for am625-dss)
> +              1 -> Single link, Single Mode (OLDI0) (driver default for am65x-dss)
> +              2 -> Single link, Single Mode (OLDI1)
> +              3 -> Single link, Duplicate Mode
> +              4 -> Dual link (Only Single Mode)
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3, 4]
> +
>         port@1:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
>           description:
>             The DSS DPI output port node from video port 2
>   
> +        properties:
> +          ti,rgb565-to-888:
> +            description:
> +              property to override DPI output to 16bit for 24bit bridge
> +            type: boolean

So you have a board with 16 DSS pins connected, going to a bridge/panel 
that only supports 24 bit input. I don't think there's anything TI 
specific here, sounds like a common situation.

"rgb565-to-888" sounds like there's some kind of conversion happening, 
but all this does is set the videoport width to 16 bits.

I'm not sure if there's a better solution but in the OMAP DSS we have 
"data-lines" property for an endpoint, which tells the driver the width 
of the bus.

  Tomi

[PATCH 2/8] dt-bindings: display: ti, am65x-dss: Add IO CTRL property for AM625 OLDI

[Aradhya Bhatia]

Add am625-io-ctrl dt property to provide access to the control MMR
registers for the OLDI TXes.

These registers are used to control the power input to the OLDI TXes as
well as to configure them in the Loopback test mode.

The MMR IO controller device has been updated since the AM65x SoC and
hence a newer property is needed to describe the one in AM625 SoC.

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

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 11d9b3821409..672765ad1f30 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -118,12 +118,33 @@ properties:
       and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
       interface to work.
 
+  ti,am625-oldi-io-ctrl:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      phandle to syscon device node mapping OLDI IO_CTRL registers, for
+      AM625 SoC. The mapped range should point to OLDI0_DAT0_IO_CTRL,
+      and map the registers up till OLDI_LB_CTRL. This property allows
+      the driver to control the power delivery to the OLDI TXes and
+      their loopback control as well.
+
   max-memory-bandwidth:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
       Input memory (from main memory to dispc) bandwidth limit in
       bytes per second
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: ti,am65x-dss
+then:
+  properties:
+    ti,am625-oldi-io-ctrl: false
+else:
+  properties:
+    ti,am65x-oldi-io-ctrl: false
+
 required:
   - compatible
   - reg
-- 
2.37.0

Re: [PATCH 2/8] dt-bindings: display: ti,am65x-dss: Add IO CTRL property for AM625 OLDI

[Rob Herring]

On Tue, Jul 19, 2022 at 01:38:39PM +0530, Aradhya Bhatia wrote:
> Add am625-io-ctrl dt property to provide access to the control MMR
> registers for the OLDI TXes.
> 
> These registers are used to control the power input to the OLDI TXes as
> well as to configure them in the Loopback test mode.
> 
> The MMR IO controller device has been updated since the AM65x SoC and
> hence a newer property is needed to describe the one in AM625 SoC.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 11d9b3821409..672765ad1f30 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -118,12 +118,33 @@ properties:
>        and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
>        interface to work.
>  
> +  ti,am625-oldi-io-ctrl:
> +    $ref: "/schemas/types.yaml#/definitions/phandle"
> +    description:
> +      phandle to syscon device node mapping OLDI IO_CTRL registers, for
> +      AM625 SoC. The mapped range should point to OLDI0_DAT0_IO_CTRL,
> +      and map the registers up till OLDI_LB_CTRL. This property allows
> +      the driver to control the power delivery to the OLDI TXes and
> +      their loopback control as well.

What's wrong with the existing ti,am65x-oldi-io-ctrl other than the less 
than ideal naming? And you just continued with the same issue so the 
next part will need yet another property. Sorry, no. Just use the 
existing property.

> +
>    max-memory-bandwidth:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
>        Input memory (from main memory to dispc) bandwidth limit in
>        bytes per second
>  
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        const: ti,am65x-dss
> +then:
> +  properties:
> +    ti,am625-oldi-io-ctrl: false
> +else:
> +  properties:
> +    ti,am65x-oldi-io-ctrl: false
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.37.0
> 
> 

Re: [PATCH 2/8] dt-bindings: display: ti,am65x-dss: Add IO CTRL property for AM625 OLDI

[Aradhya Bhatia]

On 21-Jul-22 05:02, Rob Herring wrote:
> On Tue, Jul 19, 2022 at 01:38:39PM +0530, Aradhya Bhatia wrote:
>> Add am625-io-ctrl dt property to provide access to the control MMR
>> registers for the OLDI TXes.
>>
>> These registers are used to control the power input to the OLDI TXes as
>> well as to configure them in the Loopback test mode.
>>
>> The MMR IO controller device has been updated since the AM65x SoC and
>> hence a newer property is needed to describe the one in AM625 SoC.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   .../bindings/display/ti/ti,am65x-dss.yaml     | 21 +++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 11d9b3821409..672765ad1f30 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -118,12 +118,33 @@ properties:
>>         and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
>>         interface to work.
>>   
>> +  ti,am625-oldi-io-ctrl:
>> +    $ref: "/schemas/types.yaml#/definitions/phandle"
>> +    description:
>> +      phandle to syscon device node mapping OLDI IO_CTRL registers, for
>> +      AM625 SoC. The mapped range should point to OLDI0_DAT0_IO_CTRL,
>> +      and map the registers up till OLDI_LB_CTRL. This property allows
>> +      the driver to control the power delivery to the OLDI TXes and
>> +      their loopback control as well.
> 
> What's wrong with the existing ti,am65x-oldi-io-ctrl other than the less
> than ideal naming? And you just continued with the same issue so the
> next part will need yet another property. Sorry, no. Just use the
> existing property.
> 
I introduced the new property because the peripheral was a newer and
different implementation from the previous one.

However, the same property can be re-used. I will do so in the re-spin.

>> +
>>     max-memory-bandwidth:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>       description:
>>         Input memory (from main memory to dispc) bandwidth limit in
>>         bytes per second
>>   
>> +if:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: ti,am65x-dss
>> +then:
>> +  properties:
>> +    ti,am625-oldi-io-ctrl: false
>> +else:
>> +  properties:
>> +    ti,am65x-oldi-io-ctrl: false
>> +
>>   required:
>>     - compatible
>>     - reg
>> -- 
>> 2.37.0
>>
>>

Regards
Aradhya

[PATCH 3/8] drm/tidss: Add support for DSS port properties

[Aradhya Bhatia]

Add support to enable and read the dss port properties.

The "ti,oldi-mode" property indicates the tidss driver how many OLDI
TXes are enabled as well as which mode do they need to be connected in.

The "ti,rgb565_to_888" is a special property that forces the DSS to
output 16bit RGB565 data to a 24bit RGB888 bridge. This property can be
used when the bridge does not explicity support RGB565.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 55 ++++++++++++++++++++++++++---
 drivers/gpu/drm/tidss/tidss_dispc.h |  8 +++++
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index f084f0688a54..add725fa682b 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -348,6 +348,10 @@ struct dispc_device {
 
 	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
 
+	enum dss_oldi_modes oldi_mode;
+
+	bool rgb565_to_888;
+
 	u32 *fourccs;
 	u32 num_fourccs;
 
@@ -2718,6 +2722,42 @@ static void dispc_softreset(struct dispc_device *dispc)
 		dev_warn(dispc->dev, "failed to reset dispc\n");
 }
 
+static void dispc_get_port_properties(struct dispc_device *dispc)
+{
+	u32 i = 0;
+	struct device_node *dss_ports, *vport;
+
+	dss_ports = of_get_next_child(dispc->dev->of_node, NULL);
+
+	for_each_child_of_node(dss_ports, vport) {
+		if (dispc->feat->vp_bus_type[i] == DISPC_VP_OLDI) {
+			if (of_property_read_u32(vport, "ti,oldi-mode", &dispc->oldi_mode)) {
+				dev_dbg(dispc->dev, "%s: Using OLDI defaults on vp%d.\n",
+					__func__, i);
+				if (dispc->feat->subrev == DISPC_AM65X)
+					dispc->oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE_0;
+				else
+					dispc->oldi_mode = OLDI_MODE_OFF;
+			}
+
+			/*
+			 * DISPC_AM65X DSS has a singular OLDI TX. It can not work in
+			 * Dual/Duplicate Mode. Forcing defaults.
+			 */
+			if (dispc->feat->subrev == DISPC_AM65X &&
+			    dispc->oldi_mode > OLDI_SINGLE_LINK_SINGLE_MODE_0) {
+				dev_dbg(dispc->dev,
+					"%s: Using default OLDI mode %d. AM65X can not support mode %d.\n",
+					__func__, OLDI_SINGLE_LINK_SINGLE_MODE_0, dispc->oldi_mode);
+				dispc->oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE_0;
+			}
+		} else if (dispc->feat->vp_bus_type[i] == DISPC_VP_DPI) {
+			dispc->rgb565_to_888 = of_property_read_bool(vport, "ti,rgb565-to-888");
+		}
+		i++;
+	}
+}
+
 int dispc_init(struct tidss_device *tidss)
 {
 	struct device *dev = tidss->dev;
@@ -2812,10 +2852,17 @@ int dispc_init(struct tidss_device *tidss)
 		dispc->vp_data[i].gamma_table = gamma_table;
 	}
 
-	if (feat->subrev == DISPC_AM65X) {
-		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
-		if (r)
-			return r;
+	if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) {
+		dispc_get_port_properties(dispc);
+		if (dispc->oldi_mode) {
+			r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
+			if (r)
+				return r;
+		}
+	} else {
+		/* K2G and J721E DSS do not support these properties */
+		dispc->oldi_mode = OLDI_MODE_OFF;
+		dispc->rgb565_to_888 = false;
 	}
 
 	dispc->fclk = devm_clk_get(dev, "fck");
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index a28005dafdc9..de8a95d3e3be 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,14 @@ enum dispc_dss_subrevision {
 	DISPC_AM625,
 };
 
+enum dss_oldi_modes {
+	OLDI_MODE_OFF,				/* OLDI turned off / tied off in IP. */
+	OLDI_SINGLE_LINK_SINGLE_MODE_0,		/* Single Output over OLDI 0. */
+	OLDI_SINGLE_LINK_SINGLE_MODE_1,		/* Single Output over OLDI 1. */
+	OLDI_SINGLE_LINK_DUPLICATE_MODE,	/* Duplicate Output over OLDI 0 and 1. */
+	OLDI_DUAL_LINK,				/* Combined Output over OLDI 0 and 1. */
+};
+
 struct dispc_features {
 	int min_pclk_khz;
 	int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
-- 
2.37.0

Re: [PATCH 3/8] drm/tidss: Add support for DSS port properties

[Tomi Valkeinen]

On 19/07/2022 11:08, Aradhya Bhatia wrote:
> Add support to enable and read the dss port properties.
> 
> The "ti,oldi-mode" property indicates the tidss driver how many OLDI
> TXes are enabled as well as which mode do they need to be connected in.
> 
> The "ti,rgb565_to_888" is a special property that forces the DSS to
> output 16bit RGB565 data to a 24bit RGB888 bridge. This property can be
> used when the bridge does not explicity support RGB565.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>

I think it would be good if you would split the patches that contain 
both OLDI and rgb565 changes, and arrange the series so that you first 
have patches that add the rgb565 (dt and driver), and after that, the 
OLDI changes.

They are fully separate things, and makes understanding the changes a 
bit easier.

  Tomi

[PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Aradhya Bhatia]

The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
resolution video.

Add support in the driver for the discovery of such a dual mode
connection on the OLDI video port, using the values of "ti,oldi-mode"
property.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index add725fa682b..fb1fdecfc83a 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 	}
 }
 
-enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
+enum dispc_oldi_mode_reg_val {
+	SPWG_18		= 0,
+	JEIDA_24	= 1,
+	SPWG_24		= 2,
+	DL_SPWG_18	= 4,
+	DL_JEIDA_24	= 5,
+	DL_SPWG_24	= 6,
+};
 
 struct dispc_bus_format {
 	u32 bus_fmt;
 	u32 data_width;
 	bool is_oldi_fmt;
+	bool is_dual_link;
 	enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
 };
 
 static const struct dispc_bus_format dispc_bus_formats[] = {
-	{ MEDIA_BUS_FMT_RGB444_1X12,		12, false, 0 },
-	{ MEDIA_BUS_FMT_RGB565_1X16,		16, false, 0 },
-	{ MEDIA_BUS_FMT_RGB666_1X18,		18, false, 0 },
-	{ MEDIA_BUS_FMT_RGB888_1X24,		24, false, 0 },
-	{ MEDIA_BUS_FMT_RGB101010_1X30,		30, false, 0 },
-	{ MEDIA_BUS_FMT_RGB121212_1X36,		36, false, 0 },
-	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, true, SPWG_18 },
-	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, true, SPWG_24 },
-	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, true, JEIDA_24 },
+	{ MEDIA_BUS_FMT_RGB444_1X12,		12, false, false, 0 },
+	{ MEDIA_BUS_FMT_RGB565_1X16,		16, false, false, 0 },
+	{ MEDIA_BUS_FMT_RGB666_1X18,		18, false, false, 0 },
+	{ MEDIA_BUS_FMT_RGB888_1X24,		24, false, false, 0 },
+	{ MEDIA_BUS_FMT_RGB101010_1X30,		30, false, false, 0 },
+	{ MEDIA_BUS_FMT_RGB121212_1X36,		36, false, false, 0 },
+	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, true, false, SPWG_18 },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, true, false, SPWG_24 },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, true, false, JEIDA_24 },
+	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, true, true, DL_SPWG_18 },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, true, true, DL_SPWG_24 },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, true, true, DL_JEIDA_24 },
 };
 
 static const
@@ -880,9 +891,15 @@ struct dispc_bus_format *dispc_vp_find_bus_fmt(struct dispc_device *dispc,
 					       u32 bus_fmt, u32 bus_flags)
 {
 	unsigned int i;
+	bool is_dual_link = false;
+
+	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
+	    dispc->oldi_mode == OLDI_DUAL_LINK)
+		is_dual_link = true;
 
 	for (i = 0; i < ARRAY_SIZE(dispc_bus_formats); ++i) {
-		if (dispc_bus_formats[i].bus_fmt == bus_fmt)
+		if (dispc_bus_formats[i].bus_fmt == bus_fmt &&
+		    dispc_bus_formats[i].is_dual_link == is_dual_link)
 			return &dispc_bus_formats[i];
 	}
 
-- 
2.37.0

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Tomi Valkeinen]

On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
> resolution video.
> 
> Add support in the driver for the discovery of such a dual mode
> connection on the OLDI video port, using the values of "ti,oldi-mode"
> property.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
>   1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index add725fa682b..fb1fdecfc83a 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
>   	}
>   }
>   
> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
> +enum dispc_oldi_mode_reg_val {
> +	SPWG_18		= 0,
> +	JEIDA_24	= 1,
> +	SPWG_24		= 2,
> +	DL_SPWG_18	= 4,
> +	DL_JEIDA_24	= 5,
> +	DL_SPWG_24	= 6,
> +};
>   
>   struct dispc_bus_format {
>   	u32 bus_fmt;
>   	u32 data_width;
>   	bool is_oldi_fmt;
> +	bool is_dual_link;
>   	enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>   };
>   
>   static const struct dispc_bus_format dispc_bus_formats[] = {
> -	{ MEDIA_BUS_FMT_RGB444_1X12,		12, false, 0 },
> -	{ MEDIA_BUS_FMT_RGB565_1X16,		16, false, 0 },
> -	{ MEDIA_BUS_FMT_RGB666_1X18,		18, false, 0 },
> -	{ MEDIA_BUS_FMT_RGB888_1X24,		24, false, 0 },
> -	{ MEDIA_BUS_FMT_RGB101010_1X30,		30, false, 0 },
> -	{ MEDIA_BUS_FMT_RGB121212_1X36,		36, false, 0 },
> -	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, true, SPWG_18 },
> -	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, true, SPWG_24 },
> -	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, true, JEIDA_24 },
> +	{ MEDIA_BUS_FMT_RGB444_1X12,		12, false, false, 0 },
> +	{ MEDIA_BUS_FMT_RGB565_1X16,		16, false, false, 0 },
> +	{ MEDIA_BUS_FMT_RGB666_1X18,		18, false, false, 0 },
> +	{ MEDIA_BUS_FMT_RGB888_1X24,		24, false, false, 0 },
> +	{ MEDIA_BUS_FMT_RGB101010_1X30,		30, false, false, 0 },
> +	{ MEDIA_BUS_FMT_RGB121212_1X36,		36, false, false, 0 },
> +	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, true, false, SPWG_18 },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, true, false, SPWG_24 },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, true, false, JEIDA_24 },
> +	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,	18, true, true, DL_SPWG_18 },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,	24, true, true, DL_SPWG_24 },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,	24, true, true, DL_JEIDA_24 },
>   };

So the dual link sends two pixels per clock, right? Are there panel or 
bridge drivers that support this? My initial thought was that it should 
be a new bus format.

  Tomi

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Tomi Valkeinen]

On 28/07/2022 14:03, Tomi Valkeinen wrote:
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>> resolution video.
>>
>> Add support in the driver for the discovery of such a dual mode
>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>> property.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index add725fa682b..fb1fdecfc83a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device 
>> *dispc, dispc_irq_t mask)
>>       }
>>   }
>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 2 };
>> +enum dispc_oldi_mode_reg_val {
>> +    SPWG_18        = 0,
>> +    JEIDA_24    = 1,
>> +    SPWG_24        = 2,
>> +    DL_SPWG_18    = 4,
>> +    DL_JEIDA_24    = 5,
>> +    DL_SPWG_24    = 6,
>> +};
>>   struct dispc_bus_format {
>>       u32 bus_fmt;
>>       u32 data_width;
>>       bool is_oldi_fmt;
>> +    bool is_dual_link;
>>       enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>   };
>>   static const struct dispc_bus_format dispc_bus_formats[] = {
>> -    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, 0 },
>> -    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, 0 },
>> -    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, 0 },
>> -    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, 0 },
>> -    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, 0 },
>> -    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, 0 },
>> -    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, SPWG_18 },
>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, SPWG_24 },
>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, JEIDA_24 },
>> +    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, false, 0 },
>> +    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, false, 0 },
>> +    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, false, 0 },
>> +    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, false, 0 },
>> +    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, false, 0 },
>> +    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, false, 0 },
>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, false, SPWG_18 },
>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, false, SPWG_24 },
>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, false, JEIDA_24 },
>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, true, DL_SPWG_18 },
>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, true, DL_SPWG_24 },
>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, true, 
>> DL_JEIDA_24 },
>>   };
> 
> So the dual link sends two pixels per clock, right? Are there panel or 
> bridge drivers that support this? My initial thought was that it should 
> be a new bus format.

Looks like we have drm bridges supporting dual link, and they use the 
"normal" bus format. Did you have a look at them? They require two port 
nodes for dual link, and use the existence of the second one to decide 
if dual link is used or not.

There are also lvds helpers in drm_of.c. I didn't look closely, but it 
looked to me that the helpers can tell you if the ports are connected to 
a dual link bridge. If not, you could fall back to cloning. This way no 
extra properties are needed. But you will need to add a port node, which 
I think you need to add anyway for cloning.

  Tomi

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Aradhya Bhatia]

Hi Tomi,

On 28-Jul-22 17:15, Tomi Valkeinen wrote:
> On 28/07/2022 14:03, Tomi Valkeinen wrote:
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>>> resolution video.
>>>
>>> Add support in the driver for the discovery of such a dual mode
>>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>>> property.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 39 +++++++++++++++++++++--------
>>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index add725fa682b..fb1fdecfc83a 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device 
>>> *dispc, dispc_irq_t mask)
>>>       }
>>>   }
>>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 
>>> 2 };
>>> +enum dispc_oldi_mode_reg_val {
>>> +    SPWG_18        = 0,
>>> +    JEIDA_24    = 1,
>>> +    SPWG_24        = 2,
>>> +    DL_SPWG_18    = 4,
>>> +    DL_JEIDA_24    = 5,
>>> +    DL_SPWG_24    = 6,
>>> +};
>>>   struct dispc_bus_format {
>>>       u32 bus_fmt;
>>>       u32 data_width;
>>>       bool is_oldi_fmt;
>>> +    bool is_dual_link;
>>>       enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>>   };
>>>   static const struct dispc_bus_format dispc_bus_formats[] = {
>>> -    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, 0 },
>>> -    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, 0 },
>>> -    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, 0 },
>>> -    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, 0 },
>>> -    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, 0 },
>>> -    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, 0 },
>>> -    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, SPWG_18 },
>>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, SPWG_24 },
>>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, JEIDA_24 },
>>> +    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, false, 0 },
>>> +    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, false, 0 },
>>> +    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, false, 0 },
>>> +    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, false, 0 },
>>> +    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, false, 0 },
>>> +    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, false, 0 },
>>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, false, SPWG_18 },
>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, false, SPWG_24 },
>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, false, JEIDA_24 },
>>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, true, DL_SPWG_18 },
>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, true, DL_SPWG_24 },
>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, true, DL_JEIDA_24 },
>>>   };
>>
>> So the dual link sends two pixels per clock, right? Are there panel or 
>> bridge drivers that support this? My initial thought was that it 
>> should be a new bus format.
> 
> Looks like we have drm bridges supporting dual link, and they use the 
> "normal" bus format. Did you have a look at them? They require two port 
> nodes for dual link, and use the existence of the second one to decide 
> if dual link is used or not.
The above edits were not for adding a new bus format for dual link
connections. I added them in order to be able to write the correct OLDI
config values in the register.

> 
> There are also lvds helpers in drm_of.c. I didn't look closely, but it 
> looked to me that the helpers can tell you if the ports are connected to 
> a dual link bridge. If not, you could fall back to cloning. This way no 
> extra properties are needed. But you will need to add a port node, which 
> I think you need to add anyway for cloning.
I have now seen drm_of.c and examples (renesas' rcar lvds) that use the
apis that drm_of.c is offering. In those cases, the OLDI TXes are being
modeled as separate devices, which is not the case with the tidss' OLDI
TXes. Since the only few OLDI registers are in the DSS address space,
they were just being configured through the tidss driver.

Even in DT, the dss port (for OLDI) connects to the panel port's
endpoint directly. Even in cases of dual link or cloning, it's only a
singular remote-to-endpoint connection between the (OLDI) VP and the
panel port. Hence the requirement of the properties in the earlier
patches of the series.

The use of lvds helper functions does not seem feasible in this case,
because even they read DT properties to determine the dual link
connection and those properties need to be a part of a lvds bridge
device.

I have also been considering the idea of implementing a new device
driver for the OLDI TXes, not unlike the renesas' one. That way the
driver could have the properties and the lvds helper functions at their
disposal. I am just slightly unsure if that would allow space for any
conflicts because of the shared register space.

Do let me know what you think!

Regards
Aradhya

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Tomi Valkeinen]

Hi,

On 09/08/2022 08:58, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 28-Jul-22 17:15, Tomi Valkeinen wrote:
>> On 28/07/2022 14:03, Tomi Valkeinen wrote:
>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>>>> resolution video.
>>>>
>>>> Add support in the driver for the discovery of such a dual mode
>>>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>>>> property.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 39 
>>>> +++++++++++++++++++++--------
>>>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index add725fa682b..fb1fdecfc83a 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device 
>>>> *dispc, dispc_irq_t mask)
>>>>       }
>>>>   }
>>>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 = 
>>>> 2 };
>>>> +enum dispc_oldi_mode_reg_val {
>>>> +    SPWG_18        = 0,
>>>> +    JEIDA_24    = 1,
>>>> +    SPWG_24        = 2,
>>>> +    DL_SPWG_18    = 4,
>>>> +    DL_JEIDA_24    = 5,
>>>> +    DL_SPWG_24    = 6,
>>>> +};
>>>>   struct dispc_bus_format {
>>>>       u32 bus_fmt;
>>>>       u32 data_width;
>>>>       bool is_oldi_fmt;
>>>> +    bool is_dual_link;
>>>>       enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>>>   };
>>>>   static const struct dispc_bus_format dispc_bus_formats[] = {
>>>> -    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, 0 },
>>>> -    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, 0 },
>>>> -    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, 0 },
>>>> -    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, 0 },
>>>> -    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, 0 },
>>>> -    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, 0 },
>>>> -    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, SPWG_18 },
>>>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, SPWG_24 },
>>>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, JEIDA_24 },
>>>> +    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, false, 0 },
>>>> +    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, false, 0 },
>>>> +    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, false, 0 },
>>>> +    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, false, 0 },
>>>> +    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, false, 0 },
>>>> +    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, false, 0 },
>>>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, false, SPWG_18 },
>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, false, SPWG_24 },
>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, false, 
>>>> JEIDA_24 },
>>>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, true, 
>>>> DL_SPWG_18 },
>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, true, 
>>>> DL_SPWG_24 },
>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, true, 
>>>> DL_JEIDA_24 },
>>>>   };
>>>
>>> So the dual link sends two pixels per clock, right? Are there panel 
>>> or bridge drivers that support this? My initial thought was that it 
>>> should be a new bus format.
>>
>> Looks like we have drm bridges supporting dual link, and they use the 
>> "normal" bus format. Did you have a look at them? They require two 
>> port nodes for dual link, and use the existence of the second one to 
>> decide if dual link is used or not.
> The above edits were not for adding a new bus format for dual link
> connections. I added them in order to be able to write the correct OLDI
> config values in the register.
> 
>>
>> There are also lvds helpers in drm_of.c. I didn't look closely, but it 
>> looked to me that the helpers can tell you if the ports are connected 
>> to a dual link bridge. If not, you could fall back to cloning. This 
>> way no extra properties are needed. But you will need to add a port 
>> node, which I think you need to add anyway for cloning.
> I have now seen drm_of.c and examples (renesas' rcar lvds) that use the
> apis that drm_of.c is offering. In those cases, the OLDI TXes are being
> modeled as separate devices, which is not the case with the tidss' OLDI
> TXes. Since the only few OLDI registers are in the DSS address space,
> they were just being configured through the tidss driver.

I think it's irrelevant (in the bigger picture) whether the TXes are 
separate devices, single device or part of some other device. Or why do 
you think it matters?

> Even in DT, the dss port (for OLDI) connects to the panel port's
> endpoint directly. Even in cases of dual link or cloning, it's only a
> singular remote-to-endpoint connection between the (OLDI) VP and the
> panel port. Hence the requirement of the properties in the earlier
> patches of the series.

Sorry, I don't follow. If you use cloning, you have two TX outputs, 
going to two panels, right? So you need two panel DT nodes, and those 
would connect to two OLDI TX ports in the DSS.

Afaics the existing dual link bridge/panel drivers also use two ports 
for the connection, so to use the dual link you need two ports in the DSS.

I admit I'm not familiar with LVDS dual link, but it's not clear to me 
how you see the dual OLDI TX being used with other drivers if you have 
only one port. What kind of setups have you tested?

> The use of lvds helper functions does not seem feasible in this case,
> because even they read DT properties to determine the dual link
> connection and those properties need to be a part of a lvds bridge
> device.

Can you elaborate a bit more why the DRM helpers couldn't be used here?

> I have also been considering the idea of implementing a new device
> driver for the OLDI TXes, not unlike the renesas' one. That way the
> driver could have the properties and the lvds helper functions at their
> disposal. I am just slightly unsure if that would allow space for any
> conflicts because of the shared register space.

No, I don't think new devices are needed here.

  Tomi

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Aradhya Bhatia]

Hi Tomi,

On 09-Aug-22 11:58, Tomi Valkeinen wrote:
> Hi,
> 
> On 09/08/2022 08:58, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 28-Jul-22 17:15, Tomi Valkeinen wrote:
>>> On 28/07/2022 14:03, Tomi Valkeinen wrote:
>>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>>> The 2 OLDI TXes in the AM625 SoC can be synced together to output a 2K
>>>>> resolution video.
>>>>>
>>>>> Add support in the driver for the discovery of such a dual mode
>>>>> connection on the OLDI video port, using the values of "ti,oldi-mode"
>>>>> property.
>>>>>
>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>> ---
>>>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 39 
>>>>> +++++++++++++++++++++--------
>>>>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> index add725fa682b..fb1fdecfc83a 100644
>>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> @@ -853,25 +853,36 @@ void dispc_set_irqenable(struct dispc_device 
>>>>> *dispc, dispc_irq_t mask)
>>>>>       }
>>>>>   }
>>>>> -enum dispc_oldi_mode_reg_val { SPWG_18 = 0, JEIDA_24 = 1, SPWG_24 
>>>>> = 2 };
>>>>> +enum dispc_oldi_mode_reg_val {
>>>>> +    SPWG_18        = 0,
>>>>> +    JEIDA_24    = 1,
>>>>> +    SPWG_24        = 2,
>>>>> +    DL_SPWG_18    = 4,
>>>>> +    DL_JEIDA_24    = 5,
>>>>> +    DL_SPWG_24    = 6,
>>>>> +};
>>>>>   struct dispc_bus_format {
>>>>>       u32 bus_fmt;
>>>>>       u32 data_width;
>>>>>       bool is_oldi_fmt;
>>>>> +    bool is_dual_link;
>>>>>       enum dispc_oldi_mode_reg_val oldi_mode_reg_val;
>>>>>   };
>>>>>   static const struct dispc_bus_format dispc_bus_formats[] = {
>>>>> -    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, 0 },
>>>>> -    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, 0 },
>>>>> -    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, 0 },
>>>>> -    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, 0 },
>>>>> -    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, 0 },
>>>>> -    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, 0 },
>>>>> -    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, SPWG_18 },
>>>>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, SPWG_24 },
>>>>> -    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, JEIDA_24 },
>>>>> +    { MEDIA_BUS_FMT_RGB444_1X12,        12, false, false, 0 },
>>>>> +    { MEDIA_BUS_FMT_RGB565_1X16,        16, false, false, 0 },
>>>>> +    { MEDIA_BUS_FMT_RGB666_1X18,        18, false, false, 0 },
>>>>> +    { MEDIA_BUS_FMT_RGB888_1X24,        24, false, false, 0 },
>>>>> +    { MEDIA_BUS_FMT_RGB101010_1X30,        30, false, false, 0 },
>>>>> +    { MEDIA_BUS_FMT_RGB121212_1X36,        36, false, false, 0 },
>>>>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, false, SPWG_18 },
>>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, false, SPWG_24 },
>>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, false, 
>>>>> JEIDA_24 },
>>>>> +    { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,    18, true, true, 
>>>>> DL_SPWG_18 },
>>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,    24, true, true, 
>>>>> DL_SPWG_24 },
>>>>> +    { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,    24, true, true, 
>>>>> DL_JEIDA_24 },
>>>>>   };
>>>>
>>>> So the dual link sends two pixels per clock, right? Are there panel 
>>>> or bridge drivers that support this? My initial thought was that it 
>>>> should be a new bus format.
>>>
>>> Looks like we have drm bridges supporting dual link, and they use the 
>>> "normal" bus format. Did you have a look at them? They require two 
>>> port nodes for dual link, and use the existence of the second one to 
>>> decide if dual link is used or not.
>> The above edits were not for adding a new bus format for dual link
>> connections. I added them in order to be able to write the correct OLDI
>> config values in the register.
>>
>>>
>>> There are also lvds helpers in drm_of.c. I didn't look closely, but 
>>> it looked to me that the helpers can tell you if the ports are 
>>> connected to a dual link bridge. If not, you could fall back to 
>>> cloning. This way no extra properties are needed. But you will need 
>>> to add a port node, which I think you need to add anyway for cloning.
>> I have now seen drm_of.c and examples (renesas' rcar lvds) that use the
>> apis that drm_of.c is offering. In those cases, the OLDI TXes are being
>> modeled as separate devices, which is not the case with the tidss' OLDI
>> TXes. Since the only few OLDI registers are in the DSS address space,
>> they were just being configured through the tidss driver.
> 
> I think it's irrelevant (in the bigger picture) whether the TXes are 
> separate devices, single device or part of some other device. Or why do 
> you think it matters?
> 
Sorry, by separate I meant having a separate driver irrespective of
whether or not its a part of another device.

>> Even in DT, the dss port (for OLDI) connects to the panel port's
>> endpoint directly. Even in cases of dual link or cloning, it's only a
>> singular remote-to-endpoint connection between the (OLDI) VP and the
>> panel port. Hence the requirement of the properties in the earlier
>> patches of the series.
> 
> Sorry, I don't follow. If you use cloning, you have two TX outputs, 
> going to two panels, right? So you need two panel DT nodes, and those 
> would connect to two OLDI TX ports in the DSS.
>  > Afaics the existing dual link bridge/panel drivers also use two ports
> for the connection, so to use the dual link you need two ports in the DSS.
> 
> I admit I'm not familiar with LVDS dual link, but it's not clear to me 
> how you see the dual OLDI TX being used with other drivers if you have 
> only one port. What kind of setups have you tested?
> 
In the DTs, the OLDIs are not modeled at all. Since the DSS only has a
single VP for OLDI, the DT dss port (for OLDI) is connected to a single
simple-panel node for dual link, bypassing the OLDI TX in DT. I have
this same OLDI setup and have been testing on this.

I do not have a cloning display setup with me, but I have seen DT DSS
port connected to one of 2 panel nodes while the other panel (remains as
a companion panel to the first) without any endpoint connections. Since,
the OLDI TXes (0 and 1), receive the same clocks and inputs from DSS
OLDI VP, this 'method' has worked too.

>> The use of lvds helper functions does not seem feasible in this case,
>> because even they read DT properties to determine the dual link
>> connection and those properties need to be a part of a lvds bridge
>> device.
> 
> Can you elaborate a bit more why the DRM helpers couldn't be used here?
> 
The drm_of.c helpers use DT properties to ascertain the presence of a
dual-link connection. While there wasn't a specific helper to determine
dual-link or not, the drivers use the odd/even pixel order helper which
is based on the properties "dual-lvds-odd-pixels" and "dual-lvds-odd-
pixels". If either of the properties are absent, the helper returns an
error making the driver to use single link.

These properties are LVDS specific, but they could not be added in the
DT because there is no OLDI TX DT node for our case.

>> I have also been considering the idea of implementing a new device
>> driver for the OLDI TXes, not unlike the renesas' one. That way the
>> driver could have the properties and the lvds helper functions at their
>> disposal. I am just slightly unsure if that would allow space for any
>> conflicts because of the shared register space.
> 
> No, I don't think new devices are needed here.
Okay...

I am not quite sure I understand completely what you are recommending
the OLDI to be. It seems to me that you want the OLDI TXes to be modeled
as nodes, right? Wouldn't that automatically require some sort of
standalone driver arrangement for them? Or am I missing something
important here?


Regards
Aradhya

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Tomi Valkeinen]

On 09/08/2022 12:06, Aradhya Bhatia wrote:

>>> Even in DT, the dss port (for OLDI) connects to the panel port's
>>> endpoint directly. Even in cases of dual link or cloning, it's only a
>>> singular remote-to-endpoint connection between the (OLDI) VP and the
>>> panel port. Hence the requirement of the properties in the earlier
>>> patches of the series.
>>
>> Sorry, I don't follow. If you use cloning, you have two TX outputs, 
>> going to two panels, right? So you need two panel DT nodes, and those 
>> would connect to two OLDI TX ports in the DSS.
>>  > Afaics the existing dual link bridge/panel drivers also use two ports
>> for the connection, so to use the dual link you need two ports in the 
>> DSS.
>>
>> I admit I'm not familiar with LVDS dual link, but it's not clear to me 
>> how you see the dual OLDI TX being used with other drivers if you have 
>> only one port. What kind of setups have you tested?
>>
> In the DTs, the OLDIs are not modeled at all. Since the DSS only has a
> single VP for OLDI, the DT dss port (for OLDI) is connected to a single
> simple-panel node for dual link, bypassing the OLDI TX in DT. I have
> this same OLDI setup and have been testing on this.

A DSS VP is a DSS internal port, whereas a port node in the DT is an 
external port. There doesn't have to be a 1:1 match between those.

The port in the DT represents some kind of "connector" to the outside 
world, which is usually a collection of pins that provide a video bus.

Here, as far as I can see, the DSS clearly has three external ports, two 
OLDI ports and one DPI port.

> I do not have a cloning display setup with me, but I have seen DT DSS
> port connected to one of 2 panel nodes while the other panel (remains as
> a companion panel to the first) without any endpoint connections. Since,
> the OLDI TXes (0 and 1), receive the same clocks and inputs from DSS
> OLDI VP, this 'method' has worked too.

This, and using simple-panel for dual link with single port connection, 
sounds like a hack.

A practical example: TI's customer wants to use AM625 and THC63LVD1024 
bridge. How does it work? THC63LVD1024 driver uses two LVDS ports for 
input, both of which are used in dual-link mode.

>>> The use of lvds helper functions does not seem feasible in this case,
>>> because even they read DT properties to determine the dual link
>>> connection and those properties need to be a part of a lvds bridge
>>> device.
>>
>> Can you elaborate a bit more why the DRM helpers couldn't be used here?
>>
> The drm_of.c helpers use DT properties to ascertain the presence of a
> dual-link connection. While there wasn't a specific helper to determine
> dual-link or not, the drivers use the odd/even pixel order helper which
> is based on the properties "dual-lvds-odd-pixels" and "dual-lvds-odd-
> pixels". If either of the properties are absent, the helper returns an
> error making the driver to use single link.
> 
> These properties are LVDS specific, but they could not be added in the
> DT because there is no OLDI TX DT node for our case.

If I'm not mistaken, those properties are in the port node, not the 
device node, and also, I believe those properties are on the sink side, 
so they wouldn't even be in the AM625 data. See, for example:

arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts

>>> I have also been considering the idea of implementing a new device
>>> driver for the OLDI TXes, not unlike the renesas' one. That way the
>>> driver could have the properties and the lvds helper functions at their
>>> disposal. I am just slightly unsure if that would allow space for any
>>> conflicts because of the shared register space.
>>
>> No, I don't think new devices are needed here.
> Okay...
> 
> I am not quite sure I understand completely what you are recommending
> the OLDI to be. It seems to me that you want the OLDI TXes to be modeled
> as nodes, right? Wouldn't that automatically require some sort of
> standalone driver arrangement for them? Or am I missing something
> important here?

No, I'm only talking about the DT port nodes. At the moment the AM65x DT 
bindings doc says that there are two ports, port@0 for OLDI and port@1 
for DPI. I'm saying AM625 needs three ports.

  Tomi

Re: [PATCH 4/8] drm/tidss: Add support for Dual Link LVDS Bus Format

[Aradhya Bhatia]

Hi Tomi,

On 09-Aug-22 15:21, Tomi Valkeinen wrote:
> On 09/08/2022 12:06, Aradhya Bhatia wrote:
> 
>>>> Even in DT, the dss port (for OLDI) connects to the panel port's
>>>> endpoint directly. Even in cases of dual link or cloning, it's only a
>>>> singular remote-to-endpoint connection between the (OLDI) VP and the
>>>> panel port. Hence the requirement of the properties in the earlier
>>>> patches of the series.
>>>
>>> Sorry, I don't follow. If you use cloning, you have two TX outputs, 
>>> going to two panels, right? So you need two panel DT nodes, and those 
>>> would connect to two OLDI TX ports in the DSS.
>>>  > Afaics the existing dual link bridge/panel drivers also use two ports
>>> for the connection, so to use the dual link you need two ports in the 
>>> DSS.
>>>
>>> I admit I'm not familiar with LVDS dual link, but it's not clear to 
>>> me how you see the dual OLDI TX being used with other drivers if you 
>>> have only one port. What kind of setups have you tested?
>>>
>> In the DTs, the OLDIs are not modeled at all. Since the DSS only has a
>> single VP for OLDI, the DT dss port (for OLDI) is connected to a single
>> simple-panel node for dual link, bypassing the OLDI TX in DT. I have
>> this same OLDI setup and have been testing on this.
> 
> A DSS VP is a DSS internal port, whereas a port node in the DT is an 
> external port. There doesn't have to be a 1:1 match between those.
> 
> The port in the DT represents some kind of "connector" to the outside 
> world, which is usually a collection of pins that provide a video bus.
> 
Okay, I now understand what you are saying. Indeed, I was mapping the
DSS VP and DT DSS port as 1:1 in my mind, which should not be the case.

> Here, as far as I can see, the DSS clearly has three external ports, two 
> OLDI ports and one DPI port.
> 
>> I do not have a cloning display setup with me, but I have seen DT DSS
>> port connected to one of 2 panel nodes while the other panel (remains as
>> a companion panel to the first) without any endpoint connections. Since,
>> the OLDI TXes (0 and 1), receive the same clocks and inputs from DSS
>> OLDI VP, this 'method' has worked too.
> 
> This, and using simple-panel for dual link with single port connection, 
> sounds like a hack.
> 
> A practical example: TI's customer wants to use AM625 and THC63LVD1024 
> bridge. How does it work? THC63LVD1024 driver uses two LVDS ports for 
> input, both of which are used in dual-link mode.
> 
Right. There has to be 2 ports for OLDI in DSS, to be connected to 2
ports of a single panel (dual link) or 2 ports of 2 panels (cloning).

>>>> The use of lvds helper functions does not seem feasible in this case,
>>>> because even they read DT properties to determine the dual link
>>>> connection and those properties need to be a part of a lvds bridge
>>>> device.
>>>
>>> Can you elaborate a bit more why the DRM helpers couldn't be used here?
>>>
>> The drm_of.c helpers use DT properties to ascertain the presence of a
>> dual-link connection. While there wasn't a specific helper to determine
>> dual-link or not, the drivers use the odd/even pixel order helper which
>> is based on the properties "dual-lvds-odd-pixels" and "dual-lvds-odd-
>> pixels". If either of the properties are absent, the helper returns an
>> error making the driver to use single link.
>>
>> These properties are LVDS specific, but they could not be added in the
>> DT because there is no OLDI TX DT node for our case.
> 
> If I'm not mistaken, those properties are in the port node, not the 
> device node, and also, I believe those properties are on the sink side, 
> so they wouldn't even be in the AM625 data. See, for example:
> 
> arch/arm64/boot/dts/renesas/r8a774c0-ek874-idk-2121wr.dts
Yeah, they are indeed on the sink side. I was misunderstood about this.
And the onus for properties is not on DSS nodes anymore.

This probably is a different discussion, but since the sink is now
responsible for those properties, these should get introduced in the
panel-common bindings, right?

The above example has a separate binding, but many dumb dual-link panels
will require those properties in panel-common.

> 
>>>> I have also been considering the idea of implementing a new device
>>>> driver for the OLDI TXes, not unlike the renesas' one. That way the
>>>> driver could have the properties and the lvds helper functions at their
>>>> disposal. I am just slightly unsure if that would allow space for any
>>>> conflicts because of the shared register space.
>>>
>>> No, I don't think new devices are needed here.
>> Okay...
>>
>> I am not quite sure I understand completely what you are recommending
>> the OLDI to be. It seems to me that you want the OLDI TXes to be modeled
>> as nodes, right? Wouldn't that automatically require some sort of
>> standalone driver arrangement for them? Or am I missing something
>> important here?
> 
> No, I'm only talking about the DT port nodes. At the moment the AM65x DT 
> bindings doc says that there are two ports, port@0 for OLDI and port@1 
> for DPI. I'm saying AM625 needs three ports.
Agreed.

Moreover, I will update the binding to reflect 3 ports for am625-dss.


Regards
Aradhya

[PATCH 5/8] drm/tidss: dt property to force 16bit VP output to a 24bit bridge

[Aradhya Bhatia]

Override the DSS Videoport to output 16bit RGB565 instead of 24bit
RGB888 when the dt property "ti,rgb565-to-888" has been enanbled.

Co-developed-by: Darren Etheridge <detheridge@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index fb1fdecfc83a..94fce035c1d7 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -896,6 +896,8 @@ struct dispc_bus_format *dispc_vp_find_bus_fmt(struct dispc_device *dispc,
 	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
 	    dispc->oldi_mode == OLDI_DUAL_LINK)
 		is_dual_link = true;
+	else if (bus_fmt == MEDIA_BUS_FMT_RGB888_1X24 && dispc->rgb565_to_888)
+		bus_fmt = MEDIA_BUS_FMT_RGB565_1X16;
 
 	for (i = 0; i < ARRAY_SIZE(dispc_bus_formats); ++i) {
 		if (dispc_bus_formats[i].bus_fmt == bus_fmt &&
-- 
2.37.0

[PATCH 6/8] drm/tidss: Add IO CTRL and Power support for OLDI TX in AM625

[Aradhya Bhatia]

The ctrl MMR module of the AM625 is different from the AM65X SoC. As a
result, the memory-mapped regsiters that control the OLDI TX power and
loopback have diverged in AM625 SoC.

Add support for the controller in AM625 and control OLDI.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c      | 70 +++++++++++++++++++-----
 drivers/gpu/drm/tidss/tidss_dispc_regs.h |  6 ++
 2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 94fce035c1d7..c4a5f808648f 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -934,21 +934,57 @@ int dispc_vp_bus_check(struct dispc_device *dispc, u32 hw_videoport,
 
 static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
 {
-	u32 val = power ? 0 : OLDI_PWRDN_TX;
+	u32 val;
 
 	if (WARN_ON(!dispc->oldi_io_ctrl))
 		return;
 
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
-	regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
-			   OLDI_PWRDN_TX, val);
+	if (dispc->feat->subrev == DISPC_AM65X) {
+		val = power ? 0 : OLDI_PWRDN_TX;
+
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
+				   OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
+				   OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
+				   OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
+				   OLDI_PWRDN_TX, val);
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
+				   OLDI_PWRDN_TX, val);
+
+	} else if (dispc->feat->subrev == DISPC_AM625) {
+		if (power) {
+			switch (dispc->oldi_mode) {
+			case OLDI_SINGLE_LINK_SINGLE_MODE_0:
+				/* Power down OLDI TX 1*/
+				val = OLDI1_PWRDN_TX;
+				break;
+
+			case OLDI_SINGLE_LINK_SINGLE_MODE_1:
+				/* Power down OLDI TX 0*/
+				val = OLDI0_PWRDN_TX;
+				break;
+
+			case OLDI_SINGLE_LINK_DUPLICATE_MODE:
+			case OLDI_DUAL_LINK:
+				/* No Power down */
+				val = 0;
+				break;
+
+			default:
+				/* Power down both the OLDI TXes */
+				val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
+				break;
+			}
+		} else {
+			/* Power down both the OLDI TXes */
+			val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
+		}
+
+		regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL,
+				   OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val);
+	}
 }
 
 static void dispc_set_num_datalines(struct dispc_device *dispc,
@@ -2701,9 +2737,15 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
 static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
 					 struct dispc_device *dispc)
 {
-	dispc->oldi_io_ctrl =
-		syscon_regmap_lookup_by_phandle(dev->of_node,
-						"ti,am65x-oldi-io-ctrl");
+	if (dispc->feat->subrev == DISPC_AM65X)
+		dispc->oldi_io_ctrl =
+			syscon_regmap_lookup_by_phandle(dev->of_node,
+							"ti,am65x-oldi-io-ctrl");
+	else
+		dispc->oldi_io_ctrl =
+			syscon_regmap_lookup_by_phandle(dev->of_node,
+							"ti,am625-oldi-io-ctrl");
+
 	if (PTR_ERR(dispc->oldi_io_ctrl) == -ENODEV) {
 		dispc->oldi_io_ctrl = NULL;
 	} else if (IS_ERR(dispc->oldi_io_ctrl)) {
diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
index 13feedfe5d6d..510bee70b3b8 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
@@ -238,6 +238,12 @@ enum dispc_common_regs {
 #define OLDI_DAT3_IO_CTRL			0x0C
 #define OLDI_CLK_IO_CTRL			0x10
 
+/* Only for AM625 OLDI TX */
+#define OLDI_PD_CTRL				0x100
+#define OLDI_LB_CTRL				0x104
+
 #define OLDI_PWRDN_TX				BIT(8)
+#define OLDI0_PWRDN_TX				BIT(0)
+#define OLDI1_PWRDN_TX				BIT(1)
 
 #endif /* __TIDSS_DISPC_REGS_H */
-- 
2.37.0

[PATCH 7/8] drm/tidss: Fix clock request value for OLDI videoports

[Aradhya Bhatia]

The OLDI TX(es) require a serial clock which is 7 times the pixel clock
of the display panel. When the OLDI is enabled in DSS, the pixel clock
input of the corresponding videoport gets a divided-by 7 value of the
requested clock.

For the am625-dss, the requested clock needs to be 7 times the value.

Update the clock frequency by requesting 7 times the value.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index c4a5f808648f..0b9689453ee8 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
 	int r;
 	unsigned long new_rate;
 
+	/*
+	 * For AM625 OLDI video ports, the requested pixel clock needs to take into account the
+	 * serial clock required for the serialization of DPI signals into LVDS signals. The
+	 * incoming pixel clock on the OLDI video port gets divided by 7 whenever OLDI enable bit
+	 * gets set.
+	 */
+	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
+	    dispc->feat->subrev == DISPC_AM625)
+		rate *= 7;
+
 	r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
 	if (r) {
 		dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
-- 
2.37.0

Re: [PATCH 7/8] drm/tidss: Fix clock request value for OLDI videoports

[Tomi Valkeinen]

On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The OLDI TX(es) require a serial clock which is 7 times the pixel clock
> of the display panel. When the OLDI is enabled in DSS, the pixel clock
> input of the corresponding videoport gets a divided-by 7 value of the
> requested clock.
> 
> For the am625-dss, the requested clock needs to be 7 times the value.
> 
> Update the clock frequency by requesting 7 times the value.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index c4a5f808648f..0b9689453ee8 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>   	int r;
>   	unsigned long new_rate;
>   
> +	/*
> +	 * For AM625 OLDI video ports, the requested pixel clock needs to take into account the
> +	 * serial clock required for the serialization of DPI signals into LVDS signals. The
> +	 * incoming pixel clock on the OLDI video port gets divided by 7 whenever OLDI enable bit
> +	 * gets set.
> +	 */
> +	if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
> +	    dispc->feat->subrev == DISPC_AM625)
> +		rate *= 7;
> +
>   	r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
>   	if (r) {
>   		dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",

The AM625 TRM seems to be missing the "DSS integration" section, even if 
it's referred to in three places in the TRM. Supposedly that has details 
about the clocking.

Shouldn't the source clock be 3.5x when dual-link mode is used?

While I don't know the details, this doesn't feel correct. We're 
supposed to be setting the VP pixel clock here, and the serial clock 
would be derived from that as it's done on AM65x. Is the DT clock tree 
wrong for AM625?

  Tomi

Re: [PATCH 7/8] drm/tidss: Fix clock request value for OLDI videoports

[Aradhya Bhatia]

On 28-Jul-22 15:35, Tomi Valkeinen wrote:
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The OLDI TX(es) require a serial clock which is 7 times the pixel clock
>> of the display panel. When the OLDI is enabled in DSS, the pixel clock
>> input of the corresponding videoport gets a divided-by 7 value of the
>> requested clock.
>>
>> For the am625-dss, the requested clock needs to be 7 times the value.
>>
>> Update the clock frequency by requesting 7 times the value.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index c4a5f808648f..0b9689453ee8 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device 
>> *dispc, u32 hw_videoport,
>>       int r;
>>       unsigned long new_rate;
>> +    /*
>> +     * For AM625 OLDI video ports, the requested pixel clock needs to 
>> take into account the
>> +     * serial clock required for the serialization of DPI signals 
>> into LVDS signals. The
>> +     * incoming pixel clock on the OLDI video port gets divided by 7 
>> whenever OLDI enable bit
>> +     * gets set.
>> +     */
>> +    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
>> +        dispc->feat->subrev == DISPC_AM625)
>> +        rate *= 7;
>> +
>>       r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
>>       if (r) {
>>           dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
> 
> The AM625 TRM seems to be missing the "DSS integration" section, even if 
> it's referred to in three places in the TRM. Supposedly that has details 
> about the clocking.
> 
> Shouldn't the source clock be 3.5x when dual-link mode is used?
There should not be.

Whenever OLDI is enabled, the clock generated from the PLL is 7 times
the required pixel clock.

For the OLDI TXes, the clock passes through a /2 divider. This divider
only gets activated when the dual mode has been enabled in the OLDI
configuration. Thus the OLDI TXes get 3.5x the pixel clock in dual mode.

When the OLDI has been configured for a single mode,
the PLL clock passes through the /2 divider without any change.

> 
> While I don't know the details, this doesn't feel correct. We're 
> supposed to be setting the VP pixel clock here, and the serial clock 
> would be derived from that as it's done on AM65x. Is the DT clock tree
> wrong for AM625?
Ideally, yes, its the pixel frequency that we are supposed to set here.

The same PLL clock (7 times the pixel frequency) passes through a /7
clock divider. This clock divider only gets active when OLDI is enabled.
Thus, the DSS VP clock input, only gets the actual pixel frequency that
it needs.

Since, the /7 divider is controlled by a signal from the DSS, the driver
needs to request 7 times more the pixel clock to accommodate for the
divider.

In AM65X, the system FW is able to model the 7 times requirement because
the divider is not controlled by the DSS signal. DSS signal controls a
multiplexer which receives both PLL Clock and PLL / 7 clock.


Regards
Aradhya

Re: [PATCH 7/8] drm/tidss: Fix clock request value for OLDI videoports

[Tomi Valkeinen]

On 29/07/2022 06:56, Aradhya Bhatia wrote:
> 
> On 28-Jul-22 15:35, Tomi Valkeinen wrote:
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The OLDI TX(es) require a serial clock which is 7 times the pixel clock
>>> of the display panel. When the OLDI is enabled in DSS, the pixel clock
>>> input of the corresponding videoport gets a divided-by 7 value of the
>>> requested clock.
>>>
>>> For the am625-dss, the requested clock needs to be 7 times the value.
>>>
>>> Update the clock frequency by requesting 7 times the value.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index c4a5f808648f..0b9689453ee8 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -1326,6 +1326,16 @@ int dispc_vp_set_clk_rate(struct dispc_device 
>>> *dispc, u32 hw_videoport,
>>>       int r;
>>>       unsigned long new_rate;
>>> +    /*
>>> +     * For AM625 OLDI video ports, the requested pixel clock needs 
>>> to take into account the
>>> +     * serial clock required for the serialization of DPI signals 
>>> into LVDS signals. The
>>> +     * incoming pixel clock on the OLDI video port gets divided by 7 
>>> whenever OLDI enable bit
>>> +     * gets set.
>>> +     */
>>> +    if (dispc->feat->vp_bus_type[hw_videoport] == DISPC_VP_OLDI &&
>>> +        dispc->feat->subrev == DISPC_AM625)
>>> +        rate *= 7;
>>> +
>>>       r = clk_set_rate(dispc->vp_clk[hw_videoport], rate);
>>>       if (r) {
>>>           dev_err(dispc->dev, "vp%d: failed to set clk rate to %lu\n",
>>
>> The AM625 TRM seems to be missing the "DSS integration" section, even 
>> if it's referred to in three places in the TRM. Supposedly that has 
>> details about the clocking.
>>
>> Shouldn't the source clock be 3.5x when dual-link mode is used?
> There should not be.
> 
> Whenever OLDI is enabled, the clock generated from the PLL is 7 times
> the required pixel clock.
> 
> For the OLDI TXes, the clock passes through a /2 divider. This divider
> only gets activated when the dual mode has been enabled in the OLDI
> configuration. Thus the OLDI TXes get 3.5x the pixel clock in dual mode.
> 
> When the OLDI has been configured for a single mode,
> the PLL clock passes through the /2 divider without any change.
> 
>>
>> While I don't know the details, this doesn't feel correct. We're 
>> supposed to be setting the VP pixel clock here, and the serial clock 
>> would be derived from that as it's done on AM65x. Is the DT clock tree
>> wrong for AM625?
> Ideally, yes, its the pixel frequency that we are supposed to set here.
> 
> The same PLL clock (7 times the pixel frequency) passes through a /7
> clock divider. This clock divider only gets active when OLDI is enabled.
> Thus, the DSS VP clock input, only gets the actual pixel frequency that
> it needs.
> 
> Since, the /7 divider is controlled by a signal from the DSS, the driver
> needs to request 7 times more the pixel clock to accommodate for the
> divider.
> 
> In AM65X, the system FW is able to model the 7 times requirement because
> the divider is not controlled by the DSS signal. DSS signal controls a
> multiplexer which receives both PLL Clock and PLL / 7 clock.

Could you ping the TI TRM team to add the DSS integration chapter? The 
clock tree is quite important part of the TRM.

I really don't like this one much. The dispc->vp_clk[0] here is actually 
not VP clock, but a parent clock of the VP clock, so, afaics, the driver 
gets "wrong" clock from DT data and then fixes things by manually 
multiplying the requested rate. If we have to go with a driver hack like 
this, I think it needs to be clarified more with comments, in the DT 
data also.

On AM625, the first videoport is always connected to OLDI(s), and never 
goes through as DPI, right? I.e. you cannot bypass OLDI?

If so, the /7 is, in practice, always enabled, as OLDI is the only use 
case. So why not add /7 node to the DT clock data, which would allow us 
to get rid of this code?

  Tomi

[PATCH 8/8] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Aradhya Bhatia]

The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
duplicated displays of smaller resolutions or enable a single Dual-Link
display with a higher resolution (1920x1200).

Configure the necessary register to enable the different modes.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 0b9689453ee8..28cb61259471 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 	int count = 0;
 
 	/*
-	 * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
-	 * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
+	 * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
+	 * set statically to 0.
 	 */
 
 	if (fmt->data_width == 24)
@@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
 
 	oldi_cfg |= BIT(0); /* ENABLE */
 
-	dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+	/*
+	 * As per all the current implementations of DSS, the OLDI TXes are present only on
+	 * hw_videoport = 0 (OLDI TX 0). However, the config register for 2nd OLDI TX (OLDI TX 1)
+	 * is present in the address space of hw_videoport = 1. Hence, using "hw_videoport + 1" to
+	 * configure OLDI TX 1.
+	 */
+
+	switch (dispc->oldi_mode) {
+	case OLDI_MODE_OFF:
+		oldi_cfg &= ~BIT(0); /* DISABLE */
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_SINGLE_LINK_SINGLE_MODE_0:
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_SINGLE_LINK_SINGLE_MODE_1:
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_SINGLE_LINK_DUPLICATE_MODE:
+		oldi_cfg |= BIT(5); /* DUPLICATE MODE */
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	case OLDI_DUAL_LINK:
+		oldi_cfg |= BIT(11); /* DUALMODESYNC */
+		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
+		break;
+
+	default:
+		dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
+			 __func__);
+		return;
+	}
 
 	while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
 	       count < 10000)
-- 
2.37.0

Re: [PATCH 8/8] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Tomi Valkeinen]

Hi,

On 19/07/2022 11:08, Aradhya Bhatia wrote:
> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
> duplicated displays of smaller resolutions or enable a single Dual-Link
> display with a higher resolution (1920x1200).
> 
> Configure the necessary register to enable the different modes.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>   1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 0b9689453ee8..28cb61259471 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>   	int count = 0;
>   
>   	/*
> -	 * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
> -	 * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
> +	 * For the moment MASTERSLAVE, and SRC bits of DISPC_VP_DSS_OLDI_CFG are
> +	 * set statically to 0.
>   	 */
>   
>   	if (fmt->data_width == 24)
> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct dispc_device *dispc, u32 hw_videoport,
>   
>   	oldi_cfg |= BIT(0); /* ENABLE */
>   
> -	dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +	/*
> +	 * As per all the current implementations of DSS, the OLDI TXes are present only on
> +	 * hw_videoport = 0 (OLDI TX 0). However, the config register for 2nd OLDI TX (OLDI TX 1)
> +	 * is present in the address space of hw_videoport = 1. Hence, using "hw_videoport + 1" to
> +	 * configure OLDI TX 1.
> +	 */
> +
> +	switch (dispc->oldi_mode) {
> +	case OLDI_MODE_OFF:
> +		oldi_cfg &= ~BIT(0); /* DISABLE */
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_SINGLE_LINK_SINGLE_MODE_0:
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_SINGLE_LINK_SINGLE_MODE_1:
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_SINGLE_LINK_DUPLICATE_MODE:
> +		oldi_cfg |= BIT(5); /* DUPLICATE MODE */
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	case OLDI_DUAL_LINK:
> +		oldi_cfg |= BIT(11); /* DUALMODESYNC */
> +		dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		dispc_vp_write(dispc, hw_videoport + 1, DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
> +		break;
> +
> +	default:
> +		dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
> +			 __func__);
> +		return;
> +	}
>   
>   	while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>   	       count < 10000)

This feels a bit hacky:

- The function is dispc_enable_oldi, but the above code also disables 
oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
moment.

- The function takes hw_videoport as a parameter, and is designed to 
work on that videoport. The above operates on two videoports. Isn't the 
function also called for hw_videoport +1, which would result in reg 
writes to hw_videoport + 2?

- No matching code in dispc_vp_unprepare

Obviously the duplicate mode (I presume that's "cloning") and the dual 
link complicate things here, and I have to say I haven't worked with 
such setups. But I think somehow this should be restructured so that 
common configuration (common to the OLDIs) is done somewhere else.

I would guess that there are other drivers that support cloning and dual 
mode. Did you have a look how they handle things?

  Tomi

Re: [PATCH 8/8] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Tomi Valkeinen]

On 27/07/2022 16:22, Tomi Valkeinen wrote:
> Hi,
> 
> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
>> duplicated displays of smaller resolutions or enable a single Dual-Link
>> display with a higher resolution (1920x1200).
>>
>> Configure the necessary register to enable the different modes.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 0b9689453ee8..28cb61259471 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct 
>> dispc_device *dispc, u32 hw_videoport,
>>       int count = 0;
>>       /*
>> -     * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
>> -     * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
>> +     * For the moment MASTERSLAVE, and SRC bits of 
>> DISPC_VP_DSS_OLDI_CFG are
>> +     * set statically to 0.
>>        */
>>       if (fmt->data_width == 24)
>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct 
>> dispc_device *dispc, u32 hw_videoport,
>>       oldi_cfg |= BIT(0); /* ENABLE */
>> -    dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +    /*
>> +     * As per all the current implementations of DSS, the OLDI TXes 
>> are present only on
>> +     * hw_videoport = 0 (OLDI TX 0). However, the config register for 
>> 2nd OLDI TX (OLDI TX 1)
>> +     * is present in the address space of hw_videoport = 1. Hence, 
>> using "hw_videoport + 1" to
>> +     * configure OLDI TX 1.
>> +     */
>> +
>> +    switch (dispc->oldi_mode) {
>> +    case OLDI_MODE_OFF:
>> +        oldi_cfg &= ~BIT(0); /* DISABLE */
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>> +        oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    case OLDI_DUAL_LINK:
>> +        oldi_cfg |= BIT(11); /* DUALMODESYNC */
>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>> oldi_cfg);
>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>> +        break;
>> +
>> +    default:
>> +        dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>> +             __func__);
>> +        return;
>> +    }
>>       while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>              count < 10000)
> 
> This feels a bit hacky:
> 
> - The function is dispc_enable_oldi, but the above code also disables 
> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
> moment.
> 
> - The function takes hw_videoport as a parameter, and is designed to 
> work on that videoport. The above operates on two videoports. Isn't the 
> function also called for hw_videoport +1, which would result in reg 
> writes to hw_videoport + 2?
> 
> - No matching code in dispc_vp_unprepare
> 
> Obviously the duplicate mode (I presume that's "cloning") and the dual 
> link complicate things here, and I have to say I haven't worked with 
> such setups. But I think somehow this should be restructured so that 
> common configuration (common to the OLDIs) is done somewhere else.
> 
> I would guess that there are other drivers that support cloning and dual 
> mode. Did you have a look how they handle things?

Oh, I see now... There's just one dss video port for OLDI, the same as 
in am65x, but that single video port is now connected to two OLDI TXes. 
And thus this function will only be called for the single video port.

But... The registers for the second OLDI are part of the second video 
port (DPI) register block?

  Tomi

Re: [PATCH 8/8] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Aradhya Bhatia]

Hi Tomi,

On 28-Jul-22 12:16, Tomi Valkeinen wrote:
> On 27/07/2022 16:22, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to enable 2
>>> duplicated displays of smaller resolutions or enable a single Dual-Link
>>> display with a higher resolution (1920x1200).
>>>
>>> Configure the necessary register to enable the different modes.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 +++++++++++++++++++++++++++--
>>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index 0b9689453ee8..28cb61259471 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct 
>>> dispc_device *dispc, u32 hw_videoport,
>>>       int count = 0;
>>>       /*
>>> -     * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
>>> -     * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
>>> +     * For the moment MASTERSLAVE, and SRC bits of 
>>> DISPC_VP_DSS_OLDI_CFG are
>>> +     * set statically to 0.
>>>        */
>>>       if (fmt->data_width == 24)
>>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct 
>>> dispc_device *dispc, u32 hw_videoport,
>>>       oldi_cfg |= BIT(0); /* ENABLE */
>>> -    dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +    /*
>>> +     * As per all the current implementations of DSS, the OLDI TXes 
>>> are present only on
>>> +     * hw_videoport = 0 (OLDI TX 0). However, the config register 
>>> for 2nd OLDI TX (OLDI TX 1)
>>> +     * is present in the address space of hw_videoport = 1. Hence, 
>>> using "hw_videoport + 1" to
>>> +     * configure OLDI TX 1.
>>> +     */
>>> +
>>> +    switch (dispc->oldi_mode) {
>>> +    case OLDI_MODE_OFF:
>>> +        oldi_cfg &= ~BIT(0); /* DISABLE */
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>>> +        oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    case OLDI_DUAL_LINK:
>>> +        oldi_cfg |= BIT(11); /* DUALMODESYNC */
>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>> oldi_cfg);
>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>> +        break;
>>> +
>>> +    default:
>>> +        dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>>> +             __func__);
>>> +        return;
>>> +    }
>>>       while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>>              count < 10000)
>>
>> This feels a bit hacky:
>>
>> - The function is dispc_enable_oldi, but the above code also disables 
>> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
>> moment.
>>
>> - The function takes hw_videoport as a parameter, and is designed to 
>> work on that videoport. The above operates on two videoports. Isn't 
>> the function also called for hw_videoport +1, which would result in 
>> reg writes to hw_videoport + 2?
>>
>> - No matching code in dispc_vp_unprepare
>>
>> Obviously the duplicate mode (I presume that's "cloning") and the dual 
>> link complicate things here, and I have to say I haven't worked with 
>> such setups. But I think somehow this should be restructured so that 
>> common configuration (common to the OLDIs) is done somewhere else.
>>
>> I would guess that there are other drivers that support cloning and 
>> dual mode. Did you have a look how they handle things?
> 
> Oh, I see now... There's just one dss video port for OLDI, the same as 
> in am65x, but that single video port is now connected to two OLDI TXes. 
> And thus this function will only be called for the single video port.
> > But... The registers for the second OLDI are part of the second video
> port (DPI) register block?

Yes! The config register for the second OLDI TX has been (incorrectly)
added in the register space for the DPI video port. 'dispc_vp_prepare'
is the only function calling 'dispc_enable_oldi', and
'dispc_enable_oldi' would not be called for hw_videoports = 1 (DPI)
because of the conditional check.

Hence, to activate both the OLDI-TXes connected to the OLDI video port,
I had to use the (hw_videoport + 1) way.

However, I will remove the disable part from the 'dispc_enable_oldi' and
I will implement the disabling properly under 'dispc_vp_unprepare', in
the next version.

Regards
Aradhya

Re: [PATCH 8/8] drm/tidss: Enable Dual and Duplicate Modes for OLDI

[Tomi Valkeinen]

On 28/07/2022 11:49, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 28-Jul-22 12:16, Tomi Valkeinen wrote:
>> On 27/07/2022 16:22, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 19/07/2022 11:08, Aradhya Bhatia wrote:
>>>> The AM625 DSS peripheral supports 2 OLDI TXes which can work to 
>>>> enable 2
>>>> duplicated displays of smaller resolutions or enable a single Dual-Link
>>>> display with a higher resolution (1920x1200).
>>>>
>>>> Configure the necessary register to enable the different modes.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 44 
>>>> +++++++++++++++++++++++++++--
>>>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index 0b9689453ee8..28cb61259471 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -1021,8 +1021,8 @@ static void dispc_enable_oldi(struct 
>>>> dispc_device *dispc, u32 hw_videoport,
>>>>       int count = 0;
>>>>       /*
>>>> -     * For the moment DUALMODESYNC, MASTERSLAVE, MODE, and SRC
>>>> -     * bits of DISPC_VP_DSS_OLDI_CFG are set statically to 0.
>>>> +     * For the moment MASTERSLAVE, and SRC bits of 
>>>> DISPC_VP_DSS_OLDI_CFG are
>>>> +     * set statically to 0.
>>>>        */
>>>>       if (fmt->data_width == 24)
>>>> @@ -1039,7 +1039,45 @@ static void dispc_enable_oldi(struct 
>>>> dispc_device *dispc, u32 hw_videoport,
>>>>       oldi_cfg |= BIT(0); /* ENABLE */
>>>> -    dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +    /*
>>>> +     * As per all the current implementations of DSS, the OLDI TXes 
>>>> are present only on
>>>> +     * hw_videoport = 0 (OLDI TX 0). However, the config register 
>>>> for 2nd OLDI TX (OLDI TX 1)
>>>> +     * is present in the address space of hw_videoport = 1. Hence, 
>>>> using "hw_videoport + 1" to
>>>> +     * configure OLDI TX 1.
>>>> +     */
>>>> +
>>>> +    switch (dispc->oldi_mode) {
>>>> +    case OLDI_MODE_OFF:
>>>> +        oldi_cfg &= ~BIT(0); /* DISABLE */
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_0:
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_SINGLE_LINK_SINGLE_MODE_1:
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_SINGLE_LINK_DUPLICATE_MODE:
>>>> +        oldi_cfg |= BIT(5); /* DUPLICATE MODE */
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    case OLDI_DUAL_LINK:
>>>> +        oldi_cfg |= BIT(11); /* DUALMODESYNC */
>>>> +        dispc_vp_write(dispc, hw_videoport, DISPC_VP_DSS_OLDI_CFG, 
>>>> oldi_cfg);
>>>> +        dispc_vp_write(dispc, hw_videoport + 1, 
>>>> DISPC_VP_DSS_OLDI_CFG, oldi_cfg);
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +        dev_warn(dispc->dev, "%s: Incorrect oldi mode. Returning.\n",
>>>> +             __func__);
>>>> +        return;
>>>> +    }
>>>>       while (!(oldi_reset_bit & dispc_read(dispc, DSS_SYSSTATUS)) &&
>>>>              count < 10000)
>>>
>>> This feels a bit hacky:
>>>
>>> - The function is dispc_enable_oldi, but the above code also disables 
>>> oldi. We have code in dispc_vp_unprepare() which disables OLDI at the 
>>> moment.
>>>
>>> - The function takes hw_videoport as a parameter, and is designed to 
>>> work on that videoport. The above operates on two videoports. Isn't 
>>> the function also called for hw_videoport +1, which would result in 
>>> reg writes to hw_videoport + 2?
>>>
>>> - No matching code in dispc_vp_unprepare
>>>
>>> Obviously the duplicate mode (I presume that's "cloning") and the 
>>> dual link complicate things here, and I have to say I haven't worked 
>>> with such setups. But I think somehow this should be restructured so 
>>> that common configuration (common to the OLDIs) is done somewhere else.
>>>
>>> I would guess that there are other drivers that support cloning and 
>>> dual mode. Did you have a look how they handle things?
>>
>> Oh, I see now... There's just one dss video port for OLDI, the same as 
>> in am65x, but that single video port is now connected to two OLDI 
>> TXes. And thus this function will only be called for the single video 
>> port.
>> > But... The registers for the second OLDI are part of the second video
>> port (DPI) register block?
> 
> Yes! The config register for the second OLDI TX has been (incorrectly)
> added in the register space for the DPI video port. 'dispc_vp_prepare'
> is the only function calling 'dispc_enable_oldi', and
> 'dispc_enable_oldi' would not be called for hw_videoports = 1 (DPI)
> because of the conditional check.
> 
> Hence, to activate both the OLDI-TXes connected to the OLDI video port,
> I had to use the (hw_videoport + 1) way.

I think this should be highlighted in the comment more clearly. Also, I 
don't think hw_videoport + 1 usage is good. While in this case the 
registers are in vp + 1, it's, in a way, a coincidence.

Instead, have a new variable for the VP register block which contains 
the OLDI TX 1 registers, and just set that to 1 with a comment 
clarifying that the registers for OLDI 1 are in VP1 register block, even 
if OLDI 1 is connected to VP0.

And then use that variable when calling dispc_vp_write().

  Tomi