Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

On Wed, Jan 18, 2017 at 06:48:35PM +0100, Ondřej Jirman wrote:
> Dne 18.1.2017 v 17:56 Maxime Ripard napsal(a):
> >>> What's your current plan to fix that? I guess the easiest (and most
> >>> likely to be reusable) would be to allow for clock tables, instead of
> >>> using the generic approach. We might have some other clocks (like
> >>> audio or video) that would need such a precise tuning in the future
> >>> too.
> >>
> >> My proposed solution is this for M factor (H3 specific solution):
> >>
> >> https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738
> > 
> > This one is fine.
> > 
> >> and this for P factor:
> >>
> >> https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a
> >>
> >> Perhaps it should be configurable if the P limitation is not universal
> >> for all NKMP clocks. But I haven't read all the datasheets.
> > 
> > And this is exactly what I wanted to avoid, for the reason you're
> > giving :)
> > 
> > This is some generic code, and yet you're putting a clock and SoC
> > specific limit in there. You have two ways to deal with that. Either
> > come up with some generic throttling mecanism to force a particular
> > value above or below a given threshold, but that might be tedious to
> > do, and require some significant rework.
> > 
> > Or you can use a table, which is generic and should be relatively
> > easy. I really think this is the most straightforward solution, and
> > even more so since we just want to support a limited number of
> > frequencies in this case.
> 
> So I would add a pointer to a table to struct ccu_nkmp and define the
> table itself in ccu-sun8i-h3.c?
> 
> If struct ccu_nkmp would have non-NULL pointer to a table, it would be
> used instead of the math in ccu_nkmp.c.
> 
> Sounds ok?

Yep, sounds perfect!

Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

On Wed, Jan 18, 2017 at 06:48:35PM +0100, Ondřej Jirman wrote:
> Dne 18.1.2017 v 17:56 Maxime Ripard napsal(a):
> >>> What's your current plan to fix that? I guess the easiest (and most
> >>> likely to be reusable) would be to allow for clock tables, instead of
> >>> using the generic approach. We might have some other clocks (like
> >>> audio or video) that would need such a precise tuning in the future
> >>> too.
> >>
> >> My proposed solution is this for M factor (H3 specific solution):
> >>
> >> https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738
> > 
> > This one is fine.
> > 
> >> and this for P factor:
> >>
> >> https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a
> >>
> >> Perhaps it should be configurable if the P limitation is not universal
> >> for all NKMP clocks. But I haven't read all the datasheets.
> > 
> > And this is exactly what I wanted to avoid, for the reason you're
> > giving :)
> > 
> > This is some generic code, and yet you're putting a clock and SoC
> > specific limit in there. You have two ways to deal with that. Either
> > come up with some generic throttling mecanism to force a particular
> > value above or below a given threshold, but that might be tedious to
> > do, and require some significant rework.
> > 
> > Or you can use a table, which is generic and should be relatively
> > easy. I really think this is the most straightforward solution, and
> > even more so since we just want to support a limited number of
> > frequencies in this case.
> 
> So I would add a pointer to a table to struct ccu_nkmp and define the
> table itself in ccu-sun8i-h3.c?
> 
> If struct ccu_nkmp would have non-NULL pointer to a table, it would be
> used instead of the math in ccu_nkmp.c.
> 
> Sounds ok?

Yep, sounds perfect!

Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Dne 18.1.2017 v 17:56 Maxime Ripard napsal(a):
>>> What's your current plan to fix that? I guess the easiest (and most
>>> likely to be reusable) would be to allow for clock tables, instead of
>>> using the generic approach. We might have some other clocks (like
>>> audio or video) that would need such a precise tuning in the future
>>> too.
>>
>> My proposed solution is this for M factor (H3 specific solution):
>>
>> https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738
> 
> This one is fine.
> 
>> and this for P factor:
>>
>> https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a
>>
>> Perhaps it should be configurable if the P limitation is not universal
>> for all NKMP clocks. But I haven't read all the datasheets.
> 
> And this is exactly what I wanted to avoid, for the reason you're
> giving :)
> 
> This is some generic code, and yet you're putting a clock and SoC
> specific limit in there. You have two ways to deal with that. Either
> come up with some generic throttling mecanism to force a particular
> value above or below a given threshold, but that might be tedious to
> do, and require some significant rework.
> 
> Or you can use a table, which is generic and should be relatively
> easy. I really think this is the most straightforward solution, and
> even more so since we just want to support a limited number of
> frequencies in this case.

So I would add a pointer to a table to struct ccu_nkmp and define the
table itself in ccu-sun8i-h3.c?

If struct ccu_nkmp would have non-NULL pointer to a table, it would be
used instead of the math in ccu_nkmp.c.

Sounds ok?

regards,
  o.

> Maxime
> 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Dne 18.1.2017 v 17:56 Maxime Ripard napsal(a):
>>> What's your current plan to fix that? I guess the easiest (and most
>>> likely to be reusable) would be to allow for clock tables, instead of
>>> using the generic approach. We might have some other clocks (like
>>> audio or video) that would need such a precise tuning in the future
>>> too.
>>
>> My proposed solution is this for M factor (H3 specific solution):
>>
>> https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738
> 
> This one is fine.
> 
>> and this for P factor:
>>
>> https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a
>>
>> Perhaps it should be configurable if the P limitation is not universal
>> for all NKMP clocks. But I haven't read all the datasheets.
> 
> And this is exactly what I wanted to avoid, for the reason you're
> giving :)
> 
> This is some generic code, and yet you're putting a clock and SoC
> specific limit in there. You have two ways to deal with that. Either
> come up with some generic throttling mecanism to force a particular
> value above or below a given threshold, but that might be tedious to
> do, and require some significant rework.
> 
> Or you can use a table, which is generic and should be relatively
> easy. I really think this is the most straightforward solution, and
> even more so since we just want to support a limited number of
> frequencies in this case.

So I would add a pointer to a table to struct ccu_nkmp and define the
table itself in ccu-sun8i-h3.c?

If struct ccu_nkmp would have non-NULL pointer to a table, it would be
used instead of the math in ccu_nkmp.c.

