Message ID | 6b8d17006d8ee9a1b0c4df803c1cc7caf53ea3ef.1677749625.git.quic_varada@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp4146016wrd; Thu, 2 Mar 2023 02:00:41 -0800 (PST) X-Google-Smtp-Source: AK7set+5sjQLt0sxe8Nk6YgX6elt6a67GFzE8up7Q+wSzau4sJisPGzHukNkp1l5Qj1NtKGa64D5 X-Received: by 2002:a17:906:688a:b0:8b1:7e1f:91c5 with SMTP id n10-20020a170906688a00b008b17e1f91c5mr9207995ejr.35.1677751241310; Thu, 02 Mar 2023 02:00:41 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1677751241; cv=none; d=google.com; s=arc-20160816; b=Hiua98EnORnVy17kCRP7jtv240tvNWW9k9gzSwrIqvoFDaon49sN9y8RdGtHsLEX4U Y4JcZP4x4ft/vcso3w7a56UME+4Y2vKwJDEtIo/FZlcdnqEq4g47/rUbNW1yi2VsLgn7 SNacmRfJPHm1MSwSvF3XD3rvXReOxCTXTnMe4hTDpaU/lBRlVkBySoOwhW8z9iCAG8kb Wxy+431F6sricSmtiEiI7lNbwO34SaSv4wSJyp0t6ph7Q7hYJFU3FbWlsBn4eHTiKADY OC/d+Gm8Xv2SibOIMjtRr+uYkfyYvF27/eToWlqE9juAWQjncba/wm9q4rLNsI/g2pMu NbQA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=wnrgDLPD1lPj9ElR3wqdcZE8/tN+z68YSfzzt48b0MU=; b=yPjqrwDQxGDXY6Z1Gk31WmwLcku5FGyPdDgNXGpF8ti6eJIxKZQup2aYZnsXpHgTv6 JuT52rfNLlLdV2Mo2zvq8rLpWqFl4rOspqNh0kfX26ADG0WGBzSrQ8t4hQNHbLk8A+YX nRPdo4m3h6QPlBhgaOHMs8VbJ/7gI3DRoPBo3oWmIYLpU4thYGrhUHJnwbjYYuSQtOeI m+fCn8QlHMAg64bVEBNDIccYASG6qCl5M4Mn05EZlZ2gM526p5yzosS+r6/5pv4xxe2G /1dOcqgSOu4oL5y0orUfbmrfzBbl1NPIdJPHMIIcgP2j+nULD7kLZuGjkrcwi6HN25jm VuvA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=EeTWpj1L; 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=quicinc.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l22-20020a1709061c5600b008b17a7d569bsi14726935ejg.681.2023.03.02.02.00.19; Thu, 02 Mar 2023 02:00:41 -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=@quicinc.com header.s=qcppdkim1 header.b=EeTWpj1L; 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=quicinc.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229623AbjCBJ4f (ORCPT <rfc822;davidbtadokoro@gmail.com> + 99 others); Thu, 2 Mar 2023 04:56:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39294 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230362AbjCBJ40 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 2 Mar 2023 04:56:26 -0500 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 20D2F3B3DF; Thu, 2 Mar 2023 01:56:08 -0800 (PST) Received: from pps.filterd (m0279862.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3229YLnl014893; Thu, 2 Mar 2023 09:56:06 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-type; s=qcppdkim1; bh=wnrgDLPD1lPj9ElR3wqdcZE8/tN+z68YSfzzt48b0MU=; b=EeTWpj1Lzp9P2zYBmUPiwW915qFWrBH1KJBq8JYOPH8oaSHEL7yFplIK2c9ObS/vN9fa Vof7nlLDviNc8cIu0ttTPkA+3Mp59IqbV8IJRgz4gxWb0wpEsQIYAw2JJKzYAPlpfQDu L6T1sRHjWTBaOUf5vb4qzOSns4ncTNtycf+AyoG/SKpXYDdUQl6uTRa7OZE4VlYnDhVd yxQi29bUdAzuQARczMySCuEcfMtlNEO8o38Ga+/uM0lDh8a6S/C3F8OjrDoNa3ejIrJg lYTv3e+EBiS10TxUvrJwCLDPRlW0b0Y8SuQkMbjHkPdrE0qzPDXc1rZOBUDsunSeVouR Pw== Received: from nasanppmta02.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3p2ar125uu-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 02 Mar 2023 09:56:06 +0000 Received: from nasanex01a.na.qualcomm.com ([10.52.223.231]) by NASANPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3229u5Mr015424 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 2 Mar 2023 09:56:05 GMT Received: from varda-linux.qualcomm.com (10.80.80.8) by nasanex01a.na.qualcomm.com (10.52.223.231) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Thu, 2 Mar 2023 01:56:02 -0800 From: Varadarajan Narayanan <quic_varada@quicinc.com> To: 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> CC: Varadarajan Narayanan <quic_varada@quicinc.com>, <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes Date: Thu, 2 Mar 2023 15:25:14 +0530 Message-ID: <6b8d17006d8ee9a1b0c4df803c1cc7caf53ea3ef.1677749625.git.quic_varada@quicinc.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <cover.1677749625.git.quic_varada@quicinc.com> References: <cover.1677749625.git.quic_varada@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01a.na.qualcomm.com (10.52.223.231) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: k51sylMbX8DiY7HXUasiw-hl5g6GaB3E X-Proofpoint-ORIG-GUID: k51sylMbX8DiY7HXUasiw-hl5g6GaB3E X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-03-02_04,2023-03-02_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 mlxscore=0 bulkscore=0 impostorscore=0 phishscore=0 lowpriorityscore=0 suspectscore=0 priorityscore=1501 spamscore=0 mlxlogscore=999 adultscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2303020086 X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,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?1759249685752797005?= X-GMAIL-MSGID: =?utf-8?q?1759249685752797005?= |
Series |
Enable IPQ9754 USB
|
|
Commit Message
Varadarajan Narayanan
March 2, 2023, 9:55 a.m. UTC
Add USB phy and controller related nodes
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 +++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
Comments
On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan <quic_varada@quicinc.com> wrote: > > Add USB phy and controller related nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 +++++++++++++++++++++++++++++++++++ > 1 file changed, 92 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > index 2bb4053..319b5bd 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > @@ -215,6 +215,98 @@ > #size-cells = <1>; > ranges = <0 0 0 0xffffffff>; > > + ssphy_0: ssphy@7D000 { > + compatible = "qcom,ipq9574-qmp-usb3-phy"; > + reg = <0x7D000 0x1C4>; > + #clock-cells = <1>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clocks = <&gcc GCC_USB0_AUX_CLK>, > + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>; > + clock-names = "aux", "cfg_ahb"; > + > + resets = <&gcc GCC_USB0_PHY_BCR>, > + <&gcc GCC_USB3PHY_0_PHY_BCR>; > + reset-names = "phy","common"; > + status = "disabled"; > + > + usb0_ssphy: lane@7D200 { Please use newer style device bindings for new PHYs. > + reg = <0x0007D200 0x130>, /* Tx */ > + <0x0007D400 0x200>, /* Rx */ > + <0x0007D800 0x1F8>, /* PCS */ > + <0x0007D600 0x044>; /* PCS misc */ > + #phy-cells = <0>; > + clocks = <&gcc GCC_USB0_PIPE_CLK>; > + clock-names = "pipe0"; > + clock-output-names = "gcc_usb0_pipe_clk_src"; No, this clock doesn't originate from gcc, so the gcc prefix is incorrect. > + }; > + }; > + > + qusb_phy_0: qusb@7B000 { > + compatible = "qcom,ipq9574-qusb2-phy"; > + reg = <0x07B000 0x180>; > + #phy-cells = <0>; > + > + clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, > + <&xo_board_clk>; > + clock-names = "cfg_ahb", "ref"; > + > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + status = "disabled"; > + }; > + > + usb3: usb3@8A00000 { You know the drill. This node is in the wrong place. > + compatible = "qcom,dwc3"; > + reg = <0x8AF8800 0x400>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clocks = <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_ANOC_USB_AXI_CLK>, > + <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_USB0_SLEEP_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + > + clock-names = "sys_noc_axi", > + "anoc_axi", > + "master", > + "sleep", > + "mock_utmi"; Please fix the indentation of the lists. > + > + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_ANOC_USB_AXI_CLK>, Why do you assign clock rates to the NOC clocks? Should they be set using the interconnect instead? > + <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + assigned-clock-rates = <200000000>, > + <200000000>, > + <200000000>, > + <24000000>; > + > + resets = <&gcc GCC_USB_BCR>; > + status = "disabled"; > + > + dwc_0: dwc3@8A00000 { > + compatible = "snps,dwc3"; > + reg = <0x8A00000 0xcd00>; > + clock-names = "ref"; > + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; clocks before clock-names > + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; > + phys = <&qusb_phy_0>, <&usb0_ssphy>; > + phy-names = "usb2-phy", "usb3-phy"; > + tx-fifo-resize; > + snps,dis_ep_cache_eviction; > + snps,is-utmi-l1-suspend; > + snps,hird-threshold = /bits/ 8 <0x0>; > + snps,dis_u2_susphy_quirk; > + snps,dis_u3_susphy_quirk; > + snps,quirk-frame-length-adjustment = <0x0A87F0A0>; > + dr_mode = "host"; > + }; > + }; > + > pcie0_phy: phy@84000 { > compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy"; > reg = <0x00084000 0x1bc>; /* Serdes PLL */ > -- > 2.7.4 >
On 02/03/2023 10:55, Varadarajan Narayanan wrote: > Add USB phy and controller related nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 +++++++++++++++++++++++++++++++++++ > 1 file changed, 92 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > index 2bb4053..319b5bd 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > @@ -215,6 +215,98 @@ > #size-cells = <1>; > ranges = <0 0 0 0xffffffff>; > > + ssphy_0: ssphy@7D000 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + compatible = "qcom,ipq9574-qmp-usb3-phy"; > + reg = <0x7D000 0x1C4>; > + #clock-cells = <1>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clocks = <&gcc GCC_USB0_AUX_CLK>, > + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>; > + clock-names = "aux", "cfg_ahb"; > + > + resets = <&gcc GCC_USB0_PHY_BCR>, > + <&gcc GCC_USB3PHY_0_PHY_BCR>; > + reset-names = "phy","common"; > + status = "disabled"; > + > + usb0_ssphy: lane@7D200 { > + reg = <0x0007D200 0x130>, /* Tx */ > + <0x0007D400 0x200>, /* Rx */ > + <0x0007D800 0x1F8>, /* PCS */ > + <0x0007D600 0x044>; /* PCS misc */ > + #phy-cells = <0>; > + clocks = <&gcc GCC_USB0_PIPE_CLK>; > + clock-names = "pipe0"; > + clock-output-names = "gcc_usb0_pipe_clk_src"; > + }; > + }; > + > + qusb_phy_0: qusb@7B000 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + compatible = "qcom,ipq9574-qusb2-phy"; > + reg = <0x07B000 0x180>; Lowercase hex everywhere. > + #phy-cells = <0>; > + > + clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, > + <&xo_board_clk>; > + clock-names = "cfg_ahb", "ref"; > + > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + status = "disabled"; > + }; > + > + usb3: usb3@8A00000 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + compatible = "qcom,dwc3"; > + reg = <0x8AF8800 0x400>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clocks = <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_ANOC_USB_AXI_CLK>, > + <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_USB0_SLEEP_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + > + clock-names = "sys_noc_axi", > + "anoc_axi", > + "master", > + "sleep", > + "mock_utmi"; > + > + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_ANOC_USB_AXI_CLK>, > + <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + assigned-clock-rates = <200000000>, > + <200000000>, > + <200000000>, > + <24000000>; > + > + resets = <&gcc GCC_USB_BCR>; > + status = "disabled"; > + > + dwc_0: dwc3@8A00000 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Best regards, Krzysztof
On 3/3/2023 1:09 PM, Krzysztof Kozlowski wrote: > On 02/03/2023 10:55, Varadarajan Narayanan wrote: >> Add USB phy and controller related nodes >> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 92 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> index 2bb4053..319b5bd 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> @@ -215,6 +215,98 @@ >> #size-cells = <1>; >> ranges = <0 0 0 0xffffffff>; >> >> + ssphy_0: ssphy@7D000 { > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). > >> + compatible = "qcom,ipq9574-qmp-usb3-phy"; >> + reg = <0x7D000 0x1C4>; >> + #clock-cells = <1>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + clocks = <&gcc GCC_USB0_AUX_CLK>, >> + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>; >> + clock-names = "aux", "cfg_ahb"; >> + >> + resets = <&gcc GCC_USB0_PHY_BCR>, >> + <&gcc GCC_USB3PHY_0_PHY_BCR>; >> + reset-names = "phy","common"; >> + status = "disabled"; >> + >> + usb0_ssphy: lane@7D200 { >> + reg = <0x0007D200 0x130>, /* Tx */ >> + <0x0007D400 0x200>, /* Rx */ >> + <0x0007D800 0x1F8>, /* PCS */ >> + <0x0007D600 0x044>; /* PCS misc */ >> + #phy-cells = <0>; >> + clocks = <&gcc GCC_USB0_PIPE_CLK>; >> + clock-names = "pipe0"; >> + clock-output-names = "gcc_usb0_pipe_clk_src"; >> + }; >> + }; >> + >> + qusb_phy_0: qusb@7B000 { > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). > >> + compatible = "qcom,ipq9574-qusb2-phy"; >> + reg = <0x07B000 0x180>; > Lowercase hex everywhere. > >> + #phy-cells = <0>; >> + >> + clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, >> + <&xo_board_clk>; >> + clock-names = "cfg_ahb", "ref"; >> + >> + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; >> + status = "disabled"; >> + }; >> + >> + usb3: usb3@8A00000 { > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). > >> + compatible = "qcom,dwc3"; >> + reg = <0x8AF8800 0x400>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + clocks = <&gcc GCC_SNOC_USB_CLK>, >> + <&gcc GCC_ANOC_USB_AXI_CLK>, >> + <&gcc GCC_USB0_MASTER_CLK>, >> + <&gcc GCC_USB0_SLEEP_CLK>, >> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> + >> + clock-names = "sys_noc_axi", >> + "anoc_axi", >> + "master", >> + "sleep", >> + "mock_utmi"; >> + >> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, >> + <&gcc GCC_ANOC_USB_AXI_CLK>, >> + <&gcc GCC_USB0_MASTER_CLK>, >> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> + assigned-clock-rates = <200000000>, >> + <200000000>, >> + <200000000>, >> + <24000000>; >> + >> + resets = <&gcc GCC_USB_BCR>; >> + status = "disabled"; >> + >> + dwc_0: dwc3@8A00000 { > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). > > > Best regards, > Krzysztof > Sorry. Will rectify and post a new version. Thank Varada
Dmitry, On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: > On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan > <quic_varada@quicinc.com> wrote: >> Add USB phy and controller related nodes >> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 92 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> index 2bb4053..319b5bd 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> @@ -215,6 +215,98 @@ >> #size-cells = <1>; >> ranges = <0 0 0 0xffffffff>; >> >> + ssphy_0: ssphy@7D000 { >> + compatible = "qcom,ipq9574-qmp-usb3-phy"; >> + reg = <0x7D000 0x1C4>; >> + #clock-cells = <1>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + clocks = <&gcc GCC_USB0_AUX_CLK>, >> + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>; >> + clock-names = "aux", "cfg_ahb"; >> + >> + resets = <&gcc GCC_USB0_PHY_BCR>, >> + <&gcc GCC_USB3PHY_0_PHY_BCR>; >> + reset-names = "phy","common"; >> + status = "disabled"; >> + >> + usb0_ssphy: lane@7D200 { > Please use newer style device bindings for new PHYs. > >> + reg = <0x0007D200 0x130>, /* Tx */ >> + <0x0007D400 0x200>, /* Rx */ >> + <0x0007D800 0x1F8>, /* PCS */ >> + <0x0007D600 0x044>; /* PCS misc */ >> + #phy-cells = <0>; >> + clocks = <&gcc GCC_USB0_PIPE_CLK>; >> + clock-names = "pipe0"; >> + clock-output-names = "gcc_usb0_pipe_clk_src"; > No, this clock doesn't originate from gcc, so the gcc prefix is incorrect. > >> + }; >> + }; >> + >> + qusb_phy_0: qusb@7B000 { >> + compatible = "qcom,ipq9574-qusb2-phy"; >> + reg = <0x07B000 0x180>; >> + #phy-cells = <0>; >> + >> + clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, >> + <&xo_board_clk>; >> + clock-names = "cfg_ahb", "ref"; >> + >> + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; >> + status = "disabled"; >> + }; >> + >> + usb3: usb3@8A00000 { > You know the drill. This node is in the wrong place. > >> + compatible = "qcom,dwc3"; >> + reg = <0x8AF8800 0x400>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + clocks = <&gcc GCC_SNOC_USB_CLK>, >> + <&gcc GCC_ANOC_USB_AXI_CLK>, >> + <&gcc GCC_USB0_MASTER_CLK>, >> + <&gcc GCC_USB0_SLEEP_CLK>, >> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> + >> + clock-names = "sys_noc_axi", >> + "anoc_axi", >> + "master", >> + "sleep", >> + "mock_utmi"; > Please fix the indentation of the lists. > >> + >> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, >> + <&gcc GCC_ANOC_USB_AXI_CLK>, > Why do you assign clock rates to the NOC clocks? Should they be set > using the interconnect instead? The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz respectively and are not scaled. These clocks are for the interface between the USB block and the SNOC/ANOC. Do we still need to use interconnect? >> + <&gcc GCC_USB0_MASTER_CLK>, >> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> + assigned-clock-rates = <200000000>, >> + <200000000>, >> + <200000000>, >> + <24000000>; >> + >> + resets = <&gcc GCC_USB_BCR>; >> + status = "disabled"; >> + >> + dwc_0: dwc3@8A00000 { >> + compatible = "snps,dwc3"; >> + reg = <0x8A00000 0xcd00>; >> + clock-names = "ref"; >> + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; > clocks before clock-names > >> + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; >> + phys = <&qusb_phy_0>, <&usb0_ssphy>; >> + phy-names = "usb2-phy", "usb3-phy"; >> + tx-fifo-resize; >> + snps,dis_ep_cache_eviction; >> + snps,is-utmi-l1-suspend; >> + snps,hird-threshold = /bits/ 8 <0x0>; >> + snps,dis_u2_susphy_quirk; >> + snps,dis_u3_susphy_quirk; >> + snps,quirk-frame-length-adjustment = <0x0A87F0A0>; >> + dr_mode = "host"; >> + }; >> + }; >> + >> pcie0_phy: phy@84000 { >> compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy"; >> reg = <0x00084000 0x1bc>; /* Serdes PLL */ >> -- >> 2.7.4 Will address these and post a new revision. Thanks Varada
On 06/03/2023 13:26, Varadarajan Narayanan wrote: > Dmitry, > > On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: >> On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan >> <quic_varada@quicinc.com> wrote: >>> Add USB phy and controller related nodes >>> >>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >>> --- >>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 >>> +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 92 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>> index 2bb4053..319b5bd 100644 >>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi [skipped] >>> + usb3: usb3@8A00000 { >> You know the drill. This node is in the wrong place. >> >>> + compatible = "qcom,dwc3"; >>> + reg = <0x8AF8800 0x400>; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges; >>> + >>> + clocks = <&gcc GCC_SNOC_USB_CLK>, >>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >>> + <&gcc GCC_USB0_MASTER_CLK>, >>> + <&gcc GCC_USB0_SLEEP_CLK>, >>> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >>> + >>> + clock-names = "sys_noc_axi", >>> + "anoc_axi", >>> + "master", >>> + "sleep", >>> + "mock_utmi"; >> Please fix the indentation of the lists. >> >>> + >>> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, >>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >> Why do you assign clock rates to the NOC clocks? Should they be set >> using the interconnect instead? > > The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz respectively > and are not scaled. These clocks are for the interface between the USB > block and the SNOC/ANOC. Do we still need to use interconnect? Maybe I misunderstand something here. If the snoc and anoc speeds are at 350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz? Is it enough to call clk_prepare_enable() for these clocks or the rate really needs to be set? > >>> + <&gcc GCC_USB0_MASTER_CLK>, >>> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >>> + assigned-clock-rates = <200000000>, >>> + <200000000>, >>> + <200000000>, >>> + <24000000>; >>> + >>> + resets = <&gcc GCC_USB_BCR>; >>> + status = "disabled"; >>> + >>> + dwc_0: dwc3@8A00000 { >>> + compatible = "snps,dwc3"; >>> + reg = <0x8A00000 0xcd00>; >>> + clock-names = "ref"; >>> + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; >> clocks before clock-names >> >>> + interrupts = <GIC_SPI 140 >>> IRQ_TYPE_LEVEL_HIGH>; >>> + phys = <&qusb_phy_0>, <&usb0_ssphy>; >>> + phy-names = "usb2-phy", "usb3-phy"; >>> + tx-fifo-resize; >>> + snps,dis_ep_cache_eviction; >>> + snps,is-utmi-l1-suspend; >>> + snps,hird-threshold = /bits/ 8 <0x0>; >>> + snps,dis_u2_susphy_quirk; >>> + snps,dis_u3_susphy_quirk; >>> + snps,quirk-frame-length-adjustment = >>> <0x0A87F0A0>; >>> + dr_mode = "host"; >>> + }; >>> + }; >>> + >>> pcie0_phy: phy@84000 { >>> compatible = >>> "qcom,ipq9574-qmp-gen3x1-pcie-phy"; >>> reg = <0x00084000 0x1bc>; /* Serdes PLL */ >>> -- >>> 2.7.4 > > Will address these and post a new revision. > > Thanks > > Varada >
On 3/6/2023 5:21 PM, Dmitry Baryshkov wrote: > On 06/03/2023 13:26, Varadarajan Narayanan wrote: >> Dmitry, >> >> On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: >>> On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan >>> <quic_varada@quicinc.com> wrote: >>>> Add USB phy and controller related nodes >>>> >>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >>>> --- >>>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 >>>> +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 92 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>> index 2bb4053..319b5bd 100644 >>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > [skipped] > > >>>> + usb3: usb3@8A00000 { >>> You know the drill. This node is in the wrong place. >>> >>>> + compatible = "qcom,dwc3"; >>>> + reg = <0x8AF8800 0x400>; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + ranges; >>>> + >>>> + clocks = <&gcc GCC_SNOC_USB_CLK>, >>>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >>>> + <&gcc GCC_USB0_MASTER_CLK>, >>>> + <&gcc GCC_USB0_SLEEP_CLK>, >>>> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >>>> + >>>> + clock-names = "sys_noc_axi", >>>> + "anoc_axi", >>>> + "master", >>>> + "sleep", >>>> + "mock_utmi"; >>> Please fix the indentation of the lists. >>> >>>> + >>>> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, >>>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >>> Why do you assign clock rates to the NOC clocks? Should they be set >>> using the interconnect instead? >> >> The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz >> respectively and are not scaled. These clocks are for the interface >> between the USB block and the SNOC/ANOC. Do we still need to use >> interconnect? > > Maybe I misunderstand something here. If the snoc and anoc speeds are > at 350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz? > > Is it enough to call clk_prepare_enable() for these clocks or the rate > really needs to be set? The rate of 200MHz is not being set for the SNOC/ANOC. It is for the NIU that connects the USB and SNOC/ANOC. The reason for setting the rate to 200MHz is to configure the RCG parent for these interface clocks. That said can we configure this RCG standalone in the driver and enable these clocks? Thanks Varada > > >> >>>> + <&gcc GCC_USB0_MASTER_CLK>, >>>> + <&gcc >>>> GCC_USB0_MOCK_UTMI_CLK>; >>>> + assigned-clock-rates = <200000000>, >>>> + <200000000>, >>>> + <200000000>, >>>> + <24000000>; >>>> + >>>> + resets = <&gcc GCC_USB_BCR>; >>>> + status = "disabled"; >>>> + >>>> + dwc_0: dwc3@8A00000 { >>>> + compatible = "snps,dwc3"; >>>> + reg = <0x8A00000 0xcd00>; >>>> + clock-names = "ref"; >>>> + clocks = <&gcc >>>> GCC_USB0_MOCK_UTMI_CLK>; >>> clocks before clock-names >>> >>>> + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; >>>> + phys = <&qusb_phy_0>, <&usb0_ssphy>; >>>> + phy-names = "usb2-phy", "usb3-phy"; >>>> + tx-fifo-resize; >>>> + snps,dis_ep_cache_eviction; >>>> + snps,is-utmi-l1-suspend; >>>> + snps,hird-threshold = /bits/ 8 <0x0>; >>>> + snps,dis_u2_susphy_quirk; >>>> + snps,dis_u3_susphy_quirk; >>>> + snps,quirk-frame-length-adjustment = <0x0A87F0A0>; >>>> + dr_mode = "host"; >>>> + }; >>>> + }; >>>> + >>>> pcie0_phy: phy@84000 { >>>> compatible = >>>> "qcom,ipq9574-qmp-gen3x1-pcie-phy"; >>>> reg = <0x00084000 0x1bc>; /* Serdes PLL */ >>>> -- >>>> 2.7.4 >> >> Will address these and post a new revision. >> >> Thanks >> >> Varada >> >
On 07/03/2023 08:36, Varadarajan Narayanan wrote: > > On 3/6/2023 5:21 PM, Dmitry Baryshkov wrote: >> On 06/03/2023 13:26, Varadarajan Narayanan wrote: >>> Dmitry, >>> >>> On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: >>>> On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan >>>> <quic_varada@quicinc.com> wrote: >>>>> Add USB phy and controller related nodes >>>>> >>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 >>>>> +++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 92 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>>> index 2bb4053..319b5bd 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> >> [skipped] >> >> >>>>> + usb3: usb3@8A00000 { >>>> You know the drill. This node is in the wrong place. >>>> >>>>> + compatible = "qcom,dwc3"; >>>>> + reg = <0x8AF8800 0x400>; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <1>; >>>>> + ranges; >>>>> + >>>>> + clocks = <&gcc GCC_SNOC_USB_CLK>, >>>>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >>>>> + <&gcc GCC_USB0_MASTER_CLK>, >>>>> + <&gcc GCC_USB0_SLEEP_CLK>, >>>>> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >>>>> + >>>>> + clock-names = "sys_noc_axi", >>>>> + "anoc_axi", >>>>> + "master", >>>>> + "sleep", >>>>> + "mock_utmi"; >>>> Please fix the indentation of the lists. >>>> >>>>> + >>>>> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, >>>>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >>>> Why do you assign clock rates to the NOC clocks? Should they be set >>>> using the interconnect instead? >>> >>> The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz >>> respectively and are not scaled. These clocks are for the interface >>> between the USB block and the SNOC/ANOC. Do we still need to use >>> interconnect? >> >> Maybe I misunderstand something here. If the snoc and anoc speeds are >> at 350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz? >> >> Is it enough to call clk_prepare_enable() for these clocks or the rate >> really needs to be set? > > The rate of 200MHz is not being set for the SNOC/ANOC. It is for the > NIU that connects the USB and SNOC/ANOC. The reason for setting the > rate to 200MHz is to configure the RCG parent for these interface > clocks. That said can we configure this RCG standalone in the driver > and enable these clocks? We discussed this separately with Georgi Djakov. Let me quote his IRC message: "it sounds like this is for USB port that connects to the NOC. if bandwidth scaling is not needed (or other interconnect configuration), then maybe this can go without interconnect provider driver." However as we discover more and more about this platform (e.g. PCIe using the aggre_noc region to setup some magic registers, see [1]), I'm more and more biased towards suggesting implementing the interconnect driver to setup all these tiny little things. With the DT tree being an ABI, it is much preferable to overestimate the needs rather than underestimating them (and having to cope with the backwards compatibility issues). Generally I think that PCIe/USB/whatever should not poke into NoC registers or NoC/NIU clocks directly (because this is a very platform-specific item). Rather than that it should tell the icc/opp/whatever subsystem, "please configure the SoC for me to work". [1] https://lore.kernel.org/linux-arm-msm/30cf9717-dcca-e984-c506-c71b7f8e32cd@quicinc.com/ > > Thanks > Varada > > >> >> >>> >>>>> + <&gcc GCC_USB0_MASTER_CLK>, >>>>> + <&gcc >>>>> GCC_USB0_MOCK_UTMI_CLK>; >>>>> + assigned-clock-rates = <200000000>, >>>>> + <200000000>, >>>>> + <200000000>, >>>>> + <24000000>; >>>>> + >>>>> + resets = <&gcc GCC_USB_BCR>; >>>>> + status = "disabled"; >>>>> + >>>>> + dwc_0: dwc3@8A00000 { >>>>> + compatible = "snps,dwc3"; >>>>> + reg = <0x8A00000 0xcd00>; >>>>> + clock-names = "ref"; >>>>> + clocks = <&gcc >>>>> GCC_USB0_MOCK_UTMI_CLK>; >>>> clocks before clock-names >>>> >>>>> + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; >>>>> + phys = <&qusb_phy_0>, <&usb0_ssphy>; >>>>> + phy-names = "usb2-phy", "usb3-phy"; >>>>> + tx-fifo-resize; >>>>> + snps,dis_ep_cache_eviction; >>>>> + snps,is-utmi-l1-suspend; >>>>> + snps,hird-threshold = /bits/ 8 <0x0>; >>>>> + snps,dis_u2_susphy_quirk; >>>>> + snps,dis_u3_susphy_quirk; >>>>> + snps,quirk-frame-length-adjustment = <0x0A87F0A0>; >>>>> + dr_mode = "host"; >>>>> + }; >>>>> + }; >>>>> + >>>>> pcie0_phy: phy@84000 { >>>>> compatible = >>>>> "qcom,ipq9574-qmp-gen3x1-pcie-phy"; >>>>> reg = <0x00084000 0x1bc>; /* Serdes PLL */ >>>>> -- >>>>> 2.7.4 >>> >>> Will address these and post a new revision. >>> >>> Thanks >>> >>> Varada >>> >>
On 3/7/2023 5:19 PM, Dmitry Baryshkov wrote: > On 07/03/2023 08:36, Varadarajan Narayanan wrote: >> >> On 3/6/2023 5:21 PM, Dmitry Baryshkov wrote: >>> On 06/03/2023 13:26, Varadarajan Narayanan wrote: >>>> Dmitry, >>>> >>>> On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: >>>>> On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan >>>>> <quic_varada@quicinc.com> wrote: >>>>>> Add USB phy and controller related nodes >>>>>> >>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >>>>>> --- >>>>>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 >>>>>> +++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 92 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>>>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>>>> index 2bb4053..319b5bd 100644 >>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >>> >>> [skipped] >>> >>> >>>>>> + usb3: usb3@8A00000 { >>>>> You know the drill. This node is in the wrong place. >>>>> >>>>>> + compatible = "qcom,dwc3"; >>>>>> + reg = <0x8AF8800 0x400>; >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <1>; >>>>>> + ranges; >>>>>> + >>>>>> + clocks = <&gcc GCC_SNOC_USB_CLK>, >>>>>> + <&gcc GCC_ANOC_USB_AXI_CLK>, >>>>>> + <&gcc GCC_USB0_MASTER_CLK>, >>>>>> + <&gcc GCC_USB0_SLEEP_CLK>, >>>>>> + <&gcc GCC_USB0_MOCK_UTMI_CLK>; >>>>>> + >>>>>> + clock-names = "sys_noc_axi", >>>>>> + "anoc_axi", >>>>>> + "master", >>>>>> + "sleep", >>>>>> + "mock_utmi"; >>>>> Please fix the indentation of the lists. >>>>> >>>>>> + >>>>>> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, >>>>>> + <&gcc >>>>>> GCC_ANOC_USB_AXI_CLK>, >>>>> Why do you assign clock rates to the NOC clocks? Should they be set >>>>> using the interconnect instead? >>>> >>>> The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz >>>> respectively and are not scaled. These clocks are for the interface >>>> between the USB block and the SNOC/ANOC. Do we still need to use >>>> interconnect? >>> >>> Maybe I misunderstand something here. If the snoc and anoc speeds >>> are at 350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz? >>> >>> Is it enough to call clk_prepare_enable() for these clocks or the >>> rate really needs to be set? >> >> The rate of 200MHz is not being set for the SNOC/ANOC. It is for the >> NIU that connects the USB and SNOC/ANOC. The reason for setting the >> rate to 200MHz is to configure the RCG parent for these interface >> clocks. That said can we configure this RCG standalone in the driver >> and enable these clocks? > > We discussed this separately with Georgi Djakov. Let me quote his IRC > message: "it sounds like this is for USB port that connects to the > NOC. if bandwidth scaling is not needed (or other interconnect > configuration), then maybe this can go without interconnect provider > driver." > > However as we discover more and more about this platform (e.g. PCIe > using the aggre_noc region to setup some magic registers, see [1]), > I'm more and more biased towards suggesting implementing the > interconnect driver to setup all these tiny little things. With the DT > tree being an ABI, it is much preferable to overestimate the needs > rather than underestimating them (and having to cope with the > backwards compatibility issues). > > Generally I think that PCIe/USB/whatever should not poke into NoC > registers or NoC/NIU clocks directly (because this is a very > platform-specific item). Rather than that it should tell the > icc/opp/whatever subsystem, "please configure the SoC for me to work". > > [1] > https://lore.kernel.org/linux-arm-msm/30cf9717-dcca-e984-c506-c71b7f8e32cd@quicinc.com/ Sure, will post a new revision that uses interconnect subsystem. Thanks Varada >>>>>> + <&gcc GCC_USB0_MASTER_CLK>, >>>>>> + <&gcc >>>>>> GCC_USB0_MOCK_UTMI_CLK>; >>>>>> + assigned-clock-rates = <200000000>, >>>>>> + <200000000>, >>>>>> + <200000000>, >>>>>> + <24000000>; >>>>>> + >>>>>> + resets = <&gcc GCC_USB_BCR>; >>>>>> + status = "disabled"; >>>>>> + >>>>>> + dwc_0: dwc3@8A00000 { >>>>>> + compatible = "snps,dwc3"; >>>>>> + reg = <0x8A00000 0xcd00>; >>>>>> + clock-names = "ref"; >>>>>> + clocks = <&gcc >>>>>> GCC_USB0_MOCK_UTMI_CLK>; >>>>> clocks before clock-names >>>>> >>>>>> + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; >>>>>> + phys = <&qusb_phy_0>, <&usb0_ssphy>; >>>>>> + phy-names = "usb2-phy", "usb3-phy"; >>>>>> + tx-fifo-resize; >>>>>> + snps,dis_ep_cache_eviction; >>>>>> + snps,is-utmi-l1-suspend; >>>>>> + snps,hird-threshold = /bits/ 8 >>>>>> <0x0>; >>>>>> + snps,dis_u2_susphy_quirk; >>>>>> + snps,dis_u3_susphy_quirk; >>>>>> + snps,quirk-frame-length-adjustment = <0x0A87F0A0>; >>>>>> + dr_mode = "host"; >>>>>> + }; >>>>>> + }; >>>>>> + >>>>>> pcie0_phy: phy@84000 { >>>>>> compatible = >>>>>> "qcom,ipq9574-qmp-gen3x1-pcie-phy"; >>>>>> reg = <0x00084000 0x1bc>; /* Serdes PLL */ >>>>>> -- >>>>>> 2.7.4 >>>> >>>> Will address these and post a new revision. >>>> >>>> Thanks >>>> >>>> Varada >>>> >>> >
Dmitry, On Tue, Mar 07, 2023 at 01:49:40PM +0200, Dmitry Baryshkov wrote: > On 07/03/2023 08:36, Varadarajan Narayanan wrote: > > > >On 3/6/2023 5:21 PM, Dmitry Baryshkov wrote: > >>On 06/03/2023 13:26, Varadarajan Narayanan wrote: > >>>Dmitry, > >>> > >>>On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: > >>>>On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan > >>>><quic_varada@quicinc.com> wrote: > >>>>>Add USB phy and controller related nodes > >>>>> > >>>>>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > >>>>>--- > >>>>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 > >>>>>+++++++++++++++++++++++++++++++++++ > >>>>> 1 file changed, 92 insertions(+) > >>>>> > >>>>>diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > >>>>>b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > >>>>>index 2bb4053..319b5bd 100644 > >>>>>--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > >>>>>+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > >> > >>[skipped] > >> > >> > >>>>>+ usb3: usb3@8A00000 { > >>>>You know the drill. This node is in the wrong place. > >>>> > >>>>>+ compatible = "qcom,dwc3"; > >>>>>+ reg = <0x8AF8800 0x400>; > >>>>>+ #address-cells = <1>; > >>>>>+ #size-cells = <1>; > >>>>>+ ranges; > >>>>>+ > >>>>>+ clocks = <&gcc GCC_SNOC_USB_CLK>, > >>>>>+ <&gcc GCC_ANOC_USB_AXI_CLK>, > >>>>>+ <&gcc GCC_USB0_MASTER_CLK>, > >>>>>+ <&gcc GCC_USB0_SLEEP_CLK>, > >>>>>+ <&gcc GCC_USB0_MOCK_UTMI_CLK>; > >>>>>+ > >>>>>+ clock-names = "sys_noc_axi", > >>>>>+ "anoc_axi", > >>>>>+ "master", > >>>>>+ "sleep", > >>>>>+ "mock_utmi"; > >>>>Please fix the indentation of the lists. > >>>> > >>>>>+ > >>>>>+ assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, > >>>>>+ <&gcc GCC_ANOC_USB_AXI_CLK>, > >>>>Why do you assign clock rates to the NOC clocks? Should they be set > >>>>using the interconnect instead? > >>> > >>>The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz > >>>respectively and are not scaled. These clocks are for the interface > >>>between the USB block and the SNOC/ANOC. Do we still need to use > >>>interconnect? > >> > >>Maybe I misunderstand something here. If the snoc and anoc speeds are at > >>350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz? > >> > >>Is it enough to call clk_prepare_enable() for these clocks or the rate > >>really needs to be set? > > > >The rate of 200MHz is not being set for the SNOC/ANOC. It is for the > >NIU that connects the USB and SNOC/ANOC. The reason for setting the > >rate to 200MHz is to configure the RCG parent for these interface > >clocks. That said can we configure this RCG standalone in the driver > >and enable these clocks? > > We discussed this separately with Georgi Djakov. Let me quote his IRC > message: "it sounds like this is for USB port that connects to the NOC. if > bandwidth scaling is not needed (or other interconnect configuration), then > maybe this can go without interconnect provider driver." > > However as we discover more and more about this platform (e.g. PCIe using > the aggre_noc region to setup some magic registers, see [1]), I'm more and > more biased towards suggesting implementing the interconnect driver to setup > all these tiny little things. With the DT tree being an ABI, it is much > preferable to overestimate the needs rather than underestimating them (and > having to cope with the backwards compatibility issues). > > Generally I think that PCIe/USB/whatever should not poke into NoC registers > or NoC/NIU clocks directly (because this is a very platform-specific item). > Rather than that it should tell the icc/opp/whatever subsystem, "please > configure the SoC for me to work". > > [1] https://lore.kernel.org/linux-arm-msm/30cf9717-dcca-e984-c506-c71b7f8e32cd@quicinc.com/ Dmitry, Can we remove the interconnect clocks in the next patch version (and assume that the boot loader configures them)? And add these clocks once the interconnect support is available. Thanks Varada > > > > >Thanks > >Varada > > > > > >> > >> > >>> > >>>>>+ <&gcc GCC_USB0_MASTER_CLK>, > >>>>>+ <&gcc > >>>>>GCC_USB0_MOCK_UTMI_CLK>; > >>>>>+ assigned-clock-rates = <200000000>, > >>>>>+ <200000000>, > >>>>>+ <200000000>, > >>>>>+ <24000000>; > >>>>>+ > >>>>>+ resets = <&gcc GCC_USB_BCR>; > >>>>>+ status = "disabled"; > >>>>>+ > >>>>>+ dwc_0: dwc3@8A00000 { > >>>>>+ compatible = "snps,dwc3"; > >>>>>+ reg = <0x8A00000 0xcd00>; > >>>>>+ clock-names = "ref"; > >>>>>+ clocks = <&gcc > >>>>>GCC_USB0_MOCK_UTMI_CLK>; > >>>>clocks before clock-names > >>>> > >>>>>+ interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; > >>>>>+ phys = <&qusb_phy_0>, <&usb0_ssphy>; > >>>>>+ phy-names = "usb2-phy", "usb3-phy"; > >>>>>+ tx-fifo-resize; > >>>>>+ snps,dis_ep_cache_eviction; > >>>>>+ snps,is-utmi-l1-suspend; > >>>>>+ snps,hird-threshold = /bits/ 8 <0x0>; > >>>>>+ snps,dis_u2_susphy_quirk; > >>>>>+ snps,dis_u3_susphy_quirk; > >>>>>+ snps,quirk-frame-length-adjustment = <0x0A87F0A0>; > >>>>>+ dr_mode = "host"; > >>>>>+ }; > >>>>>+ }; > >>>>>+ > >>>>> pcie0_phy: phy@84000 { > >>>>> compatible = > >>>>>"qcom,ipq9574-qmp-gen3x1-pcie-phy"; > >>>>> reg = <0x00084000 0x1bc>; /* Serdes PLL */ > >>>>>-- > >>>>>2.7.4 > >>> > >>>Will address these and post a new revision. > >>> > >>>Thanks > >>> > >>>Varada > >>> > >> > > -- > With best wishes > Dmitry >
On Thu, Mar 16, 2023 at 12:00:09PM +0530, Varadarajan Narayanan wrote: > Dmitry, > > On Tue, Mar 07, 2023 at 01:49:40PM +0200, Dmitry Baryshkov wrote: > > On 07/03/2023 08:36, Varadarajan Narayanan wrote: > > > > > >On 3/6/2023 5:21 PM, Dmitry Baryshkov wrote: > > >>On 06/03/2023 13:26, Varadarajan Narayanan wrote: > > >>>Dmitry, > > >>> > > >>>On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote: > > >>>>On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan > > >>>><quic_varada@quicinc.com> wrote: > > >>>>>Add USB phy and controller related nodes > > >>>>> > > >>>>>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > >>>>>--- > > >>>>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92 > > >>>>>+++++++++++++++++++++++++++++++++++ > > >>>>> 1 file changed, 92 insertions(+) > > >>>>> > > >>>>>diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > >>>>>b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > >>>>>index 2bb4053..319b5bd 100644 > > >>>>>--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > >>>>>+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > >> > > >>[skipped] > > >> > > >> > > >>>>>+ usb3: usb3@8A00000 { > > >>>>You know the drill. This node is in the wrong place. > > >>>> > > >>>>>+ compatible = "qcom,dwc3"; > > >>>>>+ reg = <0x8AF8800 0x400>; > > >>>>>+ #address-cells = <1>; > > >>>>>+ #size-cells = <1>; > > >>>>>+ ranges; > > >>>>>+ > > >>>>>+ clocks = <&gcc GCC_SNOC_USB_CLK>, > > >>>>>+ <&gcc GCC_ANOC_USB_AXI_CLK>, > > >>>>>+ <&gcc GCC_USB0_MASTER_CLK>, > > >>>>>+ <&gcc GCC_USB0_SLEEP_CLK>, > > >>>>>+ <&gcc GCC_USB0_MOCK_UTMI_CLK>; > > >>>>>+ > > >>>>>+ clock-names = "sys_noc_axi", > > >>>>>+ "anoc_axi", > > >>>>>+ "master", > > >>>>>+ "sleep", > > >>>>>+ "mock_utmi"; > > >>>>Please fix the indentation of the lists. > > >>>> > > >>>>>+ > > >>>>>+ assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, > > >>>>>+ <&gcc GCC_ANOC_USB_AXI_CLK>, > > >>>>Why do you assign clock rates to the NOC clocks? Should they be set > > >>>>using the interconnect instead? > > >>> > > >>>The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz > > >>>respectively and are not scaled. These clocks are for the interface > > >>>between the USB block and the SNOC/ANOC. Do we still need to use > > >>>interconnect? > > >> > > >>Maybe I misunderstand something here. If the snoc and anoc speeds are at > > >>350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz? > > >> > > >>Is it enough to call clk_prepare_enable() for these clocks or the rate > > >>really needs to be set? > > > > > >The rate of 200MHz is not being set for the SNOC/ANOC. It is for the > > >NIU that connects the USB and SNOC/ANOC. The reason for setting the > > >rate to 200MHz is to configure the RCG parent for these interface > > >clocks. That said can we configure this RCG standalone in the driver > > >and enable these clocks? > > > > We discussed this separately with Georgi Djakov. Let me quote his IRC > > message: "it sounds like this is for USB port that connects to the NOC. if > > bandwidth scaling is not needed (or other interconnect configuration), then > > maybe this can go without interconnect provider driver." > > > > However as we discover more and more about this platform (e.g. PCIe using > > the aggre_noc region to setup some magic registers, see [1]), I'm more and > > more biased towards suggesting implementing the interconnect driver to setup > > all these tiny little things. With the DT tree being an ABI, it is much > > preferable to overestimate the needs rather than underestimating them (and > > having to cope with the backwards compatibility issues). > > > > Generally I think that PCIe/USB/whatever should not poke into NoC registers > > or NoC/NIU clocks directly (because this is a very platform-specific item). > > Rather than that it should tell the icc/opp/whatever subsystem, "please > > configure the SoC for me to work". > > > > [1] https://lore.kernel.org/linux-arm-msm/30cf9717-dcca-e984-c506-c71b7f8e32cd@quicinc.com/ > > Dmitry, > Can we remove the interconnect clocks in the next patch > version (and assume that the boot loader configures them)? > > And add these clocks once the interconnect support is available. > Yes, that should work. If you do not care about interconnect scaling, then let the bootloader set whatever fixed bandwidth is required for the peripherals. Thinking from the usecase perspective of these router chipsets, you won't need interconnect provider driver as of now. Thanks, Mani > Thanks > Varada > > > > > > > > >Thanks > > >Varada > > > > > > > > >> > > >> > > >>> > > >>>>>+ <&gcc GCC_USB0_MASTER_CLK>, > > >>>>>+ <&gcc > > >>>>>GCC_USB0_MOCK_UTMI_CLK>; > > >>>>>+ assigned-clock-rates = <200000000>, > > >>>>>+ <200000000>, > > >>>>>+ <200000000>, > > >>>>>+ <24000000>; > > >>>>>+ > > >>>>>+ resets = <&gcc GCC_USB_BCR>; > > >>>>>+ status = "disabled"; > > >>>>>+ > > >>>>>+ dwc_0: dwc3@8A00000 { > > >>>>>+ compatible = "snps,dwc3"; > > >>>>>+ reg = <0x8A00000 0xcd00>; > > >>>>>+ clock-names = "ref"; > > >>>>>+ clocks = <&gcc > > >>>>>GCC_USB0_MOCK_UTMI_CLK>; > > >>>>clocks before clock-names > > >>>> > > >>>>>+ interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; > > >>>>>+ phys = <&qusb_phy_0>, <&usb0_ssphy>; > > >>>>>+ phy-names = "usb2-phy", "usb3-phy"; > > >>>>>+ tx-fifo-resize; > > >>>>>+ snps,dis_ep_cache_eviction; > > >>>>>+ snps,is-utmi-l1-suspend; > > >>>>>+ snps,hird-threshold = /bits/ 8 <0x0>; > > >>>>>+ snps,dis_u2_susphy_quirk; > > >>>>>+ snps,dis_u3_susphy_quirk; > > >>>>>+ snps,quirk-frame-length-adjustment = <0x0A87F0A0>; > > >>>>>+ dr_mode = "host"; > > >>>>>+ }; > > >>>>>+ }; > > >>>>>+ > > >>>>> pcie0_phy: phy@84000 { > > >>>>> compatible = > > >>>>>"qcom,ipq9574-qmp-gen3x1-pcie-phy"; > > >>>>> reg = <0x00084000 0x1bc>; /* Serdes PLL */ > > >>>>>-- > > >>>>>2.7.4 > > >>> > > >>>Will address these and post a new revision. > > >>> > > >>>Thanks > > >>> > > >>>Varada > > >>> > > >> > > > > -- > > With best wishes > > Dmitry > >
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi index 2bb4053..319b5bd 100644 --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi @@ -215,6 +215,98 @@ #size-cells = <1>; ranges = <0 0 0 0xffffffff>; + ssphy_0: ssphy@7D000 { + compatible = "qcom,ipq9574-qmp-usb3-phy"; + reg = <0x7D000 0x1C4>; + #clock-cells = <1>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + clocks = <&gcc GCC_USB0_AUX_CLK>, + <&gcc GCC_USB0_PHY_CFG_AHB_CLK>; + clock-names = "aux", "cfg_ahb"; + + resets = <&gcc GCC_USB0_PHY_BCR>, + <&gcc GCC_USB3PHY_0_PHY_BCR>; + reset-names = "phy","common"; + status = "disabled"; + + usb0_ssphy: lane@7D200 { + reg = <0x0007D200 0x130>, /* Tx */ + <0x0007D400 0x200>, /* Rx */ + <0x0007D800 0x1F8>, /* PCS */ + <0x0007D600 0x044>; /* PCS misc */ + #phy-cells = <0>; + clocks = <&gcc GCC_USB0_PIPE_CLK>; + clock-names = "pipe0"; + clock-output-names = "gcc_usb0_pipe_clk_src"; + }; + }; + + qusb_phy_0: qusb@7B000 { + compatible = "qcom,ipq9574-qusb2-phy"; + reg = <0x07B000 0x180>; + #phy-cells = <0>; + + clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>, + <&xo_board_clk>; + clock-names = "cfg_ahb", "ref"; + + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; + status = "disabled"; + }; + + usb3: usb3@8A00000 { + compatible = "qcom,dwc3"; + reg = <0x8AF8800 0x400>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + clocks = <&gcc GCC_SNOC_USB_CLK>, + <&gcc GCC_ANOC_USB_AXI_CLK>, + <&gcc GCC_USB0_MASTER_CLK>, + <&gcc GCC_USB0_SLEEP_CLK>, + <&gcc GCC_USB0_MOCK_UTMI_CLK>; + + clock-names = "sys_noc_axi", + "anoc_axi", + "master", + "sleep", + "mock_utmi"; + + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>, + <&gcc GCC_ANOC_USB_AXI_CLK>, + <&gcc GCC_USB0_MASTER_CLK>, + <&gcc GCC_USB0_MOCK_UTMI_CLK>; + assigned-clock-rates = <200000000>, + <200000000>, + <200000000>, + <24000000>; + + resets = <&gcc GCC_USB_BCR>; + status = "disabled"; + + dwc_0: dwc3@8A00000 { + compatible = "snps,dwc3"; + reg = <0x8A00000 0xcd00>; + clock-names = "ref"; + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>; + phys = <&qusb_phy_0>, <&usb0_ssphy>; + phy-names = "usb2-phy", "usb3-phy"; + tx-fifo-resize; + snps,dis_ep_cache_eviction; + snps,is-utmi-l1-suspend; + snps,hird-threshold = /bits/ 8 <0x0>; + snps,dis_u2_susphy_quirk; + snps,dis_u3_susphy_quirk; + snps,quirk-frame-length-adjustment = <0x0A87F0A0>; + dr_mode = "host"; + }; + }; + pcie0_phy: phy@84000 { compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy"; reg = <0x00084000 0x1bc>; /* Serdes PLL */