Message ID | 20221111092457.10546-3-johan+linaro@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp637481wru; Fri, 11 Nov 2022 01:28:25 -0800 (PST) X-Google-Smtp-Source: AA0mqf4PokZvZilG6ly0WyGKv2eOdznTCv7ZwmoGLkDbZOAuLkpscaqvt4lA3Ef89gGcdCtVBKUh X-Received: by 2002:a50:9ee3:0:b0:460:e19c:15a3 with SMTP id a90-20020a509ee3000000b00460e19c15a3mr751586edf.252.1668158905439; Fri, 11 Nov 2022 01:28:25 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668158905; cv=none; d=google.com; s=arc-20160816; b=B8sNsfYX/YKz/OJ5QvA+XxUBk7+xKNjaaFfjBRzAvgQaOpsJdHafPOO1FZLy1pYrdC oIv6TEQsLyHjKfqNjg0hA5vr0eek84WwSL13cfwe2Tur3ToZuRC7/QQgN//hFb0hQHlS JuEt/GQAkqu0u6U8d8bIeKdNi4nqhAADiN1QlZb34DahzpF7UvPgWqZiBDraShi90UXU +NsJ9h+ZPn+K8qXK5nT2yV8MHD02dbAmNACVeO/NRofXb+t7Yqm1jVNl982Ts2GQ/X1i Y6jZ70S1onrOxkUfwltGQ8JOpiVM0Q7bZcMfwprxacFoT9Mt1PqnkFpHPqu9a8UUiSw8 hLqA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=6ckIRUoxOH5qZy8GuSrVbeR1lhUTiUIM/S9BRN6cHSY=; b=DqDCwI05woGR2lc7VyxfvPZNcM1Vx94wMzNxTBlWtI57iqCLDr5AD95GFJRWVe36h9 5uGeyPA/KZz0PBLEZi3SfctMfgNhIr5AurGH7Z0Fetdb8K42NyNGtzsjkCbgQXUjtXyd grNYHKXj+AuCh3/qn8wNWC38NiM9DNkkSpFj7tWgSqbncNk0THB5hKk00/ev22etS1dt q0BLF8DmlpscRePm8y4FxXIP08RWX9b5dAmJTwgHQdVT9dbSjevBJm1FLNLkSjWlGfnG 5aQ4nDPzB0c2/OPQ8JygkdS3EepOFNgF9wrOVwE2Emqy3RzQZ4WVN8BBjPVOYJgiMmPn v44g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=i9NN+CbC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id r19-20020aa7da13000000b00462e7873c10si1635767eds.337.2022.11.11.01.28.01; Fri, 11 Nov 2022 01:28:25 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=i9NN+CbC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233280AbiKKJ1D (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Fri, 11 Nov 2022 04:27:03 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42026 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233575AbiKKJ0T (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 11 Nov 2022 04:26:19 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 929E4787B2; Fri, 11 Nov 2022 01:26:07 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 4E2ECB824AE; Fri, 11 Nov 2022 09:26:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AED87C4347C; Fri, 11 Nov 2022 09:26:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1668158764; bh=xvw8f4uLya1tYtW/a3ebgxqOAIDX0JRk6onsrY8M4CY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=i9NN+CbCyI6znH470xG2xd0oqyMZ/X5xVlrsNwCcNsYH/esAEAexnw2FXmEqXSG+d e+5LKXyOK2KDKbSgERWDCcnUY3837mr8lqk2+g+svxt8ss7XCn9xIn0PcuCLn6xKjd rs8UMQmxA0S7XaYOCcfg6vEqFv8MjUaF8zhx8InAxcX4CHmmenATzdJer/4m2b73Os RKuw74sfDiegGjhrf9FNQRuycT7XzHNrtJV+5pts4HQJGRam6w64qKXYMmUH/3+cTG +XfK8kTL8wRxo6ucLKjuRNLgoGNlnVaCNLgkqa5SSLDWCZSeG4OQtRp7CqEr4Zxy0g xz1umuv9SfhoA== Received: from johan by xi.lan with local (Exim 4.94.2) (envelope-from <johan+linaro@kernel.org>) id 1otQI4-0002kr-LX; Fri, 11 Nov 2022 10:25:36 +0100 From: Johan Hovold <johan+linaro@kernel.org> To: Vinod Koul <vkoul@kernel.org> Cc: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org> Subject: [PATCH 02/14] dt-bindings: phy: qcom,qmp-usb3-dp: fix sc8280xp bindings Date: Fri, 11 Nov 2022 10:24:45 +0100 Message-Id: <20221111092457.10546-3-johan+linaro@kernel.org> X-Mailer: git-send-email 2.37.4 In-Reply-To: <20221111092457.10546-1-johan+linaro@kernel.org> References: <20221111092457.10546-1-johan+linaro@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749191392795071228?= X-GMAIL-MSGID: =?utf-8?q?1749191392795071228?= |
Series |
phy: qcom-qmp-combo: fix sc8280xp binding (set 3/3)
|
|
Commit Message
Johan Hovold
Nov. 11, 2022, 9:24 a.m. UTC
The current QMP USB3-DP PHY bindings are based on the original MSM8996
binding which provided multiple PHYs per IP block and these in turn were
described by child nodes.
The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
if some resources are only used by either the USB or DP part of the
device there is no real benefit in describing these resources in child
nodes.
The original MSM8996 binding also ended up describing the individual
register blocks as belonging to either the wrapper node or the PHY child
nodes.
This is an unnecessary level of detail which has lead to problems when
later IP blocks using different register layouts have been forced to fit
the original mould rather than updating the binding. The bindings are
arguable also incomplete as they only the describe register blocks used
by the current Linux drivers (e.g. does not include the PCS LANE
registers).
This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
registers are used by both the USB3 and DP parts of the PHY (and where
the USB4 part of the PHY was not covered by the binding at all). Notably
there are also no DP "RX" (sic) registers as described by the current
bindings and the DP "PCS" region is really a set of DP_PHY registers.
Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
further bindings can be based on.
Note that the binding uses a PHY type index to access either the USB3 or
DP part of the PHY and that this can later be used also for the USB4
part if needed.
Similarly, the clock inputs and outputs can later be extended to support
USB4.
Also note that the current binding is simply removed instead of being
deprecated as it was only recently merged and would not allow for
supporting DP mode.
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
.../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml | 12 --
.../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 111 ++++++++++++++++++
2 files changed, 111 insertions(+), 12 deletions(-)
create mode 100644 Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
Comments
On Fri, Nov 11, 2022 at 10:24:45AM +0100, Johan Hovold wrote: > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml > @@ -0,0 +1,111 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm QMP USB4-USB3-DP PHY controller (SC8280XP) > + > +maintainers: > + - Vinod Koul <vkoul@kernel.org> > + > +description: > + The QMP PHY controller supports physical layer functionality for a number of > + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. > + > + See also: > + - include/dt-bindings/dt-bindings/phy/phy.h This trips up the binding checker which I failed to rerun after adding this reference. Apparently I missed adding a literal block marker '|' to the description. Will add in v2. Johan
On Fri, 11 Nov 2022 10:24:45 +0100, Johan Hovold wrote: > The current QMP USB3-DP PHY bindings are based on the original MSM8996 > binding which provided multiple PHYs per IP block and these in turn were > described by child nodes. > > The QMP USB3-DP PHY block provides a single multi-protocol PHY and even > if some resources are only used by either the USB or DP part of the > device there is no real benefit in describing these resources in child > nodes. > > The original MSM8996 binding also ended up describing the individual > register blocks as belonging to either the wrapper node or the PHY child > nodes. > > This is an unnecessary level of detail which has lead to problems when > later IP blocks using different register layouts have been forced to fit > the original mould rather than updating the binding. The bindings are > arguable also incomplete as they only the describe register blocks used > by the current Linux drivers (e.g. does not include the PCS LANE > registers). > > This is specifically true for later USB4-USB3-DP QMP PHYs where the TX > registers are used by both the USB3 and DP parts of the PHY (and where > the USB4 part of the PHY was not covered by the binding at all). Notably > there are also no DP "RX" (sic) registers as described by the current > bindings and the DP "PCS" region is really a set of DP_PHY registers. > > Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which > further bindings can be based on. > > Note that the binding uses a PHY type index to access either the USB3 or > DP part of the PHY and that this can later be used also for the USB4 > part if needed. > > Similarly, the clock inputs and outputs can later be extended to support > USB4. > > Also note that the current binding is simply removed instead of being > deprecated as it was only recently merged and would not allow for > supporting DP mode. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml | 12 -- > .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 111 ++++++++++++++++++ > 2 files changed, 111 insertions(+), 12 deletions(-) > create mode 100644 Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml:16:11: [error] syntax error: mapping values are not allowed here (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.example.dts' Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml:16:11: mapping values are not allowed here make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml:16:11: mapping values are not allowed here /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml: ignoring, error parsing file make: *** [Makefile:1492: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 11/11/2022 10:24, Johan Hovold wrote: > The current QMP USB3-DP PHY bindings are based on the original MSM8996 > binding which provided multiple PHYs per IP block and these in turn were > described by child nodes. > Thank you for your patch. There is something to discuss/improve. > + > +maintainers: > + - Vinod Koul <vkoul@kernel.org> Maybe you want to add also yourself? > + > +description: > + The QMP PHY controller supports physical layer functionality for a number of > + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. > + > + See also: > + - include/dt-bindings/dt-bindings/phy/phy.h > + > +properties: > + compatible: > + enum: > + - qcom,sc8280xp-qmp-usb43dp-phy > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 4 > + > + clock-names: > + items: > + - const: aux > + - const: ref > + - const: com_aux > + - const: usb3_pipe > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 2 > + > + reset-names: > + items: > + - const: phy > + - const: common > + > + vdda-phy-supply: true > + > + vdda-pll-supply: true > + > + "#clock-cells": > + const: 1 > + > + clock-output-names: > + items: > + - const: usb3_pipe > + - const: dp_link > + - const: dp_vco_div Why defining here fixed names? The purpose of this field is to actually allow customizing these - at least in most cases. If these have to be fixed, then driver should just instantiate these clocks with such names, right? Best regards, Krzysztof
On 11/11/2022 12:24, Johan Hovold wrote: > The current QMP USB3-DP PHY bindings are based on the original MSM8996 > binding which provided multiple PHYs per IP block and these in turn were > described by child nodes. > > The QMP USB3-DP PHY block provides a single multi-protocol PHY and even > if some resources are only used by either the USB or DP part of the > device there is no real benefit in describing these resources in child > nodes. > > The original MSM8996 binding also ended up describing the individual > register blocks as belonging to either the wrapper node or the PHY child > nodes. > > This is an unnecessary level of detail which has lead to problems when > later IP blocks using different register layouts have been forced to fit > the original mould rather than updating the binding. The bindings are > arguable also incomplete as they only the describe register blocks used > by the current Linux drivers (e.g. does not include the PCS LANE > registers). > > This is specifically true for later USB4-USB3-DP QMP PHYs where the TX > registers are used by both the USB3 and DP parts of the PHY (and where > the USB4 part of the PHY was not covered by the binding at all). Notably > there are also no DP "RX" (sic) registers as described by the current > bindings and the DP "PCS" region is really a set of DP_PHY registers. > > Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which > further bindings can be based on. > > Note that the binding uses a PHY type index to access either the USB3 or > DP part of the PHY and that this can later be used also for the USB4 > part if needed. > > Similarly, the clock inputs and outputs can later be extended to support > USB4. > > Also note that the current binding is simply removed instead of being > deprecated as it was only recently merged and would not allow for > supporting DP mode. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml | 12 -- > .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 111 ++++++++++++++++++ > 2 files changed, 111 insertions(+), 12 deletions(-) > create mode 100644 Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml > > diff --git a/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml > index 50b1fce530d5..2f4a419197a8 100644 > --- a/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml > @@ -23,7 +23,6 @@ properties: > - qcom,sc7180-qmp-usb3-dp-phy > - qcom,sc7280-qmp-usb3-dp-phy > - qcom,sc8180x-qmp-usb3-dp-phy > - - qcom,sc8280xp-qmp-usb43dp-phy > - qcom,sdm845-qmp-usb3-dp-phy > - qcom,sm8250-qmp-usb3-dp-phy > reg: > @@ -169,17 +168,6 @@ required: > > additionalProperties: false > > -allOf: > - - if: > - properties: > - compatible: > - contains: > - enum: > - - qcom,sc8280xp-qmp-usb43dp-phy > - then: > - required: > - - power-domains > - > examples: > - | > #include <dt-bindings/clock/qcom,gcc-sdm845.h> > diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml > new file mode 100644 > index 000000000000..bd04150acee4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml > @@ -0,0 +1,111 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm QMP USB4-USB3-DP PHY controller (SC8280XP) > + > +maintainers: > + - Vinod Koul <vkoul@kernel.org> > + > +description: > + The QMP PHY controller supports physical layer functionality for a number of > + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. > + > + See also: > + - include/dt-bindings/dt-bindings/phy/phy.h > + > +properties: > + compatible: > + enum: > + - qcom,sc8280xp-qmp-usb43dp-phy > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 4 > + > + clock-names: > + items: > + - const: aux > + - const: ref > + - const: com_aux > + - const: usb3_pipe > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 2 > + > + reset-names: > + items: > + - const: phy > + - const: common > + > + vdda-phy-supply: true > + > + vdda-pll-supply: true > + > + "#clock-cells": > + const: 1 > + > + clock-output-names: > + items: > + - const: usb3_pipe > + - const: dp_link > + - const: dp_vco_div > + > + "#phy-cells": > + const: 1 > + description: | > + PHY index > + - PHY_TYPE_USB3 > + - PHY_TYPE_DP I'm stepping on Rob's and Krzysztof's ground here, but it might be more logical and future proof to use indices instead of phy types. Just for my understanding, would USB4 support add another qserdes+tx/rx construct or would it be the same USB3 register space? > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - power-domains > + - resets > + - reset-names > + - vdda-phy-supply > + - vdda-pll-supply > + - "#clock-cells" > + - clock-output-names > + - "#phy-cells" > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,gcc-sc8280xp.h> > + > + phy@88eb000 { > + compatible = "qcom,sc8280xp-qmp-usb43dp-phy"; > + reg = <0x088eb000 0x4000>; > + > + clocks = <&gcc GCC_USB3_PRIM_PHY_AUX_CLK>, > + <&gcc GCC_USB4_EUD_CLKREF_CLK>, > + <&gcc GCC_USB3_PRIM_PHY_COM_AUX_CLK>, > + <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>; > + clock-names = "aux", "ref", "com_aux", "usb3_pipe"; > + > + power-domains = <&gcc USB30_PRIM_GDSC>; > + > + resets = <&gcc GCC_USB3_PHY_PRIM_BCR>, > + <&gcc GCC_USB4_DP_PHY_PRIM_BCR>; > + reset-names = "phy", "common"; > + > + vdda-phy-supply = <&vreg_l9d>; > + vdda-pll-supply = <&vreg_l4d>; > + > + #clock-cells = <1>; > + clock-output-names = "usb3_pipe", "dp_link", "dp_vco_div"; > + > + #phy-cells = <1>; > + };
On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: > On 11/11/2022 10:24, Johan Hovold wrote: > > The current QMP USB3-DP PHY bindings are based on the original MSM8996 > > binding which provided multiple PHYs per IP block and these in turn were > > described by child nodes. > > > > Thank you for your patch. There is something to discuss/improve. > > > > + > > +maintainers: > > + - Vinod Koul <vkoul@kernel.org> > > Maybe you want to add also yourself? Due to the lack of public documentation for these platforms and the amount of work involved in effectively reverse-engineering the corresponding details from random vendor-kernel trees, it's probably best to leave maintainence with Vinod who at least has access to some documentation. > > + > > +description: > > + The QMP PHY controller supports physical layer functionality for a number of > > + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. > > + > > + See also: > > + - include/dt-bindings/dt-bindings/phy/phy.h > > + > > +properties: > > + compatible: > > + enum: > > + - qcom,sc8280xp-qmp-usb43dp-phy > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 4 > > + > > + clock-names: > > + items: > > + - const: aux > > + - const: ref > > + - const: com_aux > > + - const: usb3_pipe > > + > > + power-domains: > > + maxItems: 1 > > + > > + resets: > > + maxItems: 2 > > + > > + reset-names: > > + items: > > + - const: phy > > + - const: common > > + > > + vdda-phy-supply: true > > + > > + vdda-pll-supply: true > > + > > + "#clock-cells": > > + const: 1 > > + > > + clock-output-names: > > + items: > > + - const: usb3_pipe > > + - const: dp_link > > + - const: dp_vco_div > > Why defining here fixed names? The purpose of this field is to actually > allow customizing these - at least in most cases. If these have to be > fixed, then driver should just instantiate these clocks with such names, > right? I'm only using these names as documentation of the indexes. The driver doesn't use these names, but that's a Linux-specific implementation detail. I noticed that several bindings leave the clock indexes unspecified, or have header files defining some or all of them. I first added a QMP header but that seemed like overkill, especially if we'd end up with one header per SoC (cf. the GCC headers) due to (known and potential) platform differences. On the other hand reproducing this list in each node is admittedly a bit redundant. Shall I add back a shared header for all PHYs handled by this driver (another implementation detail) even if this could eventually lead to describing clocks not supported by a particular SoC (so such constraints would still need to be described by the binding somehow): /* QMP clocks */ #define QMP_USB3_PIPE_CLK 0 #define QMP_DP_LINK_CLK 1 #define QMP_DP_VCO_DIV_CLK 2 Note that for SC8280XP this list may later get extended with clocks for USB4 (once we understand how to use them), but those would not apply to the older USB3-DP PHYs, so we'd still need to describe that in the binding somehow. Johan
On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote: > On 11/11/2022 12:24, Johan Hovold wrote: > > The current QMP USB3-DP PHY bindings are based on the original MSM8996 > > binding which provided multiple PHYs per IP block and these in turn were > > described by child nodes. > > > > The QMP USB3-DP PHY block provides a single multi-protocol PHY and even > > if some resources are only used by either the USB or DP part of the > > device there is no real benefit in describing these resources in child > > nodes. > > > > The original MSM8996 binding also ended up describing the individual > > register blocks as belonging to either the wrapper node or the PHY child > > nodes. > > > > This is an unnecessary level of detail which has lead to problems when > > later IP blocks using different register layouts have been forced to fit > > the original mould rather than updating the binding. The bindings are > > arguable also incomplete as they only the describe register blocks used > > by the current Linux drivers (e.g. does not include the PCS LANE > > registers). > > > > This is specifically true for later USB4-USB3-DP QMP PHYs where the TX > > registers are used by both the USB3 and DP parts of the PHY (and where > > the USB4 part of the PHY was not covered by the binding at all). Notably > > there are also no DP "RX" (sic) registers as described by the current > > bindings and the DP "PCS" region is really a set of DP_PHY registers. > > > > Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which > > further bindings can be based on. > > > > Note that the binding uses a PHY type index to access either the USB3 or > > DP part of the PHY and that this can later be used also for the USB4 > > part if needed. > > > > Similarly, the clock inputs and outputs can later be extended to support > > USB4. > > > > Also note that the current binding is simply removed instead of being > > deprecated as it was only recently merged and would not allow for > > supporting DP mode. > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > + "#clock-cells": > > + const: 1 > > + > > + clock-output-names: > > + items: > > + - const: usb3_pipe > > + - const: dp_link > > + - const: dp_vco_div > > + > > + "#phy-cells": > > + const: 1 > > + description: | > > + PHY index > > + - PHY_TYPE_USB3 > > + - PHY_TYPE_DP > > I'm stepping on Rob's and Krzysztof's ground here, but it might be more > logical and future proof to use indices instead of phy types. Why would that be more future-proof? I initially added defines for these indexes to a QMP header, but noticed that we already have PHY drivers that use the PHY types for this. So there's already a precedent for this and I didn't see any real benefit to adding multiple per-SoC defines for the same thing. > Just for my understanding, would USB4 support add another qserdes+tx/rx > construct or would it be the same USB3 register space? The TX/RX registers are shared by the all three parts of the PHY (USB4, USB3, DP), while USB4 has two dedicated sets of PLL (serdes) and PCS registers. Johan
On 14/11/2022 14:27, Johan Hovold wrote: > On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >> On 11/11/2022 10:24, Johan Hovold wrote: >>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 >>> binding which provided multiple PHYs per IP block and these in turn were >>> described by child nodes. >>> >> >> Thank you for your patch. There is something to discuss/improve. >> >> >>> + >>> +maintainers: >>> + - Vinod Koul <vkoul@kernel.org> >> >> Maybe you want to add also yourself? > > Due to the lack of public documentation for these platforms and the > amount of work involved in effectively reverse-engineering the > corresponding details from random vendor-kernel trees, it's probably > best to leave maintainence with Vinod who at least has access to some > documentation. > >>> + >>> +description: >>> + The QMP PHY controller supports physical layer functionality for a number of >>> + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. >>> + >>> + See also: >>> + - include/dt-bindings/dt-bindings/phy/phy.h >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - qcom,sc8280xp-qmp-usb43dp-phy >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 4 >>> + >>> + clock-names: >>> + items: >>> + - const: aux >>> + - const: ref >>> + - const: com_aux >>> + - const: usb3_pipe >>> + >>> + power-domains: >>> + maxItems: 1 >>> + >>> + resets: >>> + maxItems: 2 >>> + >>> + reset-names: >>> + items: >>> + - const: phy >>> + - const: common >>> + >>> + vdda-phy-supply: true >>> + >>> + vdda-pll-supply: true >>> + >>> + "#clock-cells": >>> + const: 1 >>> + >>> + clock-output-names: >>> + items: >>> + - const: usb3_pipe >>> + - const: dp_link >>> + - const: dp_vco_div >> >> Why defining here fixed names? The purpose of this field is to actually >> allow customizing these - at least in most cases. If these have to be >> fixed, then driver should just instantiate these clocks with such names, >> right? > > I'm only using these names as documentation of the indexes. The driver What do you mean by documentation of indexes? You require these specific entries and do not allow anything else. > doesn't use these names, but that's a Linux-specific implementation > detail. > > I noticed that several bindings leave the clock indexes unspecified, or > have header files defining some or all of them. I first added a QMP > header but that seemed like overkill, especially if we'd end up with > one header per SoC (cf. the GCC headers) due to (known and potential) > platform differences. Headers for the names? I do not recall such but that does not seem right. > > On the other hand reproducing this list in each node is admittedly a bit > redundant. > > Shall I add back a shared header for all PHYs handled by this driver > (another implementation detail) even if this could eventually lead to > describing clocks not supported by a particular SoC (so such constraints > would still need to be described by the binding somehow): > > /* QMP clocks */ > #define QMP_USB3_PIPE_CLK 0 > #define QMP_DP_LINK_CLK 1 > #define QMP_DP_VCO_DIV_CLK 2 What are these about? To remind - we talk about names of clocks this device creates. The output names. Whatever IDs you have are not related to the names. Best regards, Krzysztof
On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: > On 14/11/2022 14:27, Johan Hovold wrote: > > On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: > >> On 11/11/2022 10:24, Johan Hovold wrote: > >>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 > >>> binding which provided multiple PHYs per IP block and these in turn were > >>> described by child nodes. > >>> + "#clock-cells": > >>> + const: 1 > >>> + > >>> + clock-output-names: > >>> + items: > >>> + - const: usb3_pipe > >>> + - const: dp_link > >>> + - const: dp_vco_div > >> > >> Why defining here fixed names? The purpose of this field is to actually > >> allow customizing these - at least in most cases. If these have to be > >> fixed, then driver should just instantiate these clocks with such names, > >> right? > > > > I'm only using these names as documentation of the indexes. The driver > > What do you mean by documentation of indexes? You require these specific > entries and do not allow anything else. I'm using this property as documentation of the valid indexes that can be used when referring to clocks provided by this device. There are currently three and the mapping is described by the 'clock-output-names' property. > > doesn't use these names, but that's a Linux-specific implementation > > detail. > > > > I noticed that several bindings leave the clock indexes unspecified, or > > have header files defining some or all of them. I first added a QMP > > header but that seemed like overkill, especially if we'd end up with > > one header per SoC (cf. the GCC headers) due to (known and potential) > > platform differences. > > Headers for the names? I do not recall such but that does not seem right. Headers for the indexes. > > > > On the other hand reproducing this list in each node is admittedly a bit > > redundant. > > > > Shall I add back a shared header for all PHYs handled by this driver > > (another implementation detail) even if this could eventually lead to > > describing clocks not supported by a particular SoC (so such constraints > > would still need to be described by the binding somehow): > > > > /* QMP clocks */ > > #define QMP_USB3_PIPE_CLK 0 > > #define QMP_DP_LINK_CLK 1 > > #define QMP_DP_VCO_DIV_CLK 2 > > What are these about? To remind - we talk about names of clocks this > device creates. The output names. Whatever IDs you have are not related > to the names. As I mentioned above, this is not about the names that Linux gives to its representation of these clocks. Its just about defining the valid indexes in the binding. If you think that that using 'clock-output-names' for this is a bit too unconventional, I can add back the header with defines like the above instead. Note that the clock schema has: clock-output-names: description: | Recommended to be a list of strings of clock output signal names indexed by the first cell in the clock specifier. However, the meaning of clock-output-names is domain specific to the clock provider, ... Johan
On 14/11/2022 17:18, Johan Hovold wrote: > On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: >> On 14/11/2022 14:27, Johan Hovold wrote: >>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >>>> On 11/11/2022 10:24, Johan Hovold wrote: >>>>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 >>>>> binding which provided multiple PHYs per IP block and these in turn were >>>>> described by child nodes. > >>>>> + "#clock-cells": >>>>> + const: 1 >>>>> + >>>>> + clock-output-names: >>>>> + items: >>>>> + - const: usb3_pipe >>>>> + - const: dp_link >>>>> + - const: dp_vco_div >>>> >>>> Why defining here fixed names? The purpose of this field is to actually >>>> allow customizing these - at least in most cases. If these have to be >>>> fixed, then driver should just instantiate these clocks with such names, >>>> right? >>> >>> I'm only using these names as documentation of the indexes. The driver >> >> What do you mean by documentation of indexes? You require these specific >> entries and do not allow anything else. > > I'm using this property as documentation of the valid indexes that can > be used when referring to clocks provided by this device. > > There are currently three and the mapping is described by the > 'clock-output-names' property. > >>> doesn't use these names, but that's a Linux-specific implementation >>> detail. >>> >>> I noticed that several bindings leave the clock indexes unspecified, or >>> have header files defining some or all of them. I first added a QMP >>> header but that seemed like overkill, especially if we'd end up with >>> one header per SoC (cf. the GCC headers) due to (known and potential) >>> platform differences. >> >> Headers for the names? I do not recall such but that does not seem right. > > Headers for the indexes. > >>> >>> On the other hand reproducing this list in each node is admittedly a bit >>> redundant. >>> >>> Shall I add back a shared header for all PHYs handled by this driver >>> (another implementation detail) even if this could eventually lead to >>> describing clocks not supported by a particular SoC (so such constraints >>> would still need to be described by the binding somehow): >>> >>> /* QMP clocks */ >>> #define QMP_USB3_PIPE_CLK 0 >>> #define QMP_DP_LINK_CLK 1 >>> #define QMP_DP_VCO_DIV_CLK 2 Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, QMP_COMBO_DP_VCO_DIV_CLK? I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK. >> >> What are these about? To remind - we talk about names of clocks this >> device creates. The output names. Whatever IDs you have are not related >> to the names. > > As I mentioned above, this is not about the names that Linux gives to > its representation of these clocks. Its just about defining the valid > indexes in the binding. > > If you think that that using 'clock-output-names' for this is a bit too > unconventional, I can add back the header with defines like the above > instead. > > Note that the clock schema has: > > clock-output-names: > description: | > Recommended to be a list of strings of clock output signal > names indexed by the first cell in the clock specifier. > However, the meaning of clock-output-names is domain > specific to the clock provider, ... > > Johan
On 14/11/2022 16:37, Johan Hovold wrote: > On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote: >> On 11/11/2022 12:24, Johan Hovold wrote: >>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 >>> binding which provided multiple PHYs per IP block and these in turn were >>> described by child nodes. >>> >>> The QMP USB3-DP PHY block provides a single multi-protocol PHY and even >>> if some resources are only used by either the USB or DP part of the >>> device there is no real benefit in describing these resources in child >>> nodes. >>> >>> The original MSM8996 binding also ended up describing the individual >>> register blocks as belonging to either the wrapper node or the PHY child >>> nodes. >>> >>> This is an unnecessary level of detail which has lead to problems when >>> later IP blocks using different register layouts have been forced to fit >>> the original mould rather than updating the binding. The bindings are >>> arguable also incomplete as they only the describe register blocks used >>> by the current Linux drivers (e.g. does not include the PCS LANE >>> registers). >>> >>> This is specifically true for later USB4-USB3-DP QMP PHYs where the TX >>> registers are used by both the USB3 and DP parts of the PHY (and where >>> the USB4 part of the PHY was not covered by the binding at all). Notably >>> there are also no DP "RX" (sic) registers as described by the current >>> bindings and the DP "PCS" region is really a set of DP_PHY registers. >>> >>> Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which >>> further bindings can be based on. >>> >>> Note that the binding uses a PHY type index to access either the USB3 or >>> DP part of the PHY and that this can later be used also for the USB4 >>> part if needed. >>> >>> Similarly, the clock inputs and outputs can later be extended to support >>> USB4. >>> >>> Also note that the current binding is simply removed instead of being >>> deprecated as it was only recently merged and would not allow for >>> supporting DP mode. >>> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> >>> --- > >>> + "#clock-cells": >>> + const: 1 >>> + >>> + clock-output-names: >>> + items: >>> + - const: usb3_pipe >>> + - const: dp_link >>> + - const: dp_vco_div >>> + >>> + "#phy-cells": >>> + const: 1 >>> + description: | >>> + PHY index >>> + - PHY_TYPE_USB3 >>> + - PHY_TYPE_DP >> >> I'm stepping on Rob's and Krzysztof's ground here, but it might be more >> logical and future proof to use indices instead of phy types. > > Why would that be more future-proof? > > I initially added defines for these indexes to a QMP header, but noticed > that we already have PHY drivers that use the PHY types for this. So > there's already a precedent for this and I didn't see any real benefit > to adding multiple per-SoC defines for the same thing. As you guessed from my question, I was thinking about USB4 (for which we do not have a separate PHY_TYPE, but that probably shouldn't matter). Would it be a separate PHY here, or would it be a combo UBS3+USB4 plus separate DP phy? Yes, we have other PHYs, which use types as an index, however it's slightly more common to have indices instead. Anyway, this is a minor issue. It might be just that I'm more common to using indices everywhere (in other words, I have preference here, but it's not a strong requirement from my side). >> Just for my understanding, would USB4 support add another qserdes+tx/rx >> construct or would it be the same USB3 register space? > > The TX/RX registers are shared by the all three parts of the PHY (USB4, > USB3, DP), while USB4 has two dedicated sets of PLL (serdes) and PCS > registers. Ack, thanks.
On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote: > On 14/11/2022 17:18, Johan Hovold wrote: > > On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: > >> On 14/11/2022 14:27, Johan Hovold wrote: > >>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: > >>>> On 11/11/2022 10:24, Johan Hovold wrote: > >>> I noticed that several bindings leave the clock indexes unspecified, or > >>> have header files defining some or all of them. I first added a QMP > >>> header but that seemed like overkill, especially if we'd end up with > >>> one header per SoC (cf. the GCC headers) due to (known and potential) > >>> platform differences. > >>> Shall I add back a shared header for all PHYs handled by this driver > >>> (another implementation detail) even if this could eventually lead to > >>> describing clocks not supported by a particular SoC (so such constraints > >>> would still need to be described by the binding somehow): > >>> > >>> /* QMP clocks */ > >>> #define QMP_USB3_PIPE_CLK 0 > >>> #define QMP_DP_LINK_CLK 1 > >>> #define QMP_DP_VCO_DIV_CLK 2 > > Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, > QMP_COMBO_DP_VCO_DIV_CLK? "COMBO" is just the name of the Linux driver and does not belong in the binding. > I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK > QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK. Yeah, I had those in mind when creating the header and using a generic QMP prefix (even if I didn't end up using the header in v1). This could just be mapping of (arbitrary) QMP indexes to clocks and we use it for USB3, DP, UFS and later also USB4. This will however mean that the indexes are not necessarily zero-based and consecutive for a specific SoC and PHY. But that's perhaps a non-issue (cf. the PHY_TYPE defines). We'd still need to describe which clocks are available on a particular SoC and PHY, and that's partly why I used 'clock-output-names' to fix the mapping in the binding. Guess we can just list the valid defines in the property description as I did for #phy-cells. Johan
On 14/11/2022 15:18, Johan Hovold wrote: > On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: >> On 14/11/2022 14:27, Johan Hovold wrote: >>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >>>> On 11/11/2022 10:24, Johan Hovold wrote: >>>>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 >>>>> binding which provided multiple PHYs per IP block and these in turn were >>>>> described by child nodes. > >>>>> + "#clock-cells": >>>>> + const: 1 >>>>> + >>>>> + clock-output-names: >>>>> + items: >>>>> + - const: usb3_pipe >>>>> + - const: dp_link >>>>> + - const: dp_vco_div >>>> >>>> Why defining here fixed names? The purpose of this field is to actually >>>> allow customizing these - at least in most cases. If these have to be >>>> fixed, then driver should just instantiate these clocks with such names, >>>> right? >>> >>> I'm only using these names as documentation of the indexes. The driver >> >> What do you mean by documentation of indexes? You require these specific >> entries and do not allow anything else. > > I'm using this property as documentation of the valid indexes that can > be used when referring to clocks provided by this device. > > There are currently three and the mapping is described by the > 'clock-output-names' property. That's not the purpose of this property. Drop it then. The names do not define the ABI and do not document it, actually. You require now that every DTB, if providing clock-output-names, will have exactly such names instead of having fixed IDs. DTBs are not for defining the ABI. > >>> doesn't use these names, but that's a Linux-specific implementation >>> detail. >>> >>> I noticed that several bindings leave the clock indexes unspecified, or >>> have header files defining some or all of them. I first added a QMP >>> header but that seemed like overkill, especially if we'd end up with >>> one header per SoC (cf. the GCC headers) due to (known and potential) >>> platform differences. >> >> Headers for the names? I do not recall such but that does not seem right. > > Headers for the indexes. > >>> >>> On the other hand reproducing this list in each node is admittedly a bit >>> redundant. >>> >>> Shall I add back a shared header for all PHYs handled by this driver >>> (another implementation detail) even if this could eventually lead to >>> describing clocks not supported by a particular SoC (so such constraints >>> would still need to be described by the binding somehow): >>> >>> /* QMP clocks */ >>> #define QMP_USB3_PIPE_CLK 0 >>> #define QMP_DP_LINK_CLK 1 >>> #define QMP_DP_VCO_DIV_CLK 2 >> >> What are these about? To remind - we talk about names of clocks this >> device creates. The output names. Whatever IDs you have are not related >> to the names. > > As I mentioned above, this is not about the names that Linux gives to > its representation of these clocks. Its just about defining the valid > indexes in the binding. With or without the header, the IDs are part of ABI and are fixed. The headers are I think always encouraged because it makes above sentence explicit. Without the headers developers might want to change the IDs. > > If you think that that using 'clock-output-names' for this is a bit too > unconventional, I can add back the header with defines like the above > instead. > > Note that the clock schema has: > > clock-output-names: > description: | > Recommended to be a list of strings of clock output signal > names indexed by the first cell in the clock specifier. Exactly. Not to describe the ABI behind the ID. > However, the meaning of clock-output-names is domain > specific to the clock provider, ... ... because you might have more cells. Just because clock-output-names do not fit some drivers it does not mean you can use it any way you wish. It is still for names of provided clocks. Best regards, Krzysztof
On 14/11/2022 18:38, Johan Hovold wrote: > On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote: >> On 14/11/2022 17:18, Johan Hovold wrote: >>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: >>>> On 14/11/2022 14:27, Johan Hovold wrote: >>>>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >>>>>> On 11/11/2022 10:24, Johan Hovold wrote: > >>>>> I noticed that several bindings leave the clock indexes unspecified, or >>>>> have header files defining some or all of them. I first added a QMP >>>>> header but that seemed like overkill, especially if we'd end up with >>>>> one header per SoC (cf. the GCC headers) due to (known and potential) >>>>> platform differences. > >>>>> Shall I add back a shared header for all PHYs handled by this driver >>>>> (another implementation detail) even if this could eventually lead to >>>>> describing clocks not supported by a particular SoC (so such constraints >>>>> would still need to be described by the binding somehow): >>>>> >>>>> /* QMP clocks */ >>>>> #define QMP_USB3_PIPE_CLK 0 >>>>> #define QMP_DP_LINK_CLK 1 >>>>> #define QMP_DP_VCO_DIV_CLK 2 >> >> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, >> QMP_COMBO_DP_VCO_DIV_CLK? > > "COMBO" is just the name of the Linux driver and does not belong in the > binding. We do not have any standard (iow, coming from the docs) name, so we can invent it on our own. > >> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK >> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK. > > Yeah, I had those in mind when creating the header and using a generic > QMP prefix (even if I didn't end up using the header in v1). > > This could just be mapping of (arbitrary) QMP indexes to clocks and we > use it for USB3, DP, UFS and later also USB4. > > This will however mean that the indexes are not necessarily zero-based > and consecutive for a specific SoC and PHY. But that's perhaps a > non-issue (cf. the PHY_TYPE defines). Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for USB+DP PHY, but let's not go for the unified clocks index definition. > > We'd still need to describe which clocks are available on a particular > SoC and PHY, and that's partly why I used 'clock-output-names' to fix > the mapping in the binding. Guess we can just list the valid defines in > the property description as I did for #phy-cells. > > Johan
On Mon, Nov 14, 2022 at 06:31:02PM +0300, Dmitry Baryshkov wrote: > On 14/11/2022 16:37, Johan Hovold wrote: > > On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote: > >> On 11/11/2022 12:24, Johan Hovold wrote: > >>> + "#clock-cells": > >>> + const: 1 > >>> + > >>> + clock-output-names: > >>> + items: > >>> + - const: usb3_pipe > >>> + - const: dp_link > >>> + - const: dp_vco_div > >>> + > >>> + "#phy-cells": > >>> + const: 1 > >>> + description: | > >>> + PHY index > >>> + - PHY_TYPE_USB3 > >>> + - PHY_TYPE_DP > >> > >> I'm stepping on Rob's and Krzysztof's ground here, but it might be more > >> logical and future proof to use indices instead of phy types. > > > > Why would that be more future-proof? > > > > I initially added defines for these indexes to a QMP header, but noticed > > that we already have PHY drivers that use the PHY types for this. So > > there's already a precedent for this and I didn't see any real benefit > > to adding multiple per-SoC defines for the same thing. > > As you guessed from my question, I was thinking about USB4 (for which we > do not have a separate PHY_TYPE, but that probably shouldn't matter). Yeah, that's easy enough to add. > Would it be a separate PHY here, or would it be a combo UBS3+USB4 plus > separate DP phy? We don't know yet. > Yes, we have other PHYs, which use types as an index, however it's > slightly more common to have indices instead. If you look at (yaml) bindings using a single phy-cell, the majority simply ignores describing what the index is used for and which values are valid. Of the few that do describe it, the cell index is either used for something which does not allow itself for mapping to PHY_TYPE or PHY_TYPE is used. > Anyway, this is a minor issue. It might be just that I'm more common to > using indices everywhere (in other words, I have preference here, but > it's not a strong requirement from my side). I don't have a strong preference here either. Let's see what Krzysztof and Rob says. Johan
On Mon, Nov 14, 2022 at 04:49:37PM +0100, Krzysztof Kozlowski wrote: > On 14/11/2022 15:18, Johan Hovold wrote: > > On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: > >> On 14/11/2022 14:27, Johan Hovold wrote: > >>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: > >>>> On 11/11/2022 10:24, Johan Hovold wrote: > >>>>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 > >>>>> binding which provided multiple PHYs per IP block and these in turn were > >>>>> described by child nodes. > > > >>>>> + "#clock-cells": > >>>>> + const: 1 > >>>>> + > >>>>> + clock-output-names: > >>>>> + items: > >>>>> + - const: usb3_pipe > >>>>> + - const: dp_link > >>>>> + - const: dp_vco_div > >>>> > >>>> Why defining here fixed names? The purpose of this field is to actually > >>>> allow customizing these - at least in most cases. If these have to be > >>>> fixed, then driver should just instantiate these clocks with such names, > >>>> right? > >>> > >>> I'm only using these names as documentation of the indexes. The driver > >> > >> What do you mean by documentation of indexes? You require these specific > >> entries and do not allow anything else. > > > > I'm using this property as documentation of the valid indexes that can > > be used when referring to clocks provided by this device. > > > > There are currently three and the mapping is described by the > > 'clock-output-names' property. > > That's not the purpose of this property. Drop it then. The names do not > define the ABI and do not document it, actually. You require now that > every DTB, if providing clock-output-names, will have exactly such names > instead of having fixed IDs. DTBs are not for defining the ABI. Fair enough, I'll drop it. But there doesn't seem to be a good way to describe the indexes currently and most bindings simply ignore to do so. So what is the preference then? Just leave things undocumented, listing indexes in a free-text 'description', or adding a free-text reference to a binding header file and using those define names in a free-text 'description'? And if going with the last option, does this mean that every SoC and PHY type needs its own header for those three clocks or so to avoid having a common dumping ground header file where indexes will not necessarily be 0-based and consecutive. Johan
On 14/11/2022 17:32, Johan Hovold wrote: > On Mon, Nov 14, 2022 at 04:49:37PM +0100, Krzysztof Kozlowski wrote: >> On 14/11/2022 15:18, Johan Hovold wrote: >>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: >>>> On 14/11/2022 14:27, Johan Hovold wrote: >>>>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >>>>>> On 11/11/2022 10:24, Johan Hovold wrote: >>>>>>> The current QMP USB3-DP PHY bindings are based on the original MSM8996 >>>>>>> binding which provided multiple PHYs per IP block and these in turn were >>>>>>> described by child nodes. >>> >>>>>>> + "#clock-cells": >>>>>>> + const: 1 >>>>>>> + >>>>>>> + clock-output-names: >>>>>>> + items: >>>>>>> + - const: usb3_pipe >>>>>>> + - const: dp_link >>>>>>> + - const: dp_vco_div >>>>>> >>>>>> Why defining here fixed names? The purpose of this field is to actually >>>>>> allow customizing these - at least in most cases. If these have to be >>>>>> fixed, then driver should just instantiate these clocks with such names, >>>>>> right? >>>>> >>>>> I'm only using these names as documentation of the indexes. The driver >>>> >>>> What do you mean by documentation of indexes? You require these specific >>>> entries and do not allow anything else. >>> >>> I'm using this property as documentation of the valid indexes that can >>> be used when referring to clocks provided by this device. >>> >>> There are currently three and the mapping is described by the >>> 'clock-output-names' property. >> >> That's not the purpose of this property. Drop it then. The names do not >> define the ABI and do not document it, actually. You require now that >> every DTB, if providing clock-output-names, will have exactly such names >> instead of having fixed IDs. DTBs are not for defining the ABI. > > Fair enough, I'll drop it. But there doesn't seem to be a good way to > describe the indexes currently and most bindings simply ignore to do so. > > So what is the preference then? Just leave things undocumented, listing > indexes in a free-text 'description', or adding a free-text reference to > a binding header file and using those define names in a free-text > 'description'? Either 2 or 3. Several bindings for small number of constants choose option 2. > > And if going with the last option, does this mean that every SoC and PHY > type needs its own header for those three clocks or so to avoid having > a common dumping ground header file where indexes will not necessarily > be 0-based and consecutive. phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you have many of header files? Best regards, Krzysztof
On Mon, Nov 14, 2022 at 07:14:48PM +0300, Dmitry Baryshkov wrote: > On 14/11/2022 18:38, Johan Hovold wrote: > > On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote: > >> On 14/11/2022 17:18, Johan Hovold wrote: > >>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: > >>>> On 14/11/2022 14:27, Johan Hovold wrote: > >>>>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: > >>>>>> On 11/11/2022 10:24, Johan Hovold wrote: > > > >>>>> I noticed that several bindings leave the clock indexes unspecified, or > >>>>> have header files defining some or all of them. I first added a QMP > >>>>> header but that seemed like overkill, especially if we'd end up with > >>>>> one header per SoC (cf. the GCC headers) due to (known and potential) > >>>>> platform differences. > > > >>>>> Shall I add back a shared header for all PHYs handled by this driver > >>>>> (another implementation detail) even if this could eventually lead to > >>>>> describing clocks not supported by a particular SoC (so such constraints > >>>>> would still need to be described by the binding somehow): > >>>>> > >>>>> /* QMP clocks */ > >>>>> #define QMP_USB3_PIPE_CLK 0 > >>>>> #define QMP_DP_LINK_CLK 1 > >>>>> #define QMP_DP_VCO_DIV_CLK 2 > >> > >> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, > >> QMP_COMBO_DP_VCO_DIV_CLK? > > > > "COMBO" is just the name of the Linux driver and does not belong in the > > binding. > > We do not have any standard (iow, coming from the docs) name, so we can > invent it on our own. I still think the naming should reflect the hardware and not the Linux implementation if this is going into the binding. And the USB4_USB3_DP defines are going to be a superset of USB3_DP (as far we know know). > >> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK > >> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK. > > > > Yeah, I had those in mind when creating the header and using a generic > > QMP prefix (even if I didn't end up using the header in v1). > > > > This could just be mapping of (arbitrary) QMP indexes to clocks and we > > use it for USB3, DP, UFS and later also USB4. > > > > This will however mean that the indexes are not necessarily zero-based > > and consecutive for a specific SoC and PHY. But that's perhaps a > > non-issue (cf. the PHY_TYPE defines). > > Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for > USB+DP PHY, but let's not go for the unified clocks index definition. Yeah, this is the kind of issues I wanted to avoid by not using a per SoC header for three clocks which will almost always use the same indexes. Because how can you be sure that your unified per-PHY type defines will never have to be amended? Or some index left out? The only way then is to have per-SoC defines which is a pain to maintain (just consider that driver mapping table when some odd SoC shows up). Johan
On Mon, Nov 14, 2022 at 05:39:26PM +0100, Krzysztof Kozlowski wrote: > On 14/11/2022 17:32, Johan Hovold wrote: > > Fair enough, I'll drop it. But there doesn't seem to be a good way to > > describe the indexes currently and most bindings simply ignore to do so. > > > > So what is the preference then? Just leave things undocumented, listing > > indexes in a free-text 'description', or adding a free-text reference to > > a binding header file and using those define names in a free-text > > 'description'? > > Either 2 or 3. Several bindings for small number of constants choose > option 2. Ok, we have three now, but USB4 will bump this to ten or so. > > And if going with the last option, does this mean that every SoC and PHY > > type needs its own header for those three clocks or so to avoid having > > a common dumping ground header file where indexes will not necessarily > > be 0-based and consecutive. > > phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you > have many of header files? We don't know what kind of clock outputs later revisions of these PHYs will have. The only way to guarantee 0-based consecutive indexes appears to be to use per-SoC defines (e.g. as for the GCC bindings). Johan
On 14/11/2022 19:42, Johan Hovold wrote: > On Mon, Nov 14, 2022 at 07:14:48PM +0300, Dmitry Baryshkov wrote: >> On 14/11/2022 18:38, Johan Hovold wrote: >>> On Mon, Nov 14, 2022 at 06:19:25PM +0300, Dmitry Baryshkov wrote: >>>> On 14/11/2022 17:18, Johan Hovold wrote: >>>>> On Mon, Nov 14, 2022 at 03:07:41PM +0100, Krzysztof Kozlowski wrote: >>>>>> On 14/11/2022 14:27, Johan Hovold wrote: >>>>>>> On Fri, Nov 11, 2022 at 04:17:29PM +0100, Krzysztof Kozlowski wrote: >>>>>>>> On 11/11/2022 10:24, Johan Hovold wrote: >>> >>>>>>> I noticed that several bindings leave the clock indexes unspecified, or >>>>>>> have header files defining some or all of them. I first added a QMP >>>>>>> header but that seemed like overkill, especially if we'd end up with >>>>>>> one header per SoC (cf. the GCC headers) due to (known and potential) >>>>>>> platform differences. >>> >>>>>>> Shall I add back a shared header for all PHYs handled by this driver >>>>>>> (another implementation detail) even if this could eventually lead to >>>>>>> describing clocks not supported by a particular SoC (so such constraints >>>>>>> would still need to be described by the binding somehow): >>>>>>> >>>>>>> /* QMP clocks */ >>>>>>> #define QMP_USB3_PIPE_CLK 0 >>>>>>> #define QMP_DP_LINK_CLK 1 >>>>>>> #define QMP_DP_VCO_DIV_CLK 2 >>>> >>>> Maybe QMP_COMBO_USB3_PIPE_CLK, QMP_COMBO_DP_LINK_CLK, >>>> QMP_COMBO_DP_VCO_DIV_CLK? >>> >>> "COMBO" is just the name of the Linux driver and does not belong in the >>> binding. >> >> We do not have any standard (iow, coming from the docs) name, so we can >> invent it on our own. > > I still think the naming should reflect the hardware and not the Linux > implementation if this is going into the binding. Well, I always viewed docs as > > And the USB4_USB3_DP defines are going to be a superset of USB3_DP (as > far we know know). > >>>> I'll then extend this header with QMP_UFS_RX_SYMBOL_0_CLK >>>> QMP_UFS_RX_SYMBOL_1_CLK and QMP_UFS_TX_SYMBOL_0_CLK. >>> >>> Yeah, I had those in mind when creating the header and using a generic >>> QMP prefix (even if I didn't end up using the header in v1). >>> >>> This could just be mapping of (arbitrary) QMP indexes to clocks and we >>> use it for USB3, DP, UFS and later also USB4. >>> >>> This will however mean that the indexes are not necessarily zero-based >>> and consecutive for a specific SoC and PHY. But that's perhaps a >>> non-issue (cf. the PHY_TYPE defines). >> >> Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for >> USB+DP PHY, but let's not go for the unified clocks index definition. > > Yeah, this is the kind of issues I wanted to avoid by not using a per > SoC header for three clocks which will almost always use the same > indexes. > > Because how can you be sure that your unified per-PHY type defines will > never have to be amended? Or some index left out? > > The only way then is to have per-SoC defines which is a pain to > maintain (just consider that driver mapping table when some odd SoC > shows up). My vote is definitely against a per-SoC defines. > > Johan
On Mon, Nov 14, 2022 at 07:51:31PM +0300, Dmitry Baryshkov wrote: > On 14/11/2022 19:42, Johan Hovold wrote: > > On Mon, Nov 14, 2022 at 07:14:48PM +0300, Dmitry Baryshkov wrote: > >> Ugh. Please, no. We have symbol clocks for UFS PHY, USB+DP clocks for > >> USB+DP PHY, but let's not go for the unified clocks index definition. > > > > Yeah, this is the kind of issues I wanted to avoid by not using a per > > SoC header for three clocks which will almost always use the same > > indexes. > > > > Because how can you be sure that your unified per-PHY type defines will > > never have to be amended? Or some index left out? > > > > The only way then is to have per-SoC defines which is a pain to > > maintain (just consider that driver mapping table when some odd SoC > > shows up). > > My vote is definitely against a per-SoC defines. Simply stating that doesn't address the problem I was trying to describe. Johan
On 14/11/2022 17:48, Johan Hovold wrote: > On Mon, Nov 14, 2022 at 05:39:26PM +0100, Krzysztof Kozlowski wrote: >> On 14/11/2022 17:32, Johan Hovold wrote: > >>> Fair enough, I'll drop it. But there doesn't seem to be a good way to >>> describe the indexes currently and most bindings simply ignore to do so. >>> >>> So what is the preference then? Just leave things undocumented, listing >>> indexes in a free-text 'description', or adding a free-text reference to >>> a binding header file and using those define names in a free-text >>> 'description'? >> >> Either 2 or 3. Several bindings for small number of constants choose >> option 2. > > Ok, we have three now, but USB4 will bump this to ten or so. Then probably header file is the way to go. > >>> And if going with the last option, does this mean that every SoC and PHY >>> type needs its own header for those three clocks or so to avoid having >>> a common dumping ground header file where indexes will not necessarily >>> be 0-based and consecutive. >> >> phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you >> have many of header files? > > We don't know what kind of clock outputs later revisions of these PHYs > will have. The only way to guarantee 0-based consecutive indexes appears > to be to use per-SoC defines (e.g. as for the GCC bindings). Which is also fine. I don't understand still why it is a problem - even if you have multiple files, one for each SoC/phy. If USB4 brings here 10 more clocks and other SoCs/phys might bring many more options, then what else can you do? Grow the binding file with big text-based mapping of IDs? It's not a viable solution. Header or headers is the only maintainable way for such cases. Best regards, Krzysztof
On Mon, Nov 14, 2022 at 05:56:21PM +0100, Krzysztof Kozlowski wrote: > On 14/11/2022 17:48, Johan Hovold wrote: > > On Mon, Nov 14, 2022 at 05:39:26PM +0100, Krzysztof Kozlowski wrote: > >> On 14/11/2022 17:32, Johan Hovold wrote: > > > >>> Fair enough, I'll drop it. But there doesn't seem to be a good way to > >>> describe the indexes currently and most bindings simply ignore to do so. > >>> > >>> So what is the preference then? Just leave things undocumented, listing > >>> indexes in a free-text 'description', or adding a free-text reference to > >>> a binding header file and using those define names in a free-text > >>> 'description'? > >> > >> Either 2 or 3. Several bindings for small number of constants choose > >> option 2. > > > > Ok, we have three now, but USB4 will bump this to ten or so. > > Then probably header file is the way to go. > > > > >>> And if going with the last option, does this mean that every SoC and PHY > >>> type needs its own header for those three clocks or so to avoid having > >>> a common dumping ground header file where indexes will not necessarily > >>> be 0-based and consecutive. > >> > >> phy-qcom-qmp-combo.c has one qcom_qmp_dp_clks_hw_get(), so why would you > >> have many of header files? > > > > We don't know what kind of clock outputs later revisions of these PHYs > > will have. The only way to guarantee 0-based consecutive indexes appears > > to be to use per-SoC defines (e.g. as for the GCC bindings). > > Which is also fine. I don't understand still why it is a problem - even > if you have multiple files, one for each SoC/phy. If USB4 brings here 10 > more clocks and other SoCs/phys might bring many more options, then what > else can you do? Grow the binding file with big text-based mapping of > IDs? It's not a viable solution. Header or headers is the only > maintainable way for such cases. So then we must add per-SoC (and PHY type) headers even if we can possibly reuse defines from one platform for another as long as they appear to be similar enough? For example, using a "SC7180_USB3_DP" infix for the current platforms and add a new series of indexes for SC8280XP: QMP_SC7180_USB3_DP_USB3_PIPE 0 QMP_SC7180_USB3_DP_DP_LINK 1 QMP_SC7180_USB3_DP_DP_VCO_DIV 2 QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE 0 QMP_SC8280XP_USB4_USB3_DP_DP_LINK 1 QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV 2 QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE 3 ... QMP_SC8280XP_USB4_USB3_DP_USB4_RX1 9 Johan
On 14/11/2022 18:08, Johan Hovold wrote: >> >> Which is also fine. I don't understand still why it is a problem - even >> if you have multiple files, one for each SoC/phy. If USB4 brings here 10 >> more clocks and other SoCs/phys might bring many more options, then what >> else can you do? Grow the binding file with big text-based mapping of >> IDs? It's not a viable solution. Header or headers is the only >> maintainable way for such cases. > > So then we must add per-SoC (and PHY type) headers even if we can > possibly reuse defines from one platform for another as long as they > appear to be similar enough? No, you don't have to. I just got impression that future devices will bring so many changes that anyway you will end up with per-SoC defines. > For example, using a "SC7180_USB3_DP" infix > for the current platforms and add a new series of indexes for SC8280XP: > > QMP_SC7180_USB3_DP_USB3_PIPE 0 > QMP_SC7180_USB3_DP_DP_LINK 1 > QMP_SC7180_USB3_DP_DP_VCO_DIV 2 > > QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE 0 > QMP_SC8280XP_USB4_USB3_DP_DP_LINK 1 > QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV 2 > QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE 3 > ... > QMP_SC8280XP_USB4_USB3_DP_USB4_RX1 9 The names are just a names, you can even use QMP_SC7180_* on SC8280XP. You can skip the SoC part and have something shared. We already have such patterns - although maybe more often for outside components (like PMICs). The differences are: 1. For per-SoC name it's quite obvious which clock is supported on fiven SoC, 2. With shared names, you should document somewhere mapping between supported clocks and SoCs. Also what to do if new device comes with 10 new clocks entirely different - re-use/map existing defines or add completely new set of 10 of them? Best regards, Krzysztof
On Tue, Nov 15, 2022 at 09:12:54AM +0100, Krzysztof Kozlowski wrote: > On 14/11/2022 18:08, Johan Hovold wrote: > >> > >> Which is also fine. I don't understand still why it is a problem - even > >> if you have multiple files, one for each SoC/phy. If USB4 brings here 10 > >> more clocks and other SoCs/phys might bring many more options, then what > >> else can you do? Grow the binding file with big text-based mapping of > >> IDs? It's not a viable solution. Header or headers is the only > >> maintainable way for such cases. > > > > So then we must add per-SoC (and PHY type) headers even if we can > > possibly reuse defines from one platform for another as long as they > > appear to be similar enough? > > No, you don't have to. I just got impression that future devices will > bring so many changes that anyway you will end up with per-SoC defines. > > > For example, using a "SC7180_USB3_DP" infix > > for the current platforms and add a new series of indexes for SC8280XP: > > > > QMP_SC7180_USB3_DP_USB3_PIPE 0 > > QMP_SC7180_USB3_DP_DP_LINK 1 > > QMP_SC7180_USB3_DP_DP_VCO_DIV 2 > > > > QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE 0 > > QMP_SC8280XP_USB4_USB3_DP_DP_LINK 1 > > QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV 2 > > QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE 3 > > ... > > QMP_SC8280XP_USB4_USB3_DP_USB4_RX1 9 > > The names are just a names, you can even use QMP_SC7180_* on SC8280XP. > You can skip the SoC part and have something shared. We already have > such patterns - although maybe more often for outside components (like > PMICs). The differences are: > 1. For per-SoC name it's quite obvious which clock is supported on fiven > SoC, > 2. With shared names, you should document somewhere mapping between > supported clocks and SoCs. Also what to do if new device comes with 10 > new clocks entirely different - re-use/map existing defines or add > completely new set of 10 of them? Ok, thanks. I'll go with a common prefix per PHY type for now, and we can worry about hypothetical hardware revisions later. I'll use a "QMP_USB43DP_" prefix for the new SC8280XP binding, which can be reused also for the older SoCs with USB3-DP PHYs if/when we convert them as their indexes will be a subset of the SC8280XP ones: /* QMP USB4-USB3-DP clocks */ #define QMP_USB43DP_USB3_PIPE_CLK 0 #define QMP_USB43DP_DP_LINK_CLK 1 #define QMP_USB43DP_DP_VCO_DIV_CLK 2 Since I'm adding a new header anyway, I decided to go with dedicated indexes also for the PHY selection (instead of using the PHY_TYPE defines): /* QMP USB4-USB3-DP PHYs */ #define QMP_USB43DP_USB3_PHY 0 #define QMP_USB43DP_DP_PHY 1 I'll add these to a common dt-bindings/phy/phy-qcom-qmp.h header so that it can be used also for the UFS clocks (with a "QMP_UFS_" prefix). Johan
On 15/11/2022 15:22, Johan Hovold wrote: > On Tue, Nov 15, 2022 at 09:12:54AM +0100, Krzysztof Kozlowski wrote: >> On 14/11/2022 18:08, Johan Hovold wrote: >>>> >>>> Which is also fine. I don't understand still why it is a problem - even >>>> if you have multiple files, one for each SoC/phy. If USB4 brings here 10 >>>> more clocks and other SoCs/phys might bring many more options, then what >>>> else can you do? Grow the binding file with big text-based mapping of >>>> IDs? It's not a viable solution. Header or headers is the only >>>> maintainable way for such cases. >>> >>> So then we must add per-SoC (and PHY type) headers even if we can >>> possibly reuse defines from one platform for another as long as they >>> appear to be similar enough? >> >> No, you don't have to. I just got impression that future devices will >> bring so many changes that anyway you will end up with per-SoC defines. >> >>> For example, using a "SC7180_USB3_DP" infix >>> for the current platforms and add a new series of indexes for SC8280XP: >>> >>> QMP_SC7180_USB3_DP_USB3_PIPE 0 >>> QMP_SC7180_USB3_DP_DP_LINK 1 >>> QMP_SC7180_USB3_DP_DP_VCO_DIV 2 >>> >>> QMP_SC8280XP_USB4_USB3_DP_USB3_PIPE 0 >>> QMP_SC8280XP_USB4_USB3_DP_DP_LINK 1 >>> QMP_SC8280XP_USB4_USB3_DP_DP_VCO_DIV 2 >>> QMP_SC8280XP_USB4_USB3_DP_USB4_PCIE_PIPE 3 >>> ... >>> QMP_SC8280XP_USB4_USB3_DP_USB4_RX1 9 >> >> The names are just a names, you can even use QMP_SC7180_* on SC8280XP. >> You can skip the SoC part and have something shared. We already have >> such patterns - although maybe more often for outside components (like >> PMICs). The differences are: >> 1. For per-SoC name it's quite obvious which clock is supported on fiven >> SoC, >> 2. With shared names, you should document somewhere mapping between >> supported clocks and SoCs. Also what to do if new device comes with 10 >> new clocks entirely different - re-use/map existing defines or add >> completely new set of 10 of them? > > Ok, thanks. I'll go with a common prefix per PHY type for now, and we > can worry about hypothetical hardware revisions later. > > I'll use a "QMP_USB43DP_" prefix for the new SC8280XP binding, which can > be reused also for the older SoCs with USB3-DP PHYs if/when we convert > them as their indexes will be a subset of the SC8280XP ones: > > /* QMP USB4-USB3-DP clocks */ > #define QMP_USB43DP_USB3_PIPE_CLK 0 > #define QMP_USB43DP_DP_LINK_CLK 1 > #define QMP_USB43DP_DP_VCO_DIV_CLK 2 > > Since I'm adding a new header anyway, I decided to go with dedicated > indexes also for the PHY selection (instead of using the PHY_TYPE > defines): > > /* QMP USB4-USB3-DP PHYs */ > #define QMP_USB43DP_USB3_PHY 0 > #define QMP_USB43DP_DP_PHY 1 > > I'll add these to a common dt-bindings/phy/phy-qcom-qmp.h header so that > it can be used also for the UFS clocks (with a "QMP_UFS_" prefix). Sounds good, thanks. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml index 50b1fce530d5..2f4a419197a8 100644 --- a/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,sc7180-qmp-usb3-dp-phy.yaml @@ -23,7 +23,6 @@ properties: - qcom,sc7180-qmp-usb3-dp-phy - qcom,sc7280-qmp-usb3-dp-phy - qcom,sc8180x-qmp-usb3-dp-phy - - qcom,sc8280xp-qmp-usb43dp-phy - qcom,sdm845-qmp-usb3-dp-phy - qcom,sm8250-qmp-usb3-dp-phy reg: @@ -169,17 +168,6 @@ required: additionalProperties: false -allOf: - - if: - properties: - compatible: - contains: - enum: - - qcom,sc8280xp-qmp-usb43dp-phy - then: - required: - - power-domains - examples: - | #include <dt-bindings/clock/qcom,gcc-sdm845.h> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml new file mode 100644 index 000000000000..bd04150acee4 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml @@ -0,0 +1,111 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm QMP USB4-USB3-DP PHY controller (SC8280XP) + +maintainers: + - Vinod Koul <vkoul@kernel.org> + +description: + The QMP PHY controller supports physical layer functionality for a number of + controllers on Qualcomm chipsets, such as, PCIe, UFS and USB. + + See also: + - include/dt-bindings/dt-bindings/phy/phy.h + +properties: + compatible: + enum: + - qcom,sc8280xp-qmp-usb43dp-phy + + reg: + maxItems: 1 + + clocks: + maxItems: 4 + + clock-names: + items: + - const: aux + - const: ref + - const: com_aux + - const: usb3_pipe + + power-domains: + maxItems: 1 + + resets: + maxItems: 2 + + reset-names: + items: + - const: phy + - const: common + + vdda-phy-supply: true + + vdda-pll-supply: true + + "#clock-cells": + const: 1 + + clock-output-names: + items: + - const: usb3_pipe + - const: dp_link + - const: dp_vco_div + + "#phy-cells": + const: 1 + description: | + PHY index + - PHY_TYPE_USB3 + - PHY_TYPE_DP + +required: + - compatible + - reg + - clocks + - clock-names + - power-domains + - resets + - reset-names + - vdda-phy-supply + - vdda-pll-supply + - "#clock-cells" + - clock-output-names + - "#phy-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-sc8280xp.h> + + phy@88eb000 { + compatible = "qcom,sc8280xp-qmp-usb43dp-phy"; + reg = <0x088eb000 0x4000>; + + clocks = <&gcc GCC_USB3_PRIM_PHY_AUX_CLK>, + <&gcc GCC_USB4_EUD_CLKREF_CLK>, + <&gcc GCC_USB3_PRIM_PHY_COM_AUX_CLK>, + <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>; + clock-names = "aux", "ref", "com_aux", "usb3_pipe"; + + power-domains = <&gcc USB30_PRIM_GDSC>; + + resets = <&gcc GCC_USB3_PHY_PRIM_BCR>, + <&gcc GCC_USB4_DP_PHY_PRIM_BCR>; + reset-names = "phy", "common"; + + vdda-phy-supply = <&vreg_l9d>; + vdda-pll-supply = <&vreg_l4d>; + + #clock-cells = <1>; + clock-output-names = "usb3_pipe", "dp_link", "dp_vco_div"; + + #phy-cells = <1>; + };