Sounds ok?

regards,
  o.

> Maxime
> 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

Hi,

On Mon, Jan 16, 2017 at 05:51:30PM +0100, Ondřej Jirman wrote:
> Hi Maxime,
> 
> Dne 16.1.2017 v 17:43 Maxime Ripard napsal(a):
> >> It does lock up quickly with mainline ccu_nkmp_find_best algorithm
> >> for finding factors.
> >>
> >> Even with linux kernel, it breaks. It's just more difficult to hit the
> >> right conditions. I got oops only right after boot when running cpuburn
> >> to trigger thermal_zone issued OPP change, if I first run some cpupower
> >> commands. That's why I wrote this program to stress test various CPU_PLL
> >> change/factor selection algorithms independently of everything else, to
> >> get more predictable and quicker testing results.
> > 
> > Understood. Do you have the code available somewhere?
> 
> It's a bit messy, but it's here:
> 
> https://github.com/megous/h3-firmware/blob/master/main.c

Awesome, thanks!

>  What works is selecting NKMP factors so that M is always 1 and P is
>  anything other than /1 only for frequencies under 288MHz. As mandated by
>  the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
>  conditions. With that I can change CPUX frequencies randomly 20x a
>  second so far indefinitely without the main CPU ever locking up.
> 
>  Please drop or revert this patch. It is not a correct approach to the
>  problem. I'd suggest dropping the entire clock notifier mechanism, too,
>  unless it can be proven to work reliably.
> >>>
> >>> It has been proven to work reliably on a number of other SoCs.
> >>
> >> Unless it was stress tested like this with randomy changed settings, I
> >> doubt you can call it reliable. It may just be very hard to hit the
> >> issue on linux with particular OPP/thermal zone configuration. That's
> >> because the issue is dependent on before and after NKMP values. People
> >> may have just been lucky so far.
> > 
> > Yes, or maybe we just have OPPs that just don't trigger a low enough P
> > factor.
> > 
> > There's no rush anyway, the H3 cpufreq support is not enabled at the
> > moment, so that code basically does nothing for the moment.
> > 
> > What's your current plan to fix that? I guess the easiest (and most
> > likely to be reusable) would be to allow for clock tables, instead of
> > using the generic approach. We might have some other clocks (like
> > audio or video) that would need such a precise tuning in the future
> > too.
> 
> My proposed solution is this for M factor (H3 specific solution):
> 
> https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738

This one is fine.

> and this for P factor:
> 
> https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a
> 
> Perhaps it should be configurable if the P limitation is not universal
> for all NKMP clocks. But I haven't read all the datasheets.

And this is exactly what I wanted to avoid, for the reason you're
giving :)

This is some generic code, and yet you're putting a clock and SoC
specific limit in there. You have two ways to deal with that. Either
come up with some generic throttling mecanism to force a particular
value above or below a given threshold, but that might be tedious to
do, and require some significant rework.

Or you can use a table, which is generic and should be relatively
easy. I really think this is the most straightforward solution, and
even more so since we just want to support a limited number of
frequencies in this case.

Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

Hi,

On Mon, Jan 16, 2017 at 05:51:30PM +0100, Ondřej Jirman wrote:
> Hi Maxime,
> 
> Dne 16.1.2017 v 17:43 Maxime Ripard napsal(a):
> >> It does lock up quickly with mainline ccu_nkmp_find_best algorithm
> >> for finding factors.
> >>
> >> Even with linux kernel, it breaks. It's just more difficult to hit the
> >> right conditions. I got oops only right after boot when running cpuburn
> >> to trigger thermal_zone issued OPP change, if I first run some cpupower
> >> commands. That's why I wrote this program to stress test various CPU_PLL
> >> change/factor selection algorithms independently of everything else, to
> >> get more predictable and quicker testing results.
> > 
> > Understood. Do you have the code available somewhere?
> 
> It's a bit messy, but it's here:
> 
> https://github.com/megous/h3-firmware/blob/master/main.c

Awesome, thanks!

>  What works is selecting NKMP factors so that M is always 1 and P is
>  anything other than /1 only for frequencies under 288MHz. As mandated by
>  the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
>  conditions. With that I can change CPUX frequencies randomly 20x a
>  second so far indefinitely without the main CPU ever locking up.
> 
>  Please drop or revert this patch. It is not a correct approach to the
>  problem. I'd suggest dropping the entire clock notifier mechanism, too,
>  unless it can be proven to work reliably.
> >>>
> >>> It has been proven to work reliably on a number of other SoCs.
> >>
> >> Unless it was stress tested like this with randomy changed settings, I
> >> doubt you can call it reliable. It may just be very hard to hit the
> >> issue on linux with particular OPP/thermal zone configuration. That's
> >> because the issue is dependent on before and after NKMP values. People
> >> may have just been lucky so far.
> > 
> > Yes, or maybe we just have OPPs that just don't trigger a low enough P
> > factor.
> > 
> > There's no rush anyway, the H3 cpufreq support is not enabled at the
> > moment, so that code basically does nothing for the moment.
> > 
> > What's your current plan to fix that? I guess the easiest (and most
> > likely to be reusable) would be to allow for clock tables, instead of
> > using the generic approach. We might have some other clocks (like
> > audio or video) that would need such a precise tuning in the future
> > too.
> 
> My proposed solution is this for M factor (H3 specific solution):
> 
> https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738

This one is fine.

> and this for P factor:
> 
> https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a
> 
> Perhaps it should be configurable if the P limitation is not universal
> for all NKMP clocks. But I haven't read all the datasheets.

And this is exactly what I wanted to avoid, for the reason you're
giving :)

This is some generic code, and yet you're putting a clock and SoC
specific limit in there. You have two ways to deal with that. Either
come up with some generic throttling mecanism to force a particular
value above or below a given threshold, but that might be tedious to
do, and require some significant rework.

Or you can use a table, which is generic and should be relatively
easy. I really think this is the most straightforward solution, and
even more so since we just want to support a limited number of
frequencies in this case.

Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Hi Maxime,

