[3/5] ARM: dts: r7s72100: add rtc clock to device tree

Message ID 20170316175112.27913-4-chris.brandt@renesas.com
State Superseded
Headers show

Commit Message

Chris Brandt March 16, 2017, 5:51 p.m.
Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s72100.dtsi            | 9 +++++++++
 include/dt-bindings/clock/r7s72100-clock.h | 3 +++
 2 files changed, 12 insertions(+)

Comments

Geert Uytterhoeven March 17, 2017, 8:27 a.m. | #1
On Thu, Mar 16, 2017 at 6:51 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -117,6 +117,15 @@
>                         clock-output-names = "ostm0", "ostm1";
>                 };
>
> +               mstp6_clks: mstp6_clks@fcfe042c {
> +                       #clock-cells = <1>;
> +                       compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
> +                       reg = <0xfcfe042c 4>;
> +                       clocks = <&p0_clk>;
> +                       clock-indices = <R7S72100_CLK_RTC>;
> +                       clock-output-names = "rtc0";

"rtc"? There's only one.

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
Chris Brandt March 17, 2017, 1:20 p.m. | #2
Hi Geert,

On Friday, March 17, 2017, Geert Uytterhoeven wrote:
> > --- a/arch/arm/boot/dts/r7s72100.dtsi
> > +++ b/arch/arm/boot/dts/r7s72100.dtsi
> > @@ -117,6 +117,15 @@
> >                         clock-output-names = "ostm0", "ostm1";
> >                 };
> >
> > +               mstp6_clks: mstp6_clks@fcfe042c {
> > +                       #clock-cells = <1>;
> > +                       compatible = "renesas,r7s72100-mstp-clocks",
> "renesas,cpg-mstp-clocks";
> > +                       reg = <0xfcfe042c 4>;
> > +                       clocks = <&p0_clk>;
> > +                       clock-indices = <R7S72100_CLK_RTC>;
> > +                       clock-output-names = "rtc0";
> 
> "rtc"? There's only one.

The rtc-sh.c code wants to have a number at the end. So if I just put "rtc" as the clock name, it does not find it. Again, I didn't want to break any SH builds, so I just changed the DT to match the driver.


[ from the rtc-sh.c code ]

	clk_id = pdev->id;
	/* With a single device, the clock id is still "rtc0" */
	if (clk_id < 0)
		clk_id = 0;

	snprintf(clk_name, sizeof(clk_name), "rtc%d", clk_id);

	rtc->clk = devm_clk_get(&pdev->dev, clk_name);
	if (IS_ERR(rtc->clk)) {
		/*
		 * No error handling for rtc->clk intentionally, not all
		 * platforms will have a unique clock for the RTC, and
		 * the clk API can handle the struct clk pointer being
		 * NULL.
		 */
		rtc->clk = NULL;
	}

	clk_enable(rtc->clk);


Cheers

Chris
Geert Uytterhoeven March 17, 2017, 1:33 p.m. | #3
Hi Chris,

On Fri, Mar 17, 2017 at 2:20 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, March 17, 2017, Geert Uytterhoeven wrote:
>> > --- a/arch/arm/boot/dts/r7s72100.dtsi
>> > +++ b/arch/arm/boot/dts/r7s72100.dtsi
>> > @@ -117,6 +117,15 @@
>> >                         clock-output-names = "ostm0", "ostm1";
>> >                 };
>> >
>> > +               mstp6_clks: mstp6_clks@fcfe042c {
>> > +                       #clock-cells = <1>;
>> > +                       compatible = "renesas,r7s72100-mstp-clocks",
>> "renesas,cpg-mstp-clocks";
>> > +                       reg = <0xfcfe042c 4>;
>> > +                       clocks = <&p0_clk>;
>> > +                       clock-indices = <R7S72100_CLK_RTC>;
>> > +                       clock-output-names = "rtc0";
>>
>> "rtc"? There's only one.
>
> The rtc-sh.c code wants to have a number at the end. So if I just put "rtc" as the clock name, it does not find it. Again, I didn't want to break any SH builds, so I just changed the DT to match the driver.

Hmm...

> [ from the rtc-sh.c code ]
>
>         clk_id = pdev->id;
>         /* With a single device, the clock id is still "rtc0" */
>         if (clk_id < 0)
>                 clk_id = 0;
>
>         snprintf(clk_name, sizeof(clk_name), "rtc%d", clk_id);
>
>         rtc->clk = devm_clk_get(&pdev->dev, clk_name);

So in the absence of an "rtc0" clock in the device node (you don't have
"clock-names" properties in the rtc devvice node yet), it will fall back to
clk_get_sys(), and will find the global "rtc0" clock. Unless you call it
"rtc"...

Most drivers using a single clock just pass NULL instead of a name, so it
will match the first clock found.

I think the simplest solution is to check if your device is instantiated
from DT (pdev->dev.of_node != NULL), and pass NULL (or "fck", when you add
multiple clocks to the DT bindings) to devm_clk_get() if that's the case.

>         if (IS_ERR(rtc->clk)) {
>                 /*
>                  * No error handling for rtc->clk intentionally, not all
>                  * platforms will have a unique clock for the RTC, and
>                  * the clk API can handle the struct clk pointer being
>                  * NULL.
>                  */
>                 rtc->clk = NULL;
>         }

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
Chris Brandt March 17, 2017, 1:48 p.m. | #4
Hi Geert,

On Friday, March 17, 2017, Geert Uytterhoeven wrote:
> So in the absence of an "rtc0" clock in the device node (you don't have
> "clock-names" properties in the rtc devvice node yet), it will fall back
> to clk_get_sys(), and will find the global "rtc0" clock. Unless you call
> it "rtc"...
> 
> Most drivers using a single clock just pass NULL instead of a name, so it
> will match the first clock found.
> 
> I think the simplest solution is to check if your device is instantiated
> from DT (pdev->dev.of_node != NULL), and pass NULL (or "fck", when you add
> multiple clocks to the DT bindings) to devm_clk_get() if that's the case.


OK, so leave current SuperH code the way it is, but add the case for
(pdev->dev.of_node != NULL) and do things the 'new way' so I can have
an RTC clock name of "rtc".

Question: Is your idea to add all the clocks (X1, X3, EXTAL), but put the
first one as the peripheral clock (the one that runs the register interface 'p0')?


Chris
Geert Uytterhoeven March 17, 2017, 1:57 p.m. | #5
Hi Chris,

On Fri, Mar 17, 2017 at 2:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, March 17, 2017, Geert Uytterhoeven wrote:
>> So in the absence of an "rtc0" clock in the device node (you don't have
>> "clock-names" properties in the rtc devvice node yet), it will fall back
>> to clk_get_sys(), and will find the global "rtc0" clock. Unless you call
>> it "rtc"...
>>
>> Most drivers using a single clock just pass NULL instead of a name, so it
>> will match the first clock found.
>>
>> I think the simplest solution is to check if your device is instantiated
>> from DT (pdev->dev.of_node != NULL), and pass NULL (or "fck", when you add
>> multiple clocks to the DT bindings) to devm_clk_get() if that's the case.
>
> OK, so leave current SuperH code the way it is, but add the case for
> (pdev->dev.of_node != NULL) and do things the 'new way' so I can have
> an RTC clock name of "rtc".

Indeed.

> Question: Is your idea to add all the clocks (X1, X3, EXTAL), but put the
> first one as the peripheral clock (the one that runs the register interface 'p0')?

Which one is the peripheral clock doesn't matter if you use clock-names.
The convention is to use "fck" for the functional clock of the device.

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
Alexandre Belloni March 17, 2017, 3:16 p.m. | #6
On 16/03/2017 at 13:51:10 -0400, Chris Brandt wrote:

You should always include a commit message.

> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  arch/arm/boot/dts/r7s72100.dtsi            | 9 +++++++++
>  include/dt-bindings/clock/r7s72100-clock.h | 3 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
> index ed62e19..2a42a79 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -117,6 +117,15 @@
>  			clock-output-names = "ostm0", "ostm1";
>  		};
>  
> +		mstp6_clks: mstp6_clks@fcfe042c {
> +			#clock-cells = <1>;
> +			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
> +			reg = <0xfcfe042c 4>;
> +			clocks = <&p0_clk>;
> +			clock-indices = <R7S72100_CLK_RTC>;
> +			clock-output-names = "rtc0";
> +		};
> +
>  		mstp7_clks: mstp7_clks@fcfe0430 {
>  			#clock-cells = <1>;
>  			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
> diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h
> index cd2ed51..bc256d3 100644
> --- a/include/dt-bindings/clock/r7s72100-clock.h
> +++ b/include/dt-bindings/clock/r7s72100-clock.h
> @@ -29,6 +29,9 @@
>  #define R7S72100_CLK_OSTM0	1
>  #define R7S72100_CLK_OSTM1	0
>  
> +/* MSTP6 */
> +#define R7S72100_CLK_RTC	0
> +
>  /* MSTP7 */
>  #define R7S72100_CLK_ETHER	4
>  
> -- 
> 2.10.1
> 
>

Patch

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index ed62e19..2a42a79 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -117,6 +117,15 @@ 
 			clock-output-names = "ostm0", "ostm1";
 		};
 
+		mstp6_clks: mstp6_clks@fcfe042c {
+			#clock-cells = <1>;
+			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
+			reg = <0xfcfe042c 4>;
+			clocks = <&p0_clk>;
+			clock-indices = <R7S72100_CLK_RTC>;
+			clock-output-names = "rtc0";
+		};
+
 		mstp7_clks: mstp7_clks@fcfe0430 {
 			#clock-cells = <1>;
 			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h
index cd2ed51..bc256d3 100644
--- a/include/dt-bindings/clock/r7s72100-clock.h
+++ b/include/dt-bindings/clock/r7s72100-clock.h
@@ -29,6 +29,9 @@ 
 #define R7S72100_CLK_OSTM0	1
 #define R7S72100_CLK_OSTM1	0
 
+/* MSTP6 */
+#define R7S72100_CLK_RTC	0
+
 /* MSTP7 */
 #define R7S72100_CLK_ETHER	4