Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-24 Thread Stephen Boyd
Quoting spa...@codeaurora.org (2018-07-13 01:25:49)
> On 2018-07-13 01:11, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-07-12 10:21:33)
> >> ++ Display driver team,
> >> 
> >> On 7/9/2018 8:36 PM, Stephen Boyd wrote:
> >> > Quoting Taniya Das (2018-07-09 02:34:07)
> >> >>
> >> >>
> >> >> On 7/9/2018 1:07 PM, Stephen Boyd wrote:
> >> >>> Quoting Taniya Das (2018-07-09 00:07:21)
> >> 
> >> 
> >>  On 7/9/2018 11:46 AM, Stephen Boyd wrote:
> >> >>
> >> >> > Why is the nocache flag needed? Applies to all clks in this 
> >> >> file.
> >> >> >
> >> >>
> >> >> This flag is required for all RCGs whose PLLs are controlled 
> >> >> outside the
> >> >> clock controller. The display code would require the recalculated 
> >> >> rate
> >> >> always.
> >> >
> >> > Right. Why is the PLL controlled outside of the clock controller? The
> >> > rate should propagate upward to the PLL from here, so who's going
> >> > outside of that?
> >> >
> >>  The DSI0/1 PLL are not part of the display clock controller, but in 
> >>  the
> >>  display subsystem which are managed by the DRM drivers. When DRM 
> >>  drivers
> >>  query for the rate clock driver should always return the non cached 
> >>  rates.
> >> >>>
> >> >>> Why? Is the DSI PLL changing rate all the time, randomly, without going
> >> >>> through the clk APIs to do so?
> >> >>>
> >> >>
> >> >> Hmm, I am afraid I do not have an answer for this, but this was the
> >> >> requirement to always return the non cached rates from the clock driver.
> >> >>
> >> >
> >> > Ok. Who knows about this requirement? Can we add someone from the
> >> > display driver to understand more?
> >> >
> >> As per my discussions offline with the display teams,
> >> 
> >> There is a use-case where the clock framework is unaware of the PLL 
> >> VCO
> >> frequency change and thus the drivers would query to get the actual HW
> >> frequency rather than the cached one.
> >> 
> >> Do you think keeping these flags would have any impact other than 
> >> always
> >> getting the non-cached rates?
> >> 
> > 
> > The flag will make it so clk_get_rate() works in spite of something
> > changing the frequency behind the framework's back, but I want to
> > understand what and why it's changing without framework involvement. We
> > shouldn't need the flag here, because this flag is typically for clks
> > that are controlled by some other entity that the kernel doesn't have
> > control over. In this case, it seems like we have full control of the
> > clk tree for the display PLL down to this clk, so it should be 
> > perfectly
> > fine to not have this flag. The presence of the flag means that the
> > display driver is doing something wrong.
> 
> These clocks are sourced from DSI PLL. In dsi command mode there is an 
> idle use case when there
> is no activity on display, we switch of the source DSI PLL to save power 
> by calling clk_disable_unprepare().
> In this scenario if some client queries the clk_get_rate(), then if 
> NO_CACHE flag is used the clk
> driver will return the cached rate which is some non-zero value set when 
> we last called clk_set_rate(),
> before enabling the clock, whereas actually in HW this clk is disabled. 

If the clk is disabled in hardware it doesn't mean clk_get_rate() should
return 0 Hz. The frequency of the clk is set to something, so we should
return what that frequency is by reading the hardware.

> So we used the NO_CACHE flag
> for the call to land in clk_recalc_rate and we return zero if the PLL is 
> disabled.

This is super wrong. If the PLL is disabled it still has some frequency
configured.



Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-15 Thread Taniya Das

Hello Stephen,

On 7/13/2018 1:55 PM, spa...@codeaurora.org wrote:

On 2018-07-13 01:11, Stephen Boyd wrote:

Quoting Taniya Das (2018-07-12 10:21:33)

++ Display driver team,

On 7/9/2018 8:36 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-09 02:34:07)
>>
>>
>> On 7/9/2018 1:07 PM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2018-07-09 00:07:21)


 On 7/9/2018 11:46 AM, Stephen Boyd wrote:
