[PATCHv4,1/1] clk: zynq: Add clock wizard driver
Sean Anderson
sean.anderson at seco.com
Thu Jun 3 16:49:45 CEST 2021
On 6/2/21 9:56 AM, Zhengxun Li wrote:
> 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".
Yes, but while they are using CCF, this driver is not.
--Sean
>
> https://patchwork.kernel.org/project/linux-clk/patch/1617886723-27117-3-git-send-email-shubhrajyoti.datta@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 <zhengxunli at 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
>
More information about the U-Boot
mailing list