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
