| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <59fa420c-411c-4b17-9a5f-36d5726254ea@quicinc.com>
Date: Fri, 1 Nov 2024 11:47:02 +0530
From: Taniya Das <quic_tdas@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Bjorn Andersson
<andersson@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
"Stephen
Boyd" <sboyd@...nel.org>,
Abhishek Sahu <absahu@...eaurora.org>, Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon
<will@...nel.org>
CC: Ajit Pandey <quic_ajipan@...cinc.com>,
Imran Shaik
<quic_imrashai@...cinc.com>,
Jagadeesh Kona <quic_jkona@...cinc.com>,
"Stephen Boyd" <sboyd@...eaurora.org>, <linux-arm-msm@...r.kernel.org>,
<linux-clk@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 04/12] clk: qcom: camcc-qcs615: Add QCS615 camera clock
controller driver
On 10/21/2024 1:48 PM, Taniya Das wrote:
>
>
> On 10/19/2024 6:23 AM, Bryan O'Donoghue wrote:
>> On 18/10/2024 20:12, Taniya Das wrote:
>>> Add support for the camera clock controller for camera clients to
>>> be able to request for camcc clocks on QCS615 platform.
>>>
>>> Signed-off-by: Taniya Das <quic_tdas@...cinc.com>
>>> ---
>>> drivers/clk/qcom/Kconfig | 10 +
>>> drivers/clk/qcom/Makefile | 1 +
>>> drivers/clk/qcom/camcc-qcs615.c | 1588
>>> +++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 1599 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>>> index
>>> 30eb8236c9d80071a87e0332cfac7b667a08824a..bdb1c672dd90d96814b214afd234341e37e3c470 100644
>>> --- a/drivers/clk/qcom/Kconfig
>>> +++ b/drivers/clk/qcom/Kconfig
>>> @@ -460,6 +460,16 @@ config QCM_DISPCC_2290
>>> Say Y if you want to support display devices and
>>> functionality such as
>>> splash screen.
>>> +config QCS_CAMCC_615
>>> + tristate "QCS615 Camera Clock Controller"
>>> + depends on ARM64 || COMPILE_TEST
>>> + select QCS_GCC_615
>>> + help
>>> + Support for the camera clock controller on Qualcomm
>>> Technologies, Inc
>>> + QCS615 devices.
>>> + Say Y if you want to support camera devices and functionality
>>> such as
>>> + capturing pictures.
>>> +
>>> config QCS_GCC_404
>>> tristate "QCS404 Global Clock Controller"
>>> help
>>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>>> index
>>> 2b378667a63ff6eca843d7bef638a5422d35c3d3..f69c1bc13d3eca1859d9e849399e55175df869c3 100644
>>> --- a/drivers/clk/qcom/Makefile
>>> +++ b/drivers/clk/qcom/Makefile
>>> @@ -69,6 +69,7 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
>>> obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
>>> obj-$(CONFIG_QCM_GCC_2290) += gcc-qcm2290.o
>>> obj-$(CONFIG_QCM_DISPCC_2290) += dispcc-qcm2290.o
>>> +obj-$(CONFIG_QCS_CAMCC_615) += camcc-qcs615.o
>>> obj-$(CONFIG_QCS_GCC_404) += gcc-qcs404.o
>>> obj-$(CONFIG_QCS_Q6SSTOP_404) += q6sstop-qcs404.o
>>> obj-$(CONFIG_QCS_TURING_404) += turingcc-qcs404.o
>>> diff --git a/drivers/clk/qcom/camcc-qcs615.c
>>> b/drivers/clk/qcom/camcc-qcs615.c
>>> new file mode 100644
>>> index
>>> 0000000000000000000000000000000000000000..2341ddb57598eaaa7fa35300ae6635ff40da99ae
>>> --- /dev/null
>>> +++ b/drivers/clk/qcom/camcc-qcs615.c
>>> @@ -0,0 +1,1588 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights
>>> reserved.
>>> + */
>>> +
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +#include <dt-bindings/clock/qcom,qcs615-camcc.h>
>>> +
>>> +#include "clk-alpha-pll.h"
>>> +#include "clk-branch.h"
>>> +#include "clk-pll.h"
>>> +#include "clk-rcg.h"
>>> +#include "clk-regmap.h"
>>> +#include "clk-regmap-divider.h"
>>> +#include "clk-regmap-mux.h"
>>> +#include "common.h"
>>> +#include "gdsc.h"
>>> +#include "reset.h"
>>> +
>>> +enum {
>>> + DT_BI_TCXO,
>>> + DT_BI_TCXO_AO,
>>> +};
>>> +
>>> +enum {
>>> + P_BI_TCXO,
>>> + P_CAM_CC_PLL0_OUT_AUX,
>>> + P_CAM_CC_PLL1_OUT_AUX,
>>> + P_CAM_CC_PLL2_OUT_AUX2,
>>> + P_CAM_CC_PLL2_OUT_EARLY,
>>> + P_CAM_CC_PLL3_OUT_MAIN,
>>> +};
>>> +
>>> +static const struct pll_vco brammo_vco[] = {
>>> + { 500000000, 1250000000, 0 },
>>> +};
>>> +
>>> +static const struct pll_vco spark_vco[] = {
>>> + { 1000000000, 2100000000, 0 },
>>> + { 750000000, 1500000000, 1 },
>>> + { 500000000, 1000000000, 2 },
>>> + { 300000000, 500000000, 3 },
>>> + { 550000000, 1100000000, 4 },
>>> +};
>>> +
>>> +/* 600MHz configuration */
>>> +static const struct alpha_pll_config cam_cc_pll0_config = {
>>> + .l = 0x1f,
>>> + .alpha_hi = 0x40,
>>> + .alpha_en_mask = BIT(24),
>>> + .vco_val = 0x2 << 20,
>>> + .vco_mask = 0x3 << 20,
>>> + .aux_output_mask = BIT(1),
>>> + .config_ctl_val = 0x4001055b,
>>> + .test_ctl_hi_val = 0x1,
>>> + .test_ctl_hi_mask = 0x1,
>>> +};
>>> +
>>> +static struct clk_alpha_pll cam_cc_pll0 = {
>>> + .offset = 0x0,
>>> + .vco_table = spark_vco,
>>> + .num_vco = ARRAY_SIZE(spark_vco),
>>> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>> + .clkr = {
>>> + .hw.init = &(const struct clk_init_data) {
>>> + .name = "cam_cc_pll0",
>>> + .parent_data = &(const struct clk_parent_data) {
>>> + .index = DT_BI_TCXO,
>>> + },
>>> + .num_parents = 1,
>>> + .ops = &clk_alpha_pll_ops,
>>> + },
>>> + },
>>> +};
>>> +
>>> +/* 808MHz configuration */
>>> +static struct alpha_pll_config cam_cc_pll1_config = {
>>> + .l = 0x2A,
>>> + .alpha_hi = 0x15,
>>> + .alpha = 0x55555555,
>>> + .alpha_en_mask = BIT(24),
>>> + .vco_val = 0x2 << 20,
>>> + .vco_mask = 0x3 << 20,
>>> + .aux_output_mask = BIT(1),
>>> + .config_ctl_val = 0x4001055b,
>>> + .test_ctl_hi_val = 0x1,
>>> + .test_ctl_hi_mask = 0x1,
>>> +};
>>> +
>>> +static struct clk_alpha_pll cam_cc_pll1 = {
>>> + .offset = 0x1000,
>>> + .vco_table = spark_vco,
>>> + .num_vco = ARRAY_SIZE(spark_vco),
>>> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>> + .clkr = {
>>> + .hw.init = &(const struct clk_init_data) {
>>> + .name = "cam_cc_pll1",
>>> + .parent_data = &(const struct clk_parent_data) {
>>> + .index = DT_BI_TCXO,
>>> + },
>>> + .num_parents = 1,
>>> + .ops = &clk_alpha_pll_ops,
>>> + },
>>> + },
>>> +};
>>> +
>>> +/* 960MHz configuration */
>>> +static struct alpha_pll_config cam_cc_pll2_config = {
>>> + .l = 0x32,
>>> + .vco_val = 0x0 << 20,
>>
>> zero shifted any direction is still zero
>>
>> zed.c
>>
>> #include <stdio.h>
>> #include <stdint.h>
>>
>> int main(int argc, char *argv[])
>> {
>> uint32_t a = 0, b = 0 << 20;
>>
>> printf("a = %d b = %d\n", a, b);
>>
>> return 0;
>> }
>>
>> gcc -o zed zed.c
>>
>> a = 0 b = 0
>>
Correct, but this is generated. Will fix in the next patch.
>>> +static struct gdsc bps_gdsc = {
>>> + .gdscr = 0x6004,
>>> + .en_rest_wait_val = 0x2,
>>> + .en_few_wait_val = 0x2,
>>> + .clk_dis_wait_val = 0xf,
>>> + .pd = {
>>> + .name = "bps_gdsc",
>>> + },
>>> + .pwrsts = PWRSTS_OFF_ON,
>>> + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR,
>>> +};
>>> +
>>> +static struct gdsc ife_0_gdsc = {
>>> + .gdscr = 0x9004,
>>> + .en_rest_wait_val = 0x2,
>>> + .en_few_wait_val = 0x2,
>>> + .clk_dis_wait_val = 0xf,
>>> + .pd = {
>>> + .name = "ife_0_gdsc",
>>> + },
>>> + .pwrsts = PWRSTS_OFF_ON,
>>> + .flags = POLL_CFG_GDSCR,
>>> +};
>>> +
>>> +static struct gdsc ife_1_gdsc = {
>>> + .gdscr = 0xa004,
>>> + .en_rest_wait_val = 0x2,
>>> + .en_few_wait_val = 0x2,
>>> + .clk_dis_wait_val = 0xf,
>>> + .pd = {
>>> + .name = "ife_1_gdsc",
>>> + },
>>> + .pwrsts = PWRSTS_OFF_ON,
>>> + .flags = POLL_CFG_GDSCR,
>>> +};
>>
>> Shouldn't these have RETAIN flags ?
>>
No supported on this target.
>>> +
>>> +static struct gdsc ipe_0_gdsc = {
>>> + .gdscr = 0x7004,
>>> + .en_rest_wait_val = 0x2,
>>> + .en_few_wait_val = 0x2,
>>> + .clk_dis_wait_val = 0xf,
>>> + .pd = {
>>> + .name = "ipe_0_gdsc",
>>> + },
>>> + .pwrsts = PWRSTS_OFF_ON,
>>> + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
>>> +};
>>
>> I'd say those flags are very aspirational suggest POLL_CFG_GDSCR |
>> RETAIN_FF_ENABLE.
>>
>>
It does not hurt if we keep HW_CTRL_TRIGGER, but will remove.
>>> +
>>> +static struct gdsc titan_top_gdsc = {
>>> + .gdscr = 0xb134,
>>> + .en_rest_wait_val = 0x2,
>>> + .en_few_wait_val = 0x2,
>>> + .clk_dis_wait_val = 0xf,
>>> + .pd = {
>>> + .name = "titan_top_gdsc",
>>> + },
>>> + .pwrsts = PWRSTS_OFF_ON,
>>> + .flags = POLL_CFG_GDSCR,
>>> +};
>>>
>>
>> As Dmitry queried, TOP_GDSC should almost certainly be the parent of
>> the IFE/IPE/BPS and others.
>>
Sure, will fix.
>>> +static int cam_cc_qcs615_probe(struct platform_device *pdev)
>>> +{
>>> + struct regmap *regmap;
>>> +
>>> + regmap = qcom_cc_map(pdev, &cam_cc_qcs615_desc);
>>> + if (IS_ERR(regmap))
>>> + return PTR_ERR(regmap);
>>> +
>>> + clk_alpha_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
>>> + clk_alpha_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
>>> + clk_alpha_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
>>> + clk_alpha_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>>
>> Got to be missing something like
>>
>> /* Keep some clocks always-on */
>> qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */
>>
There is no GSDSC clock on QCS615.
>> If the GDSC gets declocked everything beneath it - including the stuff
>> in RETAIN goes away...
>>
>> Smells wrong.
>>
>> ---
>> bod
>
> Please help review:
> https://patchwork.kernel.org/project/linux-clk/cover/20241019-qcs615-mm-clockcontroller-v1-0-4cfb96d779ae@quicinc.com/
>
--
Thanks & Regards,
Taniya Das.
Powered by blists - more mailing lists