Message ID | 20231127-refclk_always_on-v3-1-26d969fa8f1d@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:ce62:0:b0:403:3b70:6f57 with SMTP id o2csp3053406vqx; Mon, 27 Nov 2023 04:15:07 -0800 (PST) X-Google-Smtp-Source: AGHT+IHHz/9zty36JTM0X8WdT21ZQtTFuB7ccDuTCV/lWBsc5gSxMhBS0+pYGigtxSGUbSe0J2iv X-Received: by 2002:a05:6a21:3291:b0:187:67b0:73e with SMTP id yt17-20020a056a21329100b0018767b0073emr11191150pzb.14.1701087307434; Mon, 27 Nov 2023 04:15:07 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701087307; cv=none; d=google.com; s=arc-20160816; b=Sa1fZhTuFntVQb4typQt/QNNqiiv+0qVKQQlsFZ00g5XGfchOARit5SDEgEVuWQAPP 8qCEOLYm2yrAt62FsWKn8pkW722CTHoGQjADGXg9COxHXHD9QZpIQ8d21TMIeDL/FckG 3nJNxNsED/oA7LIQm77tYPcKkxgWNzT61Ktrm4HPUiadTFfYMmawgUdpVkC20wLu+RXg xHJqc/PDGKvkLdB49r2ykyIOwICZh0Uex89XJd5uO7nnGpT9IBW+o6QMtyv1HvuHeJcT fikAzvIKlrG9kPBjuLDpGwMnIrLBd6HzaPgqMiZ/jW4wGiVzn/ZoL2eIZNzR1lMgx6lt yJwA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=GnHgs/oiZbsBfML8eUHTU9zqWhlGG3/cogQIaRxrnsQ=; fh=EFIAKKtQRIe9H8zeoA9xLqoI3gyQkUU8ZMs+AiKe2pw=; b=tJVKtAlUZsBNzZ2D/ikkCGdpKI5wYOI8GCDGTT5mVjyT5egEbINE5uCtESF21OTAtt 8wcdJ5v1YShd00Q+QkHiORjnOR37GbabKoc8HQyEN4VVY70NsuAJ+JvbwCYrWah1tbCO O0sJY1iI7lVDn2KKvxu2i9bsIhXv2ighjm+sD3sPGPzc24Zu6Vz+GhxUWsjIj3gvx6G0 PrcAE+UV+OsYXCYL+/OIpV5Ty/vkKMBnWnkdc+C0dXFTarWpUpFcooP6Jg3K4+K4Dltc PsS3lIzZGG8l0Pyy3+Gtc5If90kW74qCBNxgUT60Ubmc69siFdR3+Lln9l9h4QEZiiiW aOfQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=BkNrg9Ln; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 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 howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id w187-20020a6362c4000000b00578b8d202b0si9269369pgb.536.2023.11.27.04.15.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Nov 2023 04:15:07 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=BkNrg9Ln; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 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 (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id 559EB80AF804; Mon, 27 Nov 2023 04:15:02 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233196AbjK0MOb (ORCPT <rfc822;toshivichauhan@gmail.com> + 99 others); Mon, 27 Nov 2023 07:14:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39268 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233201AbjK0MO2 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 27 Nov 2023 07:14:28 -0500 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F007810F; Mon, 27 Nov 2023 04:14:34 -0800 (PST) Received: from pps.filterd (m0279871.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3ARBLrkv022755; Mon, 27 Nov 2023 12:14:29 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : date : subject : mime-version : content-type : content-transfer-encoding : message-id : references : in-reply-to : to : cc; s=qcppdkim1; bh=GnHgs/oiZbsBfML8eUHTU9zqWhlGG3/cogQIaRxrnsQ=; b=BkNrg9Ln+OVm09eL4y1o9817oudkE3BEyXW10a8HzllPgOLfZXYm2MY5HKtDQc+QopfH 3dgF3HooxLTlHVc1SgZwzgEuFHKPjcf+HWJ8GXsQbKFkTsChao0q+wibSal4yinJCcOM tpunsvKBMexFIiOPOQpxMWJTEOLDaMEFtyAN8OFE+qJHqm8p/p2vCCiNyjXwcx5vfduG UB9Ph9eWCPoJYwV2FFADkMqv1XUtmUA21iNgzk1WrPNnovxlmJWqUkD6K/H+XZOWIOOE FpWOyYnHVWUABSXqs/T4adR0qwv1bLvjkPcy0G4nyiY2Ol/uog+qUfuo0xQudjIMGsi4 rg== Received: from nalasppmta03.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3umt63036g-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 27 Nov 2023 12:14:28 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA03.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3ARCERp6014923 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 27 Nov 2023 12:14:27 GMT Received: from hu-krichai-hyd.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Mon, 27 Nov 2023 04:14:22 -0800 From: Krishna chaitanya chundru <quic_krichai@quicinc.com> Date: Mon, 27 Nov 2023 17:43:49 +0530 Subject: [PATCH v3 1/3] dt-bindings: phy: qcom,qmp: Add PCIe qcom,refclk-always-on property MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-ID: <20231127-refclk_always_on-v3-1-26d969fa8f1d@quicinc.com> References: <20231127-refclk_always_on-v3-0-26d969fa8f1d@quicinc.com> In-Reply-To: <20231127-refclk_always_on-v3-0-26d969fa8f1d@quicinc.com> To: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org> CC: <linux-arm-msm@vger.kernel.org>, <linux-phy@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <quic_vbadigan@quicinc.com>, <quic_ramkri@quicinc.com>, <quic_nitegupt@quicinc.com>, <quic_skananth@quicinc.com>, <quic_vpernami@quicinc.com>, <quic_parass@quicinc.com>, "Krishna chaitanya chundru" <quic_krichai@quicinc.com> X-Mailer: b4 0.13-dev-83828 X-Developer-Signature: v=1; a=ed25519-sha256; t=1701087256; l=1195; i=quic_krichai@quicinc.com; s=20230907; h=from:subject:message-id; bh=b3DOMQj6Bhju4h0J8NhBRIj6lknp3jleEt1YDn8qkHE=; b=vsA26FIK+JfF4MRQu9PTq7Xt79VhZ2FfN/C0746c2kFg5c2JrgVU1r1fUn9U4j4GnYEDYpVLI dnF5u5C6hewAZH9HhrZAMwWUbKVJesfKLSnXm0aCnZRm2BvK/c/OEhD X-Developer-Key: i=quic_krichai@quicinc.com; a=ed25519; pk=10CL2pdAKFyzyOHbfSWHCD0X0my7CXxj8gJScmn1FAg= X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: AUwF9-XudjQ4gTtbjP2KqU4U0ZhK30MU X-Proofpoint-ORIG-GUID: AUwF9-XudjQ4gTtbjP2KqU4U0ZhK30MU X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-11-27_09,2023-11-27_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 mlxscore=0 mlxlogscore=999 bulkscore=0 suspectscore=0 spamscore=0 lowpriorityscore=0 malwarescore=0 phishscore=0 clxscore=1015 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311060000 definitions=main-2311270083 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Mon, 27 Nov 2023 04:15:03 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1783719324783316408 X-GMAIL-MSGID: 1783719324783316408 |
Series |
phy: qcom-qmp-pcie: Add support to keep refclk always on
|
|
Commit Message
Krishna chaitanya chundru
Nov. 27, 2023, 12:13 p.m. UTC
Document qcom,refclk-always-on property which is needed in some platforms
to supply refclk even in PCIe low power states.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
.../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++
1 file changed, 7 insertions(+)
Comments
On 27/11/2023 13:13, Krishna chaitanya chundru wrote: > Document qcom,refclk-always-on property which is needed in some platforms > to supply refclk even in PCIe low power states. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > index 2c3d6553a7ba..c747c9f35795 100644 > --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > @@ -93,6 +93,13 @@ properties: > "#phy-cells": > const: 0 > > + qcom,refclk-always-on: > + type: boolean > + description: In some platform where PCIe switch is connected, pcie switch due to some design You received a comment to use proper wrapping: 80. Please implement it. > + limitation fails to propage clkreq signal to the host and due to that host will not send > + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. > + This property if set keeps refclk always on even in Low power states. The property name suggests that's the state of hardware - refclk is always on. Description suggests you want to instruct OS to do something. Again, third time (once from Bjorn, once from Dmitry), rephrase property name and description to describe the hardware issue. I see description improved, but not the property name. Again in the end of description you say what Linux should do. Bindings do not describe Linux OS. Best regards, Krzysztof
On 11/28/2023 2:26 PM, Krzysztof Kozlowski wrote: > On 27/11/2023 13:13, Krishna chaitanya chundru wrote: >> Document qcom,refclk-always-on property which is needed in some platforms >> to supply refclk even in PCIe low power states. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >> index 2c3d6553a7ba..c747c9f35795 100644 >> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >> @@ -93,6 +93,13 @@ properties: >> "#phy-cells": >> const: 0 >> >> + qcom,refclk-always-on: >> + type: boolean >> + description: In some platform where PCIe switch is connected, pcie switch due to some design > You received a comment to use proper wrapping: 80. Please implement it. I will update this. >> + limitation fails to propage clkreq signal to the host and due to that host will not send >> + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. >> + This property if set keeps refclk always on even in Low power states. > The property name suggests that's the state of hardware - refclk is > always on. Description suggests you want to instruct OS to do something. > > Again, third time (once from Bjorn, once from Dmitry), rephrase property > name and description to describe the hardware issue. I see description > improved, but not the property name. Again in the end of description you Both bjorn and Dmitry gave comments to change the description only, and not the property name, correct if I am wrong. > say what Linux should do. Bindings do not describe Linux OS. I will remove the last line in the next patch. - Krishna Chaitanya. > > Best regards, > Krzysztof
On Tue, Nov 28, 2023 at 02:49:18PM +0530, Krishna Chaitanya Chundru wrote: > > On 11/28/2023 2:26 PM, Krzysztof Kozlowski wrote: > > On 27/11/2023 13:13, Krishna chaitanya chundru wrote: > > > Document qcom,refclk-always-on property which is needed in some platforms > > > to supply refclk even in PCIe low power states. > > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > > --- > > > .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > > > index 2c3d6553a7ba..c747c9f35795 100644 > > > --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > > > +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > > > @@ -93,6 +93,13 @@ properties: > > > "#phy-cells": > > > const: 0 > > > + qcom,refclk-always-on: > > > + type: boolean > > > + description: In some platform where PCIe switch is connected, pcie switch due to some design > > You received a comment to use proper wrapping: 80. Please implement it. > I will update this. > > > + limitation fails to propage clkreq signal to the host and due to that host will not send > > > + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. > > > + This property if set keeps refclk always on even in Low power states. > > The property name suggests that's the state of hardware - refclk is > > always on. Description suggests you want to instruct OS to do something. > > > > Again, third time (once from Bjorn, once from Dmitry), rephrase property > > name and description to describe the hardware issue. I see description > > improved, but not the property name. Again in the end of description you > > Both bjorn and Dmitry gave comments to change the description only, and not > the property name, > > correct if I am wrong. > > > say what Linux should do. Bindings do not describe Linux OS. > > I will remove the last line in the next patch. > You should name the property as, "qcom,keep-refclk-always-on" Also, no need to stuff the binding with "why this property is required", that info should be part of the patch description. Just describe what this property means in binding. - Mani > - Krishna Chaitanya. > > > > > Best regards, > > Krzysztof >
On 01/12/2023 07:07, Manivannan Sadhasivam wrote: > On Tue, Nov 28, 2023 at 02:49:18PM +0530, Krishna Chaitanya Chundru wrote: >> >> On 11/28/2023 2:26 PM, Krzysztof Kozlowski wrote: >>> On 27/11/2023 13:13, Krishna chaitanya chundru wrote: >>>> Document qcom,refclk-always-on property which is needed in some platforms >>>> to supply refclk even in PCIe low power states. >>>> >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> --- >>>> .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >>>> index 2c3d6553a7ba..c747c9f35795 100644 >>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >>>> @@ -93,6 +93,13 @@ properties: >>>> "#phy-cells": >>>> const: 0 >>>> + qcom,refclk-always-on: >>>> + type: boolean >>>> + description: In some platform where PCIe switch is connected, pcie switch due to some design >>> You received a comment to use proper wrapping: 80. Please implement it. >> I will update this. >>>> + limitation fails to propage clkreq signal to the host and due to that host will not send >>>> + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. >>>> + This property if set keeps refclk always on even in Low power states. >>> The property name suggests that's the state of hardware - refclk is >>> always on. Description suggests you want to instruct OS to do something. >>> >>> Again, third time (once from Bjorn, once from Dmitry), rephrase property >>> name and description to describe the hardware issue. I see description >>> improved, but not the property name. Again in the end of description you >> >> Both bjorn and Dmitry gave comments to change the description only, and not >> the property name, >> >> correct if I am wrong. >> >>> say what Linux should do. Bindings do not describe Linux OS. >> >> I will remove the last line in the next patch. >> > > You should name the property as, "qcom,keep-refclk-always-on" Keep the clock by who? By driver? Then not, property should describe physical phenomena or hardware issue being fixed here, not what driver should do. Best regards, Krzysztof
On Fri, Dec 01, 2023 at 09:01:43AM +0100, Krzysztof Kozlowski wrote: > On 01/12/2023 07:07, Manivannan Sadhasivam wrote: > > On Tue, Nov 28, 2023 at 02:49:18PM +0530, Krishna Chaitanya Chundru wrote: > >> > >> On 11/28/2023 2:26 PM, Krzysztof Kozlowski wrote: > >>> On 27/11/2023 13:13, Krishna chaitanya chundru wrote: > >>>> Document qcom,refclk-always-on property which is needed in some platforms > >>>> to supply refclk even in PCIe low power states. > >>>> > >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > >>>> --- > >>>> .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ > >>>> 1 file changed, 7 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > >>>> index 2c3d6553a7ba..c747c9f35795 100644 > >>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > >>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > >>>> @@ -93,6 +93,13 @@ properties: > >>>> "#phy-cells": > >>>> const: 0 > >>>> + qcom,refclk-always-on: > >>>> + type: boolean > >>>> + description: In some platform where PCIe switch is connected, pcie switch due to some design > >>> You received a comment to use proper wrapping: 80. Please implement it. > >> I will update this. > >>>> + limitation fails to propage clkreq signal to the host and due to that host will not send > >>>> + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. > >>>> + This property if set keeps refclk always on even in Low power states. > >>> The property name suggests that's the state of hardware - refclk is > >>> always on. Description suggests you want to instruct OS to do something. > >>> > >>> Again, third time (once from Bjorn, once from Dmitry), rephrase property > >>> name and description to describe the hardware issue. I see description > >>> improved, but not the property name. Again in the end of description you > >> > >> Both bjorn and Dmitry gave comments to change the description only, and not > >> the property name, > >> > >> correct if I am wrong. > >> > >>> say what Linux should do. Bindings do not describe Linux OS. > >> > >> I will remove the last line in the next patch. > >> > > > > You should name the property as, "qcom,keep-refclk-always-on" > > Keep the clock by who? By driver? Then not, property should describe > physical phenomena or hardware issue being fixed here, not what driver > should do. > This property indeed fixes the hardware issue (though in board level) and I see a plenty of properties similar to this one instructing the OS to keep some resource ON to workaround hardware issues. So they are all wrong? - Mani > > Best regards, > Krzysztof >
On 01/12/2023 12:10, Manivannan Sadhasivam wrote: > On Fri, Dec 01, 2023 at 09:01:43AM +0100, Krzysztof Kozlowski wrote: >> On 01/12/2023 07:07, Manivannan Sadhasivam wrote: >>> On Tue, Nov 28, 2023 at 02:49:18PM +0530, Krishna Chaitanya Chundru wrote: >>>> >>>> On 11/28/2023 2:26 PM, Krzysztof Kozlowski wrote: >>>>> On 27/11/2023 13:13, Krishna chaitanya chundru wrote: >>>>>> Document qcom,refclk-always-on property which is needed in some platforms >>>>>> to supply refclk even in PCIe low power states. >>>>>> >>>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>>>> --- >>>>>> .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >>>>>> index 2c3d6553a7ba..c747c9f35795 100644 >>>>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >>>>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml >>>>>> @@ -93,6 +93,13 @@ properties: >>>>>> "#phy-cells": >>>>>> const: 0 >>>>>> + qcom,refclk-always-on: >>>>>> + type: boolean >>>>>> + description: In some platform where PCIe switch is connected, pcie switch due to some design >>>>> You received a comment to use proper wrapping: 80. Please implement it. >>>> I will update this. >>>>>> + limitation fails to propage clkreq signal to the host and due to that host will not send >>>>>> + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. >>>>>> + This property if set keeps refclk always on even in Low power states. >>>>> The property name suggests that's the state of hardware - refclk is >>>>> always on. Description suggests you want to instruct OS to do something. >>>>> >>>>> Again, third time (once from Bjorn, once from Dmitry), rephrase property >>>>> name and description to describe the hardware issue. I see description >>>>> improved, but not the property name. Again in the end of description you >>>> >>>> Both bjorn and Dmitry gave comments to change the description only, and not >>>> the property name, >>>> >>>> correct if I am wrong. >>>> >>>>> say what Linux should do. Bindings do not describe Linux OS. >>>> >>>> I will remove the last line in the next patch. >>>> >>> >>> You should name the property as, "qcom,keep-refclk-always-on" >> >> Keep the clock by who? By driver? Then not, property should describe >> physical phenomena or hardware issue being fixed here, not what driver >> should do. >> > > This property indeed fixes the hardware issue (though in board level) and I see > a plenty of properties similar to this one instructing the OS to keep some > resource ON to workaround hardware issues. So they are all wrong? What I said before: "Again, third time (once from Bjorn, once from Dmitry), rephrase property name and description to describe the hardware issue. I see description improved, but not the property name. Again in the end of description you say what Linux should do. Bindings do not describe Linux OS." Best regards, Krzysztof
On Fri, Dec 01, 2023 at 01:10:48PM +0100, Krzysztof Kozlowski wrote: > On 01/12/2023 12:10, Manivannan Sadhasivam wrote: > > On Fri, Dec 01, 2023 at 09:01:43AM +0100, Krzysztof Kozlowski wrote: > >> On 01/12/2023 07:07, Manivannan Sadhasivam wrote: > >>> On Tue, Nov 28, 2023 at 02:49:18PM +0530, Krishna Chaitanya Chundru wrote: > >>>> > >>>> On 11/28/2023 2:26 PM, Krzysztof Kozlowski wrote: > >>>>> On 27/11/2023 13:13, Krishna chaitanya chundru wrote: > >>>>>> Document qcom,refclk-always-on property which is needed in some platforms > >>>>>> to supply refclk even in PCIe low power states. > >>>>>> > >>>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > >>>>>> --- > >>>>>> .../devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml | 7 +++++++ > >>>>>> 1 file changed, 7 insertions(+) > >>>>>> > >>>>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > >>>>>> index 2c3d6553a7ba..c747c9f35795 100644 > >>>>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > >>>>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml > >>>>>> @@ -93,6 +93,13 @@ properties: > >>>>>> "#phy-cells": > >>>>>> const: 0 > >>>>>> + qcom,refclk-always-on: > >>>>>> + type: boolean > >>>>>> + description: In some platform where PCIe switch is connected, pcie switch due to some design > >>>>> You received a comment to use proper wrapping: 80. Please implement it. > >>>> I will update this. > >>>>>> + limitation fails to propage clkreq signal to the host and due to that host will not send > >>>>>> + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. > >>>>>> + This property if set keeps refclk always on even in Low power states. > >>>>> The property name suggests that's the state of hardware - refclk is > >>>>> always on. Description suggests you want to instruct OS to do something. > >>>>> > >>>>> Again, third time (once from Bjorn, once from Dmitry), rephrase property > >>>>> name and description to describe the hardware issue. I see description > >>>>> improved, but not the property name. Again in the end of description you > >>>> > >>>> Both bjorn and Dmitry gave comments to change the description only, and not > >>>> the property name, > >>>> > >>>> correct if I am wrong. > >>>> > >>>>> say what Linux should do. Bindings do not describe Linux OS. > >>>> > >>>> I will remove the last line in the next patch. > >>>> > >>> > >>> You should name the property as, "qcom,keep-refclk-always-on" > >> > >> Keep the clock by who? By driver? Then not, property should describe > >> physical phenomena or hardware issue being fixed here, not what driver > >> should do. > >> > > > > This property indeed fixes the hardware issue (though in board level) and I see > > a plenty of properties similar to this one instructing the OS to keep some > > resource ON to workaround hardware issues. So they are all wrong? > > What I said before: > "Again, third time (once from Bjorn, once from Dmitry), rephrase > property name and description to describe the hardware issue. I see > description improved, but not the property name. Again in the end of > description you say what Linux should do. Bindings do not describe Linux > OS." > You didn't answer my question: "I see a plenty of properties similar to this one instructing the OS to keep some resource ON to workaround hardware issues. So they are all wrong?" If you say they are wrong, why are they approved in the first place? - Mani > > Best regards, > Krzysztof >
On 01/12/2023 13:30, Manivannan Sadhasivam wrote: >> What I said before: >> "Again, third time (once from Bjorn, once from Dmitry), rephrase >> property name and description to describe the hardware issue. I see >> description improved, but not the property name. Again in the end of >> description you say what Linux should do. Bindings do not describe Linux >> OS." >> > > You didn't answer my question: > > "I see a plenty of properties similar to this one instructing the OS to keep some > resource ON to workaround hardware issues. So they are all wrong?" They are not the best, but it all depends on the individual case. > > If you say they are wrong, why are they approved in the first place? Because we don't have time to keep digging what the driver is doing and what is claimed in DT. Some people don't even CC us on the driver. Best regards, Krzysztof
On 1.12.2023 14:25, Krzysztof Kozlowski wrote: > On 01/12/2023 13:30, Manivannan Sadhasivam wrote: >>> What I said before: >>> "Again, third time (once from Bjorn, once from Dmitry), rephrase >>> property name and description to describe the hardware issue. I see >>> description improved, but not the property name. Again in the end of >>> description you say what Linux should do. Bindings do not describe Linux >>> OS." >>> >> >> You didn't answer my question: >> >> "I see a plenty of properties similar to this one instructing the OS to keep some >> resource ON to workaround hardware issues. So they are all wrong?" > > They are not the best, but it all depends on the individual case. > >> >> If you say they are wrong, why are they approved in the first place? > Because we don't have time to keep digging what the driver is doing and > what is claimed in DT. Some people don't even CC us on the driver. Not sure if I asked this before, but can this not be set in the config struct inside the driver? Konrad
On Fri, Dec 01, 2023 at 10:29:11PM +0100, Konrad Dybcio wrote: > On 1.12.2023 14:25, Krzysztof Kozlowski wrote: > > On 01/12/2023 13:30, Manivannan Sadhasivam wrote: > >>> What I said before: > >>> "Again, third time (once from Bjorn, once from Dmitry), rephrase > >>> property name and description to describe the hardware issue. I see > >>> description improved, but not the property name. Again in the end of > >>> description you say what Linux should do. Bindings do not describe Linux > >>> OS." > >>> > >> > >> You didn't answer my question: > >> > >> "I see a plenty of properties similar to this one instructing the OS to keep some > >> resource ON to workaround hardware issues. So they are all wrong?" > > > > They are not the best, but it all depends on the individual case. > > > >> > >> If you say they are wrong, why are they approved in the first place? > > Because we don't have time to keep digging what the driver is doing and > > what is claimed in DT. Some people don't even CC us on the driver. > Not sure if I asked this before, but can this not be set in the config > struct inside the driver? > This cannot be the default config for any platform as keeping refclk always on will have some power penalty. Only if the hardware (EP or board design) has any issue, we can turn on this option. - Mani > Konrad
On Fri, Dec 01, 2023 at 02:25:44PM +0100, Krzysztof Kozlowski wrote: > On 01/12/2023 13:30, Manivannan Sadhasivam wrote: > >> What I said before: > >> "Again, third time (once from Bjorn, once from Dmitry), rephrase > >> property name and description to describe the hardware issue. I see > >> description improved, but not the property name. Again in the end of > >> description you say what Linux should do. Bindings do not describe Linux > >> OS." > >> > > > > You didn't answer my question: > > > > "I see a plenty of properties similar to this one instructing the OS to keep some > > resource ON to workaround hardware issues. So they are all wrong?" > > They are not the best, but it all depends on the individual case. > > > > > If you say they are wrong, why are they approved in the first place? > Because we don't have time to keep digging what the driver is doing and > what is claimed in DT. Some people don't even CC us on the driver. > OK. How about, "qcom,broken-refclk"? This reflects the fact that the default refclk operation is broken on this platform, so the OS should be prepared for it (by keeping it always on). - Mani > > Best regards, > Krzysztof >
On 06/12/2023 14:10, Manivannan Sadhasivam wrote: >>> >>> If you say they are wrong, why are they approved in the first place? >> Because we don't have time to keep digging what the driver is doing and >> what is claimed in DT. Some people don't even CC us on the driver. >> > > OK. How about, "qcom,broken-refclk"? This reflects the fact that the default > refclk operation is broken on this platform, so the OS should be prepared for > it (by keeping it always on). > Sounds very good to me. Best regards, Krzysztof
On Wed, Dec 06, 2023 at 06:40:09PM +0530, Manivannan Sadhasivam wrote: > OK. How about, "qcom,broken-refclk"? This reflects the fact that the default > refclk operation is broken on this platform, so the OS should be prepared for > it (by keeping it always on). Shouldn't that be qcom,broken-clkreq since its the CLKREQ# signal used to request REFCLK that is broken, not the REFCLK itself? Johan
On Thu, Dec 07, 2023 at 10:51:09AM +0100, Johan Hovold wrote: > On Wed, Dec 06, 2023 at 06:40:09PM +0530, Manivannan Sadhasivam wrote: > > > OK. How about, "qcom,broken-refclk"? This reflects the fact that the default > > refclk operation is broken on this platform, so the OS should be prepared for > > it (by keeping it always on). > > Shouldn't that be > > qcom,broken-clkreq > > since its the CLKREQ# signal used to request REFCLK that is broken, not > the REFCLK itself? > Darn... You are right. I got carried away by the initial property name. Thanks for spotting! - Mani > Johan
On 12/6/23 14:07, Manivannan Sadhasivam wrote: > On Fri, Dec 01, 2023 at 10:29:11PM +0100, Konrad Dybcio wrote: >> On 1.12.2023 14:25, Krzysztof Kozlowski wrote: >>> On 01/12/2023 13:30, Manivannan Sadhasivam wrote: >>>>> What I said before: >>>>> "Again, third time (once from Bjorn, once from Dmitry), rephrase >>>>> property name and description to describe the hardware issue. I see >>>>> description improved, but not the property name. Again in the end of >>>>> description you say what Linux should do. Bindings do not describe Linux >>>>> OS." >>>>> >>>> >>>> You didn't answer my question: >>>> >>>> "I see a plenty of properties similar to this one instructing the OS to keep some >>>> resource ON to workaround hardware issues. So they are all wrong?" >>> >>> They are not the best, but it all depends on the individual case. >>> >>>> >>>> If you say they are wrong, why are they approved in the first place? >>> Because we don't have time to keep digging what the driver is doing and >>> what is claimed in DT. Some people don't even CC us on the driver. >> Not sure if I asked this before, but can this not be set in the config >> struct inside the driver? >> > > This cannot be the default config for any platform as keeping refclk always on > will have some power penalty. Only if the hardware (EP or board design) has any > issue, we can turn on this option. So, if I'm reading this right, this concerns some but not all 8280 boards? Konrad
On Thu, Dec 07, 2023 at 03:42:52PM +0530, Manivannan Sadhasivam wrote: > On Thu, Dec 07, 2023 at 10:51:09AM +0100, Johan Hovold wrote: > > On Wed, Dec 06, 2023 at 06:40:09PM +0530, Manivannan Sadhasivam wrote: > > > > > OK. How about, "qcom,broken-refclk"? This reflects the fact that the default > > > refclk operation is broken on this platform, so the OS should be prepared for > > > it (by keeping it always on). > > > > Shouldn't that be > > > > qcom,broken-clkreq > > > > since its the CLKREQ# signal used to request REFCLK that is broken, not > > the REFCLK itself? > > > > Darn... You are right. I got carried away by the initial property name. Thanks > for spotting! Thinking some more on this after hitting send: It may still be wrong with a 'broken-clkreq' property in the PHY instead of in the controller (or endpoint). Could there not be other ways to handle a broken clkreq signal so that this really should be a decision made by the OS, for example, to disable L1 substates and clock PM? Simply leaving the refclk always on in the PHY seems like a bit of a hack and I'm not even sure that can be considered correct. Having a property that maps directly to that behaviour has rightly been rejected, but it seems that simply renaming the flag but keeping it in the PHY may still not be the right thing to do here. Johan
On Thu, Dec 07, 2023 at 02:05:12PM +0100, Johan Hovold wrote: > On Thu, Dec 07, 2023 at 03:42:52PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Dec 07, 2023 at 10:51:09AM +0100, Johan Hovold wrote: > > > On Wed, Dec 06, 2023 at 06:40:09PM +0530, Manivannan Sadhasivam wrote: > > > > > > > OK. How about, "qcom,broken-refclk"? This reflects the fact that the default > > > > refclk operation is broken on this platform, so the OS should be prepared for > > > > it (by keeping it always on). > > > > > > Shouldn't that be > > > > > > qcom,broken-clkreq > > > > > > since its the CLKREQ# signal used to request REFCLK that is broken, not > > > the REFCLK itself? > > > > > > > Darn... You are right. I got carried away by the initial property name. Thanks > > for spotting! > > Thinking some more on this after hitting send: It may still be wrong > with a 'broken-clkreq' property in the PHY instead of in the controller > (or endpoint). > > Could there not be other ways to handle a broken clkreq signal so that > this really should be a decision made by the OS, for example, to disable > L1 substates and clock PM? > One has to weigh the power consumption between keeping refclk always on and disabling L1SS. Chaitanya, can you measure power consumption in both cases? > Simply leaving the refclk always on in the PHY seems like a bit of a > hack and I'm not even sure that can be considered correct. > I wouldn't agree it is a hack, even though it may sound like one. The option to keep refclk always on in the PHY is precisely there for usecase like this. - Mani > Having a property that maps directly to that behaviour has rightly been > rejected, but it seems that simply renaming the flag but keeping it in > the PHY may still not be the right thing to do here. > > Johan
On Thu, Dec 07, 2023 at 06:50:32PM +0530, Manivannan Sadhasivam wrote: > On Thu, Dec 07, 2023 at 02:05:12PM +0100, Johan Hovold wrote: > > On Thu, Dec 07, 2023 at 03:42:52PM +0530, Manivannan Sadhasivam wrote: > > > On Thu, Dec 07, 2023 at 10:51:09AM +0100, Johan Hovold wrote: > > > > Shouldn't that be > > > > > > > > qcom,broken-clkreq > > > > > > > > since its the CLKREQ# signal used to request REFCLK that is broken, not > > > > the REFCLK itself? > > > > > > > > > > Darn... You are right. I got carried away by the initial property name. Thanks > > > for spotting! > > > > Thinking some more on this after hitting send: It may still be wrong > > with a 'broken-clkreq' property in the PHY instead of in the controller > > (or endpoint). > > > > Could there not be other ways to handle a broken clkreq signal so that > > this really should be a decision made by the OS, for example, to disable > > L1 substates and clock PM? > > One has to weigh the power consumption between keeping refclk always on and > disabling L1SS. Chaitanya, can you measure power consumption in both cases? Sure, my point was just that that's a policy decision and not something that should be encoded in the devicetree (as was initially proposed). And that the right place for the renamed property is not necessarily in the PHY node either. > > Simply leaving the refclk always on in the PHY seems like a bit of a > > hack and I'm not even sure that can be considered correct. > > I wouldn't agree it is a hack, even though it may sound like one. The option to > keep refclk always on in the PHY is precisely there for usecase like this. I just skimmed the spec so perhaps I'm missing something, but there's definitely wordings in there that explicitly says that L1 PM substates must not be enabling unless you have a functioning CLKREQ# signal. Johan
On Thu, Dec 07, 2023 at 02:36:49PM +0100, Johan Hovold wrote: > On Thu, Dec 07, 2023 at 06:50:32PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Dec 07, 2023 at 02:05:12PM +0100, Johan Hovold wrote: > > > On Thu, Dec 07, 2023 at 03:42:52PM +0530, Manivannan Sadhasivam wrote: > > > > On Thu, Dec 07, 2023 at 10:51:09AM +0100, Johan Hovold wrote: > > > > > > Shouldn't that be > > > > > > > > > > qcom,broken-clkreq > > > > > > > > > > since its the CLKREQ# signal used to request REFCLK that is broken, not > > > > > the REFCLK itself? > > > > > > > > > > > > > Darn... You are right. I got carried away by the initial property name. Thanks > > > > for spotting! > > > > > > Thinking some more on this after hitting send: It may still be wrong > > > with a 'broken-clkreq' property in the PHY instead of in the controller > > > (or endpoint). > > > > > > Could there not be other ways to handle a broken clkreq signal so that > > > this really should be a decision made by the OS, for example, to disable > > > L1 substates and clock PM? > > > > One has to weigh the power consumption between keeping refclk always on and > > disabling L1SS. Chaitanya, can you measure power consumption in both cases? > > Sure, my point was just that that's a policy decision and not something > that should be encoded in the devicetree (as was initially proposed). > > And that the right place for the renamed property is not necessarily in > the PHY node either. > > > > Simply leaving the refclk always on in the PHY seems like a bit of a > > > hack and I'm not even sure that can be considered correct. > > > > I wouldn't agree it is a hack, even though it may sound like one. The option to > > keep refclk always on in the PHY is precisely there for usecase like this. > > I just skimmed the spec so perhaps I'm missing something, but there's > definitely wordings in there that explicitly says that L1 PM substates > must not be enabling unless you have a functioning CLKREQ# signal. > I checked 'PCI Express Base spec 4.0', and there is a wording in 'Section 5.5 L1 PM Substates': For L1 PM ... each port must have a unique instance of the signal (CLKREQ#) and the upstream and downstream port CLKREQ# signals must be connected. So yes, we should not enable L1 PM substates in this case. And also it appears to me that the property should be part of the controller node, since CLKREQ# is handled by the controller instance. - Mani > Johan
diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml index 2c3d6553a7ba..c747c9f35795 100644 --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml @@ -93,6 +93,13 @@ properties: "#phy-cells": const: 0 + qcom,refclk-always-on: + type: boolean + description: In some platform where PCIe switch is connected, pcie switch due to some design + limitation fails to propage clkreq signal to the host and due to that host will not send + refclk, which results in linkdown in L1.2 or L1.1 exit initiated by EP. + This property if set keeps refclk always on even in Low power states. + required: - compatible - reg