Quoting Sean Anderson (2022-10-27 12:11:08) > diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig > index 853958fb2c06..a6ccccf9e39b 100644 > --- a/drivers/phy/freescale/Kconfig > +++ b/drivers/phy/freescale/Kconfig > @@ -47,3 +47,25 @@ 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 Does something not compile if COMMON_CLK is disabled? > + 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 > + 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. > + > + 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 > +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; > + > +int lynx_clks_init(struct device *dev, struct regmap *regmap, Can you use auxiliary bus to register this clk controller instead and then move the clk file to drivers/clk/? > + struct clk *plls[2], struct clk *ex_dlys[2]); > + > +#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> Ideally clk.h isn't included in a clk provider. This allows us to easily identify drivers that are both a consumer (clk.h) and a provider (clk-provider.h). A provider/consumer is rare. > +#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) Should rate be unsigned long? Or you really want 32-bits here? > +{ > + 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) Does if (abs(rate - rfclk_rate) < error) work? I'm kinda surprised that we don't have a within_tolerance(x, margin) macro in math.h that would make it look like: if (within_tolerance(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); Please don't use __clk_get_hw() for this. Instead use struct clk_parent_data and set a DT index in the index member to map to this clk. > + 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; Use clk_hw_get_clk() please. > + } > + > + 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; > +}