| Message ID | 20230506-msm8226-oxilicx-v1-1-52e34b94ff22@z3ntu.xyz |
|---|---|
| State | Accepted |
| Commit | e21e74c6e251152dfc45efdf2279db2778d51223 |
| Headers | show |
| Series | clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226 | expand |
On 6.05.2023 23:20, Luca Weiss wrote: > On msm8226 we also have OXILICX_GDSC but we need a slighly different > config, with a .cxcs defined for clock but with no parent. Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and to achieve that, we sometimes define it to be the GX's (also implicitly known as "oxili-non-CX" in before-a6xx-times) parent.. Roughly speaking CX powers the "GPU hardware owned by the broader SoC that may not need the GPU core clock to be up" and GX powers the "GPU hardware owned strictly by the GPU that needs at least some GPU clocks to be enabled" Maybe 8974 simply has a bug in the driver that would do the reverse? Could you (and perhaps Dmitry on his shiny new 13yo board) test that theory, preferably on both SoCs? --- a/drivers/clk/qcom/mmcc-msm8974.c +++ b/drivers/clk/qcom/mmcc-msm8974.c @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = { .pd = { .name = "oxili", }, + .parent = &oxili_gdsc.pd, .pwrsts = PWRSTS_OFF_ON, }; @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = { .pd = { .name = "oxilicx", }, - .parent = &oxili_gdsc.pd, .pwrsts = PWRSTS_OFF_ON, }; Konrad > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > --- > drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c > index 4273fce9a4a4..39ee3953567c 100644 > --- a/drivers/clk/qcom/mmcc-msm8974.c > +++ b/drivers/clk/qcom/mmcc-msm8974.c > @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = { > .pwrsts = PWRSTS_OFF_ON, > }; > > +static struct gdsc oxilicx_gdsc_msm8226 = { > + .gdscr = 0x4034, > + .cxcs = (unsigned int []){ 0x4028 }, > + .cxc_count = 1, > + .pd = { > + .name = "oxilicx", > + }, > + .pwrsts = PWRSTS_OFF_ON, > +}; > + > static struct clk_regmap *mmcc_msm8226_clocks[] = { > [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr, > [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr, > @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = { > [MDSS_GDSC] = &mdss_gdsc, > [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc, > [CAMSS_VFE_GDSC] = &camss_vfe_gdsc, > + [OXILICX_GDSC] = &oxilicx_gdsc_msm8226, > }; > > static const struct regmap_config mmcc_msm8226_regmap_config = { > > --- > base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889 > change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d > > Best regards,
On 9.05.2023 18:57, Luca Weiss wrote: > On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote: >> On 8.05.2023 13:32, Dmitry Baryshkov wrote: >>> On 08/05/2023 10:23, Konrad Dybcio wrote: >>>> On 6.05.2023 23:20, Luca Weiss wrote: >>>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different >>>>> config, with a .cxcs defined for clock but with no parent. >>>> >>>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and >>>> to achieve that, we sometimes define it to be the GX's (also >>>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent.. >>>> >>>> Roughly speaking CX powers the "GPU hardware owned by the broader >>>> SoC that may not need the GPU core clock to be up" and GX powers >>>> the "GPU hardware owned strictly by the GPU that needs at least some >>>> GPU clocks to be enabled" >>>> >>>> Maybe 8974 simply has a bug in the driver that would do the reverse? >>>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that >>>> theory, preferably on both SoCs? >>>> >>>> --- a/drivers/clk/qcom/mmcc-msm8974.c >>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c >>>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = { >>>> .pd = { >>>> .name = "oxili", >>>> }, >>>> + .parent = &oxili_gdsc.pd, >>>> .pwrsts = PWRSTS_OFF_ON, >>>> }; >>> >>> Are you declaring oxili_gdsc to be a parent of itself? >> >> lol.. nice catch of course this line should have been >> >> + .parent = &oxilicx_gdsc.pd, >> >> and the definitions would need to be swapped > > The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226 dts. > > Only in downstream msm8974.dtsi this gdsc gets "parent-supply = > <&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc parent > doesn't even seem to be described there. > > Should I still try? No, nevermind, this SoC is cut down more than I had initially thought. Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> with a minor nit: oxilicx -> oxili_cx Konrad > >> >> Konrad >> >>>> @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = { >>>> .pd = { >>>> .name = "oxilicx", >>>> }, >>>> - .parent = &oxili_gdsc.pd, >>>> .pwrsts = PWRSTS_OFF_ON, >>>> }; >>>> >>>> Konrad >>>> >>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz> >>>>> --- >>>>> drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c >>>>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c >>>>> 100644 >>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c >>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c >>>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = { >>>>> .pwrsts = PWRSTS_OFF_ON, >>>>> }; >>>>> +static struct gdsc oxilicx_gdsc_msm8226 = { >>>>> + .gdscr = 0x4034, >>>>> + .cxcs = (unsigned int []){ 0x4028 }, >>>>> + .cxc_count = 1, >>>>> + .pd = { >>>>> + .name = "oxilicx", >>>>> + }, >>>>> + .pwrsts = PWRSTS_OFF_ON, >>>>> +}; >>>>> + >>>>> static struct clk_regmap *mmcc_msm8226_clocks[] = { >>>>> [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr, >>>>> [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr, >>>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = { >>>>> [MDSS_GDSC] = &mdss_gdsc, >>>>> [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc, >>>>> [CAMSS_VFE_GDSC] = &camss_vfe_gdsc, >>>>> + [OXILICX_GDSC] = &oxilicx_gdsc_msm8226, >>>>> }; >>>>> static const struct regmap_config mmcc_msm8226_regmap_config = { >>>>> >>>>> --- >>>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889 >>>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d >>>>> >>>>> Best regards, > > > >
On Dienstag, 16. Mai 2023 02:15:06 CEST Konrad Dybcio wrote: > On 9.05.2023 18:57, Luca Weiss wrote: > > On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote: > >> On 8.05.2023 13:32, Dmitry Baryshkov wrote: > >>> On 08/05/2023 10:23, Konrad Dybcio wrote: > >>>> On 6.05.2023 23:20, Luca Weiss wrote: > >>>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different > >>>>> config, with a .cxcs defined for clock but with no parent. > >>>> > >>>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and > >>>> to achieve that, we sometimes define it to be the GX's (also > >>>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent.. > >>>> > >>>> Roughly speaking CX powers the "GPU hardware owned by the broader > >>>> SoC that may not need the GPU core clock to be up" and GX powers > >>>> the "GPU hardware owned strictly by the GPU that needs at least some > >>>> GPU clocks to be enabled" > >>>> > >>>> Maybe 8974 simply has a bug in the driver that would do the reverse? > >>>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that > >>>> theory, preferably on both SoCs? > >>>> > >>>> --- a/drivers/clk/qcom/mmcc-msm8974.c > >>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c > >>>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = { > >>>> > >>>> .pd = { > >>>> > >>>> .name = "oxili", > >>>> > >>>> }, > >>>> > >>>> + .parent = &oxili_gdsc.pd, > >>>> > >>>> .pwrsts = PWRSTS_OFF_ON, > >>>> > >>>> }; > >>> > >>> Are you declaring oxili_gdsc to be a parent of itself? > >> > >> lol.. nice catch of course this line should have been > >> > >> + .parent = &oxilicx_gdsc.pd, > >> > >> and the definitions would need to be swapped > > > > The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226 > > dts. > > > > Only in downstream msm8974.dtsi this gdsc gets "parent-supply = > > <&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc > > parent doesn't even seem to be described there. > > > > Should I still try? > > No, nevermind, this SoC is cut down more than I had initially thought. > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > with a minor nit: oxilicx -> oxili_cx Hi Konrad, where do you want this changed? Just the .name field? But even that one is now matching the other oxilicx variant. And there's also gdscs like oxilicx_ahb_clk. Let me know. Regards Luca > > Konrad > > >> Konrad > >> > >>>> @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = { > >>>> > >>>> .pd = { > >>>> > >>>> .name = "oxilicx", > >>>> > >>>> }, > >>>> > >>>> - .parent = &oxili_gdsc.pd, > >>>> > >>>> .pwrsts = PWRSTS_OFF_ON, > >>>> > >>>> }; > >>>> > >>>> Konrad > >>>> > >>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > >>>>> --- > >>>>> > >>>>> drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++ > >>>>> 1 file changed, 11 insertions(+) > >>>>> > >>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c > >>>>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c > >>>>> 100644 > >>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c > >>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c > >>>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = { > >>>>> > >>>>> .pwrsts = PWRSTS_OFF_ON, > >>>>> > >>>>> }; > >>>>> +static struct gdsc oxilicx_gdsc_msm8226 = { > >>>>> > >>>>> + .gdscr = 0x4034, > >>>>> + .cxcs = (unsigned int []){ 0x4028 }, > >>>>> + .cxc_count = 1, > >>>>> + .pd = { > >>>>> + .name = "oxilicx", > >>>>> + }, > >>>>> + .pwrsts = PWRSTS_OFF_ON, > >>>>> +}; > >>>>> + > >>>>> > >>>>> static struct clk_regmap *mmcc_msm8226_clocks[] = { > >>>>> > >>>>> [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr, > >>>>> [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr, > >>>>> > >>>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = { > >>>>> > >>>>> [MDSS_GDSC] = &mdss_gdsc, > >>>>> [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc, > >>>>> [CAMSS_VFE_GDSC] = &camss_vfe_gdsc, > >>>>> > >>>>> + [OXILICX_GDSC] = &oxilicx_gdsc_msm8226, > >>>>> > >>>>> }; > >>>>> > >>>>> static const struct regmap_config mmcc_msm8226_regmap_config = { > >>>>> > >>>>> --- > >>>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889 > >>>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d > >>>>> > >>>>> Best regards,
On 23.05.2023 22:44, Luca Weiss wrote: > On Dienstag, 16. Mai 2023 02:15:06 CEST Konrad Dybcio wrote: >> On 9.05.2023 18:57, Luca Weiss wrote: >>> On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote: >>>> On 8.05.2023 13:32, Dmitry Baryshkov wrote: >>>>> On 08/05/2023 10:23, Konrad Dybcio wrote: >>>>>> On 6.05.2023 23:20, Luca Weiss wrote: >>>>>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different >>>>>>> config, with a .cxcs defined for clock but with no parent. >>>>>> >>>>>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and >>>>>> to achieve that, we sometimes define it to be the GX's (also >>>>>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent.. >>>>>> >>>>>> Roughly speaking CX powers the "GPU hardware owned by the broader >>>>>> SoC that may not need the GPU core clock to be up" and GX powers >>>>>> the "GPU hardware owned strictly by the GPU that needs at least some >>>>>> GPU clocks to be enabled" >>>>>> >>>>>> Maybe 8974 simply has a bug in the driver that would do the reverse? >>>>>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that >>>>>> theory, preferably on both SoCs? >>>>>> >>>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c >>>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c >>>>>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = { >>>>>> >>>>>> .pd = { >>>>>> >>>>>> .name = "oxili", >>>>>> >>>>>> }, >>>>>> >>>>>> + .parent = &oxili_gdsc.pd, >>>>>> >>>>>> .pwrsts = PWRSTS_OFF_ON, >>>>>> >>>>>> }; >>>>> >>>>> Are you declaring oxili_gdsc to be a parent of itself? >>>> >>>> lol.. nice catch of course this line should have been >>>> >>>> + .parent = &oxilicx_gdsc.pd, >>>> >>>> and the definitions would need to be swapped >>> >>> The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226 >>> dts. >>> >>> Only in downstream msm8974.dtsi this gdsc gets "parent-supply = >>> <&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc >>> parent doesn't even seem to be described there. >>> >>> Should I still try? >> >> No, nevermind, this SoC is cut down more than I had initially thought. >> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> >> with a minor nit: oxilicx -> oxili_cx > > Hi Konrad, > > where do you want this changed? Just the .name field? Yes and maybe the struct name. We shouldn't be messing with bindings since it's cosmetic. But even that one is now > matching the other oxilicx variant. And there's also gdscs like > oxilicx_ahb_clk. oxilicx literally means "the CX side of OXILI", or translating from Qualcommish to English "GPU registers accessible from the AP, not necessarily the GPU itself" Konrad > > Let me know. > > Regards > Luca > >> >> Konrad >> >>>> Konrad >>>> >>>>>> @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = { >>>>>> >>>>>> .pd = { >>>>>> >>>>>> .name = "oxilicx", >>>>>> >>>>>> }, >>>>>> >>>>>> - .parent = &oxili_gdsc.pd, >>>>>> >>>>>> .pwrsts = PWRSTS_OFF_ON, >>>>>> >>>>>> }; >>>>>> >>>>>> Konrad >>>>>> >>>>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz> >>>>>>> --- >>>>>>> >>>>>>> drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++ >>>>>>> 1 file changed, 11 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c >>>>>>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c >>>>>>> 100644 >>>>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c >>>>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c >>>>>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = { >>>>>>> >>>>>>> .pwrsts = PWRSTS_OFF_ON, >>>>>>> >>>>>>> }; >>>>>>> +static struct gdsc oxilicx_gdsc_msm8226 = { >>>>>>> >>>>>>> + .gdscr = 0x4034, >>>>>>> + .cxcs = (unsigned int []){ 0x4028 }, >>>>>>> + .cxc_count = 1, >>>>>>> + .pd = { >>>>>>> + .name = "oxilicx", >>>>>>> + }, >>>>>>> + .pwrsts = PWRSTS_OFF_ON, >>>>>>> +}; >>>>>>> + >>>>>>> >>>>>>> static struct clk_regmap *mmcc_msm8226_clocks[] = { >>>>>>> >>>>>>> [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr, >>>>>>> [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr, >>>>>>> >>>>>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = { >>>>>>> >>>>>>> [MDSS_GDSC] = &mdss_gdsc, >>>>>>> [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc, >>>>>>> [CAMSS_VFE_GDSC] = &camss_vfe_gdsc, >>>>>>> >>>>>>> + [OXILICX_GDSC] = &oxilicx_gdsc_msm8226, >>>>>>> >>>>>>> }; >>>>>>> >>>>>>> static const struct regmap_config mmcc_msm8226_regmap_config = { >>>>>>> >>>>>>> --- >>>>>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889 >>>>>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d >>>>>>> >>>>>>> Best regards, > > > >
diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c 100644 --- a/drivers/clk/qcom/mmcc-msm8974.c +++ b/drivers/clk/qcom/mmcc-msm8974.c @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = { .pwrsts = PWRSTS_OFF_ON, }; +static struct gdsc oxilicx_gdsc_msm8226 = { + .gdscr = 0x4034, + .cxcs = (unsigned int []){ 0x4028 }, + .cxc_count = 1, + .pd = { + .name = "oxilicx", + }, + .pwrsts = PWRSTS_OFF_ON, +}; + static struct clk_regmap *mmcc_msm8226_clocks[] = { [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr, [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr, @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = { [MDSS_GDSC] = &mdss_gdsc, [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc, [CAMSS_VFE_GDSC] = &camss_vfe_gdsc, + [OXILICX_GDSC] = &oxilicx_gdsc_msm8226, }; static const struct regmap_config mmcc_msm8226_regmap_config = {
On msm8226 we also have OXILICX_GDSC but we need a slighly different config, with a .cxcs defined for clock but with no parent. Signed-off-by: Luca Weiss <luca@z3ntu.xyz> --- drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++ 1 file changed, 11 insertions(+) --- base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889 change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d Best regards,