2019-11-28 06:38:12

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 00/10] clk: clk-wizard: clock-wizard: Driver updates

From: Shubhrajyoti Datta <[email protected]>

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

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

Shubhrajyoti Datta (10):
dt-bindings: add documentation of xilinx clocking wizard
clk: clock-wizard: Move the clockwizard to clk
clk: clock-wizard: Fix kernel-doc warning
clk: clock-wizard: Add support for dynamic reconfiguration
clk: clock-wizard: Add support for fractional support
clk: clock-wizard: Remove the hardcoding of the clock outputs
clk: clock-wizard: Update the fixed factor divisors
clk: clock-wizard: Make the output names unique
staging: clocking-wizard: Delete the driver from the staging
clk: clock-wizard: Fix the compilation failure

.../bindings/clock/xlnx,clocking-wizard.txt | 32 +
drivers/clk/Kconfig | 6 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-xlnx-clock-wizard.c | 710 +++++++++++++++++++++
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 | 335 ----------
drivers/staging/clocking-wizard/dt-binding.txt | 30 -
11 files changed, 749 insertions(+), 392 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt
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


2019-11-28 06:38:17

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 01/10] dt-bindings: add documentation of xilinx clocking wizard

From: Shubhrajyoti Datta <[email protected]>

Add the devicetree binding for the xilinx clocking wizard.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
.../bindings/clock/xlnx,clocking-wizard.txt | 32 ++++++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt

diff --git a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt
new file mode 100644
index 0000000..aedac84
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt
@@ -0,0 +1,32 @@
+Binding for Xilinx Clocking Wizard IP Core
+
+This binding uses the common clock binding[1]. Details about the devices can be
+found in the product guide[2].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Clocking Wizard Product Guide
+http://www.xilinx.com/support/documentation/ip_documentation/clk_wiz/v5_1/pg065-clk-wiz.pdf
+
+Required properties:
+ - compatible: Must be 'xlnx,clocking-wizard'
+ - #clock-cells: Number of cells in a clock specifier. Should be 1
+ - reg: Base and size of the cores register space
+ - clocks: Handle to input clock
+ - clock-names: Tuple containing 'clk_in1' and 's_axi_aclk'
+ - clock-output-names: Names for the output clocks
+
+Optional properties:
+ - speed-grade: Speed grade of the device (valid values are 1..3)
+
+Example:
+ clock-generator@40040000 {
+ #clock-cells = <1>;
+ reg = <0x40040000 0x1000>;
+ compatible = "xlnx,clocking-wizard";
+ speed-grade = <1>;
+ clock-names = "clk_in1", "s_axi_aclk";
+ clocks = <&clkc 15>, <&clkc 15>;
+ clock-output-names = "clk_out0", "clk_out1", "clk_out2",
+ "clk_out3", "clk_out4", "clk_out5",
+ "clk_out6", "clk_out7";
+ };
--
2.1.1

2019-11-28 06:38:22

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 02/10] clk: clock-wizard: Move the clockwizard to clk

From: Shubhrajyoti Datta <[email protected]>

Move the clocking wizard driver from staging to clk.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/Kconfig | 6 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-xlnx-clock-wizard.c | 335 ++++++++++++++++++++++++++++++++++++
3 files changed, 342 insertions(+)
create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c44247d..ff7d0f6 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -305,6 +305,12 @@ config COMMON_CLK_FIXED_MMIO
help
Support for Memory Mapped IO Fixed clocks

+config COMMON_CLK_XLNX_CLKWZRD
+ tristate "Xilinx Clocking Wizard"
+ depends on COMMON_CLK && OF
+ help
+ Support for the Xilinx Clocking Wizard IP core clock generator.
+
source "drivers/clk/actions/Kconfig"
source "drivers/clk/analogbits/Kconfig"
source "drivers/clk/bcm/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 0138fb1..bce022f 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
obj-$(CONFIG_COMMON_CLK_VC5) += clk-versaclock5.o
obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
obj-$(CONFIG_COMMON_CLK_XGENE) += clk-xgene.o
+obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-wizard.o

