[PATCH 0/2] Add Xilinx clock wizard driver support

[PATCH 0/2] Add Xilinx clock wizard driver support

[zhengxunli]

Hi,

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

Thanks,
Zhengxun

zhengxunli (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 |  66 ++++++++++++++
 drivers/clk/Kconfig                |   7 ++
 drivers/clk/Makefile               |   1 +
 drivers/clk/clk_wizard.c           | 180 +++++++++++++++++++++++++++++++++++++
 5 files changed, 256 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/dts/zynq-mxic-picozed.dts
 create mode 100644 drivers/clk/clk_wizard.c

-- 
1.9.1

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

[zhengxunli]

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 and
set 50% duty cycle by default.

Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>
---
 drivers/clk/Kconfig      |   7 ++
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk_wizard.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 drivers/clk/clk_wizard.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 4aeaa0c..4ebeccc 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -136,6 +136,13 @@ config CLK_ZYNQMP
 	  This clock driver adds support for clock realted settings for
 	  ZynqMP platform.
 
+config CLK_WIZARD
+	bool "Enable clock wizard driver support for zynq"
+	depends on CLK && ARCH_ZYNQ
+	help
+	  This clock driver adds support for clock wizard setting for
+	  Zynq platform.
+
 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..d8b878c 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_CLK_WIZARD) += clk_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_wizard.c b/drivers/clk/clk_wizard.c
