Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757485AbdLQRkc (ORCPT ); Sun, 17 Dec 2017 12:40:32 -0500 Received: from merlin.infradead.org ([205.233.59.134]:59214 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757207AbdLQRk3 (ORCPT ); Sun, 17 Dec 2017 12:40:29 -0500 Subject: Re: [PATCH v4 2/2] misc: xlnx_vcu: Add Xilinx ZYNQMP VCU logicoreIP init driver To: Dhaval Rajeshbhai Shah , "arnd@arndb.de" , "gregkh@linuxfoundation.org" , "robh+dt@kernel.org" , "mark.rutland@arm.com" Cc: "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "michal.simek@xilinx.com" , Hyun Kwon References: <29198c0a-783e-8aa0-00e4-44b1fa1acef7@infradead.org> <1513322656-4571-1-git-send-email-dshah@xilinx.com> <1513322656-4571-3-git-send-email-dshah@xilinx.com> From: Randy Dunlap Message-ID: <6758f505-2940-feb9-a14a-9d5e3962f7f2@infradead.org> Date: Sun, 17 Dec 2017 09:40:25 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6261 Lines: 198 On 12/16/2017 10:07 PM, Dhaval Rajeshbhai Shah wrote: > Hi Randy, > > Thanks a lot for the review. > >> -----Original Message----- >> From: Randy Dunlap [mailto:rdunlap@infradead.org] >> Sent: Saturday, December 16, 2017 2:18 PM >> To: Dhaval Rajeshbhai Shah ; arnd@arndb.de; >> gregkh@linuxfoundation.org; robh+dt@kernel.org; mark.rutland@arm.com >> Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> michal.simek@xilinx.com; Hyun Kwon ; Dhaval Rajeshbhai >> Shah >> Subject: Re: [PATCH v4 2/2] misc: xlnx_vcu: Add Xilinx ZYNQMP VCU logicoreIP >> init driver >> >> On 12/14/2017 11:24 PM, Dhaval Shah wrote: >>> Xilinx ZYNQMP logicoreIP Init driver is based on the new LogiCoreIP >>> design created. This driver provides the processing system and >>> programmable logic isolation. Set the frequency based on the clock >>> information get from the logicoreIP register set. >>> >>> It is put in drivers/misc as there is no subsystem for this logicoreIP. >>> >>> Signed-off-by: Dhaval Shah >>> --- >>> >>> drivers/misc/Kconfig | 15 ++ >>> drivers/misc/Makefile | 1 + >>> drivers/misc/xlnx_vcu.c | 631 >>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 647 insertions(+) >>> create mode 100644 drivers/misc/xlnx_vcu.c >> >>> diff --git a/drivers/misc/xlnx_vcu.c b/drivers/misc/xlnx_vcu.c new >>> file mode 100644 index 0000000..f489d34 >>> --- /dev/null >>> +++ b/drivers/misc/xlnx_vcu.c >>> @@ -0,0 +1,631 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Xilinx VCU Init >>> + * >>> + * Copyright (C) 2016 - 2017 Xilinx, Inc. >>> + * >>> + * Contacts Dhaval Shah >>> + */ >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> [snip] >> >> >>> +/** >>> + * xvcu_set_vcu_pll_info - Set the VCU PLL info >>> + * @xvcu: Pointer to the xvcu_device structure >>> + * >>> + * Programming the VCU PLL based on the user configuration >>> + * (ref clock freq, core clock freq, mcu clock freq). >>> + * Core clock frequency has higher priority than mcu clock frequency >>> + * Errors in following cases >>> + * - When mcu or clock clock get from logicoreIP is 0 >>> + * - When VCU PLL DIV related bits value other than 1 >>> + * - When proper data not found for given data >>> + * - When sis570_1 clocksource related operation failed >>> + * >>> + * Return: Returns status, either success or error+reason >>> + */ >>> +static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu) { >>> + u32 refclk, coreclk, mcuclk, inte, deci; >>> + u32 divisor_mcu, divisor_core, fvco; >>> + u32 clkoutdiv, vcu_pll_ctrl, pll_clk; >>> + u32 cfg_val, mod, ctrl; >>> + int ret; >>> + unsigned int i; >>> + const struct xvcu_pll_cfg *found = NULL; >>> + >>> + inte = xvcu_read(xvcu->logicore_reg_ba, VCU_PLL_CLK); >>> + deci = xvcu_read(xvcu->logicore_reg_ba, VCU_PLL_CLK_DEC); >>> + coreclk = xvcu_read(xvcu->logicore_reg_ba, VCU_CORE_CLK) * MHZ; >>> + mcuclk = xvcu_read(xvcu->logicore_reg_ba, VCU_MCU_CLK) * MHZ; >>> + if (!mcuclk || !coreclk) { >>> + dev_err(xvcu->dev, "Invalid mcu and core clock data\n"); >>> + return -EINVAL; >>> + } >>> + >>> + refclk = (inte * MHZ) + (deci * (MHZ / FRAC)); >>> + dev_dbg(xvcu->dev, "Ref clock from logicoreIP is %uHz\n", refclk); >>> + dev_dbg(xvcu->dev, "Core clock from logicoreIP is %uHz\n", coreclk); >>> + dev_dbg(xvcu->dev, "Mcu clock from logicoreIP is %uHz\n", mcuclk); >>> + >>> + clk_disable_unprepare(xvcu->pll_ref); >>> + ret = clk_set_rate(xvcu->pll_ref, refclk); >>> + if (ret) >>> + dev_warn(xvcu->dev, "failed to set logicoreIP refclk rate\n"); >>> + >>> + ret = clk_prepare_enable(xvcu->pll_ref); >>> + if (ret) { >>> + dev_err(xvcu->dev, "failed to enable pll_ref clock source\n"); >>> + return ret; >>> + } >>> + >>> + refclk = clk_get_rate(xvcu->pll_ref); >>> + >>> + /* >>> + * The divide-by-2 should be always enabled (==1) >>> + * to meet the timing in the design. >>> + * Otherwise, it's an error >>> + */ >>> + vcu_pll_ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_PLL_CTRL); >>> + clkoutdiv = vcu_pll_ctrl >> VCU_PLL_CTRL_CLKOUTDIV_SHIFT; >>> + clkoutdiv = clkoutdiv && VCU_PLL_CTRL_CLKOUTDIV_MASK; >>> + if (clkoutdiv != 1) { >>> + dev_err(xvcu->dev, "clkoutdiv value is invalid\n"); >>> + return -EINVAL; >>> + } >>> + >>> + for (i = ARRAY_SIZE(xvcu_pll_cfg) - 1; i > 0; i--) { >> >> When does that for loop terminate? >> > This loop will terminate either it reach to i =0 and one other case is when it find the proper expected value. Break statement is used to exit in that case. Hm, well, I did mis-read it, but I think that there is still an off-by-one problem. Consider the case of the value is not found in the array. If ARRAY_SIZE(xvcu_pll_cfg) is 5: for (i = ARRAY_SIZE(xvcu_pll_cfg) - 1; i > 0; i--) { i == 4: check xvcu_pll_cfg[4] i--; i == 3: check xvcu_pll_cfg[3] i--; i == 2; check xvcu_pll_cfg[2] i--; i == 1: check xvcu_pll_cfg[1] i--; i == 0; terminate for-loop; xvcu_pll_cfg[0] is never checked; Is this clear? >>> + const struct xvcu_pll_cfg *cfg = &xvcu_pll_cfg[i]; >>> + >>> + fvco = cfg->fbdiv * refclk; >>> + if (fvco >= FVCO_MIN && fvco <= FVCO_MAX) { >>> + pll_clk = fvco / VCU_PLL_DIV2; >>> + if (fvco % VCU_PLL_DIV2 != 0) >>> + pll_clk++; >>> + mod = pll_clk % coreclk; >>> + if (mod < LIMIT) { >>> + divisor_core = pll_clk / coreclk; >>> + } else if (coreclk - mod < LIMIT) { >>> + divisor_core = pll_clk / coreclk; >>> + divisor_core++; >>> + } else { >>> + continue; >>> + } >>> + if (divisor_core >= DIVISOR_MIN && >>> + divisor_core <= DIVISOR_MAX) { >>> + found = cfg; >>> + divisor_mcu = pll_clk / mcuclk; >>> + mod = pll_clk % mcuclk; >>> + if (mcuclk - mod < LIMIT) >>> + divisor_mcu++; >>> + break; > This is the break statement to exit when proper value is find. >>> + } >>> + } >>> + } >>> + >>> + if (!found) { >>> + dev_err(xvcu->dev, "Invalid clock combination.\n"); >>> + return -EINVAL; >>> + } >> >> [snip] -- ~Randy