>>
>> > Why is the nocache flag needed? Applies to all clks in 
this file.

>> >
>>
>> This flag is required for all RCGs whose PLLs are controlled 
outside the
>> clock controller. The display code would require the 
recalculated rate

>> always.
>
> Right. Why is the PLL controlled outside of the clock 
controller? The

> rate should propagate upward to the PLL from here, so who's going
> outside of that?
>
 The DSI0/1 PLL are not part of the display clock controller, but 
in the
 display subsystem which are managed by the DRM drivers. When DRM 
drivers
 query for the rate clock driver should always return the non 
cached rates.

>>>
>>> Why? Is the DSI PLL changing rate all the time, randomly, without 
going

>>> through the clk APIs to do so?
>>>
>>
>> Hmm, I am afraid I do not have an answer for this, but this was the
>> requirement to always return the non cached rates from the clock 
driver.

>>
>
> Ok. Who knows about this requirement? Can we add someone from the
> display driver to understand more?
>
As per my discussions offline with the display teams,

There is a use-case where the clock framework is unaware of the PLL VCO
frequency change and thus the drivers would query to get the actual HW
frequency rather than the cached one.

Do you think keeping these flags would have any impact other than always
getting the non-cached rates?



The flag will make it so clk_get_rate() works in spite of something
changing the frequency behind the framework's back, but I want to
understand what and why it's changing without framework involvement. We
shouldn't need the flag here, because this flag is typically for clks
that are controlled by some other entity that the kernel doesn't have
control over. In this case, it seems like we have full control of the
clk tree for the display PLL down to this clk, so it should be perfectly
fine to not have this flag. The presence of the flag means that the
display driver is doing something wrong.


These clocks are sourced from DSI PLL. In dsi command mode there is an 
idle use case when there
is no activity on display, we switch of the source DSI PLL to save power 
by calling clk_disable_unprepare().
In this scenario if some client queries the clk_get_rate(), then if 
NO_CACHE flag is used the clk
driver will return the cached rate which is some non-zero value set when 
we last called clk_set_rate(),
before enabling the clock, whereas actually in HW this clk is disabled. 
So we used the NO_CACHE flag
for the call to land in clk_recalc_rate and we return zero if the PLL is 
disabled.
I remember some customer had scripts that runs through all the clocks 
during idle screen scenario
and call clk_get_rate(), where they complained display clk_get_rate 
returns none zero value.





Do you think the clock driver needs any update for flag for the next series?


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--


Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-13 Thread spanda

On 2018-07-13 01:11, Stephen Boyd wrote:

Quoting Taniya Das (2018-07-12 10:21:33)

++ Display driver team,

On 7/9/2018 8:36 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-07-09 02:34:07)
>>
>>
>> On 7/9/2018 1:07 PM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2018-07-09 00:07:21)


 On 7/9/2018 11:46 AM, Stephen Boyd wrote:
>>
>> > Why is the nocache flag needed? Applies to all clks in this file.
>> >
>>
>> This flag is required for all RCGs whose PLLs are controlled outside the
>> clock controller. The display code would require the recalculated rate
>> always.
>
> Right. Why is the PLL controlled outside of the clock controller? The
> rate should propagate upward to the PLL from here, so who's going
> outside of that?
>
 The DSI0/1 PLL are not part of the display clock controller, but in the
 display subsystem which are managed by the DRM drivers. When DRM drivers
 query for the rate clock driver should always return the non cached rates.
>>>
>>> Why? Is the DSI PLL changing rate all the time, randomly, without going
>>> through the clk APIs to do so?
>>>
>>
>> Hmm, I am afraid I do not have an answer for this, but this was the
>> requirement to always return the non cached rates from the clock driver.
>>
>
> Ok. Who knows about this requirement? Can we add someone from the
> display driver to understand more?
>
As per my discussions offline with the display teams,

There is a use-case where the clock framework is unaware of the PLL 
VCO