Dne 16.1.2017 v 17:43 Maxime Ripard napsal(a):
>> It does lock up quickly with mainline ccu_nkmp_find_best algorithm
>> for finding factors.
>>
>> Even with linux kernel, it breaks. It's just more difficult to hit the
>> right conditions. I got oops only right after boot when running cpuburn
>> to trigger thermal_zone issued OPP change, if I first run some cpupower
>> commands. That's why I wrote this program to stress test various CPU_PLL
>> change/factor selection algorithms independently of everything else, to
>> get more predictable and quicker testing results.
> 
> Understood. Do you have the code available somewhere?

It's a bit messy, but it's here:

https://github.com/megous/h3-firmware/blob/master/main.c

 What works is selecting NKMP factors so that M is always 1 and P is
 anything other than /1 only for frequencies under 288MHz. As mandated by
 the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
 conditions. With that I can change CPUX frequencies randomly 20x a
 second so far indefinitely without the main CPU ever locking up.

 Please drop or revert this patch. It is not a correct approach to the
 problem. I'd suggest dropping the entire clock notifier mechanism, too,
 unless it can be proven to work reliably.
>>>
>>> It has been proven to work reliably on a number of other SoCs.
>>
>> Unless it was stress tested like this with randomy changed settings, I
>> doubt you can call it reliable. It may just be very hard to hit the
>> issue on linux with particular OPP/thermal zone configuration. That's
>> because the issue is dependent on before and after NKMP values. People
>> may have just been lucky so far.
> 
> Yes, or maybe we just have OPPs that just don't trigger a low enough P
> factor.
> 
> There's no rush anyway, the H3 cpufreq support is not enabled at the
> moment, so that code basically does nothing for the moment.
> 
> What's your current plan to fix that? I guess the easiest (and most
> likely to be reusable) would be to allow for clock tables, instead of
> using the generic approach. We might have some other clocks (like
> audio or video) that would need such a precise tuning in the future
> too.

My proposed solution is this for M factor (H3 specific solution):

https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738

and this for P factor:

https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a

Perhaps it should be configurable if the P limitation is not universal
for all NKMP clocks. But I haven't read all the datasheets.

It was verified with the above arisc firmware to work reliably, so it
should be enough. It is simpler and the factor table should not be
necessary in this case.

regards,
  o.

> Maxime
> 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Hi Maxime,

Dne 16.1.2017 v 17:43 Maxime Ripard napsal(a):
>> It does lock up quickly with mainline ccu_nkmp_find_best algorithm
>> for finding factors.
>>
>> Even with linux kernel, it breaks. It's just more difficult to hit the
>> right conditions. I got oops only right after boot when running cpuburn
>> to trigger thermal_zone issued OPP change, if I first run some cpupower
>> commands. That's why I wrote this program to stress test various CPU_PLL
>> change/factor selection algorithms independently of everything else, to
>> get more predictable and quicker testing results.
> 
> Understood. Do you have the code available somewhere?

It's a bit messy, but it's here:

https://github.com/megous/h3-firmware/blob/master/main.c

 What works is selecting NKMP factors so that M is always 1 and P is
 anything other than /1 only for frequencies under 288MHz. As mandated by
 the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
 conditions. With that I can change CPUX frequencies randomly 20x a
 second so far indefinitely without the main CPU ever locking up.

 Please drop or revert this patch. It is not a correct approach to the
 problem. I'd suggest dropping the entire clock notifier mechanism, too,
 unless it can be proven to work reliably.
>>>
>>> It has been proven to work reliably on a number of other SoCs.
>>
>> Unless it was stress tested like this with randomy changed settings, I
>> doubt you can call it reliable. It may just be very hard to hit the
>> issue on linux with particular OPP/thermal zone configuration. That's
>> because the issue is dependent on before and after NKMP values. People
>> may have just been lucky so far.
> 
> Yes, or maybe we just have OPPs that just don't trigger a low enough P
> factor.
> 
> There's no rush anyway, the H3 cpufreq support is not enabled at the
> moment, so that code basically does nothing for the moment.
> 
> What's your current plan to fix that? I guess the easiest (and most
> likely to be reusable) would be to allow for clock tables, instead of
> using the generic approach. We might have some other clocks (like
> audio or video) that would need such a precise tuning in the future
> too.

My proposed solution is this for M factor (H3 specific solution):

https://github.com/megous/linux/commit/88be3d421e958579026135d8acec4b3983958738

and this for P factor:

https://github.com/megous/linux/commit/d7f274ed0c13fa9b4099445cb6bf9b2f8f2cfa8a

Perhaps it should be configurable if the P limitation is not universal
for all NKMP clocks. But I haven't read all the datasheets.

It was verified with the above arisc firmware to work reliably, so it
should be enough. It is simpler and the factor table should not be
necessary in this case.

regards,
  o.

> Maxime
> 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

Hi Ondrej,

Sorry for the late reply,

