hmc7044: add dynamic PLL2 rate selection via CCF#3283
Conversation
Register pll2 as parent in the case device is hmc7044. This will enable control and propagation of clock changes to sibling channels and parent clock within the device. Support clock framework operations for the added pll2 clock. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Make the pll2 setting calculations a separate function. Calculating appropriate settings and achievable frequencies as a separate request is needed when controlling the clock via the clock framework. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
e6dc2c0 to
c153c5b
Compare
Add new set-rate-parent property. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Add a new property to make channel rate setting possible to propagate to parent. The escalation happens when a frequency that is not directly achievable is requested. Since not all requested frequencies are necessarily achievable by all channels at once, this feature allows to describe which channels should be prioritized. Channels that lack this property will work as before. That is, only operate within the range of divider settings available, and with constant parent frequency rate. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Implementation for both determine_rate and round_rate operations are typically not required. In this case supporting both is not needed and also would require extending current implementations to both support parent rate setting. To avoid any conflicts with this, drop determine rate in favor of the round rate implementation. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Split functionality to register clocks separately in preparation to support pll2 clock operations dynamically. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Round rate operation will be called to determine whether a requested frequency can be fulfilled. Here we add support to also calculate a suitable new parent rate for the case where parent rate propagation has been set for the specific clock being changed. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Relay writes to common clock framework when channel frequency settings are altered via sysfs interface. This allows to escalate frequency changes to parent clocks and do other sanity checks to the frequency being requested, in a common fashion. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
When requesting zero as output frequency for a specific channel, set the channel register to disabled. Also add the support codes for avoiding division by zero. Setting a frequency different than zero re-enables the channels with other settings restored. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Add the adi,max-deviation-ppm property. When set on a channel, any PLL2 rate change requested by a CLK_SET_RATE_PARENT channel is rejected if it would shift this channel's output frequency by more than the specified number of parts per million from its current rate. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
Account for maximum allowed deviations when setting frequency. Logic works accordingly: If set-rate-parent is enabled for a specific channel, a new parent pll2 rate will be looked for when the frequency does not match an allowed divider. If the parent clock rate changes, this implies that the accuracy of other channels might change. Therefore, check all other channel's allowed deviations. If a potential parent rate that fulfills the requested rate for the channel does not meet deviation requirements of the other channels, skip it and go to next candidate. This check applies to all channels that are not currently disabled. If any of the channel deviation requirements fail, the requested frequency will be rejected. Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
|
c153c5b to
30964aa
Compare
nunojsa
left a comment
There was a problem hiding this comment.
Thanks again for contributing. Some remarks from me
| [3] = "clkin3", | ||
| }; | ||
|
|
||
| static int hmc7044_setup(struct iio_dev *indio_dev); |
There was a problem hiding this comment.
Juts prefer that you move the function if really needed.
| static unsigned long hmc7044_pll2_recalc_rate(unsigned long vcxo_freq, | ||
| unsigned long pll2_freq, | ||
| bool force_r2_eq_1, | ||
| struct hmc7044_pll2_config *pll2) |
There was a problem hiding this comment.
logically, the struct should be the first argument
| unsigned long r2_max; | ||
|
|
||
| vcxo_freq = vcxo_freq / 1000; | ||
| pll2_freq = pll2_freq / 1000; |
There was a problem hiding this comment.
Will we gain something from DIV_ROUND_CLOSEST()?
There was a problem hiding this comment.
I thin that wouldn't gain anything here. The frequencies are exact kHz multiples so it would give the same result.
| pll2_freq = pll2_freq / 1000; | ||
|
|
||
| if (pll2_freq < HMC7044_LOW_VCO_MIN || | ||
| pll2_freq > HMC7044_HIGH_VCO_MAX) |
There was a problem hiding this comment.
I would just put it in one line
| while ((n2[0] < HMC7044_N2_MIN) && (r2[0] <= HMC7044_R2_MAX / 2)) { | ||
| n2[0] *= 2; | ||
| r2[0] *= 2; | ||
| } |
|
|
||
| /* don't propagate rate change to parent*/ | ||
| if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) | ||
| return oldrate; |
There was a problem hiding this comment.
Hmm I get the don't propagate the parent rate thing but why do we return oldrate? is it because if we can't change the parent than we can assume no new rate will work? Maybe I'm missing something
There was a problem hiding this comment.
It's because shortly before, based on the requested rate, we have calculcated what the rate will be with the current PLL2 frequency. As we are not changing that, oldrate is what we can achieve.
| struct hmc7044_chan_spec { | ||
| unsigned int num; | ||
| bool disable; | ||
| bool mute; |
There was a problem hiding this comment.
Hmm don't really like this kind of "write 0" to disable. If that isn't an allowed value we should just error out instead of doing this kind of stuff, Both CCF and IIO have explicit things to disable channels or clocks
| if (deviation < min_dev) { | ||
| min_dev = deviation; | ||
| deviation_ppm = abs(rate - newrate) * | ||
| 1000000 / rate; |
There was a problem hiding this comment.
nit. More readable if it goes to the same line
| div = hmc7044_calc_out_div(pll2_candidate, current_rate); | ||
| candidate_rate = DIV_ROUND_CLOSEST(pll2_candidate, div); | ||
| deviation_ppm = abs(current_rate - candidate_rate) * | ||
| 1000000 / current_rate; |
There was a problem hiding this comment.
Would also put it in the same line unless we pass the 101 col limit
LLM reviewThis series adds PLL2 clock framework integration to the HMC7044 jitter attenuator driver, enabling dynamic PLL2 rate changes, channel muting via zero-rate, and per-channel deviation guards ( run: 25484988610
|
|
AI complain seems valid |
|
PR Description
This series extends the HMC7044 driver to expose PLL2 as a CCF clock and support dynamic PLL2 rate selection. When a requested output frequency cannot be satisfied by an integer divider alone, the driver searches for a PLL2 rate that best approximates the target. Per-channel deviation budgets can be specified to protect other outputs from excessive frequency error when PLL2 is retuned.
Tested on zcu102 with modifications on top of main branch. Noting that curiously jesd204 link does not come up successfully with main branch, but situation is same with or without these changes applied.
Signed-off-by: Tomas Melin tomas.melin@vaisala.com
PR Type
PR Checklist