frequency change and thus the drivers would query to get the actual HW
frequency rather than the cached one.

Do you think keeping these flags would have any impact other than 
always

getting the non-cached rates?



The flag will make it so clk_get_rate() works in spite of something
changing the frequency behind the framework's back, but I want to
understand what and why it's changing without framework involvement. We
shouldn't need the flag here, because this flag is typically for clks
that are controlled by some other entity that the kernel doesn't have
control over. In this case, it seems like we have full control of the
clk tree for the display PLL down to this clk, so it should be 
perfectly

fine to not have this flag. The presence of the flag means that the
display driver is doing something wrong.


These clocks are sourced from DSI PLL. In dsi command mode there is an 
idle use case when there
is no activity on display, we switch of the source DSI PLL to save power 
by calling clk_disable_unprepare().
In this scenario if some client queries the clk_get_rate(), then if 
NO_CACHE flag is used the clk
driver will return the cached rate which is some non-zero value set when 
we last called clk_set_rate(),
before enabling the clock, whereas actually in HW this clk is disabled. 
So we used the NO_CACHE flag
for the call to land in clk_recalc_rate and we return zero if the PLL is 
disabled.
I remember some customer had scripts that runs through all the clocks 
during idle screen scenario
and call clk_get_rate(), where they complained display clk_get_rate 
returns none zero value.





Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-12 Thread Stephen Boyd
Quoting Taniya Das (2018-07-12 10:21:33)
> ++ Display driver team,
> 
> On 7/9/2018 8:36 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-07-09 02:34:07)
> >>
> >>
> >> On 7/9/2018 1:07 PM, Stephen Boyd wrote:
> >>> Quoting Taniya Das (2018-07-09 00:07:21)
> 
> 
>  On 7/9/2018 11:46 AM, Stephen Boyd wrote:
> >>
> >> > Why is the nocache flag needed? Applies to all clks in this file.
> >> >
> >>
> >> This flag is required for all RCGs whose PLLs are controlled outside 
> >> the
> >> clock controller. The display code would require the recalculated rate
> >> always.
> >
> > Right. Why is the PLL controlled outside of the clock controller? The
> > rate should propagate upward to the PLL from here, so who's going
> > outside of that?
> >
>  The DSI0/1 PLL are not part of the display clock controller, but in the
>  display subsystem which are managed by the DRM drivers. When DRM drivers
>  query for the rate clock driver should always return the non cached 
>  rates.
> >>>
> >>> Why? Is the DSI PLL changing rate all the time, randomly, without going
> >>> through the clk APIs to do so?
> >>>
> >>
> >> Hmm, I am afraid I do not have an answer for this, but this was the
> >> requirement to always return the non cached rates from the clock driver.
> >>
> > 
> > Ok. Who knows about this requirement? Can we add someone from the
> > display driver to understand more?
> > 
> As per my discussions offline with the display teams,
> 
> There is a use-case where the clock framework is unaware of the PLL VCO 
> frequency change and thus the drivers would query to get the actual HW 
> frequency rather than the cached one.
> 
> Do you think keeping these flags would have any impact other than always 
> getting the non-cached rates?
> 

The flag will make it so clk_get_rate() works in spite of something
changing the frequency behind the framework's back, but I want to
understand what and why it's changing without framework involvement. We
shouldn't need the flag here, because this flag is typically for clks
that are controlled by some other entity that the kernel doesn't have
control over. In this case, it seems like we have full control of the
clk tree for the display PLL down to this clk, so it should be perfectly
fine to not have this flag. The presence of the flag means that the
display driver is doing something wrong.



Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-12 Thread Taniya Das

++ Display driver team,

On 7/9/2018 8:36 PM, Stephen Boyd wrote:

Quoting Taniya Das (2018-07-09 02:34:07)



On 7/9/2018 1:07 PM, Stephen Boyd wrote:

Quoting Taniya Das (2018-07-09 00:07:21)



On 7/9/2018 11:46 AM, Stephen Boyd wrote:


> Why is the nocache flag needed? Applies to all clks in this file.
>

