[v3] clk: shmobile: Add R8A7740-specific clock support

Message ID	1400682086-5705-1-git-send-email-ulrich.hecht+renesas@gmail.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <linux-sh-owner@kernel.org> From: Ulrich Hecht <ulrich.hecht@gmail.com> To: linux-sh@vger.kernel.org Cc: laurent.pinchart@ideasonboard.com, horms@verge.net.au, magnus.damm@gmail.com, linux-arm-kernel@lists.infradead.org, Mike Turquette <mturquette@linaro.org>, Ulrich Hecht <ulrich.hecht+renesas@gmail.com> Subject: [PATCH v3] clk: shmobile: Add R8A7740-specific clock support Date: Wed, 21 May 2014 16:21:26 +0200 Message-Id: <1400682086-5705-1-git-send-email-ulrich.hecht+renesas@gmail.com> Sender: linux-sh-owner@vger.kernel.org Precedence: bulk

Ulrich Hecht May 21, 2014, 2:21 p.m. UTC

Driver for the R8A7740's clocks that are too specific to be supported by a
generic driver.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---

Changes since v2:
- added missing clock extal2
- removed redundant divider mask

Changes since v1:
- reshuffled to include DT bindings and Makefile changes


 .../bindings/clock/renesas,r8a7740-cpg-clocks.txt  |  39 +++++
 drivers/clk/shmobile/Makefile                      |   1 +
 drivers/clk/shmobile/clk-r8a7740.c                 | 195 +++++++++++++++++++++
 3 files changed, 235 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,r8a7740-cpg-clocks.txt
 create mode 100644 drivers/clk/shmobile/clk-r8a7740.c

Geert Uytterhoeven May 21, 2014, 2:31 p.m. UTC | #1

On Wed, May 21, 2014 at 4:21 PM, Ulrich Hecht <ulrich.hecht@gmail.com> wrote:
> Driver for the R8A7740's clocks that are too specific to be supported by a
> generic driver.

Thanks!

> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart May 21, 2014, 3:41 p.m. UTC | #2

Hi Ulrich,

Thank you for the patch.

On Wednesday 21 May 2014 16:21:26 Ulrich Hecht wrote:
> Driver for the R8A7740's clocks that are too specific to be supported by a
> generic driver.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

The implementation looks globally sane to me. There's still quite a few 
missing clocks, but there's no hurry in adding support for them at the moment. 
I'd like to get the bindings reviewed by someone outside of our team, but that 
would require making the CPG documentation (or at least the block diagram) 
available. Magnus, is there a chance for that to happen ?

Please see below for a few other comments. 

> ---
> 
> Changes since v2:
> - added missing clock extal2
> - removed redundant divider mask
> 
> Changes since v1:
> - reshuffled to include DT bindings and Makefile changes
> 
> 
>  .../bindings/clock/renesas,r8a7740-cpg-clocks.txt  |  39 +++++
>  drivers/clk/shmobile/Makefile                      |   1 +
>  drivers/clk/shmobile/clk-r8a7740.c                 | 195 ++++++++++++++++++
>  3 files changed, 235 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/clock/renesas,r8a7740-cpg-clocks.txt
> create mode 100644 drivers/clk/shmobile/clk-r8a7740.c
> 
> diff --git
> a/Documentation/devicetree/bindings/clock/renesas,r8a7740-cpg-clocks.txt
> b/Documentation/devicetree/bindings/clock/renesas,r8a7740-cpg-clocks.txt
> new file mode 100644
> index 0000000..8432e00
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,r8a7740-cpg-clocks.txt
> @@ -0,0 +1,39 @@
> +* Renesas R8A7740  Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R8A7740 SoC. It includes three PLLs
> +and several fixed ratio and variable ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be "renesas,r8a7740-cpg-clocks"
> +
> +  - reg: Base address and length of the memory resource used by the CPG
> +
> +  - clocks: Reference to the three parent clocks
> +  - #clock-cells: Must be 1
> +  - clock-output-names: The names of the clocks. Supported clocks are
> +    "system", "pllc0", "pllc1", "pllc2", "r", "usb24s", "i", "zg", "b",
> +    "m1", "hp", "hpp", "usbp", "s", "zb", "m3", and "cp".

