[PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Marc Gonzalez]

Video decoder (venus) was broken on msm8998.

PH found crude work-around:
Drop venus_sys_set_power_control() call.

Bryan suggested proper fix:
Set required register offsets in venus GDSC structs.
Set HW_CTRL flag.

GDSC = Globally Distributed Switch Controller

Use same code as mmcc-msm8996 with:
s/venus_gdsc/video_top_gdsc/
s/venus_core0_gdsc/video_subcore0_gdsc/
s/venus_core1_gdsc/video_subcore1_gdsc/

https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h
https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h

0x1024 = MMSS_VIDEO GDSCR (undocumented)
0x1028 = MMSS_VIDEO_CORE_CBCR
0x1030 = MMSS_VIDEO_AHB_CBCR
0x1034 = MMSS_VIDEO_AXI_CBCR
0x1038 = MMSS_VIDEO_MAXI_CBCR
0x1040 = MMSS_VIDEO_SUBCORE0 GDSCR (undocumented)
0x1044 = MMSS_VIDEO_SUBCORE1 GDSCR (undocumented)
0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
0x104c = MMSS_VIDEO_SUBCORE1_CBCR

Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 drivers/clk/qcom/mmcc-msm8998.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
index 1180e48c687ac..275fb3b71ede4 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -2535,6 +2535,8 @@ static struct clk_branch vmem_ahb_clk = {
 
 static struct gdsc video_top_gdsc = {
 	.gdscr = 0x1024,
+	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
+	.cxc_count = 3,
 	.pd = {
 		.name = "video_top",
 	},
@@ -2543,20 +2545,26 @@ static struct gdsc video_top_gdsc = {
 
 static struct gdsc video_subcore0_gdsc = {
 	.gdscr = 0x1040,
+	.cxcs = (unsigned int []){ 0x1048 },
+	.cxc_count = 1,
 	.pd = {
 		.name = "video_subcore0",
 	},
 	.parent = &video_top_gdsc.pd,
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = HW_CTRL,
 };
 
 static struct gdsc video_subcore1_gdsc = {
 	.gdscr = 0x1044,
+	.cxcs = (unsigned int []){ 0x104c },
+	.cxc_count = 1,
 	.pd = {
 		.name = "video_subcore1",
 	},
 	.parent = &video_top_gdsc.pd,
 	.pwrsts = PWRSTS_OFF_ON,
+	.flags = HW_CTRL,
 };
 
 static struct gdsc mdss_gdsc = {
-- 
2.34.1

Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Bjorn Andersson]

On Wed, Apr 10, 2024 at 01:13:17PM +0200, Marc Gonzalez wrote:
> Video decoder (venus) was broken on msm8998.

Could you please express something about in what way it was broken, or
how this manifested itself etc?

> 
> PH found crude work-around:

Would be nice if these names are spelled out, if you'd like to give
credit to the individuals.

> Drop venus_sys_set_power_control() call.
> 
> Bryan suggested proper fix:
> Set required register offsets in venus GDSC structs.
> Set HW_CTRL flag.
> 
> GDSC = Globally Distributed Switch Controller
> 
> Use same code as mmcc-msm8996 with:
> s/venus_gdsc/video_top_gdsc/
> s/venus_core0_gdsc/video_subcore0_gdsc/
> s/venus_core1_gdsc/video_subcore1_gdsc/
> 
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h
> 
> 0x1024 = MMSS_VIDEO GDSCR (undocumented)
> 0x1028 = MMSS_VIDEO_CORE_CBCR
> 0x1030 = MMSS_VIDEO_AHB_CBCR
> 0x1034 = MMSS_VIDEO_AXI_CBCR
> 0x1038 = MMSS_VIDEO_MAXI_CBCR
> 0x1040 = MMSS_VIDEO_SUBCORE0 GDSCR (undocumented)
> 0x1044 = MMSS_VIDEO_SUBCORE1 GDSCR (undocumented)
> 0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
> 0x104c = MMSS_VIDEO_SUBCORE1_CBCR
> 

Would you mind providing me a Fixes tag as well?

Thanks,
Bjorn

> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  drivers/clk/qcom/mmcc-msm8998.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
> index 1180e48c687ac..275fb3b71ede4 100644
> --- a/drivers/clk/qcom/mmcc-msm8998.c
> +++ b/drivers/clk/qcom/mmcc-msm8998.c
> @@ -2535,6 +2535,8 @@ static struct clk_branch vmem_ahb_clk = {
>  
>  static struct gdsc video_top_gdsc = {
>  	.gdscr = 0x1024,
> +	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
> +	.cxc_count = 3,
>  	.pd = {
>  		.name = "video_top",
>  	},
> @@ -2543,20 +2545,26 @@ static struct gdsc video_top_gdsc = {
>  
>  static struct gdsc video_subcore0_gdsc = {
>  	.gdscr = 0x1040,
> +	.cxcs = (unsigned int []){ 0x1048 },
> +	.cxc_count = 1,
>  	.pd = {
>  		.name = "video_subcore0",
>  	},
>  	.parent = &video_top_gdsc.pd,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = HW_CTRL,
>  };
>  
>  static struct gdsc video_subcore1_gdsc = {
>  	.gdscr = 0x1044,
> +	.cxcs = (unsigned int []){ 0x104c },
> +	.cxc_count = 1,
>  	.pd = {
>  		.name = "video_subcore1",
>  	},
>  	.parent = &video_top_gdsc.pd,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = HW_CTRL,
>  };
>  
>  static struct gdsc mdss_gdsc = {
> -- 
> 2.34.1
> 

Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Jeffrey Hugo]

On 4/10/2024 5:13 AM, Marc Gonzalez wrote:
> Video decoder (venus) was broken on msm8998.
> 
> PH found crude work-around:
> Drop venus_sys_set_power_control() call.
> 
> Bryan suggested proper fix:
> Set required register offsets in venus GDSC structs.
> Set HW_CTRL flag.
> 
> GDSC = Globally Distributed Switch Controller
> 
> Use same code as mmcc-msm8996 with:
> s/venus_gdsc/video_top_gdsc/
> s/venus_core0_gdsc/video_subcore0_gdsc/
> s/venus_core1_gdsc/video_subcore1_gdsc/
> 
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h
> 
> 0x1024 = MMSS_VIDEO GDSCR (undocumented)
> 0x1028 = MMSS_VIDEO_CORE_CBCR
> 0x1030 = MMSS_VIDEO_AHB_CBCR
> 0x1034 = MMSS_VIDEO_AXI_CBCR
> 0x1038 = MMSS_VIDEO_MAXI_CBCR
> 0x1040 = MMSS_VIDEO_SUBCORE0 GDSCR (undocumented)
> 0x1044 = MMSS_VIDEO_SUBCORE1 GDSCR (undocumented)
> 0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
> 0x104c = MMSS_VIDEO_SUBCORE1_CBCR

Reviewed the documentation, this is all correct.

> 
> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>   drivers/clk/qcom/mmcc-msm8998.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8998.c b/drivers/clk/qcom/mmcc-msm8998.c
> index 1180e48c687ac..275fb3b71ede4 100644
> --- a/drivers/clk/qcom/mmcc-msm8998.c
> +++ b/drivers/clk/qcom/mmcc-msm8998.c
> @@ -2535,6 +2535,8 @@ static struct clk_branch vmem_ahb_clk = {
>   
>   static struct gdsc video_top_gdsc = {
>   	.gdscr = 0x1024,
> +	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },

Checked that these (and the ones below) have the proper bits in the 
documentation to support this.  Sadly, the documentation does not 
mention using them, so I can't really tell if this is required or not.

> +	.cxc_count = 3,
>   	.pd = {
>   		.name = "video_top",
>   	},
> @@ -2543,20 +2545,26 @@ static struct gdsc video_top_gdsc = {
>   
>   static struct gdsc video_subcore0_gdsc = {
>   	.gdscr = 0x1040,
> +	.cxcs = (unsigned int []){ 0x1048 },
> +	.cxc_count = 1,
>   	.pd = {
>   		.name = "video_subcore0",
>   	},
>   	.parent = &video_top_gdsc.pd,
>   	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = HW_CTRL,
>   };
>   
>   static struct gdsc video_subcore1_gdsc = {
>   	.gdscr = 0x1044,
> +	.cxcs = (unsigned int []){ 0x104c },
> +	.cxc_count = 1,
>   	.pd = {
>   		.name = "video_subcore1",
>   	},
>   	.parent = &video_top_gdsc.pd,
>   	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = HW_CTRL,
>   };
>   
>   static struct gdsc mdss_gdsc = {

Overall, seems ok to me, but I did see Bjorn asking for some commit text 
edits which I agree with.

Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Konrad Dybcio]

On 4/10/24 13:13, Marc Gonzalez wrote:
> Video decoder (venus) was broken on msm8998.
> 
> PH found crude work-around:
> Drop venus_sys_set_power_control() call.
> 
> Bryan suggested proper fix:
> Set required register offsets in venus GDSC structs.
> Set HW_CTRL flag.
> 
> GDSC = Globally Distributed Switch Controller
> 
> Use same code as mmcc-msm8996 with:
> s/venus_gdsc/video_top_gdsc/
> s/venus_core0_gdsc/video_subcore0_gdsc/
> s/venus_core1_gdsc/video_subcore1_gdsc/
> 
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h
> 
> 0x1024 = MMSS_VIDEO GDSCR (undocumented)
> 0x1028 = MMSS_VIDEO_CORE_CBCR
> 0x1030 = MMSS_VIDEO_AHB_CBCR
> 0x1034 = MMSS_VIDEO_AXI_CBCR
> 0x1038 = MMSS_VIDEO_MAXI_CBCR
> 0x1040 = MMSS_VIDEO_SUBCORE0 GDSCR (undocumented)
> 0x1044 = MMSS_VIDEO_SUBCORE1 GDSCR (undocumented)
> 0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
> 0x104c = MMSS_VIDEO_SUBCORE1_CBCR
> 
> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---

[...]


>   static struct gdsc video_top_gdsc = {
>   	.gdscr = 0x1024,
> +	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
> +	.cxc_count = 3,

Marc, have you verified all three are necessary for stuff to work?

I'd expect 0x1028/venus core to be absolutely necessary fwiw

Konrad

Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Marc Gonzalez]

On 15/04/2024 21:56, Konrad Dybcio wrote:

> On 4/10/24 13:13, Marc Gonzalez wrote:
>
>> Video decoder (venus) was broken on msm8998.
>>
>> PH found crude work-around:
>> Drop venus_sys_set_power_control() call.
>>
>> Bryan suggested proper fix:
>> Set required register offsets in venus GDSC structs.
>> Set HW_CTRL flag.
>>
>> GDSC = Globally Distributed Switch Controller
>>
>> Use same code as mmcc-msm8996 with:
>> s/venus_gdsc/video_top_gdsc/
>> s/venus_core0_gdsc/video_subcore0_gdsc/
>> s/venus_core1_gdsc/video_subcore1_gdsc/
>>
>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h
>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h
>>
>> 0x1024 = MMSS_VIDEO GDSCR (undocumented)
>> 0x1028 = MMSS_VIDEO_CORE_CBCR
>> 0x1030 = MMSS_VIDEO_AHB_CBCR
>> 0x1034 = MMSS_VIDEO_AXI_CBCR
>> 0x1038 = MMSS_VIDEO_MAXI_CBCR
>> 0x1040 = MMSS_VIDEO_SUBCORE0 GDSCR (undocumented)
>> 0x1044 = MMSS_VIDEO_SUBCORE1 GDSCR (undocumented)
>> 0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
>> 0x104c = MMSS_VIDEO_SUBCORE1_CBCR
>>
>> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
> 
> [...]
> 
> 
>>   static struct gdsc video_top_gdsc = {
>>   	.gdscr = 0x1024,
>> +	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
>> +	.cxc_count = 3,
> 
> Marc, have you verified all three are necessary for stuff to work?
> 
> I'd expect 0x1028/venus core to be absolutely necessary fwiw

Considering the absence of public documentation, these register offsets
mostly boil down to cargo-cult programming.

Thus, using different code on 8996 and 8998 risks provoking the wrath
of the embedded gods. Better, safer to cast the same incantations.

Regards

Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Bryan O'Donoghue]

On 25/04/2024 12:25, Marc Gonzalez wrote:
>> I'd expect 0x1028/venus core to be absolutely necessary fwiw
> Considering the absence of public documentation, these register offsets
> mostly boil down to cargo-cult programming.
> 
> Thus, using different code on 8996 and 8998 risks provoking the wrath
> of the embedded gods. Better, safer to cast the same incantations.

If you are concerned this isn't right, motivated and able to do so, you 
could always build a kernel module for the downstream 8998 kernel - read 
back the new addresses and verify the bits that have been set.

My guess is that something in the boot chain prior to Linux has set the 
bits in the GDSC - lk for 8998, it'd pretty much have to be the case.

---
bod

Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue

[Konrad Dybcio]

On 25.04.2024 1:25 PM, Marc Gonzalez wrote:
> On 15/04/2024 21:56, Konrad Dybcio wrote:
> 
>> On 4/10/24 13:13, Marc Gonzalez wrote:
>>
>>> Video decoder (venus) was broken on msm8998.
>>>
>>> PH found crude work-around:
>>> Drop venus_sys_set_power_control() call.
>>>
>>> Bryan suggested proper fix:
>>> Set required register offsets in venus GDSC structs.
>>> Set HW_CTRL flag.
>>>
>>> GDSC = Globally Distributed Switch Controller
>>>
>>> Use same code as mmcc-msm8996 with:
>>> s/venus_gdsc/video_top_gdsc/
>>> s/venus_core0_gdsc/video_subcore0_gdsc/
>>> s/venus_core1_gdsc/video_subcore1_gdsc/
>>>
>>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h
>>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h
>>>
>>> 0x1024 = MMSS_VIDEO GDSCR (undocumented)
>>> 0x1028 = MMSS_VIDEO_CORE_CBCR
>>> 0x1030 = MMSS_VIDEO_AHB_CBCR
>>> 0x1034 = MMSS_VIDEO_AXI_CBCR
>>> 0x1038 = MMSS_VIDEO_MAXI_CBCR
>>> 0x1040 = MMSS_VIDEO_SUBCORE0 GDSCR (undocumented)
>>> 0x1044 = MMSS_VIDEO_SUBCORE1 GDSCR (undocumented)
>>> 0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
>>> 0x104c = MMSS_VIDEO_SUBCORE1_CBCR
>>>
>>> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>> ---
>>
>> [...]
>>
>>
>>>   static struct gdsc video_top_gdsc = {
>>>   	.gdscr = 0x1024,
>>> +	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
>>> +	.cxc_count = 3,
>>
>> Marc, have you verified all three are necessary for stuff to work?
>>
>> I'd expect 0x1028/venus core to be absolutely necessary fwiw
> 
> Considering the absence of public documentation, these register offsets
> mostly boil down to cargo-cult programming.
> 
> Thus, using different code on 8996 and 8998 risks provoking the wrath
> of the embedded gods. Better, safer to cast the same incantations.

I don't think many maintainers believe in such embedded gods and would rather
see the thing tested and answered considering it's more or less deterministic..

Konrad