On Mon, Jan 09, 2017 at 03:50:42PM +0100, Ondřej Jirman wrote:
> Dne 9.1.2017 v 10:59 Maxime Ripard napsal(a):
> > On Sat, Jan 07, 2017 at 04:49:18PM +0100, Ondřej Jirman wrote:
> >> Maxime,
> >>
> >> Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
> >>> From: Ondrej Jirman 
> >>>
> >>> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> >>> too high, and the system becomes unstable (oopses or hangs).
> >>>
> >>> Add a notifier to avoid this situation by temporarily switching
> >>> to a known stable 24 MHz oscillator.
> >>
> >> I have done more thorough testing on H3 and this approach with switching
> >> to 24MHz oscillator does not work. Motivation being that my Orange Pi
> >> One still gets lockups even with this patch under certain circumstances.
> >>
> >> So I have created a small test program for CPUS (additional OpenRISC CPU
> >> on the SoC) which randomly changes PLL_CPUX settings while main CPU is
> >> running a loop that sends messages to CPUS via msgbox.
> >>
> >> Assumption being that while CPUS is successfully receiving messages via
> >> msgbox, the main CPU didn't lock up, yet.
> >>
> >> With this I am able to quickly and thoroughly test various PLL_CPUX
> >> change and factor selection algorithms.
> >>
> >> Results are that bypassing CPUX clock by switching to 24 MHz oscillator
> >> does not work at all. Main CPU locks up in about 1 second into the test.
> >> Don't ask me why.
> > 
> > You mean that you are changing the frequency behind Linux' back? That
> > won't work. There's more to cpufreq than just changing the frequency,
> > but also adusting the number of loops per jiffy for the new frequency
> > for example. I don't really expect that setup to work even on a
> > perfectly stable system. CPUFreq *has* to be involved, otherwise, that
> > alone might introduce bugs, and you cannot draw any conclusions
> > anymore.
> 
> No, this has nothing to do with linux. I'm not running linux for this
> test. I'm running a small program on CPUS (Open RISC CPU) on the SoC
> loaded using FEL from USB.
> 
> The main cpu is just pushing messages into msgbox in a loop, so that
> CPUS can determine that the main CPU is still running ok and give
> feedback to me over UART. Not even DRAM is involved. The programs are
> running from SRAM.
> 
> This is the most direct test of PLL change stability that can be done on
> this SoC regardless of the OS. Not even CPU voltage switching is
> involved. I just set the maximum voltage and fiddle with CPU_PLL
> frequencies randomly, while waiting for the main CPU to lock up.

Ok.

> It does lock up quickly with mainline ccu_nkmp_find_best algorithm
> for finding factors.
> 
> Even with linux kernel, it breaks. It's just more difficult to hit the
> right conditions. I got oops only right after boot when running cpuburn
> to trigger thermal_zone issued OPP change, if I first run some cpupower
> commands. That's why I wrote this program to stress test various CPU_PLL
> change/factor selection algorithms independently of everything else, to
> get more predictable and quicker testing results.

Understood. Do you have the code available somewhere?

> >> What works is selecting NKMP factors so that M is always 1 and P is
> >> anything other than /1 only for frequencies under 288MHz. As mandated by
> >> the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
> >> conditions. With that I can change CPUX frequencies randomly 20x a
> >> second so far indefinitely without the main CPU ever locking up.
> >>
> >> Please drop or revert this patch. It is not a correct approach to the
> >> problem. I'd suggest dropping the entire clock notifier mechanism, too,
> >> unless it can be proven to work reliably.
> > 
> > It has been proven to work reliably on a number of other SoCs.
> 
> Unless it was stress tested like this with randomy changed settings, I
> doubt you can call it reliable. It may just be very hard to hit the
> issue on linux with particular OPP/thermal zone configuration. That's
> because the issue is dependent on before and after NKMP values. People
> may have just been lucky so far.

Yes, or maybe we just have OPPs that just don't trigger a low enough P
factor.

There's no rush anyway, the H3 cpufreq support is not enabled at the
moment, so that code basically does nothing for the moment.

What's your current plan to fix that? I guess the easiest (and most
likely to be reusable) would be to allow for clock tables, instead of
using the generic approach. We might have some other clocks (like
audio or video) that would need such a precise tuning in the future
too.

Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

Hi Ondrej,

Sorry for the late reply,

On Mon, Jan 09, 2017 at 03:50:42PM +0100, Ondřej Jirman wrote:
> Dne 9.1.2017 v 10:59 Maxime Ripard napsal(a):
> > On Sat, Jan 07, 2017 at 04:49:18PM +0100, Ondřej Jirman wrote:
> >> Maxime,
> >>
> >> Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
> >>> From: Ondrej Jirman 
> >>>
> >>> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> >>> too high, and the system becomes unstable (oopses or hangs).
> >>>
> >>> Add a notifier to avoid this situation by temporarily switching
> >>> to a known stable 24 MHz oscillator.
> >>
> >> I have done more thorough testing on H3 and this approach with switching
> >> to 24MHz oscillator does not work. Motivation being that my Orange Pi
> >> One still gets lockups even with this patch under certain circumstances.
> >>
> >> So I have created a small test program for CPUS (additional OpenRISC CPU
> >> on the SoC) which randomly changes PLL_CPUX settings while main CPU is
> >> running a loop that sends messages to CPUS via msgbox.
> >>
> >> Assumption being that while CPUS is successfully receiving messages via
> >> msgbox, the main CPU didn't lock up, yet.
> >>
> >> With this I am able to quickly and thoroughly test various PLL_CPUX
> >> change and factor selection algorithms.
> >>
> >> Results are that bypassing CPUX clock by switching to 24 MHz oscillator
> >> does not work at all. Main CPU locks up in about 1 second into the test.
> >> Don't ask me why.
> > 
> > You mean that you are changing the frequency behind Linux' back? That
> > won't work. There's more to cpufreq than just changing the frequency,
> > but also adusting the number of loops per jiffy for the new frequency
> > for example. I don't really expect that setup to work even on a
> > perfectly stable system. CPUFreq *has* to be involved, otherwise, that
> > alone might introduce bugs, and you cannot draw any conclusions
> > anymore.
> 
> No, this has nothing to do with linux. I'm not running linux for this
> test. I'm running a small program on CPUS (Open RISC CPU) on the SoC
> loaded using FEL from USB.
> 
> The main cpu is just pushing messages into msgbox in a loop, so that
> CPUS can determine that the main CPU is still running ok and give
> feedback to me over UART. Not even DRAM is involved. The programs are
> running from SRAM.
> 
> This is the most direct test of PLL change stability that can be done on
> this SoC regardless of the OS. Not even CPU voltage switching is
> involved. I just set the maximum voltage and fiddle with CPU_PLL
> frequencies randomly, while waiting for the main CPU to lock up.

Ok.

> It does lock up quickly with mainline ccu_nkmp_find_best algorithm
> for finding factors.
> 
> Even with linux kernel, it breaks. It's just more difficult to hit the
> right conditions. I got oops only right after boot when running cpuburn
> to trigger thermal_zone issued OPP change, if I first run some cpupower
> commands. That's why I wrote this program to stress test various CPU_PLL
> change/factor selection algorithms independently of everything else, to
> get more predictable and quicker testing results.

