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@xxxxxxxxxxxxx] >> Sent: Saturday, December 16, 2017 2:18 PM >> To: Dhaval Rajeshbhai Shah <DSHAH@xxxxxxxxxx>; arnd@xxxxxxxx; >> gregkh@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx >> Cc: devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >> michal.simek@xxxxxxxxxx; Hyun Kwon <hyunk@xxxxxxxxxx>; Dhaval Rajeshbhai >> Shah <DSHAH@xxxxxxxxxx> >> 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 <dshah@xxxxxxxxxx> >>> --- >>> >>> 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 <dshah@xxxxxxxxxx> >>> + */ >>> +#include <linux/clk.h> >>> +#include <linux/device.h> >>> +#include <linux/errno.h> >>> +#include <linux/io.h> >>> +#include <linux/module.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/platform_device.h> >> >> [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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
