Received: by 2002:a05:6a10:f3d0:0:0:0:0 with SMTP id a16csp326230pxv; Wed, 14 Jul 2021 05:04:14 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwevYHMYYSNLnoYEK7YNFmSszRyvnqgYHMbAgToMhNc2FA6Cy6asScnq7SaMC6RYvQwISK5 X-Received: by 2002:a92:700b:: with SMTP id l11mr6405973ilc.48.1626264254483; Wed, 14 Jul 2021 05:04:14 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1626264254; cv=none; d=google.com; s=arc-20160816; b=xVnVQd0xh1yVE7aQgRP5djNvrUtQDWA1n47B4JK8+EX0DbjTq3Z2+hpXZmSVFDr3aK 3cnAQ7GniK0gUJwiBz8DAUck8M3Y+fqtfKFdfP/2MDHnNWHFIy8SpnAA8bP5mEFud/f2 ZKGbKVpSp9dN+11c8vDGBeSC/nwf3s2FDttCcY5O6kgwFH/IBw7m1txef5SjSUKvxEWE UIHr+gCq+O86jwuVIsjMOXmc9ii4BwYN9++WWcjF9UDmrPXi6jAjkSlPiw+JUOdjOwhN M2LKJ5mCyBcy+2uFSU7k2uJrBkPHXVXpLXKwt2WUVgfvEju0XAGGHGTyF1xbf6jTlFl4 Syjg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=M5Vh7d9TvydkWQAHOvFi+Bq1nx/42JvqQ49BSsYsec4=; b=Y5g1BMeAM5A6T6H+BEkE70w8S734mN7XWUT9s+fJj8mVCm9sXgC6tk1zasilNvqCxT txFWDCxPnASdvt5dwE9vH3MqKUPKAxqyzAxgZsscgOErn0mwBgn4TeJBxK7bPnIC9aYw FZJnkht0EwWG0cj43CdHWbf3IAqtdpPkRTup7BGolS4JLojYSATc+IN02VCptbRoerML NZ6ZZsXj3k129hY/0CZxRCmjbI8UzoNx8Q3UGayo9zgo78wnbHqFT2IbmzQy+KEmXfrv yY5LJ3P9mcHy7aiq+9tmHXKKiDTT/mQVcqaxyADYno5cNCqn0lAHyLpFp2q6QwzCT5TX JX/g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id g18si2447015jat.28.2021.07.14.05.04.00; Wed, 14 Jul 2021 05:04:14 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239247AbhGNMGB (ORCPT + 99 others); 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 >