Understood. Do you have the code available somewhere?

> >> What works is selecting NKMP factors so that M is always 1 and P is
> >> anything other than /1 only for frequencies under 288MHz. As mandated by
> >> the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
> >> conditions. With that I can change CPUX frequencies randomly 20x a
> >> second so far indefinitely without the main CPU ever locking up.
> >>
> >> Please drop or revert this patch. It is not a correct approach to the
> >> problem. I'd suggest dropping the entire clock notifier mechanism, too,
> >> unless it can be proven to work reliably.
> > 
> > It has been proven to work reliably on a number of other SoCs.
> 
> Unless it was stress tested like this with randomy changed settings, I
> doubt you can call it reliable. It may just be very hard to hit the
> issue on linux with particular OPP/thermal zone configuration. That's
> because the issue is dependent on before and after NKMP values. People
> may have just been lucky so far.

Yes, or maybe we just have OPPs that just don't trigger a low enough P
factor.

There's no rush anyway, the H3 cpufreq support is not enabled at the
moment, so that code basically does nothing for the moment.

What's your current plan to fix that? I guess the easiest (and most
likely to be reusable) would be to allow for clock tables, instead of
using the generic approach. We might have some other clocks (like
audio or video) that would need such a precise tuning in the future
too.

Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Dne 9.1.2017 v 10:59 Maxime Ripard napsal(a):
> On Sat, Jan 07, 2017 at 04:49:18PM +0100, Ondřej Jirman wrote:
>> Maxime,
>>
>> Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
>>> From: Ondrej Jirman 
>>>
>>> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
>>> too high, and the system becomes unstable (oopses or hangs).
>>>
>>> Add a notifier to avoid this situation by temporarily switching
>>> to a known stable 24 MHz oscillator.
>>
>> I have done more thorough testing on H3 and this approach with switching
>> to 24MHz oscillator does not work. Motivation being that my Orange Pi
>> One still gets lockups even with this patch under certain circumstances.
>>
>> So I have created a small test program for CPUS (additional OpenRISC CPU
>> on the SoC) which randomly changes PLL_CPUX settings while main CPU is
>> running a loop that sends messages to CPUS via msgbox.
>>
>> Assumption being that while CPUS is successfully receiving messages via
>> msgbox, the main CPU didn't lock up, yet.
>>
>> With this I am able to quickly and thoroughly test various PLL_CPUX
>> change and factor selection algorithms.
>>
>> Results are that bypassing CPUX clock by switching to 24 MHz oscillator
>> does not work at all. Main CPU locks up in about 1 second into the test.
>> Don't ask me why.
> 
> You mean that you are changing the frequency behind Linux' back? That
> won't work. There's more to cpufreq than just changing the frequency,
> but also adusting the number of loops per jiffy for the new frequency
> for example. I don't really expect that setup to work even on a
> perfectly stable system. CPUFreq *has* to be involved, otherwise, that
> alone might introduce bugs, and you cannot draw any conclusions
> anymore.

No, this has nothing to do with linux. I'm not running linux for this
test. I'm running a small program on CPUS (Open RISC CPU) on the SoC
loaded using FEL from USB.

The main cpu is just pushing messages into msgbox in a loop, so that
CPUS can determine that the main CPU is still running ok and give
feedback to me over UART. Not even DRAM is involved. The programs are
running from SRAM.

This is the most direct test of PLL change stability that can be done on
this SoC regardless of the OS. Not even CPU voltage switching is
involved. I just set the maximum voltage and fiddle with CPU_PLL
frequencies randomly, while waiting for the main CPU to lock up.

It does lock up quickly with mainline ccu_nkmp_find_best algorithm for
finding factors.

Even with linux kernel, it breaks. It's just more difficult to hit the
right conditions. I got oops only right after boot when running cpuburn
to trigger thermal_zone issued OPP change, if I first run some cpupower
commands. That's why I wrote this program to stress test various CPU_PLL
change/factor selection algorithms independently of everything else, to
get more predictable and quicker testing results.

>> What works is selecting NKMP factors so that M is always 1 and P is
>> anything other than /1 only for frequencies under 288MHz. As mandated by
>> the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
>> conditions. With that I can change CPUX frequencies randomly 20x a
>> second so far indefinitely without the main CPU ever locking up.
>>
>> Please drop or revert this patch. It is not a correct approach to the
>> problem. I'd suggest dropping the entire clock notifier mechanism, too,
>> unless it can be proven to work reliably.
> 
> It has been proven to work reliably on a number of other SoCs.

Unless it was stress tested like this with randomy changed settings, I
doubt you can call it reliable. It may just be very hard to hit the
issue on linux with particular OPP/thermal zone configuration. That's
because the issue is dependent on before and after NKMP values. People
may have just been lucky so far.

regards,
  o.

> Thanks!
> Maxime
> 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Dne 9.1.2017 v 10:59 Maxime Ripard napsal(a):
> On Sat, Jan 07, 2017 at 04:49:18PM +0100, Ondřej Jirman wrote:
>> Maxime,
>>
>> Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
>>> From: Ondrej Jirman 
>>>
>>> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
>>> too high, and the system becomes unstable (oopses or hangs).
>>>
>>> Add a notifier to avoid this situation by temporarily switching
>>> to a known stable 24 MHz oscillator.
>>
>> I have done more thorough testing on H3 and this approach with switching
>> to 24MHz oscillator does not work. Motivation being that my Orange Pi
>> One still gets lockups even with this patch under certain circumstances.
>>
>> So I have created a small test program for CPUS (additional OpenRISC CPU
>> on the SoC) which randomly changes PLL_CPUX settings while main CPU is
>> running a loop that sends messages to CPUS via msgbox.
>>
>> Assumption being that while CPUS is successfully receiving messages via
>> msgbox, the main CPU didn't lock up, yet.
>>
>> With this I am able to quickly and thoroughly test various PLL_CPUX
>> change and factor selection algorithms.
>>
>> Results are that bypassing CPUX clock by switching to 24 MHz oscillator
>> does not work at all. Main CPU locks up in about 1 second into the test.
>> Don't ask me why.
> 
> You mean that you are changing the frequency behind Linux' back? That
> won't work. There's more to cpufreq than just changing the frequency,
> but also adusting the number of loops per jiffy for the new frequency
> for example. I don't really expect that setup to work even on a
> perfectly stable system. CPUFreq *has* to be involved, otherwise, that
> alone might introduce bugs, and you cannot draw any conclusions
> anymore.