new file mode 100644
index 0000000..f5c2387
--- /dev/null
+++ b/drivers/clk/clk_wizard.c
@@ -0,0 +1,180 @@
+// 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)
+
+/*
+ *	   MMCM Block Diagram
+ *
+ *	   +----------------+  +-----------------+
+ * input ->| vco_clk_div_hw |->| vco_clk_mul_hw  |--+
+ * rate    | (int divide)   |  | (frac multiply) |  |
+ *	   +----------------+  +-----------------+  |
+ *						    |
+ *	+--------------------------------VCO-rate---+
+ *	|
+ *	|  +----------------+
+ *	+->| clkout[0]	    |-> output0 rate
+ *	|  | (frac divide)  |
+ *	|  +----------------+
+ *	|
+ *	|  +----------------+
+ *	+->| clkout[1]	    |-> output1 rate
+ *	|  | (int divide)   |
+ *	|  +----------------+
+ *	|
+ *     ...
+ *	|
+ *	|  +----------------+
+ *	+->| clkout[1]	    |-> output6 rate
+ *	   | (int divide)   |
+ *	   +----------------+
+ *
+ * struct clkwzrd - Clock wizard private data structure
+ *
+ * @lock		Lock pointer
+ * @base		Memory base
+ * @vco_clk		voltage-controlled oscillator frequency
+ */
+struct clkwzd {
+	struct mutex lock;
+	void __iomem *base;
+	u64 vco_clk;
+};
+
+static int zynq_clk_wizard_enable(struct clk *clk)
+{
+	struct clkwzd *priv = dev_get_priv(clk->dev);
+	int ret;
+	u32 val;
+
+	mutex_lock(&priv->lock);
+	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);
+	}
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static unsigned long zynq_clk_wizard_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));
+
+	/* Set duty cycle to 50%. */
+	writel(50000, priv->base + OUT_DUTY(clk->id));
+
+	return 0;
+}
+
+static struct clk_ops zynq_clk_wizard_ops = {
+	.enable = zynq_clk_wizard_enable,
+	.set_rate = zynq_clk_wizard_set_rate,
+};
+
+static const struct udevice_id zynq_clk_wizard_ids[] = {
+	{ .compatible = "xlnx,clk-wizard-5.1" },
+	{ /* sentinel */ }
+};
+
+static int zynq_clk_wizard_probe(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 confguration registers */
+	cfg = readl(priv->base + FBOUT_CFG);
+
+	/* Recaculate 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);
+
+	vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));
+
+	priv->vco_clk = vco_clk;
+
+	return 0;
+}
+
+U_BOOT_DRIVER(zynq_clk_wizard) = {
+	.name = "zynq-clk-wizard",
+	.id = UCLASS_CLK,
+	.of_match = zynq_clk_wizard_ids,
+	.ops = &zynq_clk_wizard_ops,
+	.probe = zynq_clk_wizard_probe,
+	.priv_auto = sizeof(struct clkwzd),
+};
-- 
1.9.1

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

[zhengxunli]

Add the Zynq Mxic picozed development board support.

Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>
---
 arch/arm/dts/Makefile              |  3 +-
 arch/arm/dts/zynq-mxic-picozed.dts | 66 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 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 9a8de46..059bb3b 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -286,7 +286,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..d2ff358
--- /dev/null
+++ b/arch/arm/dts/zynq-mxic-picozed.dts
@@ -0,0 +1,66 @@
+/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>;
+		xlnx,clk-wizard-num-outputs = <2>;
+	};
+
+	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 2/2] board: Add Zynq Mxic picozed development board support

[Michal Simek]

Hi,

On 4/7/21 11:05 AM, zhengxunli wrote:
> Add the Zynq Mxic picozed development board support.
> 
> Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>
> ---
>  arch/arm/dts/Makefile              |  3 +-
>  arch/arm/dts/zynq-mxic-picozed.dts | 66 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 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 9a8de46..059bb3b 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -286,7 +286,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..d2ff358
> --- /dev/null
> +++ b/arch/arm/dts/zynq-mxic-picozed.dts
> @@ -0,0 +1,66 @@
> +/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>;
> +		xlnx,clk-wizard-num-outputs = <2>;
> +	};

This is definitely PL IP.

> +
> +	spi_controller: spi at 43c30000 {
> +		compatible = "mxicy,mx25f0a-spi";


And I expect this is also PL based IP.
And we have agreement that for upstream project we won't be accepting
any description for PL.
But there is not a problem with accepting driver for clocking wizard or
this IP.

And picozed board is already supported in u-boot.

Thanks,
Michal

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

[Michal Simek]

On 4/7/21 11:05 AM, zhengxunli 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 and
> set 50% duty cycle by default.
> 
> Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>

Please use full name.

> ---
>  drivers/clk/Kconfig      |   7 ++
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk_wizard.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 drivers/clk/clk_wizard.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 4aeaa0c..4ebeccc 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -136,6 +136,13 @@ config CLK_ZYNQMP
>  	  This clock driver adds support for clock realted settings for
>  	  ZynqMP platform.
>  
> +config CLK_WIZARD

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/clocking-wizard/Kconfig?h=v5.12-rc6

Small alignment with kernel would be useful.
At least CLK_XLNX_CLKWZRD.


> +	bool "Enable clock wizard driver support for zynq"
> +	depends on CLK && ARCH_ZYNQ

Clocking wizard is standard PL based IP not just related to Zynq. It can
be used by Microblaze, ARM cores, etc. It means no need to have
dependency on ZYNQ here.

> +	help
> +	  This clock driver adds support for clock wizard setting for
> +	  Zynq platform.

ditto

> +
>  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..d8b878c 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_CLK_WIZARD) += clk_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_wizard.c b/drivers/clk/clk_wizard.c
> new file mode 100644
> index 0000000..f5c2387
> --- /dev/null
> +++ b/drivers/clk/clk_wizard.c

name could be also aligned with kernel to have easier match with the kernel.

> @@ -0,0 +1,180 @@
> +// 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)
> +
> +/*

/** for kernel-doc as noted below.

> + *	   MMCM Block Diagram
> + *
> + *	   +----------------+  +-----------------+
> + * input ->| vco_clk_div_hw |->| vco_clk_mul_hw  |--+
> + * rate    | (int divide)   |  | (frac multiply) |  |
> + *	   +----------------+  +-----------------+  |
> + *						    |
> + *	+--------------------------------VCO-rate---+
> + *	|
> + *	|  +----------------+
> + *	+->| clkout[0]	    |-> output0 rate
> + *	|  | (frac divide)  |
> + *	|  +----------------+
> + *	|
> + *	|  +----------------+
> + *	+->| clkout[1]	    |-> output1 rate
> + *	|  | (int divide)   |
> + *	|  +----------------+
> + *	|
> + *     ...
> + *	|
> + *	|  +----------------+
> + *	+->| clkout[1]	    |-> output6 rate
> + *	   | (int divide)   |
> + *	   +----------------+
> + *
> + * struct clkwzrd - Clock wizard private data structure
> + *
> + * @lock		Lock pointer
> + * @base		Memory base
> + * @vco_clk		voltage-controlled oscillator frequency

This is not kernel-doc format but it looks like you are sort of using it.
Fix it and then run:
./scripts/kernel-doc -man -v drivers/clk/clk_wizard.c 1>/dev/null


> + */
> +struct clkwzd {
> +	struct mutex lock;


CHECK: struct mutex definition without comment
#144: FILE: drivers/clk/clk_wizard.c:83:
+	struct mutex lock;

> +	void __iomem *base;
> +	u64 vco_clk;
> +};
> +
> +static int zynq_clk_wizard_enable(struct clk *clk)
> +{
> +	struct clkwzd *priv = dev_get_priv(clk->dev);
> +	int ret;
> +	u32 val;
> +
> +	mutex_lock(&priv->lock);
> +	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);
> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;

return ret?

> +}
> +
> +static unsigned long zynq_clk_wizard_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));
> +
> +	/* Set duty cycle to 50%. */

