Hi Stephen,
On 1/27/23 16:51, Sean Anderson wrote:
> On 1/27/23 15:59, Stephen Boyd wrote:
>> 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?
>
> The handling of rate changes, locking, etc is all done by the clock
> subsystem. We can use the existing debugfs stuff as well. And everything
> is organized by existing API boundaries.
>
>>> 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?
>
> It is, and yes.
>
>>>
>>> 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?
>
> It's wholly self-contained, aside from an undocumented mode where you
> can output a fixed frequency (for testing). The provider exists so you
> can use assigned-clocks to ensure a particular PLL is used for a
> particular frequency. This prevents a problem where e.g. you have
> reference clocks for 100 Hz and 156.25 Hz, one lane requests 5 GHz and
> the PLL with the 156.25 Hz clock gets used. Then, later another lane
> requests 5.15625 GHz and the remaining 100 Hz PLL can't provide it. To
> prevent this, you can assign the PLLs their rates up front and the lanes
> will use the existing rates. Most of the time this should be done by the
> RCW, but there are cases where the RCW can't set up the clocks properly.
>
>> 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,
>
> This is what currently happens. See lynx_set_mode in the next patch.
>
>> 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.
>
> Can you describe this in a bit more detail? Are you suggesting to skip
> the clock framework?
>
>> BTW, what sort of phy is this? Some networking device?
>
> From the Kconfig in the next commit:
>
> This adds support for the Lynx "SerDes" devices found on various QorIQ
> SoCs. There may be up to four SerDes devices on each SoC, and each
> device supports up to eight lanes. The SerDes is configured by
> default by the RCW, but this module is necessary in order to support
> some modes (such as 2.5G SGMII or 1000BASE-KX), or clock setups (as
> only as subset of clock configurations are supported by the RCW).
> The hardware supports a variety of protocols, including Ethernet,
> SATA, PCIe, and more exotic links such as Interlaken and Aurora. This
> driver only supports Ethernet, but it will try not to touch lanes
> configured for other protocols.
>
>>>
>>> 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@xxxxxxxx>
>>> + *
>>> + * 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.
>
> OK
>
>>> +#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?
>
> PCIe rate selection can automatically modify the frequency of the
> PLL. From the reference manual:
>
> | For PCIe 8 Gbaud, the auto-negotiation also involves a combination of
> | switching PLL selects, and/or reconfiguring a PLL. There are two
> | scenarios for PLL reconfiguration:
> |
> | * PCIe starts on any PLL which runs at 5GHz, and the other PLL is off.
> | PHY wrapper will switch to the other PLL as part of Gen 3 speed
> | switch. In this case, both PLLs must be supplied with valid reference
> | clocks.
> | * PCIe starts on any PLL which runs at 5GHz, and the other PLL is
> | being used for other protocols. PHY wrapper will reconfigure the
> | current 5GHz PLL as part of Gen 3 speed switch. PCIe stays on the same
> | PLL in this case. This also applies to the scenario where one PCIe
> | port runs on all lanes.
>
> As far as I know there's no way to configure or disable this. PCIe isn't
> implemented yet, but it would likely need some way to mark PLLs for
> exclusive use (not just exclusive rate). I figured it would be better
> not to cache the rate so that things like the above would get detected
> properly.
>
> --Sean
>
>>> + 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;
I am going to try and send out v10 soon. Do you have any comments on my
reply here? At the moment the only change I am planning to make is to
remove the clk.h include.
--Sean
