Hi Stephen, Thank you for your review, and detailed comments.
The version 2 has been sent out, please give your advices. > -----Original Message----- > From: Stephen Warren [mailto:swar...@wwwdotorg.org] > Sent: 2016年9月14日 3:34 > To: Wenyou Yang - A41535 <wenyou.y...@microchip.com> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stephen Warren > <swar...@nvidia.com> > Subject: Re: [U-Boot] [PATCH v1 2/7] clk: at91: Improve the clock > implementation > > On 09/12/2016 07:54 PM, Wenyou Yang wrote: > > For the peripheral clock, provide the clock ops for the clock > > provider, such as spi0_clk. The .of_xlate is to get the clk->id, the > > .enable is to enable the spi0 peripheral clock, the .get_rate is to > > get the clock frequency. > > > > The driver for periph32ck node is responsible for recursively binding > > its children as clk devices, not provide the clock ops. > > > > So do the generated clock and system clock. > > > diff --git a/drivers/clk/at91/clk-generated.c > > b/drivers/clk/at91/clk-generated.c > > > +/** > > + * generated_clk_bind() - for the generated clock driver > > + * Recursively bind its children as clk devices. > > + * > > + * @return: 0 on success, or negative error code on failure */ > > +static int generated_clk_bind(struct udevice *dev) > ... (code to enumerate/instantiate all child DT nodes) > > +} > > + > > +static const struct udevice_id generated_clk_match[] = { > > + { .compatible = "atmel,sama5d2-clk-generated" }, > > + {} > > +}; > > + > > +U_BOOT_DRIVER(generated_clk) = { > > + .name = "generated-clk", > > + .id = UCLASS_CLK, > > + .of_match = generated_clk_match, > > + .bind = generated_clk_bind, > > +}; > > This driver shouldn't be UCLASS_CLK, and can't be without any clock ops > implemented. It appears to be for another DT node that solely exists to house > various sub-nodes which are the actual clock providers. I believe you should > make this UCLASS_MISC. I guess you need to keep the custom bind function, > since all the child nodes that it enumerates explicitly don't have a > compatible value, > so you can't rely on e.g. the default UCLASS_SIMPLE_BUS bind function to > enumerate them. > > BTW, while reading ./arch/arm/dts/sama5d2.dtsi, I noticed: > > sdmmc0_gclk: sdmmc0_gclk { > #clock-cells = <0>; > reg = <31>; > }; > > Doesn't dtc complain about that? The node has a reg property, yet there is no > unit > address in the node name to match it. Instead, that node should be: Yes, there are dtc complains like this, "Warning (unit_address_vs_reg): Node /ahb/apb/pmc@f0014000/periph64ck/sdmmc0_hclk has a reg or ranges property, but no unit name" I will send a patches to fix this warning. Others are accepted too. > > sdmmc0_gclk: sdmmc0_gclk@31 { > #clock-cells = <0>; > reg = <31>; > }; > > > +static int generic_clk_of_xlate(struct clk *clk, > > + struct fdtdec_phandle_args *args) > > { > > - struct pmc_platdata *plat = dev_get_platdata(clk->dev); > > + int periph; > > The following should likely be added here: > > if (args->args_count) { > debug("Invaild args_count: %d\n", args->args_count); > return -EINVAL; > } > > > + periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1); > > + if (periph < 0) > > + return -EINVAL; > > + > > + clk->id = periph; > > I guess that's OK. of_xlate is really intended to parse/process the clocks > property > in the client DT node. However, I suppose parsing the referenced DT node is > probably OK too. Has this Atmel clock binding, and a similar clock driver > implementation (including custom of_xlate) been reviewed for inclusion in the > Linux kernel? I'd be curious if the same technique was/wasn't allowed there. > > > +static int generic_clk_probe(struct udevice *dev) > ... > > + dev = dev_get_parent(dev); > > + dev = dev_get_parent(dev); > > That line is duplicated. > > > diff --git a/drivers/clk/at91/clk-peripheral.c > > b/drivers/clk/at91/clk-peripheral.c > > The same comments as above all apply to this file too. > > > +static ulong periph_get_rate(struct clk *clk) > > { > > + struct udevice *dev; > > + struct clk clk_dev; > > + int ret; > > > > + dev = dev_get_parent(clk->dev); > > + ret = clk_request(dev, &clk_dev); > > You haven't filled in clk_dev.id here. That needs to be filled in before > calling > clk_request(). > > > + if (ret) > > + return ret; > > + > > + ret = clk_get_by_index(dev, 0, &clk_dev); > > This over-writes everything in clk_dev, thus making the call to > clk_request() above pointless. > > > + if (ret) > > + return ret; > > + > > + return clk_get_rate(&clk_dev); > > You need to call clk_free(&clk_dev) before returning. This comment applies to > generated_clk_get_rate()/generic_clk_get_rate() too (in the previous file), > although that problem existed before this patch. > > > +static int periph_clk_probe(struct udevice *dev) { > > + struct periph_clk_platdata *plat = dev_get_platdata(dev); > > + > > + dev = dev_get_parent(dev); > > + dev = dev_get_parent(dev); > > + plat->reg_base = (struct at91_pmc *)dev_get_addr_ptr(dev); > > It would be helpful documentation-wise to use separate variables for those > parents; > make the variable name indicate what the parent is expected to be: > > dev_periph_container = dev_get_parent(dev); dev_pmc = > dev_get_parent(dev_periph_container); > > > diff --git a/drivers/clk/at91/clk-system.c > > b/drivers/clk/at91/clk-system.c > > Likewise, all the same comments above apply to this file too. > > > +/** > > + * at91_system_clk_bind() - for the system clock driver > > + * Recursively bind its children as clk devices. > > + * > > + * @return: 0 on success, or negative error code on failure */ > > +static int at91_system_clk_bind(struct udevice *dev) { > ... > > + /* > > + * If this node has "compatible" property, this is not > > + * a pin configuration node, but a normal device. skip. > > + */ > > This is nothing to do with pin configuration, but clocks. > > This function is duplicated at least 3 times in this patch. It might be > useful to > implement it in some common location, and just call it from all the different > clock > drivers. You'd need to pass in a string for the driver name to bind to, but > that's the > only difference I see. > > I wouldn't be surprised if other parts of the Atmel clock drivers could be > shared too; > of_xlate is probably common, and perhaps get_rate for some clocks that are > derived from parent clocks. Still, cleaning that up might be a job for a > separate > series. Best Regards, Wenyou Yang _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
