Re: [PATCH v2 1/2] dt-bindings: display: rockchip: Add compatible for rk3568 HDMI

[Robin Murphy <robin.murphy@arm.com>]

On 2021-07-14 10:19, Michael Riesch wrote:
> Hello Heiko,
> 
> On 7/13/21 10:49 AM, Heiko Stübner wrote:
>> Hi Michael,
>>
>> Am Dienstag, 13. Juli 2021, 10:44:00 CEST schrieb Michael Riesch:
>>> The HDMI TX block in the RK3568 requires two power supplies, which have
>>> to be enabled in some cases (at least on the RK3568 EVB1 the voltages
>>> VDDA0V9_IMAGE and VCCA1V8_IMAGE are disabled by default). It would be
>>> great if this was considered by the driver and the device tree binding.
>>> I am not sure, though, whether this is a RK3568 specific or
>>> rockchip_dw_hdmi specific thing. Maybe it can even enter the Synopsis DW
>>> HDMI driver.
>>
>> I do remember that this discussion happened many years back already.
>> And yes the supplies are needed for all but back then there was opposition
>> as these are supposedly phy-related supplies, not for the dw-hdmi itself.
>> [There are variants with an external phy, like on the rk3328]
>>
>> See discussion on [0]
>>
>> [0] https://dri-devel.freedesktop.narkive.com/pen2zWo1/patch-v3-1-2-drm-bridge-dw-hdmi-support-optional-supply-regulators
> 
> Thanks for the pointer. My summary of this discussion would be the
> following:
> 
>   - There was no consensus on how to handle the issue. The voltages still
> have to be enabled from the outside of the driver.
>   - Open question: rockchip-specific or general solution? (one may detect
> a tendency towards a rockchip-specific solution)
>   - Open question: separation of the phy from the dw_hdmi IP core?
> 
> First of all, IMHO the driver should enable those voltages, otherwise we
> will have the same discussion again in 5-6 years :-)
> 
> Then, the rockchip,dw-hdmi binding features a property "phys",
> presumably to handle external phys (e.g., for the RK3328). This fact and
> the referenced discussion suggest a rockchip-specific solution.

FWIW I've long thought that cleaning up the phy situation in dw-hdmi 
would be a good idea. It's always seemed a bit sketchy that on RK3328 we 
still validate modes against the tables for the Synopsys phy which isn't 
relevant, and if that does allow a clock rate through that the actual 
phy rejects then things just go horribly wrong and the display breaks.

> In the Rockchip documentation (at least for RK3328, RK3399 and RK3568),
> there are two extra voltages denoted as "HDMI PHY analog power". It
> would be tempting to add the internal phy to the device tree and glue it
> to the dw-hdmi using the "phys" property. However, as pointed out in the
> referenced discussion, the configuration registers of the phy are
> somewhat interleaved with the dw-hdmi registers and a clear separation
> may be tricky.

Conceptually I don't think there's any issue with the HDMI node being 
its own phy provider where appropriate. At the DT level it should simply 
be a case of having both sets of properties, e.g.:

	&hdmi {
		#phy-cells = <0>;
		phys = <&hdmi>;
	};

And at the driver level AFAICS it's pretty much just a case of dw-hdmi 
additionally registering itself as a phy provider if the internal phy is 
present - the only difference then should be that it can end up calling 
back into itself via the common phy API rather than directly via 
internal special-cases.

Robin.

> As a more pragmatic alternative, we could add optional supplies to the
> rockchip,dw-hdmi binding and evaluate the "phys" property. If the latter
> is not specified, the internal phy is used and the supplies must be
> enabled. Would such an approach be acceptable?
> 
> Best regards,
> Michael
> 
>>> On 7/7/21 2:03 PM, Benjamin Gaignard wrote:
>>>> Define a new compatible for rk3568 HDMI.
>>>> This version of HDMI hardware block needs two new clocks hclk_vio and hclk
>>>> to provide phy reference clocks.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>> version 2:
>>>> - Add the clocks needed for the phy.
>>>>
>>>>   .../bindings/display/rockchip/rockchip,dw-hdmi.yaml         | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>>> index 75cd9c686e985..cb8643b3a8b84 100644
>>>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
>>>> @@ -23,6 +23,7 @@ properties:
>>>>         - rockchip,rk3288-dw-hdmi
>>>>         - rockchip,rk3328-dw-hdmi
>>>>         - rockchip,rk3399-dw-hdmi
>>>> +      - rockchip,rk3568-dw-hdmi
>>>>   
>>>>     reg-io-width:
>>>>       const: 4
>>>> @@ -51,8 +52,11 @@ properties:
>>>>             - vpll
>>>>         - enum:
>>>>             - grf
>>>> +          - hclk_vio
>>>> +          - vpll
>>>> +      - enum:
>>>> +          - hclk
>>>>             - vpll
>>>> -      - const: vpll
>>>
>>> The description and documentation of the clocks are somewhat misleading
>>> IMHO. This is not caused by your patches, of course. But maybe this is a
>>> chance to clean them up a bit.
>>>
>>> It seems that the CEC clock is an optional clock of the dw-hdmi driver.
>>> Shouldn't it be documented in the synopsys,dw-hdmi.yaml?
>>>
>>> Also, it would be nice if the clocks hclk_vio and hclk featured a
>>> description in the binding.
>>>
>>> BTW, I am not too familiar with the syntax here, but shouldn't items in
>>> clocks and items in clock-names be aligned (currently, there is a plain
>>> list vs. an enum structure)?
>>>
>>> Best regards,
>>> Michael
>>>
>>>>   
>>>>     ddc-i2c-bus:
>>>>       $ref: /schemas/types.yaml#/definitions/phandle
>>>>
>>>
>>
>>
>>
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>