linux-kernel - Re: [PATCH v2 2/2] clk: qcom: Add support for Global Clock Controller on QCS8300

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <818a4f89-d64b-4657-8845-d01caec0a750@quicinc.com>
Date: Wed, 28 Aug 2024 15:10:57 +0530
From: Imran Shaik <quic_imrashai@...cinc.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette
	<mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Richard Cochran <richardcochran@...il.com>,
        "Ajit
 Pandey" <quic_ajipan@...cinc.com>,
        Taniya Das <quic_tdas@...cinc.com>,
        Jagadeesh Kona <quic_jkona@...cinc.com>,
        Satya Priya Kakitapalli
	<quic_skakitap@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] clk: qcom: Add support for Global Clock Controller
 on QCS8300



On 8/26/2024 6:24 PM, Andrew Lunn wrote:
> On Mon, Aug 26, 2024 at 04:25:39PM +0530, Imran Shaik wrote:
>>
>>
>> On 8/23/2024 1:29 AM, Andrew Lunn wrote:
>>>> +static int gcc_qcs8300_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct regmap *regmap;
>>>> +	int ret;
>>>> +
>>>> +	regmap = qcom_cc_map(pdev, &gcc_qcs8300_desc);
>>>> +	if (IS_ERR(regmap))
>>>> +		return PTR_ERR(regmap);
>>>> +
>>>> +	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
>>>> +				       ARRAY_SIZE(gcc_dfs_clocks));
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Keep some clocks always enabled */
>>>
>>> Sorry, but you need to explain why. Why cannot the camera driver
>>> enable these clocks when it loads? Why cannot the display driver
>>> enable these clocks when it loads.
>>>
>>
>> These clocks are recommended to be kept always ON as per the HW design and
>> also exposing clock structures and marking them critical in the kernel would
>> lead to redundant code. Based on previous discussions with clock
>> maintainers, it is recommended to keep such clocks enabled at probe and not
>> model them. This approach is consistently followed for all other targets as
>> well.
> 
> I don't see why it would add redundant code. It is a few lines of code
> in the driver, which every driver using clocks has. If you really
> don't want the clock turned off because it is unused, you can use
> CLK_IGNORE_UNUSED, along with a comment explaining why.
> 
> What i was actually guessing is that you don't actually have open
> drivers for these hardware blocks, just a blob running in user
> space. As such, it cannot turn the clocks on. If that is the case, i
> would much prefer you are honest about this, and document it.
> 

We have recently discussed enabling the clocks at probe with the 
maintainers in the below threads as well.

https://lore.kernel.org/all/664cca91-8615-d3f6-7525-15b9b6725cce@quicinc.com/

It was concluded that keeping them enabled at probe is acceptable. We 
are now following this approach across all targets.

Thanks,
Imran

> 	Andrew