Hi Masahiro, On 28 December 2015 at 10:08, Masahiro Yamada <[email protected]> wrote: > Hi Simon, > > > 2015-12-28 23:20 GMT+09:00 Simon Glass <[email protected]>: >> Hi Masahiro, >> >> On 18 December 2015 at 04:15, Masahiro Yamada >> <[email protected]> wrote: >>> This commit intends to implement "fixed-clock" as in Linux. >>> (drivers/clk/clk-fixed-rate.c in Linux) >>> >>> If you need a very simple clock to just provide fixed clock rate >>> like a crystal oscillator, you do not have to write a new driver. >>> This driver can support it. >>> >>> Note: >>> As you see in dts/ directories, fixed clocks are often collected in >>> one container node like this: >>> >>> clocks { >>> refclk_a: refclk_a { >>> #clock-cells = <0>; >>> compatible = "fixed-clock"; >>> clock-frequency = <10000000>; >>> }; >>> >>> refclk_b: refclk_b { >>> #clock-cells = <0>; >>> compatible = "fixed-clock"; >>> clock-frequency = <20000000>; >>> }; >>> }; >>> >>> This does not work in the current DM of U-Boot, unfortunately. >>> The "clocks" node must have 'compatible = "simple-bus";' or something >>> to bind children. >> >> I suppose we could explicitly probe the children of the 'clocks' node >> somewhere. What do you suggest? > > Sounds reasonable. > > Or, postpone this until somebody urges to implement it.
I haven't picked this patch up but would like to. Are you able to respin it? Sorry if you were waiting on me. Sounds good. > > >>> >>> Most of developers would be unhappy about adding such a compatible >>> string only in U-Boot because we generally want to use the same set >>> of device trees beyond projects. >> >> I'm not sure we need to change it, but if we did, we could try to >> upstream the change. > > As you suggest, no change is needed in the core part. > > >>> >>> Signed-off-by: Masahiro Yamada <[email protected]> >>> --- >>> >>> I do not understand why we need both .get_rate and .get_periph_rate. >>> >>> I set both in this driver, but I am not sure if I am doing right. >> >> This is to avoid needing a new clock device for every single clock >> divider in the SoC. For example, it is common to have a PLL be used by >> 20-30 devices. In U-Boot we can encode the device number as a >> peripheral ID, Then we can adjust those dividers by settings the >> clock's rate on a per-peripheral basis. Thus we need only one clock >> device instead of 20-30. > > I understand this. The peripheral ID is useful. > (I think this is similar to the of_clk_provider in Linux) > If so, we can only use .get_periph_rate(), and drop .get_rate(). > >> In the case of your clock I think you could return -ENOSYS for >> get_periph_rate(). > > > This is "#clock-cells = <0>" case. > > Maybe, can we use .get_periph_rate() with index == 0 > for .get_rate() ? > > I am not sure if I am understanding correctly... The idea is that the peripheral clock is a child of the main clock (e.g. with a clock enable and a divider). So get_rate(SOME_CLOCK) get_periph_rate(SOME_CLOCK, SOME_PERIPH) They are different clocks, but we avoid needing a device for every peripheral that uses the source clock. > > > >>> +#include <common.h> >>> +#include <clk.h> >>> +#include <dm/device.h> >>> + >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> +struct clk_fixed_rate { >> >> Perhaps have a _priv suffix so it is clear that this is device-private data? >> >>> + unsigned long fixed_rate; >>> +}; >>> + >>> +#define to_clk_fixed_rate(dev) ((struct clk_fixed_rate *)dev_get_priv(dev)) >> >> Should this be to_priv()? > > OK, they are local in the file, so name space conflict would never happen. > > I took these names from drivers/clk/clk-fixed-rate.c in Linux, though. OK, it's just different from how it is normally done in U-Boot. I think consistency is best. But it's a minor issue, I'll leave it up to you. > > > >>> + >>> +static ulong clk_fixed_get_rate(struct udevice *dev) >>> +{ >>> + return to_clk_fixed_rate(dev)->fixed_rate; >>> +} >>> + >>> +static ulong clk_fixed_get_periph_rate(struct udevice *dev, int ignored) >> >> Don't need this. >> >>> +{ >>> + return to_clk_fixed_rate(dev)->fixed_rate; >>> +} >>> + >>> +const struct clk_ops clk_fixed_rate_ops = { >>> + .get_rate = clk_fixed_get_rate, >>> + .get_periph_rate = clk_fixed_get_periph_rate, >>> + .get_id = clk_get_id_simple, >>> +}; >>> + >>> +static int clk_fixed_rate_probe(struct udevice *dev) >> >> Could be in the ofdata_to_platdata() method if you like. > > > Right. > >>> +{ >>> + to_clk_fixed_rate(dev)->fixed_rate = >>> + fdtdec_get_int(gd->fdt_blob, dev->of_offset, >>> + "clock-frequency", 0); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct udevice_id clk_fixed_rate_match[] = { >>> + { >>> + .compatible = "fixed-clock", >>> + }, >>> + { /* sentinel */ } >>> +}; >>> + >>> +U_BOOT_DRIVER(clk_fixed_rate) = { >>> + .name = "Fixed Rate Clock", >> >> Current we avoid spaces in the names - how about "fixed_rate_clock"? > > OK. > > > -- > Best Regards > Masahiro Yamada Regards. Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
