Quoting Shubhrajyoti Datta (2023-06-06 03:50:16) > The Clocking Wizard for Versal adaptive compute acceleration platforms. > Add Versal clocking wizard support. Maybe you can elaborate on the differences from the existing device support here. The register layout is slightly different? How so? > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx> > --- > > Changes in v4: > Update changelog > Fix warn > Previously we had tried to upstream [1] separate driver for > clocking wizard. It was decided to add support to the current > driver. So abandoning the series. > [1] https://lore.kernel.org/all/20221122121255.6823-1-shubhrajyoti.datta@xxxxxxx/ > > Changes in v3: > rename the clocks to clk_in1 and s_axi_clk dt > rename the clocks to clk_in1 and s_axi_clk in driver > > Changes in v2: > rename the clocks clk_in1 to in1 and s_axi_clk to s_axi in dt > rename the clocks clk_in1 to in1 and s_axi_clk to s_axi in driver > update the warn > Update the compatible to reflect versal > > drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 507 ++++++++++++++++----- > 1 file changed, 402 insertions(+), 105 deletions(-) > > diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > index e83f104fad02..737ab31678c1 100644 > --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c > @@ -23,15 +23,41 @@ > #define WZRD_NUM_OUTPUTS 7 > #define WZRD_ACLK_MAX_FREQ 250000000UL > > -#define WZRD_CLK_CFG_REG(n) (0x200 + 4 * (n)) > +#define WZRD_CLK_CFG_REG(v, n) (0x200 + 0x130 * (v) + 4 * (n)) 'v' is for 'is_versal' so I'm trying to figure out why the 'base' isn't adjusted instead? > > #define WZRD_CLKOUT0_FRAC_EN BIT(18) > -#define WZRD_CLKFBOUT_FRAC_EN BIT(26) > +#define WZRD_CLKFBOUT_1 0 > +#define WZRD_CLKFBOUT_2 1 > +#define WZRD_CLKOUT0_1 2 > +#define WZRD_CLKOUT0_2 3 > +#define WZRD_DESKEW_2 20 [...] > @@ -152,23 +195,42 @@ static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw, > { > struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); > void __iomem *div_addr = divider->base + divider->offset; > - unsigned int val; > + u32 div, p5en, edge, prediv2, all; > + unsigned int val, vall, valh; > > - val = readl(div_addr) >> divider->shift; > - val &= div_mask(divider->width); > + if (!divider->is_versal) { > + val = readl(div_addr) >> divider->shift; > + val &= div_mask(divider->width); > > - return divider_recalc_rate(hw, parent_rate, val, divider->table, > - divider->flags, divider->width); > + return divider_recalc_rate(hw, parent_rate, val, divider->table, > + divider->flags, divider->width); > + } > + > + edge = !!(readl(div_addr) & WZRD_CLKFBOUT_EDGE); > + p5en = !!(readl(div_addr) & WZRD_P5EN); > + prediv2 = !!(readl(div_addr) & WZRD_CLKOUT0_PREDIV2); > + vall = readl(div_addr + 4) & WZRD_CLKFBOUT_L_MASK; > + valh = readl(div_addr + 4) >> WZRD_CLKFBOUT_H_SHIFT; > + all = valh + vall + edge; > + if (!all) > + all = 1; > + > + if (prediv2) > + div = 2 * all + prediv2 * p5en; > + else > + div = all; > + > + return DIV_ROUND_UP_ULL((u64)parent_rate, div); This looks very different. Can we just have two different set of clk_ops instead? They can and should share code if they can, but try to avoid pushing an 'is_versal' condition throughout the code. It makes it impossible to read and fairly messy. > } > > static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate) > { > - int err; > - u32 value; > - unsigned long flags = 0; > struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); > void __iomem *div_addr = divider->base + divider->offset; > + u32 value, regh, edged, p5en, p5fedge, regval, regval1; > + unsigned long flags = 0; > + int err; > > if (divider->lock) > spin_lock_irqsave(divider->lock, flags); > @@ -177,26 +239,54 @@ static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate, > > value = DIV_ROUND_CLOSEST(parent_rate, rate); > > - /* Cap the value to max */ > - min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE); > - > - /* Set divisor and clear phase offset */ > - writel(value, div_addr); > - writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET); > - > - /* Check status register */ > - err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, > - value, value & WZRD_DR_LOCK_BIT_MASK, > - WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); > - if (err) > - goto err_reconfig; > - > - /* Initiate reconfiguration */ > - writel(WZRD_DR_BEGIN_DYNA_RECONF_5_2, > - divider->base + WZRD_DR_INIT_REG_OFFSET); > - writel(WZRD_DR_BEGIN_DYNA_RECONF1_5_2, > - divider->base + WZRD_DR_INIT_REG_OFFSET); > - > + if (!divider->is_versal) { > + /* Cap the value to max */ > + min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE); > + > + /* Set divisor and clear phase offset */ > + writel(value, div_addr); > + writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET); > + > + /* Check status register */ > + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, > + value, value & WZRD_DR_LOCK_BIT_MASK, > + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); > + if (err) > + goto err_reconfig; > + > + /* Initiate reconfiguration */ > + writel(WZRD_DR_BEGIN_DYNA_RECONF_5_2, > + divider->base + WZRD_DR_INIT_REG_OFFSET); > + writel(WZRD_DR_BEGIN_DYNA_RECONF1_5_2, > + divider->base + WZRD_DR_INIT_REG_OFFSET); > + > + } else { > + regh = (value / 4); > + regval1 = readl(div_addr); > + regval1 |= WZRD_CLKFBOUT_PREDIV2; > + regval1 = regval1 & ~(WZRD_CLKFBOUT_EDGE | WZRD_P5EN | WZRD_P5FEDGE); > + if (value % 4 > 1) { > + edged = 1; > + regval1 |= (edged << WZRD_EDGE_SHIFT); > + } > + p5fedge = value % 2; > + p5en = value % 2; > + regval1 = regval1 | p5en << WZRD_P5EN_SHIFT | p5fedge << WZRD_P5FEDGE_SHIFT; > + writel(regval1, div_addr); > + > + regval = regh | regh << WZRD_CLKFBOUT_H_SHIFT; > + writel(regval, div_addr + 4); > + /* Check status register */ > + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, > + value, value & WZRD_DR_LOCK_BIT_MASK, > + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); > + if (err) > + goto err_reconfig; > + > + /* Initiate reconfiguration */ > + writel(WZRD_DR_BEGIN_DYNA_RECONF, > + divider->base + WZRD_DR_INIT_VERSAL_OFFSET); > + } > /* Check status register */ > err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, > value, value & WZRD_DR_LOCK_BIT_MASK, This function could also be split and duplication extracted to some common function instead of sticking an if-else into it. > @@ -227,14 +317,35 @@ static int clk_wzrd_get_divisors(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate) > { > struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); > - unsigned long vco_freq, freq, diff; > + u64 vco_freq, freq, diff, vcomin, vcomax; > u32 m, d, o; > + u32 mmin, mmax, dmin, dmax, omin, omax; > + > + if (divider->is_versal) { > + mmin = VER_WZRD_M_MIN; > + mmax = VER_WZRD_M_MAX; > + dmin = VER_WZRD_D_MIN; > + dmax = VER_WZRD_D_MAX; > + omin = VER_WZRD_O_MIN; > + omax = VER_WZRD_O_MAX; > + vcomin = VER_WZRD_VCO_MIN; > + vcomax = VER_WZRD_VCO_MAX; > + } else { > + mmin = WZRD_M_MIN; > + mmax = WZRD_M_MAX; > + dmin = WZRD_D_MIN; > + dmax = WZRD_D_MAX; > + omin = WZRD_O_MIN; > + omax = WZRD_O_MAX; > + vcomin = WZRD_VCO_MIN; > + vcomax = WZRD_VCO_MAX; > + } Same? > > - for (m = WZRD_M_MIN; m <= WZRD_M_MAX; m++) { > - for (d = WZRD_D_MIN; d <= WZRD_D_MAX; d++) { > + for (m = mmin; m <= mmax; m++) { > + for (d = dmin; d <= dmax; d++) { > vco_freq = DIV_ROUND_CLOSEST((parent_rate * m), d); > - if (vco_freq >= WZRD_VCO_MIN && vco_freq <= WZRD_VCO_MAX) { > - for (o = WZRD_O_MIN; o <= WZRD_O_MAX; o++) { > + if (vco_freq >= vcomin && vco_freq <= vcomax) { > + for (o = omin; o <= omax; o++) { > freq = DIV_ROUND_CLOSEST_ULL(vco_freq, o); > diff = abs(freq - rate); > > @@ -588,18 +816,34 @@ static int __maybe_unused clk_wzrd_resume(struct device *dev) > static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend, > clk_wzrd_resume); > > +static const struct versal_clk_data versal_data = { > + .is_versal = true, > +}; > + > +static const struct of_device_id clk_wzrd_ids[] = { > + { .compatible = "xlnx,versal-clk-wizard", .data = &versal_data }, > + { .compatible = "xlnx,clocking-wizard" }, > + { .compatible = "xlnx,clocking-wizard-v5.2" }, > + { .compatible = "xlnx,clocking-wizard-v6.0" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, clk_wzrd_ids); > + > static int clk_wzrd_probe(struct platform_device *pdev) > { > + const char *clkout_name, *clk_name, *clk_mul_name; > + u32 regl, regh, edge, regld, reghd, edged, div; > + const struct versal_clk_data *data; > + const struct of_device_id *match; > int i, ret; > u32 reg, reg_f, mult; > unsigned long rate; > - const char *clk_name; > void __iomem *ctrl_reg; > struct clk_wzrd *clk_wzrd; > - const char *clkout_name; > struct device_node *np = pdev->dev.of_node; > int nr_outputs; > unsigned long flags = 0; > + bool is_versal = 0; > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > if (!clk_wzrd) > @@ -647,27 +891,61 @@ static int clk_wzrd_probe(struct platform_device *pdev) > goto err_disable_clk; > } > > + match = of_match_node(clk_wzrd_ids, np); Use device_get_match_data() instead so we don't have to move the match table. The match data could simply be a bool too that gets casted to a pointer in the match table. > + if (!match) { > + dev_err(&pdev->dev, "of_match_node failed\n"); > + ret = -ENODEV; > + goto err_disable_clk; > + } > + > + data = match->data; > + if (data) > + is_versal = data->is_versal; > + > clkout_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_out0", dev_name(&pdev->dev)); > if (nr_outputs == 1) { > clk_wzrd->clkout[0] = clk_wzrd_register_divider > (&pdev->dev, clkout_name, > __clk_get_name(clk_wzrd->clk_in1), 0, > - clk_wzrd->base, WZRD_CLK_CFG_REG(3), > + clk_wzrd->base, WZRD_CLK_CFG_REG(is_versal, 3), > WZRD_CLKOUT_DIVIDE_SHIFT, > WZRD_CLKOUT_DIVIDE_WIDTH, > CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO, > - DIV_ALL, &clkwzrd_lock); > + DIV_ALL, is_versal, &clkwzrd_lock); > > goto out; > } > - > - reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)); > - reg_f = reg & WZRD_CLKFBOUT_FRAC_MASK; > - reg_f = reg_f >> WZRD_CLKFBOUT_FRAC_SHIFT; > - > - reg = reg & WZRD_CLKFBOUT_MULT_MASK; > - reg = reg >> WZRD_CLKFBOUT_MULT_SHIFT; > - mult = (reg * 1000) + reg_f; > + if (is_versal) { > + /* register multiplier */ > + edge = !!(readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0)) & > + BIT(8)); > + regl = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 1)) & > + WZRD_CLKFBOUT_L_MASK) >> WZRD_CLKFBOUT_L_SHIFT; > + regh = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 1)) & > + WZRD_CLKFBOUT_H_MASK) >> WZRD_CLKFBOUT_H_SHIFT; > + mult = regl + regh + edge; > + if (!mult) > + mult = 1; > + mult = mult * WZRD_FRAC_GRADIENT; > + > + regl = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 51)) & > + WZRD_CLKFBOUT_FRAC_EN; > + if (regl) { > + regl = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 48)) & > + WZRD_VERSAL_FRAC_MASK; > + mult = mult + regl; > + } > + div = 64; > + } else { > + reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(is_versal, 0)); > + reg_f = reg & WZRD_CLKFBOUT_FRAC_MASK; > + reg_f = reg_f >> WZRD_CLKFBOUT_FRAC_SHIFT; > + > + reg = reg & WZRD_CLKFBOUT_MULT_MASK; > + reg = reg >> WZRD_CLKFBOUT_MULT_SHIFT; > + mult = (reg * 1000) + reg_f; > + div = 1000; > + } > clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_mul", dev_name(&pdev->dev)); > if (!clk_name) { > ret = -ENOMEM; > @@ -793,14 +1098,6 @@ static void clk_wzrd_remove(struct platform_device *pdev) > clk_disable_unprepare(clk_wzrd->axi_clk); > } > > -static const struct of_device_id clk_wzrd_ids[] = { > - { .compatible = "xlnx,clocking-wizard" }, > - { .compatible = "xlnx,clocking-wizard-v5.2" }, > - { .compatible = "xlnx,clocking-wizard-v6.0" }, > - { }, > -}; > -MODULE_DEVICE_TABLE(of, clk_wzrd_ids); Don't move this.
