[PATCH v3 0/3] clk: qcom: dispcc-sm8650: round of fixes

[PATCH v3 0/3] clk: qcom: dispcc-sm8650: round of fixes

[Neil Armstrong]

While trying to fix a crash when display is started late in the
boot process, I ran on multiple issues with the DISPCC clock
definitions that needed some fixups.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v3:
- Do not switch to shared ops when the only parent is tcxo
- Collected reviews
- Link to v2: https://lore.kernel.org/r/20240624-topic-sm8650-upstream-fix-dispcc-v2-0-ddaa13f3b207@linaro.org

Changes in v2:
- Squashed patch 2 into patch 1
- Dropped shared_ops to disp_cc_sleep_clk_src & disp_cc_xo_clk_src as Taniya recommends
- Dropped patch 3
- Removed  wait_val fields updates from GDSC, this requires a larger solution
- Link to v1: https://lore.kernel.org/r/20240621-topic-sm8650-upstream-fix-dispcc-v1-0-7b297dd9fcc1@linaro.org

---
Neil Armstrong (3):
      clk: qcom: dispcc-sm8650: Park RCG's clk source at XO during disable
      clk: qcom: dispcc-sm8650: add missing CLK_SET_RATE_PARENT flag
      clk: qcom: dispcc-sm8650: Update the GDSC flags

 drivers/clk/qcom/dispcc-sm8650.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
---
base-commit: 4f40be61af99a67d5580c1448acd9b74c0376389
change-id: 20240621-topic-sm8650-upstream-fix-dispcc-a1994038c003

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH v3 1/3] clk: qcom: dispcc-sm8650: Park RCG's clk source at XO during disable

[Neil Armstrong]

The RCG's clk src has to be parked at XO while disabling as per the
HW recommendation, hence use clk_rcg2_shared_ops to achieve the same.

It also changes dptx1_aux_clk_src to use the correct ops instead of
clk_dp_ops.

Fixes: 9e939f008338 ("clk: qcom: add the SM8650 Display Clock Controller driver")
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/clk/qcom/dispcc-sm8650.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-sm8650.c b/drivers/clk/qcom/dispcc-sm8650.c
index c9d2751f5cb8..80fe25afccf7 100644
--- a/drivers/clk/qcom/dispcc-sm8650.c
+++ b/drivers/clk/qcom/dispcc-sm8650.c
@@ -398,7 +398,7 @@ static struct clk_rcg2 disp_cc_mdss_dptx1_aux_clk_src = {
 		.parent_data = disp_cc_parent_data_0,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_dp_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -560,7 +560,7 @@ static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
 		.parent_data = disp_cc_parent_data_5,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_ops,
+		.ops = &clk_rcg2_shared_ops,
 	},
 };
 
@@ -575,7 +575,7 @@ static struct clk_rcg2 disp_cc_mdss_esc1_clk_src = {
 		.parent_data = disp_cc_parent_data_5,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_ops,
+		.ops = &clk_rcg2_shared_ops,
 	},
 };
 

-- 
2.34.1

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

[Neil Armstrong]

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(+)

diff --git a/drivers/clk/qcom/dispcc-sm8650.c b/drivers/clk/qcom/dispcc-sm8650.c
index 80fe25afccf7..f38f5f43acb2 100644
--- a/drivers/clk/qcom/dispcc-sm8650.c
+++ b/drivers/clk/qcom/dispcc-sm8650.c
@@ -696,6 +696,7 @@ static struct clk_regmap_div disp_cc_mdss_byte0_div_clk_src = {
 			&disp_cc_mdss_byte0_clk_src.clkr.hw,
 		},
 		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_regmap_div_ops,
 	},
 };
@@ -710,6 +711,7 @@ static struct clk_regmap_div disp_cc_mdss_byte1_div_clk_src = {
 			&disp_cc_mdss_byte1_clk_src.clkr.hw,
 		},
 		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_regmap_div_ops,
 	},
 };

-- 
2.34.1

[PATCH v3 3/3] clk: qcom: dispcc-sm8650: Update the GDSC flags

[Neil Armstrong]

Update the GDSC flags by adding missing POLL_CFG_GDSCR flag.