Based on your comment style remove this . here.

> +	writel(50000, priv->base + OUT_DUTY(clk->id));
> +
> +	return 0;
> +}
> +
> +static struct clk_ops zynq_clk_wizard_ops = {
> +	.enable = zynq_clk_wizard_enable,
> +	.set_rate = zynq_clk_wizard_set_rate,
> +};
> +
> +static const struct udevice_id zynq_clk_wizard_ids[] = {
> +	{ .compatible = "xlnx,clk-wizard-5.1" },
> +	{ /* sentinel */ }
> +};

Please move these two structures below probe which is standard location.

> +
> +static int zynq_clk_wizard_probe(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);

All these dt stuff should be done in of_to_plat function.

> +
> +	/* Read clock confguration registers */

typo in comment.

> +	cfg = readl(priv->base + FBOUT_CFG);
> +
> +	/* Recaculate VCO rate */

typo

> +	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);
> +
> +	vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));
> +
> +	priv->vco_clk = vco_clk;


just
priv->vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));


> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(zynq_clk_wizard) = {
> +	.name = "zynq-clk-wizard",
> +	.id = UCLASS_CLK,
> +	.of_match = zynq_clk_wizard_ids,
> +	.ops = &zynq_clk_wizard_ops,
> +	.probe = zynq_clk_wizard_probe,
> +	.priv_auto = sizeof(struct clkwzd),
> +};
> 

Thanks,
Michal

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

[zhengxunli at mxic.com.tw]

Hi Michal, 

Thank you for your quick reply!


"Michal Simek" <michal.simek@xilinx.com> wrote on 2021/04/07 ?? 
08:35:18:

