diff mbox series

[v1,13/15] clk: qcom: videocc-sdm845: remove unsupported clock sources

Message ID 20210325111144.2852594-14-dmitry.baryshkov@linaro.org (mailing list archive)
State Superseded
Headers show
Series [v1,01/15] clk: qcom: dispcc-sc7180: drop unused enum entries | expand

Commit Message

Dmitry Baryshkov March 25, 2021, 11:11 a.m. UTC
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(-)

Comments

Taniya Das April 2, 2021, 1:23 a.m. UTC | #1
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,
>   	},
>
Dmitry Baryshkov April 2, 2021, 1:32 a.m. UTC | #2
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.
>
> --
Dmitry Baryshkov April 2, 2021, 1:49 a.m. UTC | #3
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,
>>       },
>>
>
Stephen Boyd April 2, 2021, 5:34 p.m. UTC | #4
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.

> >>   };
Dmitry Baryshkov April 2, 2021, 8:01 p.m. UTC | #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.
> 
>>>>    };
diff mbox series

Patch

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,
 	},