# please keep this section sorted lexicographically by directory path name
obj-y += actions/
diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
new file mode 100644
index 0000000..15b7a82
--- /dev/null
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx 'Clocking Wizard' driver
+ *
+ * Copyright (C) 2013 - 2014 Xilinx
+ *
+ * Sören Brinkmann <[email protected]>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/err.h>
+
+#define WZRD_NUM_OUTPUTS 7
+#define WZRD_ACLK_MAX_FREQ 250000000UL
+
+#define WZRD_CLK_CFG_REG(n) (0x200 + 4 * (n))
+
+#define WZRD_CLKOUT0_FRAC_EN BIT(18)
+#define WZRD_CLKFBOUT_FRAC_EN BIT(26)
+
+#define WZRD_CLKFBOUT_MULT_SHIFT 8
+#define WZRD_CLKFBOUT_MULT_MASK (0xff << WZRD_CLKFBOUT_MULT_SHIFT)
+#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_MASK (0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
+
+enum clk_wzrd_int_clks {
+ wzrd_clk_mul,
+ wzrd_clk_mul_div,
+ wzrd_clk_int_max
+};
+
+/**
+ * struct clk_wzrd:
+ * @clk_data: Clock data
+ * @nb: Notifier block
+ * @base: Memory base
+ * @clk_in1: Handle to input clock 'clk_in1'
+ * @axi_clk: Handle to input clock 's_axi_aclk'
+ * @clks_internal: Internal clocks
+ * @clkout: Output clocks
+ * @speed_grade: Speed grade of the device
+ * @suspended: Flag indicating power state of the device
+ */
+struct clk_wzrd {
+ struct clk_onecell_data clk_data;
+ struct notifier_block nb;
+ void __iomem *base;
+ struct clk *clk_in1;
+ struct clk *axi_clk;
+ struct clk *clks_internal[wzrd_clk_int_max];
+ struct clk *clkout[WZRD_NUM_OUTPUTS];
+ unsigned int speed_grade;
+ bool suspended;
+};
+
+#define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
+
+/* maximum frequencies for input/output clocks per speed grade */
+static const unsigned long clk_wzrd_max_freq[] = {
+ 800000000UL,
+ 933000000UL,
+ 1066000000UL
+};
+
+static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
+ void *data)
+{
+ unsigned long max;
+ struct clk_notifier_data *ndata = data;
+ struct clk_wzrd *clk_wzrd = to_clk_wzrd(nb);
+
+ if (clk_wzrd->suspended)
+ return NOTIFY_OK;
+
+ if (ndata->clk == clk_wzrd->clk_in1)
+ max = clk_wzrd_max_freq[clk_wzrd->speed_grade - 1];
+ else if (ndata->clk == clk_wzrd->axi_clk)
+ max = WZRD_ACLK_MAX_FREQ;
+ else
+ return NOTIFY_DONE; /* should never happen */
+
+ switch (event) {
+ case PRE_RATE_CHANGE:
+ if (ndata->new_rate > max)
+ return NOTIFY_BAD;
+ return NOTIFY_OK;
+ case POST_RATE_CHANGE:
+ case ABORT_RATE_CHANGE:
+ default:
+ return NOTIFY_DONE;
+ }
+}
+
+static int __maybe_unused clk_wzrd_suspend(struct device *dev)
+{
+ struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(clk_wzrd->axi_clk);
+ clk_wzrd->suspended = true;
+
+ return 0;
+}
+
+static int __maybe_unused clk_wzrd_resume(struct device *dev)
+{
+ int ret;
+ struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
+
+ ret = clk_prepare_enable(clk_wzrd->axi_clk);
+ if (ret) {
+ dev_err(dev, "unable to enable s_axi_aclk\n");
+ return ret;
+ }
+
+ clk_wzrd->suspended = false;
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
+ clk_wzrd_resume);
+
+static int clk_wzrd_probe(struct platform_device *pdev)
+{
+ int i, ret;
+ u32 reg;
+ unsigned long rate;
+ const char *clk_name;
+ struct clk_wzrd *clk_wzrd;
+ struct resource *mem;
+ struct device_node *np = pdev->dev.of_node;
+
+ clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
+ if (!clk_wzrd)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, clk_wzrd);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ clk_wzrd->base = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(clk_wzrd->base))
+ return PTR_ERR(clk_wzrd->base);
+
+ ret = of_property_read_u32(np, "speed-grade", &clk_wzrd->speed_grade);
+ if (!ret) {
+ if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
+ dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
+ clk_wzrd->speed_grade);
+ clk_wzrd->speed_grade = 0;
+ }
+ }
+
+ clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
+ if (IS_ERR(clk_wzrd->clk_in1)) {
+ if (clk_wzrd->clk_in1 != ERR_PTR(-EPROBE_DEFER))
+ dev_err(&pdev->dev, "clk_in1 not found\n");
+ return PTR_ERR(clk_wzrd->clk_in1);
+ }
+
+ clk_wzrd->axi_clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
+ if (IS_ERR(clk_wzrd->axi_clk)) {
+ if (clk_wzrd->axi_clk != ERR_PTR(-EPROBE_DEFER))
+ dev_err(&pdev->dev, "s_axi_aclk not found\n");
+ return PTR_ERR(clk_wzrd->axi_clk);
+ }
+ ret = clk_prepare_enable(clk_wzrd->axi_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "enabling s_axi_aclk failed\n");
+ return ret;
+ }
+ rate = clk_get_rate(clk_wzrd->axi_clk);
+ if (rate > WZRD_ACLK_MAX_FREQ) {
+ dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n",
+ rate);
+ ret = -EINVAL;
+ goto err_disable_clk;
+ }
+
+ /* we don't support fractional div/mul yet */
+ reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
+ WZRD_CLKFBOUT_FRAC_EN;
+ reg |= readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2)) &
+ WZRD_CLKOUT0_FRAC_EN;
+ if (reg)
+ dev_warn(&pdev->dev, "fractional div/mul not supported\n");
+
+ /* register multiplier */
+ reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
+ WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
+ clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
+ if (!clk_name) {
+ ret = -ENOMEM;
+ goto err_disable_clk;
+ }
+ clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
+ (&pdev->dev, clk_name,
+ __clk_get_name(clk_wzrd->clk_in1),
+ 0, reg, 1);
+ kfree(clk_name);
+ if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
+ dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
+ ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
+ goto err_disable_clk;
+ }
+
+ /* register div */
+ reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
+ WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
+ clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
+ if (!clk_name) {
+ ret = -ENOMEM;
+ goto err_rm_int_clk;
+ }
+
+ clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor
+ (&pdev->dev, clk_name,
+ __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
+ 0, 1, reg);
+ if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
+ dev_err(&pdev->dev, "unable to register divider clock\n");
+ ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
+ goto err_rm_int_clk;
+ }
+
+ /* register div per output */
+ for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
+ const char *clkout_name;
+
+ if (of_property_read_string_index(np, "clock-output-names", i,
+ &clkout_name)) {
+ dev_err(&pdev->dev,
+ "clock output name not specified\n");
+ 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);
+ if (IS_ERR(clk_wzrd->clkout[i])) {
+ int j;
+
+ for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
+ clk_unregister(clk_wzrd->clkout[j]);
+ dev_err(&pdev->dev,
+ "unable to register divider clock\n");
+ ret = PTR_ERR(clk_wzrd->clkout[i]);
+ goto err_rm_int_clks;
+ }
+ }
+
+ kfree(clk_name);
+
+ clk_wzrd->clk_data.clks = clk_wzrd->clkout;
+ clk_wzrd->clk_data.clk_num = ARRAY_SIZE(clk_wzrd->clkout);
+ of_clk_add_provider(np, of_clk_src_onecell_get, &clk_wzrd->clk_data);
+
+ if (clk_wzrd->speed_grade) {
+ clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
+
+ ret = clk_notifier_register(clk_wzrd->clk_in1,
+ &clk_wzrd->nb);
+ if (ret)
+ dev_warn(&pdev->dev,
+ "unable to register clock notifier\n");
+
+ ret = clk_notifier_register(clk_wzrd->axi_clk, &clk_wzrd->nb);
+ if (ret)
+ dev_warn(&pdev->dev,
+ "unable to register clock notifier\n");
+ }
+
+ return 0;
+
+err_rm_int_clks:
+ clk_unregister(clk_wzrd->clks_internal[1]);
+err_rm_int_clk:
+ kfree(clk_name);
+ clk_unregister(clk_wzrd->clks_internal[0]);
+err_disable_clk:
+ clk_disable_unprepare(clk_wzrd->axi_clk);
+
+ return ret;
+}
+
+static int clk_wzrd_remove(struct platform_device *pdev)
+{
+ int i;
+ struct clk_wzrd *clk_wzrd = platform_get_drvdata(pdev);
+
+ of_clk_del_provider(pdev->dev.of_node);
+
+ for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
+ clk_unregister(clk_wzrd->clkout[i]);
+ for (i = 0; i < wzrd_clk_int_max; i++)
+ clk_unregister(clk_wzrd->clks_internal[i]);
+
+ if (clk_wzrd->speed_grade) {
+ clk_notifier_unregister(clk_wzrd->axi_clk, &clk_wzrd->nb);
+ clk_notifier_unregister(clk_wzrd->clk_in1, &clk_wzrd->nb);
+ }
+
+ clk_disable_unprepare(clk_wzrd->axi_clk);
+
+ return 0;
+}
+
+static const struct of_device_id clk_wzrd_ids[] = {
+ { .compatible = "xlnx,clocking-wizard" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, clk_wzrd_ids);
+
+static struct platform_driver clk_wzrd_driver = {
+ .driver = {
+ .name = "clk-wizard",
+ .of_match_table = clk_wzrd_ids,
+ .pm = &clk_wzrd_dev_pm_ops,
+ },
+ .probe = clk_wzrd_probe,
+ .remove = clk_wzrd_remove,
+};
+module_platform_driver(clk_wzrd_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Soeren Brinkmann <[email protected]");
+MODULE_DESCRIPTION("Driver for the Xilinx Clocking Wizard IP core");
--
2.1.1

2019-11-28 06:38:32

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 03/10] clk: clock-wizard: Fix kernel-doc warning

From: Shubhrajyoti Datta <[email protected]>

Update description for the clocking wizard structure

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index 15b7a82..ef9125d 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -38,7 +38,8 @@ enum clk_wzrd_int_clks {
};

/**
- * struct clk_wzrd:
+ * struct clk_wzrd - Clock wizard private data structure
+ *
* @clk_data: Clock data
* @nb: Notifier block
* @base: Memory base
--
2.1.1

2019-11-28 06:38:35

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 04/10] clk: clock-wizard: Add support for dynamic reconfiguration

From: Shubhrajyoti Datta <[email protected]>

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.

Based on the initial work from Chirag.

Signed-off-by: Chirag Parekh <[email protected]>
Signed-off-by: Shubhrajyoti Datta <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 209 +++++++++++++++++++++++++++++++++++-
1 file changed, 204 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index ef9125d..870e7fb 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -29,8 +29,23 @@
#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_NUM_RETRIES 10000
+#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
+
+/* 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 +77,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 +109,164 @@ 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 =
+ (void __iomem *)((u64)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 = 0;
+ u16 retries;
+ u32 value;
+ unsigned long flags = 0;
+ struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+ void __iomem *div_addr =
+ (void __iomem *)((u64)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 */
+ if (value > WZRD_DR_MAX_INT_DIV_VALUE)
+ 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 */
+ retries = WZRD_DR_NUM_RETRIES;
+ while (retries--) {
+ if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
+ WZRD_DR_LOCK_BIT_MASK)
+ break;
+ }
+
+ if (retries == 0) {
+ err = -ETIMEDOUT;
+ goto err_reconfig;
+ }
+
+ /* Initiate reconfiguration */
+ writel(WZRD_DR_BEGIN_DYNA_RECONF,
+ divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+ /* Check status register */
+ retries = WZRD_DR_NUM_RETRIES;
+ while (retries--) {
+ if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
+ WZRD_DR_LOCK_BIT_MASK)
+ break;
+ }
+
+ if (retries == 0)
+ err = -ETIMEDOUT;
+
+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 donot 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;
+
+ if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {
+ if (width + shift > 16) {
+ pr_warn("divider value exceeds LOWORD field\n");
+ return ERR_PTR(-EINVAL);
+ }
+ }
+
+ /* allocate the divider */
+ div = kzalloc(sizeof(*div), GFP_KERNEL);
+ if (!div)
+ return ERR_PTR(-ENOMEM);
+
+ init.name = name;
+ if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
+ init.ops = &clk_divider_ro_ops;
+ else
+ init.ops = &clk_wzrd_clk_divider_ops;
+ init.flags = flags | CLK_IS_BASIC;
+ init.parent_names = (parent_name ? &parent_name : NULL);
+ init.num_parents = (parent_name ? 1 : 0);
+
+ /* struct clk_divider assignments */
+ 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;
+
+ /* register the clock */
+ hw = &div->hw;
+ ret = clk_hw_register(dev, hw);
+ if (ret) {
+ kfree(div);
+ hw = ERR_PTR(ret);
+ }
+
+ return hw->clk;
+}
+
static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
void *data)
{
@@ -241,11 +437,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

2019-11-28 06:38:57

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 05/10] clk: clock-wizard: Add support for fractional support

From: Shubhrajyoti Datta <[email protected]>

Currently the set rate granularity is to integral divisors.
Add support for the fractional divisors.
Only the first output0 is fractional in the hardware.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 192 +++++++++++++++++++++++++++++++++---
1 file changed, 179 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index 870e7fb..bc0354a 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -26,11 +26,15 @@

#define WZRD_CLKFBOUT_MULT_SHIFT 8
#define WZRD_CLKFBOUT_MULT_MASK (0xff << WZRD_CLKFBOUT_MULT_SHIFT)
+#define WZRD_CLKFBOUT_FRAC_SHIFT 16
+#define WZRD_CLKFBOUT_FRAC_MASK (0x3ff << WZRD_CLKFBOUT_FRAC_SHIFT)
#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_CLKOUT_FRAC_SHIFT 8
+#define WZRD_CLKOUT_FRAC_MASK 0x3ff

#define WZRD_DR_MAX_INT_DIV_VALUE 255
#define WZRD_DR_NUM_RETRIES 10000
@@ -49,6 +53,7 @@
enum clk_wzrd_int_clks {
wzrd_clk_mul,
wzrd_clk_mul_div,
+ wzrd_clk_mul_frac,
wzrd_clk_int_max
};

@@ -124,7 +129,7 @@ static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
val &= div_mask(divider->width);

return divider_recalc_rate(hw, parent_rate, val, divider->table,
- divider->flags, divider->width);
+ divider->flags);
}

static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
@@ -210,6 +215,161 @@ static const struct clk_ops clk_wzrd_clk_divider_ops = {
.recalc_rate = clk_wzrd_recalc_rate,
};

+static unsigned long clk_wzrd_recalc_ratef(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ unsigned int val;
+ u32 div, frac;
+ struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+ void __iomem *div_addr =
+ (void __iomem *)((u64)divider->base + divider->offset);
+
+ val = readl(div_addr);
+ div = val & div_mask(divider->width);
+ frac = (val >> WZRD_CLKOUT_FRAC_SHIFT) & WZRD_CLKOUT_FRAC_MASK;
+
+ return ((parent_rate * 1000) / ((div * 1000) + frac));
+}
+
+static int clk_wzrd_dynamic_reconfig_f(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ int err = 0;
+ u16 retries;
+ u32 value, pre;
+ unsigned long flags = 0;
+ unsigned long rate_div, f, clockout0_div;
+ struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
+ void __iomem *div_addr =
+ (void __iomem *)((u64)divider->base + divider->offset);
+
+ if (divider->lock)
+ spin_lock_irqsave(divider->lock, flags);
+ else
+ __acquire(divider->lock);
+
+ rate_div = ((parent_rate * 1000) / rate);
+ clockout0_div = rate_div / 1000;
+
+ pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
+ f = (u32)(pre - (clockout0_div * 1000));
+ f = f & WZRD_CLKOUT_FRAC_MASK;
+
+ value = ((f << WZRD_CLKOUT_DIVIDE_WIDTH) | (clockout0_div &
+ WZRD_CLKOUT_DIVIDE_MASK));
+
+ /* Set divisor and clear phase offset */
+ writel(value, div_addr);
+ writel(0x0, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
+
+ /* Check status register */
+ retries = WZRD_DR_NUM_RETRIES;
+ while (retries--) {
+ if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
+ WZRD_DR_LOCK_BIT_MASK)
+ break;
+ }
+
+ if (!retries) {
+ err = -ETIMEDOUT;
+ goto err_reconfig;
+ }
+
+ /* Initiate reconfiguration */
+ writel(WZRD_DR_BEGIN_DYNA_RECONF,
+ divider->base + WZRD_DR_INIT_REG_OFFSET);
+
+ /* Check status register */
+ retries = WZRD_DR_NUM_RETRIES;
+ while (retries--) {
+ if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
+ WZRD_DR_LOCK_BIT_MASK)
+ break;
+ }
+
+ if (!retries)
+ err = -ETIMEDOUT;
+
+err_reconfig:
+ if (divider->lock)
+ spin_unlock_irqrestore(divider->lock, flags);
+ else
+ __release(divider->lock);
+
+ return err;
+}
+
+static long clk_wzrd_round_rate_f(struct clk_hw *hw, unsigned long rate,
+ unsigned long *prate)
+{
+ return rate;
+}
+
+static const struct clk_ops clk_wzrd_clk_divider_ops_f = {
+ .round_rate = clk_wzrd_round_rate_f,
+ .set_rate = clk_wzrd_dynamic_reconfig_f,
+ .recalc_rate = clk_wzrd_recalc_ratef,
+};
+
+static struct clk *clk_wzrd_register_divf(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;
+
+ if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {
+ if (width + shift > 16) {
+ pr_warn("divider value exceeds LOWORD field\n");
+ return ERR_PTR(-EINVAL);
+ }
+ }
+
+ /* allocate the divider */
+ div = kzalloc(sizeof(*div), GFP_KERNEL);
+ if (!div)
+ return ERR_PTR(-ENOMEM);
+
+ init.name = name;
+
+ if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
+ init.ops = &clk_divider_ro_ops;
+ else
+ init.ops = &clk_wzrd_clk_divider_ops_f;
+
+ init.flags = flags | CLK_IS_BASIC;
+ init.parent_names = (parent_name ? &parent_name : NULL);
+ init.num_parents = (parent_name ? 1 : 0);
+
+ /* struct clk_divider assignments */
+ 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;
+
+ /* register the clock */
+ hw = &div->hw;
+ ret = clk_hw_register(dev, hw);
+ if (ret) {
+ kfree(div);
+ return ERR_PTR(ret);
+ }
+
+ return hw->clk;
+}
+
static struct clk *clk_wzrd_register_divider(struct device *dev,
const char *name,
const char *parent_name,
@@ -328,7 +488,7 @@ static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
static int clk_wzrd_probe(struct platform_device *pdev)
{
int i, ret;
- u32 reg;
+ u32 reg, reg_f, mult;
unsigned long rate;
const char *clk_name;
struct clk_wzrd *clk_wzrd;
@@ -380,17 +540,13 @@ static int clk_wzrd_probe(struct platform_device *pdev)
goto err_disable_clk;
}

- /* we don't support fractional div/mul yet */
- reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
- WZRD_CLKFBOUT_FRAC_EN;
- reg |= readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2)) &
- WZRD_CLKOUT0_FRAC_EN;
- if (reg)
- dev_warn(&pdev->dev, "fractional div/mul not supported\n");
-
/* register multiplier */
reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
+ reg_f = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
+ WZRD_CLKFBOUT_FRAC_MASK) >> WZRD_CLKFBOUT_FRAC_SHIFT;
+
+ mult = ((reg * 1000) + reg_f);
clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
if (!clk_name) {
ret = -ENOMEM;
@@ -399,7 +555,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
(&pdev->dev, clk_name,
__clk_get_name(clk_wzrd->clk_in1),
- 0, reg, 1);
+ 0, mult, 1000);
kfree(clk_name);
if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
@@ -437,8 +593,18 @@ static int clk_wzrd_probe(struct platform_device *pdev)
ret = -EINVAL;
goto err_rm_int_clks;
}
- clk_wzrd->clkout[i] = clk_wzrd_register_divider(&pdev->dev,
- clkout_name,
+ if (!i)
+ clk_wzrd->clkout[i] = clk_wzrd_register_divf
+ (&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);
+ else
+ 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,
--
2.1.1

2019-11-28 06:39:06

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 06/10] clk: clock-wizard: Remove the hardcoding of the clock outputs

From: Shubhrajyoti Datta <[email protected]>

The number of output clocks are configurable in the hardware.
Currently the driver registers the maximum number of outputs.
Fix the same by registering only the outputs that are there.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index bc0354a..4c6155b 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -493,6 +493,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
const char *clk_name;
struct clk_wzrd *clk_wzrd;
struct resource *mem;
+ int outputs;
struct device_node *np = pdev->dev.of_node;

clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
@@ -583,7 +584,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
}

/* register div per output */
- for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
+ for (i = outputs - 1; i >= 0 ; i--) {
const char *clkout_name;

if (of_property_read_string_index(np, "clock-output-names", i,
@@ -614,7 +615,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
if (IS_ERR(clk_wzrd->clkout[i])) {
int j;

- for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
+ for (j = i + 1; j < outputs; j++)
clk_unregister(clk_wzrd->clkout[j]);
dev_err(&pdev->dev,
"unable to register divider clock\n");
--
2.1.1

2019-11-28 06:39:20

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 07/10] clk: clock-wizard: Update the fixed factor divisors

From: Shubhrajyoti Datta <[email protected]>

Update the fixed factor clock registration to register the divisors.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index 4c6155b..75ea745 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -491,9 +491,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
u32 reg, reg_f, mult;
unsigned long rate;
const char *clk_name;
+ void __iomem *ctrl_reg;
struct clk_wzrd *clk_wzrd;
struct resource *mem;
int outputs;
+ unsigned long flags = 0;
struct device_node *np = pdev->dev.of_node;

clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
@@ -564,19 +566,22 @@ static int clk_wzrd_probe(struct platform_device *pdev)
goto err_disable_clk;
}

- /* register div */
- reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
- WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
+ outputs = of_property_count_strings(np, "clock-output-names");
+ if (outputs == 1)
+ flags = CLK_SET_RATE_PARENT;
clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
if (!clk_name) {
ret = -ENOMEM;
goto err_rm_int_clk;
}

- clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor
+ ctrl_reg = clk_wzrd->base + WZRD_CLK_CFG_REG(0);
+ /* register div */
+ clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_divider
(&pdev->dev, clk_name,
__clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
- 0, 1, reg);
+ flags, ctrl_reg, 0, 8, CLK_DIVIDER_ONE_BASED |
+ CLK_DIVIDER_ALLOW_ZERO, &clkwzrd_lock);
if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
dev_err(&pdev->dev, "unable to register divider clock\n");
ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
@@ -597,7 +602,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
if (!i)
clk_wzrd->clkout[i] = clk_wzrd_register_divf
(&pdev->dev, clkout_name,
- clk_name, 0,
+ clk_name, flags,
clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),
WZRD_CLKOUT_DIVIDE_SHIFT,
WZRD_CLKOUT_DIVIDE_WIDTH,
--
2.1.1

2019-11-28 06:41:31

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 08/10] clk: clock-wizard: Make the output names unique

From: Shubhrajyoti Datta <[email protected]>

Incase there are more than one instance of the clocking wizard.
And if the output name given is the same then the probe fails.
Fix the same by appending the device name to the output name to
make it unique.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index 75ea745..9993543 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto err_disable_clk;
}
+ outputs = of_property_count_strings(np, "clock-output-names");
+ if (outputs == 1)
+ flags = CLK_SET_RATE_PARENT;
clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
(&pdev->dev, clk_name,
__clk_get_name(clk_wzrd->clk_in1),
@@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
goto err_disable_clk;
}

- outputs = of_property_count_strings(np, "clock-output-names");
- if (outputs == 1)
- flags = CLK_SET_RATE_PARENT;
clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
if (!clk_name) {
ret = -ENOMEM;
@@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
/* register div per output */
for (i = outputs - 1; i >= 0 ; i--) {
const char *clkout_name;
+ const char *clkout_name_wiz;

if (of_property_read_string_index(np, "clock-output-names", i,
&clkout_name)) {
@@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
ret = -EINVAL;
goto err_rm_int_clks;
}
+ clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s",
+ dev_name(&pdev->dev), clkout_name);
if (!i)
clk_wzrd->clkout[i] = clk_wzrd_register_divf
- (&pdev->dev, clkout_name,
+ (&pdev->dev, clkout_name_wiz,
clk_name, flags,
clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),
WZRD_CLKOUT_DIVIDE_SHIFT,
@@ -610,7 +613,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
NULL, &clkwzrd_lock);
else
clk_wzrd->clkout[i] = clk_wzrd_register_divider
- (&pdev->dev, clkout_name,
+ (&pdev->dev, clkout_name_wiz,
clk_name, 0,
clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12),
WZRD_CLKOUT_DIVIDE_SHIFT,
--
2.1.1

2019-11-28 06:41:45

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 09/10] staging: clocking-wizard: Delete the driver from the staging

From: Shubhrajyoti Datta <[email protected]>

Delete the driver from the staging as it is in drivers/clk.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
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 | 335 ---------------------
drivers/staging/clocking-wizard/dt-binding.txt | 30 --
7 files changed, 392 deletions(-)
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

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 6f1fa4c..29e7ab7 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -78,8 +78,6 @@ source "drivers/staging/gs_fpgaboot/Kconfig"

source "drivers/staging/unisys/Kconfig"

-source "drivers/staging/clocking-wizard/Kconfig"
-
source "drivers/staging/fbtft/Kconfig"

source "drivers/staging/fsl-dpaa2/Kconfig"
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index a90f9b3..7de50e3 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -29,7 +29,6 @@ obj-$(CONFIG_FIREWIRE_SERIAL) += fwserial/
obj-$(CONFIG_GOLDFISH) += goldfish/
obj-$(CONFIG_GS_FPGABOOT) += gs_fpgaboot/
obj-$(CONFIG_UNISYSSPAR) += unisys/
-obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clocking-wizard/
obj-$(CONFIG_FB_TFT) += fbtft/
obj-$(CONFIG_FSL_DPAA2) += fsl-dpaa2/
obj-$(CONFIG_WILC1000) += wilc1000/
diff --git a/drivers/staging/clocking-wizard/Kconfig b/drivers/staging/clocking-wizard/Kconfig
deleted file mode 100644
index 04be22d..0000000
--- a/drivers/staging/clocking-wizard/Kconfig
+++ /dev/null
@@ -1,10 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0
-#
-# Xilinx Clocking Wizard Driver
-#
-
-config COMMON_CLK_XLNX_CLKWZRD
- tristate "Xilinx Clocking Wizard"
- depends on COMMON_CLK && OF
- help
- Support for the Xilinx Clocking Wizard IP core clock generator.
diff --git a/drivers/staging/clocking-wizard/Makefile b/drivers/staging/clocking-wizard/Makefile
deleted file mode 100644
index b1f9152..0000000
--- a/drivers/staging/clocking-wizard/Makefile
+++ /dev/null
@@ -1,2 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-wizard.o
diff --git a/drivers/staging/clocking-wizard/TODO b/drivers/staging/clocking-wizard/TODO
deleted file mode 100644
index ebe99db..0000000
--- a/drivers/staging/clocking-wizard/TODO
+++ /dev/null
@@ -1,12 +0,0 @@
-TODO:
- - support for fractional multiplier
- - support for fractional divider (output 0 only)
- - support for set_rate() operations (may benefit from Stephen Boyd's
- refactoring of the clk primitives: https://lkml.org/lkml/2014/9/5/766)
- - review arithmetic
- - overflow after multiplication?
- - maximize accuracy before divisions
-
-Patches to:
- Greg Kroah-Hartman <[email protected]>
- Sören Brinkmann <[email protected]>
diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
deleted file mode 100644
index 15b7a82..0000000
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ /dev/null
@@ -1,335 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Xilinx 'Clocking Wizard' driver
- *
- * Copyright (C) 2013 - 2014 Xilinx
- *
- * Sören Brinkmann <[email protected]>
- */
-
-#include <linux/platform_device.h>
-#include <linux/clk.h>
-#include <linux/clk-provider.h>
-#include <linux/slab.h>
-#include <linux/io.h>
-#include <linux/of.h>
-#include <linux/module.h>
-#include <linux/err.h>
-
-#define WZRD_NUM_OUTPUTS 7
-#define WZRD_ACLK_MAX_FREQ 250000000UL
-
-#define WZRD_CLK_CFG_REG(n) (0x200 + 4 * (n))
-
-#define WZRD_CLKOUT0_FRAC_EN BIT(18)
-#define WZRD_CLKFBOUT_FRAC_EN BIT(26)
-
-#define WZRD_CLKFBOUT_MULT_SHIFT 8
-#define WZRD_CLKFBOUT_MULT_MASK (0xff << WZRD_CLKFBOUT_MULT_SHIFT)
-#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_MASK (0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
-
-enum clk_wzrd_int_clks {
- wzrd_clk_mul,
- wzrd_clk_mul_div,
- wzrd_clk_int_max
-};
-
-/**
- * struct clk_wzrd:
- * @clk_data: Clock data
- * @nb: Notifier block
- * @base: Memory base
- * @clk_in1: Handle to input clock 'clk_in1'
- * @axi_clk: Handle to input clock 's_axi_aclk'
- * @clks_internal: Internal clocks
- * @clkout: Output clocks
- * @speed_grade: Speed grade of the device
- * @suspended: Flag indicating power state of the device
- */
-struct clk_wzrd {
- struct clk_onecell_data clk_data;
- struct notifier_block nb;
- void __iomem *base;
- struct clk *clk_in1;
- struct clk *axi_clk;
- struct clk *clks_internal[wzrd_clk_int_max];
- struct clk *clkout[WZRD_NUM_OUTPUTS];
- unsigned int speed_grade;
- bool suspended;
-};
-
-#define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
-
-/* maximum frequencies for input/output clocks per speed grade */
-static const unsigned long clk_wzrd_max_freq[] = {
- 800000000UL,
- 933000000UL,
- 1066000000UL
-};
-
-static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
- void *data)
-{
- unsigned long max;
- struct clk_notifier_data *ndata = data;
- struct clk_wzrd *clk_wzrd = to_clk_wzrd(nb);
-
- if (clk_wzrd->suspended)
- return NOTIFY_OK;
-
- if (ndata->clk == clk_wzrd->clk_in1)
- max = clk_wzrd_max_freq[clk_wzrd->speed_grade - 1];
- else if (ndata->clk == clk_wzrd->axi_clk)
- max = WZRD_ACLK_MAX_FREQ;
- else
- return NOTIFY_DONE; /* should never happen */
-
- switch (event) {
- case PRE_RATE_CHANGE:
- if (ndata->new_rate > max)
- return NOTIFY_BAD;
- return NOTIFY_OK;
- case POST_RATE_CHANGE:
- case ABORT_RATE_CHANGE:
- default:
- return NOTIFY_DONE;
- }
-}
-
-static int __maybe_unused clk_wzrd_suspend(struct device *dev)
-{
- struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
-
- clk_disable_unprepare(clk_wzrd->axi_clk);
- clk_wzrd->suspended = true;
-
- return 0;
-}
-
-static int __maybe_unused clk_wzrd_resume(struct device *dev)
-{
- int ret;
- struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
-
- ret = clk_prepare_enable(clk_wzrd->axi_clk);
- if (ret) {
- dev_err(dev, "unable to enable s_axi_aclk\n");
- return ret;
- }
-
- clk_wzrd->suspended = false;
-
- return 0;
-}
-
-static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
- clk_wzrd_resume);
-
-static int clk_wzrd_probe(struct platform_device *pdev)
-{
- int i, ret;
- u32 reg;
- unsigned long rate;
- const char *clk_name;
- struct clk_wzrd *clk_wzrd;
- struct resource *mem;
- struct device_node *np = pdev->dev.of_node;
-
- clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
- if (!clk_wzrd)
- return -ENOMEM;
- platform_set_drvdata(pdev, clk_wzrd);
-
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- clk_wzrd->base = devm_ioremap_resource(&pdev->dev, mem);
- if (IS_ERR(clk_wzrd->base))
- return PTR_ERR(clk_wzrd->base);
-
- ret = of_property_read_u32(np, "speed-grade", &clk_wzrd->speed_grade);
- if (!ret) {
- if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
- dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
- clk_wzrd->speed_grade);
- clk_wzrd->speed_grade = 0;
- }
- }
-
- clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
- if (IS_ERR(clk_wzrd->clk_in1)) {
- if (clk_wzrd->clk_in1 != ERR_PTR(-EPROBE_DEFER))
- dev_err(&pdev->dev, "clk_in1 not found\n");
- return PTR_ERR(clk_wzrd->clk_in1);
- }
-
- clk_wzrd->axi_clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
- if (IS_ERR(clk_wzrd->axi_clk)) {
- if (clk_wzrd->axi_clk != ERR_PTR(-EPROBE_DEFER))
- dev_err(&pdev->dev, "s_axi_aclk not found\n");
- return PTR_ERR(clk_wzrd->axi_clk);
- }
- ret = clk_prepare_enable(clk_wzrd->axi_clk);
- if (ret) {
- dev_err(&pdev->dev, "enabling s_axi_aclk failed\n");
- return ret;
- }
- rate = clk_get_rate(clk_wzrd->axi_clk);
- if (rate > WZRD_ACLK_MAX_FREQ) {
- dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n",
- rate);
- ret = -EINVAL;
- goto err_disable_clk;
- }
-
- /* we don't support fractional div/mul yet */
- reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
- WZRD_CLKFBOUT_FRAC_EN;
- reg |= readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2)) &
- WZRD_CLKOUT0_FRAC_EN;
- if (reg)
- dev_warn(&pdev->dev, "fractional div/mul not supported\n");
-
- /* register multiplier */
- reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
- WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
- clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
- if (!clk_name) {
- ret = -ENOMEM;
- goto err_disable_clk;
- }
- clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
- (&pdev->dev, clk_name,
- __clk_get_name(clk_wzrd->clk_in1),
- 0, reg, 1);
- kfree(clk_name);
- if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
- dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
- ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
- goto err_disable_clk;
- }
-
- /* register div */
- reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
- WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
- clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
- if (!clk_name) {
- ret = -ENOMEM;
- goto err_rm_int_clk;
- }
-
- clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor
- (&pdev->dev, clk_name,
- __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
- 0, 1, reg);
- if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
- dev_err(&pdev->dev, "unable to register divider clock\n");
- ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
- goto err_rm_int_clk;
- }
-
- /* register div per output */
- for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
- const char *clkout_name;
-
- if (of_property_read_string_index(np, "clock-output-names", i,
- &clkout_name)) {
- dev_err(&pdev->dev,
- "clock output name not specified\n");
- 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);
- if (IS_ERR(clk_wzrd->clkout[i])) {
- int j;
-
- for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
- clk_unregister(clk_wzrd->clkout[j]);
- dev_err(&pdev->dev,
- "unable to register divider clock\n");
- ret = PTR_ERR(clk_wzrd->clkout[i]);
- goto err_rm_int_clks;
- }
- }
-
- kfree(clk_name);
-
- clk_wzrd->clk_data.clks = clk_wzrd->clkout;
- clk_wzrd->clk_data.clk_num = ARRAY_SIZE(clk_wzrd->clkout);
- of_clk_add_provider(np, of_clk_src_onecell_get, &clk_wzrd->clk_data);
-
- if (clk_wzrd->speed_grade) {
- clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
-
- ret = clk_notifier_register(clk_wzrd->clk_in1,
- &clk_wzrd->nb);
- if (ret)
- dev_warn(&pdev->dev,
- "unable to register clock notifier\n");
-
- ret = clk_notifier_register(clk_wzrd->axi_clk, &clk_wzrd->nb);
- if (ret)
- dev_warn(&pdev->dev,
- "unable to register clock notifier\n");
- }
-
- return 0;
-
-err_rm_int_clks:
- clk_unregister(clk_wzrd->clks_internal[1]);
-err_rm_int_clk:
- kfree(clk_name);
- clk_unregister(clk_wzrd->clks_internal[0]);
-err_disable_clk:
- clk_disable_unprepare(clk_wzrd->axi_clk);
-
- return ret;
-}
-
-static int clk_wzrd_remove(struct platform_device *pdev)
-{
- int i;
- struct clk_wzrd *clk_wzrd = platform_get_drvdata(pdev);
-
- of_clk_del_provider(pdev->dev.of_node);
-
- for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
- clk_unregister(clk_wzrd->clkout[i]);
- for (i = 0; i < wzrd_clk_int_max; i++)
- clk_unregister(clk_wzrd->clks_internal[i]);
-
- if (clk_wzrd->speed_grade) {
- clk_notifier_unregister(clk_wzrd->axi_clk, &clk_wzrd->nb);
- clk_notifier_unregister(clk_wzrd->clk_in1, &clk_wzrd->nb);
- }
-
- clk_disable_unprepare(clk_wzrd->axi_clk);
-
- return 0;
-}
-
-static const struct of_device_id clk_wzrd_ids[] = {
- { .compatible = "xlnx,clocking-wizard" },
- { },
-};
-MODULE_DEVICE_TABLE(of, clk_wzrd_ids);
-
-static struct platform_driver clk_wzrd_driver = {
- .driver = {
- .name = "clk-wizard",
- .of_match_table = clk_wzrd_ids,
- .pm = &clk_wzrd_dev_pm_ops,
- },
- .probe = clk_wzrd_probe,
- .remove = clk_wzrd_remove,
-};
-module_platform_driver(clk_wzrd_driver);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Soeren Brinkmann <[email protected]");
-MODULE_DESCRIPTION("Driver for the Xilinx Clocking Wizard IP core");
diff --git a/drivers/staging/clocking-wizard/dt-binding.txt b/drivers/staging/clocking-wizard/dt-binding.txt
deleted file mode 100644
index 723271e..0000000
--- a/drivers/staging/clocking-wizard/dt-binding.txt
+++ /dev/null
@@ -1,30 +0,0 @@
-Binding for Xilinx Clocking Wizard IP Core
-
-This binding uses the common clock binding[1]. Details about the devices can be
-found in the product guide[2].
-
-[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
-[2] Clocking Wizard Product Guide
-http://www.xilinx.com/support/documentation/ip_documentation/clk_wiz/v5_1/pg065-clk-wiz.pdf
-
-Required properties:
- - compatible: Must be 'xlnx,clocking-wizard'
- - reg: Base and size of the cores register space
- - clocks: Handle to input clock
- - clock-names: Tuple containing 'clk_in1' and 's_axi_aclk'
- - clock-output-names: Names for the output clocks
-
-Optional properties:
- - speed-grade: Speed grade of the device (valid values are 1..3)
-
-Example:
- clock-generator@40040000 {
- reg = <0x40040000 0x1000>;
- compatible = "xlnx,clocking-wizard";
- speed-grade = <1>;
- clock-names = "clk_in1", "s_axi_aclk";
- clocks = <&clkc 15>, <&clkc 15>;
- clock-output-names = "clk_out0", "clk_out1", "clk_out2",
- "clk_out3", "clk_out4", "clk_out5",
- "clk_out6", "clk_out7";
- };
--
2.1.1

2019-11-28 06:42:13

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH v3 10/10] clk: clock-wizard: Fix the compilation failure

From: Shubhrajyoti Datta <[email protected]>

After 90b6c5c73 (clk: Remove CLK_IS_BASIC clk flag)
The CLK_IS_BASIC is deleted. Adapt the driver for the same.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
---
drivers/clk/clk-xlnx-clock-wizard.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
index 9993543..76cfa05 100644
--- a/drivers/clk/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/clk-xlnx-clock-wizard.c
@@ -345,7 +345,7 @@ static struct clk *clk_wzrd_register_divf(struct device *dev,
else
init.ops = &clk_wzrd_clk_divider_ops_f;

- init.flags = flags | CLK_IS_BASIC;
+ init.flags = flags;
init.parent_names = (parent_name ? &parent_name : NULL);
init.num_parents = (parent_name ? 1 : 0);

@@ -402,7 +402,7 @@ static struct clk *clk_wzrd_register_divider(struct device *dev,
init.ops = &clk_divider_ro_ops;
else
init.ops = &clk_wzrd_clk_divider_ops;
- init.flags = flags | CLK_IS_BASIC;
+ init.flags = flags;
init.parent_names = (parent_name ? &parent_name : NULL);
init.num_parents = (parent_name ? 1 : 0);

--
2.1.1

2019-11-28 07:47:05

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v3 08/10] clk: clock-wizard: Make the output names unique

On Thu, Nov 28, 2019 at 12:06:15PM +0530, [email protected] wrote:
> From: Shubhrajyoti Datta <[email protected]>
>
> Incase there are more than one instance of the clocking wizard.
> And if the output name given is the same then the probe fails.
> Fix the same by appending the device name to the output name to
> make it unique.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> ---
> drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> index 75ea745..9993543 100644
> --- a/drivers/clk/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> @@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> ret = -ENOMEM;
> goto err_disable_clk;
> }
> + outputs = of_property_count_strings(np, "clock-output-names");
> + if (outputs == 1)
> + flags = CLK_SET_RATE_PARENT;
> clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
> (&pdev->dev, clk_name,
> __clk_get_name(clk_wzrd->clk_in1),
> @@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> goto err_disable_clk;
> }
>
> - outputs = of_property_count_strings(np, "clock-output-names");
> - if (outputs == 1)
> - flags = CLK_SET_RATE_PARENT;
> clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
> if (!clk_name) {
> ret = -ENOMEM;
> @@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> /* register div per output */
> for (i = outputs - 1; i >= 0 ; i--) {
> const char *clkout_name;
> + const char *clkout_name_wiz;
>
> if (of_property_read_string_index(np, "clock-output-names", i,
> &clkout_name)) {
> @@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> ret = -EINVAL;
> goto err_rm_int_clks;
> }
> + clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s",
> + dev_name(&pdev->dev), clkout_name);

If this kasprintf() crashes then clk_wzrd_register_divf() will fail.
But that was a headache to review. Just add a check for NULL. We need
a kfree() as well.

One alternative would be to just declare a buffer on the stack and use
snprintf(). We don't need to keep the name around after the call to
clk_wzrd_register_divf().

regards,
dan carpenter

2019-11-29 12:09:48

by Shubhrajyoti Datta

[permalink] [raw]
Subject: Re: [PATCH v3 08/10] clk: clock-wizard: Make the output names unique

On Thu, Nov 28, 2019 at 1:15 PM Dan Carpenter <[email protected]> wrote:
>
> On Thu, Nov 28, 2019 at 12:06:15PM +0530, [email protected] wrote:
> > From: Shubhrajyoti Datta <[email protected]>
> >
> > Incase there are more than one instance of the clocking wizard.
> > And if the output name given is the same then the probe fails.
> > Fix the same by appending the device name to the output name to
> > make it unique.
> >
> > Signed-off-by: Shubhrajyoti Datta <[email protected]>
> > ---
> > drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> > index 75ea745..9993543 100644
> > --- a/drivers/clk/clk-xlnx-clock-wizard.c
> > +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> > @@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > ret = -ENOMEM;
> > goto err_disable_clk;
> > }
> > + outputs = of_property_count_strings(np, "clock-output-names");
> > + if (outputs == 1)
> > + flags = CLK_SET_RATE_PARENT;
> > clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
> > (&pdev->dev, clk_name,
> > __clk_get_name(clk_wzrd->clk_in1),
> > @@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > goto err_disable_clk;
> > }
> >
> > - outputs = of_property_count_strings(np, "clock-output-names");
> > - if (outputs == 1)
> > - flags = CLK_SET_RATE_PARENT;
> > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
> > if (!clk_name) {
> > ret = -ENOMEM;
> > @@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > /* register div per output */
> > for (i = outputs - 1; i >= 0 ; i--) {
> > const char *clkout_name;
> > + const char *clkout_name_wiz;
> >
> > if (of_property_read_string_index(np, "clock-output-names", i,
> > &clkout_name)) {
> > @@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > ret = -EINVAL;
> > goto err_rm_int_clks;
> > }
> > + clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s",
> > + dev_name(&pdev->dev), clkout_name);
>
> If this kasprintf() crashes then clk_wzrd_register_divf() will fail.
> But that was a headache to review. Just add a check for NULL. We need
> a kfree() as well.
>
> One alternative would be to just declare a buffer on the stack and use
> snprintf(). We don't need to keep the name around after the call to
> clk_wzrd_register_divf().

Will fix in next version.

>
> regards,
> dan carpenter
>

2019-11-29 19:04:07

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v3 08/10] clk: clock-wizard: Make the output names unique

On Fri, Nov 29, 2019 at 05:37:57PM +0530, Shubhrajyoti Datta wrote:
> On Thu, Nov 28, 2019 at 1:15 PM Dan Carpenter <[email protected]> wrote:
> >
> > On Thu, Nov 28, 2019 at 12:06:15PM +0530, [email protected] wrote:
> > > From: Shubhrajyoti Datta <[email protected]>
> > >
> > > Incase there are more than one instance of the clocking wizard.
> > > And if the output name given is the same then the probe fails.
> > > Fix the same by appending the device name to the output name to
> > > make it unique.
> > >
> > > Signed-off-by: Shubhrajyoti Datta <[email protected]>
> > > ---
> > > drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++-----
> > > 1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> > > index 75ea745..9993543 100644
> > > --- a/drivers/clk/clk-xlnx-clock-wizard.c
> > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> > > @@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > > ret = -ENOMEM;
> > > goto err_disable_clk;
> > > }
> > > + outputs = of_property_count_strings(np, "clock-output-names");
> > > + if (outputs == 1)
> > > + flags = CLK_SET_RATE_PARENT;
> > > clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor
> > > (&pdev->dev, clk_name,
> > > __clk_get_name(clk_wzrd->clk_in1),
> > > @@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > > goto err_disable_clk;
> > > }
> > >
> > > - outputs = of_property_count_strings(np, "clock-output-names");
> > > - if (outputs == 1)
> > > - flags = CLK_SET_RATE_PARENT;
> > > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
> > > if (!clk_name) {
> > > ret = -ENOMEM;
> > > @@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > > /* register div per output */
> > > for (i = outputs - 1; i >= 0 ; i--) {
> > > const char *clkout_name;
> > > + const char *clkout_name_wiz;
> > >
> > > if (of_property_read_string_index(np, "clock-output-names", i,
> > > &clkout_name)) {
> > > @@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > > ret = -EINVAL;
> > > goto err_rm_int_clks;
> > > }
> > > + clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s",
> > > + dev_name(&pdev->dev), clkout_name);
> >
> > If this kasprintf() crashes then clk_wzrd_register_divf() will fail.

I meant if kasprintf() returns NULL not crashes... :/

> > But that was a headache to review. Just add a check for NULL. We need
> > a kfree() as well.

regards,
dan carpenter


2019-12-13 19:50:28

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v3 02/10] clk: clock-wizard: Move the clockwizard to clk

On Thu, Nov 28, 2019 at 12:06:09PM +0530, [email protected] wrote:
> From: Shubhrajyoti Datta <[email protected]>
>
> Move the clocking wizard driver from staging to clk.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> ---
> drivers/clk/Kconfig | 6 +
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-xlnx-clock-wizard.c | 335 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 342 insertions(+)
> create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c

I don't see anything moved here.

Rob

2019-12-13 19:54:12

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v3 01/10] dt-bindings: add documentation of xilinx clocking wizard

On Thu, Nov 28, 2019 at 12:06:08PM +0530, [email protected] wrote:
> From: Shubhrajyoti Datta <[email protected]>
>
> Add the devicetree binding for the xilinx clocking wizard.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> ---
> .../bindings/clock/xlnx,clocking-wizard.txt | 32 ++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt

New bindings should be in DT schema format.

>
> diff --git a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt
> new file mode 100644
> index 0000000..aedac84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt
> @@ -0,0 +1,32 @@
> +Binding for Xilinx Clocking Wizard IP Core
> +
> +This binding uses the common clock binding[1]. Details about the devices can be
> +found in the product guide[2].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +[2] Clocking Wizard Product Guide
> +http://www.xilinx.com/support/documentation/ip_documentation/clk_wiz/v5_1/pg065-clk-wiz.pdf
> +
> +Required properties:
> + - compatible: Must be 'xlnx,clocking-wizard'

That's not very specific. Is there only 1 version of this h/w?

> + - #clock-cells: Number of cells in a clock specifier. Should be 1
> + - reg: Base and size of the cores register space
> + - clocks: Handle to input clock
> + - clock-names: Tuple containing 'clk_in1' and 's_axi_aclk'
> + - clock-output-names: Names for the output clocks

You have to define the values.

> +
> +Optional properties:
> + - speed-grade: Speed grade of the device (valid values are 1..3)
> +
> +Example:
> + clock-generator@40040000 {
> + #clock-cells = <1>;
> + reg = <0x40040000 0x1000>;
> + compatible = "xlnx,clocking-wizard";
> + speed-grade = <1>;
> + clock-names = "clk_in1", "s_axi_aclk";
> + clocks = <&clkc 15>, <&clkc 15>;
> + clock-output-names = "clk_out0", "clk_out1", "clk_out2",
> + "clk_out3", "clk_out4", "clk_out5",
> + "clk_out6", "clk_out7";

Don't really need this to be in DT given all the names are the same
except for the index.

Rob

2020-01-05 19:46:43

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v3 09/10] staging: clocking-wizard: Delete the driver from the staging

Quoting [email protected] (2019-11-27 22:36:16)
> From: Shubhrajyoti Datta <[email protected]>
>
> Delete the driver from the staging as it is in drivers/clk.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>

Can all these patches in this series apply to the staging paths and be
picked up by Greg? Then when the driver is ready to be moved out of
staging I would like to see one patch that removes the driver from
staging and adds it to drivers/clk/ so we can be certain the diff is
minimal.

Feel free to add me and linux-clk to the review of the clocking-wizard
driver patches. I will review the driver patches that way.

2020-01-05 19:47:42

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v3 10/10] clk: clock-wizard: Fix the compilation failure

Quoting [email protected] (2019-11-27 22:36:17)
> From: Shubhrajyoti Datta <[email protected]>
>
> After 90b6c5c73 (clk: Remove CLK_IS_BASIC clk flag)
> The CLK_IS_BASIC is deleted. Adapt the driver for the same.

I don't see any CLK_IS_BASIC in the tree right now, so did it get
reintroduced by this patch series? Can you squash this into whatever
patch introduces CLK_IS_BASIC usage?

2020-01-05 19:49:58

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v3 03/10] clk: clock-wizard: Fix kernel-doc warning

Quoting [email protected] (2019-11-27 22:36:10)
> From: Shubhrajyoti Datta <[email protected]>
>
> Update description for the clocking wizard structure
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> ---

Reviewed-by: Stephen Boyd <[email protected]>

2020-01-05 19:55:50

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v3 04/10] clk: clock-wizard: Add support for dynamic reconfiguration

Quoting [email protected] (2019-11-27 22:36:11)
> From: Shubhrajyoti Datta <[email protected]>
>
> 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.
>
> Based on the initial work from Chirag.
>
> Signed-off-by: Chirag Parekh <[email protected]>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>

