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: <43d6523e-d6b7-4fda-92bb-a52fcad2fdba@linaro.org>
Date: Wed, 17 Jul 2024 11:53:50 +0200
From: neil.armstrong@...aro.org
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Taniya Das <quic_tdas@...cinc.com>,
 linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] clk: qcom: dispcc-sm8650: add missing
 CLK_SET_RATE_PARENT flag

On 17/07/2024 11:49, Dmitry Baryshkov wrote:
> On Wed, 17 Jul 2024 at 12:47, Konrad Dybcio <konrad.dybcio@...aro.org> wrote:
>>
>> On 16.07.2024 6:46 PM, Dmitry Baryshkov wrote:
>>> On Tue, Jul 16, 2024 at 03:46:24PM GMT, neil.armstrong@...aro.org wrote:
>>>> On 16/07/2024 15:44, Dmitry Baryshkov wrote:
>>>>> On Tue, 16 Jul 2024 at 15:32, Neil Armstrong <neil.armstrong@...aro.org> wrote:
>>>>>>
>>>>>> On 16/07/2024 13:20, Dmitry Baryshkov wrote:
>>>>>>> On Tue, Jul 16, 2024 at 11:05:22AM GMT, Neil Armstrong wrote:
>>>>>>>> Add the missing CLK_SET_RATE_PARENT for the byte0_div_clk_src
>>>>>>>> and byte1_div_clk_src, the clock rate should propagate to
>>>>>>>> the corresponding _clk_src.
>>>>>>>>
>>>>>>>> Fixes: 9e939f008338 ("clk: qcom: add the SM8650 Display Clock Controller driver")
>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
>>>>>>>> ---
>>>>>>>>     drivers/clk/qcom/dispcc-sm8650.c | 2 ++
>>>>>>>>     1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> This doesn't seem correct, the byte1_div_clk_src is a divisor, so the
>>>>>>> rate should not be propagated. Other platforms don't set this flag.
>>>>>>>
>>>>>>
>>>>>> Why not ? the disp_cc_mdss_byte1_clk_src has CLK_SET_RATE_PARENT and a div_table,
>>>>>> and we only pass DISP_CC_MDSS_BYTE1_CLK to the dsi controller.
>>>>>
>>>>> Yes, the driver sets byte_clk with the proper rate, then it sets
>>>>> byte_intf_clk, which results in a proper divisor.
>>>>> If we have CLK_SET_RATE_PARENT for byte1_div_clk_src, then setting
>>>>> byte_intf_clk rate will also result in a rate change for the byte_clk
>>>>> rate.
>>>>>
>>>>> Note, all other platforms don't set that flag for this reason (I think
>>>>> I had to remove it during sm8450 development for this reason).
>>>>>
>>>>
>>>> Ack, I think this deserves a comment explaining this, I'll add it.
>>>
>>> But where to place it? This applies to _all_ dispcc controllers.
>>
>> Commit message
> 
> It is already committed.
> 

The thing we keep adding new clock drivers based on previous ones that uses
specific ops and flags with no documented reasons except in commit messages,
but it's often buried into multiple cleanup changes.

So at some point we should add simple comments before each special clock
explaining what we're doing here, a good example is the clk_regmap_phy_mux_ops,
where I had to dig into commit logs and understand why we handle it differently
from downstream.

Neil


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