No, this has nothing to do with linux. I'm not running linux for this
test. I'm running a small program on CPUS (Open RISC CPU) on the SoC
loaded using FEL from USB.

The main cpu is just pushing messages into msgbox in a loop, so that
CPUS can determine that the main CPU is still running ok and give
feedback to me over UART. Not even DRAM is involved. The programs are
running from SRAM.

This is the most direct test of PLL change stability that can be done on
this SoC regardless of the OS. Not even CPU voltage switching is
involved. I just set the maximum voltage and fiddle with CPU_PLL
frequencies randomly, while waiting for the main CPU to lock up.

It does lock up quickly with mainline ccu_nkmp_find_best algorithm for
finding factors.

Even with linux kernel, it breaks. It's just more difficult to hit the
right conditions. I got oops only right after boot when running cpuburn
to trigger thermal_zone issued OPP change, if I first run some cpupower
commands. That's why I wrote this program to stress test various CPU_PLL
change/factor selection algorithms independently of everything else, to
get more predictable and quicker testing results.

>> What works is selecting NKMP factors so that M is always 1 and P is
>> anything other than /1 only for frequencies under 288MHz. As mandated by
>> the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
>> conditions. With that I can change CPUX frequencies randomly 20x a
>> second so far indefinitely without the main CPU ever locking up.
>>
>> Please drop or revert this patch. It is not a correct approach to the
>> problem. I'd suggest dropping the entire clock notifier mechanism, too,
>> unless it can be proven to work reliably.
> 
> It has been proven to work reliably on a number of other SoCs.

Unless it was stress tested like this with randomy changed settings, I
doubt you can call it reliable. It may just be very hard to hit the
issue on linux with particular OPP/thermal zone configuration. That's
because the issue is dependent on before and after NKMP values. People
may have just been lucky so far.

regards,
  o.

> Thanks!
> Maxime
> 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

On Sat, Jan 07, 2017 at 04:49:18PM +0100, Ondřej Jirman wrote:
> Maxime,
> 
> Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
> > From: Ondrej Jirman 
> > 
> > When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> > too high, and the system becomes unstable (oopses or hangs).
> > 
> > Add a notifier to avoid this situation by temporarily switching
> > to a known stable 24 MHz oscillator.
> 
> I have done more thorough testing on H3 and this approach with switching
> to 24MHz oscillator does not work. Motivation being that my Orange Pi
> One still gets lockups even with this patch under certain circumstances.
> 
> So I have created a small test program for CPUS (additional OpenRISC CPU
> on the SoC) which randomly changes PLL_CPUX settings while main CPU is
> running a loop that sends messages to CPUS via msgbox.
> 
> Assumption being that while CPUS is successfully receiving messages via
> msgbox, the main CPU didn't lock up, yet.
> 
> With this I am able to quickly and thoroughly test various PLL_CPUX
> change and factor selection algorithms.
> 
> Results are that bypassing CPUX clock by switching to 24 MHz oscillator
> does not work at all. Main CPU locks up in about 1 second into the test.
> Don't ask me why.

You mean that you are changing the frequency behind Linux' back? That
won't work. There's more to cpufreq than just changing the frequency,
but also adusting the number of loops per jiffy for the new frequency
for example. I don't really expect that setup to work even on a
perfectly stable system. CPUFreq *has* to be involved, otherwise, that
alone might introduce bugs, and you cannot draw any conclusions
anymore.

> What works is selecting NKMP factors so that M is always 1 and P is
> anything other than /1 only for frequencies under 288MHz. As mandated by
> the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
> conditions. With that I can change CPUX frequencies randomly 20x a
> second so far indefinitely without the main CPU ever locking up.
> 
> Please drop or revert this patch. It is not a correct approach to the
> problem. I'd suggest dropping the entire clock notifier mechanism, too,
> unless it can be proven to work reliably.

It has been proven to work reliably on a number of other SoCs.

Thanks!
Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

On Sat, Jan 07, 2017 at 04:49:18PM +0100, Ondřej Jirman wrote:
> Maxime,
> 
> Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
> > From: Ondrej Jirman 
> > 
> > When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> > too high, and the system becomes unstable (oopses or hangs).
> > 
> > Add a notifier to avoid this situation by temporarily switching
> > to a known stable 24 MHz oscillator.
> 
> I have done more thorough testing on H3 and this approach with switching
> to 24MHz oscillator does not work. Motivation being that my Orange Pi
> One still gets lockups even with this patch under certain circumstances.
> 
> So I have created a small test program for CPUS (additional OpenRISC CPU
> on the SoC) which randomly changes PLL_CPUX settings while main CPU is
> running a loop that sends messages to CPUS via msgbox.
> 
> Assumption being that while CPUS is successfully receiving messages via
> msgbox, the main CPU didn't lock up, yet.
> 
> With this I am able to quickly and thoroughly test various PLL_CPUX
> change and factor selection algorithms.
> 
> Results are that bypassing CPUX clock by switching to 24 MHz oscillator
> does not work at all. Main CPU locks up in about 1 second into the test.
> Don't ask me why.

You mean that you are changing the frequency behind Linux' back? That
won't work. There's more to cpufreq than just changing the frequency,
but also adusting the number of loops per jiffy for the new frequency
for example. I don't really expect that setup to work even on a
perfectly stable system. CPUFreq *has* to be involved, otherwise, that
alone might introduce bugs, and you cannot draw any conclusions
anymore.

