| Message ID | c325691e-1cbe-4589-87fc-b67a41e93294@freebox.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] clk: qcom: mmcc-msm8998: fix venus clock issue | expand |
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 >
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.
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
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
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
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
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 = {