[Patch v2 0/2] Add Xilinx clock wizard driver support

[Patch v2 0/2] Add Xilinx clock wizard driver support

[Zhengxun Li]

This series add support to enable clock wizard for zynq platform.

Changes in v2:
- naming is aligned with linux
- delete inappropriate description and code

Zhengxun Li (2):
  clk: zynq: Add clock wizard driver
  board: Add Zynq Mxic picozed development board support

 arch/arm/dts/Makefile               |   3 +-
 arch/arm/dts/zynq-mxic-picozed.dts  |  72 +++++++++++++++++
 drivers/clk/Kconfig                 |   9 +++
 drivers/clk/Makefile                |   1 +
 drivers/clk/clk-xlnx-clock-wizard.c | 152 ++++++++++++++++++++++++++++++++++++
 5 files changed, 236 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/dts/zynq-mxic-picozed.dts
 create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c

-- 
1.9.1

[Patch v2 1/2] clk: zynq: Add clock wizard driver

[Zhengxun Li]

The Clocking Wizard IP supports clock circuits customized
to your clocking requirements. The wizard support for
dynamically reconfiguring the clocking primitives for
Multiply, Divide, Phase Shift/Offset, or Duty Cycle.

Limited by uboot clk uclass without set_phase API, this
patch only provides set_rate to modify the frequency.

Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
---
 drivers/clk/Kconfig                 |   9 +++
 drivers/clk/Makefile                |   1 +
 drivers/clk/clk-xlnx-clock-wizard.c | 152 ++++++++++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 4aeaa0c..f0d4fe0 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -136,6 +136,15 @@ config CLK_ZYNQMP
 	  This clock driver adds support for clock realted settings for
 	  ZynqMP platform.
 
+config COMMON_CLK_XLNX_CLKWZRD
+	bool "Xilinx Clocking Wizard"
+	depends on CLK
+	help
+	  Support for the Xilinx Clocking Wizard IP core clock generator.
+	  Adds support for clocking wizard and compatible.
+	  This driver supports the Xilinx clocking wizard programmable clock
+	  synthesizer.
+
 config CLK_STM32MP1
 	bool "Enable RCC clock driver for STM32MP1"
 	depends on ARCH_STM32MP && CLK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 645709b..f4ddbe8 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/
 obj-$(CONFIG_CLK_VEXPRESS_OSC) += clk_vexpress_osc.o
 obj-$(CONFIG_CLK_ZYNQ) += clk_zynq.o
 obj-$(CONFIG_CLK_ZYNQMP) += clk_zynqmp.o
+obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-wizard.o
 obj-$(CONFIG_ICS8N3QV01) += ics8n3qv01.o
 obj-$(CONFIG_MACH_PIC32) += clk_pic32.o
 obj-$(CONFIG_SANDBOX) += clk_sandbox.o
diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
new file mode 100644
index 0000000..55adb16
--- /dev/null
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx 'Clocking Wizard' driver
+ *
+ * Copyright (c) 2021 Macronix Inc.
+ *
+ * Author: Zhengxun Li <zhengxunli@mxic.com.tw>
+ */
+
+#include <common.h>
+#include <clk-uclass.h>
+#include <dm.h>
+#include <div64.h>
+#include <linux/iopoll.h>
+
+#define SRR			0x0
+
+#define SR			0x4
+#define SR_LOCKED		BIT(0)
+
+#define CCR(x)			(0x200 + ((x) * 4))
+
+#define FBOUT_CFG		CCR(0)
+#define FBOUT_DIV(x)		(x)
+#define FBOUT_GET_DIV(x)	((x) & GENMASK(7, 0))
+#define FBOUT_MUL(x)		((x) << 8)
+#define FBOUT_GET_MUL(x)	(((x) & GENMASK(15, 8)) >> 8)
+#define FBOUT_FRAC(x)		((x) << 16)
+#define FBOUT_GET_FRAC(x)	(((x) & GENMASK(25, 16)) >> 16)
+#define FBOUT_FRAC_EN		BIT(26)
+
+#define FBOUT_PHASE		CCR(1)
+
+#define OUT_CFG(x)		CCR(2 + ((x) * 3))
+#define OUT_DIV(x)		(x)
+#define OUT_GET_DIV(x)		((x) & GENMASK(7, 0))
+#define OUT_FRAC(x)		((x) << 8)
+#define OUT_GET_FRAC(x)		(((x) & GENMASK(17, 8)) >> 8)
+#define OUT_FRAC_EN		BIT(18)
+
+#define OUT_PHASE(x)		CCR(3 + ((x) * 3))
+#define OUT_DUTY(x)		CCR(4 + ((x) * 3))
+
+#define CTRL			CCR(23)
+#define CTRL_SEN		BIT(2)
+#define CTRL_SADDR		BIT(1)
+#define CTRL_LOAD		BIT(0)
+
+/*
+ * struct clkwzrd - Clock wizard private data structure
+ *
+ * @lock		Lock pointer
+ * @base		Memory base
+ * @vco_clk		voltage-controlled oscillator frequency
+ */
+struct clkwzd {
+	void __iomem *base;
+	u64 vco_clk;
+};
+
+static int clk_wzrd_enable(struct clk *clk)
+{
+	struct clkwzd *priv = dev_get_priv(clk->dev);
+	int ret;
+	u32 val;
+
+	ret = readl_poll_sleep_timeout(priv->base + SR, val, val & SR_LOCKED,
+				       1, 100);
+	if (!ret) {
+		writel(CTRL_SEN | CTRL_SADDR | CTRL_LOAD, priv->base + CTRL);
+		writel(CTRL_SADDR, priv->base + CTRL);
+		ret = readl_poll_sleep_timeout(priv->base + SR, val,
+					       val & SR_LOCKED, 1, 100);
+	}
+
+	return ret;
+}
+
+static unsigned long clk_wzrd_set_rate(struct clk *clk, ulong rate)
+{
+	struct clkwzd *priv = dev_get_priv(clk->dev);
+	u64 div;
+	u32 cfg;
+
+	/* Get output clock divide value */
+	div = DIV_ROUND_DOWN_ULL(priv->vco_clk * 1000, rate);
+	if (div < 1000 || div > 255999)
+		return -EINVAL;
+
+	cfg = OUT_DIV((u32)div / 1000);
+
+	writel(cfg, priv->base + OUT_CFG(clk->id));
+
+	return 0;
+}
+
+static int clk_wzrd_of_to_plat(struct udevice *dev)
+{
+	struct clkwzd *priv = dev_get_priv(dev);
+	fdt_addr_t addr;
+	u64 clk_in1, vco_clk;
+	u32 cfg;
+
+	addr = dev_read_addr(dev);
+	if (addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	priv->base = (void __iomem *)addr;
+
+	clk_in1 = dev_read_u32_default(dev, "clock-frequency", 0);
+
+	/* Read clock configuration registers */
+	cfg = readl(priv->base + FBOUT_CFG);
+
+	/* Recalculate VCO rate */
+	if (cfg & FBOUT_FRAC_EN)
+		vco_clk = DIV_ROUND_DOWN_ULL(clk_in1 *
+					     ((FBOUT_GET_MUL(cfg) * 1000) +
+					      FBOUT_GET_FRAC(cfg)),
+					     1000);
+	else
+		vco_clk = clk_in1 * FBOUT_GET_MUL(cfg);
+
+	priv->vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));
+
+	return 0;
+}
+
+static int clk_wzrd_probe(struct udevice *dev)
+{
+	return 0;
+}
+
+static struct clk_ops clk_wzrd_ops = {
+	.enable = clk_wzrd_enable,
+	.set_rate = clk_wzrd_set_rate,
+};
+
+static const struct udevice_id clk_wzrd_ids[] = {
+	{ .compatible = "xlnx,clk-wizard-5.1" },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(clk_wzrd) = {
+	.name = "zynq-clk-wizard",
+	.id = UCLASS_CLK,
+	.of_match = clk_wzrd_ids,
+	.ops = &clk_wzrd_ops,
+	.probe = clk_wzrd_probe,
+	.of_to_plat = clk_wzrd_of_to_plat,
+	.priv_auto = sizeof(struct clkwzd),
+};
-- 
1.9.1

[Patch v2 2/2] board: Add Zynq Mxic picozed development board support

[Zhengxun Li]

Add the Zynq Mxic picozed development board support.

Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
---
 arch/arm/dts/Makefile              |  3 +-
 arch/arm/dts/zynq-mxic-picozed.dts | 72 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/dts/zynq-mxic-picozed.dts

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 1dd6c4b..b5addc4 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -287,7 +287,8 @@ dtb-$(CONFIG_ARCH_ZYNQ) += \
 	zynq-zturn.dtb \
 	zynq-zturn-v5.dtb \
 	zynq-zybo.dtb \
-	zynq-zybo-z7.dtb
+	zynq-zybo-z7.dtb \
+	zynq-mxic-picozed.dtb
 dtb-$(CONFIG_ARCH_ZYNQMP) += \
 	avnet-ultra96-rev1.dtb			\
 	avnet-ultrazedev-cc-v1.0-ultrazedev-som-v1.0.dtb	\
diff --git a/arch/arm/dts/zynq-mxic-picozed.dts b/arch/arm/dts/zynq-mxic-picozed.dts
new file mode 100644
index 0000000..1f24ca1
--- /dev/null
+++ b/arch/arm/dts/zynq-mxic-picozed.dts
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 Macronix Inc.
+ *
+ * Author: Zhengxun Li <zhengxunli@mxic.com.tw>
+ */
+
+/dts-v1/;
+/include/ "zynq-7000.dtsi"
+
+/ {
+	model = "Zynq MXIC PicoZed Development Board";
+	compatible = "mxicy,zynq-mxic-picozed", "xlnx,zynq-7000";
+
+	aliases {
+		ethernet0 = &gem0;
+		serial0 = &uart1;
+		spi0 = &spi_controller;
+	};
+
+	memory at 0 {
+		device_type = "memory";
+		reg = <0x0 0x30000000>;
+	};
+
+	chosen {
+		bootargs = "";
+		stdout-path = "serial0:115200n8";
+	};
+};
+
+&amba {
+	clkwizard: clkwizard at 43c20000 {
+		compatible = "xlnx,clk-wizard-5.1";
+		reg = <0x43c20000 0x10000>;
+		clocks = <&clkc 18>, <&clkc 18>;
+		clock-names = "aclk", "clk_in1";
+		#clock-cells = <1>;
+		clock-frequency = <133300000>;
+	};
+
+	spi_controller: spi at 43c30000 {
+		compatible = "mxicy,mx25f0a-spi";
+		reg = <0x43c30000 0x10000>;
+		reg-names = "regs";
+		clocks = <&clkwizard 0>, <&clkwizard 1>, <&clkc 18>;
+		clock-names = "send_clk", "send_dly_clk", "ps_clk";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		flash at 0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-max-frequency = <25000000>;
+			spi-tx-bus-width = <1>;
+			spi-rx-bus-width = <1>;
+		};
+	};
+};
+
+&clkc {
+	ps-clk-frequency = <33333333>;
+};
+
+&gem0 {
+	status = "okay";
+	phy-mode = "rgmii-id";
+};
+
+&uart1 {
+	status = "okay";
+};
-- 
1.9.1

