* [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
@ 2020-11-16 7:55 Michael Tretter
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
` (13 more replies)
0 siblings, 14 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
Hello,
the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
clocks created from the PLL via dividers.
This series reworks the xlnx_vcu driver to use the common clock framework to
enable other drivers to use the clocks. I originally posted a series to expose
the output clocks as fixed clocks [0]. This series now implements the full
tree from the PLL to the output clocks. Therefore, I am sending a separate
series that focuses on the clocks, but it depends on v4 of the previous series
[1].
Possible consumers for the clocks are the allegro-dvt video encoder driver or
the Xilinx Video Codec Unit [2] out of tree driver.
Patch 1 defines the identifiers that shall be used by clock consumers in the
device tree.
Patch 2 fixes the generic clk-divider to correctly use parents that are passed
via struct clk_hw instead of the clock name.
Patches 3-6 refactor the existing driver and split the function to configure
the PLL into smaller helper functions.
Patch 7 registers a fixed rate clock for the PLL. The driver calculated and
set the PLL configuration during probe, and exposing a fixed rate clock for
that rate allows to use the existing configuration with output clocks from the
common clock framework.
Patches 8-10 switch the driver to the common clock framework and register the
clock provider.
Patches 11-12 are cleanup patches.
Michael
[0] https://lore.kernel.org/linux-arm-kernel/20200619075913.18900-1-m.tretter@pengutronix.de/
[1] https://lore.kernel.org/linux-arm-kernel/20201109134818.4159342-3-m.tretter@pengutronix.de/
[2] https://github.com/Xilinx/vcu-modules
Michael Tretter (12):
ARM: dts: define indexes for output clocks
clk: divider: fix initialization with parent_hw
soc: xilinx: vcu: drop coreclk from struct xlnx_vcu
soc: xilinx: vcu: add helper to wait for PLL locked
soc: xilinx: vcu: add helpers for configuring PLL
soc: xilinx: vcu: implement PLL disable
soc: xilinx: vcu: register PLL as fixed rate clock
soc: xilinx: vcu: implement clock provider for output clocks
soc: xilinx: vcu: make pll post divider explicit
soc: xilinx: vcu: make the PLL configurable
soc: xilinx: vcu: remove calculation of PLL configuration
soc: xilinx: vcu: use bitfields for register definition
drivers/clk/clk-divider.c | 9 +-
drivers/soc/xilinx/Kconfig | 2 +-
drivers/soc/xilinx/xlnx_vcu.c | 613 ++++++++++++++++-----------
include/dt-bindings/clock/xlnx-vcu.h | 15 +
4 files changed, 383 insertions(+), 256 deletions(-)
create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
--
2.20.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 01/12] ARM: dts: define indexes for output clocks
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-02 14:33 ` Michal Simek
` (2 more replies)
2020-11-16 7:55 ` [PATCH 02/12] clk: divider: fix initialization with parent_hw Michael Tretter
` (12 subsequent siblings)
13 siblings, 3 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
The VCU System-Level Control has 4 output clocks. Define indexes for
these clocks to allow to reference them in the device tree.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
include/dt-bindings/clock/xlnx-vcu.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
diff --git a/include/dt-bindings/clock/xlnx-vcu.h b/include/dt-bindings/clock/xlnx-vcu.h
new file mode 100644
index 000000000000..1ed76b9563b6
--- /dev/null
+++ b/include/dt-bindings/clock/xlnx-vcu.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 Pengutronix, Michael Tretter <kernel@pengutronix.de>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_XLNX_VCU_H
+#define _DT_BINDINGS_CLOCK_XLNX_VCU_H
+
+#define CLK_XVCU_ENC_CORE 0
+#define CLK_XVCU_ENC_MCU 1
+#define CLK_XVCU_DEC_CORE 2
+#define CLK_XVCU_DEC_MCU 3
+#define CLK_XVCU_NUM_CLOCKS 4
+
+#endif /* _DT_BINDINGS_CLOCK_XLNX_VCU_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 02/12] clk: divider: fix initialization with parent_hw
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-02 14:28 ` Michal Simek
2020-12-13 5:42 ` Stephen Boyd
2020-11-16 7:55 ` [PATCH 03/12] soc: xilinx: vcu: drop coreclk from struct xlnx_vcu Michael Tretter
` (11 subsequent siblings)
13 siblings, 2 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
If a driver registers a divider clock with a parent_hw instead of the
parent_name, the parent_hw is ignored and the clock does not have a
parent.
Fix this by initializing the parents the same way they are initialized
for clock gates.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/clk/clk-divider.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 8de12cb0c43d..f32157cb4013 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -493,8 +493,13 @@ struct clk_hw *__clk_hw_register_divider(struct device *dev,
else
init.ops = &clk_divider_ops;
init.flags = flags;
- init.parent_names = (parent_name ? &parent_name: NULL);
- init.num_parents = (parent_name ? 1 : 0);
+ init.parent_names = parent_name ? &parent_name : NULL;
+ init.parent_hws = parent_hw ? &parent_hw : NULL;
+ init.parent_data = parent_data;
+ if (parent_name || parent_hw || parent_data)
+ init.num_parents = 1;
+ else
+ init.num_parents = 0;
/* struct clk_divider assignments */
div->reg = reg;
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 03/12] soc: xilinx: vcu: drop coreclk from struct xlnx_vcu
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
2020-11-16 7:55 ` [PATCH 02/12] clk: divider: fix initialization with parent_hw Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 04/12] soc: xilinx: vcu: add helper to wait for PLL locked Michael Tretter
` (10 subsequent siblings)
13 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
The coreclk field is newer read after being written to xlnx_vcu. Remove
the coreclk field from the xlnx_vcu and use a function local variable
instead.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 14daad4efc58..7da9643820a8 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -73,7 +73,6 @@
* @aclk: axi clock source
* @logicore_reg_ba: logicore reg base address
* @vcu_slcr_ba: vcu_slcr Register base address
- * @coreclk: core clock frequency
*/
struct xvcu_device {
struct device *dev;
@@ -81,7 +80,6 @@ struct xvcu_device {
struct clk *aclk;
struct regmap *logicore_reg_ba;
void __iomem *vcu_slcr_ba;
- u32 coreclk;
};
static struct regmap_config vcu_settings_regmap_config = {
@@ -358,10 +356,10 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
return -EINVAL;
}
- xvcu->coreclk = pll_clk / divisor_core;
+ coreclk = pll_clk / divisor_core;
mcuclk = pll_clk / divisor_mcu;
dev_dbg(xvcu->dev, "Actual Ref clock freq is %uHz\n", refclk);
- dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", xvcu->coreclk);
+ dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
vcu_pll_ctrl &= ~(VCU_PLL_CTRL_FBDIV_MASK << VCU_PLL_CTRL_FBDIV_SHIFT);
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 04/12] soc: xilinx: vcu: add helper to wait for PLL locked
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (2 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 03/12] soc: xilinx: vcu: drop coreclk from struct xlnx_vcu Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 05/12] soc: xilinx: vcu: add helpers for configuring PLL Michael Tretter
` (9 subsequent siblings)
13 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
Extract a helper function to wait until the PLL is locked. Also,
disabling the bypass was buried in the exit path on the wait loop.
Separate the different steps and add a helper function to make the code
more readable.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 46 ++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 7da9643820a8..0fd8356a3776 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -256,6 +256,22 @@ static void xvcu_write_field_reg(void __iomem *iomem, int offset,
xvcu_write(iomem, offset, val);
}
+static int xvcu_pll_wait_for_lock(struct xvcu_device *xvcu)
+{
+ void __iomem *base = xvcu->vcu_slcr_ba;
+ unsigned long timeout;
+ u32 lock_status;
+
+ timeout = jiffies + msecs_to_jiffies(2000);
+ do {
+ lock_status = xvcu_read(base, VCU_PLL_STATUS);
+ if (lock_status & VCU_PLL_STATUS_LOCK_STATUS_MASK)
+ return 0;
+ } while (!time_after(jiffies, timeout));
+
+ return -ETIMEDOUT;
+}
+
/**
* xvcu_set_vcu_pll_info - Set the VCU PLL info
* @xvcu: Pointer to the xvcu_device structure
@@ -428,8 +444,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
*/
static int xvcu_set_pll(struct xvcu_device *xvcu)
{
- u32 lock_status;
- unsigned long timeout;
int ret;
ret = xvcu_set_vcu_pll_info(xvcu);
@@ -447,24 +461,18 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
0, VCU_PLL_CTRL_RESET_MASK,
VCU_PLL_CTRL_RESET_SHIFT);
- /*
- * Defined the timeout for the max time to wait the
- * PLL_STATUS to be locked.
- */
- timeout = jiffies + msecs_to_jiffies(2000);
- do {
- lock_status = xvcu_read(xvcu->vcu_slcr_ba, VCU_PLL_STATUS);
- if (lock_status & VCU_PLL_STATUS_LOCK_STATUS_MASK) {
- xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
- 0, VCU_PLL_CTRL_BYPASS_MASK,
- VCU_PLL_CTRL_BYPASS_SHIFT);
- return 0;
- }
- } while (!time_after(jiffies, timeout));
- /* PLL is not locked even after the timeout of the 2sec */
- dev_err(xvcu->dev, "PLL is not locked\n");
- return -ETIMEDOUT;
+ ret = xvcu_pll_wait_for_lock(xvcu);
+ if (ret) {
+ dev_err(xvcu->dev, "PLL is not locked\n");
+ return ret;
+ }
+
+ xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
+ 0, VCU_PLL_CTRL_BYPASS_MASK,
+ VCU_PLL_CTRL_BYPASS_SHIFT);
+
+ return ret;
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 05/12] soc: xilinx: vcu: add helpers for configuring PLL
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (3 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 04/12] soc: xilinx: vcu: add helper to wait for PLL locked Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 06/12] soc: xilinx: vcu: implement PLL disable Michael Tretter
` (8 subsequent siblings)
13 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
The xvcu_set_vcu_pll_info function sets the rate of the PLL and enables
it, which makes it difficult to cleanly convert the driver to the common
clock framework.
Split the function and add separate functions for setting the rate,
enabling the clock and disabling the clock.
Also move the enable of the reference clock from probe to the helper
that enables the PLL.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 171 +++++++++++++++++++++-------------
1 file changed, 104 insertions(+), 67 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 0fd8356a3776..ff66551a5966 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -272,6 +272,105 @@ static int xvcu_pll_wait_for_lock(struct xvcu_device *xvcu)
return -ETIMEDOUT;
}
+static const struct xvcu_pll_cfg *xvcu_find_cfg(int div)
+{
+ const struct xvcu_pll_cfg *cfg = NULL;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(xvcu_pll_cfg) - 1; i++)
+ if (xvcu_pll_cfg[i].fbdiv == div)
+ cfg = &xvcu_pll_cfg[i];
+
+ return cfg;
+}
+
+static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
+{
+ void __iomem *base = xvcu->vcu_slcr_ba;
+ const struct xvcu_pll_cfg *cfg = NULL;
+ u32 vcu_pll_ctrl;
+ u32 cfg_val;
+
+ cfg = xvcu_find_cfg(div);
+ if (!cfg)
+ return -EINVAL;
+
+ vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_FBDIV_MASK << VCU_PLL_CTRL_FBDIV_SHIFT);
+ vcu_pll_ctrl |= (cfg->fbdiv & VCU_PLL_CTRL_FBDIV_MASK) <<
+ VCU_PLL_CTRL_FBDIV_SHIFT;
+ xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
+
+ cfg_val = (cfg->res << VCU_PLL_CFG_RES_SHIFT) |
+ (cfg->cp << VCU_PLL_CFG_CP_SHIFT) |
+ (cfg->lfhf << VCU_PLL_CFG_LFHF_SHIFT) |
+ (cfg->lock_cnt << VCU_PLL_CFG_LOCK_CNT_SHIFT) |
+ (cfg->lock_dly << VCU_PLL_CFG_LOCK_DLY_SHIFT);
+ xvcu_write(base, VCU_PLL_CFG, cfg_val);
+
+ return 0;
+}
+
+static int xvcu_pll_set_rate(struct xvcu_device *xvcu,
+ unsigned long rate, unsigned long parent_rate)
+{
+ return xvcu_pll_set_div(xvcu, rate / parent_rate);
+}
+
+static int xvcu_pll_enable(struct xvcu_device *xvcu)
+{
+ void __iomem *base = xvcu->vcu_slcr_ba;
+ u32 vcu_pll_ctrl;
+ int ret;
+
+ ret = clk_prepare_enable(xvcu->pll_ref);
+ if (ret) {
+ dev_err(xvcu->dev, "failed to enable pll_ref clock source\n");
+ return ret;
+ }
+
+ vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK <<
+ VCU_PLL_CTRL_POR_IN_SHIFT);
+ vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & VCU_PLL_CTRL_POR_IN_MASK) <<
+ VCU_PLL_CTRL_POR_IN_SHIFT;
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_PWR_POR_MASK <<
+ VCU_PLL_CTRL_PWR_POR_SHIFT);
+ vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & VCU_PLL_CTRL_PWR_POR_MASK) <<
+ VCU_PLL_CTRL_PWR_POR_SHIFT;
+ xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
+
+ xvcu_write_field_reg(base, VCU_PLL_CTRL,
+ 1, VCU_PLL_CTRL_BYPASS_MASK,
+ VCU_PLL_CTRL_BYPASS_SHIFT);
+ xvcu_write_field_reg(base, VCU_PLL_CTRL,
+ 1, VCU_PLL_CTRL_RESET_MASK,
+ VCU_PLL_CTRL_RESET_SHIFT);
+ xvcu_write_field_reg(base, VCU_PLL_CTRL,
+ 0, VCU_PLL_CTRL_RESET_MASK,
+ VCU_PLL_CTRL_RESET_SHIFT);
+
+ ret = xvcu_pll_wait_for_lock(xvcu);
+ if (ret) {
+ dev_err(xvcu->dev, "PLL is not locked\n");
+ goto err;
+ }
+
+ xvcu_write_field_reg(base, VCU_PLL_CTRL,
+ 0, VCU_PLL_CTRL_BYPASS_MASK,
+ VCU_PLL_CTRL_BYPASS_SHIFT);
+
+ return ret;
+err:
+ clk_disable_unprepare(xvcu->pll_ref);
+ return ret;
+}
+
+static void xvcu_pll_disable(struct xvcu_device *xvcu)
+{
+ clk_disable_unprepare(xvcu->pll_ref);
+}
+
/**
* xvcu_set_vcu_pll_info - Set the VCU PLL info
* @xvcu: Pointer to the xvcu_device structure
@@ -292,8 +391,8 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
u32 refclk, coreclk, mcuclk, inte, deci;
u32 divisor_mcu, divisor_core, fvco;
u32 clkoutdiv, vcu_pll_ctrl, pll_clk;
- u32 cfg_val, mod, ctrl;
- int ret, i;
+ u32 mod, ctrl;
+ int i;
const struct xvcu_pll_cfg *found = NULL;
regmap_read(xvcu->logicore_reg_ba, VCU_PLL_CLK, &inte);
@@ -312,19 +411,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
dev_dbg(xvcu->dev, "Core clock from logicoreIP is %uHz\n", coreclk);
dev_dbg(xvcu->dev, "Mcu clock from logicoreIP is %uHz\n", mcuclk);
- clk_disable_unprepare(xvcu->pll_ref);
- ret = clk_set_rate(xvcu->pll_ref, refclk);
- if (ret)
- dev_warn(xvcu->dev, "failed to set logicoreIP refclk rate\n");
-
- ret = clk_prepare_enable(xvcu->pll_ref);
- if (ret) {
- dev_err(xvcu->dev, "failed to enable pll_ref clock source\n");
- return ret;
- }
-
- refclk = clk_get_rate(xvcu->pll_ref);
-
/*
* The divide-by-2 should be always enabled (==1)
* to meet the timing in the design.
@@ -378,19 +464,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_FBDIV_MASK << VCU_PLL_CTRL_FBDIV_SHIFT);
- vcu_pll_ctrl |= (found->fbdiv & VCU_PLL_CTRL_FBDIV_MASK) <<
- VCU_PLL_CTRL_FBDIV_SHIFT;
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK <<
- VCU_PLL_CTRL_POR_IN_SHIFT);
- vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & VCU_PLL_CTRL_POR_IN_MASK) <<
- VCU_PLL_CTRL_POR_IN_SHIFT;
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_PWR_POR_MASK <<
- VCU_PLL_CTRL_PWR_POR_SHIFT);
- vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & VCU_PLL_CTRL_PWR_POR_MASK) <<
- VCU_PLL_CTRL_PWR_POR_SHIFT;
- xvcu_write(xvcu->vcu_slcr_ba, VCU_PLL_CTRL, vcu_pll_ctrl);
-
/* Set divisor for the core and mcu clock */
ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL);
ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
@@ -422,15 +495,7 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL, ctrl);
- /* Set RES, CP, LFHF, LOCK_CNT and LOCK_DLY cfg values */
- cfg_val = (found->res << VCU_PLL_CFG_RES_SHIFT) |
- (found->cp << VCU_PLL_CFG_CP_SHIFT) |
- (found->lfhf << VCU_PLL_CFG_LFHF_SHIFT) |
- (found->lock_cnt << VCU_PLL_CFG_LOCK_CNT_SHIFT) |
- (found->lock_dly << VCU_PLL_CFG_LOCK_DLY_SHIFT);
- xvcu_write(xvcu->vcu_slcr_ba, VCU_PLL_CFG, cfg_val);
-
- return 0;
+ return xvcu_pll_set_rate(xvcu, fvco, refclk);
}
/**
@@ -452,27 +517,7 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
return ret;
}
- xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
- 1, VCU_PLL_CTRL_BYPASS_MASK,
- VCU_PLL_CTRL_BYPASS_SHIFT);
- xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
- 1, VCU_PLL_CTRL_RESET_MASK,
- VCU_PLL_CTRL_RESET_SHIFT);
- xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
- 0, VCU_PLL_CTRL_RESET_MASK,
- VCU_PLL_CTRL_RESET_SHIFT);
-
- ret = xvcu_pll_wait_for_lock(xvcu);
- if (ret) {
- dev_err(xvcu->dev, "PLL is not locked\n");
- return ret;
- }
-
- xvcu_write_field_reg(xvcu->vcu_slcr_ba, VCU_PLL_CTRL,
- 0, VCU_PLL_CTRL_BYPASS_MASK,
- VCU_PLL_CTRL_BYPASS_SHIFT);
-
- return ret;
+ return xvcu_pll_enable(xvcu);
}
/**
@@ -555,12 +600,6 @@ static int xvcu_probe(struct platform_device *pdev)
return ret;
}
- ret = clk_prepare_enable(xvcu->pll_ref);
- if (ret) {
- dev_err(&pdev->dev, "pll_ref clock enable failed\n");
- goto error_aclk;
- }
-
/*
* Do the Gasket isolation and put the VCU out of reset
* Bit 0 : Gasket isolation
@@ -580,8 +619,6 @@ static int xvcu_probe(struct platform_device *pdev)
return 0;
error_pll_ref:
- clk_disable_unprepare(xvcu->pll_ref);
-error_aclk:
clk_disable_unprepare(xvcu->aclk);
return ret;
}
@@ -605,7 +642,7 @@ static int xvcu_remove(struct platform_device *pdev)
/* Add the the Gasket isolation and put the VCU in reset. */
regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
- clk_disable_unprepare(xvcu->pll_ref);
+ xvcu_pll_disable(xvcu);
clk_disable_unprepare(xvcu->aclk);
return 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 06/12] soc: xilinx: vcu: implement PLL disable
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (4 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 05/12] soc: xilinx: vcu: add helpers for configuring PLL Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock Michael Tretter
` (7 subsequent siblings)
13 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
The disabling of the PLL is not fully implemented, because according to
the ZynqMP register reference the RESET, POR_IN and PWR_POR bits have to
be set to bring the PLL into reset.
Set the bits to disable the PLL.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index ff66551a5966..34f3299afc0d 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -329,6 +329,10 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
return ret;
}
+ xvcu_write_field_reg(base, VCU_PLL_CTRL,
+ 1, VCU_PLL_CTRL_BYPASS_MASK,
+ VCU_PLL_CTRL_BYPASS_SHIFT);
+
vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK <<
VCU_PLL_CTRL_POR_IN_SHIFT);
@@ -340,15 +344,9 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
VCU_PLL_CTRL_PWR_POR_SHIFT;
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
- xvcu_write_field_reg(base, VCU_PLL_CTRL,
- 1, VCU_PLL_CTRL_BYPASS_MASK,
- VCU_PLL_CTRL_BYPASS_SHIFT);
- xvcu_write_field_reg(base, VCU_PLL_CTRL,
- 1, VCU_PLL_CTRL_RESET_MASK,
- VCU_PLL_CTRL_RESET_SHIFT);
- xvcu_write_field_reg(base, VCU_PLL_CTRL,
- 0, VCU_PLL_CTRL_RESET_MASK,
- VCU_PLL_CTRL_RESET_SHIFT);
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
+ vcu_pll_ctrl |= (0 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
+ xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
ret = xvcu_pll_wait_for_lock(xvcu);
if (ret) {
@@ -368,6 +366,18 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
static void xvcu_pll_disable(struct xvcu_device *xvcu)
{
+ void __iomem *base = xvcu->vcu_slcr_ba;
+ u32 vcu_pll_ctrl;
+
+ vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK << VCU_PLL_CTRL_POR_IN_SHIFT);
+ vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_POR_IN_MASK) << VCU_PLL_CTRL_POR_IN_SHIFT;
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_PWR_POR_MASK << VCU_PLL_CTRL_PWR_POR_SHIFT);
+ vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_PWR_POR_MASK) << VCU_PLL_CTRL_PWR_POR_SHIFT;
+ vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
+ vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
+ xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
+
clk_disable_unprepare(xvcu->pll_ref);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (5 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 06/12] soc: xilinx: vcu: implement PLL disable Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-02 14:41 ` Michal Simek
2020-11-16 7:55 ` [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks Michael Tretter
` (6 subsequent siblings)
13 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
Currently, xvcu_pll_set_rate configures the PLL to a clock rate that is
pre-calculated when probing the driver. To still make the clock
framework aware of the PLL and to allow to configure other clocks based
on the PLL rate, register the PLL as a fixed rate clock.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/Kconfig | 2 +-
drivers/soc/xilinx/xlnx_vcu.c | 17 ++++++++++++++++-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig
index 0b1708dae361..9fe703772e5a 100644
--- a/drivers/soc/xilinx/Kconfig
+++ b/drivers/soc/xilinx/Kconfig
@@ -3,7 +3,7 @@ menu "Xilinx SoC drivers"
config XILINX_VCU
tristate "Xilinx VCU logicoreIP Init"
- depends on HAS_IOMEM
+ depends on HAS_IOMEM && COMMON_CLK
select REGMAP_MMIO
help
Provides the driver to enable and disable the isolation between the
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 34f3299afc0d..725e646aa726 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -7,6 +7,7 @@
* Contacts Dhaval Shah <dshah@xilinx.com>
*/
#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/io.h>
@@ -80,6 +81,7 @@ struct xvcu_device {
struct clk *aclk;
struct regmap *logicore_reg_ba;
void __iomem *vcu_slcr_ba;
+ struct clk_hw *pll;
};
static struct regmap_config vcu_settings_regmap_config = {
@@ -403,7 +405,9 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
u32 clkoutdiv, vcu_pll_ctrl, pll_clk;
u32 mod, ctrl;
int i;
+ int ret;
const struct xvcu_pll_cfg *found = NULL;
+ struct clk_hw *hw;
regmap_read(xvcu->logicore_reg_ba, VCU_PLL_CLK, &inte);
regmap_read(xvcu->logicore_reg_ba, VCU_PLL_CLK_DEC, &deci);
@@ -505,7 +509,18 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL, ctrl);
- return xvcu_pll_set_rate(xvcu, fvco, refclk);
+ ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
+ if (ret)
+ return ret;
+
+ hw = clk_hw_register_fixed_rate(xvcu->dev, "vcu_pll",
+ __clk_get_name(xvcu->pll_ref),
+ 0, pll_clk);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
+ xvcu->pll = hw;
+
+ return 0;
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (6 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-02 14:49 ` Michal Simek
2020-12-13 5:55 ` Stephen Boyd
2020-11-16 7:55 ` [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit Michael Tretter
` (5 subsequent siblings)
13 siblings, 2 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
The VCU System-Level Control uses an internal PLL to drive the core and
MCU clock for the allegro encoder and decoder based on an external PL
clock.
In order be able to ensure that the clocks are enabled and to get their
rate from other drivers, the module must implement a clock provider and
register the clocks at the common clock framework. Other drivers are
then able to access the clock via devicetree bindings.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 191 +++++++++++++++++++++++++++-------
1 file changed, 154 insertions(+), 37 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 725e646aa726..cedc8b7859f7 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -18,6 +18,8 @@
#include <linux/platform_device.h>
#include <linux/regmap.h>
+#include <dt-bindings/clock/xlnx-vcu.h>
+
/* vcu slcr registers, bitmask and shift */
#define VCU_PLL_CTRL 0x24
#define VCU_PLL_CTRL_RESET_MASK 0x01
@@ -50,11 +52,6 @@
#define VCU_ENC_MCU_CTRL 0x34
#define VCU_DEC_CORE_CTRL 0x38
#define VCU_DEC_MCU_CTRL 0x3c
-#define VCU_PLL_DIVISOR_MASK 0x3f
-#define VCU_PLL_DIVISOR_SHIFT 4
-#define VCU_SRCSEL_MASK 0x01
-#define VCU_SRCSEL_SHIFT 0
-#define VCU_SRCSEL_PLL 1
#define VCU_PLL_STATUS 0x60
#define VCU_PLL_STATUS_LOCK_STATUS_MASK 0x01
@@ -82,6 +79,7 @@ struct xvcu_device {
struct regmap *logicore_reg_ba;
void __iomem *vcu_slcr_ba;
struct clk_hw *pll;
+ struct clk_hw_onecell_data *clk_data;
};
static struct regmap_config vcu_settings_regmap_config = {
@@ -403,7 +401,7 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
u32 refclk, coreclk, mcuclk, inte, deci;
u32 divisor_mcu, divisor_core, fvco;
u32 clkoutdiv, vcu_pll_ctrl, pll_clk;
- u32 mod, ctrl;
+ u32 mod;
int i;
int ret;
const struct xvcu_pll_cfg *found = NULL;
@@ -478,37 +476,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
- /* Set divisor for the core and mcu clock */
- ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL);
- ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
- ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) <<
- VCU_PLL_DIVISOR_SHIFT;
- ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
- ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
- xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL, ctrl);
-
- ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL);
- ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
- ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) <<
- VCU_PLL_DIVISOR_SHIFT;
- ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
- ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
- xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL, ctrl);
-
- ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL);
- ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
- ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << VCU_PLL_DIVISOR_SHIFT;
- ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
- ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
- xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL, ctrl);
-
- ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL);
- ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
- ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << VCU_PLL_DIVISOR_SHIFT;
- ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
- ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
- xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL, ctrl);
-
ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
if (ret)
return ret;
@@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
return xvcu_pll_enable(xvcu);
}
+static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
+ const char *name,
+ const char * const *parent_names,
+ u8 num_parents,
+ struct clk_hw *parent_default,
+ void __iomem *reg,
+ spinlock_t *lock)
+{
+ unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
+ u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
+ CLK_DIVIDER_ROUND_CLOSEST;
+ struct clk_hw *mux = NULL;
+ struct clk_hw *divider = NULL;
+ struct clk_hw *gate = NULL;
+ char *name_mux;
+ char *name_div;
+ int err;
+
+ name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
+ if (!name_mux) {
+ err = -ENOMEM;
+ goto err;
+ }
+ mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
+ flags, reg, 0, 1, 0, lock);
+ if (IS_ERR(mux)) {
+ err = PTR_ERR(divider);
+ goto err;
+ }
+ clk_hw_set_parent(mux, parent_default);
+
+ name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div");
+ if (!name_div) {
+ err = -ENOMEM;
+ goto err;
+ }
+ divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags,
+ reg, 4, 6, divider_flags,
+ lock);
+ if (IS_ERR(divider)) {
+ err = PTR_ERR(divider);
+ goto err;
+ }
+
+ gate = clk_hw_register_gate_parent_hw(dev, name, divider,
+ CLK_SET_RATE_PARENT, reg, 12, 0,
+ lock);
+ if (IS_ERR(gate)) {
+ err = PTR_ERR(gate);
+ goto err;
+ }
+
+ return gate;
+
+err:
+ if (!IS_ERR_OR_NULL(gate))
+ clk_hw_unregister_gate(gate);
+ if (!IS_ERR_OR_NULL(divider))
+ clk_hw_unregister_divider(divider);
+ if (!IS_ERR_OR_NULL(mux))
+ clk_hw_unregister_divider(mux);
+
+ return ERR_PTR(err);
+}
+
+static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
+{
+ struct clk_hw *gate = hw;
+ struct clk_hw *divider;
+ struct clk_hw *mux;
+
+ if (!gate)
+ return;
+
+ divider = clk_hw_get_parent(gate);
+ clk_hw_unregister_gate(gate);
+ if (!divider)
+ return;
+
+ mux = clk_hw_get_parent(divider);
+ clk_hw_unregister_mux(mux);
+ if (!divider)
+ return;
+
+ clk_hw_unregister_divider(divider);
+}
+
+static DEFINE_SPINLOCK(venc_core_lock);
+static DEFINE_SPINLOCK(venc_mcu_lock);
+
+static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
+{
+ struct device *dev = xvcu->dev;
+ const char *parent_names[2];
+ struct clk_hw *parent_default;
+ struct clk_hw_onecell_data *data;
+ struct clk_hw **hws;
+ void __iomem *reg_base = xvcu->vcu_slcr_ba;
+
+ data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ data->num = CLK_XVCU_NUM_CLOCKS;
+ hws = data->hws;
+
+ xvcu->clk_data = data;
+
+ parent_default = xvcu->pll;
+ parent_names[0] = "dummy";
+ parent_names[1] = clk_hw_get_name(parent_default);
+
+ hws[CLK_XVCU_ENC_CORE] =
+ xvcu_clk_hw_register_leaf(dev, "venc_core_clk",
+ parent_names,
+ ARRAY_SIZE(parent_names),
+ parent_default,
+ reg_base + VCU_ENC_CORE_CTRL,
+ &venc_core_lock);
+ hws[CLK_XVCU_ENC_MCU] =
+ xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk",
+ parent_names,
+ ARRAY_SIZE(parent_names),
+ parent_default,
+ reg_base + VCU_ENC_MCU_CTRL,
+ &venc_mcu_lock);
+
+ return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data);
+}
+
+static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
+{
+ struct clk_hw_onecell_data *data = xvcu->clk_data;
+ struct clk_hw **hws = data->hws;
+
+ if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_MCU]))
+ xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_MCU]);
+ if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_CORE]))
+ xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_CORE]);
+}
+
/**
* xvcu_probe - Probe existence of the logicoreIP
* and initialize PLL
@@ -639,10 +746,18 @@ static int xvcu_probe(struct platform_device *pdev)
goto error_pll_ref;
}
+ ret = xvcu_register_clock_provider(xvcu);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register clock provider\n");
+ goto error_clk_provider;
+ }
+
dev_set_drvdata(&pdev->dev, xvcu);
return 0;
+error_clk_provider:
+ xvcu_unregister_clock_provider(xvcu);
error_pll_ref:
clk_disable_unprepare(xvcu->aclk);
return ret;
@@ -664,6 +779,8 @@ static int xvcu_remove(struct platform_device *pdev)
if (!xvcu)
return -ENODEV;
+ xvcu_unregister_clock_provider(xvcu);
+
/* Add the the Gasket isolation and put the VCU in reset. */
regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (7 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-02 14:51 ` Michal Simek
2020-11-16 7:55 ` [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable Michael Tretter
` (4 subsequent siblings)
13 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
According to the downstream driver documentation due to timing
constraints the output divider of the PLL has to be set to 1/2. Add a
helper function for that check instead of burying the code in one large
setup function.
The bit is undocumented and marked as reserved in the register
reference.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 51 ++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index cedc8b7859f7..cf8456b4ef78 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -79,6 +79,7 @@ struct xvcu_device {
struct regmap *logicore_reg_ba;
void __iomem *vcu_slcr_ba;
struct clk_hw *pll;
+ struct clk_hw *pll_post;
struct clk_hw_onecell_data *clk_data;
};
@@ -272,6 +273,28 @@ static int xvcu_pll_wait_for_lock(struct xvcu_device *xvcu)
return -ETIMEDOUT;
}
+static struct clk_hw *xvcu_register_pll_post(struct device *dev,
+ const char *name,
+ const char *parent_name,
+ void __iomem *reg_base)
+{
+ u32 div;
+ u32 vcu_pll_ctrl;
+
+ /*
+ * The output divider of the PLL must be set to 1/2 to meet the
+ * timing in the design.
+ */
+ vcu_pll_ctrl = xvcu_read(reg_base, VCU_PLL_CTRL);
+ div = vcu_pll_ctrl >> VCU_PLL_CTRL_CLKOUTDIV_SHIFT;
+ div = div & VCU_PLL_CTRL_CLKOUTDIV_MASK;
+ if (div != 1)
+ return ERR_PTR(-EINVAL);
+
+ return clk_hw_register_fixed_factor(dev, "vcu_pll_post", parent_name,
+ CLK_SET_RATE_PARENT, 1, 2);
+}
+
static const struct xvcu_pll_cfg *xvcu_find_cfg(int div)
{
const struct xvcu_pll_cfg *cfg = NULL;
@@ -400,7 +423,7 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
{
u32 refclk, coreclk, mcuclk, inte, deci;
u32 divisor_mcu, divisor_core, fvco;
- u32 clkoutdiv, vcu_pll_ctrl, pll_clk;
+ u32 pll_clk;
u32 mod;
int i;
int ret;
@@ -423,19 +446,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
dev_dbg(xvcu->dev, "Core clock from logicoreIP is %uHz\n", coreclk);
dev_dbg(xvcu->dev, "Mcu clock from logicoreIP is %uHz\n", mcuclk);
- /*
- * The divide-by-2 should be always enabled (==1)
- * to meet the timing in the design.
- * Otherwise, it's an error
- */
- vcu_pll_ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_PLL_CTRL);
- clkoutdiv = vcu_pll_ctrl >> VCU_PLL_CTRL_CLKOUTDIV_SHIFT;
- clkoutdiv = clkoutdiv & VCU_PLL_CTRL_CLKOUTDIV_MASK;
- if (clkoutdiv != 1) {
- dev_err(xvcu->dev, "clkoutdiv value is invalid\n");
- return -EINVAL;
- }
-
for (i = ARRAY_SIZE(xvcu_pll_cfg) - 1; i >= 0; i--) {
const struct xvcu_pll_cfg *cfg = &xvcu_pll_cfg[i];
@@ -482,7 +492,7 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
hw = clk_hw_register_fixed_rate(xvcu->dev, "vcu_pll",
__clk_get_name(xvcu->pll_ref),
- 0, pll_clk);
+ 0, fvco);
if (IS_ERR(hw))
return PTR_ERR(hw);
xvcu->pll = hw;
@@ -609,6 +619,7 @@ static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
struct clk_hw *parent_default;
struct clk_hw_onecell_data *data;
struct clk_hw **hws;
+ struct clk_hw *hw;
void __iomem *reg_base = xvcu->vcu_slcr_ba;
data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
@@ -619,7 +630,13 @@ static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
xvcu->clk_data = data;
- parent_default = xvcu->pll;
+ hw = xvcu_register_pll_post(dev, "vcu_pll_post",
+ clk_hw_get_name(xvcu->pll), reg_base);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
+ xvcu->pll_post = hw;
+
+ parent_default = xvcu->pll_post;
parent_names[0] = "dummy";
parent_names[1] = clk_hw_get_name(parent_default);
@@ -650,6 +667,8 @@ static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_MCU]);
if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_CORE]))
xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_CORE]);
+
+ clk_hw_unregister_fixed_factor(xvcu->pll_post);
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (8 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-02 14:54 ` Michal Simek
2020-11-16 7:55 ` [PATCH 11/12] soc: xilinx: vcu: remove calculation of PLL configuration Michael Tretter
` (3 subsequent siblings)
13 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
Do not configure the PLL when probing the driver, but register the clock
in the clock framework and do the configuration based on the respective
callbacks.
This is necessary to allow the consumers, i.e., encoder and decoder
drivers, of the xlnx_vcu clock provider to set the clock rate and
actually enable the clocks without relying on some pre-configuration.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 137 ++++++++++++++++++++++++++--------
1 file changed, 106 insertions(+), 31 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index cf8456b4ef78..84d7c46cd42f 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -257,9 +257,18 @@ static void xvcu_write_field_reg(void __iomem *iomem, int offset,
xvcu_write(iomem, offset, val);
}
-static int xvcu_pll_wait_for_lock(struct xvcu_device *xvcu)
+#define to_vcu_pll(_hw) container_of(_hw, struct vcu_pll, hw)
+
+struct vcu_pll {
+ struct clk_hw hw;
+ void __iomem *reg_base;
+ unsigned long fvco_min;
+ unsigned long fvco_max;
+};
+
+static int xvcu_pll_wait_for_lock(struct vcu_pll *pll)
{
- void __iomem *base = xvcu->vcu_slcr_ba;
+ void __iomem *base = pll->reg_base;
unsigned long timeout;
u32 lock_status;
@@ -307,9 +316,9 @@ static const struct xvcu_pll_cfg *xvcu_find_cfg(int div)
return cfg;
}
-static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
+static int xvcu_pll_set_div(struct vcu_pll *pll, int div)
{
- void __iomem *base = xvcu->vcu_slcr_ba;
+ void __iomem *base = pll->reg_base;
const struct xvcu_pll_cfg *cfg = NULL;
u32 vcu_pll_ctrl;
u32 cfg_val;
@@ -334,24 +343,49 @@ static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
return 0;
}
-static int xvcu_pll_set_rate(struct xvcu_device *xvcu,
+static long xvcu_pll_round_rate(struct clk_hw *hw,
+ unsigned long rate, unsigned long *parent_rate)
+{
+ struct vcu_pll *pll = to_vcu_pll(hw);
+ unsigned int feedback_div;
+
+ rate = clamp_t(unsigned long, rate, pll->fvco_min, pll->fvco_max);
+
+ feedback_div = DIV_ROUND_CLOSEST_ULL(rate, *parent_rate);
+ feedback_div = clamp_t(unsigned int, feedback_div, 25, 125);
+
+ return *parent_rate * feedback_div;
+}
+
+static unsigned long xvcu_pll_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct vcu_pll *pll = to_vcu_pll(hw);
+ void __iomem *base = pll->reg_base;
+ unsigned int div;
+ u32 vcu_pll_ctrl;
+
+ vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
+ div = (vcu_pll_ctrl >> VCU_PLL_CTRL_FBDIV_SHIFT) & VCU_PLL_CTRL_FBDIV_MASK;
+
+ return div * parent_rate;
+}
+
+static int xvcu_pll_set_rate(struct clk_hw *hw,
unsigned long rate, unsigned long parent_rate)
{
- return xvcu_pll_set_div(xvcu, rate / parent_rate);
+ struct vcu_pll *pll = to_vcu_pll(hw);
+
+ return xvcu_pll_set_div(pll, rate / parent_rate);
}
-static int xvcu_pll_enable(struct xvcu_device *xvcu)
+static int xvcu_pll_enable(struct clk_hw *hw)
{
- void __iomem *base = xvcu->vcu_slcr_ba;
+ struct vcu_pll *pll = to_vcu_pll(hw);
+ void __iomem *base = pll->reg_base;
u32 vcu_pll_ctrl;
int ret;
- ret = clk_prepare_enable(xvcu->pll_ref);
- if (ret) {
- dev_err(xvcu->dev, "failed to enable pll_ref clock source\n");
- return ret;
- }
-
xvcu_write_field_reg(base, VCU_PLL_CTRL,
1, VCU_PLL_CTRL_BYPASS_MASK,
VCU_PLL_CTRL_BYPASS_SHIFT);
@@ -371,9 +405,9 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
vcu_pll_ctrl |= (0 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
- ret = xvcu_pll_wait_for_lock(xvcu);
+ ret = xvcu_pll_wait_for_lock(pll);
if (ret) {
- dev_err(xvcu->dev, "PLL is not locked\n");
+ pr_err("VCU PLL is not locked\n");
goto err;
}
@@ -381,15 +415,14 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
0, VCU_PLL_CTRL_BYPASS_MASK,
VCU_PLL_CTRL_BYPASS_SHIFT);
- return ret;
err:
- clk_disable_unprepare(xvcu->pll_ref);
return ret;
}
-static void xvcu_pll_disable(struct xvcu_device *xvcu)
+static void xvcu_pll_disable(struct clk_hw *hw)
{
- void __iomem *base = xvcu->vcu_slcr_ba;
+ struct vcu_pll *pll = to_vcu_pll(hw);
+ void __iomem *base = pll->reg_base;
u32 vcu_pll_ctrl;
vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
@@ -400,8 +433,49 @@ static void xvcu_pll_disable(struct xvcu_device *xvcu)
vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
+}
+
+static const struct clk_ops vcu_pll_ops = {
+ .enable = xvcu_pll_enable,
+ .disable = xvcu_pll_disable,
+ .round_rate = xvcu_pll_round_rate,
+ .recalc_rate = xvcu_pll_recalc_rate,
+ .set_rate = xvcu_pll_set_rate,
+};
+
+static struct clk_hw *xvcu_register_pll(struct device *dev,
+ void __iomem *reg_base,
+ const char *name, const char *parent,
+ unsigned long flags)
+{
+ struct vcu_pll *pll;
+ struct clk_hw *hw;
+ struct clk_init_data init;
+ int ret;
+
+ init.name = name;
+ init.parent_names = &parent;
+ init.ops = &vcu_pll_ops;
+ init.num_parents = 1;
+ init.flags = flags;
+
+ pll = devm_kmalloc(dev, sizeof(*pll), GFP_KERNEL);
+ if (!pll)
+ return ERR_PTR(-ENOMEM);
+
+ pll->hw.init = &init;
+ pll->reg_base = reg_base;
+ pll->fvco_min = FVCO_MIN;
+ pll->fvco_max = FVCO_MAX;
+
+ hw = &pll->hw;
+ ret = devm_clk_hw_register(dev, hw);
+ if (ret)
+ return ERR_PTR(ret);
+
+ clk_hw_set_rate_range(hw, pll->fvco_min, pll->fvco_max);
- clk_disable_unprepare(xvcu->pll_ref);
+ return hw;
}
/**
@@ -426,7 +500,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
u32 pll_clk;
u32 mod;
int i;
- int ret;
const struct xvcu_pll_cfg *found = NULL;
struct clk_hw *hw;
@@ -486,13 +559,9 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
- ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
- if (ret)
- return ret;
-
- hw = clk_hw_register_fixed_rate(xvcu->dev, "vcu_pll",
- __clk_get_name(xvcu->pll_ref),
- 0, fvco);
+ hw = xvcu_register_pll(xvcu->dev, xvcu,
+ "vcu_pll", __clk_get_name(xvcu->pll_ref),
+ CLK_SET_RATE_NO_REPARENT);
if (IS_ERR(hw))
return PTR_ERR(hw);
xvcu->pll = hw;
@@ -519,7 +588,7 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
return ret;
}
- return xvcu_pll_enable(xvcu);
+ return 0;
}
static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
@@ -630,6 +699,13 @@ static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
xvcu->clk_data = data;
+ hw = xvcu_register_pll(dev, reg_base,
+ "vcu_pll", __clk_get_name(xvcu->pll_ref),
+ CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
+ xvcu->pll = hw;
+
hw = xvcu_register_pll_post(dev, "vcu_pll_post",
clk_hw_get_name(xvcu->pll), reg_base);
if (IS_ERR(hw))
@@ -803,7 +879,6 @@ static int xvcu_remove(struct platform_device *pdev)
/* Add the the Gasket isolation and put the VCU in reset. */
regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
- xvcu_pll_disable(xvcu);
clk_disable_unprepare(xvcu->aclk);
return 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 11/12] soc: xilinx: vcu: remove calculation of PLL configuration
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (9 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition Michael Tretter
` (2 subsequent siblings)
13 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
As the consumers are now responsible for setting the clock rate via
clock framework, the clock rate is now calculated using round_rate and
the driver does not need to calculate the clock rate beforehand.
Remove the code that calculates the PLL configuration.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 125 ----------------------------------
1 file changed, 125 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 84d7c46cd42f..519699fdc8b9 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -59,10 +59,6 @@
#define MHZ 1000000
#define FVCO_MIN (1500U * MHZ)
#define FVCO_MAX (3000U * MHZ)
-#define DIVISOR_MIN 0
-#define DIVISOR_MAX 63
-#define FRAC 100
-#define LIMIT (10 * MHZ)
/**
* struct xvcu_device - Xilinx VCU init device structure
@@ -478,119 +474,6 @@ static struct clk_hw *xvcu_register_pll(struct device *dev,
return hw;
}
-/**
- * xvcu_set_vcu_pll_info - Set the VCU PLL info
- * @xvcu: Pointer to the xvcu_device structure
- *
- * Programming the VCU PLL based on the user configuration
- * (ref clock freq, core clock freq, mcu clock freq).
- * Core clock frequency has higher priority than mcu clock frequency
- * Errors in following cases
- * - When mcu or clock clock get from logicoreIP is 0
- * - When VCU PLL DIV related bits value other than 1
- * - When proper data not found for given data
- * - When sis570_1 clocksource related operation failed
- *
- * Return: Returns status, either success or error+reason
- */
-static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
-{
- u32 refclk, coreclk, mcuclk, inte, deci;
- u32 divisor_mcu, divisor_core, fvco;
- u32 pll_clk;
- u32 mod;
- int i;
- const struct xvcu_pll_cfg *found = NULL;
- struct clk_hw *hw;
-
- regmap_read(xvcu->logicore_reg_ba, VCU_PLL_CLK, &inte);
- regmap_read(xvcu->logicore_reg_ba, VCU_PLL_CLK_DEC, &deci);
- regmap_read(xvcu->logicore_reg_ba, VCU_CORE_CLK, &coreclk);
- coreclk *= MHZ;
- regmap_read(xvcu->logicore_reg_ba, VCU_MCU_CLK, &mcuclk);
- mcuclk *= MHZ;
- if (!mcuclk || !coreclk) {
- dev_err(xvcu->dev, "Invalid mcu and core clock data\n");
- return -EINVAL;
- }
-
- refclk = (inte * MHZ) + (deci * (MHZ / FRAC));
- dev_dbg(xvcu->dev, "Ref clock from logicoreIP is %uHz\n", refclk);
- dev_dbg(xvcu->dev, "Core clock from logicoreIP is %uHz\n", coreclk);
- dev_dbg(xvcu->dev, "Mcu clock from logicoreIP is %uHz\n", mcuclk);
-
- for (i = ARRAY_SIZE(xvcu_pll_cfg) - 1; i >= 0; i--) {
- const struct xvcu_pll_cfg *cfg = &xvcu_pll_cfg[i];
-
- fvco = cfg->fbdiv * refclk;
- if (fvco >= FVCO_MIN && fvco <= FVCO_MAX) {
- pll_clk = fvco / VCU_PLL_DIV2;
- if (fvco % VCU_PLL_DIV2 != 0)
- pll_clk++;
- mod = pll_clk % coreclk;
- if (mod < LIMIT) {
- divisor_core = pll_clk / coreclk;
- } else if (coreclk - mod < LIMIT) {
- divisor_core = pll_clk / coreclk;
- divisor_core++;
- } else {
- continue;
- }
- if (divisor_core >= DIVISOR_MIN &&
- divisor_core <= DIVISOR_MAX) {
- found = cfg;
- divisor_mcu = pll_clk / mcuclk;
- mod = pll_clk % mcuclk;
- if (mcuclk - mod < LIMIT)
- divisor_mcu++;
- break;
- }
- }
- }
-
- if (!found) {
- dev_err(xvcu->dev, "Invalid clock combination.\n");
- return -EINVAL;
- }
-
- coreclk = pll_clk / divisor_core;
- mcuclk = pll_clk / divisor_mcu;
- dev_dbg(xvcu->dev, "Actual Ref clock freq is %uHz\n", refclk);
- dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
- dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
-
- hw = xvcu_register_pll(xvcu->dev, xvcu,
- "vcu_pll", __clk_get_name(xvcu->pll_ref),
- CLK_SET_RATE_NO_REPARENT);
- if (IS_ERR(hw))
- return PTR_ERR(hw);
- xvcu->pll = hw;
-
- return 0;
-}
-
-/**
- * xvcu_set_pll - PLL init sequence
- * @xvcu: Pointer to the xvcu_device structure
- *
- * Call the api to set the PLL info and once that is done then
- * init the PLL sequence to make the PLL stable.
- *
- * Return: Returns status, either success or error+reason
- */
-static int xvcu_set_pll(struct xvcu_device *xvcu)
-{
- int ret;
-
- ret = xvcu_set_vcu_pll_info(xvcu);
- if (ret) {
- dev_err(xvcu->dev, "failed to set pll info\n");
- return ret;
- }
-
- return 0;
-}
-
static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
const char *name,
const char * const *parent_names,
@@ -834,13 +717,6 @@ static int xvcu_probe(struct platform_device *pdev)
*/
regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, VCU_GASKET_VALUE);
- /* Do the PLL Settings based on the ref clk,core and mcu clk freq */
- ret = xvcu_set_pll(xvcu);
- if (ret) {
- dev_err(&pdev->dev, "Failed to set the pll\n");
- goto error_pll_ref;
- }
-
ret = xvcu_register_clock_provider(xvcu);
if (ret) {
dev_err(&pdev->dev, "failed to register clock provider\n");
@@ -853,7 +729,6 @@ static int xvcu_probe(struct platform_device *pdev)
error_clk_provider:
xvcu_unregister_clock_provider(xvcu);
-error_pll_ref:
clk_disable_unprepare(xvcu->aclk);
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (10 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 11/12] soc: xilinx: vcu: remove calculation of PLL configuration Michael Tretter
@ 2020-11-16 7:55 ` Michael Tretter
2020-12-13 5:47 ` Stephen Boyd
2020-12-03 7:46 ` [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michal Simek
2020-12-13 5:50 ` Stephen Boyd
13 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-11-16 7:55 UTC (permalink / raw)
To: linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, sboyd, Michael Tretter
This makes the register accesses more readable and is closer to what is
usually used in the kernel.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
drivers/soc/xilinx/xlnx_vcu.c | 115 ++++++++++------------------------
1 file changed, 34 insertions(+), 81 deletions(-)
diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
index 519699fdc8b9..2a11b9e8d5fe 100644
--- a/drivers/soc/xilinx/xlnx_vcu.c
+++ b/drivers/soc/xilinx/xlnx_vcu.c
@@ -6,6 +6,7 @@
*
* Contacts Dhaval Shah <dshah@xilinx.com>
*/
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/clk-provider.h>
#include <linux/device.h>
@@ -20,41 +21,26 @@
#include <dt-bindings/clock/xlnx-vcu.h>
-/* vcu slcr registers, bitmask and shift */
#define VCU_PLL_CTRL 0x24
-#define VCU_PLL_CTRL_RESET_MASK 0x01
-#define VCU_PLL_CTRL_RESET_SHIFT 0
-#define VCU_PLL_CTRL_BYPASS_MASK 0x01
-#define VCU_PLL_CTRL_BYPASS_SHIFT 3
-#define VCU_PLL_CTRL_FBDIV_MASK 0x7f
-#define VCU_PLL_CTRL_FBDIV_SHIFT 8
-#define VCU_PLL_CTRL_POR_IN_MASK 0x01
-#define VCU_PLL_CTRL_POR_IN_SHIFT 1
-#define VCU_PLL_CTRL_PWR_POR_MASK 0x01
-#define VCU_PLL_CTRL_PWR_POR_SHIFT 2
-#define VCU_PLL_CTRL_CLKOUTDIV_MASK 0x03
-#define VCU_PLL_CTRL_CLKOUTDIV_SHIFT 16
-#define VCU_PLL_CTRL_DEFAULT 0
-#define VCU_PLL_DIV2 2
+#define VCU_PLL_CTRL_RESET BIT(0)
+#define VCU_PLL_CTRL_POR_IN BIT(1)
+#define VCU_PLL_CTRL_PWR_POR BIT(2)
+#define VCU_PLL_CTRL_BYPASS BIT(3)
+#define VCU_PLL_CTRL_FBDIV GENMASK(14, 8)
+#define VCU_PLL_CTRL_CLKOUTDIV GENMASK(18, 16)
#define VCU_PLL_CFG 0x28
-#define VCU_PLL_CFG_RES_MASK 0x0f
-#define VCU_PLL_CFG_RES_SHIFT 0
-#define VCU_PLL_CFG_CP_MASK 0x0f
-#define VCU_PLL_CFG_CP_SHIFT 5
-#define VCU_PLL_CFG_LFHF_MASK 0x03
-#define VCU_PLL_CFG_LFHF_SHIFT 10
-#define VCU_PLL_CFG_LOCK_CNT_MASK 0x03ff
-#define VCU_PLL_CFG_LOCK_CNT_SHIFT 13
-#define VCU_PLL_CFG_LOCK_DLY_MASK 0x7f
-#define VCU_PLL_CFG_LOCK_DLY_SHIFT 25
+#define VCU_PLL_CFG_RES GENMASK(3, 0)
+#define VCU_PLL_CFG_CP GENMASK(8, 5)
+#define VCU_PLL_CFG_LFHF GENMASK(12, 10)
+#define VCU_PLL_CFG_LOCK_CNT GENMASK(22, 13)
+#define VCU_PLL_CFG_LOCK_DLY GENMASK(31, 25)
#define VCU_ENC_CORE_CTRL 0x30
#define VCU_ENC_MCU_CTRL 0x34
#define VCU_DEC_CORE_CTRL 0x38
#define VCU_DEC_MCU_CTRL 0x3c
-
#define VCU_PLL_STATUS 0x60
-#define VCU_PLL_STATUS_LOCK_STATUS_MASK 0x01
+#define VCU_PLL_STATUS_LOCK_STATUS BIT(0)
#define MHZ 1000000
#define FVCO_MIN (1500U * MHZ)
@@ -234,25 +220,6 @@ static inline void xvcu_write(void __iomem *iomem, u32 offset, u32 value)
iowrite32(value, iomem + offset);
}
-/**
- * xvcu_write_field_reg - Write to the vcu reg field
- * @iomem: vcu reg space base address
- * @offset: vcu reg offset from base
- * @field: vcu reg field to write to
- * @mask: vcu reg mask
- * @shift: vcu reg number of bits to shift the bitfield
- */
-static void xvcu_write_field_reg(void __iomem *iomem, int offset,
- u32 field, u32 mask, int shift)
-{
- u32 val = xvcu_read(iomem, offset);
-
- val &= ~(mask << shift);
- val |= (field & mask) << shift;
-
- xvcu_write(iomem, offset, val);
-}
-
#define to_vcu_pll(_hw) container_of(_hw, struct vcu_pll, hw)
struct vcu_pll {
@@ -271,7 +238,7 @@ static int xvcu_pll_wait_for_lock(struct vcu_pll *pll)
timeout = jiffies + msecs_to_jiffies(2000);
do {
lock_status = xvcu_read(base, VCU_PLL_STATUS);
- if (lock_status & VCU_PLL_STATUS_LOCK_STATUS_MASK)
+ if (lock_status & VCU_PLL_STATUS_LOCK_STATUS)
return 0;
} while (!time_after(jiffies, timeout));
@@ -291,8 +258,7 @@ static struct clk_hw *xvcu_register_pll_post(struct device *dev,
* timing in the design.
*/
vcu_pll_ctrl = xvcu_read(reg_base, VCU_PLL_CTRL);
- div = vcu_pll_ctrl >> VCU_PLL_CTRL_CLKOUTDIV_SHIFT;
- div = div & VCU_PLL_CTRL_CLKOUTDIV_MASK;
+ div = FIELD_GET(VCU_PLL_CTRL_CLKOUTDIV, vcu_pll_ctrl);
if (div != 1)
return ERR_PTR(-EINVAL);
@@ -324,16 +290,15 @@ static int xvcu_pll_set_div(struct vcu_pll *pll, int div)
return -EINVAL;
vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_FBDIV_MASK << VCU_PLL_CTRL_FBDIV_SHIFT);
- vcu_pll_ctrl |= (cfg->fbdiv & VCU_PLL_CTRL_FBDIV_MASK) <<
- VCU_PLL_CTRL_FBDIV_SHIFT;
+ vcu_pll_ctrl &= ~VCU_PLL_CTRL_FBDIV;
+ vcu_pll_ctrl |= FIELD_PREP(VCU_PLL_CTRL_FBDIV, cfg->fbdiv);
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
- cfg_val = (cfg->res << VCU_PLL_CFG_RES_SHIFT) |
- (cfg->cp << VCU_PLL_CFG_CP_SHIFT) |
- (cfg->lfhf << VCU_PLL_CFG_LFHF_SHIFT) |
- (cfg->lock_cnt << VCU_PLL_CFG_LOCK_CNT_SHIFT) |
- (cfg->lock_dly << VCU_PLL_CFG_LOCK_DLY_SHIFT);
+ cfg_val = FIELD_PREP(VCU_PLL_CFG_RES, cfg->res) |
+ FIELD_PREP(VCU_PLL_CFG_CP, cfg->cp) |
+ FIELD_PREP(VCU_PLL_CFG_LFHF, cfg->lfhf) |
+ FIELD_PREP(VCU_PLL_CFG_LOCK_CNT, cfg->lock_cnt) |
+ FIELD_PREP(VCU_PLL_CFG_LOCK_DLY, cfg->lock_dly);
xvcu_write(base, VCU_PLL_CFG, cfg_val);
return 0;
@@ -362,7 +327,7 @@ static unsigned long xvcu_pll_recalc_rate(struct clk_hw *hw,
u32 vcu_pll_ctrl;
vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
- div = (vcu_pll_ctrl >> VCU_PLL_CTRL_FBDIV_SHIFT) & VCU_PLL_CTRL_FBDIV_MASK;
+ div = FIELD_GET(VCU_PLL_CTRL_FBDIV, vcu_pll_ctrl);
return div * parent_rate;
}
@@ -382,23 +347,14 @@ static int xvcu_pll_enable(struct clk_hw *hw)
u32 vcu_pll_ctrl;
int ret;
- xvcu_write_field_reg(base, VCU_PLL_CTRL,
- 1, VCU_PLL_CTRL_BYPASS_MASK,
- VCU_PLL_CTRL_BYPASS_SHIFT);
-
vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK <<
- VCU_PLL_CTRL_POR_IN_SHIFT);
- vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & VCU_PLL_CTRL_POR_IN_MASK) <<
- VCU_PLL_CTRL_POR_IN_SHIFT;
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_PWR_POR_MASK <<
- VCU_PLL_CTRL_PWR_POR_SHIFT);
- vcu_pll_ctrl |= (VCU_PLL_CTRL_DEFAULT & VCU_PLL_CTRL_PWR_POR_MASK) <<
- VCU_PLL_CTRL_PWR_POR_SHIFT;
+ vcu_pll_ctrl |= VCU_PLL_CTRL_BYPASS;
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
- vcu_pll_ctrl |= (0 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
+ vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
+ vcu_pll_ctrl &= ~VCU_PLL_CTRL_POR_IN;
+ vcu_pll_ctrl &= ~VCU_PLL_CTRL_PWR_POR;
+ vcu_pll_ctrl &= ~VCU_PLL_CTRL_RESET;
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
ret = xvcu_pll_wait_for_lock(pll);
@@ -407,9 +363,9 @@ static int xvcu_pll_enable(struct clk_hw *hw)
goto err;
}
- xvcu_write_field_reg(base, VCU_PLL_CTRL,
- 0, VCU_PLL_CTRL_BYPASS_MASK,
- VCU_PLL_CTRL_BYPASS_SHIFT);
+ vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
+ vcu_pll_ctrl &= ~VCU_PLL_CTRL_BYPASS;
+ xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
err:
return ret;
@@ -422,12 +378,9 @@ static void xvcu_pll_disable(struct clk_hw *hw)
u32 vcu_pll_ctrl;
vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_POR_IN_MASK << VCU_PLL_CTRL_POR_IN_SHIFT);
- vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_POR_IN_MASK) << VCU_PLL_CTRL_POR_IN_SHIFT;
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_PWR_POR_MASK << VCU_PLL_CTRL_PWR_POR_SHIFT);
- vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_PWR_POR_MASK) << VCU_PLL_CTRL_PWR_POR_SHIFT;
- vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
- vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
+ vcu_pll_ctrl |= VCU_PLL_CTRL_POR_IN;
+ vcu_pll_ctrl |= VCU_PLL_CTRL_PWR_POR;
+ vcu_pll_ctrl |= VCU_PLL_CTRL_RESET;
xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 02/12] clk: divider: fix initialization with parent_hw
2020-11-16 7:55 ` [PATCH 02/12] clk: divider: fix initialization with parent_hw Michael Tretter
@ 2020-12-02 14:28 ` Michal Simek
2020-12-13 5:42 ` Stephen Boyd
1 sibling, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-02 14:28 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree, Stephen Boyd
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette
Hi Stephen,
On 16. 11. 20 8:55, Michael Tretter wrote:
> If a driver registers a divider clock with a parent_hw instead of the
> parent_name, the parent_hw is ignored and the clock does not have a
> parent.
>
> Fix this by initializing the parents the same way they are initialized
> for clock gates.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> drivers/clk/clk-divider.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 8de12cb0c43d..f32157cb4013 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -493,8 +493,13 @@ struct clk_hw *__clk_hw_register_divider(struct device *dev,
> else
> init.ops = &clk_divider_ops;
> init.flags = flags;
> - init.parent_names = (parent_name ? &parent_name: NULL);
> - init.num_parents = (parent_name ? 1 : 0);
> + init.parent_names = parent_name ? &parent_name : NULL;
> + init.parent_hws = parent_hw ? &parent_hw : NULL;
> + init.parent_data = parent_data;
> + if (parent_name || parent_hw || parent_data)
> + init.num_parents = 1;
> + else
> + init.num_parents = 0;
>
> /* struct clk_divider assignments */
> div->reg = reg;
>
Can you please review this patch?
Thanks,
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 01/12] ARM: dts: define indexes for output clocks
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
@ 2020-12-02 14:33 ` Michal Simek
2020-12-07 19:21 ` Rob Herring
2020-12-13 5:44 ` Stephen Boyd
2 siblings, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-02 14:33 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette, sboyd
On 16. 11. 20 8:55, Michael Tretter wrote:
> The VCU System-Level Control has 4 output clocks. Define indexes for
> these clocks to allow to reference them in the device tree.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> include/dt-bindings/clock/xlnx-vcu.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
>
> diff --git a/include/dt-bindings/clock/xlnx-vcu.h b/include/dt-bindings/clock/xlnx-vcu.h
> new file mode 100644
> index 000000000000..1ed76b9563b6
> --- /dev/null
> +++ b/include/dt-bindings/clock/xlnx-vcu.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020 Pengutronix, Michael Tretter <kernel@pengutronix.de>
> + */
> +
> +#ifndef _DT_BINDINGS_CLOCK_XLNX_VCU_H
> +#define _DT_BINDINGS_CLOCK_XLNX_VCU_H
> +
> +#define CLK_XVCU_ENC_CORE 0
> +#define CLK_XVCU_ENC_MCU 1
> +#define CLK_XVCU_DEC_CORE 2
> +#define CLK_XVCU_DEC_MCU 3
> +#define CLK_XVCU_NUM_CLOCKS 4
> +
> +#endif /* _DT_BINDINGS_CLOCK_XLNX_VCU_H */
>
Missing vcu in subject but I can handle it myself if others patches are
fine. If not please fix it in v2.
Thanks,
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock
2020-11-16 7:55 ` [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock Michael Tretter
@ 2020-12-02 14:41 ` Michal Simek
0 siblings, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-02 14:41 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette, sboyd
On 16. 11. 20 8:55, Michael Tretter wrote:
> Currently, xvcu_pll_set_rate configures the PLL to a clock rate that is
> pre-calculated when probing the driver. To still make the clock
> framework aware of the PLL and to allow to configure other clocks based
> on the PLL rate, register the PLL as a fixed rate clock.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> drivers/soc/xilinx/Kconfig | 2 +-
> drivers/soc/xilinx/xlnx_vcu.c | 17 ++++++++++++++++-
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig
> index 0b1708dae361..9fe703772e5a 100644
> --- a/drivers/soc/xilinx/Kconfig
> +++ b/drivers/soc/xilinx/Kconfig
> @@ -3,7 +3,7 @@ menu "Xilinx SoC drivers"
>
> config XILINX_VCU
> tristate "Xilinx VCU logicoreIP Init"
> - depends on HAS_IOMEM
> + depends on HAS_IOMEM && COMMON_CLK
> select REGMAP_MMIO
> help
> Provides the driver to enable and disable the isolation between the
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index 34f3299afc0d..725e646aa726 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -7,6 +7,7 @@
> * Contacts Dhaval Shah <dshah@xilinx.com>
> */
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/io.h>
> @@ -80,6 +81,7 @@ struct xvcu_device {
> struct clk *aclk;
> struct regmap *logicore_reg_ba;
> void __iomem *vcu_slcr_ba;
> + struct clk_hw *pll;
this is introducing kernel-doc warning. Please describe it there.
M
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
2020-11-16 7:55 ` [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks Michael Tretter
@ 2020-12-02 14:49 ` Michal Simek
2020-12-13 5:55 ` Stephen Boyd
1 sibling, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-02 14:49 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette, sboyd
On 16. 11. 20 8:55, Michael Tretter wrote:
> The VCU System-Level Control uses an internal PLL to drive the core and
> MCU clock for the allegro encoder and decoder based on an external PL
> clock.
>
> In order be able to ensure that the clocks are enabled and to get their
> rate from other drivers, the module must implement a clock provider and
> register the clocks at the common clock framework. Other drivers are
> then able to access the clock via devicetree bindings.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> drivers/soc/xilinx/xlnx_vcu.c | 191 +++++++++++++++++++++++++++-------
> 1 file changed, 154 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index 725e646aa726..cedc8b7859f7 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -18,6 +18,8 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
>
> +#include <dt-bindings/clock/xlnx-vcu.h>
> +
> /* vcu slcr registers, bitmask and shift */
> #define VCU_PLL_CTRL 0x24
> #define VCU_PLL_CTRL_RESET_MASK 0x01
> @@ -50,11 +52,6 @@
> #define VCU_ENC_MCU_CTRL 0x34
> #define VCU_DEC_CORE_CTRL 0x38
> #define VCU_DEC_MCU_CTRL 0x3c
> -#define VCU_PLL_DIVISOR_MASK 0x3f
> -#define VCU_PLL_DIVISOR_SHIFT 4
> -#define VCU_SRCSEL_MASK 0x01
> -#define VCU_SRCSEL_SHIFT 0
> -#define VCU_SRCSEL_PLL 1
>
> #define VCU_PLL_STATUS 0x60
> #define VCU_PLL_STATUS_LOCK_STATUS_MASK 0x01
> @@ -82,6 +79,7 @@ struct xvcu_device {
> struct regmap *logicore_reg_ba;
> void __iomem *vcu_slcr_ba;
> struct clk_hw *pll;
> + struct clk_hw_onecell_data *clk_data;
The same here with clk_data - please update kernel-doc format.
> };
>
> static struct regmap_config vcu_settings_regmap_config = {
> @@ -403,7 +401,7 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
> u32 refclk, coreclk, mcuclk, inte, deci;
> u32 divisor_mcu, divisor_core, fvco;
> u32 clkoutdiv, vcu_pll_ctrl, pll_clk;
> - u32 mod, ctrl;
> + u32 mod;
> int i;
> int ret;
> const struct xvcu_pll_cfg *found = NULL;
> @@ -478,37 +476,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
> dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
> dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
>
> - /* Set divisor for the core and mcu clock */
> - ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL);
> - ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> - ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) <<
> - VCU_PLL_DIVISOR_SHIFT;
> - ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> - ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> - xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_CORE_CTRL, ctrl);
> -
> - ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL);
> - ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> - ctrl |= (divisor_core & VCU_PLL_DIVISOR_MASK) <<
> - VCU_PLL_DIVISOR_SHIFT;
> - ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> - ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> - xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_CORE_CTRL, ctrl);
> -
> - ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL);
> - ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> - ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << VCU_PLL_DIVISOR_SHIFT;
> - ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> - ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> - xvcu_write(xvcu->vcu_slcr_ba, VCU_ENC_MCU_CTRL, ctrl);
> -
> - ctrl = xvcu_read(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL);
> - ctrl &= ~(VCU_PLL_DIVISOR_MASK << VCU_PLL_DIVISOR_SHIFT);
> - ctrl |= (divisor_mcu & VCU_PLL_DIVISOR_MASK) << VCU_PLL_DIVISOR_SHIFT;
> - ctrl &= ~(VCU_SRCSEL_MASK << VCU_SRCSEL_SHIFT);
> - ctrl |= (VCU_SRCSEL_PLL & VCU_SRCSEL_MASK) << VCU_SRCSEL_SHIFT;
> - xvcu_write(xvcu->vcu_slcr_ba, VCU_DEC_MCU_CTRL, ctrl);
> -
> ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
> if (ret)
> return ret;
> @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
> return xvcu_pll_enable(xvcu);
> }
>
> +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
> + const char *name,
> + const char * const *parent_names,
> + u8 num_parents,
> + struct clk_hw *parent_default,
> + void __iomem *reg,
> + spinlock_t *lock)
> +{
> + unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
> + u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
> + CLK_DIVIDER_ROUND_CLOSEST;
> + struct clk_hw *mux = NULL;
> + struct clk_hw *divider = NULL;
> + struct clk_hw *gate = NULL;
> + char *name_mux;
> + char *name_div;
> + int err;
> +
> + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> + if (!name_mux) {
> + err = -ENOMEM;
> + goto err;
> + }
> + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> + flags, reg, 0, 1, 0, lock);
> + if (IS_ERR(mux)) {
> + err = PTR_ERR(divider);
mux here?
And smatch is reporting also this issue. Please take a look.
drivers/soc/xilinx/xlnx_vcu.c:541 xvcu_clk_hw_register_leaf() warn:
passing zero to 'PTR_ERR'
drivers/soc/xilinx/xlnx_vcu.c:577 xvcu_clk_hw_register_leaf() warn:
passing zero to 'ERR_PTR'
> + goto err;
> + }
> + clk_hw_set_parent(mux, parent_default);
> +
> + name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div");
> + if (!name_div) {
> + err = -ENOMEM;
> + goto err;
> + }
> + divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags,
> + reg, 4, 6, divider_flags,
> + lock);
> + if (IS_ERR(divider)) {
> + err = PTR_ERR(divider);
> + goto err;
> + }
> +
> + gate = clk_hw_register_gate_parent_hw(dev, name, divider,
> + CLK_SET_RATE_PARENT, reg, 12, 0,
> + lock);
> + if (IS_ERR(gate)) {
> + err = PTR_ERR(gate);
> + goto err;
> + }
> +
> + return gate;
> +
> +err:
> + if (!IS_ERR_OR_NULL(gate))
> + clk_hw_unregister_gate(gate);
> + if (!IS_ERR_OR_NULL(divider))
> + clk_hw_unregister_divider(divider);
> + if (!IS_ERR_OR_NULL(mux))
> + clk_hw_unregister_divider(mux);
> +
> + return ERR_PTR(err);
> +}
> +
> +static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> +{
> + struct clk_hw *gate = hw;
> + struct clk_hw *divider;
> + struct clk_hw *mux;
> +
> + if (!gate)
> + return;
> +
> + divider = clk_hw_get_parent(gate);
> + clk_hw_unregister_gate(gate);
> + if (!divider)
> + return;
> +
> + mux = clk_hw_get_parent(divider);
> + clk_hw_unregister_mux(mux);
> + if (!divider)
> + return;
> +
> + clk_hw_unregister_divider(divider);
> +}
> +
> +static DEFINE_SPINLOCK(venc_core_lock);
> +static DEFINE_SPINLOCK(venc_mcu_lock);
> +
> +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> +{
> + struct device *dev = xvcu->dev;
> + const char *parent_names[2];
> + struct clk_hw *parent_default;
> + struct clk_hw_onecell_data *data;
> + struct clk_hw **hws;
> + void __iomem *reg_base = xvcu->vcu_slcr_ba;
> +
> + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + data->num = CLK_XVCU_NUM_CLOCKS;
> + hws = data->hws;
> +
> + xvcu->clk_data = data;
> +
> + parent_default = xvcu->pll;
> + parent_names[0] = "dummy";
> + parent_names[1] = clk_hw_get_name(parent_default);
> +
> + hws[CLK_XVCU_ENC_CORE] =
> + xvcu_clk_hw_register_leaf(dev, "venc_core_clk",
> + parent_names,
> + ARRAY_SIZE(parent_names),
> + parent_default,
> + reg_base + VCU_ENC_CORE_CTRL,
> + &venc_core_lock);
> + hws[CLK_XVCU_ENC_MCU] =
> + xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk",
> + parent_names,
> + ARRAY_SIZE(parent_names),
> + parent_default,
> + reg_base + VCU_ENC_MCU_CTRL,
> + &venc_mcu_lock);
> +
> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data);
> +}
> +
> +static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
> +{
> + struct clk_hw_onecell_data *data = xvcu->clk_data;
> + struct clk_hw **hws = data->hws;
> +
> + if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_MCU]))
> + xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_MCU]);
> + if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_CORE]))
> + xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_CORE]);
> +}
> +
> /**
> * xvcu_probe - Probe existence of the logicoreIP
> * and initialize PLL
> @@ -639,10 +746,18 @@ static int xvcu_probe(struct platform_device *pdev)
> goto error_pll_ref;
> }
>
> + ret = xvcu_register_clock_provider(xvcu);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register clock provider\n");
> + goto error_clk_provider;
> + }
> +
> dev_set_drvdata(&pdev->dev, xvcu);
>
> return 0;
>
> +error_clk_provider:
> + xvcu_unregister_clock_provider(xvcu);
> error_pll_ref:
> clk_disable_unprepare(xvcu->aclk);
> return ret;
> @@ -664,6 +779,8 @@ static int xvcu_remove(struct platform_device *pdev)
> if (!xvcu)
> return -ENODEV;
>
> + xvcu_unregister_clock_provider(xvcu);
> +
> /* Add the the Gasket isolation and put the VCU in reset. */
> regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
>
>
M
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit
2020-11-16 7:55 ` [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit Michael Tretter
@ 2020-12-02 14:51 ` Michal Simek
0 siblings, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-02 14:51 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette, sboyd
On 16. 11. 20 8:55, Michael Tretter wrote:
> According to the downstream driver documentation due to timing
> constraints the output divider of the PLL has to be set to 1/2. Add a
> helper function for that check instead of burying the code in one large
> setup function.
>
> The bit is undocumented and marked as reserved in the register
> reference.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> drivers/soc/xilinx/xlnx_vcu.c | 51 ++++++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index cedc8b7859f7..cf8456b4ef78 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -79,6 +79,7 @@ struct xvcu_device {
> struct regmap *logicore_reg_ba;
> void __iomem *vcu_slcr_ba;
> struct clk_hw *pll;
> + struct clk_hw *pll_post;
kernel doc again.
M
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable
2020-11-16 7:55 ` [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable Michael Tretter
@ 2020-12-02 14:54 ` Michal Simek
0 siblings, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-02 14:54 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette, sboyd
On 16. 11. 20 8:55, Michael Tretter wrote:
> Do not configure the PLL when probing the driver, but register the clock
> in the clock framework and do the configuration based on the respective
> callbacks.
>
> This is necessary to allow the consumers, i.e., encoder and decoder
> drivers, of the xlnx_vcu clock provider to set the clock rate and
> actually enable the clocks without relying on some pre-configuration.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> drivers/soc/xilinx/xlnx_vcu.c | 137 ++++++++++++++++++++++++++--------
> 1 file changed, 106 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index cf8456b4ef78..84d7c46cd42f 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -257,9 +257,18 @@ static void xvcu_write_field_reg(void __iomem *iomem, int offset,
> xvcu_write(iomem, offset, val);
> }
>
> -static int xvcu_pll_wait_for_lock(struct xvcu_device *xvcu)
> +#define to_vcu_pll(_hw) container_of(_hw, struct vcu_pll, hw)
> +
> +struct vcu_pll {
> + struct clk_hw hw;
> + void __iomem *reg_base;
> + unsigned long fvco_min;
> + unsigned long fvco_max;
> +};
> +
> +static int xvcu_pll_wait_for_lock(struct vcu_pll *pll)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + void __iomem *base = pll->reg_base;
> unsigned long timeout;
> u32 lock_status;
>
> @@ -307,9 +316,9 @@ static const struct xvcu_pll_cfg *xvcu_find_cfg(int div)
> return cfg;
> }
>
> -static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
> +static int xvcu_pll_set_div(struct vcu_pll *pll, int div)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + void __iomem *base = pll->reg_base;
> const struct xvcu_pll_cfg *cfg = NULL;
> u32 vcu_pll_ctrl;
> u32 cfg_val;
> @@ -334,24 +343,49 @@ static int xvcu_pll_set_div(struct xvcu_device *xvcu, int div)
> return 0;
> }
>
> -static int xvcu_pll_set_rate(struct xvcu_device *xvcu,
> +static long xvcu_pll_round_rate(struct clk_hw *hw,
> + unsigned long rate, unsigned long *parent_rate)
> +{
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + unsigned int feedback_div;
> +
> + rate = clamp_t(unsigned long, rate, pll->fvco_min, pll->fvco_max);
> +
> + feedback_div = DIV_ROUND_CLOSEST_ULL(rate, *parent_rate);
> + feedback_div = clamp_t(unsigned int, feedback_div, 25, 125);
> +
> + return *parent_rate * feedback_div;
> +}
> +
> +static unsigned long xvcu_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + void __iomem *base = pll->reg_base;
> + unsigned int div;
> + u32 vcu_pll_ctrl;
> +
> + vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
> + div = (vcu_pll_ctrl >> VCU_PLL_CTRL_FBDIV_SHIFT) & VCU_PLL_CTRL_FBDIV_MASK;
> +
> + return div * parent_rate;
> +}
> +
> +static int xvcu_pll_set_rate(struct clk_hw *hw,
> unsigned long rate, unsigned long parent_rate)
> {
> - return xvcu_pll_set_div(xvcu, rate / parent_rate);
> + struct vcu_pll *pll = to_vcu_pll(hw);
> +
> + return xvcu_pll_set_div(pll, rate / parent_rate);
> }
>
> -static int xvcu_pll_enable(struct xvcu_device *xvcu)
> +static int xvcu_pll_enable(struct clk_hw *hw)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + void __iomem *base = pll->reg_base;
> u32 vcu_pll_ctrl;
> int ret;
>
> - ret = clk_prepare_enable(xvcu->pll_ref);
> - if (ret) {
> - dev_err(xvcu->dev, "failed to enable pll_ref clock source\n");
> - return ret;
> - }
> -
> xvcu_write_field_reg(base, VCU_PLL_CTRL,
> 1, VCU_PLL_CTRL_BYPASS_MASK,
> VCU_PLL_CTRL_BYPASS_SHIFT);
> @@ -371,9 +405,9 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
> vcu_pll_ctrl |= (0 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
> xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
>
> - ret = xvcu_pll_wait_for_lock(xvcu);
> + ret = xvcu_pll_wait_for_lock(pll);
> if (ret) {
> - dev_err(xvcu->dev, "PLL is not locked\n");
> + pr_err("VCU PLL is not locked\n");
> goto err;
> }
>
> @@ -381,15 +415,14 @@ static int xvcu_pll_enable(struct xvcu_device *xvcu)
> 0, VCU_PLL_CTRL_BYPASS_MASK,
> VCU_PLL_CTRL_BYPASS_SHIFT);
>
> - return ret;
> err:
> - clk_disable_unprepare(xvcu->pll_ref);
> return ret;
> }
>
> -static void xvcu_pll_disable(struct xvcu_device *xvcu)
> +static void xvcu_pll_disable(struct clk_hw *hw)
> {
> - void __iomem *base = xvcu->vcu_slcr_ba;
> + struct vcu_pll *pll = to_vcu_pll(hw);
> + void __iomem *base = pll->reg_base;
> u32 vcu_pll_ctrl;
>
> vcu_pll_ctrl = xvcu_read(base, VCU_PLL_CTRL);
> @@ -400,8 +433,49 @@ static void xvcu_pll_disable(struct xvcu_device *xvcu)
> vcu_pll_ctrl &= ~(VCU_PLL_CTRL_RESET_MASK << VCU_PLL_CTRL_RESET_SHIFT);
> vcu_pll_ctrl |= (1 & VCU_PLL_CTRL_RESET_MASK) << VCU_PLL_CTRL_RESET_SHIFT;
> xvcu_write(base, VCU_PLL_CTRL, vcu_pll_ctrl);
> +}
> +
> +static const struct clk_ops vcu_pll_ops = {
> + .enable = xvcu_pll_enable,
> + .disable = xvcu_pll_disable,
> + .round_rate = xvcu_pll_round_rate,
> + .recalc_rate = xvcu_pll_recalc_rate,
> + .set_rate = xvcu_pll_set_rate,
> +};
> +
> +static struct clk_hw *xvcu_register_pll(struct device *dev,
> + void __iomem *reg_base,
> + const char *name, const char *parent,
> + unsigned long flags)
> +{
> + struct vcu_pll *pll;
> + struct clk_hw *hw;
> + struct clk_init_data init;
> + int ret;
> +
> + init.name = name;
> + init.parent_names = &parent;
> + init.ops = &vcu_pll_ops;
> + init.num_parents = 1;
> + init.flags = flags;
> +
> + pll = devm_kmalloc(dev, sizeof(*pll), GFP_KERNEL);
> + if (!pll)
> + return ERR_PTR(-ENOMEM);
> +
> + pll->hw.init = &init;
> + pll->reg_base = reg_base;
> + pll->fvco_min = FVCO_MIN;
> + pll->fvco_max = FVCO_MAX;
> +
> + hw = &pll->hw;
> + ret = devm_clk_hw_register(dev, hw);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + clk_hw_set_rate_range(hw, pll->fvco_min, pll->fvco_max);
>
> - clk_disable_unprepare(xvcu->pll_ref);
> + return hw;
> }
>
> /**
> @@ -426,7 +500,6 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
> u32 pll_clk;
> u32 mod;
> int i;
> - int ret;
> const struct xvcu_pll_cfg *found = NULL;
> struct clk_hw *hw;
>
> @@ -486,13 +559,9 @@ static int xvcu_set_vcu_pll_info(struct xvcu_device *xvcu)
> dev_dbg(xvcu->dev, "Actual Core clock freq is %uHz\n", coreclk);
> dev_dbg(xvcu->dev, "Actual Mcu clock freq is %uHz\n", mcuclk);
>
> - ret = xvcu_pll_set_rate(xvcu, fvco, refclk);
> - if (ret)
> - return ret;
> -
> - hw = clk_hw_register_fixed_rate(xvcu->dev, "vcu_pll",
> - __clk_get_name(xvcu->pll_ref),
> - 0, fvco);
> + hw = xvcu_register_pll(xvcu->dev, xvcu,
> + "vcu_pll", __clk_get_name(xvcu->pll_ref),
> + CLK_SET_RATE_NO_REPARENT);
You should be passing address not xvcu structure itself.
Reported by sparse.
drivers/soc/xilinx/xlnx_vcu.c:562:43: warning: incorrect type in
argument 2 (different address spaces)
drivers/soc/xilinx/xlnx_vcu.c:562:43: expected void [noderef] __iomem
*reg_base
drivers/soc/xilinx/xlnx_vcu.c:562:43: got struct xvcu_device *xvcu
Thanks,
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (11 preceding siblings ...)
2020-11-16 7:55 ` [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition Michael Tretter
@ 2020-12-03 7:46 ` Michal Simek
2020-12-03 9:00 ` Michael Tretter
2020-12-13 5:50 ` Stephen Boyd
13 siblings, 1 reply; 34+ messages in thread
From: Michal Simek @ 2020-12-03 7:46 UTC (permalink / raw)
To: Michael Tretter, linux-arm-kernel, linux-clk, devicetree
Cc: rajanv, tejasp, dshah, rvisaval, kernel, robh+dt, mturquette, sboyd
On 16. 11. 20 8:55, Michael Tretter wrote:
> Hello,
>
> the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> clocks created from the PLL via dividers.
>
> This series reworks the xlnx_vcu driver to use the common clock framework to
> enable other drivers to use the clocks. I originally posted a series to expose
> the output clocks as fixed clocks [0]. This series now implements the full
> tree from the PLL to the output clocks. Therefore, I am sending a separate
> series that focuses on the clocks, but it depends on v4 of the previous series
> [1].
>
> Possible consumers for the clocks are the allegro-dvt video encoder driver or
> the Xilinx Video Codec Unit [2] out of tree driver.
>
> Patch 1 defines the identifiers that shall be used by clock consumers in the
> device tree.
>
> Patch 2 fixes the generic clk-divider to correctly use parents that are passed
> via struct clk_hw instead of the clock name.
>
> Patches 3-6 refactor the existing driver and split the function to configure
> the PLL into smaller helper functions.
>
> Patch 7 registers a fixed rate clock for the PLL. The driver calculated and
> set the PLL configuration during probe, and exposing a fixed rate clock for
> that rate allows to use the existing configuration with output clocks from the
> common clock framework.
>
> Patches 8-10 switch the driver to the common clock framework and register the
> clock provider.
>
> Patches 11-12 are cleanup patches.
>
> Michael
>
> [0] https://lore.kernel.org/linux-arm-kernel/20200619075913.18900-1-m.tretter@pengutronix.de/
> [1] https://lore.kernel.org/linux-arm-kernel/20201109134818.4159342-3-m.tretter@pengutronix.de/
> [2] https://github.com/Xilinx/vcu-modules
>
> Michael Tretter (12):
> ARM: dts: define indexes for output clocks
> clk: divider: fix initialization with parent_hw
> soc: xilinx: vcu: drop coreclk from struct xlnx_vcu
> soc: xilinx: vcu: add helper to wait for PLL locked
> soc: xilinx: vcu: add helpers for configuring PLL
> soc: xilinx: vcu: implement PLL disable
> soc: xilinx: vcu: register PLL as fixed rate clock
> soc: xilinx: vcu: implement clock provider for output clocks
> soc: xilinx: vcu: make pll post divider explicit
> soc: xilinx: vcu: make the PLL configurable
> soc: xilinx: vcu: remove calculation of PLL configuration
> soc: xilinx: vcu: use bitfields for register definition
>
> drivers/clk/clk-divider.c | 9 +-
> drivers/soc/xilinx/Kconfig | 2 +-
> drivers/soc/xilinx/xlnx_vcu.c | 613 ++++++++++++++++-----------
> include/dt-bindings/clock/xlnx-vcu.h | 15 +
> 4 files changed, 383 insertions(+), 256 deletions(-)
> create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
>
I can't see any other problem with this series.
When we are on this. Can you also please fix these issues reported by
checkpatch to have new issues more visible?
./scripts/checkpatch.pl --strict -f drivers/soc/xilinx/xlnx_vcu.c
CHECK: Alignment should match open parenthesis
#614: FILE: drivers/soc/xilinx/xlnx_vcu.c:614:
+ xvcu->vcu_slcr_ba = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
WARNING: Possible repeated word: 'the'
#707: FILE: drivers/soc/xilinx/xlnx_vcu.c:707:
+ /* Add the the Gasket isolation and put the VCU in reset. */
total: 0 errors, 1 warnings, 1 checks, 735 lines checked
Thanks,
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-12-03 7:46 ` [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michal Simek
@ 2020-12-03 9:00 ` Michael Tretter
2020-12-03 9:14 ` Michal Simek
0 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-12-03 9:00 UTC (permalink / raw)
To: Michal Simek
Cc: linux-arm-kernel, linux-clk, devicetree, rajanv, tejasp, dshah,
rvisaval, kernel, robh+dt, mturquette, sboyd
On Thu, 03 Dec 2020 08:46:12 +0100, Michal Simek wrote:
>
>
> On 16. 11. 20 8:55, Michael Tretter wrote:
> > Hello,
> >
> > the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> > clocks created from the PLL via dividers.
> >
> > This series reworks the xlnx_vcu driver to use the common clock framework to
> > enable other drivers to use the clocks. I originally posted a series to expose
> > the output clocks as fixed clocks [0]. This series now implements the full
> > tree from the PLL to the output clocks. Therefore, I am sending a separate
> > series that focuses on the clocks, but it depends on v4 of the previous series
> > [1].
> >
> > Possible consumers for the clocks are the allegro-dvt video encoder driver or
> > the Xilinx Video Codec Unit [2] out of tree driver.
> >
> > Patch 1 defines the identifiers that shall be used by clock consumers in the
> > device tree.
> >
> > Patch 2 fixes the generic clk-divider to correctly use parents that are passed
> > via struct clk_hw instead of the clock name.
> >
> > Patches 3-6 refactor the existing driver and split the function to configure
> > the PLL into smaller helper functions.
> >
> > Patch 7 registers a fixed rate clock for the PLL. The driver calculated and
> > set the PLL configuration during probe, and exposing a fixed rate clock for
> > that rate allows to use the existing configuration with output clocks from the
> > common clock framework.
> >
> > Patches 8-10 switch the driver to the common clock framework and register the
> > clock provider.
> >
> > Patches 11-12 are cleanup patches.
> >
> > Michael
> >
> > [0] https://lore.kernel.org/linux-arm-kernel/20200619075913.18900-1-m.tretter@pengutronix.de/
> > [1] https://lore.kernel.org/linux-arm-kernel/20201109134818.4159342-3-m.tretter@pengutronix.de/
> > [2] https://github.com/Xilinx/vcu-modules
> >
> > Michael Tretter (12):
> > ARM: dts: define indexes for output clocks
> > clk: divider: fix initialization with parent_hw
> > soc: xilinx: vcu: drop coreclk from struct xlnx_vcu
> > soc: xilinx: vcu: add helper to wait for PLL locked
> > soc: xilinx: vcu: add helpers for configuring PLL
> > soc: xilinx: vcu: implement PLL disable
> > soc: xilinx: vcu: register PLL as fixed rate clock
> > soc: xilinx: vcu: implement clock provider for output clocks
> > soc: xilinx: vcu: make pll post divider explicit
> > soc: xilinx: vcu: make the PLL configurable
> > soc: xilinx: vcu: remove calculation of PLL configuration
> > soc: xilinx: vcu: use bitfields for register definition
> >
> > drivers/clk/clk-divider.c | 9 +-
> > drivers/soc/xilinx/Kconfig | 2 +-
> > drivers/soc/xilinx/xlnx_vcu.c | 613 ++++++++++++++++-----------
> > include/dt-bindings/clock/xlnx-vcu.h | 15 +
> > 4 files changed, 383 insertions(+), 256 deletions(-)
> > create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
> >
>
> I can't see any other problem with this series.
Thanks for the review! I will wait a bit longer if there is some review
feedback by Stephen regarding patch 2, and then send a v2.
> When we are on this. Can you also please fix these issues reported by
> checkpatch to have new issues more visible?
>
> ./scripts/checkpatch.pl --strict -f drivers/soc/xilinx/xlnx_vcu.c
> CHECK: Alignment should match open parenthesis
> #614: FILE: drivers/soc/xilinx/xlnx_vcu.c:614:
> + xvcu->vcu_slcr_ba = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
>
> WARNING: Possible repeated word: 'the'
> #707: FILE: drivers/soc/xilinx/xlnx_vcu.c:707:
> + /* Add the the Gasket isolation and put the VCU in reset. */
>
> total: 0 errors, 1 warnings, 1 checks, 735 lines checked
Sure. I will add a patch to fix the warnings in v2.
Michael
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-12-03 9:00 ` Michael Tretter
@ 2020-12-03 9:14 ` Michal Simek
0 siblings, 0 replies; 34+ messages in thread
From: Michal Simek @ 2020-12-03 9:14 UTC (permalink / raw)
To: Michael Tretter, Michal Simek
Cc: linux-arm-kernel, linux-clk, devicetree, rajanv, tejasp, dshah,
rvisaval, kernel, robh+dt, mturquette, sboyd
On 03. 12. 20 10:00, Michael Tretter wrote:
> On Thu, 03 Dec 2020 08:46:12 +0100, Michal Simek wrote:
>>
>>
>> On 16. 11. 20 8:55, Michael Tretter wrote:
>>> Hello,
>>>
>>> the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
>>> clocks created from the PLL via dividers.
>>>
>>> This series reworks the xlnx_vcu driver to use the common clock framework to
>>> enable other drivers to use the clocks. I originally posted a series to expose
>>> the output clocks as fixed clocks [0]. This series now implements the full
>>> tree from the PLL to the output clocks. Therefore, I am sending a separate
>>> series that focuses on the clocks, but it depends on v4 of the previous series
>>> [1].
>>>
>>> Possible consumers for the clocks are the allegro-dvt video encoder driver or
>>> the Xilinx Video Codec Unit [2] out of tree driver.
>>>
>>> Patch 1 defines the identifiers that shall be used by clock consumers in the
>>> device tree.
>>>
>>> Patch 2 fixes the generic clk-divider to correctly use parents that are passed
>>> via struct clk_hw instead of the clock name.
>>>
>>> Patches 3-6 refactor the existing driver and split the function to configure
>>> the PLL into smaller helper functions.
>>>
>>> Patch 7 registers a fixed rate clock for the PLL. The driver calculated and
>>> set the PLL configuration during probe, and exposing a fixed rate clock for
>>> that rate allows to use the existing configuration with output clocks from the
>>> common clock framework.
>>>
>>> Patches 8-10 switch the driver to the common clock framework and register the
>>> clock provider.
>>>
>>> Patches 11-12 are cleanup patches.
>>>
>>> Michael
>>>
>>> [0] https://lore.kernel.org/linux-arm-kernel/20200619075913.18900-1-m.tretter@pengutronix.de/
>>> [1] https://lore.kernel.org/linux-arm-kernel/20201109134818.4159342-3-m.tretter@pengutronix.de/
>>> [2] https://github.com/Xilinx/vcu-modules
>>>
>>> Michael Tretter (12):
>>> ARM: dts: define indexes for output clocks
>>> clk: divider: fix initialization with parent_hw
>>> soc: xilinx: vcu: drop coreclk from struct xlnx_vcu
>>> soc: xilinx: vcu: add helper to wait for PLL locked
>>> soc: xilinx: vcu: add helpers for configuring PLL
>>> soc: xilinx: vcu: implement PLL disable
>>> soc: xilinx: vcu: register PLL as fixed rate clock
>>> soc: xilinx: vcu: implement clock provider for output clocks
>>> soc: xilinx: vcu: make pll post divider explicit
>>> soc: xilinx: vcu: make the PLL configurable
>>> soc: xilinx: vcu: remove calculation of PLL configuration
>>> soc: xilinx: vcu: use bitfields for register definition
>>>
>>> drivers/clk/clk-divider.c | 9 +-
>>> drivers/soc/xilinx/Kconfig | 2 +-
>>> drivers/soc/xilinx/xlnx_vcu.c | 613 ++++++++++++++++-----------
>>> include/dt-bindings/clock/xlnx-vcu.h | 15 +
>>> 4 files changed, 383 insertions(+), 256 deletions(-)
>>> create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
>>>
>>
>> I can't see any other problem with this series.
>
> Thanks for the review! I will wait a bit longer if there is some review
> feedback by Stephen regarding patch 2, and then send a v2.
Definitely good idea. We are also waiting for his review for others stuff.
Thanks,
Michal
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 01/12] ARM: dts: define indexes for output clocks
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
2020-12-02 14:33 ` Michal Simek
@ 2020-12-07 19:21 ` Rob Herring
2020-12-13 5:44 ` Stephen Boyd
2 siblings, 0 replies; 34+ messages in thread
From: Rob Herring @ 2020-12-07 19:21 UTC (permalink / raw)
To: Michael Tretter
Cc: kernel, mturquette, rajanv, rvisaval, robh+dt, devicetree,
linux-clk, michals, linux-arm-kernel, sboyd, dshah, tejasp
On Mon, 16 Nov 2020 08:55:21 +0100, Michael Tretter wrote:
> The VCU System-Level Control has 4 output clocks. Define indexes for
> these clocks to allow to reference them in the device tree.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> include/dt-bindings/clock/xlnx-vcu.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> create mode 100644 include/dt-bindings/clock/xlnx-vcu.h
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 02/12] clk: divider: fix initialization with parent_hw
2020-11-16 7:55 ` [PATCH 02/12] clk: divider: fix initialization with parent_hw Michael Tretter
2020-12-02 14:28 ` Michal Simek
@ 2020-12-13 5:42 ` Stephen Boyd
1 sibling, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2020-12-13 5:42 UTC (permalink / raw)
To: Michael Tretter, devicetree, linux-arm-kernel, linux-clk
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, Michael Tretter
Quoting Michael Tretter (2020-11-15 23:55:22)
> If a driver registers a divider clock with a parent_hw instead of the
> parent_name, the parent_hw is ignored and the clock does not have a
> parent.
>
> Fix this by initializing the parents the same way they are initialized
> for clock gates.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Any fixes tag?
with the proper fixes tag
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 01/12] ARM: dts: define indexes for output clocks
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
2020-12-02 14:33 ` Michal Simek
2020-12-07 19:21 ` Rob Herring
@ 2020-12-13 5:44 ` Stephen Boyd
2 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2020-12-13 5:44 UTC (permalink / raw)
To: Michael Tretter, devicetree, linux-arm-kernel, linux-clk
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, Michael Tretter
Quoting Michael Tretter (2020-11-15 23:55:21)
> The VCU System-Level Control has 4 output clocks. Define indexes for
> these clocks to allow to reference them in the device tree.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
Acked-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition
2020-11-16 7:55 ` [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition Michael Tretter
@ 2020-12-13 5:47 ` Stephen Boyd
0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2020-12-13 5:47 UTC (permalink / raw)
To: Michael Tretter, devicetree, linux-arm-kernel, linux-clk
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, Michael Tretter
Quoting Michael Tretter (2020-11-15 23:55:32)
> This makes the register accesses more readable and is closer to what is
> usually used in the kernel.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
` (12 preceding siblings ...)
2020-12-03 7:46 ` [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michal Simek
@ 2020-12-13 5:50 ` Stephen Boyd
2020-12-15 11:56 ` Michael Tretter
13 siblings, 1 reply; 34+ messages in thread
From: Stephen Boyd @ 2020-12-13 5:50 UTC (permalink / raw)
To: Michael Tretter, devicetree, linux-arm-kernel, linux-clk
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, Michael Tretter
Quoting Michael Tretter (2020-11-15 23:55:20)
> Hello,
>
> the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> clocks created from the PLL via dividers.
>
> This series reworks the xlnx_vcu driver to use the common clock framework to
> enable other drivers to use the clocks. I originally posted a series to expose
> the output clocks as fixed clocks [0]. This series now implements the full
> tree from the PLL to the output clocks. Therefore, I am sending a separate
> series that focuses on the clocks, but it depends on v4 of the previous series
> [1].
After this series is this anything besides a clk provider? If it's only
providing clks it would make sense to move the driver into drivers/clk/
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
2020-11-16 7:55 ` [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks Michael Tretter
2020-12-02 14:49 ` Michal Simek
@ 2020-12-13 5:55 ` Stephen Boyd
2020-12-15 11:38 ` Michael Tretter
1 sibling, 1 reply; 34+ messages in thread
From: Stephen Boyd @ 2020-12-13 5:55 UTC (permalink / raw)
To: Michael Tretter, devicetree, linux-arm-kernel, linux-clk
Cc: rajanv, tejasp, dshah, rvisaval, michals, kernel, robh+dt,
mturquette, Michael Tretter
Quoting Michael Tretter (2020-11-15 23:55:28)
> diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> index 725e646aa726..cedc8b7859f7 100644
> --- a/drivers/soc/xilinx/xlnx_vcu.c
> +++ b/drivers/soc/xilinx/xlnx_vcu.c
> @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
> return xvcu_pll_enable(xvcu);
> }
>
> +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
> + const char *name,
> + const char * const *parent_names,
> + u8 num_parents,
> + struct clk_hw *parent_default,
> + void __iomem *reg,
> + spinlock_t *lock)
> +{
> + unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
> + u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
Why u8?
> + CLK_DIVIDER_ROUND_CLOSEST;
> + struct clk_hw *mux = NULL;
> + struct clk_hw *divider = NULL;
> + struct clk_hw *gate = NULL;
> + char *name_mux;
> + char *name_div;
> + int err;
> +
> + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> + if (!name_mux) {
> + err = -ENOMEM;
> + goto err;
> + }
> + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> + flags, reg, 0, 1, 0, lock);
> + if (IS_ERR(mux)) {
> + err = PTR_ERR(divider);
> + goto err;
> + }
> + clk_hw_set_parent(mux, parent_default);
Can this be done from assigned-clock-parents binding?
> +
> + name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div");
> + if (!name_div) {
> + err = -ENOMEM;
> + goto err;
> + }
> + divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags,
> + reg, 4, 6, divider_flags,
> + lock);
> + if (IS_ERR(divider)) {
> + err = PTR_ERR(divider);
> + goto err;
> + }
> +
> + gate = clk_hw_register_gate_parent_hw(dev, name, divider,
> + CLK_SET_RATE_PARENT, reg, 12, 0,
> + lock);
> + if (IS_ERR(gate)) {
> + err = PTR_ERR(gate);
> + goto err;
> + }
> +
> + return gate;
> +
> +err:
> + if (!IS_ERR_OR_NULL(gate))
Would be nicer to have some more goto labels and skip the IS_ERR_OR_NULL
checks.
> + clk_hw_unregister_gate(gate);
> + if (!IS_ERR_OR_NULL(divider))
> + clk_hw_unregister_divider(divider);
> + if (!IS_ERR_OR_NULL(mux))
> + clk_hw_unregister_divider(mux);
> +
> + return ERR_PTR(err);
> +}
> +
> +static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> +{
> + struct clk_hw *gate = hw;
> + struct clk_hw *divider;
> + struct clk_hw *mux;
> +
> + if (!gate)
> + return;
> +
> + divider = clk_hw_get_parent(gate);
> + clk_hw_unregister_gate(gate);
> + if (!divider)
> + return;
> +
> + mux = clk_hw_get_parent(divider);
> + clk_hw_unregister_mux(mux);
> + if (!divider)
> + return;
> +
> + clk_hw_unregister_divider(divider);
> +}
> +
> +static DEFINE_SPINLOCK(venc_core_lock);
> +static DEFINE_SPINLOCK(venc_mcu_lock);
Any reason to not allocate these spinlocks too?
> +
> +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> +{
> + struct device *dev = xvcu->dev;
> + const char *parent_names[2];
> + struct clk_hw *parent_default;
> + struct clk_hw_onecell_data *data;
> + struct clk_hw **hws;
> + void __iomem *reg_base = xvcu->vcu_slcr_ba;
> +
> + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + data->num = CLK_XVCU_NUM_CLOCKS;
> + hws = data->hws;
> +
> + xvcu->clk_data = data;
> +
> + parent_default = xvcu->pll;
> + parent_names[0] = "dummy";
What is "dummy"?
> + parent_names[1] = clk_hw_get_name(parent_default);
Can we use the new way of specifying clk parents from DT or by using
direct pointers instead of using string names? The idea is to get rid of
clk_hw_get_name() eventually.
> +
> + hws[CLK_XVCU_ENC_CORE] =
> + xvcu_clk_hw_register_leaf(dev, "venc_core_clk",
> + parent_names,
> + ARRAY_SIZE(parent_names),
> + parent_default,
> + reg_base + VCU_ENC_CORE_CTRL,
> + &venc_core_lock);
> + hws[CLK_XVCU_ENC_MCU] =
> + xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk",
> + parent_names,
> + ARRAY_SIZE(parent_names),
> + parent_default,
> + reg_base + VCU_ENC_MCU_CTRL,
> + &venc_mcu_lock);
> +
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
2020-12-13 5:55 ` Stephen Boyd
@ 2020-12-15 11:38 ` Michael Tretter
2020-12-16 1:09 ` Stephen Boyd
0 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-12-15 11:38 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, linux-arm-kernel, linux-clk, rajanv, tejasp, dshah,
rvisaval, michals, kernel, robh+dt, mturquette
On Sat, 12 Dec 2020 21:55:34 -0800, Stephen Boyd wrote:
> Quoting Michael Tretter (2020-11-15 23:55:28)
> > diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> > index 725e646aa726..cedc8b7859f7 100644
> > --- a/drivers/soc/xilinx/xlnx_vcu.c
> > +++ b/drivers/soc/xilinx/xlnx_vcu.c
> > @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
> > return xvcu_pll_enable(xvcu);
> > }
> >
> > +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
> > + const char *name,
> > + const char * const *parent_names,
> > + u8 num_parents,
> > + struct clk_hw *parent_default,
> > + void __iomem *reg,
> > + spinlock_t *lock)
> > +{
> > + unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
> > + u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
>
> Why u8?
__clk_hw_register_divider expects u8 as divider_flags.
>
> > + CLK_DIVIDER_ROUND_CLOSEST;
> > + struct clk_hw *mux = NULL;
> > + struct clk_hw *divider = NULL;
> > + struct clk_hw *gate = NULL;
> > + char *name_mux;
> > + char *name_div;
> > + int err;
> > +
> > + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> > + if (!name_mux) {
> > + err = -ENOMEM;
> > + goto err;
> > + }
> > + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> > + flags, reg, 0, 1, 0, lock);
> > + if (IS_ERR(mux)) {
> > + err = PTR_ERR(divider);
> > + goto err;
> > + }
> > + clk_hw_set_parent(mux, parent_default);
>
> Can this be done from assigned-clock-parents binding?
Could be done, if the driver provides at least the PLL and the mux in addition
to the actual output clocks. Otherwise, it is not possible to reference the
PLL post divider and the mux from the device tree. I wanted to avoid to expose
the complexity to the device tree. Would you prefer, if all clocks are
provided instead of only the output clocks?
>
> > +
> > + name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div");
> > + if (!name_div) {
> > + err = -ENOMEM;
> > + goto err;
> > + }
> > + divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags,
> > + reg, 4, 6, divider_flags,
> > + lock);
> > + if (IS_ERR(divider)) {
> > + err = PTR_ERR(divider);
> > + goto err;
> > + }
> > +
> > + gate = clk_hw_register_gate_parent_hw(dev, name, divider,
> > + CLK_SET_RATE_PARENT, reg, 12, 0,
> > + lock);
> > + if (IS_ERR(gate)) {
> > + err = PTR_ERR(gate);
> > + goto err;
> > + }
> > +
> > + return gate;
> > +
> > +err:
> > + if (!IS_ERR_OR_NULL(gate))
>
> Would be nicer to have some more goto labels and skip the IS_ERR_OR_NULL
> checks.
Ack.
>
> > + clk_hw_unregister_gate(gate);
> > + if (!IS_ERR_OR_NULL(divider))
> > + clk_hw_unregister_divider(divider);
> > + if (!IS_ERR_OR_NULL(mux))
> > + clk_hw_unregister_divider(mux);
> > +
> > + return ERR_PTR(err);
> > +}
> > +
> > +static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> > +{
> > + struct clk_hw *gate = hw;
> > + struct clk_hw *divider;
> > + struct clk_hw *mux;
> > +
> > + if (!gate)
> > + return;
> > +
> > + divider = clk_hw_get_parent(gate);
> > + clk_hw_unregister_gate(gate);
> > + if (!divider)
> > + return;
> > +
> > + mux = clk_hw_get_parent(divider);
> > + clk_hw_unregister_mux(mux);
> > + if (!divider)
> > + return;
> > +
> > + clk_hw_unregister_divider(divider);
> > +}
> > +
> > +static DEFINE_SPINLOCK(venc_core_lock);
> > +static DEFINE_SPINLOCK(venc_mcu_lock);
>
> Any reason to not allocate these spinlocks too?
I will change this.
>
> > +
> > +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> > +{
> > + struct device *dev = xvcu->dev;
> > + const char *parent_names[2];
> > + struct clk_hw *parent_default;
> > + struct clk_hw_onecell_data *data;
> > + struct clk_hw **hws;
> > + void __iomem *reg_base = xvcu->vcu_slcr_ba;
> > +
> > + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > + data->num = CLK_XVCU_NUM_CLOCKS;
> > + hws = data->hws;
> > +
> > + xvcu->clk_data = data;
> > +
> > + parent_default = xvcu->pll;
> > + parent_names[0] = "dummy";
>
> What is "dummy"?
According to the register reference [0], the output clocks can be driven by an
external clock instead of the PLL, but the VCU Product Guide [1] does not
mention any ports for actually driving the clock. From my understanding of the
IP core, this is a clock mux which has a not-connected first parent. Maybe
someone at Xilinx can clarify, what is happening here.
[0] https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
[1] https://www.xilinx.com/support/documentation-navigation/see-all-versions.html?xlnxproducttypes=IP%20Cores&xlnxipcoresname=v-vcu
What would be a better way to handle this?
>
> > + parent_names[1] = clk_hw_get_name(parent_default);
>
> Can we use the new way of specifying clk parents from DT or by using
> direct pointers instead of using string names? The idea is to get rid of
> clk_hw_get_name() eventually.
It should be possible to use the direct pointers, but this really depends on
how the "dummy" clock is handled.
Thanks,
Michael
>
> > +
> > + hws[CLK_XVCU_ENC_CORE] =
> > + xvcu_clk_hw_register_leaf(dev, "venc_core_clk",
> > + parent_names,
> > + ARRAY_SIZE(parent_names),
> > + parent_default,
> > + reg_base + VCU_ENC_CORE_CTRL,
> > + &venc_core_lock);
> > + hws[CLK_XVCU_ENC_MCU] =
> > + xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk",
> > + parent_names,
> > + ARRAY_SIZE(parent_names),
> > + parent_default,
> > + reg_base + VCU_ENC_MCU_CTRL,
> > + &venc_mcu_lock);
> > +
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-12-13 5:50 ` Stephen Boyd
@ 2020-12-15 11:56 ` Michael Tretter
2020-12-16 2:37 ` Stephen Boyd
0 siblings, 1 reply; 34+ messages in thread
From: Michael Tretter @ 2020-12-15 11:56 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, linux-arm-kernel, linux-clk, rajanv, tejasp, dshah,
rvisaval, michals, kernel, robh+dt, mturquette
On Sat, 12 Dec 2020 21:50:00 -0800, Stephen Boyd wrote:
> Quoting Michael Tretter (2020-11-15 23:55:20)
> > Hello,
> >
> > the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> > clocks created from the PLL via dividers.
> >
> > This series reworks the xlnx_vcu driver to use the common clock framework to
> > enable other drivers to use the clocks. I originally posted a series to expose
> > the output clocks as fixed clocks [0]. This series now implements the full
> > tree from the PLL to the output clocks. Therefore, I am sending a separate
> > series that focuses on the clocks, but it depends on v4 of the previous series
> > [1].
>
> After this series is this anything besides a clk provider? If it's only
> providing clks it would make sense to move the driver into drivers/clk/
>
1. The driver is also responsible for resetting the entire VCU (the
VCU_GASKET_INIT register). This isn't something that an individual encoder or
decoder driver should be doing. However, other clock drivers also implement a
reset controller.
2. There are several registers for AXI performance monitoring in the VCU
System-Level Control register space. Right now, these are not used by the
driver and I have no plans to actually use them, but this might be an argument
against the move.
I think it is OK to move the driver to drivers/clk/, but I don't have a strong
opinion about it.
Michael
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
2020-12-15 11:38 ` Michael Tretter
@ 2020-12-16 1:09 ` Stephen Boyd
2020-12-21 9:18 ` Michael Tretter
0 siblings, 1 reply; 34+ messages in thread
From: Stephen Boyd @ 2020-12-16 1:09 UTC (permalink / raw)
To: Michael Tretter
Cc: devicetree, linux-arm-kernel, linux-clk, rajanv, tejasp, dshah,
rvisaval, michals, kernel, robh+dt, mturquette
Quoting Michael Tretter (2020-12-15 03:38:09)
> On Sat, 12 Dec 2020 21:55:34 -0800, Stephen Boyd wrote:
> > Quoting Michael Tretter (2020-11-15 23:55:28)
> > > diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> > > index 725e646aa726..cedc8b7859f7 100644
> > > --- a/drivers/soc/xilinx/xlnx_vcu.c
> > > +++ b/drivers/soc/xilinx/xlnx_vcu.c
> > > @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
> > > return xvcu_pll_enable(xvcu);
> > > }
> > >
> > > +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
> > > + const char *name,
> > > + const char * const *parent_names,
> > > + u8 num_parents,
> > > + struct clk_hw *parent_default,
> > > + void __iomem *reg,
> > > + spinlock_t *lock)
> > > +{
> > > + unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
> > > + u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
> >
> > Why u8?
>
> __clk_hw_register_divider expects u8 as divider_flags.
Ok.
>
> >
> > > + CLK_DIVIDER_ROUND_CLOSEST;
> > > + struct clk_hw *mux = NULL;
> > > + struct clk_hw *divider = NULL;
> > > + struct clk_hw *gate = NULL;
> > > + char *name_mux;
> > > + char *name_div;
> > > + int err;
> > > +
> > > + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> > > + if (!name_mux) {
> > > + err = -ENOMEM;
> > > + goto err;
> > > + }
> > > + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> > > + flags, reg, 0, 1, 0, lock);
> > > + if (IS_ERR(mux)) {
> > > + err = PTR_ERR(divider);
> > > + goto err;
> > > + }
> > > + clk_hw_set_parent(mux, parent_default);
> >
> > Can this be done from assigned-clock-parents binding?
>
> Could be done, if the driver provides at least the PLL and the mux in addition
> to the actual output clocks. Otherwise, it is not possible to reference the
> PLL post divider and the mux from the device tree. I wanted to avoid to expose
> the complexity to the device tree. Would you prefer, if all clocks are
> provided instead of only the output clocks?
It is fine to do this in software too so not a big deal and no need to
expose it from the device.
>
> >
> > > +
> > > +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> > > +{
> > > + struct device *dev = xvcu->dev;
> > > + const char *parent_names[2];
> > > + struct clk_hw *parent_default;
> > > + struct clk_hw_onecell_data *data;
> > > + struct clk_hw **hws;
> > > + void __iomem *reg_base = xvcu->vcu_slcr_ba;
> > > +
> > > + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> > > + if (!data)
> > > + return -ENOMEM;
> > > + data->num = CLK_XVCU_NUM_CLOCKS;
> > > + hws = data->hws;
> > > +
> > > + xvcu->clk_data = data;
> > > +
> > > + parent_default = xvcu->pll;
> > > + parent_names[0] = "dummy";
> >
> > What is "dummy"?
>
> According to the register reference [0], the output clocks can be driven by an
> external clock instead of the PLL, but the VCU Product Guide [1] does not
> mention any ports for actually driving the clock. From my understanding of the
> IP core, this is a clock mux which has a not-connected first parent. Maybe
> someone at Xilinx can clarify, what is happening here.
>
> [0] https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> [1] https://www.xilinx.com/support/documentation-navigation/see-all-versions.html?xlnxproducttypes=IP%20Cores&xlnxipcoresname=v-vcu
>
> What would be a better way to handle this?
>
> >
> > > + parent_names[1] = clk_hw_get_name(parent_default);
> >
> > Can we use the new way of specifying clk parents from DT or by using
> > direct pointers instead of using string names? The idea is to get rid of
> > clk_hw_get_name() eventually.
>
> It should be possible to use the direct pointers, but this really depends on
> how the "dummy" clock is handled.
>
I think if clk_parent_data is used then we can have the binding list the
external clk as a 'clocks' property and then if it's not present in DT
we will know that it can't ever be a parent. So it hopefully "just
works" if clk_parent_data is used.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-12-15 11:56 ` Michael Tretter
@ 2020-12-16 2:37 ` Stephen Boyd
2020-12-21 9:19 ` Michael Tretter
0 siblings, 1 reply; 34+ messages in thread
From: Stephen Boyd @ 2020-12-16 2:37 UTC (permalink / raw)
To: Michael Tretter
Cc: devicetree, linux-arm-kernel, linux-clk, rajanv, tejasp, dshah,
rvisaval, michals, kernel, robh+dt, mturquette
Quoting Michael Tretter (2020-12-15 03:56:32)
> On Sat, 12 Dec 2020 21:50:00 -0800, Stephen Boyd wrote:
> > Quoting Michael Tretter (2020-11-15 23:55:20)
> > > Hello,
> > >
> > > the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> > > clocks created from the PLL via dividers.
> > >
> > > This series reworks the xlnx_vcu driver to use the common clock framework to
> > > enable other drivers to use the clocks. I originally posted a series to expose
> > > the output clocks as fixed clocks [0]. This series now implements the full
> > > tree from the PLL to the output clocks. Therefore, I am sending a separate
> > > series that focuses on the clocks, but it depends on v4 of the previous series
> > > [1].
> >
> > After this series is this anything besides a clk provider? If it's only
> > providing clks it would make sense to move the driver into drivers/clk/
> >
>
> 1. The driver is also responsible for resetting the entire VCU (the
> VCU_GASKET_INIT register). This isn't something that an individual encoder or
> decoder driver should be doing. However, other clock drivers also implement a
> reset controller.
Right.
>
> 2. There are several registers for AXI performance monitoring in the VCU
> System-Level Control register space. Right now, these are not used by the
> driver and I have no plans to actually use them, but this might be an argument
> against the move.
I suppose if/when that happens we can have a small parent driver that
probes the compatible string and makes two child platform devices, one
for the clk part and one for the PMU? That would let us keep the code in
drivers/clk/ for ease of find-ability. This assumes that the PMU
registers don't overlap with the clk/reset registers. We usually put the
clk and reset controllers together if they use the same registers and
need to make sure the frameworks don't stomp on each other.
>
> I think it is OK to move the driver to drivers/clk/, but I don't have a strong
> opinion about it.
>
Ok. I'm not too strong on it either, but drivers/soc/ is sort of a
dumping ground for random soc things. I'm not looking at it closely but
if the driver is in drivers/clk/ I'd be more inclined to look after the
clk bits.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
2020-12-16 1:09 ` Stephen Boyd
@ 2020-12-21 9:18 ` Michael Tretter
0 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-12-21 9:18 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, dshah, mturquette, tejasp, rajanv, robh+dt, michals,
rvisaval, kernel, linux-clk, linux-arm-kernel
On Tue, 15 Dec 2020 17:09:50 -0800, Stephen Boyd wrote:
> Quoting Michael Tretter (2020-12-15 03:38:09)
> > On Sat, 12 Dec 2020 21:55:34 -0800, Stephen Boyd wrote:
> > > Quoting Michael Tretter (2020-11-15 23:55:28)
> > > > + CLK_DIVIDER_ROUND_CLOSEST;
> > > > + struct clk_hw *mux = NULL;
> > > > + struct clk_hw *divider = NULL;
> > > > + struct clk_hw *gate = NULL;
> > > > + char *name_mux;
> > > > + char *name_div;
> > > > + int err;
> > > > +
> > > > + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> > > > + if (!name_mux) {
> > > > + err = -ENOMEM;
> > > > + goto err;
> > > > + }
> > > > + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> > > > + flags, reg, 0, 1, 0, lock);
> > > > + if (IS_ERR(mux)) {
> > > > + err = PTR_ERR(divider);
> > > > + goto err;
> > > > + }
> > > > + clk_hw_set_parent(mux, parent_default);
> > >
> > > Can this be done from assigned-clock-parents binding?
> >
> > Could be done, if the driver provides at least the PLL and the mux in addition
> > to the actual output clocks. Otherwise, it is not possible to reference the
> > PLL post divider and the mux from the device tree. I wanted to avoid to expose
> > the complexity to the device tree. Would you prefer, if all clocks are
> > provided instead of only the output clocks?
>
> It is fine to do this in software too so not a big deal and no need to
> expose it from the device.
>
> >
> > >
> > > > +
> > > > +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> > > > +{
> > > > + struct device *dev = xvcu->dev;
> > > > + const char *parent_names[2];
> > > > + struct clk_hw *parent_default;
> > > > + struct clk_hw_onecell_data *data;
> > > > + struct clk_hw **hws;
> > > > + void __iomem *reg_base = xvcu->vcu_slcr_ba;
> > > > +
> > > > + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> > > > + if (!data)
> > > > + return -ENOMEM;
> > > > + data->num = CLK_XVCU_NUM_CLOCKS;
> > > > + hws = data->hws;
> > > > +
> > > > + xvcu->clk_data = data;
> > > > +
> > > > + parent_default = xvcu->pll;
> > > > + parent_names[0] = "dummy";
> > >
> > > What is "dummy"?
> >
> > According to the register reference [0], the output clocks can be driven by an
> > external clock instead of the PLL, but the VCU Product Guide [1] does not
> > mention any ports for actually driving the clock. From my understanding of the
> > IP core, this is a clock mux which has a not-connected first parent. Maybe
> > someone at Xilinx can clarify, what is happening here.
> >
> > [0] https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> > [1] https://www.xilinx.com/support/documentation-navigation/see-all-versions.html?xlnxproducttypes=IP%20Cores&xlnxipcoresname=v-vcu
> >
> > What would be a better way to handle this?
> >
> > >
> > > > + parent_names[1] = clk_hw_get_name(parent_default);
> > >
> > > Can we use the new way of specifying clk parents from DT or by using
> > > direct pointers instead of using string names? The idea is to get rid of
> > > clk_hw_get_name() eventually.
> >
> > It should be possible to use the direct pointers, but this really depends on
> > how the "dummy" clock is handled.
> >
>
> I think if clk_parent_data is used then we can have the binding list the
> external clk as a 'clocks' property and then if it's not present in DT
> we will know that it can't ever be a parent. So it hopefully "just
> works" if clk_parent_data is used.
Thanks. It actually just works. I will send v2.
Michael
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider
2020-12-16 2:37 ` Stephen Boyd
@ 2020-12-21 9:19 ` Michael Tretter
0 siblings, 0 replies; 34+ messages in thread
From: Michael Tretter @ 2020-12-21 9:19 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, dshah, mturquette, tejasp, rajanv, robh+dt, michals,
rvisaval, kernel, linux-clk, linux-arm-kernel
On Tue, 15 Dec 2020 18:37:20 -0800, Stephen Boyd wrote:
> Quoting Michael Tretter (2020-12-15 03:56:32)
> > On Sat, 12 Dec 2020 21:50:00 -0800, Stephen Boyd wrote:
> > > Quoting Michael Tretter (2020-11-15 23:55:20)
> > > > Hello,
> > > >
> > > > the xlnx_vcu soc driver is actually a clock provider of a PLL and four output
> > > > clocks created from the PLL via dividers.
> > > >
> > > > This series reworks the xlnx_vcu driver to use the common clock framework to
> > > > enable other drivers to use the clocks. I originally posted a series to expose
> > > > the output clocks as fixed clocks [0]. This series now implements the full
> > > > tree from the PLL to the output clocks. Therefore, I am sending a separate
> > > > series that focuses on the clocks, but it depends on v4 of the previous series
> > > > [1].
> > >
> > > After this series is this anything besides a clk provider? If it's only
> > > providing clks it would make sense to move the driver into drivers/clk/
> > >
> >
> > 1. The driver is also responsible for resetting the entire VCU (the
> > VCU_GASKET_INIT register). This isn't something that an individual encoder or
> > decoder driver should be doing. However, other clock drivers also implement a
> > reset controller.
>
> Right.
>
> >
> > 2. There are several registers for AXI performance monitoring in the VCU
> > System-Level Control register space. Right now, these are not used by the
> > driver and I have no plans to actually use them, but this might be an argument
> > against the move.
>
> I suppose if/when that happens we can have a small parent driver that
> probes the compatible string and makes two child platform devices, one
> for the clk part and one for the PMU? That would let us keep the code in
> drivers/clk/ for ease of find-ability. This assumes that the PMU
> registers don't overlap with the clk/reset registers. We usually put the
> clk and reset controllers together if they use the same registers and
> need to make sure the frameworks don't stomp on each other.
>
> >
> > I think it is OK to move the driver to drivers/clk/, but I don't have a strong
> > opinion about it.
> >
>
> Ok. I'm not too strong on it either, but drivers/soc/ is sort of a
> dumping ground for random soc things. I'm not looking at it closely but
> if the driver is in drivers/clk/ I'd be more inclined to look after the
> clk bits.
OK, I will move the driver to drivers/clk/xilinx/
Michael
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2020-12-21 10:24 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
2020-12-02 14:33 ` Michal Simek
2020-12-07 19:21 ` Rob Herring
2020-12-13 5:44 ` Stephen Boyd
2020-11-16 7:55 ` [PATCH 02/12] clk: divider: fix initialization with parent_hw Michael Tretter
2020-12-02 14:28 ` Michal Simek
2020-12-13 5:42 ` Stephen Boyd
2020-11-16 7:55 ` [PATCH 03/12] soc: xilinx: vcu: drop coreclk from struct xlnx_vcu Michael Tretter
2020-11-16 7:55 ` [PATCH 04/12] soc: xilinx: vcu: add helper to wait for PLL locked Michael Tretter
2020-11-16 7:55 ` [PATCH 05/12] soc: xilinx: vcu: add helpers for configuring PLL Michael Tretter
2020-11-16 7:55 ` [PATCH 06/12] soc: xilinx: vcu: implement PLL disable Michael Tretter
2020-11-16 7:55 ` [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock Michael Tretter
2020-12-02 14:41 ` Michal Simek
2020-11-16 7:55 ` [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks Michael Tretter
2020-12-02 14:49 ` Michal Simek
2020-12-13 5:55 ` Stephen Boyd
2020-12-15 11:38 ` Michael Tretter
2020-12-16 1:09 ` Stephen Boyd
2020-12-21 9:18 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit Michael Tretter
2020-12-02 14:51 ` Michal Simek
2020-11-16 7:55 ` [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable Michael Tretter
2020-12-02 14:54 ` Michal Simek
2020-11-16 7:55 ` [PATCH 11/12] soc: xilinx: vcu: remove calculation of PLL configuration Michael Tretter
2020-11-16 7:55 ` [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition Michael Tretter
2020-12-13 5:47 ` Stephen Boyd
2020-12-03 7:46 ` [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michal Simek
2020-12-03 9:00 ` Michael Tretter
2020-12-03 9:14 ` Michal Simek
2020-12-13 5:50 ` Stephen Boyd
2020-12-15 11:56 ` Michael Tretter
2020-12-16 2:37 ` Stephen Boyd
2020-12-21 9:19 ` Michael Tretter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).