Re: [PATCH v6 2/3] clk: cs2600: Add Fractional-N clock driver

[Brian Masney <bmasney@redhat.com>]

Hi Richard,

Thanks for the patch.

On Tue, Jun 30, 2026 at 04:55:48PM +0100, Richard Fitzgerald wrote:
> From: Paul Handrigan <paulha@opensource.cirrus.com>
> 
> Add driver for the Cirrus Logic CS2600 fractional-N clock synthesizer
> and multiplier.
> 
> The CS2600 is a system-clocking device using a hybrid fractional-N analog
> PLL and a digital frequency-locked loop (FLL). The CS2600 enables frequency
> synthesis and clock generation from a stable timing reference clock. The
> device can generate low-jitter clocks between 6 MHz and 75 MHz from a noisy
> clock reference between 50 Hz and 30 MHz, or from a stable clock reference
> between 8 MHz and 75 MHz.
> 
> The timing reference clock (REF_CLK_IN) can be an external clock source
> or the internal oscillator. This clock drives the PLL and is always
> required to generate output clocks, even if it is not used as the frequency
> reference.
> 
> The frequency reference source can be either REF_CLK_IN or an external
> clock on CLK_IN.
> 
> The PLL_OUT clock frequency is a ratio of the chosen frequency reference.
> The ratio is a fixed-point number of either 12.20 or 20.12 precision. The
> lower-precision high-multiplication 20.12 ratios are only available when
> CLK_IN is the frequency reference.
> 
> In smart mode the CS2600 will synthesize the output based on REF_CLK_IN
> until a clock is present on CLK_IN; it then performs a glitchless switch
> to CLK_IN as the frequency reference. A variant of smart mode
> automatically enables the PLL output only when CLK_IN becomes available.
> 
> Three output pins are derived from PLL_OUT:
> 
>   CLK_OUT:   Gated output from PLL_OUT.
>   BCLK_OUT:  Divided from PLL_OUT.
>   FSYNC_OUT: Divided from PLL_OUT.
> 
> The BCLK_OUT and FSYNC_OUT have different sets of available divide ratios
> intended to provide the typical frequency relation between bit clock and
> frame clock on I2S/TDM audio interfaces. They can also be inverted with
> respect to CLK_OUT to support various formats of serial audio data.
> FSYNC_OUT has the additional feature that it can be either a 50:50 duty
> cycle or a pulse of a programmable number of BCLK cycles.
> 
> This configuration is modeled in the driver as:
> 
>                  +------------+
>   REF_CLK_IN ----| Hybrid PLL | PLL_OUT    +--------+
>                  |   + FLL    |-----+------|  GATE  |--------- CLK_OUT
>       CLK_IN ----|            |     |      +--------+
>                  +------------+     |
>                                     |      +---------+
>                                     +------| DIVIDER |-------- BCLK_OUT
>                                     |      +---------+
>                                     |
>                                     |      +---------+
>                                     +------| DIVIDER |-------- FSYNC_OUT
>                                            +---------+
> 
> OF properties define the hardware-level configuration to match the
> hardware that the CS2600 is driving.
> 
> The CS2600_ERR_xxx register bits are not used in the driver but are
> defined in the header file as a convenience for anyone debugging a
> system.
> 
> Signed-off-by: Paul Handrigan <paulha@opensource.cirrus.com>
> Co-developed-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> 
> Changes in V6:
> 
> - Merged the functions to calculate synth mode and mult mode ratios. The
>   math is the same, only the fixed-point precision is different. The new
>   cs2600_calc_ratio() function calculates at 20:20 fixed point and then
>   converts to 20:12 if the value is too large for 12:20.
>   This also simplifies the calling code because it doesn't need all the
>   checks of which ratio function it should be calling.
> - Re-worked the rounding of calculated rates so that they didn't drift
>   away from the best rate when the clock core passed the rounded rate from
>   determine_rate() back into set_rate().
> - Changed various parts of the recalc_rate()/determine_rate()/set_rate()
>   functions that relied on the state of the other clock registers to make
>   decisions. There's no guarantee whether those other clocks have been
>   configured yet so the decision making could be invalid.
> - Removed the use of different ratio slots depending on the mode. The
>   The CS2600 doesn't care which slots are used for synth and mult ratio.
>   Using fixed slots simplifies the code.
> - Split the large cs2600_pll_out_prepare() into helper functions.
> - Request ref_clk when starting the PLL. It is the system clock of the
>   CS2600 so it is always required to clock the chip even if it isn't the
>   frequency reference parent. In previous versions ref_clk was only
>   enabled by being the parent, which meant the CS2600 wasn't clocked when
>   using mult mode with clk_in as the frequency reference parent.
> - Fix cs2600_pll_out_set_parent() to properly reconfigure the PLL when
>   changing parents. The parent selection isn't a simple mux, it changes
>   PLL mode and selected ratio registers. Previously the mode bit was
>   written but the mode ratio registers weren't updated to match.
> - Factor out the BCLK and FSYNC divider lookup into a single function. The
>   lookup code is the same apart from the divide value that each register
>   field value represents. The divide values are provided as an array.
> - Don't attempt to set a best_parent_rate when setting BCLK/FSYNC rate.
>   The parent (CLK_OUT) rate is not arbitrary, it must match the requirements
>   of the device being clocked, and only the consumer driver will know
>   what is correct.
> - Set CLK_SET_PARENT_GATE on the PLL. Its configuration cannot be updated
>   atomically, so it must be stopped to reconfigure it.
> - Split several functions into separate functions for calculating a value
>   and get/set the register.
> - Use FIELD_PREP() and FIELD_GET() instead of multiple custom macros.
> - Fix inverted register unfreeze and freeze that meant the bits affected
>   by FREEZE_EN were not actually updated.
> - Use a DEFINE_GUARD to implement the register field unfreeze.
> - Rename cs2600_ref_clk_bound_rate() to cs2600_ref_clk_set_divider() to
>   indicate it's actually setting the rate, not just finding a rounded value.
> - Remove inappropriate use of in_range() that required some confusing
>   math to pass the correct value into in_range(). It's clearer to do
>   normal min/max comparison.
> - Use DIV_ROUND_UP_ULL() to convert PPM to a frequency delta instead of
>   open-coding it.
> - Initialize all the clocks using a single common loop.
> - Use clk_init_data.parent_hws to set FSYNC, BCLK and CLK_OUT parent as
>   PLL_OUT, instead of looking up PLL_OUT by name and setting FSYNC, BCLK
>   and CLK_OUT parent to that name.
> - Smart mode selection is now two boolean properties instead of an enum
>   property. One property select smart mode, the other property enables
>   suppression of PLL_OUT until there is clock on CLK_IN.
> - Added properties to invert FSYNC and BCLK relative to CLK_OUT.
> - Added property to set the duty cycle of FSYNC (required when clocking
>   TDM audio formats).
> - Use match_string() instead of a string of strcmp() to lookup
>   cirrus,aux1-output-source value.
> - Use of the internal oscillator is now explictly flagged by the presence
>   of the cirrus,internal-oscillator property. Previously the driver
>   inferred it from the absence of a ref_clk in clock-names.
> - More error checking.
> 
> Changes before V6:
> Sorry, the previously upstreamed versions don't have a changelist for
> me to copy into here.
> 
>  MAINTAINERS                     |    1 +
>  drivers/clk/Kconfig             |    1 +
>  drivers/clk/Makefile            |    1 +
>  drivers/clk/cirrus/Kconfig      |   10 +
>  drivers/clk/cirrus/Makefile     |    3 +
>  drivers/clk/cirrus/clk-cs2600.c | 1274 +++++++++++++++++++++++++++++++
>  drivers/clk/cirrus/clk-cs2600.h |  163 ++++
>  7 files changed, 1453 insertions(+)
>  create mode 100644 drivers/clk/cirrus/Kconfig
>  create mode 100644 drivers/clk/cirrus/Makefile
>  create mode 100644 drivers/clk/cirrus/clk-cs2600.c
>  create mode 100644 drivers/clk/cirrus/clk-cs2600.h
> 

