linux-kernel - Re: [PATCH v2 6/6] clk: qcom: Add video clock controller driver for SC7280

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <162261760498.4130789.12499425999582046146@swboyd.mtv.corp.google.com>
Date:   Wed, 02 Jun 2021 00:06:44 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Michael Turquette <mturquette@...libre.com>,
        Taniya Das <tdas@...eaurora.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, robh@...nel.org, robh+dt@...nel.org,
        Taniya Das <tdas@...eaurora.org>
Subject: Re: [PATCH v2 6/6] clk: qcom: Add video clock controller driver for SC7280

Quoting Taniya Das (2021-04-27 03:33:10)
> diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c
> new file mode 100644
> index 0000000..3387154
> --- /dev/null
> +++ b/drivers/clk/qcom/videocc-sc7280.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,videocc-sc7280.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "common.h"
> +#include "reset.h"
> +#include "gdsc.h"
> +
> +enum {
> +       P_BI_TCXO,
> +       P_SLEEP_CLK,
> +       P_VIDEO_PLL0_OUT_EVEN,
> +};
> +
> +static struct pll_vco lucid_vco[] = {

const?

> +       { 249600000, 2000000000, 0 },
> +};
> +
[...]
> +
> +static const struct parent_map video_cc_parent_map_0[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_VIDEO_PLL0_OUT_EVEN, 3 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_0[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .hw = &video_pll0.clkr.hw },
> +};
> +
> +static const struct parent_map video_cc_parent_map_1[] = {
> +       { P_SLEEP_CLK, 0 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_1[] = {
> +       { .fw_name = "sleep_clk" },
> +};
> +
> +static const struct parent_map video_cc_parent_map_2[] = {
> +       { P_BI_TCXO, 0 },
> +};
> +
> +static const struct clk_parent_data video_cc_parent_data_2_ao[] = {
> +       { .fw_name = "bi_tcxo_ao" },

This is new. Why would we want the video clk parent state to turn off
when the CPU is off? Does the video engine keep XO enabled for itself?
Can you please add some comment into the code explaining why it's ok to
use the ao clk here?

> +};
> +
> +static const struct freq_tbl ftbl_video_cc_iris_clk_src[] = {
> +       F(133333333, P_VIDEO_PLL0_OUT_EVEN, 3, 0, 0),
> +       F(240000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(335000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(424000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(460000000, P_VIDEO_PLL0_OUT_EVEN, 2, 0, 0),
> +       { }
> +};
[...]
> +
> +static struct clk_branch video_cc_xo_clk = {
> +       .halt_reg = 0x7018,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0x7018,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "video_cc_xo_clk",
> +                       .parent_hws = (const struct clk_hw*[]){
> +                               &video_cc_xo_clk_src.clkr.hw,
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,

Please add a comment why it is critical.

> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +