| Message ID | 20201116075532.4019252-9-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [01/12] ARM: dts: define indexes for output clocks | expand |
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
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); > +
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); > > + >
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.
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
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);
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(-)