[PATCH v2 6/6] clk: mvebu: Add the peripheral clock driver for Armada 3700
Michael Turquette
mturquette at baylibre.com
Tue Jul 12 10:28:17 PDT 2016
Quoting Gregory CLEMENT (2016-07-12 09:30:34)
> Hi Michael,
>
> On ven., juil. 08 2016, Michael Turquette <mturquette at baylibre.com> wrote:
>
> > Quoting Gregory CLEMENT (2016-07-07 15:37:51)
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/clk.h>
> >
> > Same question as my previous email. Is clk.h necessary? Is this driver
> > also a clk consumer?
>
> I think I can remove it indeed.
>
> >
> >> +static int armada_3700_add_composite_clk(const struct clk_periph_data *data,
> >> + const char * const *parent_name,
> >> + void __iomem *reg, spinlock_t *lock,
> >> + struct device *dev, struct clk_hw *hw)
> >> +{
> >> + const struct clk_ops *mux_ops = NULL, *gate_ops = NULL,
> >> + *div_ops = NULL;
> >> + struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *div_hw = NULL;
> >> + const char * const *names;
> >> + struct clk_mux *mux = NULL;
> >> + struct clk_gate *gate = NULL;
> >> + struct clk_divider *div = NULL;
> >> + struct clk_double_div *double_div = NULL;
> >> + int num_parent;
> >> + int ret = 0;
> >> +
> >> + if (data->gate_shift != UNUSED) {
> >> + gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> >> +
> >> + if (!gate)
> >> + return -ENOMEM;
> >> +
> >> + gate->reg = reg + CLK_DIS;
> >> + gate->bit_idx = data->gate_shift;
> >> + gate->lock = lock;
> >> + gate_ops = &clk_gate_ops;
> >> + gate_hw = &gate->hw;
> >> + }
> >> +
> >> + if (data->mux_shift != UNUSED) {
> >> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> >> +
> >> + if (!mux) {
> >> + ret = -ENOMEM;
> >> + goto free_gate;
> >> + }
> >> +
> >> + mux->reg = reg + TBG_SEL;
> >> + mux->shift = data->mux_shift;
> >> + mux->mask = 0x3;
> >> + mux->lock = lock;
> >> + mux_ops = &clk_mux_ro_ops;
> >> + mux_hw = &mux->hw;
> >> + }
> >> +
> >> + if (data->div_reg1 != UNUSED) {
> >> + if (data->div_reg2 == UNUSED) {
> >> + const struct clk_div_table *clkt;
> >> + int table_size = 0;
> >> +
> >> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> >> + if (!div) {
> >> + ret = -ENOMEM;
> >> + goto free_mux;
> >> + }
> >> +
> >> + div->reg = reg + data->div_reg1;
> >> + div->table = data->table;
> >> + for (clkt = div->table; clkt->div; clkt++)
> >> + table_size++;
> >> + div->width = order_base_2(table_size);
> >> + div->lock = lock;
> >> + div_ops = &clk_divider_ro_ops;
> >> + div_hw = &div->hw;
> >> + } else {
> >> + double_div = devm_kzalloc(dev, sizeof(*double_div),
> >> + GFP_KERNEL);
> >> + if (!double_div) {
> >> + ret = -ENOMEM;
> >> + goto free_mux;
> >> + }
> >> +
> >> + double_div->reg1 = reg + data->div_reg1;
> >> + double_div->shift1 = data->div_shift1;
> >> + double_div->reg2 = reg + data->div_reg1;
> >> + double_div->shift2 = data->div_shift2;
> >> + div_ops = &clk_double_div_ops;
> >> + div_hw = &double_div->hw;
> >> + }
> >> + }
> >> +
> >> + switch (data->flags) {
> >> + case XTAL_CHILD:
> >> + /* the xtal clock is the 5th clock */
> >> + names = &parent_name[4];
> >> + num_parent = 1;
> >> + break;
> >> + case TBGA_S_CHILD:
> >> + /* the TBG A S clock is the 3rd clock */
> >> + names = &parent_name[2];
> >> + num_parent = 1;
> >> + break;
> >> + case GBE_CORE_CHILD:
> >> + names = &gbe_name[1];
> >> + num_parent = 1;
> >> + break;
> >> + case GBE_50_CHILD:
> >> + names = &gbe_name[0];
> >> + num_parent = 1;
> >> + break;
> >> + case GBE_125_CHILD:
> >> + names = &gbe_name[2];
> >> + num_parent = 1;
> >> + break;
> >> + default:
> >> + names = parent_name;
> >> + num_parent = 4;
> >> + }
> >> + hw = clk_hw_register_composite(dev, data->name,
> >> + names, num_parent,
> >> + mux_hw, mux_ops,
> >> + div_hw, div_ops,
> >> + gate_hw, gate_ops,
> >> + CLK_IGNORE_UNUSED);
> >> + if (IS_ERR(hw)) {
> >> + ret = PTR_ERR(hw);
> >> + goto free_div;
> >> + }
> >> +
> >> + return 0;
> >> +free_div:
> >> + devm_kfree(dev, div);
> >> + devm_kfree(dev, double_div);
> >> +free_mux:
> >> + devm_kfree(dev, mux);
> >> +free_gate:
> >> + devm_kfree(dev, gate);
> >> + return ret;
> >> +}
> >
> > Can this "add" function (aka registration function) be replaced with
> > static data instead? I think that all of the static data exists already,
> > this function can be removed and your probe can call clk_hw_register
> > directly.
> >
>
> I see your point and indeed we can remove some allocation. However using
> clk_hw_register with composite clock is not straight forward. Indeed the
> clk_ops is filled in the clk_hw_register_composite function and none of
> these operations are exported outside the clk-composite.c file.
>
> We can't directly point a clk_gate_ops structure or a clk_divider_ops as
> you did in the drivers/clk/meson/gxbb.c file for example. For clk
> composite it would need modify the framework to either export all the
> operation or to create set of operations directly usable and I would
> like to avoid doing it when we are close to the merge window.
>
> However I can use static data for the clk_mux, clk_gate and clk_divider
> struct.
>
> Is it OK for you?
Yes, that sounds good. Let's convert the ones we can for now. Some day
we might have a better solution for the composite clock stuff.
Regards,
Mike
>
> Thanks,
>
> Gregory
>
> > It might need a macro though, since composite clock structures are
> > rather messy. This avoids a lot of unnecessary allocations and time
> > populating data that we already have access to. In general I am trying
> > to encourage clk drivers to use only clk_hw_register() in their probe
> > instead of the helper registration functions.
> >
> > Similarly I am discouraging drivers from populating hw.init at run-time,
> > since we already have that data for that at compile-time.
> >
> > Regards,
> > Mike
>
> --
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
More information about the linux-arm-kernel
mailing list