linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taniya Das <tdas@codeaurora.org>
To: Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845
Date: Tue, 9 Oct 2018 22:56:38 +0530	[thread overview]
Message-ID: <ba0d7d92-5ad9-2dcf-c5a5-21332df193f0@codeaurora.org> (raw)
In-Reply-To: <153896666821.119890.13143150697797456341@swboyd.mtv.corp.google.com>

Hello Stephen,

On 10/8/2018 8:14 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-10-04 05:02:26)
>> Add support for the lpass clock controller found on SDM845 based devices.
>> This would allow lpass peripheral loader drivers to control the clocks to
>> bring the subsystem out of reset.
>> LPASS clocks present on the global clock controller would be registered
>> with the clock framework based on the device tree flag. Also do not gate
>> these clocks if they are left unused.
> 
> Why not gate them? This statement states what the code is doing, not why
> it's doing it which is the more crucial information that should be
> described in the commit text. Also, please add a comment about it to the
> code next to the flag.
> 
> I am concerned that it doesn't make any sense though, so probably it
> shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over some
> other larger bug that needs to be fixed.
> 

It does not have any bug, it is just that to access these lpass 
registers we would need the GCC lpass registers to be enabled. I would 
update the same in the commit text.

During clock late_init these clocks should not be accessed to check the 
clock status as they would result in unclocked access. The client would 
request these clocks in the correct order and it would not have any issue.


>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
>> index 08d593e..6379b8b 100644
>> --- a/drivers/clk/qcom/gcc-sdm845.c
>> +++ b/drivers/clk/qcom/gcc-sdm845.c
>> @@ -3169,6 +3169,32 @@ enum {
>>          },
>>   };
>>
>> +static struct clk_branch gcc_lpass_q6_axi_clk = {
>> +       .halt_reg = 0x47000,
>> +       .halt_check = BRANCH_HALT,
>> +       .clkr = {
>> +               .enable_reg = 0x47000,
>> +               .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 = 0x47008,
>> +       .halt_check = BRANCH_HALT,
>> +       .clkr = {
>> +               .enable_reg = 0x47008,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "gcc_lpass_sway_clk",
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>>   static struct gdsc pcie_0_gdsc = {
>>          .gdscr = 0x6b004,
>>          .pd = {
>> @@ -3469,6 +3495,8 @@ enum {
>>          [GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr,
>>          [GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr,
>>          [GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr,
>> +       [GCC_LPASS_Q6_AXI_CLK] = NULL,
>> +       [GCC_LPASS_SWAY_CLK] = NULL,
>>   };
>>
>>   static const struct qcom_reset_map gcc_sdm845_resets[] = {
>> @@ -3583,6 +3611,13 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
>>          if (ret)
>>                  return ret;
>>
>> +       if (!of_property_read_bool(pdev->dev.of_node, "qcom,lpass-protected")) {
>> +               gcc_sdm845_clocks[GCC_LPASS_Q6_AXI_CLK] =
>> +                       &gcc_lpass_q6_axi_clk.clkr;
>> +               gcc_sdm845_clocks[GCC_LPASS_SWAY_CLK] =
>> +                       &gcc_lpass_sway_clk.clkr;
>> +       }
>> +
>>          return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
>>   }
>>
>> diff --git a/drivers/clk/qcom/lpasscc-sdm845.c b/drivers/clk/qcom/lpasscc-sdm845.c
>> new file mode 100644
>> index 0000000..f7b9b0f
>> --- /dev/null
>> +++ b/drivers/clk/qcom/lpasscc-sdm845.c
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/bitops.h>
> 
> Is this needed?
> 

Would check this.

>> +/* CLK_OFF would not toggle until LPASS is not out of reset */
>> +static struct clk_branch lpass_qdsp6ss_xo_clk = {
>> +       .halt_reg = 0x38,
>> +       .halt_check = BRANCH_HALT_SKIP,
>> +       .clkr = {
>> +               .enable_reg = 0x38,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_xo_clk",
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +/* CLK_OFF would not toggle until LPASS is not out of reset */
> 
> Move this comment next to BRANCH_HALT_SKIP please so we know what it
> relates to.
> 

Sure would take care in the next patch.

>> +static struct clk_branch lpass_qdsp6ss_sleep_clk = {
>> +       .halt_reg = 0x3c,
>> +       .halt_check = BRANCH_HALT_SKIP,
>> +       .clkr = {
>> +               .enable_reg = 0x3c,
>> +               .enable_mask = BIT(0),
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "lpass_qdsp6ss_sleep_clk",
>> +                       .flags = CLK_IGNORE_UNUSED,
>> +                       .ops = &clk_branch2_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static int lpass_clocks_sdm845_probe(struct platform_device *pdev, int index,
>> +                                    const struct qcom_cc_desc *desc)
>> +{
>> +       struct regmap *regmap;
>> +       struct resource *res;
>> +       void __iomem *base;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +       base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>> +
>> +       regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
> 
> If this happens again in the future we should move this into the
> common.c file and let qcom_cc_probe_index() exist.
> 

Yes, I agree, could submit a patch to add the new function and then 
clean it up.

>> +
>> +       return qcom_cc_really_probe(pdev, desc, regmap);
>> +}
>> +
>> +/* LPASS CC clock controller */
> 
> Please remove this comment. It's useless.
> 

Would take care in the next patch.

>> +static const struct of_device_id lpass_cc_sdm845_match_table[] = {
>> +       { .compatible = "qcom,sdm845-lpasscc" },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, lpass_cc_sdm845_match_table);
>> +

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

--

  reply	other threads:[~2018-10-09 17:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-04 12:02 [PATCH v6] Add support for LPASS clock controller " Taniya Das
2018-10-04 12:02 ` [PATCH v6] clk: qcom: Add lpass clock controller driver " Taniya Das
2018-10-08  2:44   ` Stephen Boyd
2018-10-09 17:26     ` Taniya Das [this message]
2018-10-09 20:52       ` Stephen Boyd
2018-10-10  6:12         ` Taniya Das
2018-10-12 17:35           ` Stephen Boyd
2018-10-17 11:37             ` Taniya Das
2018-10-17 12:04               ` Taniya Das
2018-10-17 14:20                 ` Stephen Boyd
2018-10-19 10:39                   ` Taniya Das
2018-10-25 10:51                     ` tdas
2018-10-29 18:44                       ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ba0d7d92-5ad9-2dcf-c5a5-21332df193f0@codeaurora.org \
    --to=tdas@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=rnayak@codeaurora.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v6] clk: qcom: Add lpass clock controller driver for SDM845' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).