[Patch v2 2/2] board: Add Zynq Mxic picozed development board support

[Michal Simek]

On 4/29/21 11:31 AM, Zhengxun Li wrote:
> Add the Zynq Mxic picozed development board support.
> 
> Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> ---
>  arch/arm/dts/Makefile              |  3 +-
>  arch/arm/dts/zynq-mxic-picozed.dts | 72 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/dts/zynq-mxic-picozed.dts
> 
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index 1dd6c4b..b5addc4 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -287,7 +287,8 @@ dtb-$(CONFIG_ARCH_ZYNQ) += \
>  	zynq-zturn.dtb \
>  	zynq-zturn-v5.dtb \
>  	zynq-zybo.dtb \
> -	zynq-zybo-z7.dtb
> +	zynq-zybo-z7.dtb \
> +	zynq-mxic-picozed.dtb
>  dtb-$(CONFIG_ARCH_ZYNQMP) += \
>  	avnet-ultra96-rev1.dtb			\
>  	avnet-ultrazedev-cc-v1.0-ultrazedev-som-v1.0.dtb	\
> diff --git a/arch/arm/dts/zynq-mxic-picozed.dts b/arch/arm/dts/zynq-mxic-picozed.dts
> new file mode 100644
> index 0000000..1f24ca1
> --- /dev/null
> +++ b/arch/arm/dts/zynq-mxic-picozed.dts
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 Macronix Inc.
> + *
> + * Author: Zhengxun Li <zhengxunli@mxic.com.tw>
> + */
> +
> +/dts-v1/;
> +/include/ "zynq-7000.dtsi"
> +
> +/ {
> +	model = "Zynq MXIC PicoZed Development Board";
> +	compatible = "mxicy,zynq-mxic-picozed", "xlnx,zynq-7000";
> +
> +	aliases {
> +		ethernet0 = &gem0;
> +		serial0 = &uart1;
> +		spi0 = &spi_controller;
> +	};
> +
> +	memory at 0 {
> +		device_type = "memory";
> +		reg = <0x0 0x30000000>;
> +	};
> +
> +	chosen {
> +		bootargs = "";
> +		stdout-path = "serial0:115200n8";
> +	};
> +};
> +
> +&amba {
> +	clkwizard: clkwizard at 43c20000 {
> +		compatible = "xlnx,clk-wizard-5.1";
> +		reg = <0x43c20000 0x10000>;
> +		clocks = <&clkc 18>, <&clkc 18>;
> +		clock-names = "aclk", "clk_in1";
> +		#clock-cells = <1>;
> +		clock-frequency = <133300000>;
> +	};
> +
> +	spi_controller: spi at 43c30000 {
> +		compatible = "mxicy,mx25f0a-spi";
> +		reg = <0x43c30000 0x10000>;
> +		reg-names = "regs";
> +		clocks = <&clkwizard 0>, <&clkwizard 1>, <&clkc 18>;
> +		clock-names = "send_clk", "send_dly_clk", "ps_clk";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		flash at 0 {
> +			compatible = "jedec,spi-nor";
> +			reg = <0>;
> +			spi-max-frequency = <25000000>;
> +			spi-tx-bus-width = <1>;
> +			spi-rx-bus-width = <1>;
> +		};
> +	};
> +};
> +
> +&clkc {
> +	ps-clk-frequency = <33333333>;
> +};
> +
> +&gem0 {
> +	status = "okay";
> +	phy-mode = "rgmii-id";
> +};
> +
> +&uart1 {
> +	status = "okay";
> +};
> 

