Hi Simon,
2015-12-28 23:20 GMT+09:00 Simon Glass <s...@chromium.org>:
> Hi Masahiro,
>
> On 18 December 2015 at 04:15, Masahiro Yamada
> <yamada.masah...@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.
>>
>> 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.masah...@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...
>> +#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.
>> +
>> +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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
