| Message ID | 20210325111144.2852594-14-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: cleanup sm8250/sdm845/sc7180 clock drivers | expand |
Hi Dmitry, On 3/25/2021 4:41 PM, Dmitry Baryshkov wrote: > video_pll0_out_even/_odd are not supported neither in the upstream nor > in the downstream kernels, so drop those clock sources. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/clk/qcom/videocc-sdm845.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c > index 5d6a7724a194..7153f044504f 100644 > --- a/drivers/clk/qcom/videocc-sdm845.c > +++ b/drivers/clk/qcom/videocc-sdm845.c > @@ -21,24 +21,18 @@ > enum { > P_BI_TCXO, > P_CORE_BI_PLL_TEST_SE, > - P_VIDEO_PLL0_OUT_EVEN, > P_VIDEO_PLL0_OUT_MAIN, > - P_VIDEO_PLL0_OUT_ODD, > }; > > static const struct parent_map video_cc_parent_map_0[] = { > { P_BI_TCXO, 0 }, > { P_VIDEO_PLL0_OUT_MAIN, 1 }, > - { P_VIDEO_PLL0_OUT_EVEN, 2 }, > - { P_VIDEO_PLL0_OUT_ODD, 3 }, These are supported from the design, please do not remove them. It is just that in SW currently it is not being used. But SW can decide to use them as they want. As said earlier these are defined in the HW plans and thus do not want them to be updated manually to create a mismatch. > { P_CORE_BI_PLL_TEST_SE, 4 }, > }; > > static const char * const video_cc_parent_names_0[] = { > "bi_tcxo", > "video_pll0", > - "video_pll0_out_even", > - "video_pll0_out_odd", > "core_bi_pll_test_se", > }; > > @@ -79,7 +73,7 @@ static struct clk_rcg2 video_cc_venus_clk_src = { > .clkr.hw.init = &(struct clk_init_data){ > .name = "video_cc_venus_clk_src", > .parent_names = video_cc_parent_names_0, > - .num_parents = 5, > + .num_parents = 3, > .flags = CLK_SET_RATE_PARENT, > .ops = &clk_rcg2_shared_ops, > }, > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
Hi Taniya, On Fri, 2 Apr 2021 at 04:23, Taniya Das <tdas@codeaurora.org> wrote: > > Hi Dmitry, > > On 3/25/2021 4:41 PM, Dmitry Baryshkov wrote: > > video_pll0_out_even/_odd are not supported neither in the upstream nor > > in the downstream kernels, so drop those clock sources. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/clk/qcom/videocc-sdm845.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c > > index 5d6a7724a194..7153f044504f 100644 > > --- a/drivers/clk/qcom/videocc-sdm845.c > > +++ b/drivers/clk/qcom/videocc-sdm845.c > > @@ -21,24 +21,18 @@ > > enum { > > P_BI_TCXO, > > P_CORE_BI_PLL_TEST_SE, > > - P_VIDEO_PLL0_OUT_EVEN, > > P_VIDEO_PLL0_OUT_MAIN, > > - P_VIDEO_PLL0_OUT_ODD, > > }; > > > > static const struct parent_map video_cc_parent_map_0[] = { > > { P_BI_TCXO, 0 }, > > { P_VIDEO_PLL0_OUT_MAIN, 1 }, > > - { P_VIDEO_PLL0_OUT_EVEN, 2 }, > > - { P_VIDEO_PLL0_OUT_ODD, 3 }, > > These are supported from the design, please do not remove them. It is > just that in SW currently it is not being used. > But SW can decide to use them as they want. As said earlier these are > defined in the HW plans and thus do not want them to be updated manually > to create a mismatch. I see your point here. I'll drop these patches and refresh the parent_data conversion patches. > > > { P_CORE_BI_PLL_TEST_SE, 4 }, > > }; > > > > static const char * const video_cc_parent_names_0[] = { > > "bi_tcxo", > > "video_pll0", > > - "video_pll0_out_even", > > - "video_pll0_out_odd", > > "core_bi_pll_test_se", > > }; > > > > @@ -79,7 +73,7 @@ static struct clk_rcg2 video_cc_venus_clk_src = { > > .clkr.hw.init = &(struct clk_init_data){ > > .name = "video_cc_venus_clk_src", > > .parent_names = video_cc_parent_names_0, > > - .num_parents = 5, > > + .num_parents = 3, > > .flags = CLK_SET_RATE_PARENT, > > .ops = &clk_rcg2_shared_ops, > > }, > > > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation. > > -- -- With best wishes Dmitry
On 02/04/2021 04:23, Taniya Das wrote: > Hi Dmitry, > > On 3/25/2021 4:41 PM, Dmitry Baryshkov wrote: >> video_pll0_out_even/_odd are not supported neither in the upstream nor >> in the downstream kernels, so drop those clock sources. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/clk/qcom/videocc-sdm845.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/drivers/clk/qcom/videocc-sdm845.c >> b/drivers/clk/qcom/videocc-sdm845.c >> index 5d6a7724a194..7153f044504f 100644 >> --- a/drivers/clk/qcom/videocc-sdm845.c >> +++ b/drivers/clk/qcom/videocc-sdm845.c >> @@ -21,24 +21,18 @@ >> enum { >> P_BI_TCXO, >> P_CORE_BI_PLL_TEST_SE, >> - P_VIDEO_PLL0_OUT_EVEN, >> P_VIDEO_PLL0_OUT_MAIN, >> - P_VIDEO_PLL0_OUT_ODD, >> }; >> static const struct parent_map video_cc_parent_map_0[] = { >> { P_BI_TCXO, 0 }, >> { P_VIDEO_PLL0_OUT_MAIN, 1 }, >> - { P_VIDEO_PLL0_OUT_EVEN, 2 }, >> - { P_VIDEO_PLL0_OUT_ODD, 3 }, > > These are supported from the design, please do not remove them. It is > just that in SW currently it is not being used. > But SW can decide to use them as they want. As said earlier these are > defined in the HW plans and thus do not want them to be updated manually > to create a mismatch. The problem arises during conversion of these drivers to use parent_data instead of parent_names. You see, video_pll0_odd/_even are clocks which should be referenced using .hw (and thus defined inside the videocc driver) as we do for "video_pll0" parent. However there are no clk_hw entities defined for those clocks. For now I'd just use the { .name = video_pll0_out_odd" } entry for those clocks, however I still think this is not correct. > >> { P_CORE_BI_PLL_TEST_SE, 4 }, >> }; >> static const char * const video_cc_parent_names_0[] = { >> "bi_tcxo", >> "video_pll0", >> - "video_pll0_out_even", >> - "video_pll0_out_odd", >> "core_bi_pll_test_se", >> }; >> @@ -79,7 +73,7 @@ static struct clk_rcg2 video_cc_venus_clk_src = { >> .clkr.hw.init = &(struct clk_init_data){ >> .name = "video_cc_venus_clk_src", >> .parent_names = video_cc_parent_names_0, >> - .num_parents = 5, >> + .num_parents = 3, >> .flags = CLK_SET_RATE_PARENT, >> .ops = &clk_rcg2_shared_ops, >> }, >> > -- With best wishes Dmitry
Quoting Dmitry Baryshkov (2021-04-01 18:49:37) > On 02/04/2021 04:23, Taniya Das wrote: > > Hi Dmitry, > > > > On 3/25/2021 4:41 PM, Dmitry Baryshkov wrote: > >> video_pll0_out_even/_odd are not supported neither in the upstream nor > >> in the downstream kernels, so drop those clock sources. > >> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> --- > >> drivers/clk/qcom/videocc-sdm845.c | 8 +------- > >> 1 file changed, 1 insertion(+), 7 deletions(-) > >> > >> diff --git a/drivers/clk/qcom/videocc-sdm845.c > >> b/drivers/clk/qcom/videocc-sdm845.c > >> index 5d6a7724a194..7153f044504f 100644 > >> --- a/drivers/clk/qcom/videocc-sdm845.c > >> +++ b/drivers/clk/qcom/videocc-sdm845.c > >> @@ -21,24 +21,18 @@ > >> enum { > >> P_BI_TCXO, > >> P_CORE_BI_PLL_TEST_SE, > >> - P_VIDEO_PLL0_OUT_EVEN, > >> P_VIDEO_PLL0_OUT_MAIN, > >> - P_VIDEO_PLL0_OUT_ODD, > >> }; > >> static const struct parent_map video_cc_parent_map_0[] = { > >> { P_BI_TCXO, 0 }, > >> { P_VIDEO_PLL0_OUT_MAIN, 1 }, > >> - { P_VIDEO_PLL0_OUT_EVEN, 2 }, > >> - { P_VIDEO_PLL0_OUT_ODD, 3 }, > > > > These are supported from the design, please do not remove them. It is > > just that in SW currently it is not being used. > > But SW can decide to use them as they want. As said earlier these are > > defined in the HW plans and thus do not want them to be updated manually > > to create a mismatch. > > The problem arises during conversion of these drivers to use parent_data > instead of parent_names. You see, video_pll0_odd/_even are clocks which > should be referenced using .hw (and thus defined inside the videocc > driver) as we do for "video_pll0" parent. However there are no clk_hw > entities defined for those clocks. For now I'd just use the { .name = > video_pll0_out_odd" } entry for those clocks, however I still think this > is not correct. > Yes we shouldn't be adding .name anymore. Can we add the video_pll0_out_{even,odd} clks? Or if they're not used then can we remove them from the parent_data and leave some sort of comment indicating that they may be there? > > > >> { P_CORE_BI_PLL_TEST_SE, 4 }, > >> }; > >> static const char * const video_cc_parent_names_0[] = { > >> "bi_tcxo", > >> "video_pll0", > >> - "video_pll0_out_even", > >> - "video_pll0_out_odd", > >> "core_bi_pll_test_se", Looks like in this case it would be OK because the array would be length 2 instead of length 5. > >> };
On 02/04/2021 20:34, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2021-04-01 18:49:37) >> On 02/04/2021 04:23, Taniya Das wrote: >>> Hi Dmitry, >>> >>> On 3/25/2021 4:41 PM, Dmitry Baryshkov wrote: >>>> video_pll0_out_even/_odd are not supported neither in the upstream nor >>>> in the downstream kernels, so drop those clock sources. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/clk/qcom/videocc-sdm845.c | 8 +------- >>>> 1 file changed, 1 insertion(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/clk/qcom/videocc-sdm845.c >>>> b/drivers/clk/qcom/videocc-sdm845.c >>>> index 5d6a7724a194..7153f044504f 100644 >>>> --- a/drivers/clk/qcom/videocc-sdm845.c >>>> +++ b/drivers/clk/qcom/videocc-sdm845.c >>>> @@ -21,24 +21,18 @@ >>>> enum { >>>> P_BI_TCXO, >>>> P_CORE_BI_PLL_TEST_SE, >>>> - P_VIDEO_PLL0_OUT_EVEN, >>>> P_VIDEO_PLL0_OUT_MAIN, >>>> - P_VIDEO_PLL0_OUT_ODD, >>>> }; >>>> static const struct parent_map video_cc_parent_map_0[] = { >>>> { P_BI_TCXO, 0 }, >>>> { P_VIDEO_PLL0_OUT_MAIN, 1 }, >>>> - { P_VIDEO_PLL0_OUT_EVEN, 2 }, >>>> - { P_VIDEO_PLL0_OUT_ODD, 3 }, >>> >>> These are supported from the design, please do not remove them. It is >>> just that in SW currently it is not being used. >>> But SW can decide to use them as they want. As said earlier these are >>> defined in the HW plans and thus do not want them to be updated manually >>> to create a mismatch. >> >> The problem arises during conversion of these drivers to use parent_data >> instead of parent_names. You see, video_pll0_odd/_even are clocks which >> should be referenced using .hw (and thus defined inside the videocc >> driver) as we do for "video_pll0" parent. However there are no clk_hw >> entities defined for those clocks. For now I'd just use the { .name = >> video_pll0_out_odd" } entry for those clocks, however I still think this >> is not correct. >> > > Yes we shouldn't be adding .name anymore. Can we add the > video_pll0_out_{even,odd} clks? Or if they're not used then can we > remove them from the parent_data and leave some sort of comment > indicating that they may be there? Downstream kernel provides no information regarding these clocks. I'll just leave them commented out (but inplace). > >>> >>>> { P_CORE_BI_PLL_TEST_SE, 4 }, >>>> }; >>>> static const char * const video_cc_parent_names_0[] = { >>>> "bi_tcxo", >>>> "video_pll0", >>>> - "video_pll0_out_even", >>>> - "video_pll0_out_odd", >>>> "core_bi_pll_test_se", > > Looks like in this case it would be OK because the array would be length > 2 instead of length 5. > >>>> }; -- With best wishes Dmitry
diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c index 5d6a7724a194..7153f044504f 100644 --- a/drivers/clk/qcom/videocc-sdm845.c +++ b/drivers/clk/qcom/videocc-sdm845.c @@ -21,24 +21,18 @@ enum { P_BI_TCXO, P_CORE_BI_PLL_TEST_SE, - P_VIDEO_PLL0_OUT_EVEN, P_VIDEO_PLL0_OUT_MAIN, - P_VIDEO_PLL0_OUT_ODD, }; static const struct parent_map video_cc_parent_map_0[] = { { P_BI_TCXO, 0 }, { P_VIDEO_PLL0_OUT_MAIN, 1 }, - { P_VIDEO_PLL0_OUT_EVEN, 2 }, - { P_VIDEO_PLL0_OUT_ODD, 3 }, { P_CORE_BI_PLL_TEST_SE, 4 }, }; static const char * const video_cc_parent_names_0[] = { "bi_tcxo", "video_pll0", - "video_pll0_out_even", - "video_pll0_out_odd", "core_bi_pll_test_se", }; @@ -79,7 +73,7 @@ static struct clk_rcg2 video_cc_venus_clk_src = { .clkr.hw.init = &(struct clk_init_data){ .name = "video_cc_venus_clk_src", .parent_names = video_cc_parent_names_0, - .num_parents = 5, + .num_parents = 3, .flags = CLK_SET_RATE_PARENT, .ops = &clk_rcg2_shared_ops, },
video_pll0_out_even/_odd are not supported neither in the upstream nor in the downstream kernels, so drop those clock sources. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/clk/qcom/videocc-sdm845.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) -- 2.30.2