Hi Randy, Thanks a lot for the review. > -----Original Message----- > From: Randy Dunlap [mailto:rdunlap@xxxxxxxxxxxxx] > Sent: Thursday, December 14, 2017 10:43 AM > To: Dhaval Rajeshbhai Shah <DSHAH@xxxxxxxxxx>; arnd@xxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; pombredanne@xxxxxxxx; 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 v3 2/2] misc: Add Xilinx ZYNQMP VCU logicoreIP init driver > > On 12/13/2017 09:55 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> > > --- > > Changes since v3: > > No Changes. > > Changes since v2: > > * Removed the "default n" from the Kconfig > > * More help text added to explain more about the logicoreIP driver > > * SPDX id is relocated at top of the file with // style comment > > * Removed the export API and header file and make it a single driver > > which provides logocoreIP init. > > * Provide the information in commit message as well for the why driver > > in drivers/misc. > > > > drivers/misc/Kconfig | 15 ++ > > drivers/misc/Makefile | 1 + > > drivers/misc/xlnx_vcu.c | 629 > > ++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 645 insertions(+) > > create mode 100644 drivers/misc/xlnx_vcu.c > > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index > > f1a5c23..24ea516 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -496,6 +496,21 @@ config PCI_ENDPOINT_TEST > > Enable this configuration option to enable the host side test driver > > for PCI Endpoint. > > > > +config XILINX_VCU > > + tristate "Xilinx VCU logicoreIP Init" > > + help > > Please indent the help text (below) by 2 additional spaces, as documented in > coding-style.rst. I will update this. > > > + Provides the driver to enable and disable the isolation between the > > + processing system and programmable logic part by using the > logicoreIP > > + register set. This driver also configure the frequency based on > > +the > > configures > I will update this. > > + clock information get from the logicoreIP register set. > > drop ^get^ > I will update this. > > + > > + If you say yes here you get support for the logcoreIP. > > logicoreIP. > > > + > > + If unsure, say N. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called xlnx_vcu. > > + > > source "drivers/misc/c2port/Kconfig" > > source "drivers/misc/eeprom/Kconfig" > > source "drivers/misc/cb710/Kconfig" > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index > > 5ca5f64..a6bd0b1 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -55,6 +55,7 @@ obj-$(CONFIG_CXL_BASE) += cxl/ > > obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o > > obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o > > obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o > > +obj-$(CONFIG_XILINX_VCU) += xlnx_vcu.o > > > > lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o > > lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o > > diff --git a/drivers/misc/xlnx_vcu.c b/drivers/misc/xlnx_vcu.c new > > file mode 100644 index 0000000..41819f0 > > --- /dev/null > > +++ b/drivers/misc/xlnx_vcu.c > > @@ -0,0 +1,629 @@ > > +// 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> > > +> +/* Address map for different registers implemented in the VCU > > +> +LogiCORE IP. */ > > [snip] > > > > +/** > > + * xvcu_read - Read from the VCU register space > > + * @iomem: vcu reg space base address > > + * @offset: vcu reg offset from base > > + * > > + * Return: Returns 32bit value from VCU register specified > > + * > > + */ > > +static u32 xvcu_read(void __iomem *iomem, u32 offset) > > You could inline the read() and write() functions... > I will check and will update if required. > > +{ > > + return ioread32(iomem + offset); > > +} > > + > > +/** > > + * xvcu_write - Write to the VCU register space > > + * @iomem: vcu reg space base address > > + * @offset: vcu reg offset from base > > + * @value: Value to write > > + */ > > +static void xvcu_write(void __iomem *iomem, u32 offset, u32 value) { > > + iowrite32(value, iomem + offset); > > +} > > + > > +/** > > + * xvcu_write_field_reg - Write to the vcu reg field > > + * @iomem: vcu reg space base address > > + * @offset: vcu reg offset from base > > + * @field: vcu reg field to write to > > + * @mask: vcu reg mask > > + * @shift: vcu reg number of bits to shift the bitfield > > + */ > > +static void xvcu_write_field_reg(void __iomem *iomem, int offset, > > + u32 field, u32 mask, int shift) > > +{ > > + u32 val = xvcu_read(iomem, offset); > > + > > + val &= ~(mask << shift); > > + val |= (field & mask) << shift; > > + > > + xvcu_write(iomem, offset, val); > > +} > > + > > +/** > > + * 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) > > multi-line comment style: > /* > * The divide-by-2 should always be enabled (==1) > I will update this. > > + * 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--) { > > + 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; > > + } > > + } > > + } > > + > > + if (!found) { > > + dev_err(xvcu->dev, "Invalid clock combination.\n"); > > + return -EINVAL; > > + } > > + > > + xvcu->coreclk = pll_clk / divisor_core; > > + mcuclk = pll_clk / divisor_mcu; > > + dev_dbg(xvcu->dev, "Actual Ref clock freq is %uHz\n", refclk); > > + dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", xvcu->coreclk); > > + dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk); > > + > > + vcu_pll_ctrl &= ~(VCU_PLL_CTRL_FBDIV_MASK << > VCU_PLL_CTRL_FBDIV_SHIFT); > > + vcu_pll_ctrl |= (found->fbdiv & VCU_PLL_CTRL_FBDIV_MASK) << > > + VCU_PLL_CTRL_FBDIV_SHIFT; > > + vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK << > > + VCU_PLL_CTRL_POR_IN_SHIFT); > > + vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & > VCU_PLL_CTRL_POR_IN_MASK) << > > + VCU_PLL_CTRL_POR_IN_SHIFT; > > + vcu_pll_ctrl &= ~(VCU_PLL_CTRL_PWR_POR_MASK << > > + VCU_PLL_CTRL_PWR_POR_SHIFT); > > + vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & > VCU_PLL_CTRL_PWR_POR_MASK) << > > + VCU_PLL_CTRL_PWR_POR_SHIFT; > > + xvcu_write(xvcu->vcu_slcr_ba, VCU_PLL_CTRL, vcu_pll_ctrl); > > + > > + /* Set divisor for the core and mcu clock */ > > + ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL); > > + ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT); > > + ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) << > > + VCU_PLL_DIVISOR_SHIFT; > > + ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT); > > + ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << > VCU_SRCSEL_SHIFT; > > + xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL, ctrl); > > + > > + ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL); > > + ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT); > > + ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) << > > + VCU_PLL_DIVISOR_SHIFT; > > + ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT); > > + ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << > VCU_SRCSEL_SHIFT; > > + xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL, ctrl); > > + > > + ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL); > > + ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT); > > + ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << > VCU_PLL_DIVISOR_SHIFT; > > + ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT); > > + ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << > VCU_SRCSEL_SHIFT; > > + xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL, ctrl); > > + > > + ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL); > > + ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT); > > + ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << > VCU_PLL_DIVISOR_SHIFT; > > + ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT); > > + ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << > VCU_SRCSEL_SHIFT; > > + xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL, ctrl); > > + > > + /* Set RES, CP, LFHF, LOCK_CNT and LOCK_DLY cfg values */ > > + cfg_val = (found->res << VCU_PLL_CFG_RES_SHIFT) | > > + (found->cp << VCU_PLL_CFG_CP_SHIFT) | > > + (found->lfhf << VCU_PLL_CFG_LFHF_SHIFT) | > > + (found->lock_cnt << VCU_PLL_CFG_LOCK_CNT_SHIFT) | > > + (found->lock_dly << VCU_PLL_CFG_LOCK_DLY_SHIFT); > > + xvcu_write(xvcu->vcu_slcr_ba, VCU_PLL_CFG, cfg_val); > > + > > + return 0; > > +} > > + > > +/** > > + * xvcu_set_pll - PLL init sequence > > + * @xvcu: Pointer to the xvcu_device structure > > + * > > + * Call the api to set the PLL info and once that is done then > > + * init the PLL sequence to make the PLL stable. > > + * > > + * Return: Returns status, either success or error+reason > > + */ > > +static int xvcu_set_pll(struct xvcu_device *xvcu) { > > + u32 lock_status; > > + unsigned long timeout; > > + int ret; > > + > > + ret = xvcu_set_vcu_pll_info(xvcu); > > + if (ret) { > > + dev_err(xvcu->dev, "failed to set pll info\n"); > > + return ret; > > + } > > + > > + xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL, > > + 1, VCU_PLL_CTRL_BYPASS_MASK, > > + VCU_PLL_CTRL_BYPASS_SHIFT); > > + xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL, > > + 1, VCU_PLL_CTRL_RESET_MASK, > > + VCU_PLL_CTRL_RESET_SHIFT); > > + xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL, > > + 0, VCU_PLL_CTRL_RESET_MASK, > > + VCU_PLL_CTRL_RESET_SHIFT); > > + /* Defined the timeout for the max time to wait the > > comment style. > I will update this. > > + * PLL_STATUS to be locked. > > + */ > > + timeout = jiffies + msecs_to_jiffies(2000); > > + do { > > + lock_status = xvcu_read(xvcu->vcu_slcr_ba, VCU_PLL_STATUS); > > + if (lock_status & VCU_PLL_STATUS_LOCK_STATUS_MASK) { > > + xvcu_write_field_reg(xvcu->vcu_slcr_ba, > VCU_PLL_CTRL, > > + 0, VCU_PLL_CTRL_BYPASS_MASK, > > + VCU_PLL_CTRL_BYPASS_SHIFT); > > + return 0; > > + } > > + } while (!time_after(jiffies, timeout)); > > + > > + /* PLL is not locked even after the timeout of the 2sec */ > > + dev_err(xvcu->dev, "PLL is not locked\n"); > > + return -ETIMEDOUT; > > +} > > + > > +/** > > + * xvcu_probe - Probe existence of the logicoreIP > > + * and initialize PLL > > + * > > + * @pdev: Pointer to the platform_device structure > > + * > > + * Return: Returns 0 on success > > + * Negative error code otherwise > > + */ > > +static int xvcu_probe(struct platform_device *pdev) { > > + struct resource *res; > > + struct xvcu_device *xvcu; > > + int ret; > > + > > + xvcu = devm_kzalloc(&pdev->dev, sizeof(*xvcu), GFP_KERNEL); > > + if (!xvcu) > > + return -ENOMEM; > > + > > + xvcu->dev = &pdev->dev; > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > "vcu_slcr"); > > + if (!res) { > > + dev_err(&pdev->dev, "get vcu_slcr memory resource > failed.\n"); > > + return -ENODEV; > > + } > > + > > + xvcu->vcu_slcr_ba = devm_ioremap_nocache(&pdev->dev, > > + res->start, resource_size(res)); > > + if (!xvcu->vcu_slcr_ba) { > > + dev_err(&pdev->dev, "vcu_slcr register mapping failed.\n"); > > + return -ENOMEM; > > + } > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > "logicore"); > > + if (!res) { > > + dev_err(&pdev->dev, "get logicore memory resource > failed.\n"); > > + return -ENODEV; > > + } > > + > > + xvcu->logicore_reg_ba = devm_ioremap_nocache(&pdev->dev, > > + res->start, resource_size(res)); > > + if (!xvcu->logicore_reg_ba) { > > + dev_err(&pdev->dev, "logicore register mapping failed.\n"); > > + return -ENOMEM; > > + } > > + > > + xvcu->aclk = devm_clk_get(&pdev->dev, "aclk"); > > + if (IS_ERR(xvcu->aclk)) { > > + dev_err(&pdev->dev, "Could not get aclk clock\n"); > > + return PTR_ERR(xvcu->aclk); > > + } > > + > > + xvcu->pll_ref = devm_clk_get(&pdev->dev, "pll_ref"); > > + if (IS_ERR(xvcu->pll_ref)) { > > + dev_err(&pdev->dev, "Could not get pll_ref clock\n"); > > + return PTR_ERR(xvcu->pll_ref); > > + } > > + > > + ret = clk_prepare_enable(xvcu->aclk); > > + if (ret) { > > + dev_err(&pdev->dev, "aclk clock enable failed\n"); > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(xvcu->pll_ref); > > + if (ret) { > > + dev_err(&pdev->dev, "pll_ref clock enable failed\n"); > > + goto error_aclk; > > + } > > + > > + /* Do the Gasket isolation and put the VCU out of reset > > comment style. > I will update this. > > + * Bit 0 : Gasket isolation > > + * Bit 1 : put VCU out of reset > > + */ > > + xvcu_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, > > +VCU_GASKET_VALUE); > > + > > + /* Do the PLL Settings based on the ref clk,core and mcu clk freq */ > > + ret = xvcu_set_pll(xvcu); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to set the pll\n"); > > + goto error_pll_ref; > > + } > > + > > + dev_set_drvdata(&pdev->dev, xvcu); > > + > > + dev_info(&pdev->dev, "%s: Probed successfully\n", __func__); > > + > > + return 0; > > + > > +error_pll_ref: > > + clk_disable_unprepare(xvcu->pll_ref); > > +error_aclk: > > + clk_disable_unprepare(xvcu->aclk); > > + return ret; > > +} > > + > > +/** > > + * xvcu_remove - Insert gasket isolation > > + * and disable the clock > > + * @pdev: Pointer to the platform_device structure > > + * > > + * Return: Returns 0 on success > > + * Negative error code otherwise > > + */ > > +static int xvcu_remove(struct platform_device *pdev) { > > + struct xvcu_device *xvcu; > > + > > + xvcu = platform_get_drvdata(pdev); > > + if (!xvcu) > > + return -ENODEV; > > + > > + /* Add the the Gasket isolation and put the VCU in reset. > > style. > I will update this. > > + */ > > + xvcu_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0); > > + > > + clk_disable_unprepare(xvcu->pll_ref); > > + clk_disable_unprepare(xvcu->aclk); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id xvcu_of_id_table[] = { > > + { .compatible = "xlnx,vcu" }, > > + { .compatible = "xlnx,vcu-logicoreip-1.0" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, xvcu_of_id_table); > > + > > +static struct platform_driver xvcu_driver = { > > + .driver = { > > + .name = "xilinx-vcu", > > + .of_match_table = xvcu_of_id_table, > > + }, > > + .probe = xvcu_probe, > > + .remove = xvcu_remove, > > +}; > > + > > +module_platform_driver(xvcu_driver); > > + > > +MODULE_AUTHOR("Dhaval Shah <dshah@xxxxxxxxxx>"); > > +MODULE_DESCRIPTION("Xilinx VCU init Driver"); MODULE_LICENSE("GPL > > +v2"); > > > > The code has several divide operations. Does it build OK for 32-bit target? > Yes. I have verified on both 32 bit and 64 bit target. > -- > ~Randy Thanks & Regards, Dhaval ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
