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

[Simon Glass]

Hi Masahiro,

On 28 December 2015 at 10:06, Masahiro Yamada
<yamada.masahiro at socionext.com> wrote:
> Hi Simon,
>
>
> 2015-12-28 23:20 GMT+09:00 Simon Glass <sjg at chromium.org>:
>
>>>>  drivers/clk/clk-fdt.c | 37 +++++++++++++++++++++++++++++++++++++
>>>
>>> I think clk_fdt.c is better since we mostly avoid hyphens except for the uclass.
>
> OK.
>
>
>>>>  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.
>
> Looks good to me.
>
>
>
>>> Also BTW I implemented your full pinctrl for rockchip in that tree -
>>> seems to work well!
>
> That's good to hear!
>
>>> The only problem is that init is quite slow. It
>>> might be the phandle lookups, I'm not sure.
>
> I did not realize the slowness, at least on my board.
> I enable D-cache on both SPL and U-Boot proper.

Yes, and Rockchip should do the same, really. But I think it is worth
digging into. I end up with quite a lot of pinconfig devices (maybe
50). I'll see if I can figure it out in the next few weeks.

>
>
>
>>>> +{
>>>> +       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).
>
>
> My main motivation is to use device trees to get clocks, so I do not mind
> cosmetic stuff here.
>
> Yours is simpler and works for me too.
> So,  shall we replace 03 and 04 with the one in your rockchip branch?
> (with modification of the return value)
>
> If you like, please submit the patch.  I am glad to test it.

OK I did that, but we need to sort out the return value. See my email
on the patch.

Regards,
Simon