I think I would have called the system clock "sys", but that's probably just 
me :-)

> +
> +  - renesas,mode: board-specific settings of the MD_CK* bits

Would it make sense to document the bits here ?

> +
> +
> +Example
> +-------
> +
> +cpg_clocks: cpg_clocks@e6150000 {
> +        compatible = "renesas,r8a7740-cpg-clocks";
> +        reg = <0xe6150000 0x10000>;

This range covers the MSTP registers. Unlike other Renesas SoCs with CCF 
support in mainline, the MSTP registers are in the middle of the CPG address 
space here, which is quite problematic.

One straightforward solution would be to split the reg property to two ranges 
(0xe6150000 to 0xe615002f and 0xe6150068 to 0xe61500e3). That's pretty ugly 
though. Another solution, which might be more elegant, would be to keep the 
range unchanged and instantiate the MSTP DT nodes as children of the 
cpg_clocks node.

> +        clocks = <&extal1_clk>, <&extal2_clk>, <&extalr_clk>;
> +        #clock-cells = <1>;
> +        clock-output-names = "system", "pllc0", "pllc1",
> +                             "pllc2", "r",
> +                             "usb24s",
> +                             "i", "zg", "b", "m1", "hp",
> +                             "hpp", "usbp", "s", "zb", "m3",
> +                             "cp";
> +};
> +
> +&cpg_clocks {
> +	renesas,mode = <0x05>;
> +};
> diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> index 5404cb9..20388e8 100644
> --- a/drivers/clk/shmobile/Makefile
> +++ b/drivers/clk/shmobile/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
>  obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o
> +obj-$(CONFIG_ARCH_R8A7740)		+= clk-r8a7740.o
>  obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o
>  obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
> diff --git a/drivers/clk/shmobile/clk-r8a7740.c
> b/drivers/clk/shmobile/clk-r8a7740.c new file mode 100644
> index 0000000..189560f
> --- /dev/null
> +++ b/drivers/clk/shmobile/clk-r8a7740.c
> @@ -0,0 +1,195 @@
> +/*
> + * r8a7740 Core CPG Clocks
> + *
> + * Copyright (C) 2014  Ulrich Hecht
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk/shmobile.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/spinlock.h>
> +
> +struct r8a7740_cpg {
> +	struct clk_onecell_data data;
> +	spinlock_t lock;
> +	void __iomem *reg;
> +};
> +
> +#define CPG_FRQCRA	0
> +#define CPG_FRQCRB	4

For consistency with the other registers below I would write the addresses 
0x00 and 0x04.

> +#define CPG_PLLC2CR	0x2c
> +#define CPG_USBCKCR	0x8c
> +#define CPG_FRQCRC	0xe0
> +
> +#define CLK_ENABLE_ON_INIT BIT(0)
> +
> +struct div4_clk {
> +	const char *name;
> +	unsigned int reg;
> +	unsigned int shift;
> +	int flags;
> +};
> +
> +static struct div4_clk div4_clks[] = {
> +	{ "i", CPG_FRQCRA, 20, CLK_ENABLE_ON_INIT },
> +	{ "zg", CPG_FRQCRA, 16, CLK_ENABLE_ON_INIT },
> +	{ "b", CPG_FRQCRA,  8, CLK_ENABLE_ON_INIT },
> +	{ "m1", CPG_FRQCRA,  4, CLK_ENABLE_ON_INIT },
> +	{ "hp", CPG_FRQCRB,  4, 0 },
> +	{ "hpp", CPG_FRQCRC, 20, 0 },
> +	{ "usbp", CPG_FRQCRC, 16, 0 },
> +	{ "s", CPG_FRQCRC, 12, 0 },
> +	{ "zb", CPG_FRQCRC,  8, 0 },
> +	{ "m3", CPG_FRQCRC,  4, 0 },
> +	{ "cp", CPG_FRQCRC,  0, 0 },
> +	{ NULL, 0, 0, 0, 0 },
> +};
> +
> +static const struct clk_div_table div4_div_table[] = {
> +	{ 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, { 4, 8 }, { 5, 12 },
> +	{ 6, 16 }, { 7, 18 }, { 8, 24 }, { 9, 32 }, { 10, 36 }, { 11, 48 },
> +	{ 13, 72 }, { 14, 96 }, { 0, 0 }
> +};
> +
> +static u32 cpg_mode __initdata;
> +
> +static struct clk * __init
> +r8a7740_cpg_register_clock(struct device_node *np, struct r8a7740_cpg *cpg,
> +			     const char *name)
> +{
> +	const struct clk_div_table *table = NULL;
> +	const char *parent_name;
> +	unsigned int shift, reg;
> +	unsigned int mult = 1;
> +	unsigned int div = 1;
> +
> +	if (!strcmp(name, "r")) {
> +		switch (cpg_mode & (BIT(2) | BIT(1))) {
> +		case BIT(1) | BIT(2):
> +			parent_name = of_clk_get_parent_name(np, 0);
> +			div = 2048;
> +			break;
> +		case BIT(2):
> +			parent_name = of_clk_get_parent_name(np, 0);
> +			div = 1024;
> +			break;
> +		default:
> +			parent_name = of_clk_get_parent_name(np, 1);

Shouldn't this be parent number 2 instead of number 1 ?

> +			break;
> +		}
> +	} else if (!strcmp(name, "system")) {
> +		parent_name = of_clk_get_parent_name(np, 0);
> +		if (cpg_mode & BIT(1))
> +			div = 2;
> +	} else if (!strcmp(name, "pllc0")) {
> +		/* PLLC0/1 are configurable multiplier clocks. Register them as
> +		 * fixed factor clocks for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		u32 value = clk_readl(cpg->reg + CPG_FRQCRC);
> +		parent_name = "system";
> +		mult = ((value >> 24) & 0x7f) + 1;
> +	} else if (!strcmp(name, "pllc1")) {
> +		u32 value = clk_readl(cpg->reg + CPG_FRQCRA);
> +		parent_name = "system";
> +		mult = ((value >> 24) & 0x7f) + 1;
> +		div = 2;
> +	} else if (!strcmp(name, "pllc2")) {
> +		u32 value = clk_readl(cpg->reg + CPG_PLLC2CR);
> +		parent_name = "system";
> +		mult = ((value >> 24) & 0x3f) + 1;
> +	} else if (!strcmp(name, "usb24s")) {
> +		u32 value = clk_readl(cpg->reg + CPG_USBCKCR);
> +		if (value & BIT(7))
> +			parent_name = "extal2";

Shouldn't you use of_clk_get_parent_name(np, 1) instead of hardcoding "extal2" 
here ?

> +		else
> +			parent_name = "system";
> +		if (!(value & BIT(6)))
> +			div = 2;
> +	} else {
> +		struct div4_clk *c;
> +		for (c = div4_clks; c->name; c++) {
> +			if (!strcmp(name, c->name)) {
> +				parent_name = "pllc1";
> +				table = div4_div_table;
> +				reg = c->reg;
> +				shift = c->shift;
> +				break;
> +			}
> +		}
> +		if (!c->name)
> +			return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (!table) {
> +		return clk_register_fixed_factor(NULL, name, parent_name, 0,
> +						 mult, div);
> +	} else {
> +		return clk_register_divider_table(NULL, name, parent_name, 0,
> +						  cpg->reg + reg, shift, 4, 0,
> +						  table, &cpg->lock);
> +	}
> +}
> +
> +static void __init r8a7740_cpg_clocks_init(struct device_node *np)
> +{
> +	struct r8a7740_cpg *cpg;
> +	struct clk **clks;
> +	unsigned int i;
> +	int num_clks;
> +
> +	if (of_property_read_u32(np, "renesas,mode", &cpg_mode))
> +		pr_warn("%s: missing renesas,mode property\n", __func__);
> +
> +	num_clks = of_property_count_strings(np, "clock-output-names");
> +	if (num_clks < 0) {
> +		pr_err("%s: failed to count clocks\n", __func__);
> +		return;
> +	}
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +	clks = kzalloc(num_clks * sizeof(*clks), GFP_KERNEL);
> +	if (cpg == NULL || clks == NULL) {
> +		/* We're leaking memory on purpose, there's no point in cleaning
> +		 * up as the system won't boot anyway.
> +		 */
> +		return;
> +	}
> +
> +	spin_lock_init(&cpg->lock);
> +
> +	cpg->data.clks = clks;
> +	cpg->data.clk_num = num_clks;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN_ON(cpg->reg == NULL))
> +		return;
> +
> +	for (i = 0; i < num_clks; ++i) {
> +		const char *name;
> +		struct clk *clk;
> +
> +		of_property_read_string_index(np, "clock-output-names", i,
> +					      &name);
> +
> +		clk = r8a7740_cpg_register_clock(np, cpg, name);
> +		if (IS_ERR(clk))
> +			pr_err("%s: failed to register %s %s clock (%ld)\n",
> +			       __func__, np->name, name, PTR_ERR(clk));
> +		else
> +			cpg->data.clks[i] = clk;
> +	}
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +}
> +CLK_OF_DECLARE(r8a7740_cpg_clks, "renesas,r8a7740-cpg-clocks",
> +	       r8a7740_cpg_clocks_init);

