[01/13] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency

Commit Message

Jorge Ramirez-Ortiz Dec. 17, 2018, 9:46 a.m. UTC

Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
specifications.

Co-developed-by: Niklas Cassel <[e-mail]>
Signed-off-by: Niklas Cassel <[e-mail]>
Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
---
 drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Taniya Das Dec. 21, 2018, 11:19 a.m. UTC | #1

On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> specifications.
> 
> Co-developed-by: Niklas Cassel <[e-mail]>
> Signed-off-by: Niklas Cassel <[e-mail]>
> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
> ---
>   drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> index 64da032..833436a 100644
> --- a/drivers/clk/qcom/gcc-qcs404.c
> +++ b/drivers/clk/qcom/gcc-qcs404.c
> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>   	},
>   };
>   
> +static const struct pll_vco gpll0_ao_out_vco[] = {
> +	{ 800000000, 800000000, 0 },
> +};
> +
>   static struct clk_alpha_pll gpll0_ao_out_main = {
>   	.offset = 0x21000,
>   	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>   	.flags = SUPPORTS_FSM_MODE,
> +	.vco_table = gpll0_ao_out_vco,
> +	.num_vco = ARRAY_SIZE(gpll0_ao_out_vco),

Could you please help as to why this is required? This is a fixed PLL 
and we do not require a VCO table for it.

>   	.clkr = {
>   		.enable_reg = 0x45000,
>   		.enable_mask = BIT(0),
>

Jorge Ramirez-Ortiz Dec. 21, 2018, 12:36 p.m. UTC | #2

On 12/21/18 12:19, Taniya Das wrote:
>
>
> On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
>> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
>> specifications.
>>
>> Co-developed-by: Niklas Cassel <[e-mail]>
>> Signed-off-by: Niklas Cassel <[e-mail]>
>> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
>> ---
>>   drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gcc-qcs404.c 
>> b/drivers/clk/qcom/gcc-qcs404.c
>> index 64da032..833436a 100644
>> --- a/drivers/clk/qcom/gcc-qcs404.c
>> +++ b/drivers/clk/qcom/gcc-qcs404.c
>> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>>       },
>>   };
>>   +static const struct pll_vco gpll0_ao_out_vco[] = {
>> +    { 800000000, 800000000, 0 },
>> +};
>> +
>>   static struct clk_alpha_pll gpll0_ao_out_main = {
>>       .offset = 0x21000,
>>       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>       .flags = SUPPORTS_FSM_MODE,
>> +    .vco_table = gpll0_ao_out_vco,
>> +    .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
>
> Could you please help as to why this is required? This is a fixed PLL 
> and we do not require a VCO table for it.

Hi Taniya,

this patch - the additional information that it provides about the 
hardware - helps to select the right parent clock for a given frequency.

On the qcs404 this clock is one of the two parent clocks of the apcs 
clock controller (the other one being the high frequency pll)
When cpufreq sets a target frequency, there is an iteration through the 
list of parents to select the one that delivers the best match.

When attempting to set the clock for an alpha_pll, the operation does a 
sanity check to validate that the requested frequency is in fact 
reachable using the vco range: trying to set a value that is not in 
range will fail.

This patch makes sure that its range is explicitly defined.

It also helps making sure there are no rounding issues when setting its 
value: without it the clock was being read at 799MHz


>
>>       .clkr = {
>>           .enable_reg = 0x45000,
>>           .enable_mask = BIT(0),
>>
>

Taniya Das Dec. 21, 2018, 5:58 p.m. UTC | #3

Hello,

On 12/21/2018 6:06 PM, Jorge Ramirez wrote:
> On 12/21/18 12:19, Taniya Das wrote:
>>
>>
>> On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
>>> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
>>> specifications.
>>>
>>> Co-developed-by: Niklas Cassel <[e-mail]>
>>> Signed-off-by: Niklas Cassel <[e-mail]>
>>> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
>>> ---
>>>   drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/gcc-qcs404.c 
>>> b/drivers/clk/qcom/gcc-qcs404.c
>>> index 64da032..833436a 100644
>>> --- a/drivers/clk/qcom/gcc-qcs404.c
>>> +++ b/drivers/clk/qcom/gcc-qcs404.c
>>> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>>>       },
>>>   };
>>>   +static const struct pll_vco gpll0_ao_out_vco[] = {
>>> +    { 800000000, 800000000, 0 },
>>> +};
>>> +
>>>   static struct clk_alpha_pll gpll0_ao_out_main = {
>>>       .offset = 0x21000,
>>>       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>>       .flags = SUPPORTS_FSM_MODE,
>>> +    .vco_table = gpll0_ao_out_vco,
>>> +    .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
>>
>> Could you please help as to why this is required? This is a fixed PLL 
>> and we do not require a VCO table for it.
> 
> Hi Taniya,
> 
> this patch - the additional information that it provides about the 
> hardware - helps to select the right parent clock for a given frequency.
> 
> On the qcs404 this clock is one of the two parent clocks of the apcs 
> clock controller (the other one being the high frequency pll)
> When cpufreq sets a target frequency, there is an iteration through the 
> list of parents to select the one that delivers the best match.
> 
> When attempting to set the clock for an alpha_pll, the operation does a 
> sanity check to validate that the requested frequency is in fact 
> reachable using the vco range: trying to set a value that is not in 
> range will fail.
> 
> This patch makes sure that its range is explicitly defined.
> 
> It also helps making sure there are no rounding issues when setting its 
> value: without it the clock was being read at 799MHz
> 
> 

If the PLL is being read as 799MHz it would because not all the 40 bits 
of the ALPHA_VAL being programmed by the bootloaders(which are the 
original owners of this PLL). So we should go with the way they are 
being set by bootloaders and read by HLOS driver.

And a VCO range you have considered is wrong from a PLL perspective. As 
these are fixed PLLs and VCO range really does not matter here, so 
please drop this change.

>>
>>>       .clkr = {
>>>           .enable_reg = 0x45000,
>>>           .enable_mask = BIT(0),
>>>
>>
>

Bjorn Andersson Dec. 21, 2018, 7:28 p.m. UTC | #4

On Fri 21 Dec 09:58 PST 2018, Taniya Das wrote:

> Hello,
> 
> On 12/21/2018 6:06 PM, Jorge Ramirez wrote:
> > On 12/21/18 12:19, Taniya Das wrote:
> > > 
> > > 
> > > On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
> > > > Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> > > > specifications.
> > > > 
> > > > Co-developed-by: Niklas Cassel <[e-mail]>
> > > > Signed-off-by: Niklas Cassel <[e-mail]>
> > > > Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
> > > > ---
> > > >   drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/clk/qcom/gcc-qcs404.c
> > > > b/drivers/clk/qcom/gcc-qcs404.c
> > > > index 64da032..833436a 100644
> > > > --- a/drivers/clk/qcom/gcc-qcs404.c
> > > > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > > > @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
> > > >       },
> > > >   };
> > > >   +static const struct pll_vco gpll0_ao_out_vco[] = {
> > > > +    { 800000000, 800000000, 0 },
> > > > +};
> > > > +
> > > >   static struct clk_alpha_pll gpll0_ao_out_main = {
> > > >       .offset = 0x21000,
> > > >       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
> > > >       .flags = SUPPORTS_FSM_MODE,
> > > > +    .vco_table = gpll0_ao_out_vco,
> > > > +    .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
> > > 
> > > Could you please help as to why this is required? This is a fixed
> > > PLL and we do not require a VCO table for it.
> > 
> > Hi Taniya,
> > 
> > this patch - the additional information that it provides about the
> > hardware - helps to select the right parent clock for a given frequency.
> > 
> > On the qcs404 this clock is one of the two parent clocks of the apcs
> > clock controller (the other one being the high frequency pll)
> > When cpufreq sets a target frequency, there is an iteration through the
> > list of parents to select the one that delivers the best match.
> > 
> > When attempting to set the clock for an alpha_pll, the operation does a
> > sanity check to validate that the requested frequency is in fact
> > reachable using the vco range: trying to set a value that is not in
> > range will fail.
> > 
> > This patch makes sure that its range is explicitly defined.
> > 
> > It also helps making sure there are no rounding issues when setting its
> > value: without it the clock was being read at 799MHz
> > 
> > 
> 
> If the PLL is being read as 799MHz it would because not all the 40 bits of
> the ALPHA_VAL being programmed by the bootloaders(which are the original
> owners of this PLL). So we should go with the way they are being set by
> bootloaders and read by HLOS driver.
> 
> And a VCO range you have considered is wrong from a PLL perspective. As
> these are fixed PLLs and VCO range really does not matter here, so please
> drop this change.
> 

The problem here is that the PLL should be fixed at 800MHz, but the
alpha PLL is defined such that it can change. So when the mux-div is
looking for a suitable parent and divider for our CPU clock it concludes
that the best way to reach certain frequencies is to change the rate of
GPLL0.

Adding the vco_table limits the available frequencies for GPLL0 in
QCS404, without modifying the implementation of the alpha PLL.

Perhaps there's a better way to define that this particular clock
hardware can change rate, but in this implementation it must not?

Regards,
Bjorn

Jorge Ramirez-Ortiz Dec. 21, 2018, 7:45 p.m. UTC | #5

On 12/21/18 20:28, Bjorn Andersson wrote:
> On Fri 21 Dec 09:58 PST 2018, Taniya Das wrote:
>
>> Hello,
>>
>> On 12/21/2018 6:06 PM, Jorge Ramirez wrote:
>>> On 12/21/18 12:19, Taniya Das wrote:
>>>>
>>>> On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
>>>>> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
>>>>> specifications.
>>>>>
>>>>> Co-developed-by: Niklas Cassel <[e-mail]>
>>>>> Signed-off-by: Niklas Cassel <[e-mail]>
>>>>> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
>>>>> ---
>>>>>    drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/gcc-qcs404.c
>>>>> b/drivers/clk/qcom/gcc-qcs404.c
>>>>> index 64da032..833436a 100644
>>>>> --- a/drivers/clk/qcom/gcc-qcs404.c
>>>>> +++ b/drivers/clk/qcom/gcc-qcs404.c
>>>>> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>>>>>        },
>>>>>    };
>>>>>    +static const struct pll_vco gpll0_ao_out_vco[] = {
>>>>> +    { 800000000, 800000000, 0 },
>>>>> +};
>>>>> +
>>>>>    static struct clk_alpha_pll gpll0_ao_out_main = {
>>>>>        .offset = 0x21000,
>>>>>        .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>>>>        .flags = SUPPORTS_FSM_MODE,
>>>>> +    .vco_table = gpll0_ao_out_vco,
>>>>> +    .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
>>>> Could you please help as to why this is required? This is a fixed
>>>> PLL and we do not require a VCO table for it.
>>> Hi Taniya,
>>>
>>> this patch - the additional information that it provides about the
>>> hardware - helps to select the right parent clock for a given frequency.
>>>
>>> On the qcs404 this clock is one of the two parent clocks of the apcs
>>> clock controller (the other one being the high frequency pll)
>>> When cpufreq sets a target frequency, there is an iteration through the
>>> list of parents to select the one that delivers the best match.
>>>
>>> When attempting to set the clock for an alpha_pll, the operation does a
>>> sanity check to validate that the requested frequency is in fact
>>> reachable using the vco range: trying to set a value that is not in
>>> range will fail.
>>>
>>> This patch makes sure that its range is explicitly defined.
>>>
>>> It also helps making sure there are no rounding issues when setting its
>>> value: without it the clock was being read at 799MHz
>>>
>>>
>> If the PLL is being read as 799MHz it would because not all the 40 bits of
>> the ALPHA_VAL being programmed by the bootloaders(which are the original
>> owners of this PLL). So we should go with the way they are being set by
>> bootloaders and read by HLOS driver.
>>
>> And a VCO range you have considered is wrong from a PLL perspective. As
>> these are fixed PLLs and VCO range really does not matter here, so please
>> drop this change.
>>
> The problem here is that the PLL should be fixed at 800MHz, but the
> alpha PLL is defined such that it can change. So when the mux-div is
> looking for a suitable parent and divider for our CPU clock it concludes
> that the best way to reach certain frequencies is to change the rate of
> GPLL0.
>
> Adding the vco_table limits the available frequencies for GPLL0 in
> QCS404, without modifying the implementation of the alpha PLL.
>
> Perhaps there's a better way to define that this particular clock
> hardware can change rate, but in this implementation it must not?

the initialization for this particular PLL on this particular platform 
is wrong
as the interface does not apply to the platform needs even though it is an
alpha_pll

if the VCO is not an option -even though it reflects the platform 
constrains-
I would suggest nullifying the alpha_pll_ops that do not apply to this 
platform:
ie: set_rate, round_rate set to null in the probe.

allowing the interface calls (ops) to go through to later on make them fail
based on some setting would be fundamentally wrong IMO












>
> Regards,
> Bjorn
>

Stephen Boyd Dec. 21, 2018, 9:40 p.m. UTC | #6

Quoting Jorge Ramirez (2018-12-21 11:45:28)
> On 12/21/18 20:28, Bjorn Andersson wrote:
> >
> > Perhaps there's a better way to define that this particular clock
> > hardware can change rate, but in this implementation it must not?
> 
> the initialization for this particular PLL on this particular platform 
> is wrong
> as the interface does not apply to the platform needs even though it is an
> alpha_pll
> 
> if the VCO is not an option -even though it reflects the platform 
> constrains-
> I would suggest nullifying the alpha_pll_ops that do not apply to this 
> platform:
> ie: set_rate, round_rate set to null in the probe.
> 
> allowing the interface calls (ops) to go through to later on make them fail
> based on some setting would be fundamentally wrong IMO
> 

We have clk_alpha_pll_postdiv_ro_ops so maybe just add another set of
those for the alpha_pll itself?

Jorge Ramirez-Ortiz Dec. 21, 2018, 9:45 p.m. UTC | #7

On 12/21/18 22:40, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2018-12-21 11:45:28)
>> On 12/21/18 20:28, Bjorn Andersson wrote:
>>> Perhaps there's a better way to define that this particular clock
>>> hardware can change rate, but in this implementation it must not?
>> the initialization for this particular PLL on this particular platform
>> is wrong
>> as the interface does not apply to the platform needs even though it is an
>> alpha_pll
>>
>> if the VCO is not an option -even though it reflects the platform
>> constrains-
>> I would suggest nullifying the alpha_pll_ops that do not apply to this
>> platform:
>> ie: set_rate, round_rate set to null in the probe.
>>
>> allowing the interface calls (ops) to go through to later on make them fail
>> based on some setting would be fundamentally wrong IMO
>>
> We have clk_alpha_pll_postdiv_ro_ops so maybe just add another set of
> those for the alpha_pll itself?
>
>