This flag is required for all RCGs whose PLLs are controlled outside the
clock controller. The display code would require the recalculated rate
always.


Right. Why is the PLL controlled outside of the clock controller? The
rate should propagate upward to the PLL from here, so who's going
outside of that?


The DSI0/1 PLL are not part of the display clock controller, but in the
display subsystem which are managed by the DRM drivers. When DRM drivers
query for the rate clock driver should always return the non cached rates.


Why? Is the DSI PLL changing rate all the time, randomly, without going
through the clk APIs to do so?



Hmm, I am afraid I do not have an answer for this, but this was the
requirement to always return the non cached rates from the clock driver.



Ok. Who knows about this requirement? Can we add someone from the
display driver to understand more?


As per my discussions offline with the display teams,

There is a use-case where the clock framework is unaware of the PLL VCO 
frequency change and thus the drivers would query to get the actual HW 
frequency rather than the cached one.


Do you think keeping these flags would have any impact other than always 
getting the non-cached rates?


--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--


Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-09 Thread Stephen Boyd
Quoting Taniya Das (2018-07-09 02:34:07)
> 
> 
> On 7/9/2018 1:07 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-07-09 00:07:21)
> >>
> >>
> >> On 7/9/2018 11:46 AM, Stephen Boyd wrote:
> 
> > Why is the nocache flag needed? Applies to all clks in this file.
> >
> 
>  This flag is required for all RCGs whose PLLs are controlled outside the
>  clock controller. The display code would require the recalculated rate
>  always.
> >>>
> >>> Right. Why is the PLL controlled outside of the clock controller? The
> >>> rate should propagate upward to the PLL from here, so who's going
> >>> outside of that?
> >>>
> >> The DSI0/1 PLL are not part of the display clock controller, but in the
> >> display subsystem which are managed by the DRM drivers. When DRM drivers
> >> query for the rate clock driver should always return the non cached rates.
> > 
> > Why? Is the DSI PLL changing rate all the time, randomly, without going
> > through the clk APIs to do so?
> >
> 
> Hmm, I am afraid I do not have an answer for this, but this was the 
> requirement to always return the non cached rates from the clock driver.
> 

Ok. Who knows about this requirement? Can we add someone from the
display driver to understand more?



Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-09 Thread Taniya Das




On 7/9/2018 1:07 PM, Stephen Boyd wrote:

Quoting Taniya Das (2018-07-09 00:07:21)



On 7/9/2018 11:46 AM, Stephen Boyd wrote:


   > Why is the nocache flag needed? Applies to all clks in this file.
   >

This flag is required for all RCGs whose PLLs are controlled outside the
clock controller. The display code would require the recalculated rate
always.


Right. Why is the PLL controlled outside of the clock controller? The
rate should propagate upward to the PLL from here, so who's going
outside of that?


The DSI0/1 PLL are not part of the display clock controller, but in the
display subsystem which are managed by the DRM drivers. When DRM drivers
query for the rate clock driver should always return the non cached rates.


Why? Is the DSI PLL changing rate all the time, randomly, without going
through the clk APIs to do so?



Hmm, I am afraid I do not have an answer for this, but this was the 
requirement to always return the non cached rates from the clock driver.



--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--


Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-09 Thread Stephen Boyd
Quoting Taniya Das (2018-07-09 00:07:21)
> 
> 
> On 7/9/2018 11:46 AM, Stephen Boyd wrote:
> >>
> >>   > Why is the nocache flag needed? Applies to all clks in this file.
> >>   >
> >>
> >> This flag is required for all RCGs whose PLLs are controlled outside the
> >> clock controller. The display code would require the recalculated rate
> >> always.
> > 
> > Right. Why is the PLL controlled outside of the clock controller? The
> > rate should propagate upward to the PLL from here, so who's going
> > outside of that?
> > 
> The DSI0/1 PLL are not part of the display clock controller, but in the 
> display subsystem which are managed by the DRM drivers. When DRM drivers
> query for the rate clock driver should always return the non cached rates.

Why? Is the DSI PLL changing rate all the time, randomly, without going
through the clk APIs to do so?



Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-09 Thread Taniya Das