Geert Uytterhoeven May 21, 2014, 3:50 p.m. UTC | #3

Hi Laurent,

On Wed, May 21, 2014 at 5:41 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> +cpg_clocks: cpg_clocks@e6150000 {
>> +        compatible = "renesas,r8a7740-cpg-clocks";
>> +        reg = <0xe6150000 0x10000>;
>
> This range covers the MSTP registers. Unlike other Renesas SoCs with CCF
> support in mainline, the MSTP registers are in the middle of the CPG address
> space here, which is quite problematic.

This is also the case for R-Car Gen2?!?

                /* Special CPG clocks */
                cpg_clocks: cpg_clocks@e6150000 {
                        compatible = "renesas,r8a7791-cpg-clocks",
                                     "renesas,rcar-gen2-cpg-clocks";
                        reg = <0 0xe6150000 0 0x1000>;

                mstp0_clks: mstp0_clks@e6150130 {
                        compatible = "renesas,r8a7791-mstp-clocks",
"renesas,cpg-mstp-clocks";
                        reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>;

Or perhaps I am misunderstanding what you mean?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm May 22, 2014, 12:37 a.m. UTC | #4

Hi Laurent,

On Thu, May 22, 2014 at 12:41 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 21 May 2014 16:21:26 Ulrich Hecht wrote:
>> Driver for the R8A7740's clocks that are too specific to be supported by a
>> generic driver.
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>
> The implementation looks globally sane to me. There's still quite a few
> missing clocks, but there's no hurry in adding support for them at the moment.
> I'd like to get the bindings reviewed by someone outside of our team, but that
> would require making the CPG documentation (or at least the block diagram)
> available. Magnus, is there a chance for that to happen ?

Regarding documentation, as much as I'd like to see this, in practice
it feels highly unlikely since I'm not in control of the actual data
sheet distribution policy. I believe Emma Mobile series data sheet are
available for public download, but the rest of the SoCs are not
unfortunately.

This issue with closed documentation is not specific to r8a7740
though, so earlier developed CCF implementations included in upstream
like r8a7790, r8a7791, r7s72100 and r8a7779 are in the same state as
r8a7740.

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart May 22, 2014, 10:22 a.m. UTC | #5

Hi Magnus,

On Thursday 22 May 2014 09:37:40 Magnus Damm wrote:
> On Thu, May 22, 2014 at 12:41 AM, Laurent Pinchart wrote:
> > On Wednesday 21 May 2014 16:21:26 Ulrich Hecht wrote:
> >> Driver for the R8A7740's clocks that are too specific to be supported by
> >> a generic driver.
> >> 
> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> > 
> > The implementation looks globally sane to me. There's still quite a few
> > missing clocks, but there's no hurry in adding support for them at the
> > moment. I'd like to get the bindings reviewed by someone outside of our
> > team, but that would require making the CPG documentation (or at least
> > the block diagram) available. Magnus, is there a chance for that to
> > happen ?
> 
> Regarding documentation, as much as I'd like to see this, in practice it
> feels highly unlikely since I'm not in control of the actual data sheet
> distribution policy. I believe Emma Mobile series data sheet are available
> for public download, but the rest of the SoCs are not unfortunately.
> 
> This issue with closed documentation is not specific to r8a7740 though, so
> earlier developed CCF implementations included in upstream like r8a7790,
> r8a7791, r7s72100 and r8a7779 are in the same state as r8a7740.

Yes they are, although the hardware is simpler in those cases, so there's less 
potential issues. I don't have a specific concern here, just a feeling of 
uneasiness coming from publishing DT bindings that can't be properly reviewed 
by someone out of our team. We're too familiar with the hardware to take a 
step back and see the bindings from an external point of view, which is why 
I'd like an external review before committing to any DT binding stability.

There's of course no reason to delay this patch (well, except for all the 
other small comments I've made :-)), but if it were me I would mark the 
corresponding bindings with a big "EXPERIMENTAL" warning.

Magnus Damm May 22, 2014, 11:16 a.m. UTC | #6

Hi Laurent,

On Thu, May 22, 2014 at 7:22 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Thursday 22 May 2014 09:37:40 Magnus Damm wrote:
>> On Thu, May 22, 2014 at 12:41 AM, Laurent Pinchart wrote:
>> > On Wednesday 21 May 2014 16:21:26 Ulrich Hecht wrote:
>> >> Driver for the R8A7740's clocks that are too specific to be supported by
>> >> a generic driver.
>> >>
>> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> >
>> > The implementation looks globally sane to me. There's still quite a few
>> > missing clocks, but there's no hurry in adding support for them at the
>> > moment. I'd like to get the bindings reviewed by someone outside of our
>> > team, but that would require making the CPG documentation (or at least
>> > the block diagram) available. Magnus, is there a chance for that to
>> > happen ?
>>
>> Regarding documentation, as much as I'd like to see this, in practice it
>> feels highly unlikely since I'm not in control of the actual data sheet
>> distribution policy. I believe Emma Mobile series data sheet are available
>> for public download, but the rest of the SoCs are not unfortunately.
>>
>> This issue with closed documentation is not specific to r8a7740 though, so
>> earlier developed CCF implementations included in upstream like r8a7790,
>> r8a7791, r7s72100 and r8a7779 are in the same state as r8a7740.
>
> Yes they are, although the hardware is simpler in those cases, so there's less
> potential issues. I don't have a specific concern here, just a feeling of
> uneasiness coming from publishing DT bindings that can't be properly reviewed
> by someone out of our team. We're too familiar with the hardware to take a
> step back and see the bindings from an external point of view, which is why
> I'd like an external review before committing to any DT binding stability.

I understand that you cannot vouch for the stability of this binding. =)

And I agree that external review would help in making it stabler quicker.

> There's of course no reason to delay this patch (well, except for all the
> other small comments I've made :-)), but if it were me I would mark the
> corresponding bindings with a big "EXPERIMENTAL" warning.

We can chose to treat it as relatively experimental if we want to. So
your warning is fine!

How long stabilization time would you recommend for this kind of
thing? I would guess half a year?

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart May 22, 2014, 10:42 p.m. UTC | #7

Hi Geert,

On Wednesday 21 May 2014 17:50:35 Geert Uytterhoeven wrote:
> On Wed, May 21, 2014 at 5:41 PM, Laurent Pinchart wrote:
> >> +cpg_clocks: cpg_clocks@e6150000 {
> >> +        compatible = "renesas,r8a7740-cpg-clocks";
> >> +        reg = <0xe6150000 0x10000>;
> > 
> > This range covers the MSTP registers. Unlike other Renesas SoCs with CCF
> > support in mainline, the MSTP registers are in the middle of the CPG
> > address space here, which is quite problematic.
> 
> This is also the case for R-Car Gen2?!?
> 
>                 /* Special CPG clocks */
>                 cpg_clocks: cpg_clocks@e6150000 {
>                         compatible = "renesas,r8a7791-cpg-clocks",
>                                      "renesas,rcar-gen2-cpg-clocks";
>                         reg = <0 0xe6150000 0 0x1000>;
> 
>                 mstp0_clks: mstp0_clks@e6150130 {
>                         compatible = "renesas,r8a7791-mstp-clocks",
> "renesas,cpg-mstp-clocks";
>                         reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>;

Indeed :-/ It looks like we're just lucky that of_iomap() doesn't request the 
memory resource.

Do you think it would make sense to move the MSTP nodes inside the CPG node to 
better describe the hardware, and avoid future breakages if of_iomap() starts 
calling request_resource() tomorrow ?

> Or perhaps I am misunderstanding what you mean?

Laurent Pinchart May 22, 2014, 11:18 p.m. UTC | #8

Hi Magnus,

On Thursday 22 May 2014 20:16:06 Magnus Damm wrote:
> On Thu, May 22, 2014 at 7:22 PM, Laurent Pinchart wrote:
> > On Thursday 22 May 2014 09:37:40 Magnus Damm wrote:
> >> On Thu, May 22, 2014 at 12:41 AM, Laurent Pinchart wrote:
> >> > On Wednesday 21 May 2014 16:21:26 Ulrich Hecht wrote:
> >> >> Driver for the R8A7740's clocks that are too specific to be supported
> >> >> by a generic driver.
> >> >> 
> >> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> >> > 
> >> > The implementation looks globally sane to me. There's still quite a few
> >> > missing clocks, but there's no hurry in adding support for them at the
> >> > moment. I'd like to get the bindings reviewed by someone outside of our
> >> > team, but that would require making the CPG documentation (or at least
> >> > the block diagram) available. Magnus, is there a chance for that to
> >> > happen ?
> >> 
> >> Regarding documentation, as much as I'd like to see this, in practice it
> >> feels highly unlikely since I'm not in control of the actual data sheet
> >> distribution policy. I believe Emma Mobile series data sheet are
> >> available for public download, but the rest of the SoCs are not
> >> unfortunately.
> >> 
> >> This issue with closed documentation is not specific to r8a7740 though,
> >> so earlier developed CCF implementations included in upstream like
> >> r8a7790, r8a7791, r7s72100 and r8a7779 are in the same state as r8a7740.
> > 
> > Yes they are, although the hardware is simpler in those cases, so there's
> > less potential issues. I don't have a specific concern here, just a
> > feeling of uneasiness coming from publishing DT bindings that can't be
> > properly reviewed by someone out of our team. We're too familiar with the
> > hardware to take a step back and see the bindings from an external point
> > of view, which is why I'd like an external review before committing to
> > any DT binding stability.
>
> I understand that you cannot vouch for the stability of this binding. =)
> 
> And I agree that external review would help in making it stabler quicker.
> 
> > There's of course no reason to delay this patch (well, except for all the
> > other small comments I've made :-)), but if it were me I would mark the
> > corresponding bindings with a big "EXPERIMENTAL" warning.
> 
> We can chose to treat it as relatively experimental if we want to. So
> your warning is fine!
> 
> How long stabilization time would you recommend for this kind of
> thing? I would guess half a year?

I see several main reasons why DT bindings need time to stabilize.

- When implementing support for currently unsupported features of the device 
we could realize that the bindings were designed in a way that makes support 
of new features difficult in a backward-compatible way.

- When a new SoC comes out we could realize that device features or SoC 
integration parameters that we thought were set in stone are actually not, and 
don't play well with the current bindings.

- When other vendors implement bindings for similar devices we could realize 
that common needs would benefit from common bindings.

At least the last reason advocates for experimental DT bindings being 
committed to the mainline kernel, to make it possible for developers to 
identify common needs between vendors.

In this specific case I'm mostly concerned about the first reason, as I don't 
expect our new SoCs to come out with a CPG nearly-but-not-quite-identical to 
the r8a7740's. The stabilization time would thus depend entirely on our effort 
to implement support for the currently missing features. Integration with MSTP 
is the main one (the overlapping register ranges problem I've mentioned before 
is a good example, even if it doesn't cause a real problem today), and support 
for at least some of the missing clocks would be good as well. If we actively 
maintain this driver I expect that one or two kernel releases should be enough 
for the bindings to stabilize.

Please feel free to also discuss the other reasons I've mentioned above and 
how you would like to see them being handled.

Geert Uytterhoeven May 23, 2014, 6:47 a.m. UTC | #9

Hi Laurent,

On Fri, May 23, 2014 at 12:42 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 21 May 2014 17:50:35 Geert Uytterhoeven wrote:
>> On Wed, May 21, 2014 at 5:41 PM, Laurent Pinchart wrote:
>> >> +cpg_clocks: cpg_clocks@e6150000 {
>> >> +        compatible = "renesas,r8a7740-cpg-clocks";
>> >> +        reg = <0xe6150000 0x10000>;
>> >
>> > This range covers the MSTP registers. Unlike other Renesas SoCs with CCF
>> > support in mainline, the MSTP registers are in the middle of the CPG
>> > address space here, which is quite problematic.
>>
>> This is also the case for R-Car Gen2?!?
>>
>>                 /* Special CPG clocks */
>>                 cpg_clocks: cpg_clocks@e6150000 {
>>                         compatible = "renesas,r8a7791-cpg-clocks",
>>                                      "renesas,rcar-gen2-cpg-clocks";
>>                         reg = <0 0xe6150000 0 0x1000>;
>>
>>                 mstp0_clks: mstp0_clks@e6150130 {
>>                         compatible = "renesas,r8a7791-mstp-clocks",
>> "renesas,cpg-mstp-clocks";
>>                         reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>;
>
> Indeed :-/ It looks like we're just lucky that of_iomap() doesn't request the
> memory resource.
>
> Do you think it would make sense to move the MSTP nodes inside the CPG node to
> better describe the hardware, and avoid future breakages if of_iomap() starts
> calling request_resource() tomorrow ?

That indeed makes sense.

In general (not limited to r8a7740), I think we should move a few more nodes.
Currently we have almost all device nodes at the top level, while they should be
under a bus node. Or in a tree of bus nodes, matching the block diagram
in the datasheet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus TorvaldsH
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm May 26, 2014, 4:30 a.m. UTC | #10

HI Laurent,

On Fri, May 23, 2014 at 8:18 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Thursday 22 May 2014 20:16:06 Magnus Damm wrote:
>> On Thu, May 22, 2014 at 7:22 PM, Laurent Pinchart wrote:
>> > On Thursday 22 May 2014 09:37:40 Magnus Damm wrote:
>> >> On Thu, May 22, 2014 at 12:41 AM, Laurent Pinchart wrote:
>> >> > On Wednesday 21 May 2014 16:21:26 Ulrich Hecht wrote:
>> >> >> Driver for the R8A7740's clocks that are too specific to be supported
>> >> >> by a generic driver.
>> >> >>
>> >> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> >> >
>> >> > The implementation looks globally sane to me. There's still quite a few
>> >> > missing clocks, but there's no hurry in adding support for them at the
>> >> > moment. I'd like to get the bindings reviewed by someone outside of our
>> >> > team, but that would require making the CPG documentation (or at least
>> >> > the block diagram) available. Magnus, is there a chance for that to
>> >> > happen ?
>> >>
>> >> Regarding documentation, as much as I'd like to see this, in practice it
>> >> feels highly unlikely since I'm not in control of the actual data sheet
>> >> distribution policy. I believe Emma Mobile series data sheet are
>> >> available for public download, but the rest of the SoCs are not
>> >> unfortunately.
>> >>
>> >> This issue with closed documentation is not specific to r8a7740 though,
>> >> so earlier developed CCF implementations included in upstream like
>> >> r8a7790, r8a7791, r7s72100 and r8a7779 are in the same state as r8a7740.
>> >
>> > Yes they are, although the hardware is simpler in those cases, so there's
>> > less potential issues. I don't have a specific concern here, just a
>> > feeling of uneasiness coming from publishing DT bindings that can't be
>> > properly reviewed by someone out of our team. We're too familiar with the
>> > hardware to take a step back and see the bindings from an external point
>> > of view, which is why I'd like an external review before committing to
>> > any DT binding stability.
>>
>> I understand that you cannot vouch for the stability of this binding. =)
>>
>> And I agree that external review would help in making it stabler quicker.
>>
>> > There's of course no reason to delay this patch (well, except for all the
>> > other small comments I've made :-)), but if it were me I would mark the
>> > corresponding bindings with a big "EXPERIMENTAL" warning.
>>
>> We can chose to treat it as relatively experimental if we want to. So
>> your warning is fine!
>>
>> How long stabilization time would you recommend for this kind of
>> thing? I would guess half a year?
>
> I see several main reasons why DT bindings need time to stabilize.

Thanks for listing these. I agree that we need time to make the bindings stable.

> - When implementing support for currently unsupported features of the device
> we could realize that the bindings were designed in a way that makes support
> of new features difficult in a backward-compatible way.

Right, I can see how it is possible to end up in that situation by not
supporting all device hardware features in the driver from the
beginning and just "enabling" DT support. It does however seem to me
that in such case the DT binding must have been written to match the
driver and not the hardware. We should try not to. =)

So perhaps we would need more time to develop the initial DT binding
for the particular device version? Of course if this happens then we
need to fix it - no doubt about that - but exactly how to fix it needs
to be discussed and handled case-by-case IMO. Having plenty of
stabilization time (and users) will help here I think.

> - When a new SoC comes out we could realize that device features or SoC
> integration parameters that we thought were set in stone are actually not, and
> don't play well with the current bindings.

So if the documentation gets a surprise update then we should of
course update the software too. Depending on how long time the DT
binding has been in use I believe we have different options how to
handle backwards compatibility. We can ignore the DT stability
altogether or make sure to keep compatibility for some time. Exactly
what to do depends on the timing, number of users and difficulty of
maintaining compatibility and our users in the particular product
segment. Stabilization time should help here too.

> - When other vendors implement bindings for similar devices we could realize
> that common needs would benefit from common bindings.

This one is a bit different IMO.

> At least the last reason advocates for experimental DT bindings being
> committed to the mainline kernel, to make it possible for developers to
> identify common needs between vendors.

While I agree that vendors should submit their DT bindings upstream as
first step I would not necessarily use the word "experimental" for
such DT bindings. Instead I would argue that it is natural for the
vendors to cover only their own hardware to begin with. I would also
say that it is pretty normal for the kernel to over time come up with
better ways to describe the hardware.

My opinion is that it is seldom right to break DT compatibility just
because a slightly nicer common binding has been designed. Instead I
think it makes more sense to use the new-and-improved DT style for
newer SoCs and keep compatibility for older SoCs. Basically I believe
we should handle different SoC generations and their devices with
different style of bindings and keep on moving forward to better and
better ways to describe the hardware.

I don't think stabilization time would help in this case.

> In this specific case I'm mostly concerned about the first reason, as I don't
> expect our new SoCs to come out with a CPG nearly-but-not-quite-identical to
> the r8a7740's. The stabilization time would thus depend entirely on our effort
> to implement support for the currently missing features. Integration with MSTP
> is the main one (the overlapping register ranges problem I've mentioned before
> is a good example, even if it doesn't cause a real problem today), and support
> for at least some of the missing clocks would be good as well. If we actively
> maintain this driver I expect that one or two kernel releases should be enough
> for the bindings to stabilize.

I agree about your suggestion about r8a7740 stabilization time.
Regarding the particular case with MSTP my opinion is that we must
have discussed this nesting option when we designed the MSTP DT
bindings in the first place. Of course, new information may have come
up so we need to adjust. I do however think that it must make sense to
handle r8a7740 and newer SoCs in the new DT binding format but refrain
from breaking existing MSTP DT bindings for already supported SoCs.

> Please feel free to also discuss the other reasons I've mentioned above and
> how you would like to see them being handled.

As you can tell, I don't think there is any one-way-fits-all for DT
stability. In the end I think this all is about keeping a good balance
between making a stable enough kernel for our users while also not
collecting too much compatibility cruft. One way to handle this is to
separate the timing for introduction and removal of DT bindings and
give users time to move over.

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v3] clk: shmobile: Add R8A7740-specific clock support

Commit Message

Comments

Patch