something like 
clk_alpha_pll_fixed_ops?

Stephen Boyd Dec. 21, 2018, 9:51 p.m. UTC | #8

Quoting Jorge Ramirez (2018-12-21 13:45:41)
> On 12/21/18 22:40, Stephen Boyd wrote:
> > Quoting Jorge Ramirez (2018-12-21 11:45:28)
> >> On 12/21/18 20:28, Bjorn Andersson wrote:
> >>> Perhaps there's a better way to define that this particular clock
> >>> hardware can change rate, but in this implementation it must not?
> >> the initialization for this particular PLL on this particular platform
> >> is wrong
> >> as the interface does not apply to the platform needs even though it is an
> >> alpha_pll
> >>
> >> if the VCO is not an option -even though it reflects the platform
> >> constrains-
> >> I would suggest nullifying the alpha_pll_ops that do not apply to this
> >> platform:
> >> ie: set_rate, round_rate set to null in the probe.
> >>
> >> allowing the interface calls (ops) to go through to later on make them fail
> >> based on some setting would be fundamentally wrong IMO
> >>
> > We have clk_alpha_pll_postdiv_ro_ops so maybe just add another set of
> > those for the alpha_pll itself?
> >
> >
> 
> something like 
> clk_alpha_pll_fixed_ops?

Either way works. We're not consistent in naming now that we have
clk_alpha_pll_fixed_fabia_ops.

Patch

diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
index 64da032..833436a 100644
--- a/drivers/clk/qcom/gcc-qcs404.c
+++ b/drivers/clk/qcom/gcc-qcs404.c
@@ -304,10 +304,16 @@  static struct clk_alpha_pll gpll0_out_main = {
 	},
 };
 
+static const struct pll_vco gpll0_ao_out_vco[] = {
+	{ 800000000, 800000000, 0 },
+};
+
 static struct clk_alpha_pll gpll0_ao_out_main = {
 	.offset = 0x21000,
 	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
 	.flags = SUPPORTS_FSM_MODE,
+	.vco_table = gpll0_ao_out_vco,
+	.num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
 	.clkr = {
 		.enable_reg = 0x45000,
 		.enable_mask = BIT(0),