Fixes: 9e939f008338 ("clk: qcom: add the SM8650 Display Clock Controller driver")
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/clk/qcom/dispcc-sm8650.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-sm8650.c b/drivers/clk/qcom/dispcc-sm8650.c
index f38f5f43acb2..bb99c1325717 100644
--- a/drivers/clk/qcom/dispcc-sm8650.c
+++ b/drivers/clk/qcom/dispcc-sm8650.c
@@ -1610,7 +1610,7 @@ static struct gdsc mdss_gdsc = {
 		.name = "mdss_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL | RETAIN_FF_ENABLE,
+	.flags = POLL_CFG_GDSCR | HW_CTRL | RETAIN_FF_ENABLE,
 };
 
 static struct gdsc mdss_int2_gdsc = {
@@ -1619,7 +1619,7 @@ static struct gdsc mdss_int2_gdsc = {
 		.name = "mdss_int2_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL | RETAIN_FF_ENABLE,
+	.flags = POLL_CFG_GDSCR | HW_CTRL | RETAIN_FF_ENABLE,
 };
 
 static struct clk_regmap *disp_cc_sm8650_clocks[] = {

-- 
2.34.1

Re: [PATCH v3 1/3] clk: qcom: dispcc-sm8650: Park RCG's clk source at XO during disable

[Konrad Dybcio]

On 16.07.2024 11:05 AM, Neil Armstrong wrote:
> The RCG's clk src has to be parked at XO while disabling as per the
> HW recommendation, hence use clk_rcg2_shared_ops to achieve the same.
> 
> It also changes dptx1_aux_clk_src to use the correct ops instead of
> clk_dp_ops.
> 
> Fixes: 9e939f008338 ("clk: qcom: add the SM8650 Display Clock Controller driver")
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

Re: [PATCH v3 1/3] clk: qcom: dispcc-sm8650: Park RCG's clk source at XO during disable

[Dmitry Baryshkov]

On Tue, Jul 16, 2024 at 11:05:21AM GMT, Neil Armstrong wrote:
> The RCG's clk src has to be parked at XO while disabling as per the
> HW recommendation, hence use clk_rcg2_shared_ops to achieve the same.
> 
> It also changes dptx1_aux_clk_src to use the correct ops instead of
> clk_dp_ops.
> 
> Fixes: 9e939f008338 ("clk: qcom: add the SM8650 Display Clock Controller driver")
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/clk/qcom/dispcc-sm8650.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/dispcc-sm8650.c b/drivers/clk/qcom/dispcc-sm8650.c
> index c9d2751f5cb8..80fe25afccf7 100644
> --- a/drivers/clk/qcom/dispcc-sm8650.c
> +++ b/drivers/clk/qcom/dispcc-sm8650.c
> @@ -398,7 +398,7 @@ static struct clk_rcg2 disp_cc_mdss_dptx1_aux_clk_src = {
>  		.parent_data = disp_cc_parent_data_0,
>  		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
>  		.flags = CLK_SET_RATE_PARENT,
> -		.ops = &clk_dp_ops,
> +		.ops = &clk_rcg2_ops,

Should this also be clk_rcg2_shared_ops ?

>  	},
>  };
>  
> @@ -560,7 +560,7 @@ static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
>  		.parent_data = disp_cc_parent_data_5,
>  		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
>  		.flags = CLK_SET_RATE_PARENT,
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_shared_ops,
>  	},
>  };
>  
> @@ -575,7 +575,7 @@ static struct clk_rcg2 disp_cc_mdss_esc1_clk_src = {
>  		.parent_data = disp_cc_parent_data_5,
>  		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
>  		.flags = CLK_SET_RATE_PARENT,
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_shared_ops,
>  	},
>  };
>  
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 1/3] clk: qcom: dispcc-sm8650: Park RCG's clk source at XO during disable

[Neil Armstrong]

On 16/07/2024 13:40, Dmitry Baryshkov wrote:
> On Tue, Jul 16, 2024 at 11:05:21AM GMT, Neil Armstrong wrote:
>> The RCG's clk src has to be parked at XO while disabling as per the
>> HW recommendation, hence use clk_rcg2_shared_ops to achieve the same.
>>
>> It also changes dptx1_aux_clk_src to use the correct ops instead of
>> clk_dp_ops.
>>
>> Fixes: 9e939f008338 ("clk: qcom: add the SM8650 Display Clock Controller driver")
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/clk/qcom/dispcc-sm8650.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/dispcc-sm8650.c b/drivers/clk/qcom/dispcc-sm8650.c
>> index c9d2751f5cb8..80fe25afccf7 100644
>> --- a/drivers/clk/qcom/dispcc-sm8650.c
>> +++ b/drivers/clk/qcom/dispcc-sm8650.c
>> @@ -398,7 +398,7 @@ static struct clk_rcg2 disp_cc_mdss_dptx1_aux_clk_src = {
>>   		.parent_data = disp_cc_parent_data_0,
>>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
>>   		.flags = CLK_SET_RATE_PARENT,
>> -		.ops = &clk_dp_ops,
>> +		.ops = &clk_rcg2_ops,
> 
> Should this also be clk_rcg2_shared_ops ?

Nop, the only parent is tcxo

> 
>>   	},
>>   };
>>   
>> @@ -560,7 +560,7 @@ static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
>>   		.parent_data = disp_cc_parent_data_5,
>>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
>>   		.flags = CLK_SET_RATE_PARENT,
>> -		.ops = &clk_rcg2_ops,
>> +		.ops = &clk_rcg2_shared_ops,
>>   	},
>>   };
>>   
>> @@ -575,7 +575,7 @@ static struct clk_rcg2 disp_cc_mdss_esc1_clk_src = {
>>   		.parent_data = disp_cc_parent_data_5,
>>   		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
>>   		.flags = CLK_SET_RATE_PARENT,
>> -		.ops = &clk_rcg2_ops,
>> +		.ops = &clk_rcg2_shared_ops,
>>   	},
>>   };
>>   
>>
>> -- 
>> 2.34.1
>>
> 

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

[Neil Armstrong]

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.

Neil

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

[Dmitry Baryshkov]

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).

-- 
With best wishes
Dmitry

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

[neil.armstrong]

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.

Thanks,
Neil

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Konrad Dybcio]

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

Konrad

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Konrad Dybcio]

On 17.07.2024 11:49 AM, 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.

You can mention in it the message of this patch.

Konrad

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

[neil.armstrong]

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.

Neil

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

[Dmitry Baryshkov]

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).

-- 
With best wishes
Dmitry

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

[Konrad Dybcio]

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
>