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

[Stephen Warren]

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:

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.