Re: [U-Boot] [PATCH v1 2/7] clk: at91: Improve the clock implementation
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, _dev);
>
> You
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:
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, _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, _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(_dev);
You need to call clk_free(_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
