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: <7bbbf520-d70f-5df6-33ce-a888bf364aa5@linaro.org>
Date:   Fri, 26 May 2023 22:17:06 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2 3/3] media: camss: Link CAMSS power domain
Content-Language: en-US
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Yassine Oudjana <yassine.oudjana@gmail.com>,
        Robert Foss <rfoss@kernel.org>,
        Todor Tomov <todor.too@gmail.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Hans Verkuil <hansverk@cisco.com>
Cc:     Yassine Oudjana <y.oudjana@protonmail.com>,
        Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>,
        linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230526180712.8481-1-y.oudjana@protonmail.com>
 <20230526180712.8481-4-y.oudjana@protonmail.com>
 <fa395680-0e6c-3eb0-9d5a-f90a95c394b8@linaro.org>
 <e755eabf-f8d7-c2cd-aa5a-bc48c7a5d5ae@linaro.org>
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
In-Reply-To: <e755eabf-f8d7-c2cd-aa5a-bc48c7a5d5ae@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26/05/2023 21:57, Konrad Dybcio wrote:
> This code contains a whole bunch of hacky counting logic that should have
> been substituted with _byname, but now we're stuck with indices to keep
> compatibility with old DTs :/
> 
> If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
> CAMSS-related GDSCs, we could make it their parent in the clock driver
> and call it a day.

I mean, it wouldn't make much sense from a hw design POV if that weren't 
the case..

Hmm looks like its already there.

static struct gdsc vfe0_gdsc = {
         .gdscr = 0x3664,
         .cxcs = (unsigned int []){ 0x36a8 },
         .cxc_count = 1,
         .pd = {
                 .name = "vfe0",
         },
         .parent = &camss_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
};

static struct gdsc vfe1_gdsc = {
         .gdscr = 0x3674,
         .cxcs = (unsigned int []){ 0x36ac },
         .cxc_count = 1,
         .pd = {
                 .name = "vfe1",
         },
         .parent = &camss_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
};

I feel this is probably a problem in the description of dependencies for 
the CSIPHY in the dts for the 8996..

I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..

static const struct resources csiphy_res_8x96[] = {
         /* CSIPHY0 */
         {
                 .regulators = {},
                 .clock = { "top_ahb", "ispif_ahb", "ahb", 
"csiphy0_timer" },


should probably look something like

static const struct resources csiphy_res_8x96[] = {
         /* CSIPHY0 */
         {
                 .regulators = {},
                 .clock = { "top_ahb", "ispif_ahb", "ahb", 
"csiphy0_timer", "vfe0"},

But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC 
via the VFEx

Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?

git diff
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi 
b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 30257c07e1279..60e5d3f5336d4 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2120,7 +2120,8 @@ camss: camss@a00000 {
                                 "vfe0",
                                 "vfe1";
                         power-domains = <&mmcc VFE0_GDSC>,
-                                       <&mmcc VFE1_GDSC>;
+                                       <&mmcc VFE1_GDSC>,
+                                       <&mmcc CAMSS_GDSC>;

Either of those approaches should mitigate this patch.

---
bod