> "Michal Simek" <michal.simek@xilinx.com> 
> 2021/04/07 ?? 08:35
> 
> To
> 
> "zhengxunli" <zhengxunli@mxic.com.tw>, <u-boot@lists.denx.de>, 
> 
> cc
> 
> <michal.simek@xilinx.com>, <lukma@denx.de>, <miquel.raynal@bootlin.com>
> 
> Subject
> 
> Re: [PATCH 2/2] board: Add Zynq Mxic picozed development board support
> 
> Hi,
> 
> On 4/7/21 11:05 AM, zhengxunli wrote:
> > Add the Zynq Mxic picozed development board support.
> > 
> > Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>
> > ---
> >  arch/arm/dts/Makefile              |  3 +-
> >  arch/arm/dts/zynq-mxic-picozed.dts | 66 +++++++++++++++++++++++++
> +++++++++++++
> >  2 files changed, 68 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 9a8de46..059bb3b 100644
> > --- a/arch/arm/dts/Makefile
> > +++ b/arch/arm/dts/Makefile
> > @@ -286,7 +286,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..d2ff358
> > --- /dev/null
> > +++ b/arch/arm/dts/zynq-mxic-picozed.dts
> > @@ -0,0 +1,66 @@
> > +/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>;
> > +      xlnx,clk-wizard-num-outputs = <2>;
> > +   };
> 
> This is definitely PL IP.

What is the PL?

> > +
> > +   spi_controller: spi at 43c30000 {
> > +      compatible = "mxicy,mx25f0a-spi";
> 
> 
> And I expect this is also PL based IP.
> And we have agreement that for upstream project we won't be accepting
> any description for PL.
> But there is not a problem with accepting driver for clocking wizard or
> this IP.
 
It sounds like we can move on.

> And picozed board is already supported in u-boot.

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.

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



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

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 1/2] clk: zynq: Add clock wizard driver

[zhengxunli at mxic.com.tw]

Hi,

> On 4/7/21 11:05 AM, zhengxunli 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 and
> > set 50% duty cycle by default.
> > 
> > Signed-off-by: zhengxunli <zhengxunli@mxic.com.tw>
> 
> Please use full name.

Okay, got it.

> 
> > ---
> >  drivers/clk/Kconfig      |   7 ++
> >  drivers/clk/Makefile     |   1 +
> >  drivers/clk/clk_wizard.c | 180 ++++++++++++++++++++++++++++++++++
> +++++++++++++
> >  3 files changed, 188 insertions(+)
> >  create mode 100644 drivers/clk/clk_wizard.c
> > 
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 4aeaa0c..4ebeccc 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -136,6 +136,13 @@ config CLK_ZYNQMP
> >       This clock driver adds support for clock realted settings for
> >       ZynqMP platform.
> > 
> > +config CLK_WIZARD
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/drivers/staging/clocking-wizard/Kconfig?h=v5.12-rc6
> 
> Small alignment with kernel would be useful.
> At least CLK_XLNX_CLKWZRD.
>
> > +   bool "Enable clock wizard driver support for zynq"
> > +   depends on CLK && ARCH_ZYNQ
> 
> Clocking wizard is standard PL based IP not just related to Zynq. It can
> be used by Microblaze, ARM cores, etc. It means no need to have
> dependency on ZYNQ here.
> 
> > +   help
> > +     This clock driver adds support for clock wizard setting for
> > +     Zynq platform.
> 
> ditto
>

Okay, I will fix in the next version.
 
> > +
> >  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..d8b878c 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_CLK_WIZARD) += clk_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_wizard.c b/drivers/clk/clk_wizard.c
> > new file mode 100644
> > index 0000000..f5c2387
> > --- /dev/null
> > +++ b/drivers/clk/clk_wizard.c
> 
> name could be also aligned with kernel to have easier match with the 
kernel.

Okay, got it.

