Re: [PATCH v3 2/3] clk: qcom: dispcc-sm8650: add missing CLK_SET_RATE_PARENT flag

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 17.07.2024 11:57 AM, Dmitry Baryshkov wrote:
> On Wed, 17 Jul 2024 at 12:53, <neil.armstrong@linaro.org> wrote:
>>
>> On 17/07/2024 11:49, Dmitry Baryshkov wrote:
>>> On Wed, 17 Jul 2024 at 12:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>
>>>> On 16.07.2024 6:46 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Jul 16, 2024 at 03:46:24PM GMT, neil.armstrong@linaro.org wrote:
>>>>>> On 16/07/2024 15:44, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 16 Jul 2024 at 15:32, Neil Armstrong <neil.armstrong@linaro.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@linaro.org>
>>>>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.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.
> 
> Yeah, regmap_phy_mux_ops is a nasty one.
> 
> Or the whole story about converting flags from vendor DT to upstream
> driver code.
> 
> Probably it's worth specifying everything somewhere under
> Documentation/. Or in drivers/clk/qcom/common.h  Or a wiki page
> somewhere (though my preference is towards the in-kernel docs).

drivers/clk/qcom/docs.rst.. Half-joking, half not.. there is some hw
docs in Documentation/ but I'm not sure if more than 3 people know
about it.

Konrad
>