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;
Subject: Re: [PATCH 1/2] arm64: dts: ti: k3-j721e-main: add DP & DP PHY
To:     Rahul T R <r-ravikumar@ti.com>, <nm@ti.com>
CC:     <vigneshr@ti.com>, <kristo@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski@canonical.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <tomi.valkeinen@ideasonboard.com>,
        <laurent.pinchart@ideasonboard.com>
References: <20220222163230.1566-1-r-ravikumar@ti.com>
 <20220222163230.1566-2-r-ravikumar@ti.com>
From:   Kishon Vijay Abraham I <kishon@ti.com>
Message-ID: <069c1751-41a9-e47c-96d7-8ed83e1fe3f9@ti.com>
Date:   Mon, 28 Feb 2022 09:47:16 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20220222163230.1566-2-r-ravikumar@ti.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Rahul,

On 22/02/22 10:02 pm, Rahul T R wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Add DT nodes for DisplayPort and DisplayPort PHY. The DP is Cadence MHDP
> 8546 and the PHY is a Cadence Torrent PHY with TI WIZ wrapper.
> 
> A slight irregularity in the bindings is the DPTX PHY register block,
> which is in the MHDP IP, but is needed and mapped by the PHY.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> ---
>  arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 102 ++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index 599861259a30..9e2b212100bb 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -786,6 +786,82 @@
>  		#size-cells = <2>;
>  	};
>  
> +	serdes_wiz4: wiz@5050000 {
> +		compatible = "ti,j721e-wiz-10g";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		power-domains = <&k3_pds 297 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 297 1>, <&k3_clks 297 9>, <&cmn_refclk>;
> +		clock-names = "fck", "core_ref_clk", "ext_ref_clk";
> +		assigned-clocks = <&k3_clks 297 9>;
> +		assigned-clock-parents = <&k3_clks 297 10>;
> +		assigned-clock-rates = <19200000>;
> +		num-lanes = <4>;
> +		#reset-cells = <1>;
> +		ranges = <0x5050000 0x0 0x5050000 0x10000>,
> +			<0xa030a00 0x0 0xa030a00 0x40>;
> +
> +		wiz4_pll0_refclk: pll0-refclk {
> +			clocks = <&k3_clks 297 9>, <&cmn_refclk>;
> +			clock-output-names = "wiz4_pll0_refclk";
> +			#clock-cells = <0>;
> +			assigned-clocks = <&wiz4_pll0_refclk>;
> +			assigned-clock-parents = <&k3_clks 297 9>;
> +		};
> +
> +		wiz4_pll1_refclk: pll1-refclk {
> +			clocks = <&k3_clks 297 9>, <&cmn_refclk>;
> +			clock-output-names = "wiz4_pll1_refclk";
> +			#clock-cells = <0>;
> +			assigned-clocks = <&wiz4_pll1_refclk>;
> +			assigned-clock-parents = <&k3_clks 297 9>;
> +		};
> +
> +		wiz4_refclk_dig: refclk-dig {
> +			clocks = <&k3_clks 297 9>, <&cmn_refclk>;
> +			clock-output-names = "wiz4_refclk_dig";
> +			#clock-cells = <0>;
> +			assigned-clocks = <&wiz4_refclk_dig>;
> +			assigned-clock-parents = <&k3_clks 297 9>;
> +		};
> +
> +		wiz4_cmn_refclk_dig_div: cmn-refclk-dig-div {
> +			clocks = <&wiz4_refclk_dig>;
> +			#clock-cells = <0>;
> +		};
> +
> +		wiz4_cmn_refclk1_dig_div: cmn-refclk1-dig-div {
> +			clocks = <&wiz4_pll1_refclk>;
> +			#clock-cells = <0>;
> +		};

I'd prefer we deprecate creating clock sub-nodes for new platform additions and
use a similar approach as that used for AM64 (use phandle with a parameter to
refer clocks). Please refer how this was created for AM64
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am64-main.dtsi#n795

Since this is the same SERDES IP as used in AM64, you should be able to use the
same DT binding as that used in AM64. But if you'd like to adapt
"ti,j721e-wiz-10g" to use phandle with parameter in the WIZ driver (while making
sure the existing sub-node based binding is not broken), that is highly welcomed.

> +
> +		serdes4: serdes@5050000 {
> +			/*
> +			 * Note: we also map DPTX PHY registers as the Torrent
> +			 * needs to manage those.
> +			 */
> +			compatible = "ti,j721e-serdes-10g";
> +			reg = <0x5050000 0x10000>,
> +			      <0xa030a00 0x40>; /* DPTX PHY */
> +			reg-names = "torrent_phy", "dptx_phy";
> +
> +			resets = <&serdes_wiz4 0>;
> +			reset-names = "torrent_reset";
> +			clocks = <&wiz4_pll0_refclk>;
> +			clock-names = "refclk";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			torrent_phy_dp: phy@0 {
> +				reg = <0>;
> +				resets = <&serdes_wiz4 1>;
> +				cdns,phy-type = <PHY_TYPE_DP>;
> +				cdns,num-lanes = <4>;
> +				cdns,max-bit-rate = <5400>;
> +				#phy-cells = <0>;
> +			};

The link sub-nodes should be in the board DTS file.

Thanks,
Kishon