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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >>>
> >>> On 24/02/2023 22:27, Sajid Dalvi wrote:
> >>>> On Fri, Feb 24, 2023 at 2:40 PM Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>> ---
> >>>>>
> >>>>> 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
>