As I said this is clear NACK. Description for PL is not acceptable.

Thanks,
Michal

[Patch v2 1/2] clk: zynq: Add clock wizard driver

[Michal Simek]

On 4/29/21 11:31 AM, Zhengxun Li wrote:
> The Clocking Wizard IP supports clock circuits customized
> to your clocking requirements. The wizard support for
> dynamically reconfiguring the clocking primitives for
> Multiply, Divide, Phase Shift/Offset, or Duty Cycle.
> 
> Limited by uboot clk uclass without set_phase API, this
> patch only provides set_rate to modify the frequency.
> 
> Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> ---
>  drivers/clk/Kconfig                 |   9 +++
>  drivers/clk/Makefile                |   1 +
>  drivers/clk/clk-xlnx-clock-wizard.c | 152 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 162 insertions(+)
>  create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 4aeaa0c..f0d4fe0 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -136,6 +136,15 @@ config CLK_ZYNQMP
>  	  This clock driver adds support for clock realted settings for
>  	  ZynqMP platform.
>  
> +config COMMON_CLK_XLNX_CLKWZRD
> +	bool "Xilinx Clocking Wizard"
> +	depends on CLK
> +	help
> +	  Support for the Xilinx Clocking Wizard IP core clock generator.
> +	  Adds support for clocking wizard and compatible.
> +	  This driver supports the Xilinx clocking wizard programmable clock
> +	  synthesizer.
> +
>  config CLK_STM32MP1
>  	bool "Enable RCC clock driver for STM32MP1"
>  	depends on ARCH_STM32MP && CLK
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 645709b..f4ddbe8 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/
>  obj-$(CONFIG_CLK_VEXPRESS_OSC) += clk_vexpress_osc.o
>  obj-$(CONFIG_CLK_ZYNQ) += clk_zynq.o
>  obj-$(CONFIG_CLK_ZYNQMP) += clk_zynqmp.o
> +obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-wizard.o
>  obj-$(CONFIG_ICS8N3QV01) += ics8n3qv01.o
>  obj-$(CONFIG_MACH_PIC32) += clk_pic32.o
>  obj-$(CONFIG_SANDBOX) += clk_sandbox.o
> diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> new file mode 100644
> index 0000000..55adb16
> --- /dev/null
> +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx 'Clocking Wizard' driver
> + *
> + * Copyright (c) 2021 Macronix Inc.
> + *
> + * Author: Zhengxun Li <zhengxunli@mxic.com.tw>
> + */
> +
> +#include <common.h>
> +#include <clk-uclass.h>
> +#include <dm.h>
> +#include <div64.h>
> +#include <linux/iopoll.h>
> +
> +#define SRR			0x0
> +
> +#define SR			0x4
> +#define SR_LOCKED		BIT(0)
> +
> +#define CCR(x)			(0x200 + ((x) * 4))
> +
> +#define FBOUT_CFG		CCR(0)
> +#define FBOUT_DIV(x)		(x)
> +#define FBOUT_GET_DIV(x)	((x) & GENMASK(7, 0))
> +#define FBOUT_MUL(x)		((x) << 8)
> +#define FBOUT_GET_MUL(x)	(((x) & GENMASK(15, 8)) >> 8)
> +#define FBOUT_FRAC(x)		((x) << 16)
> +#define FBOUT_GET_FRAC(x)	(((x) & GENMASK(25, 16)) >> 16)
> +#define FBOUT_FRAC_EN		BIT(26)
> +
> +#define FBOUT_PHASE		CCR(1)
> +
> +#define OUT_CFG(x)		CCR(2 + ((x) * 3))
> +#define OUT_DIV(x)		(x)
> +#define OUT_GET_DIV(x)		((x) & GENMASK(7, 0))
> +#define OUT_FRAC(x)		((x) << 8)
> +#define OUT_GET_FRAC(x)		(((x) & GENMASK(17, 8)) >> 8)
> +#define OUT_FRAC_EN		BIT(18)
> +
> +#define OUT_PHASE(x)		CCR(3 + ((x) * 3))
> +#define OUT_DUTY(x)		CCR(4 + ((x) * 3))
> +
> +#define CTRL			CCR(23)
> +#define CTRL_SEN		BIT(2)
> +#define CTRL_SADDR		BIT(1)
> +#define CTRL_LOAD		BIT(0)
> +
> +/*

/**

> + * struct clkwzrd - Clock wizard private data structure
> + *
> + * @lock		Lock pointer

not listed below

> + * @base		Memory base

missing :

> + * @vco_clk		voltage-controlled oscillator frequency

missing :

> + */
> +struct clkwzd {
> +	void __iomem *base;
> +	u64 vco_clk;
> +};
> +
> +static int clk_wzrd_enable(struct clk *clk)
> +{
> +	struct clkwzd *priv = dev_get_priv(clk->dev);
> +	int ret;
> +	u32 val;
> +
> +	ret = readl_poll_sleep_timeout(priv->base + SR, val, val & SR_LOCKED,
> +				       1, 100);
> +	if (!ret) {
> +		writel(CTRL_SEN | CTRL_SADDR | CTRL_LOAD, priv->base + CTRL);
> +		writel(CTRL_SADDR, priv->base + CTRL);
> +		ret = readl_poll_sleep_timeout(priv->base + SR, val,
> +					       val & SR_LOCKED, 1, 100);
> +	}
> +
> +	return ret;
> +}
> +
> +static unsigned long clk_wzrd_set_rate(struct clk *clk, ulong rate)
> +{
> +	struct clkwzd *priv = dev_get_priv(clk->dev);
> +	u64 div;
> +	u32 cfg;
> +
> +	/* Get output clock divide value */
> +	div = DIV_ROUND_DOWN_ULL(priv->vco_clk * 1000, rate);
> +	if (div < 1000 || div > 255999)
> +		return -EINVAL;
> +
> +	cfg = OUT_DIV((u32)div / 1000);
> +
> +	writel(cfg, priv->base + OUT_CFG(clk->id));
> +
> +	return 0;
> +}
> +
> +static int clk_wzrd_of_to_plat(struct udevice *dev)
> +{
> +	struct clkwzd *priv = dev_get_priv(dev);

It is not like this.

This is of_to_plat. It means you are taking memory from
dev_get_plat(dev); here.


> +	fdt_addr_t addr;
> +	u64 clk_in1, vco_clk;
> +	u32 cfg;
> +
> +	addr = dev_read_addr(dev);
> +	if (addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;

This is fine.

> +
> +	priv->base = (void __iomem *)addr;

But this assignment should be done in probe where you copy data from
plat structures to priv structures.

> +
> +	clk_in1 = dev_read_u32_default(dev, "clock-frequency", 0);

This is not the part of DT binding. You should be able to get that
frequencies via clock framework.

> +
> +	/* Read clock configuration registers */
> +	cfg = readl(priv->base + FBOUT_CFG);
> +
> +	/* Recalculate VCO rate */
> +	if (cfg & FBOUT_FRAC_EN)
> +		vco_clk = DIV_ROUND_DOWN_ULL(clk_in1 *
> +					     ((FBOUT_GET_MUL(cfg) * 1000) +
> +					      FBOUT_GET_FRAC(cfg)),
> +					     1000);
> +	else
> +		vco_clk = clk_in1 * FBOUT_GET_MUL(cfg);
> +
> +	priv->vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));

