Re: [PATCH v6 1/4] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 01/12/2022 19:32, Kuogee Hsieh wrote:
> 
> On 11/30/2022 4:21 PM, Dmitry Baryshkov wrote:
>> On 01/12/2022 02:07, Dmitry Baryshkov wrote:
>>> On 01/12/2022 01:51, Kuogee Hsieh wrote:
>>>> Move data-lanes property from mdss_dp node to dp_out endpoint. Also
>>>> add link-frequencies property into dp_out endpoint as well. The last
>>>> frequency specified at link-frequencies will be the max link rate
>>>> supported by DP.
>>>>
>>>> Changes in v5:
>>>> -- revert changes at sc7180.dtsi and sc7280.dtsi
>>>> -- add &dp_out to sc7180-trogdor.dtsi and sc7280-herobrine.dtsi
>>>>
>>>> Changes in v6:
>>>> -- add data-lanes and link-frequencies to yaml
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>>   .../devicetree/bindings/display/msm/dp-controller.yaml  | 17 
>>>> +++++++++++++++++
>>>
>>> Separate patch. Also you didn't check the get_maintainers output, so 
>>> required parties were not included into the distribution.
>>>
>>> Also as you'd check the get_maintainers output, please fix other 
>>> email addresses too.
>>>
>>>> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi            |  6 +++++-
>>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi          |  6 +++++-
>>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> index 94bc6e1..af70343 100644
>>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>>>> @@ -90,6 +90,20 @@ properties:
>>>>           $ref: /schemas/graph.yaml#/properties/port
>>>>           description: Output endpoint of the controller
>>>> +        properties:
>>>> +          endpoint:
>>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>>> +
>>>> +          properties:
>>>> +            link-frequencies: true
>>>> +            data-lanes: true
>>>
>>> No. Use $ref for both of them.
>>>
>>>> +
>>>> +          required:
>>>> +            - link-frequencies
>>>> +            - data-lanes
>>>
>>> No, they are not required.
>>>
>>>> +
>>>> +          additionalProperties: false
>>>> +
>>>
>>> deprecation of old data-lanes property?
>>>
>>>>   required:
>>>>     - compatible
>>>>     - reg
>>>> @@ -158,6 +172,9 @@ examples:
>>>>                   reg = <1>;
>>>>                   endpoint {
>>>>                       remote-endpoint = <&typec>;
>>>> +                    data-lanes = <1 2>;
>>>> +                    link-frequencies = /bits/ 64 <160000000 270000000
>>
>> s/1600/1620
>>
>>>> + 540000000 810000000>;
>>>
>>> I guess the number of zeroes is wrong here. This is 160 MHz ... 810 
>>> Mhz, rather than 1.6 GHz ... 8.1 GHz
>>
>> Ok, I was wrong here. The old code definitely defaults to 570 
>> mega-something. Now I'd really like to read your description for the 
>> link-frequencies property, because the 
>> phy_configure_opts_dp::link_rate is clearly specified in Mb/s and it 
>> takes a fixed set of values from 1.62 Gb/s up to 8.1 Gb/s.
>>
>> I think the drm_dp_bw_code_to_link_rate() function is incorrect by 
>> itself, as it multiplies with 27000 (27 Mbps) rather than 270000 (0.27 
>> Gbps) as required by the standard. So first, we should fix the 
>> function, then all the rates would become logical.
> 
> no, drm_dp_bw_code_to_link_rate() is correct and should not be changes 
> since it impact to other dp drivers too.
> 
> 0.27Gbps/lane is specified at DP spec.
> 
> DP use 8b/10b coding rule (10 bits symbol contains 8 bits data).

At least it should get documentation that it returns Kylo-bytes per second.

But, getting back to link-frequencies. The documentation clearly says 
that it should be allowed data bus _frequencies_. And frequencies are 
measured in Hz, not in bits/sec or bytes/sec.


>>>>                   };
>>>>               };
>>>>           };
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>>>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>> index 754d2d6..39f0844 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>>>> @@ -812,7 +812,11 @@ hp_i2c: &i2c9 {
>>>>       status = "okay";
>>>>       pinctrl-names = "default";
>>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>>> -    data-lanes = <0 1>;
>>>> +};
>>>> +
>>>> +&dp_out {
>>>> +    data-lanes = <0  1>;
>>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000>;
>>>
>>> Same comment here.
>>>
>>>>   };
>>>>   &pm6150_adc {
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>> index 93e39fc..b7c343d 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>> @@ -440,7 +440,11 @@ ap_i2c_tpm: &i2c14 {
>>>>       status = "okay";
>>>>       pinctrl-names = "default";
>>>>       pinctrl-0 = <&dp_hot_plug_det>;
>>>> -    data-lanes = <0 1>;
>>>> +};
>>>> +
>>>> +&dp_out {
>>>> +    data-lanes = <0  1>;
>>>> +    link-frequencies = /bits/ 64 <160000000 270000000 540000000 
>>>> 810000000>;
>>>
>>> And here.
>>>
>>>>   };
>>>>   &mdss_mdp {
>>>
>>

-- 
With best wishes
Dmitry