Proposal changes to the ClockInterface trait

[Ioan-Cristian]

Currently, the ClockInterface trait inside kernel/src/platform/chip.rs allows enabling/disabling a clock and querying whether the clock is enabled or not. However, there are situations when one needs to retrieve the frequency of the clock. For instance, the UART peripheral requires knowledge of the input clock in order to configure itself for a certain baud rate. This issue proposes the following solution:

/// Generic operations that clock-like things are expected to support.
pub trait ClockInterface {
    fn is_enabled(&self) -> bool;
    fn enable(&self);
    fn disable(&self);
    // Returns None if the clock is stopped, Some(frequency_hz) otherwise
    fn get_frequency(&self) -> Option<u32>;
}

The associated function returns None if the clock is stopped or Some(frequency). The frequency is expressed in Hertz. The actual frequency might vary depending on the source clock quality.

Currently, sam4l, stm32f303xc, stm32f4xx and imxrt10xx crates rely on this trait. The addition doesn't break any current implementation.

[ppannuto]

We have a Frequency trait in the time HIL currently, and a somewhat thorough discussion of time-related concerns in TRD105. Should this be the same Frequency type? I could see both sides of this...

[ppannuto]

https://www.ewsn.org/file-repository/ewsn2021/Article13.pdf

This is the paper discussed on the call.

---

This comment, #3333 (comment), from Leon's old PR also likely relevant

[bradjc]

I don't think it makes sense to have a clock with a frequency of None. Either the default noop implementation should just return 0, or getting the entire ClockInterface from the peripheral manager should return an Option.

[Ioan-Cristian]

Hmm... Returning 0 when a clock is disabled can lead to divisions by 0. I think an even better signature for get_frequency is the following one:

/// Generic operations that clock-like things are expected to support.
pub trait ClockInterface {
    /* ... */
    // Returns None if the clock is stopped, Some(frequency_hz) otherwise
    fn get_frequency(&self) -> Option<NonZeroU32>;
}

Using NonZeroU32 allows the compiler to optimize the memory layout. Any thoughts?

[bradjc]

Using NonZeroU32 allows the compiler to optimize the memory layout. Any thoughts?

I don't understand what a None means. The clock doesn't have a frequency? Then it's not a clock...

Every call to get_frequency() is just going to look like clock.get_frequency().unwrap() as usize, pushing the panic-ability to correct uses of ClockInterface, while peripherals that don't use a clock won't call get_frequency() at all.

[Ioan-Cristian]

A disabled clock is still a clock. The method's comment indicates that None is returned when the clock is disabled.

[bradjc]

I guess we need to establish what we want to be able to do with this interface. Currently it seems to be designed for optimizing power by enabling clocks only when they are needed.

Then there is configuring peripherals based on the actual frequency of the clock.

There is also configuring peripherals based on a clock where the frequency can change dynamically at runtime.

Finally, it seems like there is getting runtime status of the clock (ie its current frequency, which could be 0 if it is not running).

Those are related but not identical goals. Returning None from get_frequency seems like a function in a ClockStatus trait, but that doesn't seem like the trait a chip driver developer would want for configuring the UART peripheral. They just want to know what the clock will run at when enabled.

[Ioan-Cristian]

Hmm... If I remember correctly the discussion around this issue, the problem was about peripheral initialization for static clock configuration. For instance, when I was working on the Ethernet, I enabled a higher system clock required by PHY. However, the UART configuration was hard coded as for the default system configuration. With get_frequency(), a peripheral may change its initialization parameters at boot time depending on the given clock frequency, i.e. the UART would adapt to an increate/decrease of the system's clock to maintain the desired baud rate (115200 in most cases). The proposed change tries to address this issue.

Again, if I remember correctly, dynamic clock configuration was not consired because currently Tock lacks that infrastructure.

[ppannuto]

Is the solution simply to make the function get_configured_clock_frequency() -> NonZeroU32 [or Frequency]?

We already have the is_enabled() method in that interface to answer whether the clock is powered on or not. Most clocks that I'm familiar with configure power on/off state independent of the divider register. Similarly, the given use case of setting baud depends on what the clock will be when it's running; whether the clock is currently enabled is irrelevant.

Every clock has a frequency it's configured to run at [based on its configured clock source and divider in the clock's status registers] at all times, regardless of current on/off state, which eliminates the need for None.