| 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
| ||
|
Date: Fri, 06 Jul 2018 16:39:37 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Michael Turquette <mturquette@...libre.com>,
Taniya Das <tdas@...eaurora.org>
Cc: Andy Gross <andy.gross@...aro.org>,
David Brown <david.brown@...aro.org>,
Rajendra Nayak <rnayak@...eaurora.org>,
Amit Nischal <anischal@...eaurora.org>,
linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
rohitkr@...eaurora.org, Taniya Das <tdas@...eaurora.org>
Subject: Re: [PATCH v2 2/2] clk: qcom: Add lpass clock controller driver for SDM845
Quoting Taniya Das (2018-07-04 23:55:21)
> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
> new file mode 100644
> index 0000000..5285b26
> --- /dev/null
> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,lpass-sdm845.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-branch.h"
> +#include "common.h"
> +
> +static struct clk_branch gcc_lpass_q6_axi_clk = {
> + .halt_reg = 0x0,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0x0,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_lpass_q6_axi_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch gcc_lpass_sway_clk = {
> + .halt_reg = 0x8,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0x8,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_lpass_sway_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch lpass_audio_wrapper_aon_clk = {
> + .halt_reg = 0x098,
> + .halt_check = BRANCH_VOTED,
> + .clkr = {
> + .enable_reg = 0x098,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "lpass_audio_wrapper_aon_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch lpass_q6ss_ahbm_aon_clk = {
> + .halt_reg = 0x12000,
> + .halt_check = BRANCH_VOTED,
I'll take your word for it.
> + .clkr = {
> + .enable_reg = 0x12000,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "lpass_q6ss_ahbm_aon_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch lpass_q6ss_ahbs_aon_clk = {
> + .halt_reg = 0x1f000,
> + .halt_check = BRANCH_VOTED,
> + .clkr = {
> + .enable_reg = 0x1f000,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "lpass_q6ss_ahbs_aon_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch lpass_qdsp6ss_xo_clk = {
> + .halt_reg = 0x18,
> + .halt_check = BRANCH_HALT_SKIP,
Why? Hint, add a comment.
> + .clkr = {
> + .enable_reg = 0x18,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "lpass_qdsp6ss_xo_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch lpass_qdsp6ss_sleep_clk = {
> + .halt_reg = 0x1c,
> + .halt_check = BRANCH_HALT_SKIP,
Why? Hint, add a comment.
> + .clkr = {
> + .enable_reg = 0x1c,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "lpass_qdsp6ss_sleep_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch lpass_qdsp6ss_core_clk = {
> + .halt_reg = 0x0,
> + .halt_check = BRANCH_HALT_SKIP,
Again.
> + .clkr = {
> + .enable_reg = 0x0,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "lpass_qdsp6ss_core_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct regmap_config lpass_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> +};
> +
> +static struct clk_regmap *lpass_gcc_sdm845_clocks[] = {
> + [GCC_LPASS_Q6_AXI_CLK] = &gcc_lpass_q6_axi_clk.clkr,
> + [GCC_LPASS_SWAY_CLK] = &gcc_lpass_sway_clk.clkr,
> +};
> +
> +static const struct qcom_cc_desc lpass_gcc_sdm845_desc = {
> + .config = &lpass_regmap_config,
> + .clks = lpass_gcc_sdm845_clocks,
> + .num_clks = ARRAY_SIZE(lpass_gcc_sdm845_clocks),
> +};
> +
> +static struct clk_regmap *lpass_cc_sdm845_clocks[] = {
> + [LPASS_AUDIO_WRAPPER_AON_CLK] = &lpass_audio_wrapper_aon_clk.clkr,
> + [LPASS_Q6SS_AHBM_AON_CLK] = &lpass_q6ss_ahbm_aon_clk.clkr,
> + [LPASS_Q6SS_AHBS_AON_CLK] = &lpass_q6ss_ahbs_aon_clk.clkr,
So it's kinda sad that these lists have holes in them and we just waste
space forever the more we add. How about using the same linear array,
but indexing into the array with different offsets based on the start of
the numberspace for those clks? It would mean that adding new clks
wouldn't work, so add all the clks now instead of later.
Otherwise, maybe we need to add support for qcom_cc_desc having two
cells and then indicating which first cell to map into? So the binding
would look like:
<&lpasscc 0 GCC_LPASS_Q6_AXI_CLK>
<&lpasscc 1 LPASS_Q6SS_AHBM_AON_CLK>
and our lookup function would need to be different for the multi-cell
thing, but otherwise works and avoids the hole and adding numbers later
problem.
> +};
> +
> +static const struct qcom_cc_desc lpass_cc_sdm845_desc = {
> + .config = &lpass_regmap_config,
> + .clks = lpass_cc_sdm845_clocks,
> + .num_clks = ARRAY_SIZE(lpass_cc_sdm845_clocks),
> +};
> +
> +static struct clk_regmap *lpass_qdsp6ss_sdm845_clocks[] = {
> + [LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr,
> + [LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr,
> + [LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr,
> +};
> +
> +static const struct qcom_cc_desc lpass_qdsp6ss_sdm845_desc = {
> + .config = &lpass_regmap_config,
> + .clks = lpass_qdsp6ss_sdm845_clocks,
> + .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sdm845_clocks),
> +};
> +
> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
> + const struct qcom_cc_desc *desc)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct regmap *regmap;
> + struct resource res;
> + void __iomem *base;
> +
> + if (of_address_to_resource(np, index, &res))
Just use platform device APIs instead please.
> + return -ENOMEM;
> +
> + base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
And devm_ioremap_resource().
> + if (IS_ERR(base))
> + return -ENOMEM;
> +
> + regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return qcom_cc_really_probe(pdev, desc, regmap);
> +}
> +
> +/* LPASS CC clock controller */
> +static const struct of_device_id lpass_cc_sdm845_match_table[] = {
> + { .compatible = "qcom,sdm845-lpasscc" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
> +
> +static int lpass_cc_sdm845_probe(struct platform_device *pdev)
> +{
> + const struct qcom_cc_desc *desc;
> + struct device_node *np;
> + int ret, index;
> +
> + np = pdev->dev.of_node;
> +
> + lpass_regmap_config.name = "lpass_gcc";
> + desc = &lpass_gcc_sdm845_desc;
> + index = of_property_match_string(np, "reg-names", "lpass_gcc");
> +
> + ret = lpass_clocks_sdm845_probe(pdev, index, desc);
> + if (ret)
> + return ret;
> +
> + lpass_regmap_config.name = "lpass_cc";
> + desc = &lpass_cc_sdm845_desc;
> + index = of_property_match_string(np, "reg-names", "lpass_cc");
> +
> + ret = lpass_clocks_sdm845_probe(pdev, index, desc);
> + if (ret)
> + return ret;
> +
> + lpass_regmap_config.name = "lpass_qdsp6ss";
> + desc = &lpass_qdsp6ss_sdm845_desc;
> + index = of_property_match_string(np, "reg-names", "lpass_qdsp6ss");
We shouldn't need to do this. Just index them by number and specify that
the order has to be exactly that order. reg-names is supposed to be
optional.
> +
> + ret = lpass_clocks_sdm845_probe(pdev, index, desc);
> + if (ret)
> + return ret;
> +
> + return 0;
Just 'return lpass_clocks_sdm845_probe()'.
Powered by blists - more mailing lists