[PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,
sorry to go back here...

DDC power control is working now (and I now understand that a typo in my work-in-progress
ci20.dts had switched the DDC SDA to active "1" without powering DDC and that may be beyond
what the monitor wanted to see and therefore DDC edid isn't reliable any more... Other
HDMI devices are more probably robust).

I have also tested HPD and could make events passed to user-space by setting poll_mode.

There is one subtle thing in ingenic-drm-drv I do not yet understand:

> Am 08.11.2021 um 10:37 schrieb Paul Cercueil <paul at crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller <hns at goldelico.com> a écrit :
>> Hi Paul,
>>>>>> 
>>>>>> @@ -1274,7 +1319,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>>>>>> 	/* Enable OSD if available */
>>>>>> 	if (soc_info->has_osd)
>>>>>> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>>>>>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>>>>> This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values.
>>>> I think I already commented that I think the driver should also not reset
>>>> previous register values to zero.
>>> You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before.
>>> Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch.
>> Well it is in preparation of setting more bits that are only available for the jz4780.
>> But it will be splitted into its own patch for other reasons - if we ever make osd working...
>>>> If I counted correctly this register has 18 bits which seem to include
>>>> some interrupt masks (which could be initialized somewhere else) and we
>>>> write a constant 0x1.
>>>> Of course most other bits are clearly OSD related (alpha blending),
>>>> i.e. they can have any value (incl. 0) if OSD is disabled. But here we
>>>> enable it. I think there may be missing some setting for the other bits.
>>>> So are you sure, that we can unconditionally reset *all* bits
>>>> except JZ_LCD_OSDC_OSDEN for the jz4780?
>>>> Well I have no experience with OSD being enabled at all. I.e. I have no
>>>> test scenario.

It turns out that the line

		ret = clk_prepare_enable(priv->lcd_clk);

initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 before).

and writing 

		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);

overwrites it with 0x00000001.

This makes the HDMI monitor go/stay black until I write manually 0x5 to
JZ_REG_LCD_OSDC.

This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as well.
Hence overwriting with JZ_LCD_OSDC_OSDEN breaks it.

I didn't notice before because my test setup had some additional private patches
+ reverts and it did not properly revert to regmap_write.

Now the questions:
a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why is it needed?
   Is this a not well documented difference between jz4740/60/70/80?
b) how can clk_prepare_enable() write 0x00010005 to JZ_REG_LCD_OSDC? Bug or feature?
   Something in cgu driver going wrong?
c) what do your SoC or panels do if you write 0x5 to JZ_REG_LCD_OSDC?
d) 0x00010005 also has bit 16 set which is undocumented... But this is a don't care
   according to jz4780 PM.

BR and thanks,
Nikolaus