> What works is selecting NKMP factors so that M is always 1 and P is
> anything other than /1 only for frequencies under 288MHz. As mandated by
> the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
> conditions. With that I can change CPUX frequencies randomly 20x a
> second so far indefinitely without the main CPU ever locking up.
> 
> Please drop or revert this patch. It is not a correct approach to the
> problem. I'd suggest dropping the entire clock notifier mechanism, too,
> unless it can be proven to work reliably.

It has been proven to work reliably on a number of other SoCs.

Thanks!
Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Maxime,

Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
> From: Ondrej Jirman 
> 
> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> too high, and the system becomes unstable (oopses or hangs).
> 
> Add a notifier to avoid this situation by temporarily switching
> to a known stable 24 MHz oscillator.

I have done more thorough testing on H3 and this approach with switching
to 24MHz oscillator does not work. Motivation being that my Orange Pi
One still gets lockups even with this patch under certain circumstances.

So I have created a small test program for CPUS (additional OpenRISC CPU
on the SoC) which randomly changes PLL_CPUX settings while main CPU is
running a loop that sends messages to CPUS via msgbox.

Assumption being that while CPUS is successfully receiving messages via
msgbox, the main CPU didn't lock up, yet.

With this I am able to quickly and thoroughly test various PLL_CPUX
change and factor selection algorithms.

Results are that bypassing CPUX clock by switching to 24 MHz oscillator
does not work at all. Main CPU locks up in about 1 second into the test.
Don't ask me why.

What works is selecting NKMP factors so that M is always 1 and P is
anything other than /1 only for frequencies under 288MHz. As mandated by
the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
conditions. With that I can change CPUX frequencies randomly 20x a
second so far indefinitely without the main CPU ever locking up.

Please drop or revert this patch. It is not a correct approach to the
problem. I'd suggest dropping the entire clock notifier mechanism, too,
unless it can be proven to work reliably. The bypass makes some
intuitive sense, but for some reason it doesn't work in practice (on H3
at least).

Aside from this, uboot also needs to be changed to set that it uses M
and P factors correctly.

Whatever else I try, I always hit lockups sooner or later with the test
program. I tried 24MHz bypass and staged application of multipliers and
dividers as discussed before.

I'll send a proper patch for nkmp clock driver and u-boot later.

regards,
  o.

> Signed-off-by: Ondrej Jirman 
> Tested-by: Lutz Sammer 
> ---
>  drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c 
> b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> index 614d47c..cf266c9 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> @@ -809,6 +809,13 @@ static const struct sunxi_ccu_desc sun8i_h3_ccu_desc = {
>   .num_resets = ARRAY_SIZE(sun8i_h3_ccu_resets),
>  };
>  
> +static struct ccu_mux_nb sun8i_h3_cpu_nb = {
> + .common = _clk.common,
> + .cm = _clk.mux,
> + .delay_us   = 1, /* > 8 clock cycles at 24 MHz */
> + .bypass_index   = 1, /* index of 24 MHz oscillator */
> +};
> +
>  static void __init sun8i_h3_ccu_setup(struct device_node *node)
>  {
>   void __iomem *reg;
> @@ -827,6 +834,9 @@ static void __init sun8i_h3_ccu_setup(struct device_node 
> *node)
>   writel(val | (3 << 16), reg + SUN8I_H3_PLL_AUDIO_REG);
>  
>   sunxi_ccu_probe(node, reg, _h3_ccu_desc);
> +
> + ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
> +   _h3_cpu_nb);
>  }
>  CLK_OF_DECLARE(sun8i_h3_ccu, "allwinner,sun8i-h3-ccu",
>  sun8i_h3_ccu_setup);
> 



signature.asc
Description: OpenPGP digital signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Ondřej Jirman]

Maxime,

Dne 25.11.2016 v 01:28 meg...@megous.com napsal(a):
> From: Ondrej Jirman 
> 
> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> too high, and the system becomes unstable (oopses or hangs).
> 
> Add a notifier to avoid this situation by temporarily switching
> to a known stable 24 MHz oscillator.

I have done more thorough testing on H3 and this approach with switching
to 24MHz oscillator does not work. Motivation being that my Orange Pi
One still gets lockups even with this patch under certain circumstances.

So I have created a small test program for CPUS (additional OpenRISC CPU
on the SoC) which randomly changes PLL_CPUX settings while main CPU is
running a loop that sends messages to CPUS via msgbox.

Assumption being that while CPUS is successfully receiving messages via
msgbox, the main CPU didn't lock up, yet.

With this I am able to quickly and thoroughly test various PLL_CPUX
change and factor selection algorithms.

Results are that bypassing CPUX clock by switching to 24 MHz oscillator
does not work at all. Main CPU locks up in about 1 second into the test.
Don't ask me why.

What works is selecting NKMP factors so that M is always 1 and P is
anything other than /1 only for frequencies under 288MHz. As mandated by
the H3 datasheet. Mainline ccu_nkmp_find_best doesn't respect these
conditions. With that I can change CPUX frequencies randomly 20x a
second so far indefinitely without the main CPU ever locking up.

Please drop or revert this patch. It is not a correct approach to the
problem. I'd suggest dropping the entire clock notifier mechanism, too,
unless it can be proven to work reliably. The bypass makes some
intuitive sense, but for some reason it doesn't work in practice (on H3
at least).

Aside from this, uboot also needs to be changed to set that it uses M
and P factors correctly.

Whatever else I try, I always hit lockups sooner or later with the test
program. I tried 24MHz bypass and staged application of multipliers and
dividers as discussed before.

I'll send a proper patch for nkmp clock driver and u-boot later.

regards,
  o.

