[PATCH v1 0/2] Skip waiting for link up during probe

[PATCH v1 0/2] Skip waiting for link up during probe

[Sajid Dalvi]

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
patch adds a device tree property to the Synopsis designware root
controller to skip this loop.

Sajid Dalvi (2):
  dt-bindings: PCI: dwc: Add snps,skip-wait-link-up
  PCI: dwc: Skip waiting for link up on probe

 Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c       | 5 +++--
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.39.2.637.g21b0678d19-goog

[PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up

[Sajid Dalvi]

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>
---
 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.
+
 additionalProperties: true
 
 required:
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v1 2/2] PCI: dwc: Skip waiting for link up on probe

[Sajid Dalvi]

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.

Call trace:
dw_pcie_wait_for_link << spins in a loop for 1 second
dw_pcie_host_init

Adding the snps,skip-wait-link-up property in the pcie device tree node
will skip this loop and save 1 second per interface on pcie probe for
systems that do not need to bring up the link this early in the boot
sequence.

Signed-off-by: Sajid Dalvi <sdalvi@google.com>
Signed-off-by: Andrew Chant <achant@google.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3ab6ae3712c4..f5cba44ebf39 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -477,8 +477,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_free_msi;
 	}
 
-	/* Ignore errors, the link may come up later */
-	dw_pcie_wait_for_link(pci);
+	if (!of_property_read_bool(np, "snps,skip-wait-link-up"))
+		/* Ignore errors, the link may come up later */
+		dw_pcie_wait_for_link(pci);
 
 	bridge->sysdata = pp;
 
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up

[Krzysztof Kozlowski]

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

Re: [PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up

[Sajid Dalvi]

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?

Sajid

Re: [PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up

[Krzysztof Kozlowski]

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.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up

[Krzysztof Kozlowski]

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

Re: [PATCH v1 1/2] dt-bindings: PCI: dwc: Add snps,skip-wait-link-up

[Sajid Dalvi]

Thanks Krzysztof.
I will attempt to send a new patch to address the delay solely in the driver.

Sajid

On Sat, Feb 25, 2023 at 4:08 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> 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
>