[PATCH] drm/sun4i: Workaround TCON TOP conflict between DE0 and DE1

[John Watts]

Hi there,

On Tue, Nov 12, 2024 at 10:43:44PM +0530, Parthiban wrote:
> #define TCON_TOP_PORT_DE0_MSK                   GENMASK(1, 0)
> #define TCON_TOP_PORT_DE1_MSK                   GENMASK(5, 4)
> 
> references towards DE0 and DE1 is for DE itself, not the mixers in the
> current implementation.

So the datasheet says it's for DE0/DE1 but the Allwinner driver and mainline
driver assume it's for mixers. There's a conflation between mixer and DE in
this case, especially because everywhere mixer1 is used on the A133 it is
switched to DE1. I'm also unaware of the R40 having two DEs which kind of
confirms this might be a typo. If anyone has actually tested the second output
of this it would help find out if it actually means DE1 or mixer1.

> Handling for mixer0 <-> lcd1 and mixer1 <-> lcd0 also needs to set
> DE2TCON_MUX in de clock, which is missing now.

Hmm. Are you sure? Looking at the Allwinner drivers it has the method
de_top_set_de2tcon_mux in
drivers/video/fbdev/sunxi/disp2/disp/de/lowlevel_v33x/de330/de_top.c
which I think means it's for DE3? But I don't see it called anywhere?

This might be worth discussing in the DE3 patchset.

> sun8i_tcon_top_set_hdmi_src for R40 already sets these values via quirks.
> i.e controlling the port muxing. Also D1 quirks is same as R40. So the
> original changes to make the DE1 point to TVx can also done in this quirk
> without hardcoded value?

In this case I'm using an LCD which isn't HDMI, so I'm not too sure how much
this would help. Having it as a quirk also seems a bit overkill if this is a
general preventative fix, especially since Allwinner doesn't seem to test their
functionality. Relying on it seems like a mistake in this case.

My other thought is that when sun8i_tcon_top_de_config is called it could do
something. But I'm not sure what that something would actually be, given it may
be called twice in an (I assume) unknown order.

Say, if mixer1 is set as TV0 and and mixer0 is set as TV1 we would try to set mixer1
first, see that mixer0 is already set to TV0 then ... error? Even though the
final configuration doesn't have any conflicts.

I was thinking something like this for my next patch:

	/*
	 * Make sure that by default DE0 and DE1 are set to different outputs,
	 * otherwise we get a strange tinting or unusable display on the T113.
	*/
	reg = readl(regs + TCON_TOP_PORT_SEL_REG);
	reg &= ~TCON_TOP_PORT_DE0_MSK;
	reg |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, 0);
	reg &= ~TCON_TOP_PORT_DE1_MSK;
	reg |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, 1);
	writel(reg, regs + TCON_TOP_PORT_SEL_REG);

Perhaps this could be hidden behind a quirk? I would have to check to see which
chips have this behaviour, I'm not sure if it's a bug specific to the T113 or
D1/T113 or R40 too.

Also noting at the top of the file that DE0 and DE1 mean mixer0 and mixer1
might be good to reduce confusion.

What do you think? :)

> Thanks,
> Parthiban

Thanks for your input!

John Watts