Hi, > 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 <zhengxu...@mxic.com.tw> > > Please use full name.
Okay, got it. > > > --- > > 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 > Okay, I will fix in the next version. > > + > > 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. Okay, got it. > > > @@ -0,0 +1,180 @@ > > +// 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 <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 Okay, got it. > > > + */ > > +struct clkwzd { > > + struct mutex lock; > > > CHECK: struct mutex definition without comment > #144: FILE: drivers/clk/clk_wizard.c:83: > + struct mutex lock; Okay, got it. > > > + 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? Okay, I will fix it. > > > +} > > + > > +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. Okay, I will remove this in next version. > > > + 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. Okay, got it. > > + > > +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. Okay, got it. > > + > > + /* Read clock confguration registers */ > > typo in comment. Okay, will fix it. > > + cfg = readl(priv->base + FBOUT_CFG); > > + > > + /* Recaculate VCO rate */ > > typo Okay, will fix it. > > + 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)); Okay. > > > + > > + 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), > > +}; > > Hope you’re having a great day! Thanks, Zhengxun CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. ===================================================================== ============================================================================ CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. =====================================================================