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

[Masahiro Yamada]

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.



>>> +{
>>> +       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.


-- 
Best Regards
Masahiro Yamada