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?
>>> + 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