[snip]

> diff --git a/drivers/clk/cirrus/clk-cs2600.c b/drivers/clk/cirrus/clk-cs2600.c
> new file mode 100644
> index 000000000000..73937ac868fc
> --- /dev/null
> +++ b/drivers/clk/cirrus/clk-cs2600.c

[snip]

> +
> +DEFINE_GUARD(cs2600_unfreeze, struct cs2600 *,
> +	     cs2600_clear_freeze(_T), cs2600_set_freeze(_T))

Can you go through the Sashiko output for this series?

https://sashiko.dev/#/message/20260630161121.020FB1F000E9%40smtp.kernel.org

The clear/set inverted looks questionable to me. If this is expected,
then a comment clarifying why should be added.

[snip]

> +static int cs2600_clk_register(struct cs2600 *cs2600)
> +{
> +	static const char *parent_names[2];

I wouldn't worry too about about Sashiko asking about multiple driver
instances for this section, unless you think it will eventually be used
in that way.

> +	const struct clk_hw *out_clocks_parent;
> +	struct clk_init_data init[ARRAY_SIZE(cs2600->hw)] = { };
> +	int ret, i, n, input_num = 0;
> +	const char *names[ARRAY_SIZE(cs2600_clks)];

Reverse Christmas tree order.

I can do a more thorough review once you go through the Sashiko results.

Brian