Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <1670967848-31475-3-git-send-email-quic_khsieh@quicinc.com>
References: <1670967848-31475-1-git-send-email-quic_khsieh@quicinc.com> <1670967848-31475-3-git-send-email-quic_khsieh@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Tue, 13 Dec 2022 15:06:46 -0800
Message-ID: <CAE-0n52eHYCqxUJqQXoaQ8vyqCk-QfouSun+zUp3yo5DufWbwg@mail.gmail.com>
Subject: Re: [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and
 link-frequencies property
To:     Kuogee Hsieh <quic_khsieh@quicinc.com>, agross@kernel.org,
        airlied@gmail.com, andersson@kernel.org, daniel@ffwll.ch,
        devicetree@vger.kernel.org, dianders@chromium.org,
        dmitry.baryshkov@linaro.org, dri-devel@lists.freedesktop.org,
        konrad.dybcio@somainline.org, krzysztof.kozlowski+dt@linaro.org,
        robdclark@gmail.com, robh+dt@kernel.org, sean@poorly.run,
        vkoul@kernel.org
Cc:     quic_abhinavk@quicinc.com, quic_sbillaka@quicinc.com,
        freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Kuogee Hsieh (2022-12-13 13:44:05)
> Add both data-lanes and link-frequencies property into endpoint

Why do we care? Please tell us why it's important.

>
> Changes in v7:
> -- split yaml out of dtsi patch
> -- link-frequencies from link rate to symbol rate
> -- deprecation of old data-lanes property
>
> Changes in v8:
> -- correct Bjorn mail address to kernel.org
>
> Changes in v10:
> -- add menu item to data-lanes and link-frequecnis
>
> Changes in v11:
> -- add endpoint property at port@1
>
> Changes in v12:
> -- use enum for item at data-lanes and link-frequencies
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>`
                                                       ^
Stray ` here? -----------------------------------------/

> ---
>  .../bindings/display/msm/dp-controller.yaml        | 30 ++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index f2515af..8fb9fa5 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -96,14 +97,37 @@ properties:
>
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
> +
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: "/schemas/graph.yaml#/$defs/port-base"
>          description: Input endpoint of the controller
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
>
>        port@1:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: "/schemas/graph.yaml#/$defs/port-base"

I thought the quotes weren't needed?

>          description: Output endpoint of the controller
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#

Does this need 'unevaluatedProperties: false' here?

> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +                items:
> +                  enum: [ 0, 1, 2, 3 ]
> +
> +              link-frequencies:
> +                minItems: 1
> +                maxItems: 4
> +                items:
> +                  enum: [ 1620000000, 2700000000, 5400000000, 8100000000 ]
> +
> +    required:
> +      - port@0
> +      - port@1
>
>  required:
>    - compatible
> @@ -193,6 +217,8 @@ examples:
>                  reg = <1>;
>                  endpoint {
>                      remote-endpoint = <&typec>;
> +                    data-lanes = <0 1>;
> +                    link-frequencies = /bits/ 64 <1620000000 2700000000 5400000000 8100000000>;
>                  };

So far we haven't used the output port on the DP controller in DT.

I'm still not clear on what we should do in general for DP because
there's a PHY that actually controls a lane count and lane mapping. In
my mental model of the SoC, this DP controller's output port is
connected to the DP PHY, which then sends the DP lanes out of the SoC to
the next downstream device (i.e. a DP connector or type-c muxer). Having
a remote-endpoint property with a phandle to typec doesn't fit my mental
model. I'd expect it to be the typec PHY.

That brings up the question: when we have 2 lanes vs. 4 lanes will we
duplicate the data-lanes property in the PHY binding? I suspect we'll
have to. Hopefully that sort of duplication is OK?

Similarly, we may have a redriver that limits the link-frequencies
property further (e.g. only support <= 2.7GHz). Having multiple
link-frequencies along the graph is OK, right? And isn't the
link-frequencies property known here by fact that the DP controller
tells us which SoC this controller is for, and thus we already know the
supported link frequencies?

Finally, I wonder if we should put any of this in the DP controller's
output endpoint, or if we can put these sorts of properties in the DP
PHY binding directly? Can't we do that and then when the DP controller
tries to set 4 lanes, the PHY immediately fails the call and the link
training algorithm does its thing and tries fewer lanes? And similarly,
if link-frequencies were in the PHY's binding, the PHY could fail to set
those frequencies during link training, returning an error to the DP
controller, letting the training move on to a lower frequency. If we did
that this patch series would largely be about modifying the PHY binding,
updating the PHY driver to enforce constraints, and handling errors
during link training in the DP controller (which may already be done? I
didn't check).