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

[Parthiban <parthiban@linumiz.com>]

On 11/8/24 6:59 PM, John Watts wrote:
> On Fri, Nov 08, 2024 at 11:53:57AM +0000, Andre Przywara wrote:
>> Hi John,
> 
> Hi Andre!
> 
>> Can you say *why* this patch is needed? Is there something broken that
>> needs fixing? Where does this show and why wasn't this a problem before?
> 
> Oops, that's a good point. There is currently a bug where the LCD output will
> be tinted. I have full context here which I should have probably linked in the
> patch description:
> 
> https://lore.kernel.org/linux-sunxi/Zn8GVkpwXwhaUFno@titan/T/#u
> 
>> To be honest, given the isolation on this patch, I'd rather wait for this
>> full fledged solution, especially if there is no pressing need (see above).
> 
> I'd be interested to hear if that's still the wanted solution given the link
> above. This currently blocks many people from having working LCD output.
> 
> Doing it the proper way might be overkill for now unless someone deliberately
> tries to run two DEs to the same output. I haven't seen this use case.
> 
> Allwinner kernel fork initially sets them up to values like these then makes
> sure both DEs can't be set to the same TCON.
> 
>>> -	writel(0, regs + TCON_TOP_PORT_SEL_REG);
>>> +	writel(0x20, regs + TCON_TOP_PORT_SEL_REG);
>>
>> Sorry, but that looks weird:
>> First, please explain the 0x20. Is it bit 5? If yes, what does that bit
>> mean? The commit message suggests you know that?
>>
>> And second: the comment above clearly states that those two writes just
>> *clear* some registers, to have some sane base line. So please adjust this
>> comment, and copy in some of the rationale from the commit message.
>> Explaining things in the commit message is good (so thanks for that!), but
>> having at least some terse technical explanations near the code, in a
>> comment, is better.
> 
> Bit 5 is value 3 of TCON_TOP_PORT_DE1_MSK. The R40 datasheet explains the
> values of both masks as follows:
> 
> 00: TCON_LCD0
> 01: TCON_LCD1
> 10: TCON_TV0
> 11: TCON_TV1
> 
> So this sets DE1's input to DE0.

To add, 0x20 will be DE0 <--> LCD0 and DE1 <--> TV0. Below note (copied from
R40) states the priority of the DE selection, which fails to work? Not sure,
may be disabling CORE1_SCLK_GATE and CORE1_HCLK_GATE in de2-clk helps.

With A133 following the same as T113 with single mixer without TV, still
sets 0x20 in vendor kernel.

copied from R40:
Note: The priority of DE0 is higher than DE1.
If TCON_LCD0 selects DE0 and DE1 as source at the same time, then
DE0 will be used for the source of TCON_LCD0.

Thanks,
Parthiban

> 
>>
>> Cheers,
>> Andre
> 
> Thanks,
> John Watts
>