Hi Masahiro,
On 28 December 2015 at 10:08, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
2015-12-28 23:20 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 18 December 2015 at 04:15, Masahiro Yamada yamada.masahiro@socionext.com 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 yamada.masahiro@socionext.com
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