Message-ID: <80b17309-ec9c-8942-d901-7121bc936471@linaro.org>
Date:   Sat, 25 Feb 2023 11:08:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To:     Sajid Dalvi <sdalvi@google.com>, Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     kernel-team@android.com, linux-pci@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230224195749.818282-1-sdalvi@google.com>
 <20230224195749.818282-2-sdalvi@google.com>
 <b50ab99f-e307-3a66-9198-85a71b012e5e@linaro.org>
 <CAEbtx1k-7TJPcd3+cueRoKLJcoUQLfF6nfOQFVfzB0YCUrbtqg@mail.gmail.com>
 <2e4964c1-0831-c156-3372-81a56f8d623e@linaro.org>
 <CAEbtx1=EGC+4LfSz+HqGzjJvF2O1vsB7a42ESTqbU8p5N-yUHg@mail.gmail.com>
 <28c72c1a-df7d-db05-f32f-23b170b96db5@linaro.org>
In-Reply-To: <28c72c1a-df7d-db05-f32f-23b170b96db5@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 25/02/2023 11:00, Krzysztof Kozlowski wrote:
> On 24/02/2023 23:09, Sajid Dalvi wrote:
>> On Fri, Feb 24, 2023 at 3:29 PM Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 24/02/2023 22:27, Sajid Dalvi wrote:
>>>> On Fri, Feb 24, 2023 at 2:40 PM Krzysztof Kozlowski
>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>> On 24/02/2023 20:57, Sajid Dalvi wrote:
>>>>>> When the Root Complex is probed, the default behavior is to spin in a loop
>>>>>> waiting for the link to come up. In some systems the link is not brought up
>>>>>> during probe, but later in the context of an end-point turning on.
>>>>>> This property will allow the loop to be skipped.
>>>>>>
>>>>>> Signed-off-by: Sajid Dalvi <sdalvi@google.com>
>>>>>> ---
>>>>>
>>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>
>>>>>>  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 8 ++++++++
>>>>>>  1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
>>>>>> index 1a83f0f65f19..0b8950a73b7e 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
>>>>>> @@ -197,6 +197,14 @@ properties:
>>>>>>        - contains:
>>>>>>            const: msi
>>>>>>
>>>>>> +  snps,skip-wait-link-up:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>>> +    description:
>>>>>> +      When the Root Complex is probed, the default behavior is to spin in a
>>>>>> +      loop waiting for the link to come up. In some systems the link is not
>>>>>> +      brought up during probe, but later in the context of an end-point turning
>>>>>> +      on. This property will allow the loop to be skipped.
>>>>>
>>>>> I fail to see how probe behavior is related to properties of hardware.
>>>>> You describe OS behavior, not hardware. This does not look like
>>>>> belonging to DT.
>>>>>
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>
>>>> Thanks for your response Krzysztof.
>>>> The hardware configuration of the system determines whether an
>>>> endpoint device is available during host init. If it isn't available
>>>> on a particular and dedicated pcie interface, we should skip waiting
>>>> for the link to be up. For other interfaces, possibly even on the same
>>>> system, where a device is present or maybe present we should wait for
>>>> the link to come up.
>>>
>>> Keep discussions public.
>>>
>>> Your commit and property description mentions probe, which is nothing
>>> related to hardware. Why the device would not be available during host
>>> init (I understand we do not talk about hotplug as it is already
>>> supported by Linux) in a way it is hardware property, not OS?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> + everyone else I mistakenly didn't reply to earlier
>>
>> If I understand you correctly, the usage of probe is misleading
>> because it doesn't have anything to do with the hardware.
>> So your recommendation is to replace probe with device init, in the
>> description of the property and the commit message?
> 
> No, I asked there a question for which we need answer.
> 
> device init is also OS task... You need to explain why this is a
> property of hardware, not OS behavior.

Actually let's be clearer - your cover letter says:
"In some systems the link is not brought up
during probe, but later in the context of an end-point turning on. (...)
to skip this loop."

so this is pure Linux OS stuff. You just want to control driver behavior
from DT. Not at all DT property.

Best regards,
Krzysztof