Re: [PATCH v6 4/8] phy: fsl: Add Lynx 10G SerDes driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vinod/Ioana,

Ioana, last time you commented [1], you said you were interested in testing
this out on some other SoCs. Did you ever end up doing that? I've you've had
a chance to look at this driver more closely, do you have any suggestions for
dynamic reconfiguration?

[1] https://lore.kernel.org/netdev/GV1PR04MB9055F41AD598F85648B54EE2E0AE9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

On 9/26/22 11:35 AM, Sean Anderson wrote:
> 
> 
> On 9/24/22 2:54 AM, Vinod Koul wrote:
>> On 20-09-22, 16:23, Sean Anderson wrote:
>>> This adds support for the Lynx 10G "SerDes" devices found on various NXP
>>> QorIQ SoCs. There may be up to four SerDes devices on each SoC, each
>>> supporting up to eight lanes. Protocol support for each SerDes is highly
>>> heterogeneous, with each SoC typically having a totally different
>>> selection of supported protocols for each lane. Additionally, the SerDes
>>> devices on each SoC also have differing support. One SerDes will
>>> typically support Ethernet on most lanes, while the other will typically
>>> support PCIe on most lanes.
>>> 
>>> There is wide hardware support for this SerDes. It is present on QorIQ
>>> T-Series and Layerscape processors. Because each SoC typically has
>>> specific instructions and exceptions for its SerDes, I have limited the
>>> initial scope of this module to just the LS1046A and LS1088A.
>>> Additionally, I have only added support for Ethernet protocols. There is
>>> not a great need for dynamic reconfiguration for other protocols (except
>>> perhaps for M.2 cards), so support for them may never be added.
>>> 
>>> Nevertheless, I have tried to provide an obvious path for adding support
>>> for other SoCs as well as other protocols. SATA just needs support for
>>> configuring LNmSSCR0. PCIe may need to configure the equalization
>>> registers. It also uses multiple lanes. I have tried to write the driver
>>> with multi-lane support in mind, so there should not need to be any
>>> large changes. Although there are 6 protocols supported, I have only
>>> tested SGMII and XFI. The rest have been implemented as described in
>>> the datasheet. Most of these protocols should work "as-is", but
>>> 10GBASE-KR will need PCS support for link training.
>>> 
>>> The PLLs are modeled as clocks proper. This lets us take advantage of
>>> the existing clock infrastructure. 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.
>>> 
>>> 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.
>>> 
>>> The general API call list for this PHY is documented under the driver-api
>>> docs. I think this is rather standard, except that most drivers configure
>>> the mode (protocol) at xlate-time. Unlike some other phys where e.g. PCIe
>>> x4 will use 4 separate phys all configured for PCIe, this driver uses one
>>> phy configured to use 4 lanes. This is because while the individual lanes
>>> may be configured individually, the protocol selection acts on all lanes at
>>> once. Additionally, the order which lanes should be configured in is
>>> specified by the datasheet.  To coordinate this, lanes are reserved in
>>> phy_init, and released in phy_exit.
>>> 
>>> This driver was written with reference to the LS1046A reference manual.
>>> However, it was informed by reference manuals for all processors with
>>> mEMACs, especially the T4240 (which appears to have a "maxed-out"
>>> configuration). The earlier P-Series processors appear to be similar, but
>>> have a different overall register layout (using "banks" instead of
>>> separate SerDes). Perhaps this those use a "5G Lynx SerDes."
>>> 
>>> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
>>> ---
>>> 
>>> Changes in v6:
>>> - Update MAINTAINERS to include new files
>>> - Include bitfield.h and slab.h to allow compilation on non-arm64
>>>   arches.
>>> - Depend on COMMON_CLK and either layerscape/ppc
>>> 
>>> Changes in v5:
>>> - Remove references to PHY_INTERFACE_MODE_1000BASEKX to allow this
>>>   series to be applied directly to linux/master.
>>> - Add fsl,lynx-10g.h to MAINTAINERS
>>> 
>>> Changes in v4:
>>> - Rework all debug statements to remove use of __func__. Additional
>>>   information has been provided as necessary.
>>> - Consider alternative parent rates in round_rate and not in set_rate.
>>>   Trying to modify out parent's rate in set_rate will deadlock.
>>> - Explicitly perform a stop/reset sequence in set_rate. This way we
>>>   always ensure that the PLL is properly stopped.
>>> - Set the power-down bit when disabling the PLL. We can do this now that
>>>   enable/disable aren't abused during the set rate sequence.
>>> - Fix typos in QSGMII_OFFSET and XFI_OFFSET
>>> - Rename LNmTECR0_TEQ_TYPE_PRE to LNmTECR0_TEQ_TYPE_POST to better
>>>   reflect its function (adding post-cursor equalization).
>>> - Use of_clk_hw_onecell_get instead of a custom function.
>>> - Return struct clks from lynx_clks_init instead of embedding lynx_clk
>>>   in lynx_priv.
>>> - Rework PCCR helper functions; T-series SoCs differ from Layerscape SoCs
>>>   primarily in the layout and offset of the PCCRs. This will help bring a
>>>   cleaner abstraction layer. The caps have been removed, since this handles the
>>>   only current usage.
>>> - Convert to use new binding format. As a result of this, we no longer need to
>>>   have protocols for PCIe or SATA. Additionally, modes now live in lynx_group
>>>   instead of lynx_priv.
>>> - Remove teq from lynx_proto_params, since it can be determined from
>>>   preq_ratio/postq_ratio.
>>> - Fix an early return from lynx_set_mode not releasing serdes->lock.
>>> - Rename lynx_priv.conf to .cfg, since I kept mistyping it.
>>> 
>>> Changes in v3:
>>> - Rename remaining references to QorIQ SerDes to Lynx 10G
>>> - Fix PLL enable sequence by waiting for our reset request to be cleared
>>>   before continuing. Do the same for the lock, even though it isn't as
>>>   critical. Because we will delay for 1.5ms on average, use prepare
>>>   instead of enable so we can sleep.
>>> - Document the status of each protocol
>>> - Fix offset of several bitfields in RECR0
>>> - Take into account PLLRST_B, SDRST_B, and SDEN when considering whether
>>>   a PLL is "enabled."
>>> - Only power off unused lanes.
>>> - Split mode lane mask into first/last lane (like group)
>>> - Read modes from device tree
>>> - Use caps to determine whether KX/KR are supported
>>> - Move modes to lynx_priv
>>> - Ensure that the protocol controller is not already in-use when we try
>>>   to configure a new mode. This should only occur if the device tree is
>>>   misconfigured (e.g. when QSGMII is selected on two lanes but there is
>>>   only one QSGMII controller).
>>> - Split PLL drivers off into their own file
>>> - Add clock for "ext_dly" instead of writing the bit directly (and
>>>   racing with any clock code).
>>> - Use kasprintf instead of open-coding the snprintf dance
>>> - Support 1000BASE-KX in lynx_lookup_proto. This still requires PCS
>>>   support, so nothing is truly "enabled" yet.
>>> 
>>> Changes in v2:
>>> - Rename driver to Lynx 10G (etc.)
>>> - Fix not clearing group->pll after disabling it
>>> - Support 1 and 2 phy-cells
>>> - Power off lanes during probe
>>> - Clear SGMIIaCR1_PCS_EN during probe
>>> - Rename LYNX_PROTO_UNKNOWN to LYNX_PROTO_NONE
>>> - Handle 1000BASE-KX in lynx_proto_mode_prep
>>> 
>>>  Documentation/driver-api/phy/index.rst       |    1 +
>>>  Documentation/driver-api/phy/lynx_10g.rst    |   66 +
>>>  MAINTAINERS                                  |    7 +
>>>  drivers/phy/freescale/Kconfig                |   21 +
>>>  drivers/phy/freescale/Makefile               |    3 +
>>>  drivers/phy/freescale/lynx-10g.h             |   16 +
>>>  drivers/phy/freescale/phy-fsl-lynx-10g-clk.c |  503 ++++++++
>>>  drivers/phy/freescale/phy-fsl-lynx-10g.c     | 1162 ++++++++++++++++++
>>>  8 files changed, 1779 insertions(+)
>>>  create mode 100644 Documentation/driver-api/phy/lynx_10g.rst
>>>  create mode 100644 drivers/phy/freescale/lynx-10g.h
>>>  create mode 100644 drivers/phy/freescale/phy-fsl-lynx-10g-clk.c
>>>  create mode 100644 drivers/phy/freescale/phy-fsl-lynx-10g.c
>>> 
>>> diff --git a/Documentation/driver-api/phy/index.rst b/Documentation/driver-api/phy/index.rst
>>> index 69ba1216de72..c9b7a4698dab 100644
>>> --- a/Documentation/driver-api/phy/index.rst
>>> +++ b/Documentation/driver-api/phy/index.rst
>>> @@ -7,6 +7,7 @@ Generic PHY Framework
>>>  .. toctree::
>>>  
>>>     phy
>>> +   lynx_10g
>>>     samsung-usb2
>>>  
>>>  .. only::  subproject and html
>>> diff --git a/Documentation/driver-api/phy/lynx_10g.rst b/Documentation/driver-api/phy/lynx_10g.rst
>>> new file mode 100644
>>> index 000000000000..6e85dcd8fd71
>>> --- /dev/null
>>> +++ b/Documentation/driver-api/phy/lynx_10g.rst
>>> @@ -0,0 +1,66 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +===========================
>>> +Lynx 10G Phy (QorIQ SerDes)
>>> +===========================
>>> +
>>> +Using this phy
>>> +--------------
>>> +
>>> +The general order of calls should be::
>>> +
>>> +    [devm_][of_]phy_get()
>>> +    phy_init()
>>> +    phy_power_on()
>>> +    phy_set_mode[_ext]()
>>> +    ...
>>> +    phy_power_off()
>>> +    phy_exit()
>>> +    [[of_]phy_put()]
>>> +
>>> +:c:func:`phy_get` just gets (or creates) a new :c:type:`phy` with the lanes
>>> +described in the phandle. :c:func:`phy_init` is what actually reserves the
>>> +lanes for use. Unlike some other drivers, when the phy is created, there is no
>>> +default protocol. :c:func:`phy_set_mode <phy_set_mode_ext>` must be called in
>>> +order to set the protocol.
>> 
>> This should be added/update to Generic phy documenetation and point that
>> from here please
> 
> Not all phy consumers follow this order. phy_set_mode is optional for
> some phys. Same for phy_power_on/phy_init. I know that for this phy,
> these functions must be called in this order, but is this correct for
> all the other phys?
> 
>>> +
>>> +Supporting SoCs
>>> +---------------
>>> +
>>> +Each new SoC needs a :c:type:`struct lynx_conf <lynx_conf>`, containing the
>>> +number of lanes in each device, the endianness of the device, and the helper
>>> +functions to use when selecting protocol controllers. For example, the
>>> +configuration for the LS1046A is::
>>> +
>>> +    static const struct lynx_cfg ls1046a_cfg = {
>>> +        .lanes = 4,
>>> +        .endian = REGMAP_ENDIAN_BIG,
>>> +        .mode_conflict = lynx_ls_mode_conflict,
>>> +        .mode_apply = lynx_ls_mode_apply,
>>> +        .mode_init = lynx_ls_mode_init,
>>> +    };
>>> +
>>> +In addition, you will need to add a device node as documented in
>>> +``Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml``.  This lets the
>>> +driver know which lanes are available to configure.
>>> +
>>> +Supporting Protocols
>>> +--------------------
>>> +
>>> +Each protocol is a combination of values which must be programmed into the lane
>>> +registers. To add a new protocol, first add it to :c:type:`enum lynx_protocol
>>> +<lynx_protocol>`. Add a new entry to `lynx_proto_params`, and populate the
>>> +appropriate fields. Modify `lynx_lookup_proto` to map the :c:type:`enum
>>> +phy_mode <phy_mode>` to :c:type:`enum lynx_protocol <lynx_protocol>`. Update
>>> +the ``mode_conflict``, ``mode_apply``, and ``mode_init`` helpers are updated to
>>> +support your protocol.
>>> +
>>> +You may need to modify :c:func:`lynx_set_mode` in order to support your
>>> +protocol. This can happen when you have added members to :c:type:`struct
>>> +lynx_proto_params <lynx_proto_params>`. It can also happen if you have specific
>>> +clocking requirements, or protocol-specific registers to program.
>>> +
>>> +Internal API Reference
>>> +----------------------
>>> +
>>> +.. kernel-doc:: drivers/phy/freescale/phy-fsl-lynx-10g.c
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index d6b617bfd1bd..216a75e2eb2e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -11974,6 +11974,13 @@ S:	Maintained
>>>  W:	http://linux-test-project.github.io/
>>>  T:	git git://github.com/linux-test-project/ltp.git
>>>  
>>> +LYNX 10G SERDES DRIVER
>>> +M:	Sean Anderson <sean.anderson@xxxxxxxx>
>>> +S:	Maintained
>>> +F:	Documentation/driver-api/phy/lynx_10g.rst
>>> +F:	drivers/phy/freescale/*lynx-10g*
>>> +F:	include/dt-bindings/clock/fsl,lynx-10g.h
>>> +
>>>  LYNX 28G SERDES PHY DRIVER
>>>  M:	Ioana Ciornei <ioana.ciornei@xxxxxxx>
>>>  L:	netdev@xxxxxxxxxxxxxxx
>>> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
>>> index 853958fb2c06..40d3bfb36076 100644
>>> --- a/drivers/phy/freescale/Kconfig
>>> +++ b/drivers/phy/freescale/Kconfig
>>> @@ -47,3 +47,24 @@ config PHY_FSL_LYNX_28G
>>>  	  found on NXP's Layerscape platforms such as LX2160A.
>>>  	  Used to change the protocol running on SerDes lanes at runtime.
>>>  	  Only useful for a restricted set of Ethernet protocols.
>>> +
>>> +config PHY_FSL_LYNX_10G
>>> +	tristate "Freescale QorIQ Lynx 10G SerDes support"
>>> +	depends on COMMON_CLK
>>> +	depends on ARCH_LAYERSCAPE || PPC || COMPILE_TEST
>>> +	select GENERIC_PHY
>>> +	select REGMAP_MMIO
>>> +	help
>>> +	  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 dynamic
>>> +	  reconfiguration (such as to support 1G and 10G ethernet on the same
>>> +	  interface). 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.
>>> +
>>> +	  If you have a QorIQ processor and want to dynamically reconfigure your
>>> +	  SerDes, say Y. If this driver is compiled as a module, it will be
>>> +	  named phy-fsl-lynx-10g-drv.
>>> diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
>>> index cedb328bc4d2..1f18936507e0 100644
>>> --- a/drivers/phy/freescale/Makefile
>>> +++ b/drivers/phy/freescale/Makefile
>>> @@ -3,4 +3,7 @@ obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
>>>  obj-$(CONFIG_PHY_MIXEL_LVDS_PHY)	+= phy-fsl-imx8qm-lvds-phy.o
>>>  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-dphy.o
>>>  obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)	+= phy-fsl-imx8m-pcie.o
>>> +phy-fsl-lynx-10g-drv-y			+= phy-fsl-lynx-10g.o
>>> +phy-fsl-lynx-10g-drv-y			+= phy-fsl-lynx-10g-clk.o
>> 
>> You should use a single line for this or even better:
>> 
>> obj-$(CONFIG_PHY_FSL_LYNX_10G)               += phy-fsl-lynx-10g.o phy-fsl-lynx-10g-clk.o
> 
> Doesn't this create two modules?
> 
>>> +obj-$(CONFIG_PHY_FSL_LYNX_10G)		+= phy-fsl-lynx-10g-drv.o
>>>  obj-$(CONFIG_PHY_FSL_LYNX_28G)		+= phy-fsl-lynx-28g.o
>>> diff --git a/drivers/phy/freescale/lynx-10g.h b/drivers/phy/freescale/lynx-10g.h
>>> new file mode 100644
>>> index 000000000000..75d9353a867b
>>> --- /dev/null
>>> +++ b/drivers/phy/freescale/lynx-10g.h
>>> @@ -0,0 +1,16 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) 2022 Sean Anderson <sean.anderson@xxxxxxxx>
>>> + */
>>> +
>>> +#ifndef LYNX_10G
>>> +#define LYNX_10G
>>> +
>>> +struct clk;
>>> +struct device;
>>> +struct regmap;
>> 
>> why do you want to forward declaration for these?
>> 
>>> +
>>> +int lynx_clks_init(struct device *dev, struct regmap *regmap,
>>> +		   struct clk *plls[2], struct clk *ex_dlys[2]);
> 
> These structs are used here.
> 
>>> +#endif /* LYNX 10G */
>>> diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g-clk.c b/drivers/phy/freescale/phy-fsl-lynx-10g-clk.c
>>> new file mode 100644
>>> index 000000000000..6ec32bdfb9dd
>>> --- /dev/null
>>> +++ b/drivers/phy/freescale/phy-fsl-lynx-10g-clk.c
>>> @@ -0,0 +1,503 @@
>>> +// 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>
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/device.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/math64.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/units.h>
>>> +#include <dt-bindings/clock/fsl,lynx-10g.h>
>>> +
>>> +#include "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)
>>> +#define PLLaRSTCTL_PLLRST_B	BIT(7)
>>> +#define PLLaRSTCTL_SDRST_B	BIT(6)
>>> +#define PLLaRSTCTL_SDEN		BIT(5)
>>> +
>>> +#define PLLaRSTCTL_ENABLE_SET	(PLLaRSTCTL_RST_DONE | PLLaRSTCTL_PLLRST_B | \
>>> +				 PLLaRSTCTL_SDRST_B | PLLaRSTCTL_SDEN)
>>> +#define PLLaRSTCTL_ENABLE_MASK	(PLLaRSTCTL_ENABLE_SET | PLLaRSTCTL_RST_ERR)
>>> +
>>> +#define PLLaCR0_POFF		BIT(31)
>>> +#define PLLaCR0_RFCLK_SEL	GENMASK(30, 28)
>>> +#define PLLaCR0_PLL_LCK		BIT(23)
>>> +#define PLLaCR0_FRATE_SEL	GENMASK(19, 16)
>>> +#define PLLaCR0_DLYDIV_SEL	GENMASK(1, 0)
>>> +
>>> +#define PLLaCR0_DLYDIV_SEL_16		0b01
>>> +
>>> +/**
>>> + * struct lynx_clk - Driver data for the PLLs
>>> + * @pll: The PLL clock
>>> + * @ex_dly: The "PLLa_ex_dly_clk" clock
>>> + * @ref: Our reference clock
>>> + * @dev: The serdes device
>>> + * @regmap: Our registers
>>> + * @idx: Which PLL this clock is for
>>> + */
>>> +struct lynx_clk {
>>> +	struct clk_hw pll, ex_dly;
>>> +	struct clk_hw *ref;
>>> +	struct device *dev;
>>> +	struct regmap *regmap;
>>> +	unsigned int idx;
>>> +};
>>> +
>>> +static u32 lynx_read(struct lynx_clk *clk, u32 reg)
>>> +{
>>> +	unsigned int ret = 0;
>>> +
>>> +	WARN_ON_ONCE(regmap_read(clk->regmap, reg, &ret));
>>> +	return ret;
>>> +}
>>> +
>>> +static void lynx_write(struct lynx_clk *clk, u32 val, u32 reg)
>>> +{
>>> +	WARN_ON_ONCE(regmap_write(clk->regmap, reg, val));
>>> +}
>>> +
>>> +static struct lynx_clk *lynx_pll_to_clk(struct clk_hw *hw)
>>> +{
>>> +	return container_of(hw, struct lynx_clk, pll);
>>> +}
>>> +
>>> +static struct lynx_clk *lynx_ex_dly_to_clk(struct clk_hw *hw)
>>> +{
>>> +	return container_of(hw, struct lynx_clk, ex_dly);
>>> +}
>>> +
>>> +static void lynx_pll_stop(struct lynx_clk *clk)
>>> +{
>>> +	u32 rstctl;
>>> +
>>> +	rstctl = lynx_read(clk, PLLaRSTCTL(clk->idx));
>>> +	rstctl &= ~PLLaRSTCTL_SDRST_B;
>>> +	lynx_write(clk, rstctl, PLLaRSTCTL(clk->idx));
>>> +
>>> +	ndelay(50);
>>> +
>>> +	rstctl = lynx_read(clk, PLLaRSTCTL(clk->idx));
>>> +	rstctl &= ~(PLLaRSTCTL_SDEN | PLLaRSTCTL_PLLRST_B);
>>> +	lynx_write(clk, rstctl, PLLaRSTCTL(clk->idx));
>>> +
>>> +	ndelay(100);
>>> +}
>>> +
>>> +static void lynx_pll_disable(struct clk_hw *hw)
>>> +{
>>> +	struct lynx_clk *clk = lynx_pll_to_clk(hw);
>>> +	u32 cr0;
>>> +
>>> +	dev_dbg(clk->dev, "disable pll%d\n", clk->idx);
>>> +
>>> +	lynx_pll_stop(clk);
>>> +
>>> +	cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +	cr0 |= PLLaCR0_POFF;
>>> +	lynx_write(clk, cr0, PLLaCR0(clk->idx));
>>> +}
>>> +
>>> +static int lynx_pll_reset(struct lynx_clk *clk)
>>> +{
>>> +	int ret;
>>> +	u32 rstctl = lynx_read(clk, PLLaRSTCTL(clk->idx));
>>> +
>>> +	rstctl |= PLLaRSTCTL_RSTREQ;
>>> +	lynx_write(clk, rstctl, PLLaRSTCTL(clk->idx));
>>> +	ret = read_poll_timeout(lynx_read, rstctl,
>>> +				rstctl & (PLLaRSTCTL_RST_DONE | PLLaRSTCTL_RST_ERR),
>>> +				100, 5000, true, clk, PLLaRSTCTL(clk->idx));
>>> +	if (rstctl & PLLaRSTCTL_RST_ERR)
>>> +		ret = -EIO;
>>> +	if (ret) {
>>> +		dev_err(clk->dev, "pll%d reset failed\n", clk->idx);
>>> +		return ret;
>>> +	}
>>> +
>>> +	rstctl |= PLLaRSTCTL_SDEN | PLLaRSTCTL_PLLRST_B | PLLaRSTCTL_SDRST_B;
>>> +	lynx_write(clk, rstctl, PLLaRSTCTL(clk->idx));
>>> +	return 0;
>>> +}
>>> +
>>> +static int lynx_pll_prepare(struct clk_hw *hw)
>>> +{
>>> +	struct lynx_clk *clk = lynx_pll_to_clk(hw);
>>> +	u32 rstctl = lynx_read(clk, PLLaRSTCTL(clk->idx));
>>> +	u32 cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +
>>> +	/*
>>> +	 * "Enabling" the PLL involves resetting it (and all attached lanes).
>>> +	 * Avoid doing this if we are already enabled.
>>> +	 */
>>> +	if (!(cr0 & PLLaCR0_POFF) &&
>>> +	    (rstctl & PLLaRSTCTL_ENABLE_MASK) == PLLaRSTCTL_ENABLE_SET) {
>>> +		dev_dbg(clk->dev, "pll%d already prepared\n", clk->idx);
>>> +		return 0;
>>> +	}
>>> +
>>> +	dev_dbg(clk->dev, "prepare pll%d\n", clk->idx);
>>> +
>>> +	cr0 &= ~PLLaCR0_POFF;
>>> +	lynx_write(clk, cr0, PLLaCR0(clk->idx));
>>> +
>>> +	return lynx_pll_reset(clk);
>>> +}
>>> +
>>> +static int lynx_pll_is_enabled(struct clk_hw *hw)
>>> +{
>>> +	struct lynx_clk *clk = lynx_pll_to_clk(hw);
>>> +	u32 cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +	bool enabled = !(cr0 & PLLaCR0_POFF);
>>> +
>>> +	dev_dbg(clk->dev, "pll%d %s enabled\n", clk->idx,
>>> +		enabled ? "is" : "is not");
>>> +
>>> +	return enabled;
>>> +}
>>> +
>>> +static const u32 rfclk_sel_map[8] = {
>>> +	[0b000] = 100000000,
>>> +	[0b001] = 125000000,
>>> +	[0b010] = 156250000,
>>> +	[0b011] = 150000000,
>>> +};
>>> +
>>> +/**
>>> + * lynx_rfclk_to_sel() - Convert a reference clock rate to a selector
>>> + * @rate: The reference clock rate
>>> + *
>>> + * To allow for some variation in the reference clock rate, up to 100ppm of
>>> + * error is allowed.
>>> + *
>>> + * Return: An appropriate selector for @rate, or -%EINVAL.
>>> + */
>>> +static int lynx_rfclk_to_sel(u32 rate)
>>> +{
>>> +	int ret;
>>> +
>>> +	for (ret = 0; ret < ARRAY_SIZE(rfclk_sel_map); ret++) {
>>> +		u32 rfclk_rate = rfclk_sel_map[ret];
>>> +		/* Allow an error of 100ppm */
>>> +		u32 error = rfclk_rate / 10000;
>>> +
>>> +		if (rate > rfclk_rate - error && rate < rfclk_rate + error)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static const u32 frate_sel_map[16] = {
>>> +	[0b0000] = 5000000,
>>> +	[0b0101] = 3750000,
>>> +	[0b0110] = 5156250,
>>> +	[0b0111] = 4000000,
>>> +	[0b1001] = 3125000,
>>> +	[0b1010] = 3000000,
>>> +};
>>> +
>>> +/**
>>> + * lynx_frate_to_sel() - Convert a VCO clock rate to a selector
>>> + * @rate_khz: The VCO frequency, in kHz
>>> + *
>>> + * Return: An appropriate selector for @rate_khz, or -%EINVAL.
>>> + */
>>> +static int lynx_frate_to_sel(u32 rate_khz)
>>> +{
>>> +	int ret;
>>> +
>>> +	for (ret = 0; ret < ARRAY_SIZE(frate_sel_map); ret++)
>>> +		if (frate_sel_map[ret] == rate_khz)
>>> +			return ret;
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static u32 lynx_pll_ratio(u32 frate_sel, u32 rfclk_sel)
>>> +{
>>> +	u64 frate;
>>> +	u32 rfclk, error, ratio;
>>> +
>>> +	frate = frate_sel_map[frate_sel] * (u64)HZ_PER_KHZ;
>>> +	rfclk = rfclk_sel_map[rfclk_sel];
>>> +
>>> +	if (!frate || !rfclk)
>>> +		return 0;
>>> +
>>> +	ratio = div_u64_rem(frate, rfclk, &error);
>>> +	if (!error)
>>> +		return ratio;
>>> +	return 0;
>>> +}
>>> +
>>> +static unsigned long lynx_pll_recalc_rate(struct clk_hw *hw,
>>> +					  unsigned long parent_rate)
>>> +{
>>> +	struct lynx_clk *clk = lynx_pll_to_clk(hw);
>>> +	u32 cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +	u32 frate_sel = FIELD_GET(PLLaCR0_FRATE_SEL, cr0);
>>> +	u32 rfclk_sel = FIELD_GET(PLLaCR0_RFCLK_SEL, cr0);
>>> +	u32 ratio = lynx_pll_ratio(frate_sel, rfclk_sel);
>>> +	unsigned long ret;
>>> +
>>> +	/* Ensure that the parent matches our rfclk selector */
>>> +	if (rfclk_sel == lynx_rfclk_to_sel(parent_rate))
>>> +		ret = mult_frac(parent_rate, ratio, HZ_PER_KHZ);
>>> +	else
>>> +		ret = 0;
>>> +
>>> +	dev_dbg(clk->dev, "recalc pll%d new=%llu parent=%lu\n", clk->idx,
>>> +		(u64)ret * HZ_PER_KHZ, parent_rate);
>>> +	return ret;
>>> +}
>>> +
>>> +static long lynx_pll_round_rate(struct clk_hw *hw, unsigned long rate_khz,
>>> +				unsigned long *parent_rate)
>>> +{
>>> +	int frate_sel, rfclk_sel;
>>> +	struct lynx_clk *clk = lynx_pll_to_clk(hw);
>>> +	u32 ratio;
>>> +
>>> +	dev_dbg(clk->dev, "round pll%d new=%llu parent=%lu\n", clk->idx,
>>> +		(u64)rate_khz * HZ_PER_KHZ, *parent_rate);
>>> +
>>> +	frate_sel = lynx_frate_to_sel(rate_khz);
>>> +	if (frate_sel < 0)
>>> +		return frate_sel;
>>> +
>>> +	/* Try the current parent rate */
>>> +	rfclk_sel = lynx_rfclk_to_sel(*parent_rate);
>>> +	if (rfclk_sel >= 0) {
>>> +		ratio = lynx_pll_ratio(frate_sel, rfclk_sel);
>>> +		if (ratio)
>>> +			return mult_frac(*parent_rate, ratio, HZ_PER_KHZ);
>>> +	}
>>> +
>>> +	/* Try all possible parent rates */
>>> +	for (rfclk_sel = 0;
>>> +	     rfclk_sel < ARRAY_SIZE(rfclk_sel_map);
>>> +	     rfclk_sel++) {
>>> +		unsigned long new_parent_rate;
>>> +
>>> +		ratio = lynx_pll_ratio(frate_sel, rfclk_sel);
>>> +		if (!ratio)
>>> +			continue;
>>> +
>>> +		/* Ensure the reference clock can produce this rate */
>>> +		new_parent_rate = rfclk_sel_map[rfclk_sel];
>>> +		new_parent_rate = clk_hw_round_rate(clk->ref, new_parent_rate);
>>> +		if (rfclk_sel != lynx_rfclk_to_sel(new_parent_rate))
>>> +			continue;
>>> +
>>> +		*parent_rate = new_parent_rate;
>>> +		return mult_frac(new_parent_rate, ratio, HZ_PER_KHZ);
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int lynx_pll_set_rate(struct clk_hw *hw, unsigned long rate_khz,
>>> +			   unsigned long parent_rate)
>>> +{
>>> +	int frate_sel, rfclk_sel;
>>> +	struct lynx_clk *clk = lynx_pll_to_clk(hw);
>>> +	u32 ratio, cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +
>>> +	dev_dbg(clk->dev, "set rate pll%d new=%llu parent=%lu\n", clk->idx,
>>> +		(u64)rate_khz * HZ_PER_KHZ, parent_rate);
>>> +
>>> +	frate_sel = lynx_frate_to_sel(rate_khz);
>>> +	if (frate_sel < 0)
>>> +		return frate_sel;
>>> +
>>> +	rfclk_sel = lynx_rfclk_to_sel(parent_rate);
>>> +	if (rfclk_sel < 0)
>>> +		return rfclk_sel;
>>> +
>>> +	ratio = lynx_pll_ratio(frate_sel, rfclk_sel);
>>> +	if (!ratio)
>>> +		return -EINVAL;
>>> +
>>> +	lynx_pll_stop(clk);
>>> +	cr0 &= ~(PLLaCR0_RFCLK_SEL | PLLaCR0_FRATE_SEL);
>>> +	cr0 |= FIELD_PREP(PLLaCR0_RFCLK_SEL, rfclk_sel);
>>> +	cr0 |= FIELD_PREP(PLLaCR0_FRATE_SEL, frate_sel);
>>> +	lynx_write(clk, cr0, PLLaCR0(clk->idx));
>>> +	/* Don't bother resetting if it's off */
>>> +	if (cr0 & PLLaCR0_POFF)
>>> +		return 0;
>>> +	return lynx_pll_reset(clk);
>>> +}
>>> +
>>> +static const struct clk_ops lynx_pll_clk_ops = {
>>> +	.prepare = lynx_pll_prepare,
>>> +	.disable = lynx_pll_disable,
>>> +	.is_enabled = lynx_pll_is_enabled,
>>> +	.recalc_rate = lynx_pll_recalc_rate,
>>> +	.round_rate = lynx_pll_round_rate,
>>> +	.set_rate = lynx_pll_set_rate,
>>> +};
>>> +
>>> +static void lynx_ex_dly_disable(struct clk_hw *hw)
>>> +{
>>> +	struct lynx_clk *clk = lynx_ex_dly_to_clk(hw);
>>> +	u32 cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +
>>> +	cr0 &= ~PLLaCR0_DLYDIV_SEL;
>>> +	lynx_write(clk, PLLaCR0(clk->idx), cr0);
>>> +}
>>> +
>>> +static int lynx_ex_dly_enable(struct clk_hw *hw)
>>> +{
>>> +	struct lynx_clk *clk = lynx_ex_dly_to_clk(hw);
>>> +	u32 cr0 = lynx_read(clk, PLLaCR0(clk->idx));
>>> +
>>> +	cr0 &= ~PLLaCR0_DLYDIV_SEL;
>>> +	cr0 |= FIELD_PREP(PLLaCR0_DLYDIV_SEL, PLLaCR0_DLYDIV_SEL_16);
>>> +	lynx_write(clk, PLLaCR0(clk->idx), cr0);
>>> +	return 0;
>>> +}
>>> +
>>> +static int lynx_ex_dly_is_enabled(struct clk_hw *hw)
>>> +{
>>> +	struct lynx_clk *clk = lynx_ex_dly_to_clk(hw);
>>> +
>>> +	return lynx_read(clk, PLLaCR0(clk->idx)) & PLLaCR0_DLYDIV_SEL;
>>> +}
>>> +
>>> +static unsigned long lynx_ex_dly_recalc_rate(struct clk_hw *hw,
>>> +					     unsigned long parent_rate)
>>> +{
>>> +	return parent_rate / 16;
>>> +}
>>> +
>>> +static const struct clk_ops lynx_ex_dly_clk_ops = {
>>> +	.enable = lynx_ex_dly_enable,
>>> +	.disable = lynx_ex_dly_disable,
>>> +	.is_enabled = lynx_ex_dly_is_enabled,
>>> +	.recalc_rate = lynx_ex_dly_recalc_rate,
>>> +};
>>> +
>>> +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 *pll_parents, *ex_dly_parents;
>>> +	struct clk_init_data pll_init = {
>>> +		.ops = &lynx_pll_clk_ops,
>>> +		.parent_hws = &pll_parents,
>>> +		.num_parents = 1,
>>> +		.flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_PARENT |
>>> +			 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 clk *ref;
>>> +	struct lynx_clk *clk;
>>> +	char *ref_name;
>>> +	int ret;
>>> +
>>> +	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
>>> +	if (!clk)
>>> +		return -ENOMEM;
>>> +
>>> +	clk->dev = dev;
>>> +	clk->regmap = regmap;
>>> +	clk->idx = index;
>>> +
>>> +	ref_name = kasprintf(GFP_KERNEL, "ref%d", index);
>>> +	pll_init.name = kasprintf(GFP_KERNEL, "%s.pll%d_khz", dev_name(dev),
>>> +				  index);
>>> +	ex_dly_init.name = kasprintf(GFP_KERNEL, "%s.pll%d_ex_dly_khz",
>>> +				     dev_name(dev), index);
>>> +	if (!ref_name || !pll_init.name || !ex_dly_init.name) {
>>> +		ret = -ENOMEM;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ref = devm_clk_get(dev, ref_name);
>>> +	if (IS_ERR(clk->ref)) {
>>> +		ret = PTR_ERR(clk->ref);
>>> +		dev_err_probe(dev, ret, "could not get %s\n", ref_name);
>>> +		goto out;
>>> +	}
>>> +
>>> +	clk->ref = __clk_get_hw(ref);
>>> +	pll_parents = clk->ref;
>>> +	clk->pll.init = &pll_init;
>>> +	ret = devm_clk_hw_register(dev, &clk->pll);
>>> +	if (ret) {
>>> +		dev_err_probe(dev, ret, "could not register %s\n",
>>> +			      pll_init.name);
>>> +		goto out;
>>> +	}
>>> +
>>> +	ex_dly_parents = &clk->pll;
>>> +	clk->ex_dly.init = &ex_dly_init;
>>> +	ret = devm_clk_hw_register(dev, &clk->ex_dly);
>>> +	if (ret)
>>> +		dev_err_probe(dev, ret, "could not register %s\n",
>>> +			      ex_dly_init.name);
>>> +
>>> +	hw_data->hws[LYNX10G_PLLa(index)] = &clk->pll;
>>> +	hw_data->hws[LYNX10G_PLLa_EX_DLY(index)] = &clk->ex_dly;
>>> +
>>> +out:
>>> +	kfree(ref_name);
>>> +	kfree(pll_init.name);
>>> +	kfree(ex_dly_init.name);
>>> +	return ret;
>>> +}
>>> +
>>> +#define NUM_PLLS 2
>>> +#define NUM_CLKS (NUM_PLLS * LYNX10G_CLKS_PER_PLL)
>>> +
>>> +int lynx_clks_init(struct device *dev, struct regmap *regmap,
>>> +		   struct clk *plls[2], struct clk *ex_dlys[2])
>>> +{
>>> +	int ret, i;
>>> +	struct clk_hw_onecell_data *hw_data;
>>> +
>>> +	hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, NUM_CLKS),
>>> +			       GFP_KERNEL);
>>> +	if (!hw_data)
>>> +		return -ENOMEM;
>>> +	hw_data->num = NUM_CLKS;
>>> +
>>> +	for (i = 0; i < NUM_PLLS; i++) {
>>> +		ret = lynx_clk_init(hw_data, dev, regmap, i);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		plls[i] = hw_data->hws[LYNX10G_PLLa(i)]->clk;
>>> +		ex_dlys[i] = hw_data->hws[LYNX10G_PLLa_EX_DLY(i)]->clk;
>>> +	}
>>> +
>>> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, hw_data);
>>> +	if (ret)
>>> +		dev_err_probe(dev, ret, "could not register clock provider\n");
>>> +
>>> +	return ret;
>>> +}
>> 
>> Why is this not a clock driver in drivers/clk/ ?
> 
> Because it is closely integrated with the phy. IMO it should really live
> in the very same file as the phy driver itself, due to how closely
> related they are. AFAIK there are no other hardware consumers of these
> PLLs other than the SerDes.
> 
>>> +#define abs_diff(a, b) ({ \
>>> +	typeof(a) _a = (a); \
>>> +	typeof(b) _b = (b); \
>>> +	_a > _b ? _a - _b : _b - _a; \
>>> +})
>> 
>> are you sure there is not existing helper which can help you with this?
> 
> From what I can tell, there is no existing helper. Maybe there should be.
> 
> --Sean

Are my above responses satisfactory? I noticed that they are fairly similar to
what you remarked last time [2]. In several cases I gave similar responses, but
you never acknowledged them. I am worried that we are talking past each other...

--Sean

[2] https://lore.kernel.org/linux-phy/YsPWMYjyu2nyk+w8@matsya/



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux