Re: [PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

Re: [PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

[zhengxunli]

Hi Shubhrajyoti,

My name is Zhengxun and I am the engineer from Macronix. We are
using the platform PicoZed 7015/7030 SOM (System On Module),
which is based on Xilinx Zynq®-7000 All Programmable (AP) SoC to
verify our Flash dirver. Of course, we are also using your clock
wizard, our version seems to be v5.2, but something went wrong.

+static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long 
rate,
+ unsigned long parent_rate)
+{
+                int err;
+                u32 value;
+                unsigned long flags = 0;
+                struct clk_wzrd_divider *divider = 
to_clk_wzrd_divider(hw);
+                void __iomem *div_addr = divider->base + divider->offset;
+
+                if (divider->lock)
+                                spin_lock_irqsave(divider->lock, flags);
+                else
+                                __acquire(divider->lock);
+
+                value = DIV_ROUND_CLOSEST(parent_rate, rate);
+
+                /* Cap the value to max */
+                min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
+
+                /* Set divisor and clear phase offset */
+                writel(value, div_addr);
+                writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);

Why phase always set to zero? We want to support DTR operation in
Flash driver. Can you add a set_phase function to adjust the phase?

+                /* Check status register */
+                err = readl_poll_timeout(divider->base + 
WZRD_DR_STATUS_REG_OFFSET,
+                                                                 value, 
value & WZRD_DR_LOCK_BIT_MASK,
+ WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
+                if (err)
+                                goto err_reconfig;
+
+                /* Initiate reconfiguration */
+                writel(WZRD_DR_BEGIN_DYNA_RECONF,
+                       divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+                /* Check status register */
+                err = readl_poll_timeout(divider->base + 
WZRD_DR_STATUS_REG_OFFSET,
+                                                                 value, 
value & WZRD_DR_LOCK_BIT_MASK,
+ WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);

According to pg015-clk-wiz.pdf, Clocking Wizard v5.2 and later, need to 
write
0x00000007 followed by 0x00000002 into Clock Configuration Register 23, to
consolidate the redundant bits that the IP has upgraded, right?

Can you compatible to v5.2?

+err_reconfig:
+                if (divider->lock)
+                                spin_unlock_irqrestore(divider->lock, 
flags);
+                else
+                                __release(divider->lock);
+                return err;
+}

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.

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

Re: [PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

[Greg KH]

On Mon, Mar 08, 2021 at 02:59:47PM +0800, zhengxunli@mxic.com.tw wrote:
> 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.

This footer is not compatible with Linux kernel development, sorry.
Message is now deleted.

greg k-h

Re: [PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

[Zhengxun Li]

Hi Shubhrajyoti,

My name is Zhengxun and I am the engineer from Macronix. We are
using the platform PicoZed 7015/7030 SOM (System On Module),
which is based on Xilinx Zynq®-7000 All Programmable (AP) SoC to
verify our Flash driver. Of course, we are also using your clock
wizard, our version seems to be v5.2, but something went wrong.

+static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ int err;
+ u32 value;
+ unsigned long flags = 0;
+ struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+ void __iomem *div_addr = divider->base + divider->offset;
+
+ if (divider->lock)
+ spin_lock_irqsave(divider->lock, flags);
+ else
+ __acquire(divider->lock);
+
+ value = DIV_ROUND_CLOSEST(parent_rate, rate);
+
+ /* Cap the value to max */
+ min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
+
+ /* Set divisor and clear phase offset */
+ writel(value, div_addr);
+ writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);

Why phase always set to zero? We want to support DTR operation in
Flash driver. Can you add a set_phase function to adjust the phase?

+ /* Check status register */
+ err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET,
+ value, value & WZRD_DR_LOCK_BIT_MASK,
+ WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
+ if (err)
+ goto err_reconfig;
+
+ /* Initiate reconfiguration */
+ writel(WZRD_DR_BEGIN_DYNA_RECONF,
+ divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+ /* Check status register */
+ err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET,
+ value, value & WZRD_DR_LOCK_BIT_MASK,
+ WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);

According to pg015-clk-wiz.pdf, Clocking Wizard v5.2 and later, need to write
0x00000007 followed by 0x00000002 into Clock Configuration Register 23, to
consolidate the redundant bits that the IP has upgraded, right?

Can you compatible to v5.2?

Thanks,
Zhengxun

Re: [PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

[Shubhrajyoti Datta]

Hi Zhengxun,
Thanks for the review.

On Mon, Mar 15, 2021 at 12:37 PM Zhengxun Li <zhengxunli.mxic@gmail.com> wrote:
>
> Hi Shubhrajyoti,
>
> My name is Zhengxun and I am the engineer from Macronix. We are
> using the platform PicoZed 7015/7030 SOM (System On Module),
> which is based on Xilinx Zynq®-7000 All Programmable (AP) SoC to
> verify our Flash driver. Of course, we are also using your clock
> wizard, our version seems to be v5.2, but something went wrong.
>
> +static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + int err;
> + u32 value;
> + unsigned long flags = 0;
> + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> + void __iomem *div_addr = divider->base + divider->offset;
> +
> + if (divider->lock)
> + spin_lock_irqsave(divider->lock, flags);
> + else
> + __acquire(divider->lock);
> +
> + value = DIV_ROUND_CLOSEST(parent_rate, rate);
> +
> + /* Cap the value to max */
> + min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
> +
> + /* Set divisor and clear phase offset */
> + writel(value, div_addr);
> + writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
>
> Why phase always set to zero? We want to support DTR operation in
> Flash driver. Can you add a set_phase function to adjust the phase?
>
> + /* Check status register */
> + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET,
> + value, value & WZRD_DR_LOCK_BIT_MASK,
> + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
> + if (err)
> + goto err_reconfig;
> +
> + /* Initiate reconfiguration */
> + writel(WZRD_DR_BEGIN_DYNA_RECONF,
> + divider->base + WZRD_DR_INIT_REG_OFFSET);
> +
> + /* Check status register */
> + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET,
> + value, value & WZRD_DR_LOCK_BIT_MASK,
> + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
>
> According to pg015-clk-wiz.pdf, Clocking Wizard v5.2 and later, need to write
> 0x00000007 followed by 0x00000002 into Clock Configuration Register 23, to
> consolidate the redundant bits that the IP has upgraded, right?
>
> Can you compatible to v5.2?

Will fix in the next version.

>
> Thanks,
> Zhengxun

Re: [PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

[Shubhrajyoti Datta]

On Mon, Mar 15, 2021 at 12:37 PM Zhengxun Li <zhengxunli.mxic@gmail.com> wrote:
>
> Hi Shubhrajyoti,
>
> My name is Zhengxun and I am the engineer from Macronix. We are
> using the platform PicoZed 7015/7030 SOM (System On Module),
> which is based on Xilinx Zynq®-7000 All Programmable (AP) SoC to
> verify our Flash driver. Of course, we are also using your clock
> wizard, our version seems to be v5.2, but something went wrong.
>
> +static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + int err;
> + u32 value;
> + unsigned long flags = 0;
> + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> + void __iomem *div_addr = divider->base + divider->offset;
> +
> + if (divider->lock)
> + spin_lock_irqsave(divider->lock, flags);
> + else
> + __acquire(divider->lock);
> +
> + value = DIV_ROUND_CLOSEST(parent_rate, rate);
> +
> + /* Cap the value to max */
> + min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
> +
> + /* Set divisor and clear phase offset */
> + writel(value, div_addr);
> + writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
>
> Why phase always set to zero? We want to support DTR operation in
> Flash driver. Can you add a set_phase function to adjust the phase?
>
The phase setting is a separate feature will address in a separate series.

>

[PATCH v10 0/9] clk: clocking-wizard: driver updates

[Shubhrajyoti Datta]

In the thread [1] Greg suggested that we move the driver
to the clk from the staging.
Add patches to address the concerns regarding the fractional and
set rate support in the TODO.

The patch set does the following
- Trivial fixes for kernel doc.
- Move the driver to the clk folder
- Add capability to set rate.
- Add fractional support.
- Add support for configurable outputs.
- Make the output names unique so that multiple instances
do not crib.

Changes in the v3:
Added the cover-letter.
Add patches for rate setting and fractional support
Add patches for warning.
Remove the driver from staging as suggested

v4:
Reorder the patches.
Merge the CLK_IS_BASIC patch.
Add the yaml form of binding document

v5:
Fix a mismerge

v6:
Fix the yaml warning
use poll timedout

v7:
Binding doc updates
Use common divisor function.

v8:
Fix Robs comments

v9:
Fix device tree warnings

v10:
Reorder the patches
Update the speed grade description.

[1] https://spinics.net/lists/linux-driver-devel/msg117326.html

Shubhrajyoti Datta (9):
  staging: clocking-wizard: Fix kernel-doc warning
  staging: clocking-wizard: Rename speed-grade to xlnx,speed-grade
  staging: clocking-wizard: Update the fixed factor divisors
  staging: clocking-wizard: Allow changing of parent rate for single
    output
  staging: clocking-wizard: Add support for dynamic reconfiguration
  staging: clocking-wizard: Add support for fractional support
  staging: clocking-wizard: Remove the hardcoding of the clock outputs
  dt-bindings: add documentation of xilinx clocking wizard
  clk: clock-wizard: Add the clockwizard to clk directory

 .../bindings/clock/xlnx,clocking-wizard.yaml       |  72 +++
 drivers/clk/Kconfig                                |   9 +
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-xlnx-clock-wizard.c                | 636 +++++++++++++++++++++
 drivers/staging/Kconfig                            |   2 -
 drivers/staging/Makefile                           |   1 -
 drivers/staging/clocking-wizard/Kconfig            |  10 -
 drivers/staging/clocking-wizard/Makefile           |   2 -
 drivers/staging/clocking-wizard/TODO               |  12 -
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 333 -----------
 drivers/staging/clocking-wizard/dt-binding.txt     |  30 -
 11 files changed, 718 insertions(+), 390 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
 create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c
 delete mode 100644 drivers/staging/clocking-wizard/Kconfig
 delete mode 100644 drivers/staging/clocking-wizard/Makefile
 delete mode 100644 drivers/staging/clocking-wizard/TODO
 delete mode 100644 drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
 delete mode 100644 drivers/staging/clocking-wizard/dt-binding.txt

-- 
2.1.1

[PATCH v10 5/9] staging: clocking-wizard: Add support for dynamic reconfiguration

[Shubhrajyoti Datta]

The patch adds support for dynamic reconfiguration of clock output rate.
Output clocks are registered as dividers and set rate callback function
is used for dynamic reconfiguration.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Co-developed-by: Chirag Parekh <chirag.parekh@xilinx.com>
---
v6:
Remove the typecast.
use min for capping frequency.
use polled timeout

v7:
Use devm functions
Add the spinlock

v10:
Add a codeveloped by tag.

 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 178 ++++++++++++++++++++-
 1 file changed, 173 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index e08fc2f..b1bfdb86 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -15,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/iopoll.h>
 
 #define WZRD_NUM_OUTPUTS	7
 #define WZRD_ACLK_MAX_FREQ	250000000UL
@@ -29,8 +30,24 @@
 #define WZRD_DIVCLK_DIVIDE_SHIFT	0
 #define WZRD_DIVCLK_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
 #define WZRD_CLKOUT_DIVIDE_SHIFT	0
+#define WZRD_CLKOUT_DIVIDE_WIDTH	8
 #define WZRD_CLKOUT_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
 
+#define WZRD_DR_MAX_INT_DIV_VALUE	255
+#define WZRD_DR_STATUS_REG_OFFSET	0x04
+#define WZRD_DR_LOCK_BIT_MASK		0x00000001
+#define WZRD_DR_INIT_REG_OFFSET		0x25C
+#define WZRD_DR_DIV_TO_PHASE_OFFSET	4
+#define WZRD_DR_BEGIN_DYNA_RECONF	0x03
+
+#define WZRD_USEC_POLL		10
+#define WZRD_TIMEOUT_POLL		1000
+/* Get the mask from width */
+#define div_mask(width)			((1 << (width)) - 1)
+
+/* Extract divider instance from clock hardware instance */
+#define to_clk_wzrd_divider(_hw) container_of(_hw, struct clk_wzrd_divider, hw)
+
 enum clk_wzrd_int_clks {
 	wzrd_clk_mul,
 	wzrd_clk_mul_div,
@@ -62,6 +79,29 @@ struct clk_wzrd {
 	bool suspended;
 };
 
+/**
+ * struct clk_wzrd_divider - clock divider specific to clk_wzrd
+ *
+ * @hw:		handle between common and hardware-specific interfaces
+ * @base:	base address of register containing the divider
+ * @offset:	offset address of register containing the divider
+ * @shift:	shift to the divider bit field
+ * @width:	width of the divider bit field
+ * @flags:	clk_wzrd divider flags
+ * @table:	array of value/divider pairs, last entry should have div = 0
+ * @lock:	register lock
+ */
+struct clk_wzrd_divider {
+	struct clk_hw hw;
+	void __iomem *base;
+	u16 offset;
+	u8 shift;
+	u8 width;
+	u8 flags;
+	const struct clk_div_table *table;
+	spinlock_t *lock;  /* divider lock */
+};
+
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
 
 /* maximum frequencies for input/output clocks per speed grade */
@@ -71,6 +111,131 @@ static const unsigned long clk_wzrd_max_freq[] = {
 	1066000000UL
 };
 
+/* spin lock variable for clk_wzrd */
+static DEFINE_SPINLOCK(clkwzrd_lock);
+
+static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+	void __iomem *div_addr = divider->base + divider->offset;
+	unsigned int val;
+
+	val = readl(div_addr) >> divider->shift;
+	val &= div_mask(divider->width);
+
+	return divider_recalc_rate(hw, parent_rate, val, divider->table,
+			divider->flags, divider->width);
+}
+
+static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
+				     unsigned long parent_rate)
+{
+	int err;
+	u32 value;
+	unsigned long flags = 0;
+	struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+	void __iomem *div_addr = divider->base + divider->offset;
+
+	if (divider->lock)
+		spin_lock_irqsave(divider->lock, flags);
+	else
+		__acquire(divider->lock);
+
+	value = DIV_ROUND_CLOSEST(parent_rate, rate);
+
+	/* Cap the value to max */
+	min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
+
+	/* Set divisor and clear phase offset */
+	writel(value, div_addr);
+	writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
+
+	/* Check status register */
+	err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET,
+				 value, value & WZRD_DR_LOCK_BIT_MASK,
+				 WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
+	if (err)
+		goto err_reconfig;
+
+	/* Initiate reconfiguration */
+	writel(WZRD_DR_BEGIN_DYNA_RECONF,
+	       divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+	/* Check status register */
+	err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET,
+				 value, value & WZRD_DR_LOCK_BIT_MASK,
+				 WZRD_USEC_POLL, WZRD_TIMEOUT_POLL);
+err_reconfig:
+	if (divider->lock)
+		spin_unlock_irqrestore(divider->lock, flags);
+	else
+		__release(divider->lock);
+	return err;
+}
+
+static long clk_wzrd_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *prate)
+{
+	u8 div;
+
+	/*
+	 * since we don't change parent rate we just round rate to closest
+	 * achievable
+	 */
+	div = DIV_ROUND_CLOSEST(*prate, rate);
+
+	return *prate / div;
+}
+
+static const struct clk_ops clk_wzrd_clk_divider_ops = {
+	.round_rate = clk_wzrd_round_rate,
+	.set_rate = clk_wzrd_dynamic_reconfig,
+	.recalc_rate = clk_wzrd_recalc_rate,
+};
+
+static struct clk *clk_wzrd_register_divider(struct device *dev,
+					     const char *name,
+					     const char *parent_name,
+					     unsigned long flags,
+					     void __iomem *base, u16 offset,
+					     u8 shift, u8 width,
+					     u8 clk_divider_flags,
+					     const struct clk_div_table *table,
+					     spinlock_t *lock)
+{
+	struct clk_wzrd_divider *div;
+	struct clk_hw *hw;
+	struct clk_init_data init;
+	int ret;
+
+	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_wzrd_clk_divider_ops;
+	init.flags = flags;
+	init.parent_names =  &parent_name;
+	init.num_parents =  1;
+
+	div->base = base;
+	div->offset = offset;
+	div->shift = shift;
+	div->width = width;
+	div->flags = clk_divider_flags;
+	div->lock = lock;
+	div->hw.init = &init;
+	div->table = table;
+
+	hw = &div->hw;
+	ret = devm_clk_hw_register(dev, hw);
+	if (ret)
+		hw = ERR_PTR(ret);
+
+	return hw->clk;
+}
+
 static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
 				 void *data)
 {
@@ -251,11 +416,14 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 			ret = -EINVAL;
 			goto err_rm_int_clks;
 		}
-		reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2) + i * 12);
-		reg &= WZRD_CLKOUT_DIVIDE_MASK;
-		reg >>= WZRD_CLKOUT_DIVIDE_SHIFT;
-		clk_wzrd->clkout[i] = clk_register_fixed_factor
-			(&pdev->dev, clkout_name, clk_name, 0, 1, reg);
+		clk_wzrd->clkout[i] = clk_wzrd_register_divider(&pdev->dev,
+								clkout_name,
+				clk_name, 0,
+				clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),
+				WZRD_CLKOUT_DIVIDE_SHIFT,
+				WZRD_CLKOUT_DIVIDE_WIDTH,
+				CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
+				NULL, &clkwzrd_lock);
 		if (IS_ERR(clk_wzrd->clkout[i])) {
 			int j;
 
-- 
2.1.1