linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taniya Das <tdas@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	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,
	devicetree@vger.kernel.org, robh@kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v4 5/5] clk: qcom: Add Global Clock controller (GCC) driver for SC7180
Date: Thu, 31 Oct 2019 16:59:26 +0530	[thread overview]
Message-ID: <fa17b97d-bfc4-4e9c-78b5-c225e5b38946@codeaurora.org> (raw)
In-Reply-To: <20191029175941.GA27773@google.com>

Hi Matthias,

Thanks for your comments.

On 10/29/2019 11:29 PM, Matthias Kaehlcke wrote:
> Hi Taniya,
> 
> On Mon, Oct 14, 2019 at 03:53:08PM +0530, Taniya Das wrote:
>> Add support for the global clock controller found on SC7180
>> based devices. This should allow most non-multimedia device
>> drivers to probe and control their clocks.
>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>

> 
> v3 also had
> 
> +	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,
> 
> Removing it makes the dpu_mdss driver unhappy:
> 
> [    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available
> 
> because:
> 
>          mdss: mdss@ae00000 {
>      	        ...
> 
>   =>             clocks = <&gcc GCC_DISP_AHB_CLK>,
>                           <&gcc GCC_DISP_HF_AXI_CLK>,
>                           <&dispcc DISP_CC_MDSS_MDP_CLK>;
>                  clock-names = "iface", "gcc_bus", "core";
> 	};
> 

The basic idea as you mentioned below was to move the CRITICAL clocks to 
probe. The clock provider to return NULL in case the clocks are not 
registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

Yes it would throw these warnings, but no functional issue is observed 
from display. I have tested it on the cheza board.
I guess we could fix the DRM driver to use the "devm_clk_get_optional()" 
instead?

> More clocks were removed in v4:
> 
> -       [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr,
> -       [GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
> -       [GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr,
> 
> I guess this part of "remove registering the CRITICAL clocks to clock provider
> and leave them always ON from the GCC probe." (change log entry), but are you
> sure nobody is going to reference these clocks?
> 

Even if they are referenced clk provider would return NULL.

>> +static int gcc_sc7180_probe(struct platform_device *pdev)
>> +{
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	regmap = qcom_cc_map(pdev, &gcc_sc7180_desc);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	/*
>> +	 * Disable the GPLL0 active input to MM blocks, NPU
>> +	 * and GPU via MISC registers.
>> +	 */
>> +	regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3);
>> +	regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
>> +	regmap_update_bits(regmap, 0x71028, 0x3, 0x3);
> 
> In v3 this was:
> 
> 	regmap_update_bits(regmap, GCC_MMSS_MISC, 0x3, 0x3);
> 	regmap_update_bits(regmap, GCC_NPU_MISC, 0x3, 0x3);
> 	regmap_update_bits(regmap, GCC_GPU_MISC, 0x3, 0x3);
> 
> IMO register names seem preferable, why switch to literal addresses
> instead?
> 

:). These cleanups where done based on the comments I had received 
during SDM845 review. If Stephen is fine moving them to names, I could 
submit them in the next patch series.

>> +
>> +	/*
>> +	 * Keep the clocks always-ON
>> +	 * GCC_CPUSS_GNOC_CLK, GCC_VIDEO_AHB_CLK, GCC_DISP_AHB_CLK
>> +	 * GCC_GPU_CFG_AHB_CLK
>> +	 */
>> +	regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
>> +	regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
>> +	regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
>> +	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
> 
> ditto, register names seem preferable.
> 

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

--

  reply	other threads:[~2019-10-31 11:29 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-14 10:23 [PATCH v4 0/5] Add Global Clock controller (GCC) driver for SC7180 Taniya Das
2019-10-14 10:23 ` [PATCH v4 1/5] clk: qcom: rcg: update the DFS macro for RCG Taniya Das
2019-11-07 21:11   ` Stephen Boyd
2019-10-14 10:23 ` [PATCH v4 2/5] clk: qcom: common: Return NULL from clk_hw OF provider Taniya Das
2019-11-07 21:11   ` Stephen Boyd
2019-10-14 10:23 ` [PATCH v4 3/5] dt-bindings: clock: Add YAML schemas for the QCOM GCC clock bindings Taniya Das
2019-10-14 16:49   ` Rob Herring
2019-11-07 21:11   ` Stephen Boyd
2019-10-14 10:23 ` [PATCH v4 4/5] dt-bindings: clock: Introduce " Taniya Das
2019-10-14 16:50   ` Rob Herring
2019-11-07 21:11   ` Stephen Boyd
2019-10-14 10:23 ` [PATCH v4 5/5] clk: qcom: Add Global Clock controller (GCC) driver for SC7180 Taniya Das
2019-10-29 17:59   ` Matthias Kaehlcke
2019-10-31 11:29     ` Taniya Das [this message]
2019-10-31 17:41       ` Matthias Kaehlcke
2019-11-07 21:06         ` Stephen Boyd
2019-11-08  2:06           ` Rob Clark
2019-11-08  6:35             ` Stephen Boyd
2019-11-08 16:54               ` Rob Clark
2019-11-08 18:42                 ` Stephen Boyd
2019-11-08 19:40                   ` Rob Clark
2019-11-08 21:14                     ` Stephen Boyd
2019-11-14  1:02                     ` Matthias Kaehlcke
2019-11-14  5:30                       ` Rob Clark
2019-11-14 17:04                         ` Matthias Kaehlcke
2019-11-07 21:12   ` Stephen Boyd
2019-11-14 17:34   ` Matthias Kaehlcke

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=fa17b97d-bfc4-4e9c-78b5-c225e5b38946@codeaurora.org \
    --to=tdas@codeaurora.org \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.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=mka@chromium.org \
    --cc=mturquette@baylibre.com \
    --cc=rnayak@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).