From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DATE_IN_FUTURE_06_12, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DE298C4708F for ; Wed, 2 Jun 2021 05:57:13 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C0968610E5 for ; Wed, 2 Jun 2021 05:57:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C0968610E5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 961C982EAC; Wed, 2 Jun 2021 07:57:10 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="P0OriIyS"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4518782EBB; Wed, 2 Jun 2021 07:57:09 +0200 (CEST) Received: from mail-io1-xd32.google.com (mail-io1-xd32.google.com [IPv6:2607:f8b0:4864:20::d32]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id CF89D82EAC for ; Wed, 2 Jun 2021 07:57:05 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=zhengxunli.mxic@gmail.com Received: by mail-io1-xd32.google.com with SMTP id v9so1251040ion.11 for ; Tue, 01 Jun 2021 22:57:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=sKvN0OeU7qn//n+vpkMO8d1W1BfnoPDdzVj2bPXWRtg=; b=P0OriIySctT2k+nGqR/HfTktvQ/xFsaMMQUZvOfGXxVfIwmHzO2VlY2AVc9Kh/V/wB PPAodmae7O2WkmUodEuaEJf6S4q2AvNZuvtbHmOTv4qoNEgnU17+vrrZdKshcClVhKAx FBBEmgCbF/l80IooQQ2fV/tLlGqVqJu9IW1PVE7BP2ZYhSWhrDfbqrgcsY5hob1CSggD Ces8wqV6lOupFluSFJ2Qz3iWq6PMITFZ8MUQI8cFxgCjVCKgjzOsC+7+QzKPifAKHh05 NofDg5vNKwEQG+sA+yiIGF75+UkM8s0t4/h3oLXcN4dU0FN1ahJ70AGeGSGxWECpNUS3 1Bqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=sKvN0OeU7qn//n+vpkMO8d1W1BfnoPDdzVj2bPXWRtg=; b=r+zd73d/yOT1fcZNIkgSU/Laj60uvwOnPXF6MaduixKydXiPJB3Lyh35x3A0abdRIR 5N91a5GOzDe0OJgkO7IA9Qp1cEzzje8E9SfsMTw6gBE79q5x8lC55VbQHVA/oGa0i95t T9SgpYlvAsltofuk/qCZCHJxR1KhuQsmUk5nJaHrJXMYxDDUlPbUdtVuHvZhBY1qopWM O2/wXqAyj/XFcdjnPPGAbGPlOloDIAs2B+gYmaNysDggpyxFTwvEcNKyPCtaHziyhumC sOWXrIWjLpauWZzPUd5wElFt6JEG2hjzj0pYHhwLmO2Y0HJ9xlnylOJwA7CCZc7uVaWQ vwAA== X-Gm-Message-State: AOAM5317ee0GmbBGkLs6w4OmglD9EBhEPrZiHEN879N21tJ+WcjjqU+G iIpRE5h8BuN6rqoL5py8Za20h30PcuwrdksJ7wY= X-Google-Smtp-Source: ABdhPJwYKiUY67nXC6mQAOs5TJcdwpEClCkQIe3Fg5LmNJUB7Uo0cH0lrV1KObbyQLnKeN9nwGvNy7p5qXVHc4nRgeA= X-Received: by 2002:a02:9542:: with SMTP id y60mr8812838jah.134.1622613424502; Tue, 01 Jun 2021 22:57:04 -0700 (PDT) MIME-Version: 1.0 From: Zhengxun Li Date: Wed, 2 Jun 2021 13:56:27 +0000 Message-ID: Subject: Re: [PATCHv4,1/1] clk: zynq: Add clock wizard driver To: sean.anderson@seco.com, zhengxunli@mxic.com.tw, u-boot@lists.denx.de, Michal Simek , lukma@denx.de Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.102.4 at phobos.denx.de X-Virus-Status: Clean 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 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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