[PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks

[Stephen Boyd]

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