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

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

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) ;)

> 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>;
	};
};

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]