Re: [PATCH v9 04/10] clk: Add Lynx 10G SerDes PLL driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting Sean Anderson (2022-12-29 16:01:33)
> This adds support for the PLLs found in Lynx 10G "SerDes" devices found on
> various NXP QorIQ SoCs. There are two PLLs in each SerDes. This driver has
> been split from the main PHY driver to allow for better review, even though
> these PLLs are not present anywhere else besides the SerDes. An auxiliary
> device is not used as it offers no benefits over a function call (and there
> is no need to have a separate device).
> 
> The PLLs are modeled as clocks proper to let us take advantage of the
> existing clock infrastructure.

What advantage do we gain?

> I have not given the same treatment to the
> per-lane clocks because they need to be programmed in-concert with the rest
> of the lane settings. One tricky thing is that the VCO (PLL) rate exceeds
> 2^32 (maxing out at around 5GHz). This will be a problem on 32-bit
> platforms, since clock rates are stored as unsigned longs. To work around
> this, the pll clock rate is generally treated in units of kHz.

This looks like a disadvantage. Are we reporting the frequency in kHz to
the clk framework?

> 
> The PLLs are configured rather interestingly. Instead of the usual direct
> programming of the appropriate divisors, the input and output clock rates
> are selected directly. Generally, the only restriction is that the input
> and output must be integer multiples of each other. This suggests some kind
> of internal look-up table. The datasheets generally list out the supported
> combinations explicitly, and not all input/output combinations are
> documented. I'm not sure if this is due to lack of support, or due to an
> oversight. If this becomes an issue, then some combinations can be
> blacklisted (or whitelisted). This may also be necessary for other SoCs
> which have more stringent clock requirements.

I'm wondering if a clk provider should be created at all here. Who is
the consumer of the clk? The phy driver itself? Does the clk provided
need to interact with other clks in the system? Or is the clk tree
wholly self-contained?

Can the phy consumer configure the output frequency directly via
phy_configure() or when the phy is enabled? I'm thinking the phy driver
can call clk_set_rate() on the parent 'rfclk' before or after setting
the bits to control the output rate, and use clk_round_rate() to figure
out what input frequencies are supported for the output frequency
desired. This would avoid kHz overflowing 32-bits, and the big clk lock
getting blocked on some other clk in the system changing rates.

BTW, what sort of phy is this? Some networking device?

> 
> diff --git a/drivers/clk/clk-fsl-lynx-10g.c b/drivers/clk/clk-fsl-lynx-10g.c
> new file mode 100644
> index 000000000000..61f68b5ae675
> --- /dev/null
> +++ b/drivers/clk/clk-fsl-lynx-10g.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
> + *
> + * This file contains the implementation for the PLLs found on Lynx 10G phys.
> + *
> + * XXX: The VCO rate of the PLLs can exceed ~4GHz, which is the maximum rate
> + * expressable in an unsigned long. To work around this, rates are specified in
> + * kHz. This is as if there was a division by 1000 in the PLL.
> + */
> +
> +#include <linux/clk.h>

Is this include used? If not, please remove.

> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/bitfield.h>
> +#include <linux/math64.h>
> +#include <linux/phy/lynx-10g.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/units.h>
> +#include <dt-bindings/clock/fsl,lynx-10g.h>
> +
> +#define PLL_STRIDE     0x20
> +#define PLLa(a, off)   ((a) * PLL_STRIDE + (off))
> +#define PLLaRSTCTL(a)  PLLa(a, 0x00)
> +#define PLLaCR0(a)     PLLa(a, 0x04)
> +
> +#define PLLaRSTCTL_RSTREQ      BIT(31)
> +#define PLLaRSTCTL_RST_DONE    BIT(30)
> +#define PLLaRSTCTL_RST_ERR     BIT(29)
[...]
> +
> +static int lynx_clk_init(struct clk_hw_onecell_data *hw_data,
> +                        struct device *dev, struct regmap *regmap,
> +                        unsigned int index)
> +{
> +       const struct clk_hw *ex_dly_parents;
> +       struct clk_parent_data pll_parents[1] = { };
> +       struct clk_init_data pll_init = {
> +               .ops = &lynx_pll_clk_ops,
> +               .parent_data = pll_parents,
> +               .num_parents = 1,
> +               .flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_PARENT |

Why is the nocache flag used?

> +                        CLK_OPS_PARENT_ENABLE,
> +       };
> +       struct clk_init_data ex_dly_init = {
> +               .ops = &lynx_ex_dly_clk_ops,
> +               .parent_hws = &ex_dly_parents,
> +               .num_parents = 1,
> +       };
> +       struct lynx_clk *clk;
> +       int ret;