Re: [PATCH v2 2/2] clk: qcom: Add lpass clock controller driver for SDM845

[Taniya Das <tdas@codeaurora.org>]

On 7/7/2018 5:09 AM, Stephen Boyd wrote:
> 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,
>> +               },
>> +       },
>> +};
>> +

Moved the above clocks to GCC driver.

>> +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.
> 

Added a comment in the next patch.

>> +       .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.

Added a comment in the next patch.

> 
>> +       .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.
> 

Added a comment in the next patch.

>> +       .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.
> 

There are no more clocks from these CCs which would be required to be 
controlled. These are only required for the Low Pass Audio subsystem to 
be brought out of reset.

> 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.
> 

I have moved to use the platform device APIs.

>> +               return -ENOMEM;
>> +
>> +       base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
> 
> And devm_ioremap_resource().

Use the above API to map.

> 
>> +       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.
> 

Have fixed the index in the next patch.

>> +
>> +       ret = lpass_clocks_sdm845_probe(pdev, index, desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return 0;
> 
> Just 'return lpass_clocks_sdm845_probe()'.
> 

removed the check for ret.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--