Is exporting __clk_notify() acceptable?

Is exporting __clk_notify() acceptable?

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

Hi,
I'm working on implementing proper cpufreq support for Raspberry Pi and stumbled
upon an issue I'd like your opinion on:

- Raspberry Pi's boot and other internal operations are operated by a firmware
  running in it's VPU/GPU (called Video Core 4).

- Even though Raspberry Pi exports it's clock registers, we're unable to modify
  them as we could clash with the firmware. Messing around with clocks behind
  the firmware's back would potentially void the board's warranty or even break
  it. So we're stuck using a firmware frequency scaling interface the board
  provides.

- Some peripherals depend on VPU's clock (I2C, SPI, UART, ...). This clock is
  updated by the firmware interface whenever we scale the frequency.

- A solution would be to set all these clocks as non cachable, and alert the
  peripherals through the clock notifier before and after the frequency scaling
  happens. This way they could reprogram their dividers. It would be triggered
  through a custom cpufreq driver that, with the knowledge of the possible VPU
  frequencies, will pre-notify the peripherals, trigger the frequency scaling
  and post-notify the peripherals. In order to achieve this, I figured I need
  to create an exported version of __clk_notify(), which would trigger
  notifications both for the provided clock and it's children.

In principle, would it be acceptable to export the function? Any alternatives?

Note that cpufreq notifiers are not an option as they provide the CPU clock
change rates which are useless to the peripherals. Also note that the
peripherals need to know the clock rate change in advance.

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Is exporting __clk_notify() acceptable?

[Stephen Boyd]

Quoting Nicolas Saenz Julienne (2019-04-11 10:43:22)
> Hi,
> I'm working on implementing proper cpufreq support for Raspberry Pi and stumbled
> upon an issue I'd like your opinion on:

You may want to work with Eric. I think Eric has been looking at
Raspberry Pi clk things from what I can recall.

> 
> - Raspberry Pi's boot and other internal operations are operated by a firmware
>   running in it's VPU/GPU (called Video Core 4).
> 
> - Even though Raspberry Pi exports it's clock registers, we're unable to modify
>   them as we could clash with the firmware. Messing around with clocks behind
>   the firmware's back would potentially void the board's warranty or even break
>   it. So we're stuck using a firmware frequency scaling interface the board
>   provides.
> 
> - Some peripherals depend on VPU's clock (I2C, SPI, UART, ...). This clock is
>   updated by the firmware interface whenever we scale the frequency.

Ok. Got it.


> 
> - A solution would be to set all these clocks as non cachable, and alert the
>   peripherals through the clock notifier before and after the frequency scaling
>   happens. This way they could reprogram their dividers. It would be triggered
>   through a custom cpufreq driver that, with the knowledge of the possible VPU
>   frequencies, will pre-notify the peripherals, trigger the frequency scaling
>   and post-notify the peripherals. In order to achieve this, I figured I need
>   to create an exported version of __clk_notify(), which would trigger
>   notifications both for the provided clock and it's children.
> 
> In principle, would it be acceptable to export the function? Any alternatives?

I don't think we want to export __clk_notify(). If anything we've been
trying to slowly remove the notifier chains that the clk framework
implements, so exporting it would be going in the wrong direction.

> 
> Note that cpufreq notifiers are not an option as they provide the CPU clock
> change rates which are useless to the peripherals. Also note that the
> peripherals need to know the clock rate change in advance.
> 

How does it work today? Do the peripherals set clk rates under the
assumption that the parent of the clk they're dealing with (I guess VPU
clk?) isn't going to change rate?

If that's right then I see two pitfalls. The first is that we probably
want to maintain the frequency of those child clks when the VPU clk
changes rate. The second is that we should integrate the VPU clk into
the clk framework so that when it changes rate, the new frequency
propagates down to the child clks. It could still use the firmware
interface to change the VPU clk in the clk provider driver. I suppose
cpufreq-dt could be layered on top of the VPU clk too so that it all
just becomes a clk tree management problem.

After that's in place, I recommend that the downstream/child device
drivers using clks that are children of the VPU clk migrate to
clk_rate_exclusive_get() and clk_set_rate_exclusive() APIs. Once all the
child clks are "owning" the rate they want their clks to be at, then we
need to fix the clk framework rate setting code-path to let the parent
VPU clk change rates from CPUfreq and work out how to achieve the
"exclusive", i.e. "locked", rates for the children. If we do that then
we should be in good shape and we can let the VPU clk change rate while
maintaining the child clk rates for devices like UART, i2c, etc.

Re: Is exporting __clk_notify() acceptable?

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

Stephen Boyd <sboyd@kernel.org> writes:

> Quoting Nicolas Saenz Julienne (2019-04-11 10:43:22)
>> Hi,
>> I'm working on implementing proper cpufreq support for Raspberry Pi and stumbled
>> upon an issue I'd like your opinion on:
>
> You may want to work with Eric. I think Eric has been looking at
> Raspberry Pi clk things from what I can recall.

Sorry for the delay, I've been busy with the switch to my new job.  I
haven't been doing much with the clocks recently, but hopefully I can
shed some light.

>> Note that cpufreq notifiers are not an option as they provide the CPU clock
>> change rates which are useless to the peripherals. Also note that the
>> peripherals need to know the clock rate change in advance.
>> 
>
> How does it work today? Do the peripherals set clk rates under the
> assumption that the parent of the clk they're dealing with (I guess VPU
> clk?) isn't going to change rate?
>
> If that's right then I see two pitfalls. The first is that we probably
> want to maintain the frequency of those child clks when the VPU clk
> changes rate. The second is that we should integrate the VPU clk into
> the clk framework so that when it changes rate, the new frequency
> propagates down to the child clks. It could still use the firmware
> interface to change the VPU clk in the clk provider driver. I suppose
> cpufreq-dt could be layered on top of the VPU clk too so that it all
> just becomes a clk tree management problem.

Linux doesn't drive changing the VPU clock.  It's controlled by the
firmware's turbo/undervoltage/temperature management thread, and the
Foundation wants to keep it that way, unfortunately (that way all
devices have the same temperature management, rather than relying on the
OS's behavior).

We do expose the VPU (aka bus or core) clock in upstream Linux, but if
you try to read its rate you just get some sample.  Don't try
propagating a rate change through VPU clock, it will not go well (Linux
doesn't control all the leaf clocks, so it would just be flipping whose
clocks get screwed up by rate changes, and you'd be racing the
firmware's thread).

Right now in the downstream tree they set up the peripherals using the
best-case core clock, so things only get clocked down from there:

commit 6239f614fa5ac3893465f71738e031ee175be14b
Author: Phil Elwell <phil@raspberrypi.org>
Date:   Mon Mar 6 09:06:18 2017 +0000

    clk-bcm2835: Read max core clock from firmware
    
    The VPU is responsible for managing the core clock, usually under
    direction from the bcm2835-cpufreq driver but not via the clk-bcm2835
    driver. Since the core frequency can change without warning, it is
    safer to report the maximum clock rate to users of the core clock -
    I2C, SPI and the mini UART - to err on the safe side when calculating
    clock divisors.
    
    If the DT node for the clock driver includes a reference to the
    firmware node, use the firmware API to query the maximum core clock
    instead of reading the divider registers.
    
    Prior to this patch, a "100KHz" I2C bus was sometimes clocked at about
    160KHz. In particular, switching to the 4.9 kernel was likely to break
    SenseHAT usage on a Pi3.
    
    Signed-off-by: Phil Elwell <phil@raspberrypi.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]