Hi Randy,
> -----Original Message----- > From: Randy Dunlap [mailto:rdun...@infradead.org] > Sent: Sunday, December 17, 2017 9:40 AM > To: Dhaval Rajeshbhai Shah <ds...@xilinx.com>; a...@arndb.de; > gre...@linuxfoundation.org; robh...@kernel.org; mark.rutl...@arm.com > Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; > michal.si...@xilinx.com; Hyun Kwon <hy...@xilinx.com> > Subject: Re: [PATCH v4 2/2] misc: xlnx_vcu: Add Xilinx ZYNQMP VCU logicoreIP > init driver > > 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:rdun...@infradead.org] > >> Sent: Saturday, December 16, 2017 2:18 PM > >> To: Dhaval Rajeshbhai Shah <ds...@xilinx.com>; a...@arndb.de; > >> gre...@linuxfoundation.org; robh...@kernel.org; mark.rutl...@arm.com > >> Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; > >> michal.si...@xilinx.com; Hyun Kwon <hy...@xilinx.com>; Dhaval > >> Rajeshbhai Shah <ds...@xilinx.com> > >> 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 <ds...@xilinx.com> > >>> --- > >>> > >>> 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 <ds...@xilinx.com> > >>> + */ > >>> +#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? > Yes. You are right. It will miss index 0 in that case. I will update and send the next patch set. > > >>> + 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
