Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450418 > unrolled thread

Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate  application method Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-26 09:00 +0200
    Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman <megous@megous.com> - 2016-07-28 13:30 +0200
      Re: [linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock  implementation is wrong Chen-Yu Tsai <wens@csie.org> - 2016-07-28 13:40 +0200
      Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-28 23:10 +0200
        Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman <megous@megous.com> - 2016-07-29 00:10 +0200
          Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-31 22:30 +0200
            Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman <megous@megous.com> - 2016-08-01 00:10 +0200

#1450418 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> >>> If so, then yes, trying to switch to the 24MHz oscillator before
> >>> applying the factors, and then switching back when the PLL is stable
> >>> would be a nice solution.
> >>>
> >>> I just checked, and all the SoCs we've had so far have that
> >>> possibility, so if it works, for now, I'd like to stick to that.
> >>
> >> It would need to be tested. U-boot does the change only once, while the
> >> kernel would be doing it all the time and between various frequencies
> >> and PLL settings. So the issues may show up with this solution too.
> > 
> > That would have the benefit of being quite easy to document, not be a
> > huge amount of code and it would work on all the CPUs PLLs we have so
> > far, so still, a pretty big win. If it doesn't, of course, we don't
> > really have the choice.
> 
> It's probably more code though. It has to access different register from
> the one that is already defined in dts, which would add a lot of code
> and require dts changes. The original patch I sent is simpler than that.

Why?

You can use container_of to retrieve the parent structure of the clock
notifier, and then you get a ccu_common structure pointer, with the
CCU base address, the clock register, its lock, etc.

Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.

I don't really get why anything should be changed in the DT, or why it
would add a lot of code. Or maybe we're not talking about the same
thing?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [next] | [standalone]

#1451886 — Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

[Multipart message — attachments visible in raw view] — view raw

Hi Maxime,

I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
to this.

On 26.7.2016 08:32, Maxime Ripard wrote:
> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
>>>>> If so, then yes, trying to switch to the 24MHz oscillator before
>>>>> applying the factors, and then switching back when the PLL is stable
>>>>> would be a nice solution.
>>>>>
>>>>> I just checked, and all the SoCs we've had so far have that
>>>>> possibility, so if it works, for now, I'd like to stick to that.
>>>>
>>>> It would need to be tested. U-boot does the change only once, while the
>>>> kernel would be doing it all the time and between various frequencies
>>>> and PLL settings. So the issues may show up with this solution too.
>>>
>>> That would have the benefit of being quite easy to document, not be a
>>> huge amount of code and it would work on all the CPUs PLLs we have so
>>> far, so still, a pretty big win. If it doesn't, of course, we don't
>>> really have the choice.
>>
>> It's probably more code though. It has to access different register from
>> the one that is already defined in dts, which would add a lot of code
>> and require dts changes. The original patch I sent is simpler than that.
> 
> Why?
> 
> You can use container_of to retrieve the parent structure of the clock
> notifier, and then you get a ccu_common structure pointer, with the
> CCU base address, the clock register, its lock, etc.
> 
> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
> 
> I don't really get why anything should be changed in the DT, or why it
> would add a lot of code. Or maybe we're not talking about the same
> thing?

I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
it very liberally uses divider parameters, even in situations that are
out of spec compared to the current code in the kernel.

In the current code and especially in the original vendor code, divider
parameters are used as last resort only. Presumably because, of the
inherent trouble in changing them, as I described to you in other email.

The new ccu code uses dividers often and even at very high frequencies,
which goes against the spec.

In the vendor code M is never anything else but 0, and P is used only
for frequencies below 288MHz, which matches the H3 datasheet, which says:

"The P factor only use in the condition that PLL output less than 288
MHz."

Also other datasheets of similar socs from Allwinner state that M should
not be used in production code. So it may be that they either forgot to
state it in the H3 datasheet, or it can be used. In any case, they never
use M in their code, so it may be wise to keep to that.

When I boot with the new CCU code I get this:

PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
PLL_CPUX : freq = 1008MHz

Mathematically it works, but it is against the spec.

Also, this:

analyzing CPU 0:
  driver: cpufreq-dt
  CPUs which run at the same hardware frequency: 0 1 2 3
  CPUs which need to have their frequency coordinated by software: 0 1 2 3
  maximum transition latency: 1.74 ms
  hardware limits: 120 MHz - 1.37 GHz
  available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
  available cpufreq governors: conservative ondemand userspace powersave
performance
  current policy: frequency should be within 240 MHz and 240 MHz.
                  The governor "performance" may decide which speed to use
                  within this range.
  current CPU frequency: 24.0 MHz (asserted by call to hardware)


Somehow, the new CCU code sets the CPUX to 24MHz no matter what.

I'm using your pen/clk-rework branch without any other patches that were
later sent to the mailing list.

regards,
  Ondrej

> 
> Maxime
> 

[toc] | [prev] | [next] | [standalone]

#1451890 — Re: [linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

Hi,

On Thu, Jul 28, 2016 at 7:27 PM, Ondřej Jirman <megous@megous.com> wrote:
> Hi Maxime,
>
> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
> to this.
>
> On 26.7.2016 08:32, Maxime Ripard wrote:
>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before
>>>>>> applying the factors, and then switching back when the PLL is stable
>>>>>> would be a nice solution.
>>>>>>
>>>>>> I just checked, and all the SoCs we've had so far have that
>>>>>> possibility, so if it works, for now, I'd like to stick to that.
>>>>>
>>>>> It would need to be tested. U-boot does the change only once, while the
>>>>> kernel would be doing it all the time and between various frequencies
>>>>> and PLL settings. So the issues may show up with this solution too.
>>>>
>>>> That would have the benefit of being quite easy to document, not be a
>>>> huge amount of code and it would work on all the CPUs PLLs we have so
>>>> far, so still, a pretty big win. If it doesn't, of course, we don't
>>>> really have the choice.
>>>
>>> It's probably more code though. It has to access different register from
>>> the one that is already defined in dts, which would add a lot of code
>>> and require dts changes. The original patch I sent is simpler than that.
>>
>> Why?
>>
>> You can use container_of to retrieve the parent structure of the clock
>> notifier, and then you get a ccu_common structure pointer, with the
>> CCU base address, the clock register, its lock, etc.
>>
>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
>>
>> I don't really get why anything should be changed in the DT, or why it
>> would add a lot of code. Or maybe we're not talking about the same
>> thing?
>
> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
> it very liberally uses divider parameters, even in situations that are
> out of spec compared to the current code in the kernel.
>
> In the current code and especially in the original vendor code, divider
> parameters are used as last resort only. Presumably because, of the
> inherent trouble in changing them, as I described to you in other email.
>
> The new ccu code uses dividers often and even at very high frequencies,
> which goes against the spec.
>
> In the vendor code M is never anything else but 0, and P is used only
> for frequencies below 288MHz, which matches the H3 datasheet, which says:
>
> "The P factor only use in the condition that PLL output less than 288
> MHz."
>
> Also other datasheets of similar socs from Allwinner state that M should
> not be used in production code. So it may be that they either forgot to
> state it in the H3 datasheet, or it can be used. In any case, they never
> use M in their code, so it may be wise to keep to that.
>
> When I boot with the new CCU code I get this:
>
> PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
> PLL_CPUX : freq = 1008MHz
>
> Mathematically it works, but it is against the spec.
>
> Also, this:
>
> analyzing CPU 0:
>   driver: cpufreq-dt
>   CPUs which run at the same hardware frequency: 0 1 2 3
>   CPUs which need to have their frequency coordinated by software: 0 1 2 3
>   maximum transition latency: 1.74 ms
>   hardware limits: 120 MHz - 1.37 GHz
>   available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
> MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
> GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
>   available cpufreq governors: conservative ondemand userspace powersave
> performance
>   current policy: frequency should be within 240 MHz and 240 MHz.
>                   The governor "performance" may decide which speed to use
>                   within this range.
>   current CPU frequency: 24.0 MHz (asserted by call to hardware)
>
>
> Somehow, the new CCU code sets the CPUX to 24MHz no matter what.
>
> I'm using your pen/clk-rework branch without any other patches that were
> later sent to the mailing list.

The H3 CPUX clock does not have the CLK_SET_RATE_PARENT flag set, which
means cpufreq's attempts to set the clk rate will, in some cases, be
ignored as the change is not propagated up to the parent PLL. In the
worst case it will select the 24 MHz oscillator. The fix is to add
CLK_SET_RATE_PARENT. And then you'll have to deal with the PLL potentially
going too high, as you stated in your other mail, and can be mitigated
with my clk notifier patch.

ChenYu

> regards,
>   Ondrej
>
>>
>> Maxime
>>
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1452106 — Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

[Multipart message — attachments visible in raw view] — view raw

Hi Ondrej,

On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ondřej Jirman wrote:
> Hi Maxime,
> 
> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
> to this.

You can find it in the clock maintainers tree:
https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng

> On 26.7.2016 08:32, Maxime Ripard wrote:
> > On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> >>>>> If so, then yes, trying to switch to the 24MHz oscillator before
> >>>>> applying the factors, and then switching back when the PLL is stable
> >>>>> would be a nice solution.
> >>>>>
> >>>>> I just checked, and all the SoCs we've had so far have that
> >>>>> possibility, so if it works, for now, I'd like to stick to that.
> >>>>
> >>>> It would need to be tested. U-boot does the change only once, while the
> >>>> kernel would be doing it all the time and between various frequencies
> >>>> and PLL settings. So the issues may show up with this solution too.
> >>>
> >>> That would have the benefit of being quite easy to document, not be a
> >>> huge amount of code and it would work on all the CPUs PLLs we have so
> >>> far, so still, a pretty big win. If it doesn't, of course, we don't
> >>> really have the choice.
> >>
> >> It's probably more code though. It has to access different register from
> >> the one that is already defined in dts, which would add a lot of code
> >> and require dts changes. The original patch I sent is simpler than that.
> > 
> > Why?
> > 
> > You can use container_of to retrieve the parent structure of the clock
> > notifier, and then you get a ccu_common structure pointer, with the
> > CCU base address, the clock register, its lock, etc.
> > 
> > Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
> > 
> > I don't really get why anything should be changed in the DT, or why it
> > would add a lot of code. Or maybe we're not talking about the same
> > thing?
> 
> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
> it very liberally uses divider parameters, even in situations that are
> out of spec compared to the current code in the kernel.
> 
> In the current code and especially in the original vendor code, divider
> parameters are used as last resort only. Presumably because, of the
> inherent trouble in changing them, as I described to you in other email.
> 
> The new ccu code uses dividers often and even at very high frequencies,
> which goes against the spec.
> 
> In the vendor code M is never anything else but 0, and P is used only
> for frequencies below 288MHz, which matches the H3 datasheet, which says:

In the vendor code, P is never used either. All the boards we had so
far don't go that low, so we cannot make any of these assumptions,
especially since the vendor code has had the bad habit of doing
something wrong and / or useless in the past.

However, this implementation is a new thing, and it was using the H3
precisely because of its early stage of support to use it as a testbed
for the more established.

If you feel like we should use a different formula to favour the
multipliers over the dividers (or want to change the class of the CPU
PLL to an NKM or something else, this is totally doable.

> "The P factor only use in the condition that PLL output less than 288
> MHz."

And the datasheet also had some issues, either misleading or wrong
comments in the past. Don't get me wrong, I'm not saying that this is
wrong, just that we should not follow it religiously, and that we
should trust more the experiments than the datasheet.

> Also other datasheets of similar socs from Allwinner state that M should
> not be used in production code.

Which ones specifically?

> So it may be that they either forgot to state it in the H3
> datasheet, or it can be used. In any case, they never use M in their
> code, so it may be wise to keep to that.
> 
> When I boot with the new CCU code I get this:
> 
> PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
> PLL_CPUX : freq = 1008MHz
> 
> Mathematically it works, but it is against the spec.
> 
> Also, this:
> 
> analyzing CPU 0:
>   driver: cpufreq-dt
>   CPUs which run at the same hardware frequency: 0 1 2 3
>   CPUs which need to have their frequency coordinated by software: 0 1 2 3
>   maximum transition latency: 1.74 ms
>   hardware limits: 120 MHz - 1.37 GHz
>   available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
> MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
> GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
>   available cpufreq governors: conservative ondemand userspace powersave
> performance
>   current policy: frequency should be within 240 MHz and 240 MHz.
>                   The governor "performance" may decide which speed to use
>                   within this range.
>   current CPU frequency: 24.0 MHz (asserted by call to hardware)
> 
> 
> Somehow, the new CCU code sets the CPUX to 24MHz no matter what.
> 
> I'm using your pen/clk-rework branch without any other patches that were
> later sent to the mailing list.

It's probably because of CLK_SET_RATE_PARENT on the CPU clock missing,
as Chen-Yu suggested.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1452145 — Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

[Multipart message — attachments visible in raw view] — view raw

On 28.7.2016 23:00, Maxime Ripard wrote:
> Hi Ondrej,
> 
> On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ondřej Jirman wrote:
>> Hi Maxime,
>>
>> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
>> to this.
> 
> You can find it in the clock maintainers tree:
> https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng
> 
>> On 26.7.2016 08:32, Maxime Ripard wrote:
>>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
>>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before
>>>>>>> applying the factors, and then switching back when the PLL is stable
>>>>>>> would be a nice solution.
>>>>>>>
>>>>>>> I just checked, and all the SoCs we've had so far have that
>>>>>>> possibility, so if it works, for now, I'd like to stick to that.
>>>>>>
>>>>>> It would need to be tested. U-boot does the change only once, while the
>>>>>> kernel would be doing it all the time and between various frequencies
>>>>>> and PLL settings. So the issues may show up with this solution too.
>>>>>
>>>>> That would have the benefit of being quite easy to document, not be a
>>>>> huge amount of code and it would work on all the CPUs PLLs we have so
>>>>> far, so still, a pretty big win. If it doesn't, of course, we don't
>>>>> really have the choice.
>>>>
>>>> It's probably more code though. It has to access different register from
>>>> the one that is already defined in dts, which would add a lot of code
>>>> and require dts changes. The original patch I sent is simpler than that.
>>>
>>> Why?
>>>
>>> You can use container_of to retrieve the parent structure of the clock
>>> notifier, and then you get a ccu_common structure pointer, with the
>>> CCU base address, the clock register, its lock, etc.
>>>
>>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
>>>
>>> I don't really get why anything should be changed in the DT, or why it
>>> would add a lot of code. Or maybe we're not talking about the same
>>> thing?
>>
>> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
>> it very liberally uses divider parameters, even in situations that are
>> out of spec compared to the current code in the kernel.
>>
>> In the current code and especially in the original vendor code, divider
>> parameters are used as last resort only. Presumably because, of the
>> inherent trouble in changing them, as I described to you in other email.
>>
>> The new ccu code uses dividers often and even at very high frequencies,
>> which goes against the spec.
>>
>> In the vendor code M is never anything else but 0, and P is used only
>> for frequencies below 288MHz, which matches the H3 datasheet, which says:
> 
> In the vendor code, P is never used either. All the boards we had so
> far don't go that low, so we cannot make any of these assumptions,
> especially since the vendor code has had the bad habit of doing
> something wrong and / or useless in the past.

P is used in the arisc firmware according to the spec for the lower
frequencies.

> However, this implementation is a new thing, and it was using the H3
> precisely because of its early stage of support to use it as a testbed
> for the more established.
> 
> If you feel like we should use a different formula to favour the
> multipliers over the dividers (or want to change the class of the CPU
> PLL to an NKM or something else, this is totally doable.

I think the original formula that's currently in the mainline kernel is
better and avoids fiddling with dividers too much.

>> "The P factor only use in the condition that PLL output less than 288
>> MHz."
> 
> And the datasheet also had some issues, either misleading or wrong
> comments in the past. Don't get me wrong, I'm not saying that this is
> wrong, just that we should not follow it religiously, and that we
> should trust more the experiments than the datasheet.

I can believe that. :) Regardless, I think the reasons given for
avoiding dividers are quite reasonable. It's based on how PLL block
works, not what manual says.

>> Also other datasheets of similar socs from Allwinner state that M should
>> not be used in production code.
> 
> Which ones specifically?

A83T for example. You can search for "only for test" phrase.

https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf

Those PLLs are a bit different though.


regards,
  Ondrej

>> So it may be that they either forgot to state it in the H3
>> datasheet, or it can be used. In any case, they never use M in their
>> code, so it may be wise to keep to that.
>>
>> When I boot with the new CCU code I get this:
>>
>> PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 
0
>> PLL_CPUX : freq = 1008MHz
>>
>> Mathematically it works, but it is against the spec.
>>
>> Also, this:
>>
>> analyzing CPU 0:
>>   driver: cpufreq-dt
>>   CPUs which run at the same hardware frequency: 0 1 2 3
>>   CPUs which need to have their frequency coordinated by software: 0 1 2 3
>>   maximum transition latency: 1.74 ms
>>   hardware limits: 120 MHz - 1.37 GHz
>>   available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
>> MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
>> GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
>>   available cpufreq governors: conservative ondemand userspace powersave
>> performance
>>   current policy: frequency should be within 240 MHz and 240 MHz.
>>                   The governor "performance" may decide which speed to use
>>                   within this range.
>>   current CPU frequency: 24.0 MHz (asserted by call to hardware)
>>
>>
>> Somehow, the new CCU code sets the CPUX to 24MHz no matter what.
>>
>> I'm using your pen/clk-rework branch without any other patches that were
>> later sent to the mailing list.
> 
> It's probably because of CLK_SET_RATE_PARENT on the CPU clock missing,
> as Chen-Yu suggested.
> 
> Maxime
> 

[toc] | [prev] | [next] | [standalone]

#1452886 — Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Jul 29, 2016 at 12:01:09AM +0200, Ondřej Jirman wrote:
> On 28.7.2016 23:00, Maxime Ripard wrote:
> > Hi Ondrej,
> > 
> > On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ondřej Jirman wrote:
> >> Hi Maxime,
> >>
> >> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
> >> to this.
> > 
> > You can find it in the clock maintainers tree:
> > https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng
> > 
> >> On 26.7.2016 08:32, Maxime Ripard wrote:
> >>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> >>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before
> >>>>>>> applying the factors, and then switching back when the PLL is stable
> >>>>>>> would be a nice solution.
> >>>>>>>
> >>>>>>> I just checked, and all the SoCs we've had so far have that
> >>>>>>> possibility, so if it works, for now, I'd like to stick to that.
> >>>>>>
> >>>>>> It would need to be tested. U-boot does the change only once, while the
> >>>>>> kernel would be doing it all the time and between various frequencies
> >>>>>> and PLL settings. So the issues may show up with this solution too.
> >>>>>
> >>>>> That would have the benefit of being quite easy to document, not be a
> >>>>> huge amount of code and it would work on all the CPUs PLLs we have so
> >>>>> far, so still, a pretty big win. If it doesn't, of course, we don't
> >>>>> really have the choice.
> >>>>
> >>>> It's probably more code though. It has to access different register from
> >>>> the one that is already defined in dts, which would add a lot of code
> >>>> and require dts changes. The original patch I sent is simpler than that.
> >>>
> >>> Why?
> >>>
> >>> You can use container_of to retrieve the parent structure of the clock
> >>> notifier, and then you get a ccu_common structure pointer, with the
> >>> CCU base address, the clock register, its lock, etc.
> >>>
> >>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
> >>>
> >>> I don't really get why anything should be changed in the DT, or why it
> >>> would add a lot of code. Or maybe we're not talking about the same
> >>> thing?
> >>
> >> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
> >> it very liberally uses divider parameters, even in situations that are
> >> out of spec compared to the current code in the kernel.
> >>
> >> In the current code and especially in the original vendor code, divider
> >> parameters are used as last resort only. Presumably because, of the
> >> inherent trouble in changing them, as I described to you in other email.
> >>
> >> The new ccu code uses dividers often and even at very high frequencies,
> >> which goes against the spec.
> >>
> >> In the vendor code M is never anything else but 0, and P is used only
> >> for frequencies below 288MHz, which matches the H3 datasheet, which says:
> > 
> > In the vendor code, P is never used either. All the boards we had so
> > far don't go that low, so we cannot make any of these assumptions,
> > especially since the vendor code has had the bad habit of doing
> > something wrong and / or useless in the past.
> 
> P is used in the arisc firmware according to the spec for the lower
> frequencies.

Yes, but has anyone actually tested those frequencies? Judging from
the FEX files I could gather, cpufreq never actually goes lower than
480 MHz.

> > However, this implementation is a new thing, and it was using the H3
> > precisely because of its early stage of support to use it as a testbed
> > for the more established.
> > 
> > If you feel like we should use a different formula to favour the
> > multipliers over the dividers (or want to change the class of the CPU
> > PLL to an NKM or something else, this is totally doable.
> 
> I think the original formula that's currently in the mainline kernel is
> better and avoids fiddling with dividers too much.

Yeah, but the older formula is not generic at all. The whole rework
was precisely to avoid doing the whole one driver per clock that was
just becoming a nightmare to maintain, and a pain to add support for
new SoCs. That code will be used for A10's CPU and VE PLLs too for
example. And they probably have the same constraints, but with
different variations (available values of each factors for example).

> >> "The P factor only use in the condition that PLL output less than 288
> >> MHz."
> > 
> > And the datasheet also had some issues, either misleading or wrong
> > comments in the past. Don't get me wrong, I'm not saying that this is
> > wrong, just that we should not follow it religiously, and that we
> > should trust more the experiments than the datasheet.
> 
> I can believe that. :) Regardless, I think the reasons given for
> avoiding dividers are quite reasonable. It's based on how PLL block
> works, not what manual says.

Yes, indeed.

Would replacing the current factors computation function by something
like:

for (m = 1; m < max_m; m++)
    for (p = 1; p < max_p; p++)
        for (n = 1; n < max_n; n++)
	     for (k = 1; k < max_k; k++)
	         if rate == computed rate
		     break;

work for you?

That way, we will favor the multipliers over the dividers, and we can
always "blacklist" p or m (or both) by setting their maximum to 1
(this would need an extra field in _ccu_div though).

> >> Also other datasheets of similar socs from Allwinner state that M should
> >> not be used in production code.
> > 
> > Which ones specifically?
> 
> A83T for example. You can search for "only for test" phrase.
> 
> https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf
> 
> Those PLLs are a bit different though.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1452898 — Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

[Multipart message — attachments visible in raw view] — view raw

Hi,

On 31.7.2016 12:31, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Jul 29, 2016 at 12:01:09AM +0200, Ondřej Jirman wrote:
>> On 28.7.2016 23:00, Maxime Ripard wrote:
>>> Hi Ondrej,
>>>
>>> On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ondřej Jirman wrote:
>>>> Hi Maxime,
>>>>
>>>> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
>>>> to this.
>>>
>>> You can find it in the clock maintainers tree:
>>> https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng
>>>
>>>> On 26.7.2016 08:32, Maxime Ripard wrote:
>>>>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
>>>>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before
>>>>>>>>> applying the factors, and then switching back when the PLL is stable
>>>>>>>>> would be a nice solution.
>>>>>>>>>
>>>>>>>>> I just checked, and all the SoCs we've had so far have that
>>>>>>>>> possibility, so if it works, for now, I'd like to stick to that.
>>>>>>>>
>>>>>>>> It would need to be tested. U-boot does the change only once, while the
>>>>>>>> kernel would be doing it all the time and between various frequencies
>>>>>>>> and PLL settings. So the issues may show up with this solution too.
>>>>>>>
>>>>>>> That would have the benefit of being quite easy to document, not be a
>>>>>>> huge amount of code and it would work on all the CPUs PLLs we have so
>>>>>>> far, so still, a pretty big win. If it doesn't, of course, we don't
>>>>>>> really have the choice.
>>>>>>
>>>>>> It's probably more code though. It has to access different register from
>>>>>> the one that is already defined in dts, which would add a lot of code
>>>>>> and require dts changes. The original patch I sent is simpler than that.
>>>>>
>>>>> Why?
>>>>>
>>>>> You can use container_of to retrieve the parent structure of the clock
>>>>> notifier, and then you get a ccu_common structure pointer, with the
>>>>> CCU base address, the clock register, its lock, etc.
>>>>>
>>>>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
>>>>>
>>>>> I don't really get why anything should be changed in the DT, or why it
>>>>> would add a lot of code. Or maybe we're not talking about the same
>>>>> thing?
>>>>
>>>> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
>>>> it very liberally uses divider parameters, even in situations that are
>>>> out of spec compared to the current code in the kernel.
>>>>
>>>> In the current code and especially in the original vendor code, divider
>>>> parameters are used as last resort only. Presumably because, of the
>>>> inherent trouble in changing them, as I described to you in other email.
>>>>
>>>> The new ccu code uses dividers often and even at very high frequencies,
>>>> which goes against the spec.
>>>>
>>>> In the vendor code M is never anything else but 0, and P is used only
>>>> for frequencies below 288MHz, which matches the H3 datasheet, which says:
>>>
>>> In the vendor code, P is never used either. All the boards we had so
>>> far don't go that low, so we cannot make any of these assumptions,
>>> especially since the vendor code has had the bad habit of doing
>>> something wrong and / or useless in the past.
>>
>> P is used in the arisc firmware according to the spec for the lower
>> frequencies.
> 
> Yes, but has anyone actually tested those frequencies? Judging from
> the FEX files I could gather, cpufreq never actually goes lower than
> 480 MHz.
> 

I tested it. It works well down to 60MHz. You can even run on 24MHz if
you run directly from 24mhz osc. It's terribly slow, but it works ok.

I've rebased my working branch over the mainline kernel, which now
contains the sunxi-ng, and tested it. Cpufreq seems to work on orange pi
pc without any changes discussed in this thread. I didn't do any
extensive testing though. But it doesn't hang on boot or cpufreq config
changes.

You can see it here:

https://github.com/megous/linux/commits/orange-pi-4.8

>>> However, this implementation is a new thing, and it was using the H3
>>> precisely because of its early stage of support to use it as a testbed
>>> for the more established.
>>>
>>> If you feel like we should use a different formula to favour the
>>> multipliers over the dividers (or want to change the class of the CPU
>>> PLL to an NKM or something else, this is totally doable.
>>
>> I think the original formula that's currently in the mainline kernel is
>> better and avoids fiddling with dividers too much.
> 
> Yeah, but the older formula is not generic at all. The whole rework
> was precisely to avoid doing the whole one driver per clock that was
> just becoming a nightmare to maintain, and a pain to add support for
> new SoCs. That code will be used for A10's CPU and VE PLLs too for
> example. And they probably have the same constraints, but with
> different variations (available values of each factors for example).

Fair enough.

>>>> "The P factor only use in the condition that PLL output less than 288
>>>> MHz."
>>>
>>> And the datasheet also had some issues, either misleading or wrong
>>> comments in the past. Don't get me wrong, I'm not saying that this is
>>> wrong, just that we should not follow it religiously, and that we
>>> should trust more the experiments than the datasheet.
>>
>> I can believe that. :) Regardless, I think the reasons given for
>> avoiding dividers are quite reasonable. It's based on how PLL block
>> works, not what manual says.
> 
> Yes, indeed.
> 
> Would replacing the current factors computation function by something
> like:
> 
> for (m = 1; m < max_m; m++)
>     for (p = 1; p < max_p; p++)
>         for (n = 1; n < max_n; n++)
> 	     for (k = 1; k < max_k; k++)
> 	         if rate == computed rate
> 		     break;
> 
> work for you?

This would be better.

thank you and regards,
  Ondrej

> That way, we will favor the multipliers over the dividers, and we can
> always "blacklist" p or m (or both) by setting their maximum to 1
> (this would need an extra field in _ccu_div though).
> 
>>>> Also other datasheets of similar socs from Allwinner state that M should
>>>> not be used in production code.
>>>
>>> Which ones specifically?
>>
>> A83T for example. You can search for "only for test" phrase.
>>
>> https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf
>>
>> Those PLLs are a bit different though.
> 
> Thanks,
> Maxime
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web