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

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

Hi Michael,

On 27/05/25 11:32, Michael Walle wrote:
> Hi Aradhya,
> 
> On Mon May 26, 2025 at 4:17 PM CEST, Aradhya Bhatia wrote:
>> Thank you for reviewing and testing the patches! =)
> 
> Thank you for your dedication to bring this feature upstream :)
> 
>> On 26/05/25 15:05, Michael Walle wrote:
>>>
>>>> +static int get_oldi_mode(struct device_node *oldi_tx, int *companion_instance)
>>>> +{
>>>> +	struct device_node *companion;
>>>> +	struct device_node *port0, *port1;
>>>> +	u32 companion_reg;
>>>> +	bool secondary_oldi = false;
>>>> +	int pixel_order;
>>>> +
>>>> +	/*
>>>> +	 * Find if the OLDI is paired with another OLDI for combined OLDI
>>>> +	 * operation (dual-link or clone).
>>>> +	 */
>>>> +	companion = of_parse_phandle(oldi_tx, "ti,companion-oldi", 0);
>>>> +	if (!companion)
>>>> +		/*
>>>> +		 * The OLDI TX does not have a companion, nor is it a
>>>> +		 * secondary OLDI. It will operate independently.
>>>> +		 */
>>>> +		return OLDI_MODE_SINGLE_LINK;
>>>
>>> How is this supposed to work? If I read this code correctly, the
>>> second (companion) port is always reported as SINGLE_LINK if its
>>> device tree node doesn't have a ti,companion-oldi property. But
>>> reading the device tree binding, the companion-old property is only
>>> for the first OLDI port.
>>
>> With this series, the dt-schema for oldi changes a bit as well. Both the
>> OLDIs, primary or secondary, need to pass each other's phandles now.
>> The "ti,companion-oldi" and "ti,secondary-oldi" properties are not
>> mutually exclusive anymore.
> 
> Ok, I thought so. But then you'll have to update the binding doc and
> example (Patch 2/3) ;)
> 

Ah, that's right. The example wasn't updated there. Thank you! =)

>> Something like this.
>>
>> &oldi0 {
>> 	// primary oldi
>> 	ti,companion-oldi = <&oldi1>;
>> };
>>
>>
>> &oldi1 {
>> 	// secondary oldi
>> 	ti,secondary-oldi = true;
>> 	ti,companion-oldi = <&oldi0>;
>> };
>>
>>
>> If there is no companion for any OLDI dt node, then the OLDI TX will be
>> deemed as acting by itself, and in a single-link mode.
> 
> And it's possible to still have these properties and treat them as
> two distinct transmitters? I'm wondering if it's possible to have
> the companion-oldi and secondary-oldi property inside the generic
> SoC dtsi, so you don't have to repeat it in every board dts.
> 
> If I read the code correctly, the panel has to have the even and odd
> pixel properties to be detected as dual-link. Correct? Thus it would
> be possible to have
> 
> oldi0: oldi@0 {
>  	ti,companion-oldi = <&oldi1>;
> };
> 
> oldi1: oldi@1 {
>  	ti,secondary-oldi;
>  	ti,companion-oldi = <&oldi0>;
> };
> 
> in the soc.dtsi and in a board dts:
> 
> panel {
> 	port {
> 		remote-endpoint = <&oldi0>;
> 	};
> };

In this case, the secondary OLDI (oldi1) would remain disabled from
soc.dtsi.

I gave this a quick try. Turns out, of_parse_phandle() is not able to
return an error when primary OLDI tries to find a companion -- which is
important for the driver to detect an absence of any secondary OLDI.

Since the driver code registers a companion for primary OLDI, and
further does not find the "dual-lvds-{odd,even}-pixels" properties,
the driver ends up trying for a Clone Mode.

So, for single-link , we'd have to actively delete the "companion-oldi"
property, in the board.dts/panel.dtso.


But, say, the disabled-node's phandle parse is circumvented, somehow,
and we don't need to delete the property explicitly.

There would still be one concern, I am afraid.

In AM67A DSS (future scope at the moment), the 2 OLDIs can act
independently. Like a 2x Independent Single-Link. Both the OLDI dt nodes
will be enabled.

So, if the soc.dtsi has them already connected, then the
board.dts/panel.dtso would still need to explicitly delete those
properties to get the 2 OLDI TXes to work independently.

-- 
Regards
Aradhya


> 
> Or with a dual link panel:
> 
> dualpanel {
> 	ports {
> 		port@0 {
> 			dual-lvds-odd-pixels;
> 			remote-endpoint = <&oldi0>;
> 		};
> 
> 		port@1 {
> 			dual-lvds-even-pixels;
> 			remote-endpoint = <&oldi1>;
> 		};
> 	};
> };
> 
>>>
>>> FWIW, I've tested this series and I get twice the clock rate as
>>> expected and the second link is reported as "OLDI_MODE_SINGLE_LINK".
>>> I'll dig deeper into this tomorrow.
>>>
>>
>> I was able to reproduce this behavior as you mention when the second
>> oldi dt does not have a companion-oldi property.
>>
>> However, upon analysis, I realize that even having the correct dt as I
>> mention above, will fall into another bug in the code and fail during
>> the OLDI init.
>>
>> Unfortunately, two wrongs in my setup yesterday caused my testing to
>> pass!
>>
>> I will post another revision, if you want to hold out on debugging
>> further!
> 
> Sure!
> 
> -michael