Re: [PATCH v4 2/2] clocking-wizard: Add support for versal clocking wizard

[Stephen Boyd <sboyd@kernel.org>]

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@amd.com>
> ---
> 
> 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@amd.com/
> 
> 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.