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

[Zhengxun Li <zhengxunli.mxic@gmail.com>]

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