From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Fri, 15 Jan 2016 06:22:02 -0700 Subject: [U-Boot] [RFC PATCH 6/6] clk: add fixed rate clock driver In-Reply-To: References: <1450437315-26333-1-git-send-email-yamada.masahiro@socionext.com> <1450437315-26333-7-git-send-email-yamada.masahiro@socionext.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Masahiro, On 28 December 2015 at 10:08, Masahiro Yamada wrote: > Hi Simon, > > > 2015-12-28 23:20 GMT+09:00 Simon Glass : >> Hi Masahiro, >> >> On 18 December 2015 at 04:15, Masahiro Yamada >> 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 >>> --- >>> >>> 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 >>> +#include >>> +#include >>> + >>> +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