From: Dhaval Rajeshbhai Shah <DSHAH@xilinx.com>
To: Randy Dunlap <rdunlap@infradead.org>, "arnd@arndb.de" <arnd@arndb.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>
CC: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "michal.simek@xilinx.com" <michal.simek@xilinx.com>,
        Hyun Kwon <hyunk@xilinx.com>
Subject: RE: [PATCH v4 2/2] misc: xlnx_vcu: Add Xilinx ZYNQMP VCU logicoreIP
 init driver
Thread-Topic: [PATCH v4 2/2] misc: xlnx_vcu: Add Xilinx ZYNQMP VCU logicoreIP
 init driver
Thread-Index: AQHTdXXfzc8WifQZzEuO3wpYZwF/YaNGjJGAgACCMqCAAMKOgIAAx/hQ
Date: Mon, 18 Dec 2017 05:37:17 +0000
Message-ID: <BY2PR0201MB1879B8A82CCF5A9FB20135AAC10E0@BY2PR0201MB1879.namprd02.prod.outlook.com>
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>
 <d9e263f9-827c-656d-77fd-bbbb2b172040@infradead.org>
 <BY2PR0201MB1879AFE10931133E229C0849C1090@BY2PR0201MB1879.namprd02.prod.outlook.com>
 <6758f505-2940-feb9-a14a-9d5e3962f7f2@infradead.org>
In-Reply-To: <6758f505-2940-feb9-a14a-9d5e3962f7f2@infradead.org>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4f5be1cb-a1fa-400a-66f2-08d545d987d7
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Dec 2017 05:37:17.8906
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR0201MB1007
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id vBI5cSLo003149
Content-Length: 7245
Lines: 213

Hi Randy,

> -----Original Message-----
> From: Randy Dunlap [mailto:rdunlap@infradead.org]
> Sent: Sunday, December 17, 2017 9:40 AM
> To: Dhaval Rajeshbhai Shah <DSHAH@xilinx.com>; 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 <hyunk@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:rdunlap@infradead.org]
> >> Sent: Saturday, December 16, 2017 2:18 PM
> >> To: Dhaval Rajeshbhai Shah <DSHAH@xilinx.com>; 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 <hyunk@xilinx.com>; Dhaval
> >> Rajeshbhai Shah <DSHAH@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 <dshah@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 <dshah@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