Hi Sean, Thanks for your reply.
>> This clock driver adds support for clock related settings for >> Zynq platform. >> >> +config COMMON_CLK_XLNX_CLKWZRD > > Why not just CLK_XILNIX_WIZARD? Do we need "COMMON" in here? Follow the linux patch "clk: clocking-wizard: Add the clockwizard to clk directory". https://patchwork.kernel.org/project/linux-clk/patch/1617886723-27117-3-git-send-email-shubhrajyoti.da...@xilinx.com/ >> + bool "Xilinx Clocking Wizard" >> + depends on CLK >> + help >> + Support for the Xilinx Clocking Wizard IP core clock generator. >> + Adds support for clocking wizard and compatible. >> + This driver supports the Xilinx clocking wizard programmable clock >> + synthesizer. > > This can all be one sentence. Please also add some basic information > about the clock wizard. What sorts of systems might one find this device > on? It looks like configuration is determined from registers within the > clock itself. Perhaps add a note about that. Okay. >> + >> config CLK_ZYNQMP >> bool "Enable clock driver support for ZynqMP" >> depends on ARCH_ZYNQMP >> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile >> index 645709b855..f4ddbe831a 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_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-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-xlnx-clock-wizard.c >> b/drivers/clk/clk-xlnx-clock-wizard.c >> new file mode 100644 >> index 0000000000..76c0eb27e6 >> --- /dev/null >> +++ b/drivers/clk/clk-xlnx-clock-wizard.c >> @@ -0,0 +1,177 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Xilinx 'Clocking Wizard' driver >> + * >> + * Copyright (c) 2021 Macronix Inc. >> + * >> + * Author: Zhengxun Li <zhengxu...@mxic.com.tw> >> + */ >> + >> +#include <common.h> >> +#include <clk-uclass.h> >> +#include <dm.h> >> +#include <div64.h> >> +#include <dm/device_compat.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)) > > Please use FIELD_PREP and FIELD_GET. And please define the mask > separately. Okay. >> +#define OUT_FRAC(x) ((x) << 8) >> +#define OUT_GET_FRAC(x) (((x) & GENMASK(17, 8)) >> 8) > > ditto > >> +#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) >> + >> +/** >> + * struct clkwzrd - Clock wizard private data structure >> + * >> + * @base: memory base >> + * @vco_clk: voltage-controlled oscillator frequency >> + */ >> +struct clkwzd { >> + void *base; >> + u64 vco_clk; >> +}; >> + >> +struct clkwzd_plat { >> + fdt_addr_t addr; >> +}; >> + >> +static int clk_wzrd_enable(struct clk *clk) >> +{ >> + struct clkwzd *priv = dev_get_priv(clk->dev); >> + int ret; >> + u32 val; >> + >> + 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); >> + } >> + >> + return ret; >> +} >> + >> +static unsigned long clk_wzrd_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)); >> + >> + return 0; >> +} >> + >> +static struct clk_ops clk_wzrd_ops = { >> + .enable = clk_wzrd_enable, >> + .set_rate = clk_wzrd_set_rate, >> +}; >> + >> +static int clk_wzrd_probe(struct udevice *dev) >> +{ >> + struct clkwzd_plat *plat = dev_get_plat(dev); >> + struct clkwzd *priv = dev_get_priv(dev); >> + struct clk clk; >> + u64 clock, vco_clk; >> + u32 cfg; >> + int ret; >> + >> + priv->base = (void *)plat->addr; >> + >> + ret = clk_get_by_index(dev, 0, &clk); > > With reference to Linux's drivers/staging/clocking-wizard/dt-binding.txt, > please use clk_get_by_name(dev, "clk_in1", &clk) Okay. >> + if (ret < 0) { >> + dev_err(dev, "failed to get clock\n"); >> + return ret; >> + } >> + >> + clock = clk_get_rate(&clk); > > Please name this rate or something to that effect. Okay. >> + if (IS_ERR_VALUE(clock)) { >> + dev_err(dev, "failed to get rate\n"); >> + return clock; >> + } >> + >> + ret = clk_enable(&clk); >> + if (ret) { >> + dev_err(dev, "failed to enable clock\n"); > > Missing clk_free() Okay. Thanks, Zhengxun