Are these more like Co-developed-by: tags?

> diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> index ef9125d..870e7fb 100644
> --- a/drivers/clk/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> @@ -29,8 +29,23 @@
> #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_NUM_RETRIES 10000
> +#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
> +
> +/* 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 +77,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 */

Is this register lock used by anything? I'm mostly wondering if dividers
are in a shared register with some other clk so we need to have this
lock. Otherwise it adds more conditional locking code for no gain and
should be removed to simplify.

> +};
> +
> #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
>
> /* maximum frequencies for input/output clocks per speed grade */
> @@ -71,6 +109,164 @@ 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 =
> + (void __iomem *)((u64)divider->base + divider->offset);

Is this casting necessary?

> + 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 = 0;
> + u16 retries;
> + u32 value;
> + unsigned long flags = 0;
> + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
> + void __iomem *div_addr =
> + (void __iomem *)((u64)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 */
> + if (value > WZRD_DR_MAX_INT_DIV_VALUE)
> + value = WZRD_DR_MAX_INT_DIV_VALUE;

Please use min().

> +
> + /* Set divisor and clear phase offset */
> + writel(value, div_addr);
> + writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET);
> +
> + /* Check status register */
> + retries = WZRD_DR_NUM_RETRIES;
> + while (retries--) {
> + if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
> + WZRD_DR_LOCK_BIT_MASK)
> + break;
> + }

Is this readl_poll_timeout()?

> +
> + if (retries == 0) {
> + err = -ETIMEDOUT;
> + goto err_reconfig;
> + }
> +
> + /* Initiate reconfiguration */
> + writel(WZRD_DR_BEGIN_DYNA_RECONF,
> + divider->base + WZRD_DR_INIT_REG_OFFSET);
> +
> + /* Check status register */
> + retries = WZRD_DR_NUM_RETRIES;
> + while (retries--) {
> + if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) &
> + WZRD_DR_LOCK_BIT_MASK)
> + break;
> + }
> +
> + if (retries == 0)
> + err = -ETIMEDOUT;

readl_poll_timeout()?

> +
> +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 donot change parent rate we just round rate to closest

s/donot/don't/

> + * 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;
> +
> + if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {

Do you use HIWORD mask things? I thought that was mostly rockchip
specific quirk. If not, please remove this code.

> + if (width + shift > 16) {
> + pr_warn("divider value exceeds LOWORD field\n");
> + return ERR_PTR(-EINVAL);
> + }
> + }
> +
> + /* allocate the divider */
> + div = kzalloc(sizeof(*div), GFP_KERNEL);
> + if (!div)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
> + init.ops = &clk_divider_ro_ops;
> + else
> + init.ops = &clk_wzrd_clk_divider_ops;
> + init.flags = flags | CLK_IS_BASIC;

Oh yeah, don't add this flag. This code won't compile so please compile
test each patch in a series to make sure we don't have compilation
bisection holes.

> + init.parent_names = (parent_name ? &parent_name : NULL);
> + init.num_parents = (parent_name ? 1 : 0);

Is it sometimes the root of the tree? Seems unlikely so probably just
assume there is a parent all the time.

> +
> + /* struct clk_divider assignments */

Drop useless comment please.

> + 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;
> +
> + /* register the clock */

Drop useless comment please.

> + hw = &div->hw;
> + ret = clk_hw_register(dev, hw);
> + if (ret) {
> + kfree(div);
> + hw = ERR_PTR(ret);
> + }
> +
> + return hw->clk;
> +}
> +
> static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
> void *data)
> {
> @@ -241,11 +437,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),

Please remove useless parenthesis.

> + 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
>

2020-01-05 19:57:07

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v3 06/10] clk: clock-wizard: Remove the hardcoding of the clock outputs

Quoting [email protected] (2019-11-27 22:36:13)
> diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> index bc0354a..4c6155b 100644
> --- a/drivers/clk/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> @@ -493,6 +493,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> const char *clk_name;
> struct clk_wzrd *clk_wzrd;
> struct resource *mem;
> + int outputs;
> struct device_node *np = pdev->dev.of_node;
>
> clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
> @@ -583,7 +584,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> }
>
> /* register div per output */
> - for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
> + for (i = outputs - 1; i >= 0 ; i--) {

Where is 'outputs' assigned in this patch?

> const char *clkout_name;
>
> if (of_property_read_string_index(np, "clock-output-names", i,
> @@ -614,7 +615,7 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> if (IS_ERR(clk_wzrd->clkout[i])) {
> int j;
>
> - for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
> + for (j = i + 1; j < outputs; j++)
> clk_unregister(clk_wzrd->clkout[j]);
> dev_err(&pdev->dev,
> "unable to register divider clock\n");

2020-01-05 20:01:08

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v3 07/10] clk: clock-wizard: Update the fixed factor divisors

Quoting [email protected] (2019-11-27 22:36:14)
> From: Shubhrajyoti Datta <[email protected]>
>
> Update the fixed factor clock registration to register the divisors.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> ---
> drivers/clk/clk-xlnx-clock-wizard.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> index 4c6155b..75ea745 100644
> --- a/drivers/clk/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> @@ -491,9 +491,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> u32 reg, reg_f, mult;
> unsigned long rate;
> const char *clk_name;
> + void __iomem *ctrl_reg;
> struct clk_wzrd *clk_wzrd;
> struct resource *mem;
> int outputs;
> + unsigned long flags = 0;
> struct device_node *np = pdev->dev.of_node;
>
> clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
> @@ -564,19 +566,22 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> goto err_disable_clk;
> }
>
> - /* register div */
> - reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
> - WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
> + outputs = of_property_count_strings(np, "clock-output-names");
> + if (outputs == 1)
> + flags = CLK_SET_RATE_PARENT;

What does the number of clk outputs have to do with the ability to
change the rate of a parent clk? The commit text doesn't inform me of
what this is for either. Please help us understand.

> clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
> if (!clk_name) {
> ret = -ENOMEM;
> goto err_rm_int_clk;
> }
>

2020-01-06 04:18:20

by Shubhrajyoti Datta

[permalink] [raw]
Subject: Re: [PATCH v3 07/10] clk: clock-wizard: Update the fixed factor divisors

On Mon, Jan 6, 2020 at 1:30 AM Stephen Boyd <[email protected]> wrote:
>
> Quoting [email protected] (2019-11-27 22:36:14)
> > From: Shubhrajyoti Datta <[email protected]>
> >
> > Update the fixed factor clock registration to register the divisors.
> >
> > Signed-off-by: Shubhrajyoti Datta <[email protected]>
> > ---
> > drivers/clk/clk-xlnx-clock-wizard.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c
> > index 4c6155b..75ea745 100644
> > --- a/drivers/clk/clk-xlnx-clock-wizard.c
> > +++ b/drivers/clk/clk-xlnx-clock-wizard.c
> > @@ -491,9 +491,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > u32 reg, reg_f, mult;
> > unsigned long rate;
> > const char *clk_name;
> > + void __iomem *ctrl_reg;
> > struct clk_wzrd *clk_wzrd;
> > struct resource *mem;
> > int outputs;
> > + unsigned long flags = 0;
> > struct device_node *np = pdev->dev.of_node;
> >
> > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
> > @@ -564,19 +566,22 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> > goto err_disable_clk;
> > }
> >
> > - /* register div */
> > - reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
> > - WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
> > + outputs = of_property_count_strings(np, "clock-output-names");
> > + if (outputs == 1)
> > + flags = CLK_SET_RATE_PARENT;
>
> What does the number of clk outputs have to do with the ability to
> change the rate of a parent clk? The commit text doesn't inform me of
> what this is for either. Please help us understand.

If there are multiple clocks then changing the rate of the parent
changes the rate of all the
outputs so we donot allow changing the rate of the parent if there are
multiple clocks.
If there is only one output then that is not an issue.

I will update the description in the next version.
>
> > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
> > if (!clk_name) {
> > ret = -ENOMEM;
> > goto err_rm_int_clk;
> > }
> >