> 
> > @@ -0,0 +1,180 @@
> > +// 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)
> > +
> > +/*
> 
> /** for kernel-doc as noted below.
> 
> > + *      MMCM Block Diagram
> > + *
> > + *      +----------------+  +-----------------+
> > + * input ->| vco_clk_div_hw |->| vco_clk_mul_hw  |--+
> > + * rate    | (int divide)   |  | (frac multiply) |  |
> > + *      +----------------+  +-----------------+  |
> > + *                      |
> > + *   +--------------------------------VCO-rate---+
> > + *   |
> > + *   |  +----------------+
> > + *   +->| clkout[0]       |-> output0 rate
> > + *   |  | (frac divide)  |
> > + *   |  +----------------+
> > + *   |
> > + *   |  +----------------+
> > + *   +->| clkout[1]       |-> output1 rate
> > + *   |  | (int divide)   |
> > + *   |  +----------------+
> > + *   |
> > + *     ...
> > + *   |
> > + *   |  +----------------+
> > + *   +->| clkout[1]       |-> output6 rate
> > + *      | (int divide)   |
> > + *      +----------------+
> > + *
> > + * struct clkwzrd - Clock wizard private data structure
> > + *
> > + * @lock      Lock pointer
> > + * @base      Memory base
> > + * @vco_clk      voltage-controlled oscillator frequency
> 
> This is not kernel-doc format but it looks like you are sort of using 
it.
> Fix it and then run:
> ./scripts/kernel-doc -man -v drivers/clk/clk_wizard.c 1>/dev/null

Okay, got it.

> 
> > + */
> > +struct clkwzd {
> > +   struct mutex lock;
> 
> 
> CHECK: struct mutex definition without comment
> #144: FILE: drivers/clk/clk_wizard.c:83:
> +   struct mutex lock;

Okay, got it.

> 
> > +   void __iomem *base;
> > +   u64 vco_clk;
> > +};
> > +
> > +static int zynq_clk_wizard_enable(struct clk *clk)
> > +{
> > +   struct clkwzd *priv = dev_get_priv(clk->dev);
> > +   int ret;
> > +   u32 val;
> > +
> > +   mutex_lock(&priv->lock);
> > +   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);
> > +   }
> > +   mutex_unlock(&priv->lock);
> > +
> > +   return 0;
> 
> return ret?

Okay, I will fix it.

> 
> > +}
> > +
> > +static unsigned long zynq_clk_wizard_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));
> > +
> > +   /* Set duty cycle to 50%. */
> 
> Based on your comment style remove this . here.

Okay, I will remove this in next version.

> 
> > +   writel(50000, priv->base + OUT_DUTY(clk->id));
> > +
> > +   return 0;
> > +}
> > +
> > +static struct clk_ops zynq_clk_wizard_ops = {
> > +   .enable = zynq_clk_wizard_enable,
> > +   .set_rate = zynq_clk_wizard_set_rate,
> > +};
> > +
> > +static const struct udevice_id zynq_clk_wizard_ids[] = {
> > +   { .compatible = "xlnx,clk-wizard-5.1" },
> > +   { /* sentinel */ }
> > +};
> 
> Please move these two structures below probe which is standard location.

Okay, got it.

> > +
> > +static int zynq_clk_wizard_probe(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);
> 
> All these dt stuff should be done in of_to_plat function.

Okay, got it.

> > +
> > +   /* Read clock confguration registers */
> 
> typo in comment.
 
Okay, will fix it.

> > +   cfg = readl(priv->base + FBOUT_CFG);
> > +
> > +   /* Recaculate VCO rate */
> 
> typo

Okay, will fix it.
 
> > +   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);
> > +
> > +   vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));
> > +
> > +   priv->vco_clk = vco_clk;
> 
> 
> just
> priv->vco_clk = DIV_ROUND_DOWN_ULL(vco_clk, FBOUT_GET_DIV(cfg));

Okay.

> 
> > +
> > +   return 0;
> > +}
> > +
> > +U_BOOT_DRIVER(zynq_clk_wizard) = {
> > +   .name = "zynq-clk-wizard",
> > +   .id = UCLASS_CLK,
> > +   .of_match = zynq_clk_wizard_ids,
> > +   .ops = &zynq_clk_wizard_ops,
> > +   .probe = zynq_clk_wizard_probe,
> > +   .priv_auto = sizeof(struct clkwzd),
> > +};
> > 

Hope you?re having a great day! 

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.

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