Re: [PATCH v1 2/4] dt-bindings: PCI: qcom,pcie-sa8775p: document link_down reset

From: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Cc: <lpieralisi@kernel.org>, <kwilczynski@kernel.org>,
	<manivannan.sadhasivam@linaro.org>, <robh@kernel.org>,
	<bhelgaas@google.com>, <krzk+dt@kernel.org>,
	<neil.armstrong@linaro.org>, <abel.vesa@linaro.org>,
	<kw@linux.com>, <conor+dt@kernel.org>, <vkoul@kernel.org>,
	<kishon@kernel.org>, <andersson@kernel.org>,
	<konradybcio@kernel.org>, <linux-arm-msm@vger.kernel.org>,
	<linux-pci@vger.kernel.org>, <linux-phy@lists.infradead.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<quic_qianyu@quicinc.com>, <quic_krichai@quicinc.com>,
	<quic_vbadigan@quicinc.com>
Subject: Re: [PATCH v1 2/4] dt-bindings: PCI: qcom,pcie-sa8775p: document link_down reset
Date: Wed, 4 Jun 2025 15:58:33 +0800	[thread overview]
Message-ID: <e8d1b60c-97fe-4f50-8ead-66711f1aa3a7@quicinc.com> (raw)
In-Reply-To: <drr7cngryldptgzbmac7l2xpryugbrnydke3alq5da2mfvmgm5@nwjsqkef7ypc>

On 6/3/2025 9:11 PM, Dmitry Baryshkov wrote:
> On Thu, May 29, 2025 at 11:54:14AM +0800, Ziyue Zhang wrote:
>> Each PCIe controller on sa8775p supports 'link_down'reset on hardware,
>> document it.
> I don't think it's possible to "support" reset in hardware. Either it
> exists and is routed, or it is not.

Hi Dmitry,

I will change the commit msg to
'Each PCIe controller on sa8775p includes 'link_down'reset on hardware,
document it.'
"Supports" implies that the PCIe controller has an active role in enabling
or managing the reset functionality—it suggests that the controller is designed
to accommodate or facilitate this feature.
  "Includes" simply states that the reset functionality is present in the
hardware—it exists, whether or not it's actively managed or configurable.
So I think change it to includes will be better.

BRs
Ziyue

>> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
>> ---
>>   .../devicetree/bindings/pci/qcom,pcie-sa8775p.yaml  | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sa8775p.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sa8775p.yaml
>> index e3fa232da2ca..805258cbcf2f 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sa8775p.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sa8775p.yaml
>> @@ -61,11 +61,14 @@ properties:
>>         - const: global
>>   
>>     resets:
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 2
> Shouldn't we just update this to maxItems:2 / minItems:2 and drop
> minItems:1 from the next clause?

Hi Dmitry,

link_down reset is optional. In many other platforms, like sm8550
and x1e80100, link_down reset is documented as a optional reset.
PCIe will works fine without link_down reset. So I think setting it
as optional is better.

BRs
Ziyue

>>   
>>     reset-names:
>> +    minItems: 1
>>       items:
>> -      - const: pci
>> +      - const: pci # PCIe core reset
>> +      - const: link_down # PCIe link down reset
>>   
>>   required:
>>     - interconnects
>> @@ -161,8 +164,10 @@ examples:
>>   
>>               power-domains = <&gcc PCIE_0_GDSC>;
>>   
>> -            resets = <&gcc GCC_PCIE_0_BCR>;
>> -            reset-names = "pci";
>> +            resets = <&gcc GCC_PCIE_0_BCR>,
>> +                     <&gcc GCC_PCIE_0_LINK_DOWN_BCR>;
>> +            reset-names = "pci",
>> +                          "link_down";
>>   
>>               perst-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>               wake-gpios = <&tlmm 0 GPIO_ACTIVE_HIGH>;
>> -- 
>> 2.34.1
>>
>>
>> -- 
>> linux-phy mailing list
>> linux-phy@lists.infradead.org
>> https://lists.infradead.org/mailman/listinfo/linux-phy