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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5AB19C43458 for ; Wed, 8 Jul 2026 21:55:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XORfde21DYqvEo1FEy2oY86fAnoBupmOv2AIKS5S/h8=; b=1Zql6Cy7myJXMT e3rq4BqXOQtI7HA8NxLzup9PVgXZpEO1AclKiFA8c29djWyuDf27E8523v3KAnSG2bn7+bwPgdHjA S4aKnpCdCcg86UzpdXOco+EWSZmvbGi6ZWZKoRAQbniAXzFVU+sVmFsiahQUHNkQKkSf2foyjLJQ1 9OmNiNdfjZF/MgVmkIsD44LMh0mqGsqWmruOPiHs8+OYGhKmKLpeYYkSlBOjohJfziAlLZ//C1vX3 jcjwXAMHZhd3kQBrVcN5kodfwIY4jFvR2bttq2Hs7lCcz8A4uCnWvEwg16TRU/3s4qNT4Gv3hbEnL cyl625KrB2jc+MBg9GJQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1whaFH-00000000TlY-0MIC; Wed, 08 Jul 2026 21:55:55 +0000 Received: from tor.source.kernel.org ([172.105.4.254]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1whaFF-00000000Tks-38Wi for linux-phy@lists.infradead.org; Wed, 08 Jul 2026 21:55:53 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by tor.source.kernel.org (Postfix) with ESMTP id C4BF56001A; Wed, 8 Jul 2026 21:55:52 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E81091F000E9; Wed, 8 Jul 2026 21:55:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783547752; bh=18M0o3bms/FWqXkbeCx1DIT7uNO8cNtKfuRe5v9zuL4=; h=Date:Subject:To:Cc:References:From:In-Reply-To; b=T4pVv/DPQZeN1DflmRgh80MUtzF9ZIXol+CG7beLlbMqWe2yB0eeM23vHt6LrhzUP Kw2eAK9UensxbWIL3lmtHLMQaM+CgXE6N5jQsCwx4FcvBzJTqXzQglWYFJDAjXV8bD nnq/ckAfCDP50jZiHUcpG/Mzvupi+1WXoEYmPLEVWgBclB4GLDg/WkxJ7dagh8gIoj pcI8MdPYvWTJcPKC8NuNJRtynSVzH7MDXmIV2N6Bg9ktXYCJE5co0CMjyEMsrC8U7b aICTMh5iAcwZn2IJ73teHp1ATvjqvWmY9we+rhZpTgEHReWJEkNEupbSEytVjQ6cfg uRg1qhZhatayA== Message-ID: <3ab676ac-919b-4a80-8dd6-71dd7f6fe06c@kernel.org> Date: Wed, 8 Jul 2026 22:55:48 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema To: Vladimir Zapolskiy , Bryan O'Donoghue , Vinod Koul , Kishon Vijay Abraham I , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Neil Armstrong Cc: linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <20260708-x1e-csi2-phy-v9-0-0210b90c04cf@linaro.org> <20260708-x1e-csi2-phy-v9-1-0210b90c04cf@linaro.org> From: Bryan O'Donoghue Content-Language: en-US Autocrypt: addr=bod@kernel.org; keydata= xsFNBGRJNSgBEADD7Vm2ZFa+v+JGJ2QYTJqQAkqis/uOHkhdFNXqpBarVBd47QU/DMNU5Rxg jedMQEmHoeDbJ6UOpjbrUQ63c5sgG1JbroHJJctwsEI75OOlekMuebEbjIJBLfgENGwPBMHv piv5TgCWr0VgYaXfp2eh2LINFywzqj823HiDPibQAXDrjzvF1ogksi/6cQZs8d4if8YQkLOr YISFouG+eR0nN1I7mUfIddXOWu6lJeTyqbWVurv58k2ekIXKaOC9ixLHFbcfYV0hOgRaTwQC B8CYF9nfqZla19iItfsN9QxN+ZdQjcRoYipp6HPCMfJlKH7GfaFcW93LKc4DKJ2lVL+pg/OQ lythZbjRPY492NG9kZ65aYstCs90uhMUEVVPuGUw7wBEku+6IEwZfrbMVKeWzLlPyM4Hv9hM 8ktxSmxWsPTPqpBC8eyeAQLalMELAyVcZlkaCtEcbj7w4l/JkYz+4l37obG8ZD+B34udBUUz MsAJ8foDFrBh2MOFA3hxD6G90D23mmWsri7pnKA2tZs92aQX7Ee+FbCyg6g5ln62Sq83ZDbf 53DdBs55EVpBadeInWmXhzCHPQx06H+CwTEjShTYIaMmBfrewvYUDKvFTC5iKQhAEUgt6i94 JsbG7NoeqcxkUMcBOEUQ3uCQG1D70ugspgXc0wd3Rimiq6535wARAQABzSFCcnlhbiBPJ0Rv bm9naHVlIDxib2RAa2VybmVsLm9yZz7CwZEEEwEIADsWIQTmk/sqq6Nt4Rerb7QicTuzoY3I OgUCZ+R+mwIbAwULCQgHAgIiAgYVCgkICwIEFgIDAQIeBwIXgAAKCRAicTuzoY3IOimUD/94 BwVEJX31JRe2sxbB/e1w2p8x1bxvTw5AeIzpV3ox7coJg1bSU2mnGuj1V4o0Yxf/3zmcJzCN VfVjwRF8Ii3GnC7uUXk2t+87piQfKTyJAYQABhZUKgoVJbjJq/S+C3XCKIyBA+EiezoUsgsA jTzwU+FzV7zVWIXFPJNtBERLwboE9w9U3KjAExOa1kSY8eLrsg6kOwlOHWy5UsQqYOjrS96M mzm2xuc1+RCjrndAyYhCnrOKvJ67HsPnBeJCjw7ImGD/U1GchwYbX8o3DO3JNHm3qfC86ZqX 2sCouENg4OzgPTtLKUrueM6xsu6KMM7gj17vxsiR3KQEoJnnMB8D1xtBofN3mFZE0wD9M24m 8yGunZbtntMCUHzIrlJgAPwKWKuGOYtA8UgMTFkccnUJtQrg9KotKtEF/FuftG9zLG9XEkt4 5ZdNgbSoLWgelu3T47mbOJ8LHhiLaCWP7yrovtVAvLUQ1BsiA42u8ECrFCFvQj9nrejE/ICv kP+uqcKtdDvP9HrIGycF1WZyfZLp0RvopKW92FLvI4I1QFWJ+wenk6+LGyJ5bzlrWzevjxmf nHcXE6sJBHrE7eijlbbImDAi3uLYN8Nd9Dm11IDAy4GAIQxSiQn0yblDhPiyGtchy80EVkCm g9k17Wol+2E2mC4DKgVdCkyUtTRSLgsJCs7BTQRkSTUoARAAuTnmWHBS6izRcEE93ajpzI7h dgQO4U3IRvOEsvIKR5NGcNEs0ngGebwsZ/lVULjN4vYU0LleqVhPBidNXUoZCN3A0F0Z2Ov8 NZdef+2EhQPBVWxFO7JBzhe8Z3ALj+wFtlg8akJjBzU56azW/iJzAobqHVrudzKoO2b1/CMg VbiAQ+RXjgfN5kY/HqYDU7mw+hXuUV9PbtX1L8xqQQac95oM9rHzKHHpiVwxTeJnGQsa+THi Kze+YET3rCoGHMvOQEJhdrucTv5FpAakKdkOFNel9FFckLRKEuWgCzhpFsjQ7xbirQgFUxG9 vlk1+q4hMRGNyEqoD6svYEeqbiUSd0oPUJeioiC3rNMRCNHLVrfZ2J6SCPkxfda08uzSdDQU 1/YPjOh8ZtQDMu7WctZ3XO288Z1gyBR49V7fbFs2w4sQxG+h/enlxqP7fdw1mjUlZjU5huCJ ielS0oEaIpmUpkugli7x4WhwLnhK2EbSoz7nLBC0y+ALUOdMlz/Y1l9xRt+bkDhpmf4O4IcI MxgZ0QMLq8rHDkGaEbsgZZHQPS58T0XE3IP30Q9SNxsruCMXtd2hYtBssf/wohc6JVsTtMg2 VYTPDPIFNZFSXupEJB7jlqpDWJ8ooJfJRLBatbjT5+mVQaMYB7Hs/t+zWYWaJKHyc8O6WLEC NUV5Tdt5EkkAEQEAAcLBdgQYAQoAIBYhBOaT+yqro23hF6tvtCJxO7Ohjcg6BQJkSTUoAhsM AAoJECJxO7Ohjcg6LuIQALnXt36OUuK43wqw6UYt0cnN6EbUqJHApAF5eNFn0jCCB2XELjSz JKJwuNAweowBdabiBniJ+501WIW+ewEsz1uby5fUQjZuCEsIkuaIluyfUFPb73qrQyAGuusd 7teA4WT+/jUku9g7lX5sVoRCrKQPkd16f6Bzfztyqyjcn43/X5yQI+wlboQ6HuKe/3I3yiOx OgmCHzOawpC9PvhEcKj79RLM3Zz5Ts5AuHpRX70Jz8Be76LwVFLp5Msx3S24ZTU1lBo2uiJ3 xSkay2lTpyVWRPx9vgcwzxGguOPJQJwsQeLb7wpoJMPpD3ERoaRii7Q7hvmxklpZjhKYWB3d t6nQ497Ek9loCrp3MIjRCSDN5xEGffiHks9yTeGMUQwO4tX8RE04uOJPkUY7uCFzFqN6/qey X3oFfPgkULMdiHofPAL1OskZSTzGPSfTYRE46NCJw8yoZBQ/oOyWeqaUQbK0wmW/g81wm8p7 LKSGEglMpiX07M1AotgvylN5C8fjbouoK+/RAMsXkk8jba6rPfuuXPaDjCyyKn6zSVHETnHW 3AJbgVY50T8STpnxayBQvWbCvu+6NOEjXCbyaOJig+5l0zlGN9XHjdANXC5HnwmyaGRL9YDq Jh2nVXVJDincOdQRdKcJjYLqaOAoWrYWSDi1iZGspHBTDrnOvfMQzzHY In-Reply-To: X-BeenThere: linux-phy@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux Phy Mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org On 08/07/2026 08:44, Vladimir Zapolskiy wrote: > On 7/8/26 02:39, Bryan O'Donoghue wrote: >> Add a base schema initially compatible with x1e80100 to describe MIPI CSI2 >> PHY devices. >> >> The hardware can support both CPHY, DPHY and a special split-mode DPHY. >> >> The schema here defines two ports with three endpoints: >> >> port@0: Sensor input. >> endpoint@0: primary sensor >> endpoint@1: optional second sensor, implies DPHY split-mode >> >> port@1: Controller output. >> >> The CSIPHY devices have their own pinouts on the SoC as well as their own >> individual voltage rails. >> >> The need to model voltage rails on a per-PHY basis leads us to define >> CSIPHY devices as individual nodes. >> >> Two nice outcomes in terms of schema and DT arise from this change. >> >> 1. The ability to define on a per-PHY basis voltage rails. >> 2. The ability to require those voltage. >> >> We have had a complete bodge upstream for this where a single set of >> voltage rail for all CSIPHYs has been buried inside of CAMSS. >> >> Much like the I2C bus which is dedicated to Camera sensors - the CCI bus in >> CAMSS parlance, the CSIPHY devices should be individually modelled. >> >> Signed-off-by: Bryan O'Donoghue >> --- >> .../bindings/phy/qcom,x1e80100-csi2-phy.yaml | 202 +++++++++++++++++++++ >> 1 file changed, 202 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml >> new file mode 100644 >> index 0000000000000..a7fbf6804cd9e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml >> @@ -0,0 +1,202 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/qcom,x1e80100-csi2-phy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm SoC CSI2 PHY >> + >> +maintainers: >> + - Bryan O'Donoghue >> + >> +description: >> + Qualcomm MIPI CSI2 C-PHY/D-PHY combination PHY. Connects MIPI CSI2 sensors >> + to Qualcomm's Camera CSI Decoder. The PHY supports both C-PHY and D-PHY >> + modes. >> + >> +properties: >> + compatible: >> + const: qcom,x1e80100-csi2-phy >> + >> + reg: >> + maxItems: 1 >> + >> + "#phy-cells": >> + const: 1 >> + description: >> + The single cell specifies the PHY operating mode. > > Unfortunately my review comment given before was ignored before publishing > this version, thus I will repeat. > > There is a clash between the proposed phy cells value and 'bus-type' property > of the media endpoint, the proposed value of phy cells brings no information, > and therefore the whole proposed '#phy-cells' prorperty shall be removed. > > There shall be no third link introduced between CAMSS and CAMSS CSIPHYs. As explained in the cover letter: I'm following guidance from Rob on that. 20260708-x1e-csi2-phy-v9-0-0210b90c04cf@linaro.org Old guidance granted, so let me see if I can get his attention to this matter. Perhaps your suggestion is fine by him if so then fine, if not I will stick to his original nudge, either way its up to him as the original reviewer and senior schema maintainer to call this one. >> + >> + clocks: >> + maxItems: 3 >> + >> + clock-names: >> + items: >> + - const: core >> + - const: timer >> + - const: ahb >> + >> + interrupts: >> + maxItems: 1 >> + >> + operating-points-v2: true >> + >> + power-domains: >> + items: >> + - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller. >> + - description: MMCX voltage rail >> + - description: MXC or MXA voltage rail >> + >> + power-domain-names: >> + items: >> + - const: top >> + - const: mmcx >> + - const: mx > > None of the power domains finds its place in this device tree node, the > child device belongs to CAMSS, which already enables these power domains. Right, I think the whole subnode debate has run its course. Its a problem for JPEG and a bit of a fake debate, there is no real "bus" here so what is the point of mutating DT to look like a bus ? We are forever lecturing people about "making fake DT stuff for convenience". Peer nodes with TITAN_TOP_GDSC will perfectly adequately describe this hardware and the JPEG too. > >> + >> + vdda-0p8-supply: >> + description: Phandle to a 0.8V regulator supply to a PHY. > > The property name shall be vdda-0p9-supply, the description shall be > changed accordingly Hmm what ? I'll have to look back through my notes, I thought the opposite. Let me follow up in v10 after grepping for this again. > >> + >> + vdda-1p2-supply: >> + description: Phandle to 1.2V regulator supply to a PHY. >> + >> + ports: >> + $ref: /schemas/graph.yaml#/properties/ports >> + >> + properties: >> + port@0: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + description: >> + Sensor input. Always present. A single sensor is described by a >> + single endpoint with one to four data lanes. DPHY split mode, >> + where two independent sensors share the same PHY, is described >> + by two endpoints; endpoint@0 with exactly two-data lanes and >> + endpoint@1 with exactly one data-lane. >> + unevaluatedProperties: false >> + >> + patternProperties: >> + "^endpoint(@[0-9a-f]+)?$": > > This is too wide regexp mask for one or two endpoints only. Yeah you're right, this is just fluff. > >> + $ref: /schemas/media/video-interfaces.yaml# >> + unevaluatedProperties: false >> + properties: >> + data-lanes: >> + minItems: 1 >> + maxItems: 4 >> + remote-endpoint: true >> + required: >> + - data-lanes >> + - remote-endpoint >> + >> + allOf: >> + - if: >> + required: >> + - endpoint@1 >> + then: >> + properties: >> + endpoint@0: >> + properties: >> + data-lanes: >> + minItems: 2 >> + maxItems: 2 >> + endpoint@1: >> + properties: >> + data-lanes: >> + maxItems: 1 >> + required: >> + - endpoint@0 >> + >> + port@1: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + description: Output to the CAMSS CSID controller. >> + unevaluatedProperties: false >> + >> + patternProperties: >> + "^endpoint(@[0-9a-f]+)?$": >> + $ref: /schemas/graph.yaml#/$defs/endpoint-base >> + unevaluatedProperties: false >> + properties: >> + remote-endpoint: true >> + required: >> + - remote-endpoint >> + >> + required: >> + - port@0 >> + - port@1 >> + >> +required: >> + - compatible >> + - reg >> + - "#phy-cells" >> + - clocks >> + - clock-names >> + - interrupts >> + - operating-points-v2 >> + - power-domains >> + - power-domain-names >> + - vdda-0p8-supply >> + - vdda-1p2-supply >> + - ports >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include >> + #include >> + #include >> + #include >> + >> + phy@ace4000 { >> + compatible = "qcom,x1e80100-csi2-phy"; >> + reg = <0x0ace4000 0x2000>; >> + #phy-cells = <1>; >> + >> + clocks = <&camcc CAM_CC_CSIPHY0_CLK>, >> + <&camcc CAM_CC_CSI0PHYTIMER_CLK>, >> + <&camcc CAM_CC_CORE_AHB_CLK>; >> + clock-names = "core", >> + "timer", >> + "ahb"; >> + >> + interrupts = ; >> + >> + operating-points-v2 = <&csiphy_opp_table>; >> + >> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>, >> + <&rpmhpd RPMHPD_MMCX>, >> + <&rpmhpd RPMHPD_MX>; >> + power-domain-names = "top", >> + "mmcx", >> + "mx"; >> + >> + vdda-0p8-supply = <&vreg_l2c_0p8>; >> + vdda-1p2-supply = <&vreg_l1c_1p2>; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + csiphy0_in: endpoint { >> + data-lanes = <0 1 2 3>; > > The previously given review comment about numeration of lanes was > not implemented. I understand your comment but as Nihal pointed out - CAMSS has an established pattern for this - and I think his argument is convincing - we should stick to that pattern. > >> + remote-endpoint = <&sensor_out>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + csiphy0_out: endpoint { >> + remote-endpoint = <&csid_in>; >> + }; >> + }; >> + }; >> + }; >> + >> + csiphy_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-300000000 { >> + opp-hz = /bits/ 64 <300000000>; >> + required-opps = <&rpmhpd_opp_low_svs_d1>, >> + <&rpmhpd_opp_low_svs_d1>; >> + }; >> + }; >> > > -- > Best wishes, > Vladimir -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy