[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c110cd89a3144a20b21ec392f50eee68@codeaurora.org>
Date: Fri, 19 Jun 2020 14:55:08 -0700
From: tanmay@...eaurora.org
To: Stephen Boyd <swboyd@...omium.org>
Cc: agross@...nel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
seanpaul@...omium.org, robdclark@...il.com, dianders@...omium.org,
aravindh@...eaurora.org, abhinavk@...eaurora.org,
bjorn.andersson@...aro.org
Subject: Re: [PATCH v2] arm64: dts: qcom: sc7180: Add Display Port dt node
Thanks Stephen for reviews.
I will post new change addressing your all comments.
On 2020-06-18 18:12, Stephen Boyd wrote:
> Quoting Tanmay Shah (2020-06-18 16:21:13)
>> Enable DP driver for sc7180.
>
> Add DP device node on sc7180? This isn't a driver.
>
Done.
>>
>> This change depends-on following series:
>> https://patchwork.freedesktop.org/series/78583/
>> and https://patchwork.freedesktop.org/patch/351990/
>>
>> Changes in v2:
>>
>> - Add assigned-clocks and assigned-clock-parents
>> - Remove cell-index and pixel_rcg
>> - Change compatible to qcom,sc7180-dp
>>
>> Signed-off-by: Tanmay Shah <tanmay@...eaurora.org>
>> ---
>> arch/arm64/boot/dts/qcom/sc7180.dtsi | 57
>> ++++++++++++++++++++++++++--
>> 1 file changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 916401f7e87c..26fe623e3b0f 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -2216,10 +2216,19 @@ ports {
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> + port@1 {
>> + reg = <1>;
>> + dpu_intf1_out:
>> endpoint {
>> +
>> remote-endpoint =
>> +
>> <&dsi0_in>;
>> + };
>> + };
>> +
>> port@0 {
>> reg = <0>;
>> - dpu_intf1_out:
>> endpoint {
>> -
>> remote-endpoint = <&dsi0_in>;
>> + dpu_intf0_out:
>> endpoint {
>> +
>> remote-endpoint =
>> +
>> <&dp_in>;
>> };
>> };
>> };
>
> I thought this wasn't supposed to change? At least according to the
> binding it shouldn't be needed.
>
Thanks. Yes it should be port@2 as per new dpu-sc7180.yaml bindings in
below patch.
https://patchwork.freedesktop.org/patch/371087/?series=78583&rev=1
>> @@ -2293,6 +2302,46 @@ dsi_phy: dsi-phy@...4400 {
>> };
>> };
>>
>> + msm_dp: displayport-controller@...0000{
>> + status = "ok";
>
> Please use status = "disabled";
>
Ok got it. Board's DT will enable it.
>> + compatible = "qcom,sc7180-dp";
>> +
>> + reg = <0 0xae90000 0 0x1400>;
>> + reg-names = "dp_controller";
>> +
>> + interrupt-parent = <&mdss>;
>
> Any reason why this isn't under the mdss node like the other display
> device nodes?
>
Done. Moved msm_dp node under mdss.
>> + interrupts = <12 0>;
>> +
>> + clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> + <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> + <&dispcc
>> DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> + <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
>> + clock-names = "core_aux", "ctrl_link",
>> + "ctrl_link_iface",
>> "stream_pixel";
>> + #clock-cells = <1>;
>> + assigned-clocks = <&dispcc
>> DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> + assigned-clock-parents = <&msm_dp 1>;
>> +
>> + data-lanes = <0 1>;
>
> This can and should be left to the board files. At the SoC level my
> understanding is that there are four lanes possible, so no need to
> artificially limit it here.
>
Removed from here. Driver changes I will be posting accordingly.
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + port@0 {
>> + reg = <0>;
>> + dp_in: endpoint {
>> + remote-endpoint =
>> +
>> <&dpu_intf0_out>;
>
> I'd prefer these were on one line, regardless of the 80 character line
> limit/suggestion.
>
ok done.
>> + };
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> + dp_out: endpoint { };
>> + };
>> + };
>> + };
>> +
>> dispcc: clock-controller@...0000 {
>> compatible = "qcom,sc7180-dispcc";
>> reg = <0 0x0af00000 0 0x200000>;
Powered by blists - more mailing lists