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

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

Hi Michael,

Thank you for reviewing and testing the patches! =)

On 26/05/25 15:05, Michael Walle wrote:
> Hi Aradhya,
> 
>> +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.

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.


> 
> 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!

Thank you for reporting this!

--
Regards
Aradhya

> 
>> +
>> +	if (of_property_read_u32(companion, "reg", &companion_reg))
>> +		return OLDI_MODE_UNSUPPORTED;
>> +
>> +	if (companion_reg > (TIDSS_MAX_OLDI_TXES - 1))
>> +		/* Invalid companion OLDI reg value. */
>> +		return OLDI_MODE_UNSUPPORTED;
>> +
>> +	*companion_instance = (int)companion_reg;
>> +
>> +	if (of_property_read_bool(oldi_tx, "ti,secondary-oldi"))
>> +		secondary_oldi = true;
>> +
>> +	/*
>> +	 * We need to work out if the sink is expecting us to function in
>> +	 * dual-link mode. We do this by looking at the DT port nodes, the
>> +	 * OLDI TX ports are connected to. If they are marked as expecting
>> +	 * even pixels and odd pixels, then we need to enable dual-link.
>> +	 */
>> +	port0 = of_graph_get_port_by_id(oldi_tx, 1);
>> +	port1 = of_graph_get_port_by_id(companion, 1);
>> +	pixel_order = drm_of_lvds_get_dual_link_pixel_order(port0, port1);
>> +	of_node_put(port0);
>> +	of_node_put(port1);
>> +	of_node_put(companion);
>> +
>> +	switch (pixel_order) {
>> +	case -EINVAL:
>> +		/*
>> +		 * The dual-link properties were not found in at least
>> +		 * one of the sink nodes. Since 2 OLDI ports are present
>> +		 * in the DT, it can be safely assumed that the required
>> +		 * configuration is Clone Mode.
>> +		 */
>> +		return (secondary_oldi ? OLDI_MODE_CLONE_SECONDARY_SINGLE_LINK :
>> +					 OLDI_MODE_CLONE_SINGLE_LINK);
>> +
>> +	case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>> +		return (secondary_oldi ? OLDI_MODE_SECONDARY_DUAL_LINK :
>> +					 OLDI_MODE_DUAL_LINK);
>> +
>> +	/* Unsupported OLDI Modes */
>> +	case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>> +	default:
>> +		return OLDI_MODE_UNSUPPORTED;
>> +	}
>> +}