| 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: <0ce9fdb6-e224-ced7-ec32-fe67b2ca6127@marek.ca>
Date: Wed, 23 Sep 2020 12:07:16 -0400
From: Jonathan Marek <jonathan@...ek.ca>
To: Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Michael Turquette <mturquette@...libre.com>,
linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v2 5/5] clk: qcom: add video clock controller driver for
SM8250
On 9/22/20 2:46 PM, Stephen Boyd wrote:
> Quoting Jonathan Marek (2020-09-03 20:09:54)
>> Add support for the video clock controller found on SM8250 based devices.
>>
>> Derived from the downstream driver.
>>
>> Signed-off-by: Jonathan Marek <jonathan@...ek.ca>
>> ---
>> drivers/clk/qcom/Kconfig | 9 +
>> drivers/clk/qcom/Makefile | 1 +
>> drivers/clk/qcom/videocc-sm8250.c | 518 ++++++++++++++++++++++++++++++
>> 3 files changed, 528 insertions(+)
>> create mode 100644 drivers/clk/qcom/videocc-sm8250.c
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 40d7ee9886c9..95efa38211d5 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -453,6 +453,15 @@ config SM_VIDEOCC_8150
>> Say Y if you want to support video devices and functionality such as
>> video encode and decode.
>>
>> +config SM_VIDEOCC_8250
>> + tristate "SM8250 Video Clock Controller"
>> + select SDM_GCC_8250
>> + select QCOM_GDSC
>> + help
>> + Support for the video clock controller on SM8250 devices.
>> + Say Y if you want to support video devices and functionality such as
>> + video encode and decode.
>> +
>> config SPMI_PMIC_CLKDIV
>> tristate "SPMI PMIC clkdiv Support"
>> depends on SPMI || COMPILE_TEST
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 6f4c580d2728..55fb20800b66 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -69,6 +69,7 @@ obj-$(CONFIG_SM_GCC_8250) += gcc-sm8250.o
>> obj-$(CONFIG_SM_GPUCC_8150) += gpucc-sm8150.o
>> obj-$(CONFIG_SM_GPUCC_8250) += gpucc-sm8250.o
>> obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o
>> +obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.o
>> obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
>> obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
>> obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
>> diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
>> new file mode 100644
>> index 000000000000..a814d10945c4
>> --- /dev/null
>> +++ b/drivers/clk/qcom/videocc-sm8250.c
>> @@ -0,0 +1,518 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2018-2020, 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-sm8250.h>
>> +
> [...]
>> +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 = &(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,
>> + },
>> +};
>> +
>> +static struct clk_rcg2 video_cc_xo_clk_src = {
>> + .cmd_rcgr = 0xecc,
>> + .mnd_width = 0,
>> + .hid_width = 5,
>> + .parent_map = video_cc_parent_map_0,
>> + .freq_tbl = ftbl_video_cc_ahb_clk_src,
>> + .clkr.hw.init = &(struct clk_init_data){
>> + .name = "video_cc_xo_clk_src",
>> + .parent_data = video_cc_parent_data_0,
>> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0),
>> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
>
> Similar critical clk comment, see below.
>
>> + .ops = &clk_rcg2_ops,
>> + },
>> +};
>> +
>> +static struct clk_branch video_cc_ahb_clk = {
>> + .halt_reg = 0xe58,
>> + .halt_check = BRANCH_HALT,
>> + .clkr = {
>> + .enable_reg = 0xe58,
>> + .enable_mask = BIT(0),
>> + .hw.init = &(struct clk_init_data){
>> + .name = "video_cc_ahb_clk",
>> + .parent_data = &(const struct clk_parent_data){
>> + .hw = &video_cc_ahb_clk_src.clkr.hw,
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
>
> Similar critical clk comment, see below.
>
>> + .ops = &clk_branch2_ops,
>> + },
>> + },
>> +};
>> +
>> +static struct clk_branch video_cc_mvs0_clk = {
>> + .halt_reg = 0xd34,
>> + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */
>
> Is this resolved?
>
Downstream has this clock as BRANCH_HALT_VOTED, but with the upstream
venus driver (with patches to enable sm8250), that results in a
"video_cc_mvs0_clk status stuck at 'off" error. AFAIK venus
enables/disables this clock on its own (venus still works without
touching this clock), but I didn't want to remove this in case it might
be needed. I removed these clocks in the v3 I just sent.
>> + .clkr = {
>> + .enable_reg = 0xd34,
>> + .enable_mask = BIT(0),
>> + .hw.init = &(struct clk_init_data){
>> + .name = "video_cc_mvs0_clk",
>> + .parent_data = &(const struct clk_parent_data){
>> + .hw = &video_cc_mvs0_div_clk_src.clkr.hw,
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + .ops = &clk_branch2_ops,
>> + },
>> + },
>> +};
>> +
> [...]
>> +
>> +static struct clk_branch video_cc_xo_clk = {
>> + .halt_reg = 0xeec,
>> + .halt_check = BRANCH_HALT,
>> + .clkr = {
>> + .enable_reg = 0xeec,
>> + .enable_mask = BIT(0),
>> + .hw.init = &(struct clk_init_data){
>> + .name = "video_cc_xo_clk",
>> + .parent_data = &(const struct clk_parent_data){
>> + .hw = &video_cc_xo_clk_src.clkr.hw,
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
>
> Please add a coment why it's critical. If no consumer of this clk exists
> it's preferred to move the enabling to probe and not waste memory
> modeling it in software.
>
>> + .ops = &clk_branch2_ops,
>> + },
>> + },
>> +};
>> +
Powered by blists - more mailing lists