From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 30117C77B73 for ; Fri, 14 Apr 2023 17:48:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229461AbjDNRsV (ORCPT ); Fri, 14 Apr 2023 13:48:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38238 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229479AbjDNRsT (ORCPT ); Fri, 14 Apr 2023 13:48:19 -0400 Received: from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com [IPv6:2a00:1450:4864:20::12d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4E489AD28 for ; Fri, 14 Apr 2023 10:48:08 -0700 (PDT) Received: by mail-lf1-x12d.google.com with SMTP id i26so24759865lfc.6 for ; Fri, 14 Apr 2023 10:48:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1681494486; x=1684086486; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=q8j1jr5BkH9v0KQPH+g+TSZh3RFTw5oSiPfNJDMkbUI=; b=eFVLbYGIciacDsJBsokCs+zDUEpRfZe/mbufuCA7rjpl8voW1F8fW+mDemas8NMbQB U+2rhs7A/DGfQBE+5bCXo9w4Vw3mKbqiSDg/qRSx/mZug/YPNnD/f0y3nrqYQJeKlT0R xnm8Ipc4zKrkWklZQeU6uIORyrZvw/hnq33b9gNN6dBUcq3bu/Z/fgt7hWFVhug2PYvH pSoAE+gz7Gky2f5sev3FQb32VcUO/8BmEcuzEAGMDCZCLe9nsvaNyW1jHuJo8T1ABF3K P5HMB5LD/D83HIzJY+7wSBCeDtS9pC54TsViwQSnjELHdeMy0zHq6oc8dfdTrp/DL2xr dR1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681494486; x=1684086486; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=q8j1jr5BkH9v0KQPH+g+TSZh3RFTw5oSiPfNJDMkbUI=; b=fuPVHCHic2Q3pk2YW5q3LN0bW2X2wR3Ucl6FUrSVw6ta16ew+D3uRATcwYAoO7UauP MQoRFxHFJARwFr9hj5PKco61u86mY7WEZ7Hhjj6irS4xr704tjYqD+j0W8AxH1Wp8+gm j30ej6qhMkGa+Qr/Gy+wExC9BZNkHz3lZ4qtu3CHHrtaMXbQ0C8ws38T6MuTAe2yUnnW gN5i6Ew5z5OMG5nVpPhhHJmnG98Rb/5O//LyttKOaeR1kL1Oh8+l4680HODYotdxbwAV BIbgkc4rKXLRhnN/C/JxGYOv+ayZAtAMZexvd2pOzr1dK/KuzGeZ2SKdXX475t4ZNgAW /TUQ== X-Gm-Message-State: AAQBX9e9yNaxmWMP/e+RpOP/ve3Jv4l1nBQeU6nxHedoIVtuXhrAUi7w zzQWG6QPRSoFBC/XM8578J7Iow== X-Google-Smtp-Source: AKy350Y8qOhfBv0DEfMa0AOWbSWbMI2Avxvvgkp83SOAZx+cTkGCMDntuL3v/WIQVj/hLPyMQFsvAw== X-Received: by 2002:a19:ae17:0:b0:4ec:5607:9055 with SMTP id f23-20020a19ae17000000b004ec56079055mr1962569lfc.31.1681494486467; Fri, 14 Apr 2023 10:48:06 -0700 (PDT) Received: from [192.168.1.101] (abyk99.neoplus.adsl.tpnet.pl. [83.9.30.99]) by smtp.gmail.com with ESMTPSA id r13-20020ac252ad000000b004d5a6dcb94fsm901536lfm.33.2023.04.14.10.48.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 14 Apr 2023 10:48:06 -0700 (PDT) Message-ID: <34797b11-b654-a9a4-ac26-5287ca582a82@linaro.org> Date: Fri, 14 Apr 2023 19:48:03 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH v2 2/2] clk: qcom: Introduce SM8350 VIDEOCC Content-Language: en-US To: Dmitry Baryshkov Cc: Bjorn Andersson , Andy Gross , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Philipp Zabel , Taniya Das , Marijn Suijten , linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <20230413-topic-lahaina_vidcc-v2-0-f721d507e555@linaro.org> <20230413-topic-lahaina_vidcc-v2-2-f721d507e555@linaro.org> From: Konrad Dybcio In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 14.04.2023 18:31, Dmitry Baryshkov wrote: > On Fri, 14 Apr 2023 at 14:26, Konrad Dybcio wrote: >> >> Add support for the Video Clock Controller found on the SM8350 SoC. >> >> Signed-off-by: Konrad Dybcio >> --- [...] >> +static struct clk_rcg2 video_cc_ahb_clk_src = { >> + .cmd_rcgr = 0xbd4, >> + .mnd_width = 0, >> + .hid_width = 5, >> + .parent_map = video_cc_parent_map_0, >> + .freq_tbl = ftbl_video_cc_ahb_clk_src, >> + .clkr.hw.init = &(const struct clk_init_data) { >> + .name = "video_cc_ahb_clk_src", >> + .parent_data = video_cc_parent_data_0, >> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), >> + .flags = CLK_SET_RATE_PARENT, >> + .ops = &clk_rcg2_shared_ops, >> + }, >> +}; > > Do we need this clock at all? We don't have the child > video_cc_ahb_clk, so potentially CCF can try disabling or modifying > this clock. Hm.. I see a few things: 1. downstream kona has it, upstream does not 2. it's shared so we never actually hard-shut it off.. 2a. ..but it'd be good to ensure it's on when it's ready.. 2b. ..but we never do anyway.. 2c. ..but should we even? doesn't Venus govern it internally? > >> + >> +static const struct freq_tbl ftbl_video_cc_mvs0_clk_src[] = { >> + F(720000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >> + F(1014000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >> + F(1098000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >> + F(1332000000, P_VIDEO_PLL0_OUT_MAIN, 1, 0, 0), >> + { } >> +}; >> + [...] >> +static struct clk_branch video_cc_mvs1_clk = { >> + .halt_reg = 0xdb4, >> + .halt_check = BRANCH_HALT_VOTED, > > As a note, sm8250 has BRANCH_HALT here. No, it does on the div2 clk, and so do we: [...] >> +}; >> + >> +static struct clk_branch video_cc_mvs1_div2_clk = { >> + .halt_reg = 0xdf4, >> + .halt_check = BRANCH_HALT_VOTED, >> + .hwcg_reg = 0xdf4, [...] >> + >> +static const struct qcom_reset_map video_cc_sm8350_resets[] = { >> + [CVP_VIDEO_CC_INTERFACE_BCR] = { 0xe54 }, >> + [CVP_VIDEO_CC_MVS0_BCR] = { 0xd14 }, > > Would it be better to use common VIDEO_CC prefix here (IOW: > VIDEO_CC_CVP_MVS0_BCR, VIDEO_CC_CVP_INTERFACE_BCR), etc. My best guess would be that the ones prefixed with CVP_ are actual INTF/INSTANCEn(CORE) reset lines whereas the ones containing _CLK_ reset their clock sub-branches. > >> + [VIDEO_CC_MVS0C_CLK_ARES] = { 0xc34, 2 }, >> + [CVP_VIDEO_CC_MVS0C_BCR] = { 0xbf4 }, >> + [CVP_VIDEO_CC_MVS1_BCR] = { 0xd94 }, >> + [VIDEO_CC_MVS1C_CLK_ARES] = { 0xcd4, 2 }, >> + [CVP_VIDEO_CC_MVS1C_BCR] = { 0xc94 }, >> +}; [...] >> + ret = pm_runtime_resume_and_get(&pdev->dev); >> + if (ret) >> + return ret; >> + >> + regmap = qcom_cc_map(pdev, &video_cc_sm8350_desc); >> + if (IS_ERR(regmap)) { >> + pm_runtime_put(&pdev->dev); >> + return PTR_ERR(regmap); >> + }; > > Extra semicolon Ooeh! > >> + >> + clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config); >> + clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config); >> + >> + /* >> + * Keep clocks always enabled: >> + * video_cc_ahb_clk >> + * video_cc_xo_clk >> + */ >> + regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0)); >> + regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0)); >> + >> + ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap); >> + pm_runtime_put(&pdev->dev); >> + >> + return ret; >> +} >> + >> +static const struct dev_pm_ops video_cc_sm8350_pm_ops = { >> + SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL) > > The driver doesn't use pm_clk at all. Are these PM_OPS correct? I'm unsure. I see the pm state changing in debugfs when the clocks are (not) consumed. But let's continue our discussion about using pm_clks for AHB. > >> +}; >> + >> +static const struct of_device_id video_cc_sm8350_match_table[] = { >> + { .compatible = "qcom,sm8350-videocc" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, video_cc_sm8350_match_table); >> + >> +static struct platform_driver video_cc_sm8350_driver = { >> + .probe = video_cc_sm8350_probe, >> + .driver = { >> + .name = "sm8350-videocc", >> + .of_match_table = video_cc_sm8350_match_table, >> + .pm = &video_cc_sm8350_pm_ops, >> + }, >> +}; >> +module_platform_driver(video_cc_sm8350_driver); >> + >> +MODULE_DESCRIPTION("QTI SM8350 VIDEOCC Driver"); >> +MODULE_LICENSE("GPL"); >> >> -- >> 2.40.0 >> > > Generic note: the register layout follows closely sm8250. However the > existing differences probably do not warrant merging them. No, I don't think merging any designs that are farther away than 8150 and 8155 or 8992 and 8994 etc. is a good idea.. I don't want to ever look at something like dispcc-sm8[123]50.c again! Konrad >