[PATCH v2 3/4] clk: mediatek: Add basic clocks for Mediatek MT8135.
jamesjj.liao at mediatek.com
Mon Dec 22 18:22:28 PST 2014
On Mon, 2014-12-22 at 19:07 +0100, Matthias Brugger wrote:
> 2014-12-04 7:52 GMT+01:00 James Liao <jamesjj.liao at mediatek.com>:
> > + if (clk_data)
> > + clk_data->clks[gate->id] = clk;
> > +
> > + pr_debug("gate %3d: %s\n", i, gate->name);
> Please review the use of pr_debug in this file, there are quite a few
> (if not all) which are not needed.
OK, I'll remove some debug log.
> > + /* PERI1 */
> > + GATE(PERI_USBSLV_CK, usbslv_ck, axi_sel, peri1_cg_regs, 8, 0),
> > + GATE(PERI_USB1_MCU_CK, usb1_mcu_ck, axi_sel, peri1_cg_regs, 7, 0),
> > + GATE(PERI_USB0_MCU_CK, usb0_mcu_ck, axi_sel, peri1_cg_regs, 6, 0),
> > + GATE(PERI_GCPU_CK, gcpu_ck, gcpu_sel, peri1_cg_regs, 5, 0),
> > + GATE(PERI_FHCTL_CK, fhctl_ck, clk26m, peri1_cg_regs, 4, 0),
> > + GATE(PERI_SPI1_CK, spi1_ck, spi_sel, peri1_cg_regs, 3, 0),
> > + GATE(PERI_AUXADC_CK, auxadc_ck, clk26m, peri1_cg_regs, 2, 0),
> > + GATE(PERI_PERI_PWRAP_CK, peri_pwrap_ck, axi_sel, peri1_cg_regs, 1, 0),
> > + GATE(PERI_I2C6_CK, i2c6_ck, axi_sel, peri1_cg_regs, 0, 0),
> > +};
> This clocks look pretty much the same as in mt6589. Can you please
> check the other SoCs (especially mt6589 and mt6592) and put the parts
> of the clock tree which are identical together?
Subsystem clocks (include INFRA and PERI) are similar among Mediatek
SoCs, but not the same. Some clocks may be added or removed, and some
clock bits may be shifted, i.e. a register bit may represent different
clock in different SoCs. So I think it's not a good way to share the
same clock tree implementation.
> > +/*
> > + * device tree support
> > + */
> Please delte this comment, it's not necessary
> > + r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> > + if (r)
> > + pr_err("could not register clock provide\n");
> Please make all these error messages more explanatory. Apart from that
> it should be "provider" instead of "provide", right?
You are right. I'll change the error message to show which provider
> > + clk_data = alloc_clk_data(INFRA_NR_CLK);
> > +
> > + init_clk_infrasys(base, clk_data);
> Why do you put init_clk_gates in a extra function which does nothing else?
In the beginning clock initialization was not invoked from device tree.
But now all of them are init from device tree, so it may be a better way
to remove init_clk_*() and merge the code into this init function. I'll
refine it in next patch.
More information about the linux-arm-kernel