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: <23575f97-332b-0392-fc20-0a52775d03b9@quicinc.com>
Date:   Thu, 10 Aug 2023 12:20:43 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH v3 5/6] clk: qcom: mmcc-msm8998: Fix the SMMU GDSC
Content-Language: en-US
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@somainline.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
        Imran Khan <kimran@codeaurora.org>,
        "Rajendra Nayak" <quic_rjendra@quicinc.com>,
        Joonwoo Park <joonwoop@codeaurora.org>,
        Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        "Joerg Roedel" <joro@8bytes.org>
CC:     Marijn Suijten <marijn.suijten@somainline.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Jami Kettunen <jami.kettunen@somainline.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <iommu@lists.linux.dev>
References: <20230531-topic-8998_mmssclk-v3-0-ba1b1fd9ee75@linaro.org>
 <20230531-topic-8998_mmssclk-v3-5-ba1b1fd9ee75@linaro.org>
From:   Jeffrey Hugo <quic_jhugo@quicinc.com>
In-Reply-To: <20230531-topic-8998_mmssclk-v3-5-ba1b1fd9ee75@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/9/2023 1:20 PM, Konrad Dybcio wrote:
> The SMMU GDSC doesn't have to be ALWAYS-ON and shouldn't feature the
> HW_CTRL flag (it's separate from hw_ctrl_addr).  In addition to that,
> it should feature a cxc entry for bimc_smmu_axi_clk and be marked as
> votable.

I appear to have confused HW_CTRL with hw_ctrl_addr.  Thanks for fixing 
that.

I recall I made it always-on for display handoff.  The bootloader on the 
laptops will enable the display, which means the MDP is active and using 
the SMMU.  The SMMU is powered by the GDSC as you know.  The MDP is 
going to be polling a framebuffer in DDR, which EFI services (efifb) is 
going to be updating.  All of this is active during linux boot, which is 
how the kernel bootlog gets printed on screen.

If I remember right, the GDSC will be registered.  When it is done 
probing, there will be no consumers.  So the Linux framework will step 
in and turn it off before the consumers come up.  This kills power to 
the SMMU.  If the SMMU doesn't come back on before the MDP polls DDR 
again, you get a bus hang and a crash.

I assumed that any msm8998 device would be using the MDP/GPU and thus 
the SMMU would pretty much always be powered on.

I expected this patch to break the laptop.  It does not in my testing. 
However, I see that I disabled the MMCC node in DT with a todo about the 
display.  So the GDSC is never registered, and then never gets turned 
off.  I believe that todo is pending some updates I need to make to the 
TI DSI/eDP bridge because the I2C port on the bridge is not wired up.  I 
should really dust that off and complete it.

Regardless, even with the todo addressed, I think removing always-on 
will still break the laptops unless the bootloader handoff of display 
was solved and I missed it.

I get that for your usecase, a phone where the bootloader does not init 
the display, always-on has the potential to burn extra power.  I'm not 
sure how to make both of us happy through.

Do you have any suggestions?

> 
> Fix all of these issues.
> 
> Fixes: d14b15b5931c ("clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/clk/qcom/mmcc-msm8998.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
> index d0a5440e2291..4fdc41e7d2a8 100644
> --- a/drivers/clk/qcom/mmcc-msm8998.c
> +++ b/drivers/clk/qcom/mmcc-msm8998.c
> @@ -2627,11 +2627,13 @@ static struct gdsc camss_cpp_gdsc = {
>   static struct gdsc bimc_smmu_gdsc = {
>   	.gdscr = 0xe020,
>   	.gds_hw_ctrl = 0xe024,
> +	.cxcs = (unsigned int []){ 0xe008 },
> +	.cxc_count = 1,
>   	.pd = {
>   		.name = "bimc_smmu",
>   	},
>   	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = HW_CTRL | ALWAYS_ON,
> +	.flags = VOTABLE,
>   };
>   
>   static struct clk_regmap *mmcc_msm8998_clocks[] = {
>