| Message ID | 20170316175112.27913-4-chris.brandt@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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 > >
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
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(+)