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