[U-Boot] [PATCH v2 1/2] clk: clk-uclass: Add post binding for CLK uclass

[Simon Glass]

Hi Wenyou,

On 15 June 2016 at 19:08, Yang, Wenyou <Wenyou.Yang at atmel.com> wrote:
>
>
>> -----Original Message-----
>> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
>> Sent: 2016年6月10日 8:34
>> To: Yang, Wenyou <Wenyou.Yang at atmel.com>
>> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Andreas Bießmann
>> <andreas at biessmann.org>
>> Subject: Re: [PATCH v2 1/2] clk: clk-uclass: Add post binding for CLK uclass
>>
>> Hi Wenyou,
>>
>> On 7 June 2016 at 01:11, Wenyou Yang <wenyou.yang at atmel.com> wrote:
>> > Add post binding support for CLK uclass to recursively bind its
>> > children as clk devices.
>> >
>> > Signed-off-by: Wenyou Yang <wenyou.yang at atmel.com>
>> > ---
>> >
>> > Changes in v2: None
>> >
>> >  drivers/clk/clk-uclass.c | 51
>> > ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 51 insertions(+)
>>
>>
>> Can you please explain what this is for? We would normally call
>> dm_scan_fdt_node() for this, but it seems that you are specifically to create
>> drivers for things with no compatible strings.
>
> Yes, for pheriph32ck node have a lot of child nodes with no compatible strings.
> This patch purpose is to bind these children nodes. Otherwise these nodes will not be bound.
>
> For example,
>
> ---8<----
>         ahb {
>                 apb {
>                         pmc: pmc at f0014000 {
>
>                                 [snip]
>
>                                 periph32ck {
>                                         compatible = "atmel,at91sam9x5-clk-peripheral";
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>                                         clocks = <&h32ck>;
>
>                                         macb0_clk: macb0_clk {
>                                                 #clock-cells = <0>;
>                                                 reg = <5>;
>                                                 atmel,clk-output-range = <0 83000000>;
>                                         };
>
>                                         [snip]
>
>                                         pioA_clk: pioA_clk {
>                                                 #clock-cells = <0>;
>                                                 reg = <18>;
>                                                 atmel,clk-output-range = <0 83000000>;
>                                         };
>
>                                         [snip]
>
>                                         spi0_clk: spi0_clk {
>                                                 #clock-cells = <0>;
>                                                 reg = <33>;
>                                                 atmel,clk-output-range = <0 83000000>;
>                                         };
>
>                                         [snip]
>                                 };
>                         };
>
>                         spi0: spi at f8000000 {
>                                 compatible = "atmel,at91rm9200-spi";
>                                 reg = <0xf8000000 0x100>;
>                                 clocks = <&spi0_clk>;
>                                 clock-names = "spi_clk";
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                                 status = "disabled";
>                         };
>
>                         macb0: ethernet at f8008000 {
>                                 compatible = "cdns,macb";
>                                 reg = <0xf8008000 0x1000>;
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                                 clocks = <&macb0_clk>, <&macb0_clk>;
>                                 clock-names = "hclk", "pclk";
>                                 status = "disabled";
>                         };
>
>                         [snip]
>
>                         pioA: gpio at fc038000 {
>                                 compatible = "atmel,sama5d2-gpio";
>                                 reg = <0xfc038000 0x600>;
>                                 clocks = <&pioA_clk>;
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
>                                 pinctrl {
>                                         compatible = "atmel,sama5d2-pinctrl";
>                                 };
>                         };
>                 };
>         };
>
> ---->8-----
>

In that case I think your clock driver should handle this in its
bind() method. It can scan the device tree and create the devices as
needed. I don't think this feature should go into generic code, at
least not yet.

Also please see Stephen Warren's clock patch, which supports multiple
clock IDs per clock device. Assuming he respins this soon I will be
applying it to dm/master.

http://patchwork.ozlabs.org/patch/625342/

>>
>> >
>> > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index
>> > b483c1e..ee568e4 100644
>> > --- a/drivers/clk/clk-uclass.c
>> > +++ b/drivers/clk/clk-uclass.c
>> > @@ -106,7 +106,58 @@ int clk_get_by_index(struct udevice *dev, int
>> > index, struct udevice **clk_devp)  }  #endif
>> >
>> > +/**
>> > + * clk_post-bind() - post binding for CLK uclass
>> > + * Recursively bind its children as clk devices.
>> > + *
>> > + * @dev: clk device
>> > + * @return: 0 on success, or negative error code on failure  */
>> > +static int clk_post_bind(struct udevice *dev) {
>> > +       const void *fdt = gd->fdt_blob;
>> > +       int offset = dev->of_offset;
>> > +       bool pre_reloc_only = !(gd->flags & GD_FLG_RELOC);
>> > +       const char *name;
>> > +       int ret;
>> > +
>> > +       for (offset = fdt_first_subnode(fdt, offset);
>> > +            offset > 0;
>> > +            offset = fdt_next_subnode(fdt, offset)) {
>> > +               if (pre_reloc_only &&
>> > +                   !fdt_getprop(fdt, offset, "u-boot,dm-pre-reloc", NULL))
>> > +                       continue;
>> > +               /*
>> > +                * If this node has "compatible" property, this is not a clk
>> > +                * node, but a normal device. skip.
>> > +                */
>> > +               fdt_get_property(fdt, offset, "compatible", &ret);
>> > +               if (ret >= 0)
>> > +                       continue;
>> > +
>> > +               if (ret != -FDT_ERR_NOTFOUND)
>> > +                       return ret;
>> > +
>> > +               name = fdt_get_name(fdt, offset, NULL);
>> > +               if (!name)
>> > +                       return -EINVAL;
>> > +
>> > +               ret = device_bind_driver_to_node(dev, "clk", name,
>> > +                                                offset, NULL);
>> > +               if (ret)
>> > +                       return ret;
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>> >  UCLASS_DRIVER(clk) = {
>> >         .id             = UCLASS_CLK,
>> > +       .post_bind      = clk_post_bind,
>> >         .name           = "clk",
>> >  };
>> > +
>> > +U_BOOT_DRIVER(clk_generic) = {
>> > +       .id     = UCLASS_CLK,
>> > +       .name   = "clk",
>> > +};
>> > --
>> > 2.7.4
>> >

Regards,
Simon