And all of this should be in probe.

of_to_plat function is here just to read data from DT and save it to
platdata structure. The reason is that you can create that platdata
structures yourself and you can support also !DT case.


> +
> +	return 0;
> +}
> +
> +static int clk_wzrd_probe(struct udevice *dev)
> +{
> +	return 0;
> +}
> +
> +static struct clk_ops clk_wzrd_ops = {
> +	.enable = clk_wzrd_enable,
> +	.set_rate = clk_wzrd_set_rate,
> +};
> +
> +static const struct udevice_id clk_wzrd_ids[] = {
> +	{ .compatible = "xlnx,clk-wizard-5.1" },

xlnx,clocking-wizard
please

> +	{ /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(clk_wzrd) = {
> +	.name = "zynq-clk-wizard",
> +	.id = UCLASS_CLK,
> +	.of_match = clk_wzrd_ids,
> +	.ops = &clk_wzrd_ops,
> +	.probe = clk_wzrd_probe,
> +	.of_to_plat = clk_wzrd_of_to_plat,
> +	.priv_auto = sizeof(struct clkwzd),

you need to get memory for .plat_auto here.

> +};
> 

Thanks,
Michal

[Patch v2 1/2] clk: zynq: Add clock wizard driver

[zhengxunli at mxic.com.tw]

Hi Michal,

Thank you for your review.
 
> On 4/29/21 11:31 AM, Zhengxun Li wrote:
> > The Clocking Wizard IP supports clock circuits customized
> > to your clocking requirements. The wizard support for
> > dynamically reconfiguring the clocking primitives for
> > Multiply, Divide, Phase Shift/Offset, or Duty Cycle.
> > 
> > Limited by uboot clk uclass without set_phase API, this
> > patch only provides set_rate to modify the frequency.
> > 
> > Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> > ---
> >  drivers/clk/Kconfig                 |   9 +++
> >  drivers/clk/Makefile                |   1 +
> >  drivers/clk/clk-xlnx-clock-wizard.c | 152 +++++++++++++++++++++++
> +++++++++++++
> >  3 files changed, 162 insertions(+)
> >  create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c
> > 
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 4aeaa0c..f0d4fe0 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -136,6 +136,15 @@ config CLK_ZYNQMP
> >       This clock driver adds support for clock realted settings for
> >       ZynqMP platform.
> > 
> > +config COMMON_CLK_XLNX_CLKWZRD
> > +   bool "Xilinx Clocking Wizard"
> > +   depends on CLK
> > +   help
> > +     Support for the Xilinx Clocking Wizard IP core clock generator.
> > +     Adds support for clocking wizard and compatible.
> > +     This driver supports the Xilinx clocking wizard programmable 
clock
> > +     synthesizer.
> > +
> >  config CLK_STM32MP1
> >     bool "Enable RCC clock driver for STM32MP1"
> >     depends on ARCH_STM32MP && CLK
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 645709b..f4ddbe8 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/
> >  obj-$(CONFIG_CLK_VEXPRESS_OSC) += clk_vexpress_osc.o
> >  obj-$(CONFIG_CLK_ZYNQ) += clk_zynq.o
> >  obj-$(CONFIG_CLK_ZYNQMP) += clk_zynqmp.o
> > +obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-wizard.o
> >  obj-$(CONFIG_ICS8N3QV01) += ics8n3qv01.o
> >  obj-$(CONFIG_MACH_PIC32) += clk_pic32.o
> >  obj-$(CONFIG_SANDBOX) += clk_sandbox.o
> > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/
> clk-xlnx-clock-wizard.c
> > new file mode 100644
> > index 0000000..55adb16
> > --- /dev/null
> > +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Xilinx 'Clocking Wizard' driver
> > + *
> > + * Copyright (c) 2021 Macronix Inc.
> > + *
> > + * Author: Zhengxun Li <zhengxunli@mxic.com.tw>
> > + */
> > +
> > +#include <common.h>
> > +#include <clk-uclass.h>
> > +#include <dm.h>
> > +#include <div64.h>
> > +#include <linux/iopoll.h>
> > +
> > +#define SRR         0x0
> > +
> > +#define SR         0x4
> > +#define SR_LOCKED      BIT(0)
> > +
> > +#define CCR(x)         (0x200 + ((x) * 4))
> > +
> > +#define FBOUT_CFG      CCR(0)
> > +#define FBOUT_DIV(x)      (x)
> > +#define FBOUT_GET_DIV(x)   ((x) & GENMASK(7, 0))
> > +#define FBOUT_MUL(x)      ((x) << 8)
> > +#define FBOUT_GET_MUL(x)   (((x) & GENMASK(15, 8)) >> 8)
> > +#define FBOUT_FRAC(x)      ((x) << 16)
> > +#define FBOUT_GET_FRAC(x)   (((x) & GENMASK(25, 16)) >> 16)
> > +#define FBOUT_FRAC_EN      BIT(26)
> > +
> > +#define FBOUT_PHASE      CCR(1)
> > +
> > +#define OUT_CFG(x)      CCR(2 + ((x) * 3))
> > +#define OUT_DIV(x)      (x)
> > +#define OUT_GET_DIV(x)      ((x) & GENMASK(7, 0))
> > +#define OUT_FRAC(x)      ((x) << 8)
> > +#define OUT_GET_FRAC(x)      (((x) & GENMASK(17, 8)) >> 8)
> > +#define OUT_FRAC_EN      BIT(18)
> > +
> > +#define OUT_PHASE(x)      CCR(3 + ((x) * 3))
> > +#define OUT_DUTY(x)      CCR(4 + ((x) * 3))
> > +
> > +#define CTRL         CCR(23)
> > +#define CTRL_SEN      BIT(2)
> > +#define CTRL_SADDR      BIT(1)
> > +#define CTRL_LOAD      BIT(0)
> > +
> > +/*
> 
> /**
> 
> > + * struct clkwzrd - Clock wizard private data structure
> > + *
> > + * @lock      Lock pointer
> 
> not listed below
>
> > + * @base      Memory base
>
>
> > + * @vco_clk      voltage-controlled oscillator frequency

Okay.
 
> > + */
> > +struct clkwzd {
> > +   void __iomem *base;
> > +   u64 vco_clk;
> > +};
> > +
> > +static int clk_wzrd_enable(struct clk *clk)
> > +{
> > +   struct clkwzd *priv = dev_get_priv(clk->dev);
> > +   int ret;
> > +   u32 val;
> > +
> > +   ret = readl_poll_sleep_timeout(priv->base + SR, val, val & 
SR_LOCKED,
> > +                   1, 100);
> > +   if (!ret) {
> > +      writel(CTRL_SEN | CTRL_SADDR | CTRL_LOAD, priv->base + CTRL);
> > +      writel(CTRL_SADDR, priv->base + CTRL);
> > +      ret = readl_poll_sleep_timeout(priv->base + SR, val,
> > +                      val & SR_LOCKED, 1, 100);
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static unsigned long clk_wzrd_set_rate(struct clk *clk, ulong rate)
> > +{
> > +   struct clkwzd *priv = dev_get_priv(clk->dev);
> > +   u64 div;
> > +   u32 cfg;
> > +
> > +   /* Get output clock divide value */
> > +   div = DIV_ROUND_DOWN_ULL(priv->vco_clk * 1000, rate);
> > +   if (div < 1000 || div > 255999)
> > +      return -EINVAL;
> > +
> > +   cfg = OUT_DIV((u32)div / 1000);
> > +
> > +   writel(cfg, priv->base + OUT_CFG(clk->id));
> > +
> > +   return 0;
> > +}
> > +
> > +static int clk_wzrd_of_to_plat(struct udevice *dev)
> > +{
> > +   struct clkwzd *priv = dev_get_priv(dev);
> 
> It is not like this.
> 
> This is of_to_plat. It means you are taking memory from
> dev_get_plat(dev); here.

Okay, got it.

> > +   fdt_addr_t addr;
> > +   u64 clk_in1, vco_clk;
> > +   u32 cfg;
> > +
> > +   addr = dev_read_addr(dev);
> > +   if (addr == FDT_ADDR_T_NONE)
> > +      return -EINVAL;
> 
> This is fine.
> 
> > +
> > +   priv->base = (void __iomem *)addr;
> 
> But this assignment should be done in probe where you copy data from
> plat structures to priv structures.

Do you mean priv->base = (void __iomem *)plat->addr?
 
> > +
> > +   clk_in1 = dev_read_u32_default(dev, "clock-frequency", 0);
> 
> This is not the part of DT binding. You should be able to get that
> frequencies via clock framework.

Can you provide some hints about this? I am new to clock driver.

> > +
> > +   /* Read clock configuration registers */
> > +   cfg = readl(priv->base + FBOUT_CFG);
> > +
> > +   /* Recalculate VCO rate */
> > +   if (cfg & FBOUT_FRAC_EN)
> > +      vco_clk = DIV_ROUND_DOWN_ULL(clk_in1 *
> > +                    ((FBOUT_GET_MUL(cfg) * 1000) +
> > +                     FBOUT_GET_FRAC(cfg)),
> > +                    1000);
> > +   else
> > +      vco_clk = clk_in1 * FBOUT_GET_MUL(cfg);
> > +
> > +   priv->vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));
> 
> And all of this should be in probe.
> 
> of_to_plat function is here just to read data from DT and save it to
> platdata structure. The reason is that you can create that platdata
> structures yourself and you can support also !DT case.

Okay, got it.
 
> > +
> > +   return 0;
> > +}
> > +
> > +static int clk_wzrd_probe(struct udevice *dev)
> > +{
> > +   return 0;
> > +}
> > +
> > +static struct clk_ops clk_wzrd_ops = {
> > +   .enable = clk_wzrd_enable,
> > +   .set_rate = clk_wzrd_set_rate,
> > +};
> > +
> > +static const struct udevice_id clk_wzrd_ids[] = {
> > +   { .compatible = "xlnx,clk-wizard-5.1" },
> 
> xlnx,clocking-wizard
> please

Okay.

> > +   { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(clk_wzrd) = {
> > +   .name = "zynq-clk-wizard",
> > +   .id = UCLASS_CLK,
> > +   .of_match = clk_wzrd_ids,
> > +   .ops = &clk_wzrd_ops,
> > +   .probe = clk_wzrd_probe,
> > +   .of_to_plat = clk_wzrd_of_to_plat,
> > +   .priv_auto = sizeof(struct clkwzd),
> 
> you need to get memory for .plat_auto here.

Okay.

On the other hand, if we want to add set_phase feature to clock wizard, 
can you make some suggestions? I checked the clk-uclass and it does not 
seem to be supported.

Thanks,
Zhengxun


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

[Patch v2 1/2] clk: zynq: Add clock wizard driver

[Michal Simek]

Hi,

snip.

>>> +
>>> +   priv->base = (void __iomem *)addr;
>>
>> But this assignment should be done in probe where you copy data from
>> plat structures to priv structures.
> 
> Do you mean priv->base = (void __iomem *)plat->addr?

yes.


>>> +
>>> +   clk_in1 = dev_read_u32_default(dev, "clock-frequency", 0);
>>
>> This is not the part of DT binding. You should be able to get that
>> frequencies via clock framework.
> 
> Can you provide some hints about this? I am new to clock driver.

You should look at drivers/serial/serial_zynq.c where you can find out
clk_get_by_index(), clk_get_rate(), clk_enable()

That clk_get_rate() is the function you should use to get that frequency.

snip.

> 
>>> +   { /* sentinel */ }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(clk_wzrd) = {
>>> +   .name = "zynq-clk-wizard",
>>> +   .id = UCLASS_CLK,
>>> +   .of_match = clk_wzrd_ids,
>>> +   .ops = &clk_wzrd_ops,
>>> +   .probe = clk_wzrd_probe,
>>> +   .of_to_plat = clk_wzrd_of_to_plat,
>>> +   .priv_auto = sizeof(struct clkwzd),
>>
>> you need to get memory for .plat_auto here.
> 
> Okay.
> 
> On the other hand, if we want to add set_phase feature to clock wizard, 
> can you make some suggestions? I checked the clk-uclass and it does not 
> seem to be supported.

Then you have to add it to uclass first but don't know content here.

> 
> Thanks,
> Zhengxun
> 
> 
> CONFIDENTIALITY NOTE:
> 
> This e-mail and any attachments may contain confidential information 
> and/or personal data, which is protected by applicable laws. Please be 
> reminded that duplication, disclosure, distribution, or use of this e-mail 
> (and/or its attachments) or any part thereof is prohibited. If you receive 
> this e-mail in error, please notify us immediately and delete this mail as 
> well as its attachment(s) from your system. In addition, please be 
> informed that collection, processing, and/or use of personal data is 
> prohibited unless expressly permitted by personal data protection laws. 
> Thank you for your attention and cooperation.
> 
> Macronix International Co., Ltd.
> 
> =====================================================================
> 
> 
> 
> ============================================================================
> 
> CONFIDENTIALITY NOTE:
> 
> This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.
> 
> Macronix International Co., Ltd.
> 
> =====================================================================

And get rid of these. Next time I won't reply to this email. Sorry.

Thanks,
Michal