[U-Boot] [PATCH v1 2/7] clk: at91: Improve the clock implementation

[Wenyou.Yang at microchip.com]

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:swarren at wwwdotorg.org]
> Sent: 2016年9月14日 3:34
> To: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Stephen Warren
> <swarren at 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 at 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 at 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