Message-ID: <772f5e33-5040-4a68-84f4-25e048aa4432@quicinc.com>
Date: Wed, 4 Jun 2025 18:02:10 +0800
From: Qiang Yu <quic_qianyu@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        Ziyue Zhang
	<quic_ziyuzhan@...cinc.com>
CC: <lpieralisi@...nel.org>, <kwilczynski@...nel.org>,
        <manivannan.sadhasivam@...aro.org>, <robh@...nel.org>,
        <bhelgaas@...gle.com>, <krzk+dt@...nel.org>,
        <neil.armstrong@...aro.org>, <abel.vesa@...aro.org>, <kw@...ux.com>,
        <conor+dt@...nel.org>, <vkoul@...nel.org>, <kishon@...nel.org>,
        <andersson@...nel.org>, <konradybcio@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <linux-phy@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_krichai@...cinc.com>,
        <quic_vbadigan@...cinc.com>
Subject: Re: [PATCH v1 2/4] dt-bindings: PCI: qcom,pcie-sa8775p: document
 link_down reset


On 6/4/2025 5:15 PM, Dmitry Baryshkov wrote:
> On Wed, Jun 04, 2025 at 03:58:33PM +0800, Ziyue Zhang wrote:
>> 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@...cinc.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.
> You are describing a hardware. How can a reset be optional in the
> _hardware_? It's either routed or not.
>
> I feel a bit confused. According to the theory above, everything seems to
> be non-optional when describing hardware, such as registers, clocks,
> resets, regulators, and interrupts—all of them either exist or do not.
>
> Seems like I misunderstand the concept of 'optional'? Is 'optional' only
> used for compatibility across different platforms?
>
> Additionally, we have documented the PCIe global interrupt as optional. I
> was taught that, in the PCIe driver, this interrupt is retrieved using the
> platform_get_irq_byname_optional API, so it can be documented as optional.
> However, this still seems to contradict the theory mentioned earlier.
>> 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@...ts.infradead.org
>>>> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Qiang Yu

Powered by blists - more mailing lists