[PATCH 1/2] clk: zynq: Add clock wizard driver

[Michal Simek]

On 4/7/21 11:05 AM, zhengxunli wrote:
> The Clocking Wizard IP supports clock circuits customized
> to your clocking requirements. The wizard support for
> dynamically reconfiguring the clocking primitives for
> Multiply, Divide, Phase Shift/Offset, or Duty Cycle.
> 
> Limited by uboot clk uclass without set_phase API, this
> patch only provides set_rate to modify the frequency and
> set 50% duty cycle by default.
> 
> Signed-off-by: zhengxunli <zhengxunli at mxic.com.tw>

Please use full name.

> ---
>  drivers/clk/Kconfig      |   7 ++
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk_wizard.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 drivers/clk/clk_wizard.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 4aeaa0c..4ebeccc 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -136,6 +136,13 @@ config CLK_ZYNQMP
>  	  This clock driver adds support for clock realted settings for
>  	  ZynqMP platform.
>  
> +config CLK_WIZARD

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/clocking-wizard/Kconfig?h=v5.12-rc6

Small alignment with kernel would be useful.
At least CLK_XLNX_CLKWZRD.


> +	bool "Enable clock wizard driver support for zynq"
> +	depends on CLK && ARCH_ZYNQ

Clocking wizard is standard PL based IP not just related to Zynq. It can
be used by Microblaze, ARM cores, etc. It means no need to have
dependency on ZYNQ here.

> +	help
> +	  This clock driver adds support for clock wizard setting for
> +	  Zynq platform.

ditto

> +
>  config CLK_STM32MP1
>  	bool "Enable RCC clock driver for STM32MP1"
>  	depends on ARCH_STM32MP && CLK
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 645709b..d8b878c 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/
>  obj-$(CONFIG_CLK_VEXPRESS_OSC) += clk_vexpress_osc.o
>  obj-$(CONFIG_CLK_ZYNQ) += clk_zynq.o
>  obj-$(CONFIG_CLK_ZYNQMP) += clk_zynqmp.o
> +obj-$(CONFIG_CLK_WIZARD) += clk_wizard.o
>  obj-$(CONFIG_ICS8N3QV01) += ics8n3qv01.o
>  obj-$(CONFIG_MACH_PIC32) += clk_pic32.o
>  obj-$(CONFIG_SANDBOX) += clk_sandbox.o
> diff --git a/drivers/clk/clk_wizard.c b/drivers/clk/clk_wizard.c
> new file mode 100644
> index 0000000..f5c2387
> --- /dev/null
> +++ b/drivers/clk/clk_wizard.c

name could be also aligned with kernel to have easier match with the kernel.

> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx 'Clocking Wizard' driver
> + *
> + * Copyright (c) 2021 Macronix Inc.
> + *
> + * Author: Zhengxun Li <zhengxunli at mxic.com.tw>
> + */
> +
> +#include <common.h>
> +#include <clk-uclass.h>
> +#include <dm.h>
> +#include <div64.h>
> +#include <linux/iopoll.h>
> +
> +#define SRR			0x0
> +
> +#define SR			0x4
> +#define SR_LOCKED		BIT(0)
> +
> +#define CCR(x)			(0x200 + ((x) * 4))
> +
> +#define FBOUT_CFG		CCR(0)
> +#define FBOUT_DIV(x)		(x)
> +#define FBOUT_GET_DIV(x)	((x) & GENMASK(7, 0))
> +#define FBOUT_MUL(x)		((x) << 8)
> +#define FBOUT_GET_MUL(x)	(((x) & GENMASK(15, 8)) >> 8)
> +#define FBOUT_FRAC(x)		((x) << 16)
> +#define FBOUT_GET_FRAC(x)	(((x) & GENMASK(25, 16)) >> 16)
> +#define FBOUT_FRAC_EN		BIT(26)
> +
> +#define FBOUT_PHASE		CCR(1)
> +
> +#define OUT_CFG(x)		CCR(2 + ((x) * 3))
> +#define OUT_DIV(x)		(x)
> +#define OUT_GET_DIV(x)		((x) & GENMASK(7, 0))
> +#define OUT_FRAC(x)		((x) << 8)
> +#define OUT_GET_FRAC(x)		(((x) & GENMASK(17, 8)) >> 8)
> +#define OUT_FRAC_EN		BIT(18)
> +
> +#define OUT_PHASE(x)		CCR(3 + ((x) * 3))
> +#define OUT_DUTY(x)		CCR(4 + ((x) * 3))
> +
> +#define CTRL			CCR(23)
> +#define CTRL_SEN		BIT(2)
> +#define CTRL_SADDR		BIT(1)
> +#define CTRL_LOAD		BIT(0)
> +
> +/*

/** for kernel-doc as noted below.

> + *	   MMCM Block Diagram
> + *
> + *	   +----------------+  +-----------------+
> + * input ->| vco_clk_div_hw |->| vco_clk_mul_hw  |--+
> + * rate    | (int divide)   |  | (frac multiply) |  |
> + *	   +----------------+  +-----------------+  |
> + *						    |
> + *	+--------------------------------VCO-rate---+
> + *	|
> + *	|  +----------------+
> + *	+->| clkout[0]	    |-> output0 rate
> + *	|  | (frac divide)  |
> + *	|  +----------------+
> + *	|
> + *	|  +----------------+
> + *	+->| clkout[1]	    |-> output1 rate
> + *	|  | (int divide)   |
> + *	|  +----------------+
> + *	|
> + *     ...
> + *	|
> + *	|  +----------------+
> + *	+->| clkout[1]	    |-> output6 rate
> + *	   | (int divide)   |
> + *	   +----------------+
> + *
> + * struct clkwzrd - Clock wizard private data structure
> + *
> + * @lock		Lock pointer
> + * @base		Memory base
> + * @vco_clk		voltage-controlled oscillator frequency

This is not kernel-doc format but it looks like you are sort of using it.
Fix it and then run:
./scripts/kernel-doc -man -v drivers/clk/clk_wizard.c 1>/dev/null


> + */
> +struct clkwzd {
> +	struct mutex lock;


CHECK: struct mutex definition without comment
#144: FILE: drivers/clk/clk_wizard.c:83:
+	struct mutex lock;

> +	void __iomem *base;
> +	u64 vco_clk;
> +};
> +
> +static int zynq_clk_wizard_enable(struct clk *clk)
> +{
> +	struct clkwzd *priv = dev_get_priv(clk->dev);
> +	int ret;
> +	u32 val;
> +
> +	mutex_lock(&priv->lock);
> +	ret = readl_poll_sleep_timeout(priv->base + SR, val, val & SR_LOCKED,
> +				       1, 100);
> +	if (!ret) {
> +		writel(CTRL_SEN | CTRL_SADDR | CTRL_LOAD, priv->base + CTRL);
> +		writel(CTRL_SADDR, priv->base + CTRL);
> +		ret = readl_poll_sleep_timeout(priv->base + SR, val,
> +					       val & SR_LOCKED, 1, 100);
> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;

return ret?

> +}
> +
> +static unsigned long zynq_clk_wizard_set_rate(struct clk *clk, ulong rate)
> +{
> +	struct clkwzd *priv = dev_get_priv(clk->dev);
> +	u64 div;
> +	u32 cfg;
> +
> +	/* Get output clock divide value */
> +	div = DIV_ROUND_DOWN_ULL(priv->vco_clk * 1000, rate);
> +	if (div < 1000 || div > 255999)
> +		return -EINVAL;
> +
> +	cfg = OUT_DIV((u32)div / 1000);
> +
> +	writel(cfg, priv->base + OUT_CFG(clk->id));
> +
> +	/* Set duty cycle to 50%. */

Based on your comment style remove this . here.

> +	writel(50000, priv->base + OUT_DUTY(clk->id));
> +
> +	return 0;
> +}
> +
> +static struct clk_ops zynq_clk_wizard_ops = {
> +	.enable = zynq_clk_wizard_enable,
> +	.set_rate = zynq_clk_wizard_set_rate,
> +};
> +
> +static const struct udevice_id zynq_clk_wizard_ids[] = {
> +	{ .compatible = "xlnx,clk-wizard-5.1" },
> +	{ /* sentinel */ }
> +};

Please move these two structures below probe which is standard location.

> +
> +static int zynq_clk_wizard_probe(struct udevice *dev)
> +{
> +	struct clkwzd *priv = dev_get_priv(dev);
> +	fdt_addr_t addr;
> +	u64 clk_in1, vco_clk;
> +	u32 cfg;
> +
> +	addr = dev_read_addr(dev);
> +	if (addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;
> +
> +	priv->base = (void __iomem *)addr;
> +
> +	clk_in1 = dev_read_u32_default(dev, "clock-frequency", 0);

All these dt stuff should be done in of_to_plat function.

> +
> +	/* Read clock confguration registers */

typo in comment.

> +	cfg = readl(priv->base + FBOUT_CFG);
> +
> +	/* Recaculate VCO rate */

typo

> +	if (cfg & FBOUT_FRAC_EN)
> +		vco_clk = DIV_ROUND_DOWN_ULL(clk_in1 *
> +					  ((FBOUT_GET_MUL(cfg) * 1000) +
> +					   FBOUT_GET_FRAC(cfg)),
> +					  1000);
> +	else
> +		vco_clk = clk_in1 * FBOUT_GET_MUL(cfg);
> +
> +	vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));
> +
> +	priv->vco_clk = vco_clk;


just
priv->vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));


> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(zynq_clk_wizard) = {
> +	.name = "zynq-clk-wizard",
> +	.id = UCLASS_CLK,
> +	.of_match = zynq_clk_wizard_ids,
> +	.ops = &zynq_clk_wizard_ops,
> +	.probe = zynq_clk_wizard_probe,
> +	.priv_auto = sizeof(struct clkwzd),
> +};
> 

Thanks,
Michal