> Signed-off-by: Ondrej Jirman 
> Tested-by: Lutz Sammer 
> ---
>  drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c 
> b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> index 614d47c..cf266c9 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> @@ -809,6 +809,13 @@ static const struct sunxi_ccu_desc sun8i_h3_ccu_desc = {
>   .num_resets = ARRAY_SIZE(sun8i_h3_ccu_resets),
>  };
>  
> +static struct ccu_mux_nb sun8i_h3_cpu_nb = {
> + .common = _clk.common,
> + .cm = _clk.mux,
> + .delay_us   = 1, /* > 8 clock cycles at 24 MHz */
> + .bypass_index   = 1, /* index of 24 MHz oscillator */
> +};
> +
>  static void __init sun8i_h3_ccu_setup(struct device_node *node)
>  {
>   void __iomem *reg;
> @@ -827,6 +834,9 @@ static void __init sun8i_h3_ccu_setup(struct device_node 
> *node)
>   writel(val | (3 << 16), reg + SUN8I_H3_PLL_AUDIO_REG);
>  
>   sunxi_ccu_probe(node, reg, _h3_ccu_desc);
> +
> + ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
> +   _h3_cpu_nb);
>  }
>  CLK_OF_DECLARE(sun8i_h3_ccu, "allwinner,sun8i-h3-ccu",
>  sun8i_h3_ccu_setup);
> 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

On Fri, Nov 25, 2016 at 01:28:47AM +0100, meg...@megous.com wrote:
> From: Ondrej Jirman 
> 
> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> too high, and the system becomes unstable (oopses or hangs).
> 
> Add a notifier to avoid this situation by temporarily switching
> to a known stable 24 MHz oscillator.
> 
> Signed-off-by: Ondrej Jirman 
> Tested-by: Lutz Sammer 

Applied, thanks!
Maxime

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


signature.asc
Description: PGP signature

Re: [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Maxime Ripard]

On Fri, Nov 25, 2016 at 01:28:47AM +0100, meg...@megous.com wrote:
> From: Ondrej Jirman 
> 
> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> too high, and the system becomes unstable (oopses or hangs).
> 
> Add a notifier to avoid this situation by temporarily switching
> to a known stable 24 MHz oscillator.
> 
> Signed-off-by: Ondrej Jirman 
> Tested-by: Lutz Sammer 

Applied, thanks!
Maxime

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


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Chen-Yu Tsai]

On Fri, Nov 25, 2016 at 8:28 AM,   wrote:
> From: Ondrej Jirman 
>
> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> too high, and the system becomes unstable (oopses or hangs).
>
> Add a notifier to avoid this situation by temporarily switching
> to a known stable 24 MHz oscillator.
>
> Signed-off-by: Ondrej Jirman 
> Tested-by: Lutz Sammer 

A Fixes tag would be nice. Otherwise,

Acked-by: Chen-Yu Tsai 

Re: [linux-sunxi] [PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[Chen-Yu Tsai]

On Fri, Nov 25, 2016 at 8:28 AM,   wrote:
> From: Ondrej Jirman 
>
> When adjusting PLL_CPUX on H3, the PLL is temporarily driven
> too high, and the system becomes unstable (oopses or hangs).
>
> Add a notifier to avoid this situation by temporarily switching
> to a known stable 24 MHz oscillator.
>
> Signed-off-by: Ondrej Jirman 
> Tested-by: Lutz Sammer 

A Fixes tag would be nice. Otherwise,

Acked-by: Chen-Yu Tsai 

[PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[megous]

From: Ondrej Jirman 

When adjusting PLL_CPUX on H3, the PLL is temporarily driven
too high, and the system becomes unstable (oopses or hangs).

Add a notifier to avoid this situation by temporarily switching
to a known stable 24 MHz oscillator.

Signed-off-by: Ondrej Jirman 
Tested-by: Lutz Sammer 
---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c 
b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 614d47c..cf266c9 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -809,6 +809,13 @@ static const struct sunxi_ccu_desc sun8i_h3_ccu_desc = {
.num_resets = ARRAY_SIZE(sun8i_h3_ccu_resets),
 };
 
+static struct ccu_mux_nb sun8i_h3_cpu_nb = {
+   .common = _clk.common,
+   .cm = _clk.mux,
+   .delay_us   = 1, /* > 8 clock cycles at 24 MHz */
+   .bypass_index   = 1, /* index of 24 MHz oscillator */
+};
+
 static void __init sun8i_h3_ccu_setup(struct device_node *node)
 {
void __iomem *reg;
@@ -827,6 +834,9 @@ static void __init sun8i_h3_ccu_setup(struct device_node 
*node)
writel(val | (3 << 16), reg + SUN8I_H3_PLL_AUDIO_REG);
 
sunxi_ccu_probe(node, reg, _h3_ccu_desc);
+
+   ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
+ _h3_cpu_nb);
 }
 CLK_OF_DECLARE(sun8i_h3_ccu, "allwinner,sun8i-h3-ccu",
   sun8i_h3_ccu_setup);
-- 
2.10.2

[PATCH] clk: sunxi-ng: fix PLL_CPUX adjusting on H3

[megous]

From: Ondrej Jirman 

When adjusting PLL_CPUX on H3, the PLL is temporarily driven
too high, and the system becomes unstable (oopses or hangs).

Add a notifier to avoid this situation by temporarily switching
to a known stable 24 MHz oscillator.

Signed-off-by: Ondrej Jirman 
Tested-by: Lutz Sammer 
---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c 
b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 614d47c..cf266c9 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -809,6 +809,13 @@ static const struct sunxi_ccu_desc sun8i_h3_ccu_desc = {
.num_resets = ARRAY_SIZE(sun8i_h3_ccu_resets),
 };
 
+static struct ccu_mux_nb sun8i_h3_cpu_nb = {
+   .common = _clk.common,
+   .cm = _clk.mux,
+   .delay_us   = 1, /* > 8 clock cycles at 24 MHz */
+   .bypass_index   = 1, /* index of 24 MHz oscillator */
+};
+
 static void __init sun8i_h3_ccu_setup(struct device_node *node)
 {
void __iomem *reg;
@@ -827,6 +834,9 @@ static void __init sun8i_h3_ccu_setup(struct device_node 
*node)
writel(val | (3 << 16), reg + SUN8I_H3_PLL_AUDIO_REG);
 
sunxi_ccu_probe(node, reg, _h3_ccu_desc);
+
+   ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
+ _h3_cpu_nb);
 }
 CLK_OF_DECLARE(sun8i_h3_ccu, "allwinner,sun8i-h3-ccu",
   sun8i_h3_ccu_setup);
-- 
2.10.2