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

[OndÅej Jirman]

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
>

Attachment: signature.asc
Description: OpenPGP digital signature