Re: [PATCHv4,1/1] clk: zynq: Add clock wizard driver

[Sean Anderson <sean.anderson@seco.com>]

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@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
>