[U-Boot] [RESEND PATCH 4/5] clk: add device tree support for clock framework

[Simon Glass]

Hi Masahiro,

On 27 December 2015 at 21:23, Simon Glass <sjg at chromium.org> wrote:
> Hi Masahiro,
>
> On 22 December 2015 at 03:04, Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
>> Add device tree binding support for the clock uclass.  This allows
>> clock consumers to get the peripheral ID based on the "clocks"
>> property in the device tree.
>>
>> Usage:
>> Assume the following device tree:
>>
>>   clk: myclock {
>>           compatible = "myclocktype";
>>           #clock-cells = <1>;
>>   };
>>
>>   uart {
>>           compatible = "myuart";
>>           clocks = <&clk 3>;
>>   };
>>
>>   i2c {
>>           compatible = "myi2c";
>>           clocks = <&clk 5>;
>>   };
>>
>> The UART, I2C driver can get the peripheral ID 3, 5, respectively
>> by calling fdt_clk_get().  The clock provider should set its get_id
>> callback to clk_get_id_simple.  This should be enough for most cases
>> although more complicated DT-PeripheralID translation would be
>> possible by a specific get_id callback.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>> ---
>>
>>  drivers/clk/Makefile  |  1 +
>>  drivers/clk/clk-fdt.c | 37 +++++++++++++++++++++++++++++++++++++
>
> I think clk_fdt.c is better since we mostly avoid hyphens except for the uclass.
>
>>  include/clk.h         | 20 ++++++++++++++++++++
>>  3 files changed, 58 insertions(+)
>>  create mode 100644 drivers/clk/clk-fdt.c
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 4a6a4a8..5fcdf39 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -6,6 +6,7 @@
>>  #
>>
>>  obj-$(CONFIG_CLK) += clk-uclass.o
>> +obj-$(CONFIG_$(SPL_)OF_CONTROL) += clk-fdt.o
>>  obj-$(CONFIG_ROCKCHIP_RK3036) += clk_rk3036.o
>>  obj-$(CONFIG_ROCKCHIP_RK3288) += clk_rk3288.o
>>  obj-$(CONFIG_SANDBOX) += clk_sandbox.o
>> diff --git a/drivers/clk/clk-fdt.c b/drivers/clk/clk-fdt.c
>> new file mode 100644
>> index 0000000..fc53157
>> --- /dev/null
>> +++ b/drivers/clk/clk-fdt.c
>> @@ -0,0 +1,37 @@
>> +/*
>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro at socionext.com>
>> + *
>> + * Device Tree support for clk uclass
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <clk.h>
>> +#include <dm/uclass.h>
>> +#include <fdtdec.h>
>> +
>> +int fdt_clk_get(const void *fdt, int nodeoffset, int index,
>> +               struct udevice **dev)
>
> I think this should work using a device rather than a node offset.
> I've pushed a working tree to u-boot-dm/rockchip-working to show what
> I mean.
>
> Also BTW I implemented your full pinctrl for rockchip in that tree -
> seems to work well! The only problem is that init is quite slow. It
> might be the phandle lookups, I'm not sure.
>
>> +{
>> +       struct fdtdec_phandle_args clkspec;
>> +       struct udevice *clkdev;
>> +       int rc;
>> +
>> +       rc = fdtdec_parse_phandle_with_args(fdt, nodeoffset, "clocks",
>> +                                           "#clock-cells", 0, index, &clkspec);
>> +       if (rc)
>> +               return rc;
>> +
>> +       rc = uclass_get_device_by_of_offset(UCLASS_CLK, clkspec.node, &clkdev);
>> +       if (rc)
>> +               return rc;
>> +
>> +       rc = clk_get_id(clkdev, clkspec.args_count, clkspec.args);
>> +       if (rc < 0)
>> +               return rc;
>> +
>> +       if (dev)
>> +               *dev = clkdev;
>> +
>> +       return rc;
>> +}
>> diff --git a/include/clk.h b/include/clk.h
>> index 1efbaf2..518cb47 100644
>> --- a/include/clk.h
>> +++ b/include/clk.h
>> @@ -121,4 +121,24 @@ static inline int clk_get_id_simple(struct udevice *dev, int args_count,
>>         return args_count > 0 ? args[0] : 0;
>>  }
>>
>> +#if CONFIG_IS_ENABLED(OF_CONTROL)
>> +/**
>> + * fdt_clk_get() - Get peripheral ID from device tree
>> + *
>> + * @fdt:       FDT blob
>> + * @periph:    Offset of clock consumer node
>> + * @index:     index of a phandle to parse out in "clocks" property
>> + * @dev:       if not NULL, filled with pointer of clock provider
>> + * @return peripheral ID, or -ve error code
>> + */
>> +int fdt_clk_get(const void *fdt, int nodeoffset, int index,
>> +               struct udevice **dev);
>> +#else
>> +static inline int fdt_clk_get(const void *fdt, int nodeoffset, int index,
>> +                             struct udevice **dev);

With a bit more thought, I think the concerns I have are mostly
cosmetic. This function should be passed a struct udevice rather than
fdt and nodeoffset, since it can get them itself. Also I'm not keen on
the fdt_ prefix. Other than that it is similar to the
clk_get_by_index() that I was fiddling with for rockchip. I think it
is better to return the periph_id as a return value (as you have)
rather than a pointer arg (as I did).

>> +{
>> +       return -ENOSYS;
>> +}
>> +#endif
>> +
>>  #endif /* _CLK_H_ */
>> --
>> 1.9.1
>>
>
> Regards,
> Simon

Regards,
Simon