On 10/28/22 12:13, Sean Anderson wrote:
On 10/27/22 19:03, Stephen Boyd wrote: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_CLKDoes something not compile if COMMON_CLK is disabled?ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function `lynx_pll_round_rate': phy-fsl-lynx-10g-clk.c:(.text+0x444): undefined reference to `clk_hw_round_rate' ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function `lynx_clks_init': phy-fsl-lynx-10g-clk.c:(.text+0x5eb): undefined reference to `devm_clk_hw_register' ld: phy-fsl-lynx-10g-clk.c:(.text+0x625): undefined reference to `devm_clk_hw_register'+ 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/?I don't want to have to deal with my clock driver getting unbound (aka the user has come and decided to make my life harder). Dynamic binding will only add complexity in this situation. I don't know how much context you've picked up, but this driver - Has one consumer, and is is the serdes. - Is not accessible from outside the serdes. - Does not share any code with other drivers. - Has bits in its registers which can control the reset process of lanes using the PLLs. These drivers are tightly coupled to each other. It is very likely IMO that changes to one (bugs, features, etc) will affect the other. For this reason, I think it makes sense to keep them in the same source directory. I actually would have preferred to keep them in the same file.+ 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.I don't see why it would be rare. Most clocks will use some external clock as their reference, so they are (therefore) consumers. Although as discussed below, apparently you can get the clock core to do this for you...+#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?Probably should be unsigned long.+{ + 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 think so.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))Ditto for the abs_diff macro used in the other half of this driver.+ 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.OK
Oh, I remember why I did this. I need the reference clock for clk_hw_round_rate, which is AFAICT the only correct way to implement round_rate. --Sean
+ 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.I don't want to do that, because then I'd have to generate the clock ID again. And why do we even need a new clock consumer in the first place? This is only for internal use by the driver; the consumer is the same as the producer. --Sean+ } + + 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; +}