On 7/9/2018 11:46 AM, Stephen Boyd wrote:

Quoting Taniya Das (2018-07-08 20:38:03)

Hello Stephen,

Thanks for your review comments.

On 7/9/2018 5:24 AM, Stephen Boyd wrote:

Quoting Taniya Das (2018-06-23 07:19:27)

diff --git a/drivers/clk/qcom/dispcc-sdm845.c b/drivers/clk/qcom/dispcc-sdm845.c
new file mode 100644
index 000..af437e0
--- /dev/null
+++ b/drivers/clk/qcom/dispcc-sdm845.c
@@ -0,0 +1,674 @@
+// SPDX-License-Identifier: GPL-2.0

[...]

+static struct clk_alpha_pll disp_cc_pll0 = {
+   .offset = 0x0,
+   .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+   .clkr = {
+   .hw.init = &(struct clk_init_data){
+   .name = "disp_cc_pll0",
+   .parent_names = (const char *[]){ "bi_tcxo" },
+   .num_parents = 1,
+   .ops = &clk_alpha_pll_fabia_ops,
+   },
+   },
+};
+
+static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
+   .cmd_rcgr = 0x20d0,
+   .mnd_width = 0,
+   .hid_width = 5,
+   .parent_map = disp_cc_parent_map_0,
+   .clkr.hw.init = &(struct clk_init_data){
+   .name = "disp_cc_mdss_byte0_clk_src",
+   .parent_names = disp_cc_parent_names_0,
+   .num_parents = 4,
+   .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,


Why is there the no cache flag? Last time I asked I don't think I got
any answer, and there isn't a comment here so please at least add a
comment to the code so we don't forget.



I think you missed my comment from the earlier email. I would add the
comment and submit again.


Hmm.. ok.



  > Why is the nocache flag needed? Applies to all clks in this file.
  >

This flag is required for all RCGs whose PLLs are controlled outside the
clock controller. The display code would require the recalculated rate
always.


Right. Why is the PLL controlled outside of the clock controller? The
rate should propagate upward to the PLL from here, so who's going
outside of that?

The DSI0/1 PLL are not part of the display clock controller, but in the 
display subsystem which are managed by the DRM drivers. When DRM drivers

query for the rate clock driver should always return the non cached rates.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--


Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-08 Thread Stephen Boyd
Quoting Taniya Das (2018-07-08 20:38:03)
> Hello Stephen,
> 
> Thanks for your review comments.
> 
> On 7/9/2018 5:24 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-06-23 07:19:27)
> >> diff --git a/drivers/clk/qcom/dispcc-sdm845.c 
> >> b/drivers/clk/qcom/dispcc-sdm845.c
> >> new file mode 100644
> >> index 000..af437e0
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/dispcc-sdm845.c
> >> @@ -0,0 +1,674 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> > [...]
> >> +static struct clk_alpha_pll disp_cc_pll0 = {
> >> +   .offset = 0x0,
> >> +   .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> >> +   .clkr = {
> >> +   .hw.init = &(struct clk_init_data){
> >> +   .name = "disp_cc_pll0",
> >> +   .parent_names = (const char *[]){ "bi_tcxo" },
> >> +   .num_parents = 1,
> >> +   .ops = &clk_alpha_pll_fabia_ops,
> >> +   },
> >> +   },
> >> +};
> >> +
> >> +static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
> >> +   .cmd_rcgr = 0x20d0,
> >> +   .mnd_width = 0,
> >> +   .hid_width = 5,
> >> +   .parent_map = disp_cc_parent_map_0,
> >> +   .clkr.hw.init = &(struct clk_init_data){
> >> +   .name = "disp_cc_mdss_byte0_clk_src",
> >> +   .parent_names = disp_cc_parent_names_0,
> >> +   .num_parents = 4,
> >> +   .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
> > 
> > Why is there the no cache flag? Last time I asked I don't think I got
> > any answer, and there isn't a comment here so please at least add a
> > comment to the code so we don't forget.
> >
> 
> I think you missed my comment from the earlier email. I would add the 
> comment and submit again.

Hmm.. ok.

> 
>  > Why is the nocache flag needed? Applies to all clks in this file.
>  >
> 
> This flag is required for all RCGs whose PLLs are controlled outside the 
> clock controller. The display code would require the recalculated rate 
> always.

Right. Why is the PLL controlled outside of the clock controller? The
rate should propagate upward to the PLL from here, so who's going
outside of that?



Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-08 Thread Taniya Das

Hello Stephen,

Thanks for your review comments.

On 7/9/2018 5:24 AM, Stephen Boyd wrote:

Quoting Taniya Das (2018-06-23 07:19:27)

diff --git a/drivers/clk/qcom/dispcc-sdm845.c b/drivers/clk/qcom/dispcc-sdm845.c
new file mode 100644
index 000..af437e0
--- /dev/null
+++ b/drivers/clk/qcom/dispcc-sdm845.c
@@ -0,0 +1,674 @@
+// SPDX-License-Identifier: GPL-2.0

[...]

+static struct clk_alpha_pll disp_cc_pll0 = {
+   .offset = 0x0,
+   .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+   .clkr = {
+   .hw.init = &(struct clk_init_data){
+   .name = "disp_cc_pll0",
+   .parent_names = (const char *[]){ "bi_tcxo" },
+   .num_parents = 1,
+   .ops = &clk_alpha_pll_fabia_ops,
+   },
+   },
+};
+
+static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
+   .cmd_rcgr = 0x20d0,
+   .mnd_width = 0,
+   .hid_width = 5,
+   .parent_map = disp_cc_parent_map_0,
+   .clkr.hw.init = &(struct clk_init_data){
+   .name = "disp_cc_mdss_byte0_clk_src",
+   .parent_names = disp_cc_parent_names_0,
+   .num_parents = 4,
+   .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,


Why is there the no cache flag? Last time I asked I don't think I got
any answer, and there isn't a comment here so please at least add a
comment to the code so we don't forget.



I think you missed my comment from the earlier email. I would add the 
comment and submit again.


> Why is the nocache flag needed? Applies to all clks in this file.
>

This flag is required for all RCGs whose PLLs are controlled outside the 
clock controller. The display code would require the recalculated rate 
always.



+   .ops = &clk_byte2_ops,
+   },
+};
+
+static struct clk_rcg2 disp_cc_mdss_byte1_clk_src = {
+   .cmd_rcgr = 0x20ec,
+   .mnd_width = 0,
+   .hid_width = 5,
+   .parent_map = disp_cc_parent_map_0,
+   .clkr.hw.init = &(struct clk_init_data){
+   .name = "disp_cc_mdss_byte1_clk_src",
+   .parent_names = disp_cc_parent_names_0,
+   .num_parents = 4,
+   .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
+   .ops = &clk_byte2_ops,
+   },
+};
+
+static const struct freq_tbl ftbl_disp_cc_mdss_esc0_clk_src[] = {
+   F(1920, P_BI_TCXO, 1, 0, 0),
+   { }
+};
+
+static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
+   .cmd_rcgr = 0x2108,
+   .mnd_width = 0,
+   .hid_width = 5,
+   .parent_map = disp_cc_parent_map_0,
+   .freq_tbl = ftbl_disp_cc_mdss_esc0_clk_src,
+   .clkr.hw.init = &(struct clk_init_data){
+   .name = "disp_cc_mdss_esc0_clk_src",
+   .parent_names = disp_cc_parent_names_0,
+   .num_parents = 4,
+   .ops = &clk_rcg2_ops,
+   },
+};
+
+static struct clk_rcg2 disp_cc_mdss_esc1_clk_src = {
+   .cmd_rcgr = 0x2120,
+   .mnd_width = 0,
+   .hid_width = 5,
+   .parent_map = disp_cc_parent_map_0,
+   .freq_tbl = ftbl_disp_cc_mdss_esc0_clk_src,
+   .clkr.hw.init = &(struct clk_init_data){
+   .name = "disp_cc_mdss_esc1_clk_src",
+   .parent_names = disp_cc_parent_names_0,
+   .num_parents = 4,
+   .ops = &clk_rcg2_ops,
+   },
+};
+

