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;
Message-ID: <8a704b63-9ef1-ed4d-3ee5-35ebfd2a2318@linaro.org>
Date:   Wed, 18 Jan 2023 12:50:54 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH v3 5/8] arm64: dts: qcom: Add msm8939 SoC
Content-Language: en-US
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To:     Stephan Gerhold <stephan@gerhold.net>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        djakov@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, benl@squareup.com,
        shawn.guo@linaro.org, fabien.parent@linaro.org, leo.yan@linaro.org,
        dmitry.baryshkov@linaro.org, Jun Nie <jun.nie@linaro.org>,
        James Willcox <jwillcox@squareup.com>,
        Joseph Gates <jgates@squareup.com>,
        Max Chen <mchen@squareup.com>, Zac Crosby <zac@squareup.com>,
        Vincent Knecht <vincent.knecht@mailoo.org>
References: <20230117024846.1367794-1-bryan.odonoghue@linaro.org>
 <20230117024846.1367794-6-bryan.odonoghue@linaro.org>
 <Y8fC/GCHfENQmBNC@gerhold.net>
 <cf4920e6-c007-20a5-ba3a-5005b22f891b@linaro.org>
In-Reply-To: <cf4920e6-c007-20a5-ba3a-5005b22f891b@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 18/01/2023 11:50, Bryan O'Donoghue wrote:
>>> +                clocks = <&gcc GCC_MDSS_MDP_CLK>,
>>> +                     <&gcc GCC_MDSS_AHB_CLK>,
>>> +                     <&gcc GCC_MDSS_AXI_CLK>,
>>> +                     <&gcc GCC_MDSS_BYTE1_CLK>,
>>> +                     <&gcc GCC_MDSS_PCLK1_CLK>,
>>> +                     <&gcc GCC_MDSS_ESC1_CLK>;
>>> +                clock-names = "mdp_core",
>>> +                          "iface",
>>> +                          "bus",
>>> +                          "byte",
>>> +                          "pixel",
>>> +                          "core";
>>> +                assigned-clocks = <&gcc BYTE1_CLK_SRC>,
>>> +                          <&gcc PCLK1_CLK_SRC>;
>>> +                assigned-clock-parents = <&dsi_phy1 0>,
>>> +                             <&dsi_phy1 1>;
>>
>> Does this work? Confusingly, BYTE1/PCLK1_CLK_SRC can only have dsi0pll
>> as parent in gcc-msm8939 and not the dsi1pll. <&dsi_phy1 0/1> is not a
>> valid parent for those clocks.
> 
> No you're right, its all wrong. I will correct it
> 
>          mdss_dsi0: qcom,mdss_dsi@1a98000 {
>                  compatible = "qcom,mdss-dsi-ctrl";
>                  label = "MDSS DSI CTRL->0";
>                  cell-index = <0>;
>                  reg = <0x1a98000 0x25c>,
>                        <0x1a98500 0x2b0>,
>                        <0x193e000 0x30>;
>                  reg-names = "dsi_ctrl", "dsi_phy", "mmss_misc_phys";
>                  qcom,mdss-fb-map = <&mdss_fb0>;
>                  qcom,mdss-mdp = <&mdss_mdp>;
>                  gdsc-supply = <&gdsc_mdss>;
>                  vdda-supply = <&pm8916_l2>;
>                  vdd-supply = <&pm8916_l17>;
>                  vddio-supply = <&pm8916_l6>;
>                  clocks = <&clock_gcc clk_gcc_mdss_mdp_clk>,
>                           <&clock_gcc clk_gcc_mdss_ahb_clk>,
>                           <&clock_gcc clk_gcc_mdss_axi_clk>,
>                           <&clock_gcc_mdss clk_gcc_mdss_byte0_clk>,
>                           <&clock_gcc_mdss clk_gcc_mdss_pclk0_clk>,
>                           <&clock_gcc clk_gcc_mdss_esc0_clk>;

Sorry what am I saying that's the wrong dsiX

Here's downstream - byte1, plck1, esc1 exist

         mdss_dsi1: qcom,mdss_dsi@1aa0000 {
                 compatible = "qcom,mdss-dsi-ctrl";
                 label = "MDSS DSI CTRL->1";
                 cell-index = <1>;
                 reg = <0x1aa0000 0x25c>,
                       <0x1aa0500 0x2b0>,
                       <0x193e000 0x30>;
                 reg-names = "dsi_ctrl", "dsi_phy", "mmss_misc_phys";
                 qcom,mdss-fb-map = <&mdss_fb0>;
                 qcom,mdss-mdp = <&mdss_mdp>;
                 gdsc-supply = <&gdsc_mdss>;
                 vdda-supply = <&pm8916_l2>;
                 vdd-supply = <&pm8916_l17>;
                 vddio-supply = <&pm8916_l6>;
                 clocks = <&clock_gcc clk_gcc_mdss_mdp_clk>,
                          <&clock_gcc clk_gcc_mdss_ahb_clk>,
                          <&clock_gcc clk_gcc_mdss_axi_clk>,
                          <&clock_gcc_mdss clk_gcc_mdss_byte1_clk>,
                          <&clock_gcc_mdss clk_gcc_mdss_pclk1_clk>,
                          <&clock_gcc clk_gcc_mdss_esc1_clk>;
                 clock-names = "mdp_core_clk", "iface_clk", "bus_clk",
                                 "byte_clk", "pixel_clk", "core_clk";


Parent clock is gpll0a but they waggle different rcgr addresses

static struct clk_rcg2 byte0_clk_src = {
         .cmd_rcgr = 0x4d044,    <- here
         .hid_width = 5,
         .parent_map = gcc_xo_gpll0a_dsibyte_map,
         .clkr.hw.init = &(struct clk_init_data){
                 .name = "byte0_clk_src",

static struct clk_rcg2 byte1_clk_src = {
         .cmd_rcgr = 0x4d0b0,    <- and here
         .hid_width = 5,
         .parent_map = gcc_xo_gpll0a_dsibyte_map,
         .clkr.hw.init = &(struct clk_init_data){
                 .name = "byte1_clk_src",

It *should* work even with the wrong name but, I will send a GCC update 
to address the naming, which looks wrong.

---
bod