From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 85690C11F66 for ; Wed, 14 Jul 2021 12:03:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 60C156128C for ; Wed, 14 Jul 2021 12:03:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239287AbhGNMGB (ORCPT ); Wed, 14 Jul 2021 08:06:01 -0400 Received: from foss.arm.com ([217.140.110.172]:34250 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230492AbhGNMGA (ORCPT ); Wed, 14 Jul 2021 08:06:00 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 21097D6E; Wed, 14 Jul 2021 05:03:09 -0700 (PDT) Received: from [10.57.36.240] (unknown [10.57.36.240]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E03B53F694; Wed, 14 Jul 2021 05:03:06 -0700 (PDT) Subject: Re: [PATCH v2 1/2] dt-bindings: display: rockchip: Add compatible for rk3568 HDMI To: Michael Riesch , =?UTF-8?Q?Heiko_St=c3=bcbner?= , Benjamin Gaignard , hjc@rock-chips.com, airlied@linux.ie, daniel@ffwll.ch, robh+dt@kernel.org, algea.cao@rock-chips.com, andy.yan@rock-chips.com Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, kernel@collabora.com References: <20210707120323.401785-1-benjamin.gaignard@collabora.com> <20210707120323.401785-2-benjamin.gaignard@collabora.com> <1bd64284-0a20-12e3-e2e7-19cdfdbf1a25@wolfvision.net> <3865833.Ac65pObt5d@diego> <33532a38-52e6-179a-9ed9-76bbb9618168@wolfvision.net> From: Robin Murphy Message-ID: Date: Wed, 14 Jul 2021 13:02:59 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <33532a38-52e6-179a-9ed9-76bbb9618168@wolfvision.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> --- >>>> 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 >