* Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue
2024-04-10 11:13 [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue Marc Gonzalez
@ 2024-04-11 3:07 ` Bjorn Andersson
2024-04-12 14:49 ` Jeffrey Hugo
2024-04-15 19:56 ` Konrad Dybcio
2 siblings, 0 replies; 7+ messages in thread
From: Bjorn Andersson @ 2024-04-11 3:07 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Konrad Dybcio, Michael Turquette, Stephen Boyd, MSM, linux-clk,
Dmitry Baryshkov, Bryan O Donoghue, Vikash Garodia, Jeffrey Hugo,
Douglas Anderson, Pierre-Hugues Husson, Arnaud Vrac
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue
2024-04-10 11:13 [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue Marc Gonzalez
2024-04-11 3:07 ` Bjorn Andersson
@ 2024-04-12 14:49 ` Jeffrey Hugo
2024-04-15 19:56 ` Konrad Dybcio
2 siblings, 0 replies; 7+ messages in thread
From: Jeffrey Hugo @ 2024-04-12 14:49 UTC (permalink / raw)
To: Marc Gonzalez, Bjorn Andersson, Konrad Dybcio, Michael Turquette,
Stephen Boyd
Cc: MSM, linux-clk, Dmitry Baryshkov, Bryan O Donoghue,
Vikash Garodia, Douglas Anderson, Pierre-Hugues Husson,
Arnaud Vrac
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue
2024-04-10 11:13 [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue Marc Gonzalez
2024-04-11 3:07 ` Bjorn Andersson
2024-04-12 14:49 ` Jeffrey Hugo
@ 2024-04-15 19:56 ` Konrad Dybcio
2024-04-25 11:25 ` Marc Gonzalez
2 siblings, 1 reply; 7+ messages in thread
From: Konrad Dybcio @ 2024-04-15 19:56 UTC (permalink / raw)
To: Marc Gonzalez, Bjorn Andersson, Michael Turquette, Stephen Boyd
Cc: MSM, linux-clk, Dmitry Baryshkov, Bryan O Donoghue,
Vikash Garodia, Jeffrey Hugo, Douglas Anderson,
Pierre-Hugues Husson, Arnaud Vrac
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue
2024-04-15 19:56 ` Konrad Dybcio
@ 2024-04-25 11:25 ` Marc Gonzalez
2024-04-25 22:31 ` Bryan O'Donoghue
2024-04-27 0:47 ` Konrad Dybcio
0 siblings, 2 replies; 7+ messages in thread
From: Marc Gonzalez @ 2024-04-25 11:25 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Michael Turquette, Stephen Boyd
Cc: MSM, linux-clk, Dmitry Baryshkov, Bryan O Donoghue,
Vikash Garodia, Jeffrey Hugo, Douglas Anderson,
Pierre-Hugues Husson, Arnaud Vrac
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue
2024-04-25 11:25 ` Marc Gonzalez
@ 2024-04-25 22:31 ` Bryan O'Donoghue
2024-04-27 0:47 ` Konrad Dybcio
1 sibling, 0 replies; 7+ messages in thread
From: Bryan O'Donoghue @ 2024-04-25 22:31 UTC (permalink / raw)
To: Marc Gonzalez, Konrad Dybcio, Bjorn Andersson, Michael Turquette,
Stephen Boyd
Cc: MSM, linux-clk, Dmitry Baryshkov, Vikash Garodia, Jeffrey Hugo,
Douglas Anderson, Pierre-Hugues Husson, Arnaud Vrac
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] clk: qcom: mmcc-msm8998: fix venus clock issue
2024-04-25 11:25 ` Marc Gonzalez
2024-04-25 22:31 ` Bryan O'Donoghue
@ 2024-04-27 0:47 ` Konrad Dybcio
1 sibling, 0 replies; 7+ messages in thread
From: Konrad Dybcio @ 2024-04-27 0:47 UTC (permalink / raw)
To: Marc Gonzalez, Bjorn Andersson, Michael Turquette, Stephen Boyd
Cc: MSM, linux-clk, Dmitry Baryshkov, Bryan O Donoghue,
Vikash Garodia, Jeffrey Hugo, Douglas Anderson,
Pierre-Hugues Husson, Arnaud Vrac
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
^ permalink raw reply [flat|nested] 7+ messages in thread