[...]

+
+MODULE_LICENSE("GPL v2");


MODULE_DESCRIPTION?



--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--


Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

2018-07-08 Thread Stephen Boyd
Quoting Taniya Das (2018-06-23 07:19:27)
> diff --git a/drivers/clk/qcom/dispcc-sdm845.c 
> b/drivers/clk/qcom/dispcc-sdm845.c
> new file mode 100644
> index 000..af437e0
> --- /dev/null
> +++ b/drivers/clk/qcom/dispcc-sdm845.c
> @@ -0,0 +1,674 @@
> +// SPDX-License-Identifier: GPL-2.0
[...]
> +static struct clk_alpha_pll disp_cc_pll0 = {
> +   .offset = 0x0,
> +   .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +   .clkr = {
> +   .hw.init = &(struct clk_init_data){
> +   .name = "disp_cc_pll0",
> +   .parent_names = (const char *[]){ "bi_tcxo" },
> +   .num_parents = 1,
> +   .ops = &clk_alpha_pll_fabia_ops,
> +   },
> +   },
> +};
> +
> +static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
> +   .cmd_rcgr = 0x20d0,
> +   .mnd_width = 0,
> +   .hid_width = 5,
> +   .parent_map = disp_cc_parent_map_0,
> +   .clkr.hw.init = &(struct clk_init_data){
> +   .name = "disp_cc_mdss_byte0_clk_src",
> +   .parent_names = disp_cc_parent_names_0,
> +   .num_parents = 4,
> +   .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,

Why is there the no cache flag? Last time I asked I don't think I got
any answer, and there isn't a comment here so please at least add a
comment to the code so we don't forget.

> +   .ops = &clk_byte2_ops,
> +   },
> +};
> +
> +static struct clk_rcg2 disp_cc_mdss_byte1_clk_src = {
> +   .cmd_rcgr = 0x20ec,
> +   .mnd_width = 0,
> +   .hid_width = 5,
> +   .parent_map = disp_cc_parent_map_0,
> +   .clkr.hw.init = &(struct clk_init_data){
> +   .name = "disp_cc_mdss_byte1_clk_src",
> +   .parent_names = disp_cc_parent_names_0,
> +   .num_parents = 4,
> +   .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
> +   .ops = &clk_byte2_ops,
> +   },
> +};
> +
> +static const struct freq_tbl ftbl_disp_cc_mdss_esc0_clk_src[] = {
> +   F(1920, P_BI_TCXO, 1, 0, 0),
> +   { }
> +};
> +
> +static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
> +   .cmd_rcgr = 0x2108,
> +   .mnd_width = 0,
> +   .hid_width = 5,
> +   .parent_map = disp_cc_parent_map_0,
> +   .freq_tbl = ftbl_disp_cc_mdss_esc0_clk_src,
> +   .clkr.hw.init = &(struct clk_init_data){
> +   .name = "disp_cc_mdss_esc0_clk_src",
> +   .parent_names = disp_cc_parent_names_0,
> +   .num_parents = 4,
> +   .ops = &clk_rcg2_ops,
> +   },
> +};
> +
> +static struct clk_rcg2 disp_cc_mdss_esc1_clk_src = {
> +   .cmd_rcgr = 0x2120,
> +   .mnd_width = 0,
> +   .hid_width = 5,
> +   .parent_map = disp_cc_parent_map_0,
> +   .freq_tbl = ftbl_disp_cc_mdss_esc0_clk_src,
> +   .clkr.hw.init = &(struct clk_init_data){
> +   .name = "disp_cc_mdss_esc1_clk_src",
> +   .parent_names = disp_cc_parent_names_0,
> +   .num_parents = 4,
> +   .ops = &clk_rcg2_ops,
> +   },
> +};
> +
[...]
> +
> +MODULE_LICENSE("GPL v2");

MODULE_DESCRIPTION?